hil: make hil_report.md a rendering of hil_report.json - #3840
Merged
Conversation
hathach
added a commit
that referenced
this pull request
Aug 21, 2026
…3840 Three findings the PR deliberately did not close, each with what is already established, what remains, and why it was split out.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR centralizes HIL reporting around hil_report.json, rendering Markdown from the canonical sidecar while adding accumulation, failure metadata, and unified verdict generation.
Changes:
- Adds shared report rendering, normalization, merging, and summarization.
- Updates HIL orchestration, CI sidecar handling, and workflow caveat contracts.
- Moves the summary CLI and updates tests, documentation, and operator guidance.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Reviewed change and final comments |
|---|---|
test/hil/test/test_hil_report.py |
Adds report behavior and regression tests. |
test/hil/test/test_hil_health.py |
Updates health-related test coverage. |
test/hil/test/test_hil_bounded.py |
Updates moved report tests and imports. |
test/hil/hil_test.py |
Uses centralized report APIs. Critical (4 votes): Pass the run’s accumulate state to mark_report_no_boards() so accumulated rows are preserved. |
test/hil/hil_ci.sh |
Stages and transfers the JSON merge base. Critical (3 votes): Fetch Markdown and JSON together and commit or discard them as a pair to preserve the rendering invariant. |
test/hil/helper/hil_summary.py |
Removes the superseded summary module. |
test/hil/helper/hil_report.py |
New report owner, renderer, merger, and CLI. Critical (1 vote): Preserve abandonment caveats only for non-fresh accumulation. Critical (1 vote): Ensure recovery sidecars include rows represented by preserved Markdown. Moderate (2 votes): Coerce non-list rows during normalization. Moderate (4 votes): Use normalized data in the CLI. Moderate (3 votes): Safely normalize or render unhashable cell values. |
test/hil/helper/hil_health.py |
Retains process-cleanup functionality. |
docs/superpowers/specs/2026-08-21-hil-report-module-design.md |
Documents the module design. Nit (4 votes): Update the contract/non-goal section to describe the implemented caveat interface. |
docs/superpowers/plans/2026-08-21-hil-report-module.md |
Documents the implementation plan. |
docs/superpowers/followup/pr3836-report-single-source.md |
Removes the completed follow-up. |
docs/superpowers/followup/pr3803-hil-blindness-reporting.md |
Updates report references. |
.claude/workflows/test-hil-validate.mjs |
Tests caveat-aware verdict handling. |
.claude/workflows/hil-validate.js |
Adds caveat-aware workflow verdicts. Critical (4 votes): Carry the latest relevant retry caveat into the result. Critical (1 vote): Require caveat in the response schema. |
.claude/agents/hil-operator.md |
Updates the operator CLI and contract documentation. |
Suppressed comments (9)
docs/superpowers/plans/2026-08-21-hil-report-module.md:658
- The plan's out-of-scope list still says the workflow will not carry
caveat, while this PR changes the operator schema/return payload and makes that field gate the workflow verdict. This committed plan is now inconsistent with the implementation; update the stated scope and validation contract.
## Out of scope
Each its own follow-up, unchanged from the spec:
- Splitting `accumulate_report`'s `mret` folding from its merge.
- The flat `HIL_POOL_TIMEOUT` that does not scale with board count.
- Carrying `caveat` through the operator/workflow return contract (`hil-validate.js:34`).
test/hil/helper/hil_report.py:253
- If a fresh run fails before
accumulate_report()creates either artifact (for example,Manager()/Pool()setup fails after the fresh wipe),_load()returns unreadable and_stamp_markdown()immediately returns becausehil_report.mdis absent._abandon_exitthen leaves no JSON caveat for the CLI/agent to consume, recreating the missing-report failure this change is meant to remove. Create a caveat-only document when both artifacts are missing, while retaining the markdown fallback for an unrecoverable sidecar beside an existing table.
# Unreadable sidecar, or the document write failed. Either way the markdown is what
# the PR comment reads, so stamp it directly rather than giving up.
try:
_stamp_markdown(report_dir, notice)
test/hil/helper/hil_report.py:6
- The invariant stated here is not true for all current artifact writers:
.github/workflows/build.ymlstill directlyechoshil_report.mdfor no-affected-board legs (for example lines 473 and 559) and creates no JSON sidecar. Either route those skip artifacts through the same document renderer or explicitly scope this claim to the HIL Python writers; otherwise the advertised JSON/Markdown invariant still has exceptions.
The markdown IS a rendering of the sidecar -- every writer goes through render_report(), so
a table can never contain something the JSON does not. This module owns the whole life of
test/hil/helper/hil_report.py:12
- This dual-mode explanation says a guarded
hil_healthimport is below, but the module defines its own_pand has no such import. The stale description can send future maintainers looking for an import-cycle workaround that no longer exists; document the local_pimplementation instead.
Dual-mode by design: imported as `helper.hil_report` by hil_test.py, and run as a script by
the operator (see .claude/agents/hil-operator.md). A script run puts test/hil/helper on
sys.path rather than test/hil, hence the guarded hil_health import below.
test/hil/hil_test.py:2296
- The timeout/worker-abort report is passed
scope=''even when the invocation was filtered with-bor-bt. A scoped run that dies on the pool guard consequently loses the scope field and is again indistinguishable from a full run that lost boards. Compute the invocation scope before entering the pool and pass it on this path (and to its fallback).
hil_report.accumulate_report(mret, report_dir, fresh, '',
health_banner + _blind_note(mret)
+ _stray_note(mret), caveat=banner)
test/hil/hil_test.py:2335
- The worker-abort path has the same scope loss as the timeout path: it hard-codes an empty scope in the document even for a board-filtered invocation. This makes the new scope field absent precisely on another failure path where it is needed; pass the current invocation scope instead.
hil_report.accumulate_report(mret, report_dir, fresh, '',
health_banner + _blind_note(mret)
+ _stray_note(mret), caveat=banner)
test/hil/test/test_hil_report.py:850
- This test only scans the entire shell source for a line containing both
warningandaccumulate; the surrounding comments already satisfy that predicate. Removing the runtime warning would therefore leave the test green. Exercise the accumulate/no-merge-base branch or assert the specific executable warning command instead of searching all source lines.
warn = [ln for ln in (Path(HIL_DIR) / 'hil_ci.sh').read_text().splitlines()
if 'warning' in ln.lower() and 'accumulate' in ln.lower()]
self.assertTrue(warn, 'no warning when --accumulate has no local sidecar')
test/hil/test/test_hil_report.py:1015
- This test does not exercise the render-failure ordering it claims to pin:
_load()converts the seeded numeric duration toNone, soaccumulate_report()renders successfully and the assertion only checks that both files contain the wordabandoned. A regression that writes JSON and then fails while rendering would still pass this test. Forcerender_report()to fail (or otherwise inject a renderer failure) and assert the committed artifacts remain consistent.
(rd / hil_report.REPORT_JSON).write_text(json.dumps(
{'rows': [{'board': 'boardA', 'cells': {'t': 'pass'}, 'duration': 119.0}],
'banner': '', 'caveat': '', 'scope': ''}))
hil_report.accumulate_report([('boardB', 0, 0, [('boardB', {'t': 'OK'}, '1s')], 0)],
rd, False, '', '')
doc = json.loads((rd / hil_report.REPORT_JSON).read_text())
self.assertEqual((rd / hil_report.REPORT_MD).read_text(),
hil_report.render_report(doc) + '\n')
test/hil/test/test_hil_report.py:635
- The test name and docstring claim to exercise a renderer failure, but the bare
'boardA'row is discarded by_load()beforerender_report()runs, so this path never raises and the ordering invariant is not tested. Make the renderer actually fail (for example by patching it) and assert the pre-existing JSON/markdown remain consistent after the failure.
(rd / hil_report.REPORT_JSON).write_text(json.dumps(
{'rows': ['boardA'], 'banner': '', 'scope': '', 'caveat': ''}))
hil_report.mark_report_abandoned(rd, 'the worker pool would not shut down.')
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| }) | ||
|
|
||
| const { pass, wedged, locked } = summarize(results, args.force) | ||
| const { pass, wedged, locked } = summarize(results, args.force, first?.caveat || '') |
Comment on lines
+35
to
+38
| // the run-level caveat: abandoned / aborted / selected-no-boards. `banner` carries rig | ||
| // HEALTH across an --accumulate retry; `caveat` carries how THIS run ended, and every | ||
| // row can still say pass while it failed — so it gates `pass` in summarize() below. | ||
| caveat: { type: 'string' }, |
Comment on lines
+52
to
+55
| **Non-goals (explicit follow-ups, not this change):** | ||
| - Splitting `accumulate_report`'s `mret` folding from its merge (see "Deliberate wart"). | ||
| - The flat `HIL_POOL_TIMEOUT` that does not scale with board count (`hil_test.py:225`). | ||
| - Carrying `caveat` through the operator/workflow return contract (`hil-validate.js:34`'s schema). |
Comment on lines
+267
to
+270
| if _already_abandoned(doc): | ||
| # SKILL.md gives the two notices OPPOSITE rules, and an abandon outranks a | ||
| # filter that matched nothing -- do not overwrite the record of a failed run. | ||
| return |
Comment on lines
+69
to
+70
| for r in raw.get('rows') or []: | ||
| if not isinstance(r, dict) or 'board' not in r: |
| print(f'error: {jpath} not found -- did hil_test.py run in this directory?', | ||
| file=sys.stderr) | ||
| return 1 | ||
| json.dump(summarize(cfg, boards, json.loads(jpath.read_text())), sys.stdout, indent=2) |
Comment on lines
+75
to
+76
| 'cells': {str(k): v for k, v in cells.items()} | ||
| if isinstance(cells, dict) else {}, |
Comment on lines
+362
to
+366
| body = prior if prior.strip() else render_matrix( | ||
| [(r['board'], r['cells'], r['duration']) for r in doc['rows']]) | ||
| report_dir.mkdir(parents=True, exist_ok=True) | ||
| (report_dir / REPORT_JSON).write_text(json.dumps(doc, indent=2) + '\n') | ||
| (report_dir / REPORT_MD).write_text(head + '\n' + body, encoding='utf-8') |
Comment on lines
+409
to
+411
| if scp -q "$REMOTE:$REMOTE_DIR/$extra" "$ROOT_DIR/$extra.tmp" 2>/dev/null \ | ||
| && [ -f "$ROOT_DIR/$extra.tmp" ]; then | ||
| mv -f "$ROOT_DIR/$extra.tmp" "$ROOT_DIR/$extra" |
| except OSError: | ||
| pass | ||
| rd = Path(os.environ.get('HIL_REPORT_DIR', '.')) | ||
| hil_report.mark_report_no_boards(rd, msg) |
|
Code-size comparison skipped: PR selection built no families on this push. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @@ -0,0 +1,63 @@ | |||
| # Two loose ends in the report document | |||
Contributor
There was a problem hiding this comment.
CLAUDE.md adherence — this follow-up doc bundles two unrelated deferred items into one file. The root CLAUDE.md "Deferred work" rule requires:
…one doc per follow-up, in
docs/superpowers/followup/pr<NNN>-<topic>.md… Never bundle unrelated follow-ups into one file.
The two items here are independent — different files, different root causes, no shared fix:
- Item 1 (L6) — a documentation fix to
.claude/skills/hil/SKILL.md(no-boards behaviour drift). - Item 2 (L36) — a durability fix to
write_report's two non-atomic writes inhil_report.py.
The doc itself frames them as two ("Two loose ends", L1; "These two are a documentation drift and a durability edge", L60-62), and the PR description lists them as separate bullets each deserving "its own PR." The sibling pr3840-mret-board-result.md correctly keeps to a single topic.
Consider splitting into two docs (e.g. pr3840-skill-md-no-boards-drift.md and pr3840-write-report-atomicity.md) so each can land — and be deleted — independently when its own PR merges.
Hardware-in-the-loop (HIL) Test Reporthfp-iar✅ 54 passed · ❌ 2 failed · ⚪ 0 skipped · blank not run
hfp.json✅ 56 passed · ❌ 0 failed · ⚪ 0 skipped · blank not run
tinyusb-esp.json✅ 20 passed · ❌ 4 failed · ⚪ 0 skipped · blank not run
tinyusb.json
✅ 326 passed · ❌ 55 failed · ⚪ 17 skipped · blank not run
|
|
hathach
added a commit
that referenced
this pull request
Aug 21, 2026
…3840 Three findings the PR deliberately did not close, each with what is already established, what remains, and why it was split out.
hathach
force-pushed
the
hil-report
branch
3 times, most recently
from
August 21, 2026 15:26
19cf92e to
aa4a078
Compare
Contributor
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
hathach
added a commit
that referenced
this pull request
Aug 25, 2026
.build.families is read twice in the build-extras block: as a `|`-joined regex for the metrics artifact pattern, and implicitly as "is anything selected". An EMPTY list and one REJECTED by the charset guard both leave the regex empty and mean opposite things, but the branch tested only `-z "$FAMILY_REGEX"` - so every nothing-selected PR took the fall-open path and DISCARDED the correct all-empty matrix ci_set_matrix had just produced. Both merged PRs show it: #3842 (a skill, README.rst and .gitignore) and #3840 (test/hil/** plus docs) each rebuilt all 74 cmake legs after the selector had correctly chosen no families. The HIL axis was right in both - rig jobs skipped on #3842, full rig on #3840 per rule 2 - because it never passes through this block, and no UNSCOPED marker appeared because ci_set_matrix behaved perfectly; the override happens in the shell afterwards. That combination made it look like the classifier was at fault when it was not. Now branches on which case it is: a rejected list falls open exactly as before (all three values drop together), while an empty list keeps the all-empty matrix so every leg skips, sets build_filtered=false because nothing was built and there is nothing to compare a baseline against, and leaves EXAMPLE_MAP at '{}'. Renames FAM_* to FAMILY_* - FAM meant hw/bsp board family, and the abbreviation read like it might be something else in a file that also says "family" for the per-family example map and the get_deps family tokens. The block had no test at all, which is how this shipped twice. test_ci_metrics now extracts it from build.yml and runs it for real against all three shapes; the empty case fails with `129 != 0` against master, and the scoped case with `'' != 'stm32f4|rp2040'`. The probe quotes its shell assignments with shlex.quote, so a TMPDIR containing a space cannot fail it for an unrelated reason.
hathach
added a commit
that referenced
this pull request
Aug 25, 2026
…3845) ci: an empty selection must build nothing, plus selector follow-ups A PR whose build axis legitimately selected nothing rebuilt everything. build.yml reads .build.families twice - as a |-joined regex, and implicitly as "is anything selected" - but tested only -z "$FAMILY_REGEX", which an empty list and a charset-rejected one both satisfy while meaning opposite things. ci_set_matrix had already returned the correct all-empty matrix; the fall-open branch discarded it. #3842 and #3840 each spent 74 cmake legs on it. Branch on the two cases instead, rename FAM_* to FAMILY_*, and cover the block with a test that extracts it from build.yml and executes it - it had no test at all, which is how this shipped through two merges. Follow-ups to the same machinery: glob.escape the repo root at five sites, so a checkout path containing [ or * stops failing closed; drop the ci-full label, read after the matrix was already computed and so never functional; delete 13 mcu:MKL25ZXX / mcu:SAME5X skip tokens matching no board; carry the rule table in the module docstring, guarded against drift; and pin six selection behaviours a mutation pass proved untested. Cut the selector's cost 1.8x (26.0s -> 14.6s) with 0 divergences over 260 paths, and stop scoping the membrowse upload by the PR example filter.
test_the_child_is_reaped_even_when_work_raises asserted that no 'sleep 20' survives, but pgrep -f searches every process on the host -- another agent session's retry loop was sleeping 20s between attempts, so the test failed 3/3 in isolation while that ran. It is a pre-commit hook, so it blocked every commit. A distinctive duration scopes it back to our own child. Unrelated to the report work.
The markdown IS a rendering of the sidecar now: every writer goes through render_report(), so a table can never contain something the JSON does not. Four writers previously composed it independently and three wrote no JSON at all -- and those three are exactly the paths where the run died, so a JSON consumer saw nothing on the runs that mattered most. The per-board verdicts an agent hands back reported the whole fleet as "no report row" while a human reading the markdown saw the real story. The document gains the two fields the markdown carried but the JSON did not: `scope` (a three-board PR run and a full run that lost 24 boards were indistinguishable) and `caveat` (how the run ENDED -- abandoned, aborted, no boards). `banner` keeps its existing meaning: the rig-health conditions the cells were collected under. The distinction is load-bearing, because banner carries across an --accumulate retry and caveat must not; conflating them made a clean retry publish an abandonment that never happened, and let a stale notice from an earlier attempt silence a genuinely new one. Consolidating into helper/hil_report.py is what makes that complete. The renderer, the writers, the merge and the fold to per-board verdicts live in one module that hil_test.py and hil_health.py both import. That dissolves the import cycle which forced write_timeout_report to compose its own markdown -- the pool-guard fallback renders like everyone else, so all five writers are byte-identical -- and removes the second copy of the cell classifier, which hil_summary.py's docstring described as "the EXACT classifier hil_test.py's own tally uses". Two copies of one rule, kept in sync by hand against re-typed emoji literals: change REPORT_CELL and the human's table and the agent's verdict silently disagree. hil_summary.py is deleted; its CLI moves here and the two harness docs that invoke it by name follow. `caveat` gates the workflow verdict and is required by its schema, because on the abandon path every row can legitimately pass while hil_test.py exits non-zero, and an operator omitting the field would silently disable the gate. NOT purely code motion, and worth reading as new code: measured against master, hil_test.py held only render_matrix and accumulate_report. render_report, write_report, mark_report_abandoned, mark_report_no_boards, _load, cell_state and the scope/caveat plumbing are new, and three rounds of review found their defects there. Each was reproduced before being fixed and is pinned by a test: a stuck board that already had a row got no pool-timeout cell and summarized as a pass; a stale board-locked cell masked it, so a board that wedged the rig was published as LOCKED and re-run; mark_report_abandoned republished the markdown even when it declined to stamp, inspected the wrong field, gave up on a missing or torn sidecar, and called the table "partial" against SKILL.md's contract that it IS this run's; write_report swallowed OSError, making two layers of fallback dead code, and committed the JSON before rendering; a sidecar with a null banner or a non-list rows killed a fully successful run with no artifact at all; an unhashable cell value raised on the normal accumulate path; the no-boards exit republished a previous run's rows, and its guard blocked even a fresh run. _load is the trust boundary for all of it, since hil_ci.sh uploads a sidecar as the --accumulate merge base and it is therefore untrusted input. A corrupt cell drops rather than being coerced to str, which would classify it as a pass. Verified on the rig: a 25-board fleet run, ten randomized passes mixing fresh and --accumulate over different board sets, and every containment path exercised against the module actually staged there.
hil_ci.sh rm -rf's REMOTE_DIR at the start of every run, and accumulate_report merges onto the sidecar in the run's cwd -- so a remote retry found nothing, silently started fresh, and its one-row table REPLACED the full-fleet one it was meant to extend. The copy-back at the end of the script has always existed; this is the other half of it. Verified on the rig end to end, which is the only place this shows up: a fresh run of stm32f407disco, then a retry spelled `-av`, and the merged sidecar holds all three rows where it previously held only nanoch32v203's two. Gated on --accumulate rather than unconditional: a fresh run unlinks the sidecar anyway. argparse decides whether the flag is present rather than a case arm -- hil_test.py declares `-a, --accumulate`, so `-av`, `-va`, `--accum` and `--acc` all mean it, and hil-validate.js tells operators to retry "adding -v". A missing merge base warns loudly instead of failing silently, since the run succeeds either way and a mistyped retry quietly shrinking the published table is the actual damage. The upload also checks provenance: hil_report.json is not namespaced by CONFIG or REMOTE, so a run against another rig leaves a sidecar behind that would otherwise merge in, publishing boards that never ran here. The copy-back fetches both halves to temps and commits them as a pair, or neither. It used to delete both before scps that are allowed to fail silently, so an ssh drop at the end of a 60-minute run destroyed the report and the next retry's merge base; copying them independently was no better, since a markdown that arrived beside a sidecar that did not left the local pair failing the rendering invariant. <config>.failed is not uploaded: hil_test.py only ever writes it, never reads it. The retry spec already reaches the rig as the -b/-bt arguments the caller expanded from it.
Records why the report code was spread across three modules and what the consolidation buys, so the next reader does not have to re-derive it from the diff. Names the new functions explicitly rather than presenting the change as pure code motion -- that framing points reviewers away from the code that carried the defects. Three findings this PR deliberately does not close get one handoff each, per CLAUDE.md: the worker-result tuple hil_report still unpacks positionally, SKILL.md's no-boards rule drifting from the code, and write_report's two non-atomic writes. Drops the pr3836 handoff, which this branch implements.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
hil_report.jsonandhil_report.mdwere written independently. Four writers produced the markdown and three wrote no JSON at all — and those three are the paths where a run died, so a JSON consumer saw nothing exactly when it mattered.hil_summary.py, which builds the per-board verdicts an agent hands back, reported the whole fleet as "no report row" while a human read the real story from the markdown.The markdown is now a rendering of the sidecar: every writer goes through
render_report(), so a table can never contain something the JSON does not.What changed
Two new fields.
scope— a three-board PR run and a full run that lost 24 boards were indistinguishable.caveat— how the run ended, which the abandon path used to express by prepending text to a file it had not written.bannervscaveatis load-bearing.bannercarries rig health across an--accumulateretry;caveatrecords how a run ended and must not. Conflating them made a clean retry publish an abandonment that never happened.One module owns the document.
helper/hil_report.pyholds the vocabulary, renderer, writers, merge and per-board fold — dissolving the import cycle that forcedwrite_timeout_reportto compose its own markdown, and removing a duplicate cell classifier kept in sync by hand.hil_summary.pyis deleted; its CLI moves here.hil_ci.shuploads the sidecar so a remote--accumulatehas a merge base; it wipesREMOTE_DIRevery run, so a retry previously started fresh and its one-row table replaced the full-fleet one.Not pure code motion
Against
master,hil_test.pyheld onlyrender_matrixandaccumulate_report.render_report,write_report,mark_report_abandoned,mark_report_no_boards,_loadandcell_stateare new — where three review rounds found the defects. Each was reproduced before being fixed and is pinned by a test: a board that ate the whole pool guard summarized as a pass; a staleboard-lockedcell masked a wedge, so a board that hung the rig was published as LOCKED and re-run.Verification
500 unit tests,
pre-commitclean, three/code-review maxrounds. On the rig: a 25-board fleet run, then 10 randomized passes throughhil_ci.shmixing fresh and--accumulate, checking after each that the markdown re-renders from the sidecar byte for byte. Zero problems. Espressif is covered by CI's esp jobs.