diff options
19 files changed, 2880 insertions, 1013 deletions
diff --git a/.claude/agents/hil-operator.md b/.claude/agents/hil-operator.md index a37501211..81fdc08e9 100644 --- a/.claude/agents/hil-operator.md +++ b/.claude/agents/hil-operator.md @@ -68,12 +68,12 @@ For a board run, do NOT transcribe the report table. Run the tests, then hand ba output verbatim: ```bash -python3 test/hil/helper/hil_summary.py <config> -b BOARD [-b BOARD...] # from the report dir +python3 test/hil/helper/hil_report.py <config> -b BOARD [-b BOARD...] # from the report dir ``` -`{"results": <its results array, verbatim>, "banner": <its banner, verbatim>, "wedged": ["board", ...]}` +`{"results": <its results array, verbatim>, "banner": <its banner, verbatim>, "caveat": <its caveat, verbatim>, "wedged": ["board", ...]}` -`results` and `banner` are copied, never retyped, reworded or re-ordered: report rows are named +`results`, `banner` and `caveat` are copied, never retyped, reworded or re-ordered (`caveat` is the run-level notice — abandoned, aborted, no-boards — and it can say the run failed while every row says pass): report rows are named per variant, a variant name need not start with the board name, and lock contention is a cell rather than a phrase, so re-deriving any of it by hand is how this contract broke before. `wedged` is yours — the boards your run left unresponsive, usually none — and the only field you diff --git a/.claude/workflows/hil-validate.js b/.claude/workflows/hil-validate.js index bc0bda8b9..741ba481d 100644 --- a/.claude/workflows/hil-validate.js +++ b/.claude/workflows/hil-validate.js @@ -11,10 +11,10 @@ if (!args || !Array.isArray(args.boards) || args.boards.length === 0) { throw new Error('args must be { boards: string[], force? } with the boards already built') } -// The operator returns hil_summary.py's JSON verbatim plus its own observations. It does NOT +// The operator returns hil_report.py's JSON verbatim plus its own observations. It does NOT // retype the report table: rows are named per variant, a variant need not start with the board // name, and lock contention is a cell rather than a phrase — rebuilding board identity from -// prose produced a defect in each of four review rounds. hil_summary.py does that join against +// prose produced a defect in each of four review rounds. hil_report.py does that join against // the roster, so `locked` and `ran` arrive as fields and nothing here parses a detail string. const BOARD = { type: 'object', additionalProperties: false, @@ -26,12 +26,16 @@ const BOARD = { } const HIL = { type: 'object', additionalProperties: false, - required: ['results', 'wedged'], + required: ['results', 'wedged', 'caveat'], properties: { results: { type: 'array', items: BOARD }, // the operator's own observation — not derivable from the report wedged: { type: 'array', items: { type: 'string' } }, banner: { type: 'string' }, + // 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' }, }, } @@ -51,12 +55,12 @@ const runBoards = (boards, isRetry = false) => agent( (args.force ? 'THE USER HAS EXPLICITLY AUTHORIZED FORCING: run hil_test.py with HIL_NO_BOARD_LOCK=1 in the environment (bypasses the board lock check; do NOT release or kill the existing holder). ' : 'A board whose lock is held (a dev session or concurrent CI job) fails fast inside the run without blocking the others — never force the lock. ') + - 'If hil_test.py refuses the run with "board(s) not in <config>", re-run it WITHOUT the unknown names but keep the FULL board list on the hil_summary call below — it emits a ran:false entry for every board you name, so the unknown ones surface as "no report row" instead of costing the whole batch. ' + + 'If hil_test.py refuses the run with "board(s) not in <config>", re-run it WITHOUT the unknown names but keep the FULL board list on the hil_report call below — it emits a ran:false entry for every board you name, so the unknown ones surface as "no report row" instead of costing the whole batch. ' + 'Use the config for this host (hostname first). Run hil_test.py as a BACKGROUND Bash task and wait for it (a stuck fleet runs to its pool guard, 60 min by default — beyond any foreground timeout); never cancel it early. ' + 'On non-lock failures retry ONCE from the re-run spec hil_test.py just wrote — `<config>.failed`, which already begins with --accumulate — adding -v. A usbtest battery that produced per-case verdicts is NOT auto-retried, so its result already stands. ' + 'THEN, from the directory the run wrote its report to, produce the results with:\n' + - ` python3 test/hil/helper/hil_summary.py <the config you used> ${boards.map((b) => `-b ${b}`).join(' ')}\n` + - 'Return its `results` array and `banner` EXACTLY as printed — do not retype, reword, re-order or "correct" them, and never transcribe the markdown table instead. ' + + ` python3 test/hil/helper/hil_report.py <the config you used> ${boards.map((b) => `-b ${b}`).join(' ')}\n` + + 'Return its `results` array, `banner` and `caveat` EXACTLY as printed — do not retype, reword, re-order or "correct" them, and never transcribe the markdown table instead. ' + 'Add `wedged`: the board names whose board or fixture your run left unresponsive (usually none). That is your own observation and the one field you author; put `dmesg | tail -50` in your reply text for any board you list.', { label: boards.length === 1 ? `hil:${boards[0]}` : `hil:${boards.length} boards`, @@ -64,7 +68,7 @@ const runBoards = (boards, isRetry = false) => agent( }, ) -// A lookup, not a reconciliation: hil_summary.py emits exactly one entry per requested board, +// A lookup, not a reconciliation: hil_report.py emits exactly one entry per requested board, // so a missing entry means the operator dropped it rather than that the names disagree. const byBoard = (out) => new Map((out?.results || []) .filter((r) => r && typeof r.board === 'string') @@ -91,7 +95,9 @@ const results = args.boards.map((b) => { }) for (const r of results) log(`${r.board}: ${r.pass ? 'PASS' : r.locked ? 'LOCKED' : 'FAIL'}`) if (first?.banner) log(`report banner: ${first.banner.trim().split('\n')[0]}`) +if (first?.caveat) log(`report caveat: ${first.caveat.trim().split('\n')[0]}`) +let runCaveat = first?.caveat || '' // A concurrent CI job may have held some boards (its hil_test.py flock). // CI finishes a board in minutes — retry locked boards once, at the end. if (!args.force) { @@ -116,19 +122,26 @@ if (!args.force) { } log(`${b}: retry ${results[i].pass ? 'PASS' : 'FAIL'}`) } + // the retry's own run-level verdict, not the first attempt's: a retry that abandoned + // or aborted must sink the run even though its rows may all say pass. + if (again?.caveat) runCaveat = again.caveat } } -// pass/wedged/locked in one place so it can be exercised without running an agent -const summarize = (rs, force) => ({ - pass: rs.every((r) => r.pass), +// pass/wedged/locked in one place so it can be exercised without running an agent. +// `caveat` is a RUN-level verdict and must gate `pass`: on the abandon and no-boards +// paths every row can legitimately say pass while the run itself failed (hil_test.py +// os._exit(1) -- a red job), so per-row agreement alone published those runs as green. +const summarize = (rs, force, caveat) => ({ + pass: rs.every((r) => r.pass) && !/^\*\*HIL run (abandoned|aborted|selected no boards)/m + .test(caveat || ''), wedged: rs.filter((r) => r.wedged).map((r) => r.board), locked: force ? [] : rs.filter((r) => !r.pass && r.locked).map((r) => r.board), }) -const { pass, wedged, locked } = summarize(results, args.force) +const { pass, wedged, locked } = summarize(results, args.force, runCaveat) if (wedged.length) log(`WEDGED boards needing usb-kernel-recover: ${wedged.join(', ')}`) // Workers cannot prompt the user — surface still-locked boards for the main // session to ask: force (re-invoke with force: true), wait, or accept. if (locked.length) log(`still locked after retry: ${locked.join(', ')} — ask the user: force / keep waiting / accept`) -return { pass, results, wedged, locked } +return { pass, results, wedged, locked, caveat: runCaveat } diff --git a/.claude/workflows/test-hil-validate.mjs b/.claude/workflows/test-hil-validate.mjs index db73095f6..e8be57cfc 100644 --- a/.claude/workflows/test-hil-validate.mjs +++ b/.claude/workflows/test-hil-validate.mjs @@ -4,7 +4,7 @@ // `board locked` out of a prose detail, folding rows, keeping a wedged flag alive -- produced // a defect in each of four review rounds, including a test that asserted an invariant using // the one input shape that could not break it. That logic now lives in -// test/hil/helper/hil_summary.py, where the roster is, and arrives here as fields. What is +// test/hil/helper/hil_report.py, where the roster is, and arrives here as fields. What is // left is a lookup and a verdict, and this pins both. // // Run: node .claude/workflows/test-hil-validate.mjs @@ -23,6 +23,11 @@ const cut = (start, end) => { } const body = cut('const byBoard =', 'const first = await runBoards') + cut('const summarize =', 'const { pass, wedged, locked } =') +// more than one schema declares `required:`; pick the HIL one by its contents +const HIL_REQUIRED = (src.match(/required: \[[^\]]*\]/g) || []) + .map((m) => m.replace('required: ', '').replace(/'/g, '"')) + .map((m) => JSON.parse(m)) + .find((a) => a.includes('wedged')) || [] const { byBoard, summarize, wedgedFor } = new Function(`${body}; return { byBoard, summarize, wedgedFor }`)() let failed = 0 @@ -65,5 +70,27 @@ check('wedged surfaces', summarize([R('a', false, false, true)], false).wedged, check('a wedged board that passed still surfaces', summarize([R('a', true, false, true)], false).wedged, ['a']) +// A run-level caveat outranks per-row agreement: on the abandon and no-boards paths every +// row can legitimately pass while hil_test.py exits non-zero. Row agreement alone published +// those runs green. +check('all rows pass and no caveat is a pass', + summarize([R('a', true), R('b', true)], false, '').pass, true) +check('an abandoned run is not a pass', + summarize([R('a', true)], false, + '**HIL run abandoned: the worker pool would not shut down.** x').pass, false) +check('an aborted run is not a pass', + summarize([R('a', true)], false, '**HIL run aborted: a worker raised RuntimeError**').pass, + false) +check('a no-boards run is not a pass', + summarize([R('a', true)], false, '**HIL run selected no boards.** filters emptied').pass, + false) +check('a rig-health note is NOT a caveat and does not fail the run', + summarize([R('a', true)], false, '> **Rig note.** 2 process(es) in D state').pass, true) +check('a retry that abandoned sinks the run even with all rows passing', + summarize([R('a', true)], false, + '**HIL run abandoned: the worker pool would not shut down.** retry').pass, false) +check('an omitted caveat cannot silently disable the gate (schema requires it)', + HIL_REQUIRED.includes('caveat'), true) + console.log(failed ? `\n${failed} FAILED` : '\nall checks passed') process.exit(failed ? 1 : 0) diff --git a/docs/superpowers/followup/pr3803-hil-blindness-reporting.md b/docs/superpowers/followup/pr3803-hil-blindness-reporting.md index 69ff939b0..374ee62c7 100644 --- a/docs/superpowers/followup/pr3803-hil-blindness-reporting.md +++ b/docs/superpowers/followup/pr3803-hil-blindness-reporting.md @@ -158,7 +158,8 @@ def _board_result_on_error(name, exc): """A row for a board that died by exception. err_count 1, no per-test detail, but the blindness and stray fields are still accurate -- they explain the failure more often than the exception text does.""" - rows = [(name, {BOUNDARY_CELL: f'{REPORT_CELL["fail"]} {type(exc).__name__}'}, None)] + rows = [(name, {hil_report.BOUNDARY_CELL: + f'{hil_report.REPORT_CELL["fail"]} {type(exc).__name__}'}, None)] return _board_result(name, 1, [], rows, 0.0, True) ``` diff --git a/docs/superpowers/followup/pr3836-report-single-source.md b/docs/superpowers/followup/pr3836-report-single-source.md deleted file mode 100644 index f4ccc77c2..000000000 --- a/docs/superpowers/followup/pr3836-report-single-source.md +++ /dev/null @@ -1,470 +0,0 @@ -# One Source of Truth for the HIL Report Implementation Plan - -> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. - -**Goal:** Make `hil_report.md` a rendering of `hil_report.json` rather than a second, independently written artifact, so no run can produce a table whose contents are not in the JSON. - -**Architecture:** `hil_report.json` gains the two fields the markdown carries but the JSON does not (`scope`, and a `caveat` for text prepended after the fact). A single `render_report(doc) -> str` turns that document into the markdown, and every writer — the normal path, the pool-guard fallback, the no-boards exit, and `_abandon_exit` — goes through `write_report(report_dir, doc)`, which writes both files from the same dict. `_abandon_exit` stops doing a text-prepend on a file it did not write and instead sets `doc['caveat']`. - -**Tech Stack:** Python 3.13 stdlib only (`json`, `pathlib`); existing unit suites under `test/hil/test/` run with plain `unittest`. - -**Spec:** none — this is a follow-up split out of the `claude/hil-doc-audit` branch. The evidence it argues from is inline below. - -**Origin:** split out of PR #3836 (the HIL one-run rework + `.claude` instruction audit). Delete this file when its own PR lands. - -## Global Constraints - -- **No behaviour change to the containment paths' ordering or exit codes.** `_abandon_exit` runs while the interpreter is being torn down; its own comments record that anything raising between the pool's `finally` and `os._exit` hangs the process in multiprocessing's unbounded `join()` (reproduced at rc=124/25s with SIGTERM-ignoring workers). Serialisation added there must stay inside the existing `try`/`except` and must never raise past it. -- **The markdown stays the human artifact.** `.github/workflows/build.yml:487` uploads `hil_report.md`, `test/hil/hil_ci.sh:293` copies only it back, and `.claude/skills/hil/SKILL.md` tells the operator to paste that table verbatim. It becomes generated output, not a dropped file. -- **Banner outranks the scope note outranks the table.** Preserve the existing order (`hil_test.py:2153-2161`): the caveat is outermost because that is where `hil/SKILL.md` tells an agent to look. -- **`--accumulate` merges from the JSON** (`hil_test.py:2102-2115`), including carrying the prior banner forward. Adding fields must not break that merge for a sidecar written by an older version. -- Run `python3 -m unittest discover -s test/hil/test` (115 tests, ~78 s) before each commit; `pre-commit run --files <changed>` before pushing. - -## Why this is worth doing - -Four writers produce `hil_report.md`, and three of them write no JSON at all: - -| Writer | JSON? | Line | -|---|---|---| -| `accumulate_report` — the normal path | yes | `hil_test.py:2149`, `:2162` | -| `**HIL run selected no boards.**` | **no** | `hil_test.py:2317` | -| pool-guard fallback → `hil_health.write_timeout_report(...)` | **no** | `hil_test.py:2469`, `hil_health.py:346` | -| `_abandon_exit` — prepends to whatever `.md` exists | **no** | `hil_test.py:2603` | - -Those three are exactly the paths where the run died, so they are the cases where the artifact matters most and where a JSON consumer sees nothing. `test/hil/helper/hil_summary.py` (added on the origin branch) reads the JSON to build the per-board verdicts an agent hands back — on any of those three paths it finds no file and reports "no report row for this board" for the whole fleet, while a human reading the markdown sees the real story. - -Separately, `scope` exists only in the markdown (`hil_test.py:2154`, from `accumulate_report`'s `scope: str = ''` parameter at `:2092`). A PR-scoped three-board table and a full-fleet run that lost 24 boards are indistinguishable in the JSON. - ---- - -### Task 1: Put `scope` in the JSON - -**Files:** -- Modify: `test/hil/hil_test.py:2092-2163` (`accumulate_report`) -- Test: `test/hil/test/test_hil_bounded.py` (new class beside `CaveatSurvivesAccumulate`) - -**Interfaces:** -- Produces: `hil_report.json` gains a top-level `"scope": str` (empty string when unscoped). Existing keys `rows` and `banner` are unchanged. - -- [ ] **Step 1: Write the failing test** - -```python -class ScopeSurvivesInTheJson(unittest.TestCase): - """A scoped run's small table is indistinguishable from a full run that lost boards. - The markdown says so; the JSON did not, so any JSON consumer could not tell.""" - - def _rows(self, board, cell): - return [(board, 0, 0, [(board, {cell: 'OK'}, '1s')], 0)] - - def test_scope_is_recorded_in_the_sidecar(self): - import json - td = TemporaryDirectory() - self.addCleanup(td.cleanup) - rd = Path(td.name) - hil_test.accumulate_report(self._rows('boardA', 'cdc_msc'), rd, True, - '-b boardA', '') - doc = json.loads((rd / 'hil_report.json').read_text()) - self.assertEqual(doc['scope'], '-b boardA') - - def test_an_unscoped_run_records_an_empty_scope(self): - import json - td = TemporaryDirectory() - self.addCleanup(td.cleanup) - rd = Path(td.name) - hil_test.accumulate_report(self._rows('boardA', 'cdc_msc'), rd, True, '', '') - self.assertEqual(json.loads((rd / 'hil_report.json').read_text())['scope'], '') -``` - -- [ ] **Step 2: Run it to verify it fails** - -Run: `python3 test/hil/test/test_hil_bounded.py ScopeSurvivesInTheJson` -Expected: FAIL — `KeyError: 'scope'` - -- [ ] **Step 3: Add the field** - -In `accumulate_report`, change the `jpath.write_text(...)` call at `hil_test.py:2149`: - -```python - jpath.write_text(json.dumps({'rows': [{'board': k, 'cells': c, 'duration': d} - for k, (c, d) in acc.items()], - 'banner': banner, - 'scope': scope}, indent=2) + '\n') -``` - -- [ ] **Step 4: Run the tests** - -Run: `python3 test/hil/test/test_hil_bounded.py ScopeSurvivesInTheJson` → PASS -Run: `python3 -m unittest discover -s test/hil/test` → 117 tests OK (the merge at `:2102` reads only `rows` and `banner`, so an older sidecar without `scope` still loads). - -- [ ] **Step 5: Commit** - -```bash -git add test/hil/hil_test.py test/hil/test/test_hil_bounded.py -git commit -m "hil_test: record the run's scope in hil_report.json - -The markdown says a scoped table is scoped; the JSON did not, so a consumer -could not tell a three-board PR run from a full run that lost 24 boards." -``` - ---- - -### Task 2: Render the markdown from the document - -**Files:** -- Modify: `test/hil/hil_test.py:1921` (`render_matrix`), `:2149-2163` (`accumulate_report`'s tail) -- Test: `test/hil/test/test_hil_bounded.py` - -**Interfaces:** -- Consumes: the `scope` key from Task 1. -- Produces: `render_report(doc: dict) -> str`, where `doc` is `{'rows': [{'board','cells','duration'}], 'banner': str, 'scope': str, 'caveat': str}`. `caveat` is optional and empty by default (Task 4 sets it). Order is caveat, banner, scope note, table. - -- [ ] **Step 1: Write the failing test** - -```python -class RenderReportIsPureFunctionOfTheDocument(unittest.TestCase): - def _doc(self, **kw): - d = {'rows': [{'board': 'boardA', 'cells': {'cdc_msc': 'pass'}, 'duration': '1s'}], - 'banner': '', 'scope': '', 'caveat': ''} - d.update(kw) - return d - - def test_table_comes_from_rows(self): - md = hil_test.render_report(self._doc()) - self.assertIn('boardA', md) - self.assertIn('cdc_msc', md) - - def test_scope_note_appears_above_the_table(self): - md = hil_test.render_report(self._doc(scope='-b boardA')) - self.assertLess(md.index('Scoped run'), md.index('boardA')) - - def test_banner_outranks_the_scope_note(self): - md = hil_test.render_report(self._doc(scope='-b boardA', - banner='> **Rig dirty.** x\n')) - self.assertLess(md.index('Rig dirty'), md.index('Scoped run')) - - def test_caveat_is_outermost(self): - md = hil_test.render_report(self._doc(banner='> **Rig dirty.** x\n', - caveat='**HIL run abandoned.**\n')) - self.assertLess(md.index('abandoned'), md.index('Rig dirty')) - - def test_a_document_with_no_rows_still_renders(self): - md = hil_test.render_report(self._doc(rows=[])) - self.assertIn('No tests were run.', md) -``` - -- [ ] **Step 2: Run it to verify it fails** - -Run: `python3 test/hil/test/test_hil_bounded.py RenderReportIsPureFunctionOfTheDocument` -Expected: FAIL — `AttributeError: module 'hil_test' has no attribute 'render_report'` - -- [ ] **Step 3: Add `render_report` and route `accumulate_report` through it** - -Add beside `render_matrix` (after `hil_test.py:1919`): - -```python -def render_report(doc: dict) -> str: - """The markdown IS a rendering of the sidecar. Every writer goes through here, so a - table can never contain something the JSON does not.""" - md = render_matrix([(r['board'], r['cells'], r.get('duration')) - for r in doc.get('rows', [])]) - if doc.get('scope'): - # a scoped run's small table is otherwise indistinguishable from a full one, and - # it replaces the previous full table in the sticky PR comment - md = f'_Scoped run: {doc["scope"]}. Boards/tests not listed were not run._\n\n' + md - # banner, then caveat: a rig-health caveat outranks the table AND the scope note, and an - # abandon notice outranks even that -- the top of the report is where hil/SKILL.md tells - # the agent to look - if doc.get('banner'): - md = doc['banner'] + '\n' + md - if doc.get('caveat'): - md = doc['caveat'] + '\n' + md - return md -``` - -Then replace `accumulate_report`'s tail (`hil_test.py:2153-2163`) with: - -```python - doc = {'rows': [{'board': k, 'cells': c, 'duration': d} for k, (c, d) in acc.items()], - 'banner': banner, 'scope': scope, 'caveat': ''} - jpath.write_text(json.dumps(doc, indent=2) + '\n') - md = render_report(doc) - (report_dir / REPORT_MD).write_text(md + '\n', encoding='utf-8') - return md -``` - -- [ ] **Step 4: Run the tests** - -Run: `python3 -m unittest discover -s test/hil/test` -Expected: 122 OK. `CaveatSurvivesAccumulate` must still pass — it asserts the banner survives a rerun, which is now the `banner` key round-tripping through the document. - -- [ ] **Step 5: Commit** - -```bash -git add test/hil/hil_test.py test/hil/test/test_hil_bounded.py -git commit -m "hil_test: render the markdown from the report document - -One function turns the sidecar into the table, so the markdown cannot carry -anything the JSON lacks. Ordering (caveat > banner > scope > table) is pinned -by tests rather than by the order of three string concatenations." -``` - ---- - -### Task 3: Give the two early-exit paths a document - -**Files:** -- Modify: `test/hil/hil_test.py:2313-2320` (no-boards exit), `test/hil/helper/hil_health.py:346` (`write_timeout_report`) -- Test: `test/hil/test/test_hil_health.py` (beside `WriteTimeoutReport`), `test/hil/test/test_hil_bounded.py` - -**Interfaces:** -- Consumes: `render_report(doc)` from Task 2. -- Produces: `write_report(report_dir: Path, doc: dict) -> None`, which writes `hil_report.json` and `hil_report.md` from one dict. Both early-exit paths call it. - -- [ ] **Step 1: Write the failing test** - -```python -class EveryExitPathLeavesBothArtifacts(unittest.TestCase): - """hil_summary.py builds an agent's verdicts from the JSON. A path that writes only - markdown reports the whole fleet as 'no report row' while a human sees the real story.""" - - def test_the_no_boards_exit_writes_json_too(self): - import json - td = TemporaryDirectory() - self.addCleanup(td.cleanup) - rd = Path(td.name) - hil_test.write_report(rd, {'rows': [], 'banner': '', 'scope': '', - 'caveat': '**HIL run selected no boards.** why\n'}) - self.assertIn('selected no boards', (rd / 'hil_report.md').read_text()) - doc = json.loads((rd / 'hil_report.json').read_text()) - self.assertEqual(doc['rows'], []) - self.assertIn('selected no boards', doc['caveat']) -``` - -and, in `test_hil_health.py`: - -```python - def test_timeout_report_writes_the_sidecar(self): - import json - td = TemporaryDirectory() - self.addCleanup(td.cleanup) - rd = Path(td.name) - hil_health.write_timeout_report(rd, [{'name': 'boardA'}], 3600, 'hil_report.md') - self.assertTrue((rd / 'hil_report.json').is_file()) - self.assertIn('boardA', (rd / 'hil_report.json').read_text()) -``` - -- [ ] **Step 2: Run them to verify they fail** - -Run: `python3 test/hil/test/test_hil_bounded.py EveryExitPathLeavesBothArtifacts` -Expected: FAIL — `AttributeError: module 'hil_test' has no attribute 'write_report'` -Run: `python3 test/hil/test/test_hil_health.py WriteTimeoutReport` -Expected: FAIL — `hil_report.json` is not a file - -- [ ] **Step 3: Add `write_report` and use it in both paths** - -Beside `render_report`: - -```python -def write_report(report_dir: Path, doc: dict) -> None: - """Write both artifacts from one document. Best-effort by design: every caller is on a - failure path where an OSError must not replace the failure being reported.""" - try: - 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(render_report(doc) + '\n', encoding='utf-8') - except OSError: - pass -``` - -Replace the no-boards block at `hil_test.py:2315-2320` with: - -```python - rd = Path(os.environ.get('HIL_REPORT_DIR', '.')) - write_report(rd, {'rows': [], 'banner': '', 'scope': '', - 'caveat': f'**HIL run selected no boards.** {msg}\n'}) -``` - -In `hil_health.write_timeout_report`, after the markdown is composed, write the sidecar next to it with a row per stuck board: - -```python - json_path = report_dir / 'hil_report.json' - json_path.write_text(json.dumps( - {'rows': [{'board': b['name'], 'cells': {'pool-timeout': 'fail'}, - 'duration': None} for b in boards], - 'banner': banner, 'scope': '', 'caveat': prefix}, indent=2) + '\n') -``` - -Keep it inside the function's existing broad `try` — a roster entry without `name` must not escape, which is what that handler exists to prevent. - -- [ ] **Step 4: Run the tests** - -Run: `python3 -m unittest discover -s test/hil/test` -Expected: 124 OK. - -- [ ] **Step 5: Commit** - -```bash -git add test/hil/hil_test.py test/hil/helper/hil_health.py test/hil/test/ -git commit -m "hil_test, hil_health: write the sidecar on the early-exit paths too - -The no-boards exit and the pool-guard fallback wrote markdown only, so a JSON -consumer saw nothing on exactly the runs that failed. hil_summary.py reported -the whole fleet as 'no report row' while the markdown told the real story." -``` - ---- - -### Task 4: Make `_abandon_exit` set a field instead of prepending text - -**Files:** -- Modify: `test/hil/hil_test.py` (`_abandon_exit`, the `if report is not None:` block near `:2622`), and its call site at `:2603` -- Test: `test/hil/test/test_hil_bounded.py` - -**Interfaces:** -- Consumes: `write_report`/`render_report` from Tasks 2–3. -- Produces: `_abandon_exit(pool, mgr, abandoned, err_count, report_dir: Path | None = None)` — the parameter becomes the **directory**, not the markdown path. - -- [ ] **Step 1: Write the failing test** - -```python -class AbandonNoticeLandsInBothArtifacts(unittest.TestCase): - def test_abandon_sets_the_caveat_not_just_the_markdown(self): - import json - td = TemporaryDirectory() - self.addCleanup(td.cleanup) - rd = Path(td.name) - hil_test.accumulate_report( - [('boardA', 0, 0, [('boardA', {'cdc_msc': 'OK'}, '1s')], 0)], rd, True, '', '') - hil_test.mark_report_abandoned(rd, 'the worker pool would not shut down.') - doc = json.loads((rd / 'hil_report.json').read_text()) - self.assertIn('abandoned', doc['caveat']) - self.assertEqual(len(doc['rows']), 1, 'the finished board must survive') - md = (rd / 'hil_report.md').read_text() - self.assertLess(md.index('abandoned'), md.index('boardA')) - - def test_marking_a_missing_report_is_a_no_op(self): - td = TemporaryDirectory() - self.addCleanup(td.cleanup) - hil_test.mark_report_abandoned(Path(td.name), 'x') # must not raise -``` - -- [ ] **Step 2: Run it to verify it fails** - -Run: `python3 test/hil/test/test_hil_bounded.py AbandonNoticeLandsInBothArtifacts` -Expected: FAIL — `AttributeError: module 'hil_test' has no attribute 'mark_report_abandoned'` - -- [ ] **Step 3: Implement it** - -```python -def mark_report_abandoned(report_dir: Path, why: str) -> None: - """Stamp an existing report as abandoned, in BOTH artifacts. - - Best-effort and silent: this runs while the interpreter is being torn down, and an - exception here hangs the process in multiprocessing's unbounded join().""" - try: - jpath = report_dir / REPORT_JSON - doc = json.loads(jpath.read_text()) if jpath.is_file() else None - if doc is None: - return - doc['caveat'] = (f'**HIL run abandoned: {why}** The table below is this run\'s ' - f'partial result.\n') - write_report(report_dir, doc) - except (OSError, ValueError, TypeError): - pass -``` - -Then in `_abandon_exit`, replace the read-modify-write of the markdown with `mark_report_abandoned(report, ...)` and change the call site at `:2603` from `report_dir / REPORT_MD` to `report_dir`. - -- [ ] **Step 4: Run the tests** - -Run: `python3 -m unittest discover -s test/hil/test` -Expected: 126 OK. - -- [ ] **Step 5: Commit** - -```bash -git add test/hil/hil_test.py test/hil/test/test_hil_bounded.py -git commit -m "hil_test: stamp abandonment into the document, not onto the markdown - -_abandon_exit did a text prepend on a file it had not written, so the caveat -never reached the JSON and an agent reading the sidecar saw a clean partial -report under a red job. Still best-effort and still silent: it runs while the -interpreter is being torn down." -``` - ---- - -### Task 5: Prove the two artifacts cannot disagree - -**Files:** -- Test: `test/hil/test/test_hil_bounded.py` - -- [ ] **Step 1: Write the test** - -```python -class MarkdownIsAlwaysARenderingOfTheJson(unittest.TestCase): - """The property this whole change buys: whatever wrote the report, re-rendering the - sidecar reproduces the markdown byte for byte.""" - - def _check(self, rd): - import json - doc = json.loads((rd / 'hil_report.json').read_text()) - self.assertEqual((rd / 'hil_report.md').read_text(), - hil_test.render_report(doc) + '\n') - - def test_normal_path(self): - td = TemporaryDirectory(); self.addCleanup(td.cleanup); rd = Path(td.name) - hil_test.accumulate_report( - [('boardA', 0, 0, [('boardA', {'cdc_msc': 'OK'}, '1s')], 0)], rd, True, - '-b boardA', '> **Rig note.** x\n') - self._check(rd) - - def test_after_an_accumulate_rerun(self): - td = TemporaryDirectory(); self.addCleanup(td.cleanup); rd = Path(td.name) - hil_test.accumulate_report( - [('boardA', 0, 0, [('boardA', {'cdc_msc': 'OK'}, '1s')], 0)], rd, True, '', '') - hil_test.accumulate_report( - [('boardB', 0, 0, [('boardB', {'cdc_msc': 'OK'}, '1s')], 0)], rd, False, '', '') - self._check(rd) - - def test_after_abandonment(self): - td = TemporaryDirectory(); self.addCleanup(td.cleanup); rd = Path(td.name) - hil_test.accumulate_report( - [('boardA', 0, 0, [('boardA', {'cdc_msc': 'OK'}, '1s')], 0)], rd, True, '', '') - hil_test.mark_report_abandoned(rd, 'the worker pool would not shut down.') - self._check(rd) - - def test_no_boards_exit(self): - td = TemporaryDirectory(); self.addCleanup(td.cleanup); rd = Path(td.name) - hil_test.write_report(rd, {'rows': [], 'banner': '', 'scope': '', - 'caveat': '**HIL run selected no boards.** why\n'}) - self._check(rd) -``` - -- [ ] **Step 2: Run it** - -Run: `python3 test/hil/test/test_hil_bounded.py MarkdownIsAlwaysARenderingOfTheJson` -Expected: PASS on all four. A failure here means a writer still bypasses `render_report`. - -- [ ] **Step 3: Full gate and commit** - -```bash -python3 -m unittest discover -s test/hil/test # 130 OK -pre-commit run --files test/hil/hil_test.py test/hil/helper/hil_health.py \ - test/hil/test/test_hil_bounded.py test/hil/test/test_hil_health.py -git add test/hil/test/test_hil_bounded.py -git commit -m "test/hil: pin that the markdown is always a rendering of the sidecar - -Four writers, one renderer. This is the invariant the change exists to create, -so it is asserted directly rather than inferred from the writers." -``` - ---- - -## Out of scope - -Deliberately not included, each its own follow-up: - -- **The flat `HIL_POOL_TIMEOUT`.** `hil_test.py:225` is a per-process 3600 s guard that does not scale with board count. It was per board when runs were serial; the origin branch made one run cover the fleet, so a 27-board run shares one budget. Real, and a scheduling change rather than a reporting one. -- **`hil_ci.sh` accumulate in remote mode.** `hil_ci.sh:183` `rm -rf`s `REMOTE_DIR` every run and the copies are one-way, so a remote `--accumulate` retry has no merge base and its one-row report overwrites the local full-fleet one. Fixing that means uploading `hil_report.json` and `<config>.failed` before the run, or keeping `REMOTE_DIR` when `--accumulate` is present. -- **Dropping `hil_report.md` entirely.** Not proposed. It is the PR artifact and what the `hil` skill tells operators to paste; this plan makes it generated, not redundant. diff --git a/docs/superpowers/followup/pr3840-mret-board-result.md b/docs/superpowers/followup/pr3840-mret-board-result.md new file mode 100644 index 000000000..7b8da7b9c --- /dev/null +++ b/docs/superpowers/followup/pr3840-mret-board-result.md @@ -0,0 +1,90 @@ +# Give the HIL worker result a name + +**Origin:** split out of PR #3840 (making `hil_report.md` a rendering of `hil_report.json`). +Delete this file when its own PR lands. + +## What is established + +`test_board()` returns a bare tuple that three producers build and fourteen call sites read +positionally. It has grown 5 → 6 → 7 fields, and the code already works around its own +shape: + +```python +hil_test.py:1992 dirty = [(r[0], r[6]) for r in mret if len(r) > 6 and r[6]] +hil_test.py:2014 blind = [r[0] for r in mret if len(r) > 5 and r[5]] +hil_test.py:2386 for name, _, _, _, dur, *_ in mret: +hil_report.py:306 for name, _, _, rows, *_ in mret: +``` + +Two facts make this worth closing rather than tolerating: + +- **The declared type is already wrong.** `hil_test.py:1711` says + `tuple[str, int, list[str], list, float]` — five fields — while the main return at `:1872` + yields seven (`+ sysfs_blind(), stray`). +- **A wrong slot is a wrong verdict, not a crash.** Field 5 is `blind`, which decides whether + a board's red cells are reported as broken hardware or as "could not tell". Inserting a + field mid-tuple makes `r[5]` read the wrong slot and keep running. + +It has bitten once already: `test_hil_bounded.py`'s +`test_both_row_widths_survive_the_report_writers` exists because the blindness flag widened +the tuple to 6 while the pool-timeout path still synthesised 5-field rows, and *"a +fixed-width unpack in either one raises INSIDE the containment path, which is where a raise +costs every board's results."* That is why the unpacks end in `*_`. + +## What remains + +A `NamedTuple` with defaults. Verified to pickle across the pool boundary and to stay +fully tuple-compatible — existing `r[0]`, `e[1]`, `for name, _, _, rows, *_` and `len(r)` +all keep working, so it lands without touching the fourteen consumers: + +```python +class BoardResult(NamedTuple): + """What one worker returns. Field ORDER is load-bearing: it is unpacked positionally + in a dozen places, and the pool-timeout path synthesises one by hand.""" + name: str + err_count: int + failed_tests: list[str] + rows: list | None # None from the pool-timeout synthesis, never [] + duration: float + blind: bool = False # defaults, so a synthesised result is full-width + stray: int = 0 +``` + +Then a second, smaller step removes the coupling itself: `accumulate_report` takes +`[(name, rows)]` pairs instead of `mret`, and `hil_test` does the extraction because it owns +the shape. One line at each end; the subtle merge logic — stale lock clearing, +`BOUNDARY_CELL`, `duration=None` preservation — is untouched. + +## Sizing + +| | Sites | +|---|---| +| Producers to convert | 4 (`hil_test.py:1724`, `:1872`, `:2283`, `:2327`) | +| Arity guards deleted | 2 (`:1992`, `:2014`) | +| Wrong annotation fixed | 1 (`:1711`) | +| `hil_report`'s coupled line | 1 (`:306`) | +| Positional consumers (optional migration) | 14 | +| **Test fixtures building tuples by hand** | **34** | + +Production code is roughly ten changed lines. **The work is dominated by the test +fixtures**, which is also the risk. + +## Do this first, or the refactor is unverifiable + +`test_hil_report.py` (27 sites) and `test_hil_bounded.py` (7) construct plain tuples by +hand — `('boardA', 0, [], [], 1.0, True)`. A producer that forgot to switch to +`BoardResult`, or a pickling regression, **passes the entire 310-test suite** and surfaces +only on the rig. Convert the fixtures to build `BoardResult` as task 1, before touching any +producer. This ordering is not optional. + +Second trap: `rows` is `None` on the pool-timeout path (`hil_test.py:2283`), never `[]`, and +`accumulate_report` guards with `if rows and ...`. A well-meaning `rows: list = []` default +silently changes that path. Pin it with a test before the conversion. + +## Why it was split out + +PR #3840 touches the report document. This touches `test_board`'s return and the containment +paths, where a raise costs every board's results rather than one board's — a different blast +radius, needing its own review and its own rig run. #3840 is twice-reviewed and dogfooded +ten times on hardware; folding this in would reset that surface for a latent-trap cleanup +that is not causing bugs today. diff --git a/docs/superpowers/followup/pr3840-skill-md-no-boards-drift.md b/docs/superpowers/followup/pr3840-skill-md-no-boards-drift.md new file mode 100644 index 000000000..a039a8c12 --- /dev/null +++ b/docs/superpowers/followup/pr3840-skill-md-no-boards-drift.md @@ -0,0 +1,38 @@ +# `SKILL.md` contradicts the code on no-boards tables + +**Origin:** split out of PR #3840, surfaced by its second review round. Delete this file +when its own PR lands. + +`.claude/skills/hil/SKILL.md:150-151` tells the reading agent: + +> `**HIL run selected no boards.**` — the filters intersected to nothing, so there is **no +> table at all**. Report that (and the filter shown), never `"pass": true`. + +That was true when the no-boards exit wrote a bare notice. It no longer is. An +`--accumulate` no-boards run keeps the accumulated rows — deliberately, because wiping them +destroyed real results — so the artifact now reads: + +``` +**HIL run selected no boards.** filters emptied + +**✅ 1 passed · ❌ 0 failed · ⚪ 0 skipped · blank not run** + +| Board | t | duration | +... +``` + +The behaviour is correct; the documentation is wrong, and wrong in the direction that +matters. An agent is told to expect no table, sees one, and has no rule for whether those +rows are reportable. **They are not this run's** — they are a previous attempt's, carried +forward. + +**What remains:** update that bullet to describe both cases — a fresh run has no table, an +`--accumulate` run shows the previous attempt's rows under the notice and they must not be +reported as this run's. Add a test asserting the fresh case renders no matrix, so the two +halves cannot drift again. + +## Why it was split out + +PR #3840 fixed the findings that changed a verdict. This is a documentation drift: the +behaviour is correct and the doc describing it is not, so it is better reviewed on its own +than appended to a branch already carrying a module consolidation. diff --git a/docs/superpowers/followup/pr3840-write-report-atomicity.md b/docs/superpowers/followup/pr3840-write-report-atomicity.md new file mode 100644 index 000000000..2094207bb --- /dev/null +++ b/docs/superpowers/followup/pr3840-write-report-atomicity.md @@ -0,0 +1,30 @@ +# `write_report` commits the two artifacts non-atomically + +**Origin:** split out of PR #3840, surfaced by its second review round. Delete this file +when its own PR lands. + +```python +md = render_report(doc) + '\n' +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(md, encoding='utf-8') +``` + +Rendering before writing closed the *render-failure* case: a raise can no longer commit a +sidecar the markdown contradicts. It does not close the *interrupted-between-writes* case. A +kill between those two lines leaves the pair disagreeing — and this runs on the containment +path, on the way to `os._exit`, on a rig whose jobs get cancelled by the GitHub job ceiling. + +**What remains:** write both to temp files, then `os.replace` both. The window shrinks from +two full writes to two renames, and neither file is ever observed half-written. `os.replace` +is atomic per file on POSIX; the pair is still not transactional, which is acceptable and +should be said in the docstring rather than implied away. + +Worth pairing with a test that kills between the writes — or, more practically, one that +asserts no partial file is ever visible by checking the temp-then-rename shape directly. + +## Why it was split out + +A durability edge, not a wrong verdict. PR #3840 closed the render-failure half of this +(nothing is written until the markdown renders); the interrupted-between-writes half needs +a temp-then-rename and is better reviewed on its own. diff --git a/docs/superpowers/plans/2026-08-21-hil-report-module.md b/docs/superpowers/plans/2026-08-21-hil-report-module.md new file mode 100644 index 000000000..5a7c832d8 --- /dev/null +++ b/docs/superpowers/plans/2026-08-21-hil-report-module.md @@ -0,0 +1,658 @@ +# hil_report.py Module Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Fold every function that produces, renders, merges or reads `hil_report.json`/`hil_report.md` into one module, `test/hil/helper/hil_report.py`, and take the two fixes that consolidation enables. + +**Architecture:** A new leaf-ish module owns the report document. `hil_test.py` and `hil_health.py` both import it, which dissolves the circular-import constraint that forced `write_timeout_report` to compose its own markdown. The duplicated cell classifier (`cell_kind` in `hil_test`, `cell_state` in `hil_summary`) collapses into one. `hil_summary.py` is deleted and its CLI moves in. + +**Tech Stack:** Python 3.13 stdlib only (`json`, `argparse`, `pathlib`); existing unit suites under `test/hil/test/` run with plain `unittest`. + +**Spec:** `docs/superpowers/specs/2026-08-21-hil-report-module-design.md` + +## Global Constraints + +- **Behaviour-preserving motion.** `hil_test.py`'s CLI, arguments, output and report format stay byte-identical. The two intended exceptions are named in the spec: the `hil_summary.py` → `hil_report.py` CLI path, and `write_timeout_report` rendering instead of concatenating. +- **`hil_report.py` must work in two modes.** It is imported as `helper.hil_report` by `hil_test.py`, and run as a script by the operator (`python3 test/hil/helper/hil_report.py <config> -b BOARD`). A script run puts `test/hil/helper/` on `sys.path`, *not* `test/hil/`, so `from helper import hil_health` fails in that mode. Task 1 pins both modes with tests. +- **Containment paths must never raise.** `mark_report_abandoned` and `write_timeout_report` run while the interpreter is being torn down or on the way to `os._exit`; anything escaping hangs the process in multiprocessing's unbounded `join()`. Their existing broad handlers move with them unchanged. +- **`hil_ci.sh` stages helpers by an explicit list** (`test/hil/hil_ci.sh:222-228`). A helper module missing from it reaches the rig absent, and the run dies with `ImportError` *after* `REMOTE_DIR` has been wiped. `RemoteStaging.test_import_closure_is_staged_to_the_rig` in `test_hil_bounded.py` already enforces this from the AST import closure; Task 1 only has to add the file to the list. +- Run `python3 -m unittest discover -s test/hil/test` (~82 s) before each commit; `pre-commit run --files <changed>` before pushing. + +--- + +### Task 1: The module, the vocabulary, one classifier, and the render half + +**Files:** +- Create: `test/hil/helper/hil_report.py` +- Create: `test/hil/test/test_hil_report.py` +- Modify: `test/hil/hil_test.py:110` (`REPORT_CELL`), `:1715` (`BOUNDARY_CELL`), `:1902-1903` (`REPORT_MD`/`REPORT_JSON`), `:1921-1978` (`render_matrix`), `:1981-2003` (`render_report`), `:67` (imports) +- Modify: `test/hil/hil_ci.sh:222-228` (scp list) +- Modify: `test/hil/test/test_hil_bounded.py` (move `RenderReportIsPureFunctionOfTheDocument` out) + +**Interfaces:** +- Produces: `helper.hil_report` exposing `REPORT_MD`, `REPORT_JSON`, `REPORT_CELL`, `BOUNDARY_CELL`, `LOCKED_CELL`, `cell_state(v) -> str`, `render_matrix(rows_all) -> str`, `render_report(doc) -> str`. +- `hil_test.py` re-exports nothing: call sites become `hil_report.NAME`. + +- [ ] **Step 1: Write the failing tests** + +Create `test/hil/test/test_hil_report.py`: + +```python +#!/usr/bin/env python3 +# SPDX-License-Identifier: MIT +# Unit tests for the report document: the vocabulary, the one cell classifier, rendering, +# the four writers, and the fold to per-board verdicts. Split out of test_hil_bounded.py +# and test_hil_health.py when the report code moved into helper/hil_report.py. +# Run directly: +# python3 test/hil/test/test_hil_report.py +import json +import os +import subprocess +import sys +import unittest +from pathlib import Path +from tempfile import TemporaryDirectory + +TEST_DIR = os.path.dirname(os.path.abspath(__file__)) +HIL_DIR = os.path.dirname(TEST_DIR) +sys.path.insert(0, HIL_DIR) + +from helper import hil_report + + +class OneClassifierForBothArtifacts(unittest.TestCase): + """The markdown tally and the agent's verdict used to classify cells with two separate + copies of one rule -- hil_test's cell_kind against REPORT_CELL, and hil_summary's + cell_state against its own re-typed '❌'/'⚪' literals. Change the icons and the table + and the verdict silently disagree.""" + + def test_bare_states(self): + self.assertEqual(hil_report.cell_state('fail'), 'fail') + self.assertEqual(hil_report.cell_state('skip'), 'skip') + self.assertEqual(hil_report.cell_state('pass'), 'pass') + + def test_icon_prefixed_metrics_carry_their_verdict(self): + self.assertEqual(hil_report.cell_state(f'{hil_report.REPORT_CELL["fail"]} 29/30'), 'fail') + self.assertEqual(hil_report.cell_state(f'{hil_report.REPORT_CELL["skip"]} board wedged'), + 'skip') + + def test_an_unprefixed_metric_is_a_pass(self): + """Load-bearing: a passing test may return a plain metric string. Classifying + unknown shapes as fail would publish a green table as a red verdict.""" + self.assertEqual(hil_report.cell_state('480.0 MBps'), 'pass') + self.assertEqual(hil_report.cell_state('1103 KB/s'), 'pass') + + def test_a_non_string_cell_does_not_raise(self): + """render_matrix's copy guarded with isinstance; hil_summary's did not, because its + caller str()'d first. The merged one keeps the guard -- it is the safer superset.""" + self.assertEqual(hil_report.cell_state(None), 'pass') + + def test_the_icons_come_from_REPORT_CELL(self): + """No second copy of the emoji anywhere in the module.""" + src = (Path(HIL_DIR) / 'helper' / 'hil_report.py').read_text(encoding='utf-8') + for icon in ('❌', '⚪', '✅'): + self.assertEqual(src.count(f"'{icon}'"), 1, + f'{icon} is spelled as a literal more than once') + + +class ModuleWorksImportedAndAsAScript(unittest.TestCase): + """It is imported as helper.hil_report by hil_test, and run as a script by the operator + (.claude/agents/hil-operator.md). A script run puts helper/ on sys.path, NOT test/hil, + so a plain `from helper import hil_health` breaks the CLI and only the CLI.""" + + def test_importable_as_a_package_module(self): + r = subprocess.run( + [sys.executable, '-c', + f'import sys; sys.path.insert(0, {HIL_DIR!r}); ' + f'from helper import hil_report; print(hil_report.REPORT_JSON)'], + capture_output=True, text=True, timeout=60) + self.assertEqual(r.returncode, 0, r.stderr) + self.assertIn('hil_report.json', r.stdout) + + def test_runnable_as_a_script(self): + r = subprocess.run( + [sys.executable, str(Path(HIL_DIR) / 'helper' / 'hil_report.py'), '--help'], + capture_output=True, text=True, timeout=60) + self.assertEqual(r.returncode, 0, r.stderr) + + +class HilCiStagesEveryHelperTheRunImports(unittest.TestCase): + """hil_ci.sh copies helper modules by an EXPLICIT list. One missing module reaches the + rig absent and the run dies with ImportError -- after REMOTE_DIR has already been + rm -rf'd, so the previous run's report and re-run spec are gone too.""" + + def test_the_scp_list_covers_what_hil_test_imports(self): + sh = (Path(HIL_DIR) / 'hil_ci.sh').read_text(encoding='utf-8') + staged = {line.split('helper/')[1].rstrip('" \\\n') + for line in sh.splitlines() if '/test/hil/helper/' in line and '.py' in line} + imported = set() + for mod in (Path(HIL_DIR) / 'hil_test.py', Path(HIL_DIR) / 'helper' / 'hil_report.py'): + src = mod.read_text(encoding='utf-8') + for raw in src.splitlines(): + line = raw.strip() # hil_report's own import is indented in a try + if line.startswith('from helper import '): + imported |= {f'{n.strip()}.py' for n in line.split('import', 1)[1].split(',')} + elif line.startswith('from helper.'): + imported.add(line.split('.')[1].split(' ')[0] + '.py') + missing = imported - staged + self.assertEqual(missing, set(), + f'hil_ci.sh does not stage {missing}; a remote run will ImportError') + + +if __name__ == '__main__': + unittest.main() +``` + +Then **move** the class `RenderReportIsPureFunctionOfTheDocument` from `test/hil/test/test_hil_bounded.py` into this file verbatim, changing only `hil_test.render_report` → `hil_report.render_report` throughout. + +- [ ] **Step 2: Run them to verify they fail** + +Run: `python3 test/hil/test/test_hil_report.py` +Expected: FAIL — `ModuleNotFoundError: No module named 'helper.hil_report'` + +- [ ] **Step 3: Create the module** + +Create `test/hil/helper/hil_report.py`: + +```python +#!/usr/bin/env python3 +# SPDX-License-Identifier: MIT +"""The HIL report document: one owner for hil_report.json and hil_report.md. + +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 that document: the cell vocabulary, the one classifier both artifacts share, rendering, +the four writers, and the fold to one machine-readable verdict per board. + +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. +""" +import argparse +import json +import sys +from pathlib import Path + +try: # imported as part of the helper package + from helper.hil_health import _p +except ImportError: # run as a script: helper/ is sys.path[0] + from hil_health import _p + +REPORT_MD = 'hil_report.md' +REPORT_JSON = 'hil_report.json' +# The status vocabulary, shared by the code that WRITES a cell (hil_test's test runners) and +# the code that reads one back (cell_state). One dict, so the human's table and the agent's +# verdict cannot drift apart. +REPORT_CELL = {'pass': '✅', 'fail': '❌', 'skip': '⚪'} +BOUNDARY_CELL = 'same-PID boundary' +LOCKED_CELL = 'board-locked' + + +def cell_state(v) -> str: + """'pass' | 'fail' | 'skip' for one report cell. + + THE classifier -- the markdown tally and the per-board verdict both call this, so they + cannot disagree. 'fail' or a ❌ prefix is a failure, 'skip' or a ⚪ prefix is a skip, and + EVERYTHING ELSE is a pass. That last arm is load-bearing: a passing test may return a + plain metric string ('480.0 MBps') that lands in the cell unprefixed, while failures are + guaranteed marked -- TestFail's docstring pins that its metric is icon-prefixed precisely + so render and tally treat it as a failure. Classifying unknown shapes as fail here would + publish a green table as a red verdict. + + isinstance-guarded: cells are usually str but a caller may hand over None or a number, + and .startswith on those raises inside a report writer that must not raise.""" + if v == 'fail' or (isinstance(v, str) and v.startswith(REPORT_CELL['fail'])): + return 'fail' + if v == 'skip' or (isinstance(v, str) and v.startswith(REPORT_CELL['skip'])): + return 'skip' + return 'pass' +``` + +Then move, verbatim, from `hil_test.py`: +- `render_matrix` (`hil_test.py:1921-1978`) — with one change: delete its nested `cell_kind` + definition and call the module-level `cell_state` instead. The line + `kinds = [cell_kind(v) for _, cells, _ in rows_all for v in cells.values()]` becomes + `kinds = [cell_state(v) for _, cells, _ in rows_all for v in cells.values()]`. +- `render_report` (`hil_test.py:1981-2003`) — unchanged. + +Add a placeholder CLI so `--help` works (Task 4 fills in `summarize`): + +```python +def main() -> int: + ap = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + ap.add_argument('config_file') + ap.add_argument('-b', '--board', action='append', default=[], + help='boards to report on; default: every board in the config') + ap.add_argument('--report-dir', default='.', help=f'where {REPORT_JSON} lives (default: cwd)') + ap.parse_args() + raise SystemExit('hil_report: summarize() lands in Task 4') + + +if __name__ == '__main__': + sys.exit(main()) +``` + +- [ ] **Step 4: Point `hil_test.py` at the module** + +In `hil_test.py:67`, extend the import: + +```python +from helper import hil_health, hil_lock, hil_report, hil_util +``` + +Delete `REPORT_CELL` (`:110`), `BOUNDARY_CELL` (`:1715`), `REPORT_MD`/`REPORT_JSON` +(`:1902-1903`), `render_matrix` and `render_report` from `hil_test.py`. Then rewrite every +reference to the moved names as `hil_report.<name>`. Find them all with: + +```bash +grep -n "REPORT_CELL\|BOUNDARY_CELL\|REPORT_MD\|REPORT_JSON\|render_matrix\|render_report" \ + test/hil/hil_test.py +``` + +Known sites: `:876`, `:1369`, `:1459`, `:1490`, `:1492`, `:1508`, `:1818`, `:1834`, `:2162`, +`:2191-2192`, `:2209-2210`, `:2403`, `:2592`. + +- [ ] **Step 5: Stage the new module for remote runs** + +In `test/hil/hil_ci.sh:222-228`, add the module to the scp list (keep alphabetical-ish order +with the rest): + +```bash +scp -q "$ROOT_DIR/test/hil/helper/__init__.py" \ + "$ROOT_DIR/test/hil/helper/hil_util.py" \ + "$ROOT_DIR/test/hil/helper/hil_health.py" \ + "$ROOT_DIR/test/hil/helper/hil_lock.py" \ + "$ROOT_DIR/test/hil/helper/hil_report.py" \ + "$ROOT_DIR/test/hil/helper/hil_summary.py" \ + "$ROOT_DIR/test/hil/helper/hil_select.py" \ + "$REMOTE:$REMOTE_DIR/test/hil/helper/" +``` + +- [ ] **Step 6: Run the tests** + +Run: `python3 test/hil/test/test_hil_report.py` → OK +Run: `python3 -m unittest discover -s test/hil/test` → 274 OK (266 + 8 new: 5 classifier, +2 dual-mode, 1 scp guard; `RenderReport…` moves rather than adds) + +- [ ] **Step 7: Commit** + +```bash +git add test/hil/helper/hil_report.py test/hil/hil_test.py test/hil/hil_ci.sh \ + test/hil/test/test_hil_report.py test/hil/test/test_hil_bounded.py +git commit -m "hil_report: new module for the report vocabulary, classifier and rendering + +The markdown tally and the agent's verdict classified cells with two separate +copies of one rule, the second documented as 'the EXACT classifier hil_test.py's +own tally uses'. One cell_state now serves both, keyed off the one REPORT_CELL." +``` + +--- + +### Task 2: Move the three writers + +**Files:** +- Modify: `test/hil/helper/hil_report.py` (add the writers) +- Modify: `test/hil/hil_test.py:2005-2036` (`write_report`, `mark_report_abandoned`), `:2149-2212` (`accumulate_report`) +- Modify: `test/hil/test/test_hil_bounded.py` (move three classes out), `test/hil/test/test_hil_report.py` + +**Interfaces:** +- Consumes: `render_report`, `REPORT_MD`, `REPORT_JSON`, `BOUNDARY_CELL` from Task 1. +- Produces: `hil_report.write_report(report_dir, doc)`, `hil_report.mark_report_abandoned(report_dir, why)`, `hil_report.accumulate_report(mret, report_dir, fresh, scope='', banner='') -> str`. + +- [ ] **Step 1: Move the tests** + +Move these classes from `test/hil/test/test_hil_bounded.py` into `test/hil/test/test_hil_report.py`, +verbatim except `hil_test.<name>` → `hil_report.<name>` for the three moved functions: + +- `ScopeSurvivesInTheJson` +- `EveryExitPathLeavesBothArtifacts` +- `AbandonNoticeLandsInBothArtifacts` +- `CaveatSurvivesAccumulate` +- `MarkdownIsAlwaysARenderingOfTheJson` + +`AbandonNoticeLandsInBothArtifacts.test_an_existing_abandon_caveat_is_not_overwritten` calls +`hil_health.write_timeout_report`; leave that call as-is — Task 3 moves it. + +- [ ] **Step 2: Run them to verify they fail** + +Run: `python3 test/hil/test/test_hil_report.py` +Expected: FAIL — `AttributeError: module 'helper.hil_report' has no attribute 'write_report'` + +- [ ] **Step 3: Move the functions** + +Cut `write_report` (`hil_test.py:2005-2014`), `mark_report_abandoned` (`:2016-2036`) and +`accumulate_report` (`:2149-2212`) from `hil_test.py` and paste them into `hil_report.py` +below `render_report`, unchanged. + +Add to `accumulate_report`'s docstring, after the existing text, so the wart is recorded +where a reader meets it: + +``` + `mret` is hil_test.py's worker-result shape (name, err, fts, rows, ...), so this one + function knows something about its caller that the rest of the module does not. Folding + mret into rows could live in hil_test and only the merge here, but that would rewrite + the subtle parts -- stale board-locked clearing, BOUNDARY_CELL dropping, duration=None + preservation -- for a tidier seam. Data-shape coupling, not an import cycle. +``` + +- [ ] **Step 4: Update the call sites** + +In `hil_test.py`, the three call sites become `hil_report.*`: + +```bash +grep -n "accumulate_report(\|write_report(\|mark_report_abandoned(" test/hil/hil_test.py +``` + +Known sites: `:2260` (inside `_abandon_exit`), `:2351` (no-boards exit), `:2486`, `:2525`, +`:2618`. + +- [ ] **Step 5: Run the tests** + +Run: `python3 -m unittest discover -s test/hil/test` → 274 OK (motion only, no count change) + +- [ ] **Step 6: Commit** + +```bash +git add test/hil/helper/hil_report.py test/hil/hil_test.py \ + test/hil/test/test_hil_report.py test/hil/test/test_hil_bounded.py +git commit -m "hil_report: move the report writers off hil_test + +write_report, mark_report_abandoned and accumulate_report join the renderer they +already call. Pure motion; accumulate_report's knowledge of mret's tuple shape +moves with it and is now documented rather than implicit." +``` + +--- + +### Task 3: `write_timeout_report` renders like everyone else + +**Files:** +- Modify: `test/hil/helper/hil_report.py` (receive the function) +- Modify: `test/hil/helper/hil_health.py:347-398` (remove it), `:19` (drop `import json`) +- Modify: `test/hil/hil_test.py:2498` (call site) +- Modify: `test/hil/test/test_hil_health.py` (move `WriteTimeoutReport` out), `test/hil/test/test_hil_report.py` + +**Interfaces:** +- Consumes: `render_report`, `write_report` from Tasks 1-2. +- Produces: `hil_report.write_timeout_report(report_dir, boards, secs, banner='', prefix='')`. The `md_name` parameter is **gone** — the module owns `REPORT_MD`. + +- [ ] **Step 1: Write the failing tests** + +Move `WriteTimeoutReport` from `test/hil/test/test_hil_health.py` into +`test/hil/test/test_hil_report.py`, changing `hil_health.write_timeout_report` → +`hil_report.write_timeout_report` and dropping the `md_name` argument from every call. Two +of its tests change substantively: + +```python + def test_the_prior_attempts_rows_survive(self): + """Was: the prior MARKDOWN TEXT survives below the banner. It now re-renders from + the merged sidecar, so the guarantee is stated against rows -- one table with the + stuck boards in it, rather than a banner stapled above a duplicate table.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report( + [('done', 0, 0, [('done', {'cdc_msc': 'OK'}, '1s')], 0)], rd, True, '', '') + hil_report.write_timeout_report(rd, [{'name': 'stuck'}], 3600) + doc = json.loads((rd / hil_report.REPORT_JSON).read_text()) + self.assertEqual([r['board'] for r in doc['rows']], ['done', 'stuck']) + md = (rd / hil_report.REPORT_MD).read_text() + self.assertIn('done', md) + self.assertIn('stuck', md) + self.assertIn('abandoned', md) + self.assertLess(md.index('abandoned'), md.index('done')) + self.assertEqual(md.count('| Board'), 1, 'the prior table was duplicated, not merged') + + def test_prefix_carries_the_preflight_diagnosis(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.write_timeout_report(rd, [{'name': 'b1'}], 4200, + prefix='> **wedged usb_hub_wq worker.**\n') + out = (rd / hil_report.REPORT_MD).read_text() + self.assertTrue(out.startswith('> **wedged usb_hub_wq worker.**')) + self.assertIn('timed out after 4200s', out) + self.assertIn('b1', out) +``` + +And in `MarkdownIsAlwaysARenderingOfTheJson`, **delete** +`test_the_pool_guard_fallback_agrees_even_if_it_does_not_render` and add the fifth case in +its place: + +```python + def test_the_pool_guard_fallback(self): + """The last writer to join the invariant: it composed its own markdown only because + hil_health could not import the renderer.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report( + [('done', 0, 0, [('done', {'cdc_msc': 'OK'}, '1s')], 0)], rd, True, '', '') + hil_report.write_timeout_report(rd, [{'name': 'stuck'}], 3600, + prefix='> **wedged usb_hub_wq worker.**\n') + self._check(rd) +``` + +- [ ] **Step 2: Run them to verify they fail** + +Run: `python3 test/hil/test/test_hil_report.py` +Expected: FAIL — `AttributeError: module 'helper.hil_report' has no attribute 'write_timeout_report'` + +- [ ] **Step 3: Move it and make it render** + +Add to `hil_report.py`, and delete `hil_health.py:347-398` plus its now-unused +`import json` at `hil_health.py:19`: + +```python +def write_timeout_report(report_dir: Path, boards, secs: int, + banner: str = '', prefix: str = '') -> None: + """Leave a report behind when the worker pool has to be abandoned. + + map_async is all-or-nothing, so a timeout loses every per-board result and the report + dir would stay empty with no reason for the failure. Any prior attempt's rows are kept + and the stuck boards are merged in beside them. + + `prefix` carries the preflight rig-health verdict: the timeout aborts before + accumulate_report, so without it the report loses the one line saying WHY the pool never + finished.""" + try: + # Built INSIDE the try: a roster entry without a 'name' key raises while assembling + # the board list, and outside the try that escaped and stranded the runner -- which + # is exactly what the broad handler below exists to prevent. + caveat = (prefix + '\n' if prefix else '') + (banner or ( + f'**HIL run abandoned: worker pool timed out after {secs}s.**\n\n' + f'No per-board results could be collected for this attempt, so any rows below ' + f'are from an earlier one. Boards dispatched:\n\n' + + '\n'.join(f'- {b.get("name", "?")}' for b in boards) + '\n')) + # Rows MERGE rather than replace: an earlier attempt's finished boards are real + # results and this attempt has none of its own. Own handler, because a torn sidecar + # must not cost the stuck rows -- losing the old table is a nicety, losing the + # caveat is the failure. + jpath = report_dir / REPORT_JSON + try: + doc = json.loads(jpath.read_text()) if jpath.is_file() else {} + rows = list(doc.get('rows', [])) + except (OSError, ValueError, TypeError, AttributeError): + doc, rows = {}, [] + done = {r.get('board') for r in rows if isinstance(r, dict)} + rows += [{'board': b.get('name', '?'), 'cells': {'pool-timeout': 'fail'}, + 'duration': None} for b in boards if b.get('name', '?') not in done] + write_report(report_dir, {'rows': rows, 'banner': doc.get('banner', ''), + 'scope': doc.get('scope', ''), 'caveat': caveat}) + except Exception as e: # noqa: BLE001 + # Deliberately broad: this is the first statement of the pool-abandon path, so ANY + # escape skips kill_pool_children and os._exit and strands the runner. + _p(f'warning: cannot write {REPORT_MD} to {report_dir}: {e}', flush=True) +``` + +Update `hil_health.py`'s module docstring: its first line reads "Shutting a wedged HIL run +down: kill what the workers spawned, then report." — drop ", then report". + +- [ ] **Step 4: Update the call site** + +`hil_test.py:2498` becomes: + +```python + hil_report.write_timeout_report( + report_dir, [b for b in config_boards + if b['name'] in stuck], POOL_TIMEOUT, + prefix=health_banner) +``` + +- [ ] **Step 5: Run the tests** + +Run: `python3 -m unittest discover -s test/hil/test` → 274 OK (one deleted, one added) + +- [ ] **Step 6: Commit** + +```bash +git add test/hil/helper/hil_report.py test/hil/helper/hil_health.py test/hil/hil_test.py \ + test/hil/test/test_hil_report.py test/hil/test/test_hil_health.py +git commit -m "hil_report: the pool-guard fallback renders like every other writer + +It composed its own markdown for one reason: hil_health cannot import hil_test +back, so it could not reach render_report. With the renderer in a module both +import, that constraint is gone and all five writers are byte-identical -- +MarkdownIsAlwaysARenderingOfTheJson covers the fifth, and the weaker +'agrees even if it does not render' promise is deleted. + +hil_health goes back to doing one thing: killing wedged processes." +``` + +--- + +### Task 4: Fold `hil_summary.py` in and delete it + +**Files:** +- Modify: `test/hil/helper/hil_report.py` (real `summarize` + CLI) +- Delete: `test/hil/helper/hil_summary.py` +- Modify: `test/hil/hil_ci.sh` (drop `hil_summary.py` from the scp list) +- Modify: `.claude/agents/hil-operator.md:71`, `.claude/workflows/hil-validate.js:14,17,54,58,67`, `.claude/workflows/test-hil-validate.mjs:7` +- Modify: `test/hil/test/test_hil_bounded.py` (move `SummaryFoldsReportToBoards` out), `test/hil/test/test_hil_report.py` + +**Interfaces:** +- Consumes: `cell_state`, `LOCKED_CELL`, `REPORT_JSON` from Task 1. +- Produces: `hil_report.variants_of(cfg, board) -> list`, `hil_report.summarize(cfg, boards, report) -> dict` returning `{'results': [...], 'banner': str, 'caveat': str}`; CLI `python3 test/hil/helper/hil_report.py <config> [-b BOARD]... [--report-dir DIR]`. + +- [ ] **Step 1: Move the tests** + +Move `SummaryFoldsReportToBoards` from `test/hil/test/test_hil_bounded.py` into +`test/hil/test/test_hil_report.py`, changing the subprocess target from +`helper/hil_summary.py` to `helper/hil_report.py` in both places (`test_hil_bounded.py:1675` +and `:1757`). Add one test pinning that the old entry point is gone: + +```python + def test_the_old_entry_point_is_gone(self): + """hil_summary.py's CLI moved here. A leftover file would keep working while + drifting from the module that now owns the fold.""" + self.assertFalse((Path(HIL_DIR) / 'helper' / 'hil_summary.py').exists()) +``` + +- [ ] **Step 2: Run them to verify they fail** + +Run: `python3 test/hil/test/test_hil_report.py` +Expected: FAIL — the subprocess exits non-zero with `hil_report: summarize() lands in Task 4` + +- [ ] **Step 3: Move `summarize` in and delete the old file** + +Copy `variants_of` (`hil_summary.py:47-52`) and `summarize` (`:54-92`) into `hil_report.py` +verbatim, with two changes: `cell_state(str(val))` becomes `cell_state(val)` (the merged +classifier is isinstance-guarded, so the `str()` is dead), and the module's own +`FAIL_ICON`/`SKIP_ICON`/`LOCKED_CELL`/`cell_state` definitions are NOT copied — Task 1's +already serve. + +Replace the Task 1 placeholder `main()` with the real one from `hil_summary.py:94-115`, +changing `Path(a.report_dir) / 'hil_report.json'` to `Path(a.report_dir) / REPORT_JSON`. + +Then: + +```bash +git rm test/hil/helper/hil_summary.py +``` + +- [ ] **Step 4: Update the consumers** + +`test/hil/hil_ci.sh` — remove the `hil_summary.py` line from the scp list added in Task 1. + +`.claude/agents/hil-operator.md:71`: + +```bash +python3 test/hil/helper/hil_report.py <config> -b BOARD [-b BOARD...] # from the report dir +``` + +`.claude/workflows/hil-validate.js:58`: + +```javascript + ` python3 test/hil/helper/hil_report.py <the config you used> ${boards.map((b) => `-b ${b}`).join(' ')}\n` + +``` + +In `.claude/workflows/hil-validate.js` lines 14, 17, 54 and 67, and +`.claude/workflows/test-hil-validate.mjs` line 7, replace the prose mentions of +`hil_summary.py` with `hil_report.py`. Change nothing else in those files — the operator's +return contract (`{results, banner, wedged}`) is untouched. + +- [ ] **Step 5: Run the tests** + +Run: `python3 test/hil/test/test_hil_report.py` → OK +Run: `python3 -m unittest discover -s test/hil/test` → 275 OK +Run: `node .claude/workflows/test-hil-validate.mjs` → OK +Run: `grep -rn "hil_summary" . --include=*.py --include=*.sh --include=*.js --include=*.mjs --include=*.md | grep -v docs/superpowers` → no hits + +- [ ] **Step 6: Commit** + +```bash +git add test/hil/helper/hil_report.py test/hil/hil_ci.sh test/hil/test/ \ + .claude/agents/hil-operator.md .claude/workflows/hil-validate.js \ + .claude/workflows/test-hil-validate.mjs +git rm --cached test/hil/helper/hil_summary.py 2>/dev/null || true +git commit -m "hil_report: fold hil_summary in; one module owns the document end to end + +The fold to per-board verdicts is the read half of the artifact the rest of this +module writes, and it carried the second copy of the cell classifier. The CLI +keeps its arguments; only its path changes, which the two harness docs that +invoke it by name follow." +``` + +--- + +## Validation + +- [ ] **Full gate** + +```bash +python3 -m unittest discover -s test/hil/test # 275 OK +pre-commit run --all-files +``` + +- [ ] **Prove the motion changed no behaviour.** Re-render the real fleet report captured + before the refactor and diff it against what the branch produces now: + +```bash +python3 - <<'EOF' +import json, sys +sys.path.insert(0, 'test/hil') +from helper import hil_report +doc = json.load(open('hil_report.json')) # the pair the rig produced pre-refactor +assert open('hil_report.md').read() == hil_report.render_report(doc) + '\n', 'render drifted' +print('render is byte-identical to the pre-refactor artifact') +EOF +``` + +- [ ] **Rig re-check.** `hil_report.py` must reach the rig and the CLI must run there: + +```bash +bash test/hil/hil_ci.sh -b stm32f407disco -b nanoch32v203 +ssh [email protected] 'cd /tmp/tinyusb-hil && python3 test/hil/helper/hil_report.py \ + test/hil/tinyusb.json -b stm32f407disco -b nanoch32v203' +``` + +Expect a two-board table, `md == render_report(json)`, and a `summarize` verdict naming both +boards — `nanoch32v203` proving the variant fold still works through the moved code. + +## 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`). diff --git a/docs/superpowers/specs/2026-08-21-hil-report-module-design.md b/docs/superpowers/specs/2026-08-21-hil-report-module-design.md new file mode 100644 index 000000000..41e7000b7 --- /dev/null +++ b/docs/superpowers/specs/2026-08-21-hil-report-module-design.md @@ -0,0 +1,144 @@ +# hil_report.py: one owner for the HIL report document + +**Date:** 2026-08-21 +**Branch:** `hil-report` (continues the report-unification work already on it) + +## Motivation + +`hil_report.json` and `hil_report.md` are now one document rendered two ways, but the code that +produces, renders, merges and reads that document is spread across three modules: + +| Module | Report-related content | +|---|---| +| `hil_test.py` | `REPORT_CELL`, `BOUNDARY_CELL`, `REPORT_MD`, `REPORT_JSON`, `render_matrix`, `render_report`, `write_report`, `mark_report_abandoned`, `accumulate_report` | +| `helper/hil_health.py` | `write_timeout_report` — composes its own markdown | +| `helper/hil_summary.py` | `cell_state`, `variants_of`, `summarize`, CLI | + +Two concrete defects follow from that spread. + +**One classifier, two copies.** `hil_test.py:1966` (`cell_kind`, keyed off `REPORT_CELL`) and +`hil_summary.py:34` (`cell_state`, with its own re-typed `FAIL_ICON, SKIP_ICON = '❌', '⚪'`) +implement the same rule. The latter's docstring says it is *"the EXACT classifier hil_test.py's own +tally uses"* — the duplication was noticed and documented as an obligation to keep in sync, rather +than removed. Change `REPORT_CELL` and the human's table and the agent's verdict silently disagree: +the markdown says ❌ where the JSON says `pass`. That is the same class of defect this branch +exists to eliminate, one layer up. + +**A writer that cannot render.** `hil_test.py` imports `hil_health`, so `hil_health` cannot import +`hil_test` back. That is the only reason `write_timeout_report` composes its own markdown instead of +calling `render_report`, and the only reason the pool-guard fallback is held to a weaker promise +(same boards and caveat in both artifacts, not byte-identical) while the other four writers are +exact. The constraint is structural, not essential: a leaf module both can import dissolves it. + +## Goal / non-goals + +**Goal:** `test/hil/helper/hil_report.py` becomes the single owner of the report document. + +**This is NOT purely code motion, and the distinction matters for review.** Measured against +`master`, `hil_test.py` contains only `render_matrix` and `accumulate_report`. Everything else in +the new module — `render_report`, `write_report`, `mark_report_abandoned`, `mark_report_no_boards`, +`_load`, `_write_stuck_over_prior_md`, `cell_state`, and the `scope`/`caveat` plumbing — is NEW +code, roughly 150 lines of it, and two rounds of review found most of their defects there. Read +those functions as new, not as relocated. `hil_test.py`'s CLI, arguments and table format do stay +unchanged. + +**Deliberate user-visible changes:** +1. `hil_summary.py` is deleted; its CLI moves to `hil_report.py`. The documented command becomes + `python3 test/hil/helper/hil_report.py <config> -b BOARD [-b BOARD…]`. +2. `write_timeout_report` re-renders from the merged sidecar instead of stapling its banner above + the previous attempt's markdown text. Output improves — one table containing the stuck boards, + rather than a fresh banner above a duplicate table — but it is a change (see Testing). + +**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`). + +## Resulting layout (`test/hil/`) + +| File | ~Lines | Role | +|---|---|---| +| `hil_test.py` | 2390 (−250) | tests + orchestration + CLI | +| `helper/hil_report.py` (new) | ~400 | the report document: vocabulary, render, write, merge, fold, CLI | +| `helper/hil_health.py` | ~345 (−53) | killing wedged processes only | +| `helper/hil_summary.py` | deleted | superseded by `hil_report.py` | + +Import graph: `hil_health` is a leaf; `hil_report` → `hil_health` (for `_p`, the +BrokenPipeError-safe print used on containment paths); `hil_test` → both. No cycles. + +## hil_report.py + +Stdlib only (`json`, `argparse`, `pathlib`) beyond that one `_p` import. Sections, in order: + +**Vocabulary.** `REPORT_MD`, `REPORT_JSON`, `REPORT_CELL`, `BOUNDARY_CELL`, `LOCKED_CELL`. +`REPORT_CELL` becomes the single source of the status icons; `hil_summary.py`'s `FAIL_ICON`/ +`SKIP_ICON` literals are deleted. + +**Classifier.** One `cell_state(v) -> 'pass' | 'fail' | 'skip'`, replacing both `cell_kind` and the +old `cell_state`. Keeps the surviving docstring's warning that the `pass` arm is load-bearing: a +passing test may return an unprefixed metric string (`'480.0 MBps'`), while failures are guaranteed +icon-marked, so classifying unknown shapes as `fail` would publish a green table as a red verdict. + +**Render.** `render_matrix(rows_all)`, `render_report(doc)`. Unchanged; `render_matrix`'s inline +`cell_kind` is replaced by a call to the module-level `cell_state`. + +**Write.** `write_report`, `accumulate_report`, `mark_report_abandoned`, `write_timeout_report`. +Moved verbatim except `write_timeout_report`, which loses its `md_name` parameter (the module owns +`REPORT_MD`) and renders instead of concatenating. + +**Fold.** `variants_of`, `summarize`, and the `main()` CLI from `hil_summary.py`. + +## Deliberate wart + +`accumulate_report` moves wholesale, keeping its knowledge of `mret`'s worker-result tuple shape. +The cleaner boundary would split "fold `mret` → rows" (`hil_test`'s domain) from "merge rows → doc" +(`hil_report`'s), but that rewrites subtle, well-tested logic — stale `board-locked` clearing, +`BOUNDARY_CELL` dropping, `duration=None` preservation — for a tidier seam. It is a data-shape +coupling, not an import cycle. Moving it verbatim keeps the motion reviewable as motion. + +## The sharp edge + +`hil_ci.sh:222-228` stages helper modules by an **explicit scp list**. A new `helper/hil_report.py` +that is not added there reaches the rig missing, and the run dies with `ImportError` *after* +`REMOTE_DIR` has already been wiped — so the previous run's report and re-run spec are gone too. + +This is already guarded: `test_hil_bounded.py`'s `RemoteStaging.test_import_closure_is_staged_to_the_rig` +walks the AST import closure from `hil_test.py`, `usbtest.py` and `mtp_test.py` and requires an exact +scp entry for each file. Adding the module to the list is all this change needs; no new guard is +warranted, and an earlier draft of this document wrongly claimed none existed. + +## Consumers to update + +| File | Change | +|---|---| +| `test/hil/hil_ci.sh:226` | `hil_summary.py` → `hil_report.py` in the scp list | +| `.claude/agents/hil-operator.md:71` | the documented command | +| `.claude/workflows/hil-validate.js:58` | the command the operator is told to run | +| `.claude/workflows/hil-validate.js:14,17,54,67`, `test-hil-validate.mjs:7` | stale `hil_summary.py` mentions in comments | + +No logic in the `.claude` files changes — the operator's return contract +(`{results, banner, wedged}`) is untouched. + +## Testing + +New `test/hil/test/test_hil_report.py`. The report-specific classes move there from +`test_hil_bounded.py` (`CaveatSurvivesAccumulate`, `SummaryFoldsReportToBoards`, +`ScopeSurvivesInTheJson`, `RenderReportIsPureFunctionOfTheDocument`, +`EveryExitPathLeavesBothArtifacts`, `AbandonNoticeLandsInBothArtifacts`, +`MarkdownIsAlwaysARenderingOfTheJson`) and from `test_hil_health.py` (`WriteTimeoutReport`). + +Three test changes are substantive rather than mechanical: + +1. `WriteTimeoutReport.test_keeps_a_previous_attempts_table` asserts the prior **markdown text** + survives. It becomes an assertion that the prior attempt's **rows** survive — the same guarantee + against the new representation. +2. `MarkdownIsAlwaysARenderingOfTheJson` gains a fifth case for the pool-guard fallback, which now + satisfies the byte-identical invariant like the other four. +3. `test_the_pool_guard_fallback_agrees_even_if_it_does_not_render` — the weaker promise — is + deleted, because the promise it encoded no longer applies. + +Gate: `python3 -m unittest discover -s test/hil/test` at 275 — the current 266, minus the one +deleted test, plus the fifth invariant case, the scp-list guard, two dual-mode import tests, +five classifier tests and one pinning that the old entry point is gone — then +`pre-commit run --all-files`. Because this lands on a +branch already validated on hardware, it closes with a rig re-check: the invariant check against a +real report pair and a scoped `--accumulate` run, not the full fleet. diff --git a/test/hil/helper/hil_health.py b/test/hil/helper/hil_health.py index 92f0accc8..aa811eeb4 100644 --- a/test/hil/helper/hil_health.py +++ b/test/hil/helper/hil_health.py @@ -1,6 +1,6 @@ #!/usr/bin/env python3 # SPDX-License-Identifier: MIT -"""Shutting a wedged HIL run down: kill what the workers spawned, then report. +"""Shutting a wedged HIL run down: kill what the workers spawned. A device whose usbfs node is held by a D-state process cannot be freed -- SIGKILL is not delivered in uninterruptible sleep -- so the goal is never to fix the rig from here. It is @@ -341,40 +341,3 @@ def kill_pool_children(pool, *extra) -> int: # SIGKILL is asynchronous and a D-state task ignores it: only a confirmed survivor # justifies the caller's power-cycle wording return len(_kill_and_confirm(killed_pids)) if killed_pids else 0 - - -def write_timeout_report(report_dir: Path, boards, secs: int, md_name: str, - banner: str = '', prefix: str = '') -> None: - """Leave a report behind when the worker pool has to be abandoned. - - map_async is all-or-nothing, so a timeout loses every per-board result and the report - dir would stay empty with no reason for the failure. Any prior attempt's markdown is - kept below the banner.""" - # `prefix` carries the preflight rig-health verdict: the timeout aborts before - # accumulate_report, so without it the report loses the one line saying WHY the pool - # never finished. The '\n' stops Markdown lazy continuation pulling the banner into - # the blockquote. - try: - # Built INSIDE the try: a roster entry without a 'name' key raises KeyError while - # assembling the board list, and outside the try that escaped and stranded the - # runner -- which is exactly what the broad handler below exists to prevent. - head = (prefix + '\n' if prefix else '') + (banner or ( - f'**HIL run abandoned: worker pool timed out after {secs}s.**\n\n' - f'No per-board results could be collected for this attempt, so the ' - f'table below (if any) is from an earlier one. Boards dispatched:\n\n' - + '\n'.join(f'- {b.get("name", "?")}' for b in boards) + '\n')) - report_dir.mkdir(parents=True, exist_ok=True) - md_path = report_dir / md_name - # Its own handler so it cannot take the write down with it: a report torn by an - # attempt killed mid-write raises UnicodeDecodeError (a ValueError, and prior - # reports always contain status emoji), which under a shared try skipped the write - # entirely. Losing the old table is a nicety; losing the banner is the failure. - try: - prior = md_path.read_text(encoding='utf-8') if md_path.is_file() else '' - except (OSError, ValueError): - prior = '' - md_path.write_text(head + (f'\n{prior}' if prior else ''), encoding='utf-8') - except Exception as e: # noqa: BLE001 - # Deliberately broad: this is the first statement of the pool-abandon path, so ANY - # escape skips kill_pool_children and os._exit and strands the runner. - _p(f'warning: cannot write {md_name} to {report_dir}: {e}', flush=True) diff --git a/test/hil/helper/hil_report.py b/test/hil/helper/hil_report.py new file mode 100644 index 000000000..b73c030a9 --- /dev/null +++ b/test/hil/helper/hil_report.py @@ -0,0 +1,544 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: MIT +"""The HIL report document: one owner for hil_report.json and hil_report.md. + +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 +that document: the cell vocabulary, the one classifier both artifacts share, rendering, the +writers, and the fold to one machine-readable verdict per board. + +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. +""" +import argparse +import json +import sys +from pathlib import Path + + + +def _p(*args, **kwargs) -> None: + """Print that cannot raise. Defined here rather than imported from hil_health: this + module is ALSO run as a script (hil-operator.md invokes it by path), and under + PYTHONSAFEPATH=1 -- which the suite's own MTP fixtures set -- sys.path[0] is not the + script dir, so any sibling import dies before argparse runs. Five lines beat that.""" + try: + print(*args, **kwargs) + except (OSError, ValueError): + # ValueError too: printing to a CLOSED stream raises "I/O operation on closed + # file", and escaping here skips the containment path's os._exit. + pass + +REPORT_MD = 'hil_report.md' +REPORT_JSON = 'hil_report.json' +# The status vocabulary, shared by the code that WRITES a cell (hil_test's test runners) and +# the code that reads one back (cell_state). One dict, so the human's table and the agent's +# verdict cannot drift apart. +REPORT_CELL = {'pass': '✅', 'fail': '❌', 'skip': '⚪'} +BOUNDARY_CELL = 'same-PID boundary' +LOCKED_CELL = 'board-locked' +# A pseudo-test column, not a real one: write_timeout_report marks the boards that were +# still dispatched when the pool guard fired. accumulate_report clears it on a retry. +POOL_TIMEOUT_CELL = 'pool-timeout' + + +def _load(report_dir: Path) -> tuple: + """(doc, readable) for the sidecar, coerced to the canonical shape. + + hil_ci.sh uploads a sidecar as the --accumulate merge base, so a non-conforming one is + reachable from OUTSIDE the harness -- and every writer here runs on a path where a + TypeError costs the whole report. Coerce once, at the boundary, instead of guarding + each use: `banner: null` used to kill a fully successful run with a traceback and no + artifact at all, and `cells: null` sent write_timeout_report down its fallback so a + board that ate the whole pool guard was published as a pass. + + `readable` is False only when a sidecar EXISTS but could not be parsed, or is absent -- + both mean its rows are unrecoverable, which callers use to avoid destroying a markdown + that may still hold them.""" + jpath = report_dir / REPORT_JSON + if not jpath.is_file(): + return {'rows': [], 'banner': '', 'scope': '', 'caveat': ''}, False + try: + raw = json.loads(jpath.read_text()) + if not isinstance(raw, dict): + raise ValueError('sidecar is not an object') + except (OSError, ValueError, TypeError): + return {'rows': [], 'banner': '', 'scope': '', 'caveat': ''}, False + rows = [] + # isinstance, not `or []`: a sidecar with `rows: 1` iterates an int and raises outside + # the parse handler above. + for r in (raw.get('rows') if isinstance(raw.get('rows'), list) else []): + if not isinstance(r, dict) or 'board' not in r: + continue + cells = r.get('cells') + dur = r.get('duration') + # VALUES as well as keys: render_matrix does REPORT_CELL.get(v, v), which raises + # TypeError on an unhashable value, and cell_state does v.startswith. A non-str + # cell is corrupt, and dropping it renders blank -- "not run" -- which is the + # honest reading. Coercing it to str would make it classify as a PASS. + rows.append({'board': str(r['board']), + 'cells': {str(k): v for k, v in cells.items() if isinstance(v, str)} + if isinstance(cells, dict) else {}, + 'duration': dur if isinstance(dur, str) else None}) + text = lambda k: raw[k] if isinstance(raw.get(k), str) else '' + return {'rows': rows, 'banner': text('banner'), 'scope': text('scope'), + 'caveat': text('caveat')}, True + + +def cell_state(v) -> str: + """'pass' | 'fail' | 'skip' for one report cell. + + THE classifier -- the markdown tally and the per-board verdict both call this, so they + cannot disagree. 'fail' or a fail-icon prefix is a failure, 'skip' or a skip-icon prefix + is a skip, and EVERYTHING ELSE is a pass. That last arm is load-bearing: a passing test + may return a plain metric string ('480.0 MBps') that lands in the cell unprefixed, while + failures are guaranteed marked -- TestFail's docstring pins that its metric is + icon-prefixed precisely so render and tally treat it as a failure. Classifying unknown + shapes as fail here would publish a green table as a red verdict. + + isinstance-guarded: cells are usually str but a caller may hand over None or a number, + and .startswith on those raises inside a report writer that must not raise.""" + if v == 'fail' or (isinstance(v, str) and v.startswith(REPORT_CELL['fail'])): + return 'fail' + if v == 'skip' or (isinstance(v, str) and v.startswith(REPORT_CELL['skip'])): + return 'skip' + return 'pass' + + +def render_matrix(rows_all: list) -> str: + """Render rows (list of (row_label, {example: status}, duration)) as an aligned + markdown matrix: columns = tests (bare names) centered, boards left-aligned, + per-row duration as the trailing column.""" + seen = set() + for _, cells, _ in rows_all: + seen.update(cells) + if not seen: + return 'No tests were run.' + + # metric-bearing columns pinned first, the rest alphabetical: stable regardless of the + # shuffled execution order + pinned = ['usbtest', 'cdc_msc_throughput', 'msc_file_explorer', 'msc_file_explorer_freertos'] + + def col_key(t): + name = t.rsplit('/', 1)[-1] + return (pinned.index(name) if name in pinned else len(pinned), name, t) + + columns = sorted(seen, key=col_key) + headers = [c.rsplit('/', 1)[-1] for c in columns] + ['duration'] # bare example names + + def cell(cells, col): + v = cells.get(col) + if v is None: + return '' + return REPORT_CELL.get(v, v) # status symbol, or a metric string (e.g. speed) verbatim + + rows_vals = [(lbl, [cell(cells, c) for c in columns] + [dur or '']) + for lbl, cells, dur in rows_all] + board_hdr = 'Board' + board_w = max([len(board_hdr)] + [len(lbl) for lbl, _ in rows_vals]) + col_w = [max([len(h)] + [len(vals[i]) for _, vals in rows_vals]) + for i, h in enumerate(headers)] + + def line(label, values): + padded = [label.ljust(board_w)] + [v.center(w) for v, w in zip(values, col_w)] + return '| ' + ' | '.join(padded) + ' |' + + header = line(board_hdr, headers) + sep = '| ' + '-' * board_w + ' | ' + ' | '.join(':' + '-' * (w - 2) + ':' for w in col_w) + ' |' + body = [line(lbl, vals) for lbl, vals in rows_vals] + + # tally run cells (not-run cells are absent from the dicts). A cell is a bare status or + # a metric string carrying its own icon ("❌ 29/30"), so classify by the leading icon -- + # through cell_state, the same call the per-board verdict makes. + kinds = [cell_state(v) for _, cells, _ in rows_all for v in cells.values()] + failed = kinds.count('fail') + skipped = kinds.count('skip') + passed = kinds.count('pass') + summary = (f'**{REPORT_CELL["pass"]} {passed} passed · {REPORT_CELL["fail"]} {failed} failed · ' + f'{REPORT_CELL["skip"]} {skipped} skipped · blank not run**') + + return summary + '\n\n' + '\n'.join([header, sep] + body) + + +def render_report(doc: dict) -> str: + """The markdown IS a rendering of the sidecar. Every writer goes through here, so a + table can never contain something the JSON does not.""" + # .get throughout, not subscripts: mark_report_abandoned renders a sidecar it did NOT + # write (hil_ci.sh reuses a persistent REMOTE_DIR, so it may be an older version's or + # a torn one) on the way to os._exit, and a KeyError there is not in its handler -- + # it would unwind into multiprocessing's unbounded join and hang the runner it is + # trying to free. Same reason summarize() below reads cells as `r.get('cells') or {}`. + md = render_matrix([(r.get('board', '?'), r.get('cells') or {}, r.get('duration')) + for r in doc.get('rows') or [] if isinstance(r, dict)]) + if doc.get('scope'): + # a scoped run's small table is otherwise indistinguishable from a full one, and + # it replaces the previous full table in the sticky PR comment + md = f'_Scoped run: {doc["scope"]}. Boards/tests not listed were not run._\n\n' + md + # banner, then caveat: a rig-health caveat outranks the table AND the scope note, and an + # abandon notice outranks even that -- the top of the report is where hil/SKILL.md tells + # the agent to look + if doc.get('banner'): + md = doc['banner'] + '\n' + md + if doc.get('caveat'): + md = doc['caveat'] + '\n' + md + return md + + +def write_report(report_dir: Path, doc: dict) -> None: + """Write both artifacts from one document. + + RAISES on failure, deliberately: every caller is on a path whose own handler exists to + report exactly this (write_timeout_report's _p warning, hil_test's fallback-of-the- + fallback). Swallowing OSError here made both of those dead code, so an unwritable or + root-owned report dir produced no artifact AND no message. + + Renders BEFORE writing anything: committing the JSON first and then raising in + render_report left a sidecar saying "abandoned" beside a markdown still reading as a + clean green table -- the one invariant this module exists to hold.""" + md = render_report(doc) + '\n' + 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(md, encoding='utf-8') + + +def _abandon_notice(why: str) -> str: + # Wording is a CONTRACT: .claude/skills/hil/SKILL.md pins this banner as the case where + # "the table below IS this run's ... Report the results AND the abandonment". Calling + # the table partial would send the reading agent to re-run boards that already passed. + return (f'**HIL run abandoned: {why}** The table below was collected before the ' + f'abandon; treat board results as unverified.\n') + + +def _already_abandoned(doc: dict) -> bool: + """Whether THIS attempt already recorded how it ended. + + `caveat` only. It used to check `banner` too, because hil_test.py folded its abandon + notices in there -- but banner is carried across an --accumulate retry by design, so a + stale notice from an earlier attempt silenced a genuinely new abandon and the run's own + failure went unrecorded. banner now carries rig HEALTH (which describes the conditions + the cells were collected under, and so must persist); caveat carries the run's OUTCOME + (which must not).""" + return '**HIL run ab' in doc.get('caveat', '') + + +def _stamp_markdown(report_dir: Path, notice: str) -> None: + """Last line of defence: prepend the notice to the markdown itself. + + pr_comment.yml cats only hil_report.md, so a path that gives up here publishes a clean + green table under an abandoned, non-zero job. Master did this unconditionally.""" + mpath = report_dir / REPORT_MD + if not mpath.is_file(): + return + # errors='replace' and catch ValueError: a torn report or a LANG=C locale raises + # UnicodeDecodeError -- NOT an OSError -- straight past os._exit. + body = mpath.read_text(encoding='utf-8', errors='replace') + if '**HIL run ab' not in body[:2000]: + mpath.write_text(notice + '\n' + body, encoding='utf-8') + + +def mark_report_abandoned(report_dir: Path, why: str) -> None: + """Stamp an existing report as abandoned, in BOTH artifacts. + + Best-effort and silent: this runs while the interpreter is being torn down, and an + exception here hangs the process in multiprocessing's unbounded join().""" + notice = _abandon_notice(why) + try: + doc, readable = _load(report_dir) + if readable: + if _already_abandoned(doc): + return # whoever got there first wins, WRITE included + doc['caveat'] = notice + write_report(report_dir, doc) + return + except (OSError, ValueError, TypeError, AttributeError): + pass # fall through -- a failure here must not cost the stamp entirely + # 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) + except (OSError, ValueError, TypeError, AttributeError): + pass + + +def mark_report_no_boards(report_dir: Path, msg: str, fresh: bool = True) -> None: + """Record that the board filters intersected to nothing. + + `fresh` mirrors hil_test's own flag, because this runs BEFORE the fresh wipe: without + it a fresh run whose filter emptied re-published the PREVIOUS run's green rows under + this run's red job -- the stale-table failure it exists to prevent. An --accumulate run + keeps them, since nothing this attempt did invalidates them.""" + try: + doc, _ = _load(report_dir) + if not fresh and _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. + # Only while ACCUMULATING, though: this runs before the fresh wipe, so guarding + # a fresh run would leave the previous attempt's rows AND its abandon notice + # published as this run's. + return + # A fresh run carries NOTHING from the prior sidecar -- rows, banner and scope + # alike, matching accumulate_report, which builds from an empty prior when fresh. + # Resetting only rows republished a stale rig-health note and a stale scope line + # under this run's notice, from a leftover or uploaded sidecar. + prior = {'rows': [], 'banner': '', 'scope': ''} if fresh else doc + write_report(report_dir, {'rows': prior['rows'], 'banner': prior['banner'], + 'scope': prior['scope'], + 'caveat': f'**HIL run selected no boards.** {msg}\n'}) + except (OSError, ValueError, TypeError, AttributeError): + pass # loud on stdout already; the exit code is what the job reads + + +def accumulate_report(mret: list, report_dir: Path, fresh: bool, scope: str = '', + banner: str = '', caveat: str = '') -> str: + """Merge this run's results into json in report_dir, then (re)write + the markdown matrix to md. `fresh` (a first run, no --accumulate) + starts a new report; otherwise a re-run accumulates so boards/tests that + already passed are preserved while re-run cells are updated. `scope` names the + board filter, if any, so a scoped table is not mistaken for a full one. + Returns the md. + + `mret` is hil_test.py's worker-result shape (name, err, fts, rows, ...), so this one + function knows something about its caller that the rest of the module does not. Folding + mret into rows could live in hil_test and only the merge here, but that would rewrite + the subtle parts -- stale board-locked clearing, BOUNDARY_CELL dropping, duration=None + preservation -- for a tidier seam. Data-shape coupling, not an import cycle.""" + # ONE canonical load: a sidecar reaching here may have been uploaded by hil_ci.sh as + # the merge base, so it is untrusted input. `banner` carries forward -- it describes + # the conditions the earlier cells were collected under, and the .failed spec re-runs + # only FAILURES so those passes are never re-earned. `caveat` does NOT: it records how + # a RUN ENDED, and this attempt has not ended yet. Carrying it made a clean retry + # publish "HIL run abandoned" over a run where nothing was abandoned. + prior = {'rows': [], 'banner': ''} + if not fresh: + prior, _ = _load(report_dir) + acc = {r['board']: [dict(r['cells']), r['duration']] for r in prior['rows']} + prior_banner = prior['banner'] + + # current cells override prior for boards/tests that ran; a filtered run reports + # duration None, keeping the previous full-run value + for name, _, _, rows, *_ in mret: + if rows and not any(LOCKED_CELL in cells for _, cells, _ in rows): + # board ran for real: clear a stale lock-failure cell (its row is keyed by + # board name; test rows may be variant names) + stale = acc.get(name) + if stale is not None: + stale[0].pop(LOCKED_CELL, None) + # and the pool-timeout mark: write_timeout_report stamps it on a board that + # never reported, and update() below MERGES, so without this a board that + # passed clean on the retry kept a red cell for ever. + stale[0].pop(POOL_TIMEOUT_CELL, None) + if not stale[0]: + # variant-keyed boards never repopulate the board-name row, so drop it + # or it renders as a blank ghost row + del acc[name] + for row_label, cells, dur in rows: + row = acc.setdefault(row_label, [{}, None]) + # a row that ran is no longer pool-timed-out, whatever it is keyed by + row[0].pop(POOL_TIMEOUT_CELL, None) + # the boundary cell is only ever written on failure, so a re-run of this + # variant that cleared the boundary must drop the previous attempt's ❌ + if BOUNDARY_CELL not in cells: + row[0].pop(BOUNDARY_CELL, None) + row[0].update(cells) + if dur is not None: + row[1] = dur + + report_dir.mkdir(parents=True, exist_ok=True) + # by LINE, deduped: attempts repeat the same caveat far more often than they add a new + # one, and three copies of the D-state note reads as three incidents + seen, merged = set(), [] + for line in (prior_banner + banner).splitlines(): + if line.strip() and line not in seen: + seen.add(line) + merged.append(line) + banner = '\n'.join(merged) + '\n' if merged else '' + doc = {'rows': [{'board': k, 'cells': c, 'duration': d} for k, (c, d) in acc.items()], + 'banner': banner, 'scope': scope, 'caveat': caveat} + # through write_report, not hand-rolled: writing the JSON and only then rendering is + # the ordering write_report exists to forbid -- a render failure left the sidecar ahead + # of the markdown, which is the one invariant this module holds. + write_report(report_dir, doc) + return render_report(doc) + + +def _write_stuck_over_prior_md(report_dir: Path, doc: dict) -> None: + """Sidecar unrecoverable: rebuild it from the stuck rows alone, but leave the + markdown's existing table beneath the caveat rather than throwing real results away. + + The one place the md-is-a-rendering-of-the-json invariant is deliberately suspended, + because there is no readable json left for it to be a rendering of.""" + try: + prior = (report_dir / REPORT_MD).read_text(encoding='utf-8') + except (OSError, ValueError): + prior = '' + # Say so explicitly: those rows exist only as rendered text, so no later --accumulate + # can merge them back. Claiming the sidecar represents them would be false. + note = ('_The table below is a previous attempt\'s rendered output. The sidecar could ' + 'not be read, so those rows are NOT in it and will not survive another run._\n') + head = (doc['banner'] + '\n' if doc['banner'] else '') + doc['caveat'] + '\n' + note + 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') + + +def write_timeout_report(report_dir: Path, boards, secs: int, + banner: str = '', prefix: str = '') -> None: + """Leave a report behind when the worker pool has to be abandoned. + + map_async is all-or-nothing, so a timeout loses every per-board result and the report + dir would stay empty with no reason for the failure. Any prior attempt's rows are kept + and each stuck board is marked with a POOL_TIMEOUT_CELL beside them. + + `prefix` is the preflight rig-health verdict and goes to the BANNER, where rig health + lives and where an --accumulate retry carries it forward; the abandon notice goes to + the caveat, which does not carry. Folding both into the caveat is what made a clean + retry report an abandonment that had not happened.""" + try: + # names INSIDE the try: a roster entry that is not a dict raises here, and outside + # it that escaped and stranded the runner. + names = [b.get('name', '?') if isinstance(b, dict) else '?' for b in boards] + caveat = banner or ( + f'**HIL run abandoned: worker pool timed out after {secs}s.**\n\n' + f'No per-board results could be collected for this attempt. Rows other than ' + f'the {POOL_TIMEOUT_CELL} cells below are from an earlier attempt. Boards ' + f'dispatched:\n\n' + '\n'.join(f'- {n}' for n in names) + '\n') + doc, readable = _load(report_dir) + rows = doc['rows'] + by_board = {r['board']: r for r in rows} + for name in names: + row = by_board.get(name) + if row is None: + rows.append({'board': name, 'cells': {POOL_TIMEOUT_CELL: 'fail'}, + 'duration': None}) + else: + # _load guarantees `cells` is a dict, so a null-cells row from an uploaded + # sidecar can no longer send this down the fallback and publish a board + # that ate the whole pool guard as a pass. + row['cells'][POOL_TIMEOUT_CELL] = 'fail' + out = {'rows': rows, 'scope': doc['scope'], 'caveat': caveat, + 'banner': ((doc['banner'] + prefix) if prefix not in doc['banner'] + else doc['banner'])} + if not readable and (report_dir / REPORT_MD).is_file(): + # `readable` covers ABSENT as well as torn: an absent sidecar beside an intact + # markdown used to re-render from the stuck row alone and destroy real results. + _write_stuck_over_prior_md(report_dir, out) + return + write_report(report_dir, out) + except Exception as e: # noqa: BLE001 + # Deliberately broad: this is the first statement of the pool-abandon path, so ANY + # escape skips kill_pool_children and os._exit and strands the runner. + _p(f'warning: cannot write {REPORT_MD} to {report_dir}: {e}', flush=True) + try: + # Same wording as above and the same guarded name extraction -- the fallback + # used to re-derive b.get("name") outside any try and raise identically, so a + # malformed roster left NO artifact at all. + names = [b.get('name', '?') if isinstance(b, dict) else '?' for b in boards] + head = (prefix + '\n' if prefix else '') + (banner or ( + f'**HIL run abandoned: worker pool timed out after {secs}s.**\n\n' + f'No per-board results could be collected for this attempt, so the table ' + f'below (if any) is from an earlier one. Boards dispatched:\n\n' + + '\n'.join(f'- {n}' for n in names) + '\n')) + try: + prior = (report_dir / REPORT_MD).read_text(encoding='utf-8') + except (OSError, ValueError): + prior = '' + report_dir.mkdir(parents=True, exist_ok=True) + (report_dir / REPORT_MD).write_text( + head + (f'\n{prior}' if prior else ''), encoding='utf-8') + except Exception as e2: # noqa: BLE001 + _p(f'warning: fallback {REPORT_MD} write failed too: {e2}', flush=True) + + +def variants_of(cfg: dict, board: str) -> list: + for b in cfg.get('boards', []): + if b['name'] == board: + return [v['name'] for v in (b.get('variant') or [])] or [board] + return [board] + + +def summarize(cfg: dict, boards: list, report: dict) -> dict: + # .get, not a subscript: this is the one reader an agent's verdict depends on, and a + # row without 'board' used to kill the CLI with a traceback and no results at all -- + # hil-validate.js then reports every board as "hil-operator returned no entry". + rows = {r['board']: r.get('cells') or {} + for r in (report.get('rows') or []) + if isinstance(r, dict) and 'board' in r} + owner = {v['name']: b['name'] for b in cfg.get('boards', []) + for v in (b.get('variant') or [])} + results = [] + for board in boards: + names = variants_of(cfg, board) + mine = {n: rows[n] for n in names if n in rows} + # a variant name that is neither declared nor prefixed cannot be attributed; the + # `<board>-` fallback only helps ad-hoc builds, it is not the primary path. It must + # also never steal a row DECLARED by another board: a declared variant need not start + # with its own board's name, so it may happen to start with this board's name plus '-'. + mine.update({n: c for n, c in rows.items() + if n.startswith(f'{board}-') and n not in mine + and owner.get(n, board) == board}) + # the BOARD-name row too: hil_test writes lock contention and pool timeouts keyed + # by board name, but variants_of returns only DECLARED variant names -- and + # nanoch32v203 / ch32v307v_r1_1v0 declare none equal to their board name. Without + # this those rows are invisible, so a lock held by concurrent CI is published as a + # hardware FAIL and hil-validate.js never retries it. + if board in rows and board not in mine: + mine[board] = rows[board] + if not mine: + results.append({'board': board, 'ran': False, 'pass': False, 'locked': False, + 'detail': 'no report row for this board'}) + continue + # a wedge outranks lock contention: `locked` short-circuits `detail` below, so a + # stale board-locked cell from an earlier attempt used to mask the pool-timeout + # cell the retry added -- publishing a board that hung the rig as LOCKED, which + # hil-validate.js then RE-RUNS, paying another pool guard on it. + wedged = any(POOL_TIMEOUT_CELL in cells for cells in mine.values()) + locked = not wedged and any(LOCKED_CELL in cells for cells in mine.values()) + bad = [] + for vname, cells in sorted(mine.items()): + for test, val in sorted(cells.items()): + if test == LOCKED_CELL: + continue + if cell_state(val) == 'fail': + bad.append(f'{vname} {test}: {val}') + ok = not bad and not locked + if locked: + detail = 'held by another holder; not flashed' + elif bad: + detail = '; '.join(bad) + else: + detail = f'{len(mine)} variant(s), {sum(len(c) for c in mine.values())} cell(s) ok' + results.append({'board': board, 'ran': True, 'pass': ok, 'locked': locked, + 'detail': detail}) + # `caveat` too: an abandoned or no-boards run says so THERE, and this JSON is all + # an agent gets -- leaving it in the sidecar puts it back where only a human looks. + return {'results': results, 'banner': report.get('banner', ''), + 'caveat': report.get('caveat', '')} + + +def main() -> int: + ap = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + ap.add_argument('config_file') + ap.add_argument('-b', '--board', action='append', default=[], + help='boards to report on; default: every board in the config') + ap.add_argument('--report-dir', default='.', help=f'where {REPORT_JSON} lives (default: cwd)') + a = ap.parse_args() + + cfg = json.loads(Path(a.config_file).read_text()) + boards = a.board or [b['name'] for b in cfg.get('boards', [])] + jpath = Path(a.report_dir) / REPORT_JSON + if not jpath.is_file(): + print(f'error: {jpath} not found -- did hil_test.py run in this directory?', + file=sys.stderr) + return 1 + # through _load, like every writer: feeding raw JSON to summarize left the one reader an + # agent's verdict depends on crashing on the malformed sidecars the writers tolerate. + doc, _ = _load(Path(a.report_dir)) + json.dump(summarize(cfg, boards, doc), sys.stdout, indent=2) + print() + return 0 + +if __name__ == '__main__': + sys.exit(main()) diff --git a/test/hil/helper/hil_summary.py b/test/hil/helper/hil_summary.py deleted file mode 100644 index e566bead0..000000000 --- a/test/hil/helper/hil_summary.py +++ /dev/null @@ -1,115 +0,0 @@ -#!/usr/bin/env python3 -# SPDX-License-Identifier: MIT -"""Fold hil_report.json into one machine-readable verdict per BOARD. - -A workflow driving hil_test.py through an operator agent has no filesystem access, so the -agent has to carry the results across. It must carry them, not retype them: the previous -design asked the agent to transcribe the markdown table, and every defect found in four -review rounds came from re-parsing that prose -- variant row names vs board names, -`board locked` vs `board-locked`, folding several variant rows into one verdict, rows that -matched no board. All of it is a join, and the join belongs here, where the roster is. - -Report rows are named per VARIANT (hil_test.py builds them from `vname`), and a variant name -is not required to start with the board name -- nanoch32v203 produces only `-fsdev`/`-usbfs`, -ch32v307v_r1_1v0 only `-usbhs`/`-usbfs`. The config is what maps them back. - -Emits, on stdout: - {"results": [{"board", "ran", "pass", "locked", "detail"}...], "banner": str} - -`locked` is a field, not a prefix to grep for. `ran` false means the board produced no row at -all, which is not the same as failing. - -Usage: hil_summary.py <config.json> [-b BOARD]... [--report-dir DIR] -""" -import argparse -import json -import sys -from pathlib import Path - -FAIL_ICON, SKIP_ICON = '❌', '⚪' # a pass needs no icon: unmarked = pass -LOCKED_CELL = 'board-locked' - - -def cell_state(v: str) -> str: - """'pass' | 'fail' | 'skip' -- the EXACT classifier hil_test.py's own tally uses - (cell_kind in render_matrix): 'fail' or a ❌ prefix is a failure, 'skip' or a ⚪ - prefix is a skip, and EVERYTHING ELSE is a pass. That last arm is load-bearing: a - passing test may return a plain metric string ('480.0 MBps') that lands in the cell - unprefixed, while failures are guaranteed marked -- TestFail's docstring pins that its - metric is icon-prefixed precisely so render/tally treat it as a failure. Classifying - unknown shapes as fail here would publish a green table as a red verdict.""" - if v == 'fail' or v.startswith(FAIL_ICON): - return 'fail' - if v == 'skip' or v.startswith(SKIP_ICON): - return 'skip' - return 'pass' - - -def variants_of(cfg: dict, board: str) -> list: - for b in cfg.get('boards', []): - if b['name'] == board: - return [v['name'] for v in (b.get('variant') or [])] or [board] - return [board] - - -def summarize(cfg: dict, boards: list, report: dict) -> dict: - rows = {r['board']: r.get('cells') or {} for r in report.get('rows', [])} - owner = {v['name']: b['name'] for b in cfg.get('boards', []) - for v in (b.get('variant') or [])} - results = [] - for board in boards: - names = variants_of(cfg, board) - mine = {n: rows[n] for n in names if n in rows} - # a variant name that is neither declared nor prefixed cannot be attributed; the - # `<board>-` fallback only helps ad-hoc builds, it is not the primary path. It must - # also never steal a row DECLARED by another board: a declared variant need not start - # with its own board's name, so it may happen to start with this board's name plus '-'. - mine.update({n: c for n, c in rows.items() - if n.startswith(f'{board}-') and n not in mine - and owner.get(n, board) == board}) - if not mine: - results.append({'board': board, 'ran': False, 'pass': False, 'locked': False, - 'detail': 'no report row for this board'}) - continue - locked = any(LOCKED_CELL in cells for cells in mine.values()) - bad = [] - for vname, cells in sorted(mine.items()): - for test, val in sorted(cells.items()): - if test == LOCKED_CELL: - continue - if cell_state(str(val)) == 'fail': - bad.append(f'{vname} {test}: {val}') - ok = not bad and not locked - if locked: - detail = 'held by another holder; not flashed' - elif bad: - detail = '; '.join(bad) - else: - detail = f'{len(mine)} variant(s), {sum(len(c) for c in mine.values())} cell(s) ok' - results.append({'board': board, 'ran': True, 'pass': ok, 'locked': locked, - 'detail': detail}) - return {'results': results, 'banner': report.get('banner', '')} - - -def main() -> int: - ap = argparse.ArgumentParser() - ap.add_argument('config_file') - ap.add_argument('-b', '--board', action='append', default=[], - help='boards to report on; default: every board in the config') - ap.add_argument('--report-dir', default='.', help='where hil_report.json lives (default: cwd)') - a = ap.parse_args() - - cfg = json.loads(Path(a.config_file).read_text()) - boards = a.board or [b['name'] for b in cfg.get('boards', [])] - jpath = Path(a.report_dir) / 'hil_report.json' - if not jpath.is_file(): - 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) - print() - return 0 - - -if __name__ == '__main__': - sys.exit(main()) diff --git a/test/hil/hil_ci.sh b/test/hil/hil_ci.sh index 514b0f174..daa787242 100644 --- a/test/hil/hil_ci.sh +++ b/test/hil/hil_ci.sh @@ -210,6 +210,69 @@ rm -rf -- "$1" mkdir -p -- "$1/test/hil/helper" "$1/examples" REMOTE +# The --accumulate merge base. The wipe above just cleared REMOTE_DIR, and +# accumulate_report merges onto the sidecar in the RUN's cwd (hil_test.py:2193 sets +# `fresh = not args.accumulate`, and only a non-fresh run reads it) -- so without this a +# remote retry starts from nothing and its one-row table REPLACES the full-fleet one it was +# meant to extend. The copy-back at the end of this script has always existed; this is the +# other half of it. +# +# Gated, not unconditional: a fresh run unlinks the sidecar anyway (hil_test.py:2244), so +# uploading there is wasted work that also obscures what the wipe means. +# +# <config>.failed is deliberately NOT uploaded: hil_test.py only ever writes it, never +# reads it -- the retry spec reaches the rig as the -b/-bt arguments the caller expanded +# from it (`hil_ci.sh $(cat <config>.failed)`). +# argparse decides, not a case arm: hil_test.py declares `-a, --accumulate`, so argparse +# also accepts `-av`, `-va`, `--accum` and `--acc` -- and hil-validate.js tells the +# operator to retry "adding -v", which makes `-av` the natural spelling. A hand-rolled +# match missed all four: no upload, and the else-branch warning never fired either, so the +# one-row table replaced the full-fleet one in silence. +ACCUMULATE=$(python3 - ${ARGS[@]+"${ARGS[@]}"} <<'PY' +import argparse, sys +p = argparse.ArgumentParser(add_help=False) +p.add_argument('-a', '--accumulate', action='store_true') +p.add_argument('-v', '--verbose', action='store_true') # so -av/-va bundle as they do there +print(1 if p.parse_known_args(sys.argv[1:])[0].accumulate else 0) +PY +) || ACCUMULATE=0 +if [ "$ACCUMULATE" = 1 ]; then + if [ -f "$ROOT_DIR/hil_report.json" ]; then + # Provenance: hil_report.json is not namespaced by CONFIG or REMOTE (build.yml and + # pr_comment.yml read that exact name), so a `REMOTE=hifiphile CONFIG=.../hfp.json` + # run leaves an hfp sidecar behind that a later ci.lan retry would merge, publishing + # boards that never ran here. Require at least one row to belong to THIS roster. + if python3 - "$ROOT_DIR/hil_report.json" "$CONFIG" <<'PY' +import json, sys +try: + rows = json.load(open(sys.argv[1])).get('rows') or [] + cfg = json.load(open(sys.argv[2])).get('boards') or [] +except Exception: + sys.exit(1) +known = set() +for b in cfg: + known.add(b.get('name')) + known.update(v.get('name') for v in (b.get('variant') or [])) +sys.exit(0 if not rows or any(r.get('board') in known for r in rows if isinstance(r, dict)) + else 1) +PY + then + echo "==> Uploading hil_report.json as the --accumulate merge base" + scp -q "$ROOT_DIR/hil_report.json" "$REMOTE:$REMOTE_DIR/" + else + echo "==> warning: $ROOT_DIR/hil_report.json holds no board from $(basename "$CONFIG")" \ + "-- it is from another rig or config, so it is NOT being uploaded; this run's" \ + "table will REPLACE rather than extend" >&2 + fi + else + # Loud, because this is the failure mode: the run still succeeds, and quietly + # publishes a small table where a full one used to be. + echo "==> warning: --accumulate was requested but $ROOT_DIR/hil_report.json does not" \ + "exist, so there is nothing to merge onto -- this run's table will REPLACE the" \ + "previous one rather than extend it" >&2 + fi +fi + # Copy HIL test script and config echo "==> Copying test scripts" scp -q "$ROOT_DIR/test/hil/hil_test.py" \ @@ -223,7 +286,7 @@ scp -q "$ROOT_DIR/test/hil/helper/__init__.py" \ "$ROOT_DIR/test/hil/helper/hil_util.py" \ "$ROOT_DIR/test/hil/helper/hil_health.py" \ "$ROOT_DIR/test/hil/helper/hil_lock.py" \ - "$ROOT_DIR/test/hil/helper/hil_summary.py" \ + "$ROOT_DIR/test/hil/helper/hil_report.py" \ "$REMOTE:$REMOTE_DIR/test/hil/helper/" # Copy only firmware binaries (elf/bin/hex) plus esptool metadata @@ -316,19 +379,41 @@ REMOTE # Copy the generated report back to the local checkout (best-effort; the run's # exit code is preserved regardless of whether a report was produced). -scp -q "$REMOTE:$REMOTE_DIR/hil_report.md" "$ROOT_DIR/hil_report.md" \ - && echo "==> Report copied to $ROOT_DIR/hil_report.md" \ - || echo "==> warning: no hil_report.md copied back" >&2 +# rm -f FIRST, exactly as the sidecar loop below does: the markdown and the JSON are two +# halves of ONE document now, so leaving a stale table behind when the copy fails -- beside +# a sidecar that was correctly removed -- publishes last run's green results under this +# run's red job, and the operator's hil_report.py call exits 1 against the missing sidecar. +# Fetch BOTH halves to temps and commit them as a pair. Separate fetch/rename meant a +# markdown that arrived beside a sidecar that did not left the local pair failing the +# rendering invariant, and the next --accumulate retry merging the wrong base. Deleting +# first and then scp'ing was worse still: an ssh drop at the end of a 60-minute run +# destroyed the report outright. +md_ok=0; json_ok=0 +scp -q "$REMOTE:$REMOTE_DIR/hil_report.md" "$ROOT_DIR/hil_report.md.tmp" 2>/dev/null \ + && [ -f "$ROOT_DIR/hil_report.md.tmp" ] && md_ok=1 +scp -q "$REMOTE:$REMOTE_DIR/hil_report.json" "$ROOT_DIR/hil_report.json.tmp" 2>/dev/null \ + && [ -f "$ROOT_DIR/hil_report.json.tmp" ] && json_ok=1 +if [ "$md_ok" = 1 ] && [ "$json_ok" = 1 ]; then + mv -f "$ROOT_DIR/hil_report.md.tmp" "$ROOT_DIR/hil_report.md" + mv -f "$ROOT_DIR/hil_report.json.tmp" "$ROOT_DIR/hil_report.json" + echo "==> Report copied to $ROOT_DIR/hil_report.md (+ sidecar)" +else + rm -f "$ROOT_DIR/hil_report.md.tmp" "$ROOT_DIR/hil_report.json.tmp" + # All or nothing: a half-copied pair is worse than none. The stale local markdown goes + # because that is what gets pasted into a PR as this run's results; the stale sidecar + # goes with it so the two cannot disagree. + rm -f "$ROOT_DIR/hil_report.md" "$ROOT_DIR/hil_report.json" + echo "==> warning: report copy-back incomplete (md=$md_ok json=$json_ok); removed the" \ + "stale local pair -- an --accumulate retry has no merge base until a run succeeds" >&2 +fi -# The re-run spec and the JSON sidecar live in the run's cwd on the rig (REMOTE_DIR), and the -# next invocation rm -rf's it. Without copying them back, the `--accumulate` retry every doc on -# this branch prescribes has nothing to read and nothing to merge onto. Delete the local copies -# FIRST: a green run writes no .failed, so a silent no-op scp would leave last run's spec in -# the checkout looking current, and "retry from the spec" would re-flash boards that passed. -for extra in "$(basename "$CONFIG").failed" hil_report.json; do - rm -f "$ROOT_DIR/$extra" - scp -q "$REMOTE:$REMOTE_DIR/$extra" "$ROOT_DIR/$extra" 2>/dev/null \ - && echo "==> $extra copied to $ROOT_DIR/$extra" || true -done +# The re-run spec lives in the run's cwd on the rig and the next invocation rm -rf's it. +# Delete the local copy first: a green run writes no .failed, so a silent no-op scp would +# leave last run's spec looking current and "retry from the spec" would re-flash boards +# that passed. +spec="$(basename "$CONFIG").failed" +rm -f "$ROOT_DIR/$spec" +scp -q "$REMOTE:$REMOTE_DIR/$spec" "$ROOT_DIR/$spec" 2>/dev/null \ + && echo "==> $spec copied to $ROOT_DIR/$spec" || true exit $rc diff --git a/test/hil/hil_test.py b/test/hil/hil_test.py index fcd7c7e6f..83e5082d4 100755 --- a/test/hil/hil_test.py +++ b/test/hil/hil_test.py @@ -64,7 +64,7 @@ from multiprocessing import TimeoutError as MpTimeoutError sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) # PYTHONSAFEPATH drops it import hil_flash -from helper import hil_health, hil_lock, hil_util +from helper import hil_health, hil_lock, hil_report, hil_util from helper.hil_util import device_tests, dual_tests, host_test # Raw Lock/Semaphore objects in Pool initargs are inheritable only under fork @@ -106,9 +106,6 @@ STATUS_OK = "\033[32mOK\033[0m" STATUS_FAILED = "\033[31mFailed\033[0m" STATUS_SKIPPED = "\033[33mSkipped\033[0m" -# Plain (non-ANSI) cell symbols for hil_report.md; a missing binary counts as skipped. -REPORT_CELL = {'pass': '✅', 'fail': '❌', 'skip': '⚪'} - class TestFail(AssertionError): """Fail a test but still surface a metric string in its report cell (e.g. usbtest's '❌ 29/30' @@ -871,7 +868,7 @@ def test_device_cdc_msc_throughput(board): # payload, so an HS board reads as suspiciously slow. Say so rather than publish a green # cell whose scale is a guess. scale = '' if speed_known else ' FS?' - return f'{REPORT_CELL["pass"]} C {pair(cdc_r, cdc_w)} M {pair(msc_r, msc_w)}{scale}' + return f'{hil_report.REPORT_CELL["pass"]} C {pair(cdc_r, cdc_w)} M {pair(msc_r, msc_w)}{scale}' def test_device_dfu(board): @@ -1364,7 +1361,7 @@ def test_device_usbtest(board): f'no cafe:4010 device with serial {uid}' if seen is False else f'cannot tell whether cafe:4010 {uid} is present: the bounded sysfs reads did ' f'not answer{hil_util.sysfs_blind_note()}', - metric=f'{REPORT_CELL["fail"]} 0/30') + metric=f'{hil_report.REPORT_CELL["fail"]} 0/30') # settle: right after flashing the enumeration can bounce once (and on dual-port parts # the other port's stale node — same serial and PID — lingers), and testusb run into # that gap sees the device drop mid-case @@ -1454,7 +1451,7 @@ def test_device_usbtest(board): board_wedged = (f'{board["name"]}: usbtest reported a hang and was killed ' f'before it could report a verdict') raise TestFail(f'usbtest did not run: {detail}', - metric=f'{REPORT_CELL["fail"]} 0/30') + metric=f'{hil_report.REPORT_CELL["fail"]} 0/30') # A HUNG case that recovery could not clear leaves a D-state holder on this board's # usbfs node. Latch it: the remaining examples would each flash THROUGH that node, @@ -1485,9 +1482,9 @@ def test_device_usbtest(board): # the re-run spec. parsed=True: a retry re-pays the whole battery to re-observe a # wedge, and flashes through the poisoned node to do it. raise TestFail(f'usbtest {passed}/{total} but the device wedged ({board_wedged})', - metric=f'{REPORT_CELL["fail"]} {passed}/{total}', parsed=True) + metric=f'{hil_report.REPORT_CELL["fail"]} {passed}/{total}', parsed=True) if failed == 0 and notrun == 0 and total > 0: - return f'{REPORT_CELL["pass"]} {passed}/{total}' + return f'{hil_report.REPORT_CELL["pass"]} {passed}/{total}' bad = [c.get('num') for c in data.get('cases', []) if c.get('status') not in ('PASS', 'BUDGET')] why = f'usbtest {passed}/{total}' @@ -1503,7 +1500,7 @@ def test_device_usbtest(board): why += f'; {notrun} case(s) never ran ({reason}), so this says nothing about them' # parsed ONLY when every case ran: an aborted battery (budget expiry, kernel hang, bus # drop) leaves BUDGET entries, and those are exactly what a reflash retry can fix. - raise TestFail(why, metric=f'{REPORT_CELL["fail"]} {passed}/{total}', + raise TestFail(why, metric=f'{hil_report.REPORT_CELL["fail"]} {passed}/{total}', parsed=(notrun == 0)) @@ -1705,10 +1702,6 @@ def build_board(board: Board) -> tuple[str, int]: return name, failed -# pseudo-test column for a variant boundary the park-flash could not clear (see below) -BOUNDARY_CELL = 'same-PID boundary' - - def test_board(board: Board) -> tuple[str, int, list[str], list, float]: swept = False name = board['name'] @@ -1722,7 +1715,7 @@ def test_board(board: Board) -> tuple[str, int, list[str], list, float]: log_line(f'{name:25} {STATUS_FAILED}: {e}') # visible report row so the ❌ matches the exit code; failed-tests stays empty so a # re-run repeats the whole board (no bogus -bt filter) - return name, 1, [], [(name, {'board-locked': 'fail'}, None)], 0.0 + return name, 1, [], [(name, {hil_report.LOCKED_CELL: 'fail'}, None)], 0.0 # after the lock: flock wait behind a concurrent run is not board cost t_board = time.monotonic() try: @@ -1809,7 +1802,7 @@ def test_board(board: Board) -> tuple[str, int, list[str], list, float]: # charging again would double-count one incident in the exit code if not wedge_skip: err_count += 1 - cells[BOUNDARY_CELL] = 'fail' + cells[hil_report.BOUNDARY_CELL] = 'fail' # blaming run_list[0] would re-run an innocent test that then passes, # leaving the boundary unretested; re-run the whole board instead board_wide_fail = True @@ -1825,7 +1818,7 @@ def test_board(board: Board) -> tuple[str, int, list[str], list, float]: # Do NOT flash through a poisoned node: each attempt enumerates into # it, blocks uninterruptibly and leaves another stray behind. Report # the skip so the cell is not mistaken for a pass. - cells[test] = f'{REPORT_CELL["skip"]} board wedged' + cells[test] = f'{hil_report.REPORT_CELL["skip"]} board wedged' # ...and re-run the WHOLE board, like the boundary-failure path above: # these tests never executed, so naming them individually in the .failed # spec is not enough -- an --accumulate re-run that fixes only the wedged @@ -1893,8 +1886,6 @@ def test_board(board: Board) -> tuple[str, int, list[str], list, float]: _lock_fh.close() -REPORT_MD = 'hil_report.md' -REPORT_JSON = 'hil_report.json' # controller hints from previous runs: uid -> {'name', 'pci', 'duration'}. Only 'pci' is # consumed (dispatch order and first-flash budgeting, never battery serialization). PCI # addresses are boot-stable, so the cache survives reboots and goes stale on re-cabling. @@ -1912,66 +1903,6 @@ def schedule_boards(boards: list, pci_of_uid: dict) -> list: return [b for grp in itertools.zip_longest(*buckets.values()) for b in grp if b is not None] -def render_matrix(rows_all: list) -> str: - """Render rows (list of (row_label, {example: status}, duration)) as an aligned - markdown matrix: columns = tests (bare names) centered, boards left-aligned, - per-row duration as the trailing column.""" - seen = set() - for _, cells, _ in rows_all: - seen.update(cells) - if not seen: - return 'No tests were run.' - - # metric-bearing columns pinned first, the rest alphabetical: stable regardless of the - # shuffled execution order - pinned = ['usbtest', 'cdc_msc_throughput', 'msc_file_explorer', 'msc_file_explorer_freertos'] - - def col_key(t): - name = t.rsplit('/', 1)[-1] - return (pinned.index(name) if name in pinned else len(pinned), name, t) - - columns = sorted(seen, key=col_key) - headers = [c.rsplit('/', 1)[-1] for c in columns] + ['duration'] # bare example names - - def cell(cells, col): - v = cells.get(col) - if v is None: - return '' - return REPORT_CELL.get(v, v) # status symbol, or a metric string (e.g. speed) verbatim - - rows_vals = [(lbl, [cell(cells, c) for c in columns] + [dur or '']) - for lbl, cells, dur in rows_all] - board_hdr = 'Board' - board_w = max([len(board_hdr)] + [len(lbl) for lbl, _ in rows_vals]) - col_w = [max([len(h)] + [len(vals[i]) for _, vals in rows_vals]) - for i, h in enumerate(headers)] - - def line(label, values): - padded = [label.ljust(board_w)] + [v.center(w) for v, w in zip(values, col_w)] - return '| ' + ' | '.join(padded) + ' |' - - header = line(board_hdr, headers) - sep = '| ' + '-' * board_w + ' | ' + ' | '.join(':' + '-' * (w - 2) + ':' for w in col_w) + ' |' - body = [line(lbl, vals) for lbl, vals in rows_vals] - - # tally run cells (not-run cells are absent from the dicts). A cell is a bare status or - # a metric string carrying its own icon ("❌ 29/30"), so classify by the leading icon. - def cell_kind(v): - if v == 'fail' or (isinstance(v, str) and v.startswith(REPORT_CELL['fail'])): - return 'fail' - if v == 'skip' or (isinstance(v, str) and v.startswith(REPORT_CELL['skip'])): - return 'skip' - return 'pass' - kinds = [cell_kind(v) for _, cells, _ in rows_all for v in cells.values()] - failed = kinds.count('fail') - skipped = kinds.count('skip') - passed = kinds.count('pass') - summary = (f'**{REPORT_CELL["pass"]} {passed} passed · {REPORT_CELL["fail"]} {failed} failed · ' - f'{REPORT_CELL["skip"]} {skipped} skipped · blank not run**') - - return summary + '\n\n' + '\n'.join([header, sep] + body) - - def _write_failed_spec(failed_fname: Path, report_dir: Path, mret: list) -> None: """Re-run spec: only the failed boards (-b), each restricted to its own failed tests (-bt); a board with failures but no test list re-runs entirely. @@ -2083,87 +2014,13 @@ def _blind_note(mret: list) -> str: f'{", ".join(blind)}. See the usb-kernel-recover skill.\n') -def accumulate_report(mret: list, report_dir: Path, fresh: bool, scope: str = '', - banner: str = '') -> str: - """Merge this run's results into hil_report.json in report_dir, then (re)write - the markdown matrix to hil_report.md. `fresh` (a first run, no --accumulate) - starts a new report; otherwise a re-run accumulates so boards/tests that - already passed are preserved while re-run cells are updated. `scope` names the - board filter, if any, so a scoped table is not mistaken for a full one. - Returns the md.""" - acc = {} # ordered {row_label: [cells dict, duration str|None]} - prior_banner = '' - jpath = report_dir / REPORT_JSON - if not fresh and jpath.is_file(): - try: - saved = json.loads(jpath.read_text()) - # CI keys the report dir by run id, so the sidecar is from an earlier attempt - for entry in saved.get('rows', []): - acc[entry['board']] = [dict(entry['cells']), entry.get('duration')] - # ... and so is the caveat those cells were collected under. A rerun on a rig - # that has since recovered contributes no banner, and the .failed spec reruns - # only FAILURES -- so the earlier attempt's passes are never re-earned and - # would be published as clean results of a rig that was not. - prior_banner = saved.get('banner', '') - except (ValueError, KeyError, TypeError): - pass # corrupt/old sidecar: start fresh - - # current cells override prior for boards/tests that ran; a filtered run reports - # duration None, keeping the previous full-run value - for name, _, _, rows, *_ in mret: - if rows and not any('board-locked' in cells for _, cells, _ in rows): - # board ran for real: clear a stale lock-failure cell (its row is keyed by - # board name; test rows may be variant names) - stale = acc.get(name) - if stale is not None: - stale[0].pop('board-locked', None) - if not stale[0]: - # variant-keyed boards never repopulate the board-name row, so drop it - # or it renders as a blank ghost row - del acc[name] - for row_label, cells, dur in rows: - row = acc.setdefault(row_label, [{}, None]) - # the boundary cell is only ever written on failure, so a re-run of this - # variant that cleared the boundary must drop the previous attempt's ❌ - if BOUNDARY_CELL not in cells: - row[0].pop(BOUNDARY_CELL, None) - row[0].update(cells) - if dur is not None: - row[1] = dur - - report_dir.mkdir(parents=True, exist_ok=True) - # by LINE, deduped: attempts repeat the same caveat far more often than they add a new - # one, and three copies of the D-state note reads as three incidents - seen, merged = set(), [] - for line in (prior_banner + banner).splitlines(): - if line.strip() and line not in seen: - seen.add(line) - merged.append(line) - banner = '\n'.join(merged) + '\n' if merged else '' - jpath.write_text(json.dumps({'rows': [{'board': k, 'cells': c, 'duration': d} - for k, (c, d) in acc.items()], - 'banner': banner}, indent=2) + '\n') - - md = render_matrix([(k, c, d) for k, (c, d) in acc.items()]) - if scope: - # a scoped run's small table is otherwise indistinguishable from a full one, and - # it replaces the previous full table in the sticky PR comment - md = f'_Scoped run: {scope}. Boards/tests not listed were not run._\n\n' + md - # LAST, so it is outermost: a rig-health caveat outranks the table AND the scope note, - # and the top of the report is where hil/SKILL.md tells the agent to look for it. - if banner: - md = banner + '\n' + md - (report_dir / REPORT_MD).write_text(md + '\n', encoding='utf-8') - return md - - # containment paths print through hil_health._p: stdout may already be a dead pipe (a # dropped ssh session), and a BrokenPipeError there would skip os._exit _p = hil_health._p def _abandon_exit(pool, mgr, abandoned: bool, err_count: int, - report: Path | None = None) -> None: + report_dir: Path | None = None) -> None: """Free the runner when the pool could not be shut down. Returns only if not abandoned. Must run even while an exception is propagating: multiprocessing's atexit handler @@ -2199,24 +2056,11 @@ def _abandon_exit(pool, mgr, abandoned: bool, err_count: int, 'stay locked.', flush=True) # A report already written by accumulate_report says nothing about the abandon, and a # green table under a red job is how an agent ends up pasting it as this run's result. - # Prepend the caveat; best-effort, never at the cost of exiting. - if report is not None: - try: - if report.exists(): - # utf-8 explicitly (the cells are ✅/❌/⚪) and catch ValueError too: a torn - # report or a LANG=C locale raises UnicodeDecodeError -- NOT an OSError -- - # straight past os._exit, stranding the runner. - body = report.read_text(encoding='utf-8', errors='replace') - # Only when no banner is there yet, searched anywhere in the head rather - # than at char 0: write_timeout_report's banner must stay FIRST (its table - # is a PREVIOUS attempt's) and it puts the rig-health quote above itself. - if '**HIL run ab' not in body[:2000]: - report.write_text( - '**HIL run abandoned: the worker pool would not shut down.** The ' - 'table below was collected before the abandon; treat board ' - 'results as unverified.\n\n' + body, encoding='utf-8') - except (OSError, ValueError): - pass + # Set the caveat in the DOCUMENT -- prepending to the markdown alone left the sidecar, + # which is all hil_report.summarize() and therefore an agent ever sees, saying nothing. + # Best-effort, never at the cost of exiting. + if report_dir is not None: + hil_report.mark_report_abandoned(report_dir, 'the worker pool would not shut down.') try: sys.stdout.flush() except OSError: @@ -2305,13 +2149,11 @@ def main() -> None: print(msg, flush=True) # loud AND leaving evidence: exiting with no report at all lets the PR comment # keep the previous push's stale table under a red job - try: - rd = Path(os.environ.get('HIL_REPORT_DIR', '.')) - rd.mkdir(parents=True, exist_ok=True) - (rd / REPORT_MD).write_text(f'**HIL run selected no boards.** {msg}\n', - encoding='utf-8') - except OSError: - pass + rd = Path(os.environ.get('HIL_REPORT_DIR', '.')) + # fresh must be threaded through: this runs BEFORE the `if fresh:` wipe below, so + # defaulting it here wiped an --accumulate run's accumulated rows -- the exact + # regression the parameter exists to prevent. + hil_report.mark_report_no_boards(rd, msg, fresh=not args.accumulate) sys.exit(1) @@ -2387,14 +2229,14 @@ def main() -> None: # BEFORE Manager()/Pool(), not inside the try: hil_ci.sh reuses a persistent REMOTE_DIR # and scp's the report back unconditionally, so if a fork failure (OSError/EAGAIN right # after a convoy -- the case this whole block guards) skipped the wipe, the finally's - # _abandon_exit would prepend "HIL run abandoned" to the PREVIOUS run's table and + # _abandon_exit would stamp "HIL run abandoned" onto the PREVIOUS run's report and # publish last night's board results as this run's. Nothing is live yet here, so an # OSError from the wipe itself just exits with its traceback -- it cannot strand the # interpreter in multiprocessing's unbounded atexit join, which is what deferring it # was protecting against. if fresh: report_dir.mkdir(parents=True, exist_ok=True) - for f in (REPORT_JSON, REPORT_MD): + for f in (hil_report.REPORT_JSON, hil_report.REPORT_MD): (report_dir / f).unlink(missing_ok=True) failed_fname.unlink(missing_ok=True) try: @@ -2446,9 +2288,9 @@ def main() -> None: f'are this run\'s; {len(stuck)} never reported and are NOT in ' f'the table: {", ".join(stuck)}. Re-run covers those.\n') try: - accumulate_report(mret, report_dir, fresh, '', + hil_report.accumulate_report(mret, report_dir, fresh, '', health_banner + _blind_note(mret) - + _stray_note(mret) + banner) + + _stray_note(mret), caveat=banner) except Exception as rerr: # noqa: BLE001 - the raise below must still happen # FALL BACK, do not just warn: accumulate_report can raise on an # unwritable/root-owned report dir or a torn JSON, and _abandon_exit @@ -2458,9 +2300,9 @@ def main() -> None: print(f'warning: partial report failed: {type(rerr).__name__}: {rerr}; ' f'falling back to the board list', flush=True) try: - hil_health.write_timeout_report( + hil_report.write_timeout_report( report_dir, [b for b in config_boards - if b['name'] in stuck], POOL_TIMEOUT, REPORT_MD, + if b['name'] in stuck], POOL_TIMEOUT, prefix=health_banner) except Exception as re2: # noqa: BLE001 print(f'warning: fallback report failed too: ' @@ -2485,9 +2327,9 @@ def main() -> None: f'{len(mret)} board(s) below finished and are this run\'s; ' f'{len(stuck)} did not report: {", ".join(stuck)}.\n') try: - accumulate_report(mret, report_dir, fresh, '', + hil_report.accumulate_report(mret, report_dir, fresh, '', health_banner + _blind_note(mret) - + _stray_note(mret) + banner) + + _stray_note(mret), caveat=banner) except Exception as re2: # noqa: BLE001 - the raise below must still happen print(f'warning: partial report failed: {type(re2).__name__}: {re2}', flush=True) @@ -2578,12 +2420,12 @@ def main() -> None: # looks exactly like a full run that happened to be small scoped = sorted(set(args.board) | set(board_test)) scope = f'{len(scoped)} board(s) — {", ".join(scoped)}' if scoped else '' - report = accumulate_report(mret, report_dir, fresh, scope, + report = hil_report.accumulate_report(mret, report_dir, fresh, scope, health_banner + _blind_note(mret) + _stray_note(mret)) print() print(report) - print(f'\nReport written to {(report_dir / REPORT_MD).resolve()}') + print(f'\nReport written to {(report_dir / hil_report.REPORT_MD).resolve()}') duration = time.time() - duration print() @@ -2594,7 +2436,7 @@ def main() -> None: # In the finally, not after: any raise above (accumulate_report sits outside the # OSError handler) would skip the abandon path and unwind into multiprocessing's # unbounded atexit join, hanging the runner. - _abandon_exit(pool, mgr, pool_abandoned, err_count, report_dir / REPORT_MD) + _abandon_exit(pool, mgr, pool_abandoned, err_count, report_dir) # Same clamp: exit status is a byte either way, so 256 failures would report green. sys.exit(min(err_count, 125)) diff --git a/test/hil/test/test_ci_select.py b/test/hil/test/test_ci_select.py index c34bccd1f..f3e000cb4 100644 --- a/test/hil/test/test_ci_select.py +++ b/test/hil/test/test_ci_select.py @@ -984,6 +984,7 @@ class TestTheHarnessTestsAreNotTheHarness(unittest.TestCase): 'test/hil/test/test_ci_select.py', 'test/hil/test/test_hil_bounded.py', 'test/hil/test/test_hil_health.py', + 'test/hil/test/test_hil_report.py', 'test/hil/test/test_hil_util.py', ], 'test/hil/test/ gained or lost a file; it is carved out of rule 2, so confirm ' 'the rig still does not read anything in there before updating this list') diff --git a/test/hil/test/test_hil_bounded.py b/test/hil/test/test_hil_bounded.py index c6d454f0e..715d520db 100644 --- a/test/hil/test/test_hil_bounded.py +++ b/test/hil/test/test_hil_bounded.py @@ -39,6 +39,7 @@ serial_stub.SerialTimeoutException = type('SerialTimeoutException', (Exception,) sys.modules.setdefault('serial', serial_stub) import hil_flash import hil_test +from helper import hil_report def write_script(path: Path, body: str) -> None: @@ -936,11 +937,17 @@ class RunWhileContract(unittest.TestCase): def boom(): raise AssertionError('x') + # a duration no other process would plausibly pick: `pgrep -f` searches the WHOLE + # machine, so a bare `sleep 20` matched an unrelated background job -- another + # agent session's retry loop, in the case that exposed this -- and failed a test + # about our own child. Observed failing 3/3 in isolation while that loop ran. + sentinel = '20.0451' with self.assertRaises(AssertionError): - self.hil_util.run_alongside(['sleep', '20'], boom, 1) + self.hil_util.run_alongside(['sleep', sentinel], boom, 1) # nothing of ours is left running: the reap ran on the error path too import subprocess - out = subprocess.run(['pgrep', '-f', '^sleep 20'], capture_output=True, text=True) + out = subprocess.run(['pgrep', '-f', f'^sleep {sentinel}'], + capture_output=True, text=True) seen['strays'] = [p for p in out.stdout.split() if p] self.assertEqual(seen['strays'], [], 'work() raising leaked the child') @@ -1146,10 +1153,16 @@ class AbandonExitSurvivesAFailedFork(unittest.TestCase): def test_none_pool_and_manager_still_write_the_banner(self): # a subprocess, because _abandon_exit ends in os._exit: in-process it would take # the test runner with it, before any assertion could run + import json import subprocess with TemporaryDirectory() as td: - report = Path(td) / 'hil_report.md' - report.write_text('| board | test |\n|---|---|\n', encoding='utf-8') + rd = Path(td) + # it takes the report DIRECTORY now and re-renders both artifacts from the + # sidecar, so seed the sidecar -- the markdown is output, not input + (rd / 'hil_report.json').write_text(json.dumps( + {'rows': [{'board': 'boardA', 'cells': {'cdc_msc': 'pass'}, + 'duration': '1s'}], + 'banner': '', 'scope': '', 'caveat': ''})) src = ( 'import sys, types\n' f'sys.path.insert(0, {str(Path(TEST_DIR).parents[0])!r})\n' @@ -1160,12 +1173,14 @@ class AbandonExitSurvivesAFailedFork(unittest.TestCase): 'sys.modules.setdefault("serial", st)\n' 'import hil_test\n' f'hil_test._abandon_exit(None, None, True, 1, __import__("pathlib")' - f'.Path({str(report)!r}))\n') + f'.Path({str(rd)!r}))\n') r = subprocess.run([sys.executable, '-c', src], capture_output=True, text=True, timeout=120) self.assertEqual(r.returncode, 1, r.stderr) - self.assertTrue(report.read_text().startswith('**HIL run abandoned'), - 'the abandon banner never reached the report') + self.assertTrue((rd / 'hil_report.md').read_text().startswith( + '**HIL run abandoned'), 'the abandon banner never reached the report') + self.assertIn('abandoned', + json.loads((rd / 'hil_report.json').read_text())['caveat']) def test_kill_pool_children_tolerates_a_pool_that_never_existed(self): from helper import hil_health @@ -1647,124 +1662,6 @@ class StagingCoversEveryBoardForm(unittest.TestCase): "last run's re-run spec survived a green run") -class SummaryFoldsReportToBoards(unittest.TestCase): - """hil_summary.py replaces the agent retyping the markdown table. Report rows are named per - VARIANT and a variant need not start with the board name, so the config is what maps them - back -- the previous string-matching design produced a defect in each of four review rounds.""" - - def _sum(self, boards, rows, cfg_boards=None, banner=''): - import json - import subprocess - td = TemporaryDirectory() - self.addCleanup(td.cleanup) - d = Path(td.name) - (d / 'hil_report.json').write_text(json.dumps( - {'rows': [{'board': b, 'cells': c, 'duration': '1s'} for b, c in rows], - 'banner': banner})) - cfg = d / 'cfg.json' - cfg.write_text(json.dumps({'boards': cfg_boards or [{'name': b} for b in boards]})) - args = [a for b in boards for a in ('-b', b)] - r = subprocess.run(['python3', str(Path(TEST_DIR).parents[0] / 'helper' / 'hil_summary.py'), - str(cfg), *args, '--report-dir', str(d)], - capture_output=True, text=True, timeout=60) - self.assertEqual(r.returncode, 0, r.stderr) - return json.loads(r.stdout)['results'] - - def test_variant_rows_fold_onto_their_board(self): - """nanoch32v203 never produces a row named after the board.""" - got = self._sum(['nanoch32v203'], - [('nanoch32v203-fsdev', {'usbtest': 'pass'}), - ('nanoch32v203-usbfs', {'usbtest': 'pass'})], - cfg_boards=[{'name': 'nanoch32v203', - 'variant': [{'name': 'nanoch32v203-fsdev'}, - {'name': 'nanoch32v203-usbfs'}]}]) - self.assertEqual([r['board'] for r in got], ['nanoch32v203']) - self.assertTrue(got[0]['pass']) - self.assertTrue(got[0]['ran']) - - def test_one_failing_variant_fails_the_board(self): - got = self._sum(['nano'], - [('nano-a', {'usbtest': 'pass'}), ('nano-b', {'usbtest': '❌ 29/30'})], - cfg_boards=[{'name': 'nano', 'variant': [{'name': 'nano-a'}, - {'name': 'nano-b'}]}]) - self.assertFalse(got[0]['pass']) - self.assertIn('29/30', got[0]['detail']) - - def test_lock_contention_is_a_field_not_a_prefix(self): - got = self._sum(['alpha'], [('alpha', {'board-locked': 'fail'})]) - self.assertTrue(got[0]['locked']) - self.assertFalse(got[0]['pass']) - - def test_a_board_with_no_row_is_marked_not_run(self): - got = self._sum(['alpha', 'beta'], [('alpha', {'usbtest': 'pass'})]) - self.assertTrue(got[0]['ran']) - self.assertFalse(got[1]['ran']) - self.assertFalse(got[1]['pass']) - - def test_a_metric_cell_counts_by_its_icon(self): - got = self._sum(['a', 'b'], [('a', {'cdc_msc_throughput': '✅ C 1.2 M 3.4'}), - ('b', {'cdc_msc_throughput': '❌ C 0.0 M 0.0'})]) - self.assertTrue(got[0]['pass']) - self.assertFalse(got[1]['pass']) - - def test_skipped_cells_do_not_fail_a_board(self): - got = self._sum(['a'], [('a', {'usbtest': 'skip', 'cdc_msc': 'pass'})]) - self.assertTrue(got[0]['pass']) - - def test_a_plain_metric_cell_is_a_pass(self): - """Mirrors hil_test.py's own tally (cell_kind): failures are ALWAYS marked -- 'fail' - or a ❌ prefix, per TestFail's docstring -- while a passing test may return a plain - metric string that lands in the cell unprefixed. Treating unknown shapes as fail - would publish a green table as a red verdict.""" - got = self._sum(['a'], [('a', {'device_speed': '480.0 MBps'})]) - self.assertTrue(got[0]['pass']) - - def test_a_declared_variant_of_another_board_is_not_stolen(self): - """A declared variant need not start with its own board's name, so it may start with - a DIFFERENT board's name plus '-'. The prefix fallback must not attribute it twice.""" - got = self._sum(['alpha', 'beta'], - [('beta-x', {'usbtest': 'fail'})], - cfg_boards=[{'name': 'alpha', 'variant': [{'name': 'beta-x'}]}, - {'name': 'beta'}]) - self.assertTrue(got[0]['ran']) - self.assertFalse(got[0]['pass']) - self.assertFalse(got[1]['ran'], "beta must not inherit alpha's row") - - -class CaveatSurvivesAccumulate(unittest.TestCase): - """CI reruns with --accumulate: the sidecar keeps every earlier attempt's cells, but the - banner was recomputed per attempt. A first attempt on a degraded rig and a clean rerun - therefore published the degraded attempt's PASSES with no caveat on them -- and the - generated .failed spec reruns only failures, so those cells are never re-earned.""" - - def _rows(self, board, cell): - return [(board, 0, 0, [(board, {cell: 'OK'}, '1s')], 0)] - - def test_an_earlier_attempts_caveat_is_still_on_the_report(self): - td = TemporaryDirectory() - self.addCleanup(td.cleanup) - rd = Path(td.name) - banner = '> **Rig note.** 2 process(es) in D state at start.\n' - - hil_test.accumulate_report(self._rows('boardA', 'cdc_msc'), rd, True, '', banner) - self.assertIn('Rig note', (rd / hil_test.REPORT_MD).read_text()) - - # the rerun: clean rig, so this attempt contributes no banner of its own - md = hil_test.accumulate_report(self._rows('boardB', 'cdc_msc'), rd, False, '', '') - self.assertIn('boardA', md) # the earlier cells are kept ... - self.assertIn('Rig note', md, - 'the caveat the earlier cells were collected under was dropped') - - def test_the_same_caveat_twice_is_not_stacked(self): - td = TemporaryDirectory() - self.addCleanup(td.cleanup) - rd = Path(td.name) - banner = '> **Rig note.** 2 process(es) in D state at start.\n' - hil_test.accumulate_report(self._rows('boardA', 'cdc_msc'), rd, True, '', banner) - md = hil_test.accumulate_report(self._rows('boardB', 'cdc_msc'), rd, False, '', banner) - self.assertEqual(md.count('Rig note'), 1) - - class BlindWorkerReachesTheReport(unittest.TestCase): """A worker that exhausts its bounded-read budget answers SYSFS_UNKNOWN for every attribute, so its "device not found" means "could not tell". That reached the log and @@ -1796,7 +1693,7 @@ class BlindWorkerReachesTheReport(unittest.TestCase): wide = ('boardA', 1, ['device/cdc_msc'], [('boardA', {'cdc_msc': '❌'}, '2s')], 2.0, True) narrow = ('stuck', 1, [], None, 0) # what the timeout path builds hil_test._write_failed_spec(rd / 'x.failed', rd, [wide, narrow]) - md = hil_test.accumulate_report([wide], rd, True, '', hil_test._blind_note([wide])) + md = hil_report.accumulate_report([wide], rd, True, '', hil_test._blind_note([wide])) self.assertIn('boardA', md) self.assertIn('not all verdicts are evidence', md.lower()) diff --git a/test/hil/test/test_hil_health.py b/test/hil/test/test_hil_health.py index 5695cad6d..c7864f568 100644 --- a/test/hil/test/test_hil_health.py +++ b/test/hil/test/test_hil_health.py @@ -467,49 +467,6 @@ class KillPoolChildren(PatchCase): self.assertEqual(hil_health.kill_pool_children(NoPool()), 0) -class WriteTimeoutReport(unittest.TestCase): - def test_prefix_carries_the_preflight_diagnosis(self): - """The timeout aborts before accumulate_report, so without the prefix the artifact - and the PR comment lose the one line saying WHY the pool never finished.""" - with TemporaryDirectory() as td: - d = Path(td) - hil_health.write_timeout_report(d, [{'name': 'b1'}], 4200, 'r.md', - prefix='> **wedged usb_hub_wq worker.**\n') - out = (d / 'r.md').read_text() - self.assertTrue(out.startswith('> **wedged usb_hub_wq worker.**')) - self.assertIn('timed out after 4200s', out) - self.assertIn('- b1', out) - - def test_writes_a_report_where_there_would_be_none(self): - with TemporaryDirectory() as td: - hil_health.write_timeout_report(Path(td), [{'name': 'ra6m5_ek'}], 4200, - 'hil_report.md') - md = (Path(td) / 'hil_report.md').read_text() - self.assertIn('4200s', md) - self.assertIn('ra6m5_ek', md) - - def test_keeps_a_previous_attempts_table(self): - with TemporaryDirectory() as td: - path = Path(td) / 'hil_report.md' - path.write_text('| board | cdc_msc |\n') - hil_health.write_timeout_report(Path(td), [{'name': 'b1'}], 4200, 'hil_report.md') - md = path.read_text() - self.assertIn('abandoned', md) - self.assertIn('| board | cdc_msc |', md) - self.assertLess(md.index('abandoned'), md.index('| board |')) - - def test_custom_banner_is_used(self): - with TemporaryDirectory() as td: - hil_health.write_timeout_report(Path(td), [], 0, 'hil_report.md', - banner='**refused to start.**\n') - self.assertIn('refused to start', (Path(td) / 'hil_report.md').read_text()) - - def test_unwritable_dir_does_not_raise(self): - """The caller may be about to os._exit; losing the report must not also lose the - exit path.""" - hil_health.write_timeout_report(Path('/proc/nonexistent/nope'), [], 0, 'x.md') - - class WorkerSweepsItsOwnChildren(unittest.TestCase): """maxtasksperchild=1 makes a worker exit the moment its task returns, so by the time main()'s finally sweeps, the strays have been reparented to init and are off the pool's diff --git a/test/hil/test/test_hil_report.py b/test/hil/test/test_hil_report.py new file mode 100644 index 000000000..9ab39bde3 --- /dev/null +++ b/test/hil/test/test_hil_report.py @@ -0,0 +1,1162 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: MIT +# Unit tests for the report document: the vocabulary, the one cell classifier, rendering, +# the four writers, and the fold to per-board verdicts. Split out of test_hil_bounded.py +# and test_hil_health.py when the report code moved into helper/hil_report.py. +# Run directly: +# python3 test/hil/test/test_hil_report.py +import json +import os +import subprocess +import sys +import unittest +from pathlib import Path +from tempfile import TemporaryDirectory + +TEST_DIR = os.path.dirname(os.path.abspath(__file__)) +HIL_DIR = os.path.dirname(TEST_DIR) +sys.path.insert(0, HIL_DIR) + +from helper import hil_report + + +class OneClassifierForBothArtifacts(unittest.TestCase): + """The markdown tally and the agent's verdict used to classify cells with two separate + copies of one rule -- hil_test's cell_kind against REPORT_CELL, and hil_summary's + cell_state against its own re-typed '❌'/'⚪' literals. Change the icons and the table + and the verdict silently disagree.""" + + def test_bare_states(self): + self.assertEqual(hil_report.cell_state('fail'), 'fail') + self.assertEqual(hil_report.cell_state('skip'), 'skip') + self.assertEqual(hil_report.cell_state('pass'), 'pass') + + def test_icon_prefixed_metrics_carry_their_verdict(self): + self.assertEqual(hil_report.cell_state(f'{hil_report.REPORT_CELL["fail"]} 29/30'), 'fail') + self.assertEqual(hil_report.cell_state(f'{hil_report.REPORT_CELL["skip"]} board wedged'), + 'skip') + + def test_an_unprefixed_metric_is_a_pass(self): + """Load-bearing: a passing test may return a plain metric string. Classifying + unknown shapes as fail would publish a green table as a red verdict.""" + self.assertEqual(hil_report.cell_state('480.0 MBps'), 'pass') + self.assertEqual(hil_report.cell_state('1103 KB/s'), 'pass') + + def test_a_non_string_cell_does_not_raise(self): + """render_matrix's copy guarded with isinstance; hil_summary's did not, because its + caller str()'d first. The merged one keeps the guard -- it is the safer superset.""" + self.assertEqual(hil_report.cell_state(None), 'pass') + + def test_the_icons_come_from_REPORT_CELL(self): + """No second copy of the emoji anywhere in the module.""" + src = (Path(HIL_DIR) / 'helper' / 'hil_report.py').read_text(encoding='utf-8') + # CODE only: prose may quote an icon to explain a rule. The old assertion counted + # the single-quoted spelling `'❌'`, which a second copy written as "❌" would have + # sailed past. + code = '\n'.join(line.split('#', 1)[0] for line in src.splitlines()) + for icon in ('❌', '⚪', '✅'): + self.assertEqual(code.count(icon), 1, + f'{icon} is spelled in code more than once; REPORT_CELL is' + f' meant to be the one source') + + +class ModuleWorksImportedAndAsAScript(unittest.TestCase): + """It is imported as helper.hil_report by hil_test, and run as a script by the operator + (.claude/agents/hil-operator.md). A script run puts helper/ on sys.path, NOT test/hil, + so a plain `from helper import hil_health` breaks the CLI and only the CLI.""" + + def test_importable_as_a_package_module(self): + r = subprocess.run( + [sys.executable, '-c', + f'import sys; sys.path.insert(0, {HIL_DIR!r}); ' + f'from helper import hil_report; print(hil_report.REPORT_JSON)'], + capture_output=True, text=True, timeout=60) + self.assertEqual(r.returncode, 0, r.stderr) + self.assertIn('hil_report.json', r.stdout) + + def test_runnable_as_a_script(self): + r = subprocess.run( + [sys.executable, str(Path(HIL_DIR) / 'helper' / 'hil_report.py'), '--help'], + capture_output=True, text=True, timeout=60) + self.assertEqual(r.returncode, 0, r.stderr) + + +class RenderReportIsPureFunctionOfTheDocument(unittest.TestCase): + """Four writers used to compose the markdown independently, so a table could carry + something the sidecar did not. One renderer, and the ordering it guarantees, is what + stops that -- pinned here rather than left to the order of three concatenations.""" + + def _doc(self, **kw): + d = {'rows': [{'board': 'boardA', 'cells': {'cdc_msc': 'pass'}, 'duration': '1s'}], + 'banner': '', 'scope': '', 'caveat': ''} + d.update(kw) + return d + + def test_table_comes_from_rows(self): + md = hil_report.render_report(self._doc()) + self.assertIn('boardA', md) + self.assertIn('cdc_msc', md) + + def test_scope_note_appears_above_the_table(self): + md = hil_report.render_report(self._doc(scope='-b boardA')) + self.assertLess(md.index('Scoped run'), md.index('boardA')) + + def test_banner_outranks_the_scope_note(self): + md = hil_report.render_report(self._doc(scope='-b boardA', + banner='> **Rig dirty.** x\n')) + self.assertLess(md.index('Rig dirty'), md.index('Scoped run')) + + def test_caveat_is_outermost(self): + md = hil_report.render_report(self._doc(banner='> **Rig dirty.** x\n', + caveat='**HIL run abandoned.**\n')) + self.assertLess(md.index('abandoned'), md.index('Rig dirty')) + + def test_a_document_with_no_rows_still_renders(self): + md = hil_report.render_report(self._doc(rows=[])) + self.assertIn('No tests were run.', md) + + def test_a_malformed_row_does_not_raise(self): + """mark_report_abandoned renders a sidecar it did not write -- hil_ci.sh reuses a + persistent REMOTE_DIR, so it can be an older version's or a torn one -- and it runs + on the way to os._exit, where a KeyError hangs the runner in multiprocessing's + unbounded join() instead of freeing it.""" + md = hil_report.render_report(self._doc( + rows=[{'board': 'boardA', 'cells': {'cdc_msc': 'pass'}}, {'board': 'half'}, + {}])) + self.assertIn('boardA', md) # the intact row still renders ... + self.assertIn('half', md) # ... and a cell-less one becomes a blank row + + +class ScopeSurvivesInTheJson(unittest.TestCase): + """A scoped run's small table is indistinguishable from a full run that lost boards. + The markdown says so; the JSON did not, so any JSON consumer could not tell.""" + + def _rows(self, board, cell): + return [(board, 0, 0, [(board, {cell: 'OK'}, '1s')], 0)] + + def test_scope_is_recorded_in_the_sidecar(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report(self._rows('boardA', 'cdc_msc'), rd, True, + '-b boardA', '') + doc = json.loads((rd / hil_report.REPORT_JSON).read_text()) + self.assertEqual(doc['scope'], '-b boardA') + + def test_an_unscoped_run_records_an_empty_scope(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report(self._rows('boardA', 'cdc_msc'), rd, True, '', '') + self.assertEqual(json.loads((rd / hil_report.REPORT_JSON).read_text())['scope'], '') + + +class EveryExitPathLeavesBothArtifacts(unittest.TestCase): + """summarize() builds an agent's verdicts from the JSON. A path that writes only + markdown reports the whole fleet as 'no report row' while a human sees the real story.""" + + def test_the_no_boards_exit_writes_json_too(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.write_report(rd, {'rows': [], 'banner': '', 'scope': '', + 'caveat': '**HIL run selected no boards.** why\n'}) + self.assertIn('selected no boards', (rd / hil_report.REPORT_MD).read_text()) + doc = json.loads((rd / hil_report.REPORT_JSON).read_text()) + self.assertEqual(doc['rows'], []) + self.assertIn('selected no boards', doc['caveat']) + + def test_write_report_raises_so_its_callers_can_report_it(self): + """write_report is NOT best-effort. Swallowing the OSError made + write_timeout_report's _p warning and hil_test's fallback-of-the-fallback dead + code -- an unwritable report dir produced no artifact and no message.""" + with self.assertRaises(OSError): + hil_report.write_report(Path('/proc/nonexistent/nope'), + {'rows': [], 'banner': '', 'scope': '', 'caveat': 'x\n'}) + + def test_the_guarded_callers_still_do_not_raise(self): + """They are the ones on the way to os._exit, where a raise hangs the interpreter + in multiprocessing's unbounded join().""" + bad = Path('/proc/nonexistent/nope') + hil_report.mark_report_abandoned(bad, 'the worker pool would not shut down.') + hil_report.mark_report_no_boards(bad, 'filters intersected to nothing') + import io + from contextlib import redirect_stdout + with redirect_stdout(io.StringIO()): + hil_report.write_timeout_report(bad, [{'name': 'b1'}], 3600) + + +class AbandonNoticeLandsInBothArtifacts(unittest.TestCase): + """_abandon_exit did a text prepend on a file it had not written, so the caveat never + reached the JSON and an agent reading the sidecar saw a clean partial report under a + red job.""" + + def test_abandon_sets_the_caveat_not_just_the_markdown(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report( + [('boardA', 0, 0, [('boardA', {'cdc_msc': 'OK'}, '1s')], 0)], rd, True, '', '') + hil_report.mark_report_abandoned(rd, 'the worker pool would not shut down.') + doc = json.loads((rd / hil_report.REPORT_JSON).read_text()) + self.assertIn('abandoned', doc['caveat']) + self.assertEqual(len(doc['rows']), 1, 'the finished board must survive') + md = (rd / hil_report.REPORT_MD).read_text() + self.assertLess(md.index('abandoned'), md.index('boardA')) + + def test_marking_a_missing_report_is_a_no_op(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + hil_report.mark_report_abandoned(Path(td.name), 'x') # must not raise + + def test_a_sidecar_with_a_malformed_row_still_gets_stamped(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + (rd / hil_report.REPORT_JSON).write_text(json.dumps( + {'rows': [{'board': 'boardA'}], 'banner': '', 'scope': '', 'caveat': ''})) + hil_report.mark_report_abandoned(rd, 'x') + self.assertIn('abandoned', + json.loads((rd / hil_report.REPORT_JSON).read_text())['caveat']) + self.assertIn('abandoned', (rd / hil_report.REPORT_MD).read_text()) + + def test_a_torn_sidecar_is_a_no_op(self): + """This runs while the interpreter is being torn down: a raise here hangs the + process in multiprocessing's unbounded join().""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + (rd / hil_report.REPORT_JSON).write_text('{ truncated mid-') + hil_report.mark_report_abandoned(rd, 'x') # must not raise + + def test_an_existing_abandon_caveat_is_not_overwritten(self): + """The pool-timeout path names the stuck boards and the rig-health verdict; this + one only knows the pool would not shut down. Whoever got there first wins -- + the guard _abandon_exit used to spell as "'**HIL run ab' not in body[:2000]".""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.write_timeout_report(rd, [{'name': 'stuck'}], 3600, + prefix='> **wedged usb_hub_wq worker.**\n') + hil_report.mark_report_abandoned(rd, 'the worker pool would not shut down.') + doc = json.loads((rd / hil_report.REPORT_JSON).read_text()) + self.assertIn('timed out after 3600s', doc['caveat']) + self.assertIn('wedged usb_hub_wq worker', doc['banner']) # rig health, not outcome + self.assertNotIn('would not shut down', doc['caveat']) + + +class CaveatSurvivesAccumulate(unittest.TestCase): + """CI reruns with --accumulate: the sidecar keeps every earlier attempt's cells, but the + banner was recomputed per attempt. A first attempt on a degraded rig and a clean rerun + therefore published the degraded attempt's PASSES with no caveat on them -- and the + generated .failed spec reruns only failures, so those cells are never re-earned.""" + + def _rows(self, board, cell): + return [(board, 0, 0, [(board, {cell: 'OK'}, '1s')], 0)] + + def test_an_earlier_attempts_caveat_is_still_on_the_report(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + banner = '> **Rig note.** 2 process(es) in D state at start.\n' + + hil_report.accumulate_report(self._rows('boardA', 'cdc_msc'), rd, True, '', banner) + self.assertIn('Rig note', (rd / hil_report.REPORT_MD).read_text()) + + # the rerun: clean rig, so this attempt contributes no banner of its own + md = hil_report.accumulate_report(self._rows('boardB', 'cdc_msc'), rd, False, '', '') + self.assertIn('boardA', md) # the earlier cells are kept ... + self.assertIn('Rig note', md, + 'the caveat the earlier cells were collected under was dropped') + + def test_the_same_caveat_twice_is_not_stacked(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + banner = '> **Rig note.** 2 process(es) in D state at start.\n' + hil_report.accumulate_report(self._rows('boardA', 'cdc_msc'), rd, True, '', banner) + md = hil_report.accumulate_report(self._rows('boardB', 'cdc_msc'), rd, False, '', banner) + self.assertEqual(md.count('Rig note'), 1) + + +class MarkdownIsAlwaysARenderingOfTheJson(unittest.TestCase): + """The property this whole change buys: whatever wrote the report, re-rendering the + sidecar reproduces the markdown byte for byte. Four writers, one renderer -- asserted + directly rather than inferred from the writers.""" + + def _check(self, rd): + 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') + + def test_normal_path(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report( + [('boardA', 0, 0, [('boardA', {'cdc_msc': 'OK'}, '1s')], 0)], rd, True, + '-b boardA', '> **Rig note.** x\n') + self._check(rd) + + def test_after_an_accumulate_rerun(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report( + [('boardA', 0, 0, [('boardA', {'cdc_msc': 'OK'}, '1s')], 0)], rd, True, '', '') + hil_report.accumulate_report( + [('boardB', 0, 0, [('boardB', {'cdc_msc': 'OK'}, '1s')], 0)], rd, False, '', '') + self._check(rd) + + def test_after_abandonment(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report( + [('boardA', 0, 0, [('boardA', {'cdc_msc': 'OK'}, '1s')], 0)], rd, True, '', '') + hil_report.mark_report_abandoned(rd, 'the worker pool would not shut down.') + self._check(rd) + + def test_no_boards_exit(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.write_report(rd, {'rows': [], 'banner': '', 'scope': '', + 'caveat': '**HIL run selected no boards.** why\n'}) + self._check(rd) + + def test_the_pool_guard_fallback(self): + """The last writer to join the invariant: it composed its own markdown only because + hil_health could not import the renderer.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report( + [('done', 0, 0, [('done', {'cdc_msc': 'OK'}, '1s')], 0)], rd, True, '', '') + hil_report.write_timeout_report(rd, [{'name': 'stuck'}], 3600, + prefix='> **wedged usb_hub_wq worker.**\n') + self._check(rd) + +class WriteTimeoutReport(unittest.TestCase): + def test_prefix_carries_the_preflight_diagnosis(self): + """The timeout aborts before accumulate_report, so without the prefix the artifact + and the PR comment lose the one line saying WHY the pool never finished.""" + with TemporaryDirectory() as td: + d = Path(td) + hil_report.write_timeout_report(d, [{'name': 'b1'}], 4200, + prefix='> **wedged usb_hub_wq worker.**\n') + out = (d / hil_report.REPORT_MD).read_text() + # the abandon notice leads (run outcome), the rig-health prefix follows in the + # banner -- prefix used to be folded INTO the caveat, which is what made a clean + # --accumulate retry inherit an abandonment that had not happened + self.assertTrue(out.startswith('**HIL run abandoned: worker pool timed out'), out[:80]) + self.assertIn('> **wedged usb_hub_wq worker.**', out) + self.assertIn('timed out after 4200s', out) + self.assertIn('- b1', out) + + def test_writes_a_report_where_there_would_be_none(self): + with TemporaryDirectory() as td: + hil_report.write_timeout_report(Path(td), [{'name': 'ra6m5_ek'}], 4200) + md = (Path(td) / hil_report.REPORT_MD).read_text() + self.assertIn('4200s', md) + self.assertIn('ra6m5_ek', md) + + def test_the_prior_attempts_rows_survive(self): + """Was: the prior MARKDOWN TEXT survives below the banner. It now re-renders from + the merged sidecar, so the guarantee is stated against rows -- one table with the + stuck boards in it, rather than a banner stapled above a duplicate table.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report( + [('done', 0, 0, [('done', {'cdc_msc': 'OK'}, '1s')], 0)], rd, True, '', '') + hil_report.write_timeout_report(rd, [{'name': 'stuck'}], 3600) + doc = json.loads((rd / hil_report.REPORT_JSON).read_text()) + self.assertEqual([r['board'] for r in doc['rows']], ['done', 'stuck']) + md = (rd / hil_report.REPORT_MD).read_text() + self.assertIn('done', md) + self.assertIn('stuck', md) + self.assertIn('abandoned', md) + self.assertLess(md.index('abandoned'), md.index('done')) + self.assertEqual(md.count('| Board'), 1, 'the prior table was duplicated, not merged') + + def test_custom_banner_is_used(self): + with TemporaryDirectory() as td: + hil_report.write_timeout_report(Path(td), [], 0, + banner='**refused to start.**\n') + self.assertIn('refused to start', (Path(td) / hil_report.REPORT_MD).read_text()) + + def test_timeout_report_writes_the_sidecar(self): + """This path used to write markdown only, so summarize() -- which is all an + agent gets -- reported the whole fleet as 'no report row' on exactly the runs + that failed.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.write_timeout_report(rd, [{'name': 'boardA'}], 3600) + self.assertTrue((rd / hil_report.REPORT_JSON).is_file()) + self.assertIn('boardA', (rd / hil_report.REPORT_JSON).read_text()) + + def test_the_sidecar_keeps_a_previous_attempts_rows(self): + """An earlier attempt's finished boards are real results and this attempt has none + of its own, so the rows merge rather than replace.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + (rd / hil_report.REPORT_JSON).write_text(json.dumps( + {'rows': [{'board': 'done', 'cells': {'cdc_msc': 'pass'}, 'duration': '1s'}], + 'banner': '', 'scope': '', 'caveat': ''})) + hil_report.write_timeout_report(rd, [{'name': 'stuck'}], 3600) + rows = json.loads((rd / hil_report.REPORT_JSON).read_text())['rows'] + self.assertEqual([r['board'] for r in rows], ['done', 'stuck']) + + def test_a_torn_sidecar_does_not_lose_the_stuck_boards(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + (rd / hil_report.REPORT_JSON).write_text('{ truncated mid-') + hil_report.write_timeout_report(rd, [{'name': 'stuck'}], 3600) + rows = json.loads((rd / hil_report.REPORT_JSON).read_text())['rows'] + self.assertEqual([r['board'] for r in rows], ['stuck']) + + def test_a_roster_entry_without_a_name_does_not_escape(self): + """The broad handler exists to stop a KeyError here stranding the runner, but a + report that silently loses its only board is worse than one saying '?'.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.write_timeout_report(rd, [{}], 3600) + rows = json.loads((rd / hil_report.REPORT_JSON).read_text())['rows'] + self.assertEqual([r['board'] for r in rows], ['?']) + + def test_unwritable_dir_does_not_raise(self): + """The caller may be about to os._exit; losing the report must not also lose the + exit path.""" + hil_report.write_timeout_report(Path('/proc/nonexistent/nope'), [], 0) + + +class SummaryFoldsReportToBoards(unittest.TestCase): + """summarize() replaces the agent retyping the markdown table. Report rows are named per + VARIANT and a variant need not start with the board name, so the config is what maps them + back -- the previous string-matching design produced a defect in each of four review rounds.""" + + def _sum(self, boards, rows, cfg_boards=None, banner=''): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + d = Path(td.name) + (d / 'hil_report.json').write_text(json.dumps( + {'rows': [{'board': b, 'cells': c, 'duration': '1s'} for b, c in rows], + 'banner': banner})) + cfg = d / 'cfg.json' + cfg.write_text(json.dumps({'boards': cfg_boards or [{'name': b} for b in boards]})) + args = [a for b in boards for a in ('-b', b)] + r = subprocess.run(['python3', str(Path(TEST_DIR).parents[0] / 'helper' / 'hil_report.py'), + str(cfg), *args, '--report-dir', str(d)], + capture_output=True, text=True, timeout=60) + self.assertEqual(r.returncode, 0, r.stderr) + return json.loads(r.stdout)['results'] + + def test_variant_rows_fold_onto_their_board(self): + """nanoch32v203 never produces a row named after the board.""" + got = self._sum(['nanoch32v203'], + [('nanoch32v203-fsdev', {'usbtest': 'pass'}), + ('nanoch32v203-usbfs', {'usbtest': 'pass'})], + cfg_boards=[{'name': 'nanoch32v203', + 'variant': [{'name': 'nanoch32v203-fsdev'}, + {'name': 'nanoch32v203-usbfs'}]}]) + self.assertEqual([r['board'] for r in got], ['nanoch32v203']) + self.assertTrue(got[0]['pass']) + self.assertTrue(got[0]['ran']) + + def test_one_failing_variant_fails_the_board(self): + got = self._sum(['nano'], + [('nano-a', {'usbtest': 'pass'}), ('nano-b', {'usbtest': '❌ 29/30'})], + cfg_boards=[{'name': 'nano', 'variant': [{'name': 'nano-a'}, + {'name': 'nano-b'}]}]) + self.assertFalse(got[0]['pass']) + self.assertIn('29/30', got[0]['detail']) + + def test_lock_contention_is_a_field_not_a_prefix(self): + got = self._sum(['alpha'], [('alpha', {'board-locked': 'fail'})]) + self.assertTrue(got[0]['locked']) + self.assertFalse(got[0]['pass']) + + def test_a_board_with_no_row_is_marked_not_run(self): + got = self._sum(['alpha', 'beta'], [('alpha', {'usbtest': 'pass'})]) + self.assertTrue(got[0]['ran']) + self.assertFalse(got[1]['ran']) + self.assertFalse(got[1]['pass']) + + def test_a_metric_cell_counts_by_its_icon(self): + got = self._sum(['a', 'b'], [('a', {'cdc_msc_throughput': '✅ C 1.2 M 3.4'}), + ('b', {'cdc_msc_throughput': '❌ C 0.0 M 0.0'})]) + self.assertTrue(got[0]['pass']) + self.assertFalse(got[1]['pass']) + + def test_skipped_cells_do_not_fail_a_board(self): + got = self._sum(['a'], [('a', {'usbtest': 'skip', 'cdc_msc': 'pass'})]) + self.assertTrue(got[0]['pass']) + + def test_a_plain_metric_cell_is_a_pass(self): + """Mirrors hil_test.py's own tally (cell_kind): failures are ALWAYS marked -- 'fail' + or a ❌ prefix, per TestFail's docstring -- while a passing test may return a plain + metric string that lands in the cell unprefixed. Treating unknown shapes as fail + would publish a green table as a red verdict.""" + got = self._sum(['a'], [('a', {'device_speed': '480.0 MBps'})]) + self.assertTrue(got[0]['pass']) + + def test_a_declared_variant_of_another_board_is_not_stolen(self): + """A declared variant need not start with its own board's name, so it may start with + a DIFFERENT board's name plus '-'. The prefix fallback must not attribute it twice.""" + got = self._sum(['alpha', 'beta'], + [('beta-x', {'usbtest': 'fail'})], + cfg_boards=[{'name': 'alpha', 'variant': [{'name': 'beta-x'}]}, + {'name': 'beta'}]) + self.assertTrue(got[0]['ran']) + self.assertFalse(got[0]['pass']) + self.assertFalse(got[1]['ran'], "beta must not inherit alpha's row") + + + def test_the_caveat_reaches_the_agents_verdict(self): + """The abandon/no-boards notice lives in the document now, and this JSON is all an + agent gets -- dropping it here puts the caveat back where only a human sees it.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + d = Path(td.name) + (d / 'hil_report.json').write_text(json.dumps( + {'rows': [{'board': 'boardA', 'cells': {'cdc_msc': 'pass'}, 'duration': '1s'}], + 'banner': '', 'scope': '', + 'caveat': '**HIL run abandoned: the worker pool would not shut down.**\n'})) + cfg = d / 'cfg.json' + cfg.write_text(json.dumps({'boards': [{'name': 'boardA'}]})) + r = subprocess.run( + ['python3', str(Path(TEST_DIR).parents[0] / 'helper' / 'hil_report.py'), + str(cfg), '-b', 'boardA', '--report-dir', str(d)], + capture_output=True, text=True, timeout=60) + self.assertEqual(r.returncode, 0, r.stderr) + self.assertIn('abandoned', json.loads(r.stdout)['caveat']) + + def test_an_older_sidecar_without_a_caveat_still_summarises(self): + got = self._sum(['boardA'], [('boardA', {'cdc_msc': 'pass'})]) + self.assertTrue(got[0]['pass']) + + def test_the_old_entry_point_is_gone(self): + """hil_summary.py's CLI moved here. A leftover file would keep working while + drifting from the module that now owns the fold.""" + self.assertFalse((Path(HIL_DIR) / 'helper' / 'hil_summary.py').exists()) + + +class AbandonStampIsNotDestructive(unittest.TestCase): + """mark_report_abandoned runs on the way to os._exit, on a report it did not write. + Every case here was a live regression found by review.""" + + def _doc(self, **kw): + d = {'rows': [{'board': 'OLD', 'cells': {'t': 'pass'}, 'duration': '9s'}], + 'banner': '', 'scope': '', 'caveat': ''} + d.update(kw) + return d + + def test_declining_to_stamp_does_not_republish_the_markdown(self): + """The guard skipped the caveat assignment but write_report ran anyway, so a + no-op call still overwrote THIS run's table with a re-render of an older sidecar.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + (rd / hil_report.REPORT_JSON).write_text(json.dumps(self._doc( + caveat='**HIL run abandoned: worker pool timed out after 3600s.**\n'))) + (rd / hil_report.REPORT_MD).write_text('THIS RUN table with boardX\n') + hil_report.mark_report_abandoned(rd, 'the worker pool would not shut down.') + self.assertEqual((rd / hil_report.REPORT_MD).read_text(), + 'THIS RUN table with boardX\n') + + def test_a_banner_borne_abandon_notice_also_wins(self): + """The pool-timeout path puts its notice in `banner` (hil_test.py:2300), not + `caveat`. SKILL.md gives the two notices OPPOSITE rules, so stamping the vaguer + one on top tells the agent to publish rows it is meant to discard.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report( + [('a', 0, 0, [('a', {'t': 'OK'}, '1s')], 0)], rd, True, '', '', + caveat='**HIL run abandoned: worker pool timed out after 3600s.** 2 never' + ' reported.\n') + hil_report.mark_report_abandoned(rd, 'the worker pool would not shut down.') + md = (rd / hil_report.REPORT_MD).read_text() + self.assertTrue(md.startswith('**HIL run abandoned: worker pool timed out'), md[:80]) + self.assertNotIn('would not shut down', md) + + def test_a_missing_sidecar_still_stamps_the_markdown(self): + """Master read the MARKDOWN and prepended unconditionally, so it always stamped. + pr_comment.yml cats only hil_report.md -- giving up here publishes a clean green + table under an abandoned, non-zero job.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + (rd / hil_report.REPORT_MD).write_text('**✅ 27 passed · ❌ 0 failed**\n') + hil_report.mark_report_abandoned(rd, 'the worker pool would not shut down.') + md = (rd / hil_report.REPORT_MD).read_text(encoding='utf-8') + self.assertIn('abandoned', md) + self.assertIn('27 passed', md) + + def test_a_torn_sidecar_still_stamps_the_markdown(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + (rd / hil_report.REPORT_JSON).write_text('{ truncated mid-') + (rd / hil_report.REPORT_MD).write_text('**✅ 27 passed**\n') + hil_report.mark_report_abandoned(rd, 'the worker pool would not shut down.') + self.assertIn('abandoned', (rd / hil_report.REPORT_MD).read_text(encoding='utf-8')) + + def test_the_wording_matches_the_skill_contract(self): + """SKILL.md pins this banner as 'the table below IS this run's ... Report the + results AND the abandonment'. Calling it 'partial' sends the agent to re-run + boards that already passed.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + (rd / hil_report.REPORT_JSON).write_text(json.dumps(self._doc())) + hil_report.mark_report_abandoned(rd, 'the worker pool would not shut down.') + caveat = json.loads((rd / hil_report.REPORT_JSON).read_text())['caveat'] + self.assertNotIn('partial', caveat) + self.assertIn('unverified', caveat) + + +class WriteReportFailsLoudly(unittest.TestCase): + def test_a_render_failure_does_not_leave_a_committed_json(self): + """It wrote the JSON, then rendered. A render raise left the sidecar saying + 'abandoned' beside a markdown that still read as a clean green table -- breaking + the one invariant this module exists to hold.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + (rd / hil_report.REPORT_MD).write_text('STALE GREEN TABLE\n') + (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.') + doc = json.loads((rd / hil_report.REPORT_JSON).read_text()) + md = (rd / hil_report.REPORT_MD).read_text() + # either both moved or neither did -- never a sidecar the markdown contradicts + self.assertEqual('abandoned' in doc.get('caveat', ''), 'abandoned' in md, + 'the sidecar was committed without its markdown') + + def test_a_non_dict_row_does_not_cost_the_abandon_stamp(self): + """A row that is a bare string raised out of render_report, so the stamp was lost + entirely -- the failure mode this whole function exists to prevent.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + (rd / hil_report.REPORT_JSON).write_text(json.dumps( + {'rows': ['boardA', {'board': 'good', 'cells': {'t': 'pass'}}], + 'banner': '', 'scope': '', 'caveat': ''})) + hil_report.mark_report_abandoned(rd, 'the worker pool would not shut down.') + md = (rd / hil_report.REPORT_MD).read_text() + self.assertIn('abandoned', md) + self.assertIn('good', md) + + def test_an_unwritable_dir_reaches_the_callers_warning(self): + """write_report swallowing OSError made write_timeout_report's broad handler -- + and hil_test's fallback-of-the-fallback -- dead code: no artifact, no message.""" + import io + from contextlib import redirect_stdout + buf = io.StringIO() + with redirect_stdout(buf): + hil_report.write_timeout_report(Path('/proc/nonexistent/nope'), + [{'name': 'b1'}], 3600, prefix='x\n') + self.assertIn('warning', buf.getvalue().lower(), 'the failure was silent') + + +class PoolTimeoutCellIsHonest(unittest.TestCase): + def test_a_stuck_board_with_a_prior_row_still_gets_the_cell(self): + """`not in done` skipped the cell for any board carrying an earlier attempt's row, + so a board that just ate the 60-minute guard summarized as pass:true.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report( + [('stm32f4', 0, 0, [('stm32f4', {'cdc_msc': 'OK'}, '1s')], 0)], rd, True, '', '') + hil_report.write_timeout_report(rd, [{'name': 'stm32f4'}], 3600) + doc = json.loads((rd / hil_report.REPORT_JSON).read_text()) + verdict = hil_report.summarize({'boards': [{'name': 'stm32f4'}]}, ['stm32f4'], doc) + self.assertFalse(verdict['results'][0]['pass'], + 'a board that hung the pool was published as a pass') + + def test_a_clean_retry_clears_the_cell(self): + """accumulate_report clears stale board-locked and BOUNDARY_CELL cells but not + this one, so a board that passed clean on the retry stayed red forever.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.write_timeout_report(rd, [{'name': 'stuck'}], 3600) + hil_report.accumulate_report( + [('stuck', 0, 0, [('stuck', {'cdc_msc': 'OK'}, '2s')], 0)], rd, False, '', '') + doc = json.loads((rd / hil_report.REPORT_JSON).read_text()) + self.assertNotIn('pool-timeout', doc['rows'][0]['cells']) + verdict = hil_report.summarize({'boards': [{'name': 'stuck'}]}, ['stuck'], doc) + self.assertTrue(verdict['results'][0]['pass']) + + def test_a_torn_sidecar_does_not_destroy_an_intact_markdown(self): + """Re-rendering from an unusable sidecar threw away real results the human copy + still had. Master concatenated below its banner and kept them.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + (rd / hil_report.REPORT_MD).write_text('| Board | t |\n| a | OK |\n| b | OK |\n') + (rd / hil_report.REPORT_JSON).write_text('{ truncated') + hil_report.write_timeout_report(rd, [{'name': 'stuck'}], 3600) + md = (rd / hil_report.REPORT_MD).read_text() + self.assertIn('| a | OK |', md, "an earlier attempt's real results were destroyed") + self.assertIn('stuck', md) + + +class SummarizeSeesEveryRow(unittest.TestCase): + def test_board_name_rows_reach_a_variant_boards_verdict(self): + """hil_test writes lock-contention and pool-timeout rows keyed by BOARD name, but + variants_of returns only declared variant names -- so for nanoch32v203 and + ch32v307v_r1_1v0 those rows were invisible and a held lock published as a + hardware FAIL that hil-validate.js never retried.""" + cfg = {'boards': [{'name': 'nano', + 'variant': [{'name': 'nano-fsdev'}, {'name': 'nano-usbfs'}]}]} + doc = {'rows': [{'board': 'nano', 'cells': {'board-locked': 'fail'}, + 'duration': None}], 'banner': '', 'scope': '', 'caveat': ''} + r = hil_report.summarize(cfg, ['nano'], doc)['results'][0] + self.assertTrue(r['ran']) + self.assertTrue(r['locked'], 'a held lock was published as a hardware failure') + + def test_a_malformed_row_does_not_kill_the_cli(self): + """summarize is the one reader with no defense, and it is the only one an agent's + verdict depends on.""" + out = hil_report.summarize({'boards': [{'name': 'a'}]}, ['a'], + {'rows': [{'cells': {}}, {'board': 'a', + 'cells': {'t': 'pass'}}]}) + self.assertTrue(out['results'][0]['pass']) + + +class NoBoardsExitKeepsWhatRan(unittest.TestCase): + def test_it_does_not_wipe_an_accumulated_sidecar(self): + """Master wrote only markdown here, so the sidecar survived. Writing rows:[] + unconditionally makes an --accumulate rerun whose filters empty erase every + board that had already passed.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report( + [('a', 0, 0, [('a', {'t': 'OK'}, '1s')], 0)], rd, True, '', '') + hil_report.mark_report_no_boards(rd, 'No boards left after the flasher filter', + fresh=False) + doc = json.loads((rd / hil_report.REPORT_JSON).read_text()) + self.assertEqual([r['board'] for r in doc['rows']], ['a']) + self.assertIn('selected no boards', doc['caveat']) + self.assertIn('selected no boards', (rd / hil_report.REPORT_MD).read_text()) + + +class TheMergeBehavioursAreActuallyPinned(unittest.TestCase): + """accumulate_report's docstring cites these three as the reason not to split it, yet + deleting any of them left the whole suite green. Mutation-verified.""" + + def test_a_cleared_boundary_drops_the_previous_attempts_mark(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report( + [('b', 0, 0, [('b-v', {hil_report.BOUNDARY_CELL: 'fail'}, '1s')], 0)], + rd, True, '', '') + hil_report.accumulate_report( + [('b', 0, 0, [('b-v', {'cdc_msc': 'OK'}, '2s')], 0)], rd, False, '', '') + cells = json.loads((rd / hil_report.REPORT_JSON).read_text())['rows'][0]['cells'] + self.assertNotIn(hil_report.BOUNDARY_CELL, cells) + + def test_a_board_that_really_ran_drops_its_stale_lock_cell(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report( + [('b', 0, 0, [('b', {hil_report.LOCKED_CELL: 'fail'}, None)], 0)], rd, True, '', '') + hil_report.accumulate_report( + [('b', 0, 0, [('b', {'cdc_msc': 'OK'}, '2s')], 0)], rd, False, '', '') + rows = json.loads((rd / hil_report.REPORT_JSON).read_text())['rows'] + self.assertEqual([r['board'] for r in rows], ['b']) + self.assertNotIn(hil_report.LOCKED_CELL, rows[0]['cells']) + + def test_a_filtered_rerun_keeps_the_previous_duration(self): + """A -t-filtered re-run reports duration None; blanking the column loses the only + record of how long the full run took.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report( + [('b', 0, 0, [('b', {'cdc_msc': 'OK'}, '119s')], 0)], rd, True, '', '') + hil_report.accumulate_report( + [('b', 0, 0, [('b', {'cdc_msc': 'OK'}, None)], 0)], rd, False, '', '') + self.assertEqual(json.loads( + (rd / hil_report.REPORT_JSON).read_text())['rows'][0]['duration'], '119s') + + +class TheFooterCountsAreNotSwapped(unittest.TestCase): + """SKILL.md tells the operator to paste the footer counts verbatim, and swapping the + failed/skipped tallies left the suite green.""" + + def test_each_kind_is_counted_under_its_own_label(self): + md = hil_report.render_matrix([ + ('b', {'p1': 'pass', 'p2': 'pass', 'f1': 'fail', + 's1': f'{hil_report.REPORT_CELL["skip"]} board wedged'}, '1s')]) + self.assertIn(f'{hil_report.REPORT_CELL["pass"]} 2 passed', md) + self.assertIn(f'{hil_report.REPORT_CELL["fail"]} 1 failed', md) + self.assertIn(f'{hil_report.REPORT_CELL["skip"]} 1 skipped', md) + + +class HilCiUploadsTheAccumulateMergeBase(unittest.TestCase): + """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 without an upload a remote + `--accumulate` retry silently starts from nothing and its one-row table REPLACES the + full-fleet one. The copy-back at the end has always existed; the upload did not.""" + + def _gate(self, *args): + """Run the real gate block out of hil_ci.sh and return its ACCUMULATE verdict. + + Executed, not grepped: the previous pair of tests searched the source text and + stayed green when `if [ "$ACCUMULATE" = 1 ]` was mutated to `if true`, because the + comment block above it mentions --accumulate five times.""" + sh = (Path(HIL_DIR) / 'hil_ci.sh').read_text(encoding='utf-8') + a = sh.index('ACCUMULATE=$(python3 -') + b = sh.index(') || ACCUMULATE=0', a) + len(') || ACCUMULATE=0') + script = 'ARGS=("$@")\n' + sh[a:b] + '\necho "$ACCUMULATE"' + r = subprocess.run(['bash', '-c', script, '_', *args], + capture_output=True, text=True, timeout=60) + self.assertEqual(r.returncode, 0, r.stderr) + return r.stdout.strip() + + def test_every_spelling_argparse_accepts_is_detected(self): + """hil_test.py declares `-a, --accumulate`, so argparse also takes -av, -va, + --accum and --acc; hil-validate.js tells the operator to retry 'adding -v'.""" + for spelling in ('--accumulate', '-a', '-av', '-va', '--accum', '--acc'): + self.assertEqual(self._gate(spelling), '1', f'{spelling} was not detected') + + def test_a_run_without_it_is_not_treated_as_accumulate(self): + for spelling in ('-b', '-v', '--retry'): + self.assertEqual(self._gate(spelling), '0', f'{spelling} falsely detected') + + def test_the_sidecar_is_uploaded_and_gated(self): + sh = (Path(HIL_DIR) / 'hil_ci.sh').read_text(encoding='utf-8') + up = [ln for ln in sh.splitlines() + if 'scp' in ln and 'hil_report.json' in ln and '$REMOTE:' in ln] + self.assertTrue(up, 'nothing uploads hil_report.json; --accumulate has no merge base') + self.assertIn('if [ "$ACCUMULATE" = 1 ]', sh, 'the upload is not gated') + + def test_a_missing_merge_base_is_loud(self): + """The damage: --accumulate with nothing to merge onto succeeds and quietly + publishes a small table where a full one used to be.""" + 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') + + +class RunOutcomeAndRigHealthAreSeparate(unittest.TestCase): + """`banner` describes the CONDITIONS cells were collected under, so it carries across a + retry. `caveat` describes how a RUN ENDED, so it must not: a clean retry that reports + an earlier attempt's abandonment tells the agent a green run failed.""" + + def test_a_clean_retry_drops_the_previous_abandon_notice(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report([('a', 0, 0, [('a', {'t': 'OK'}, '1s')], 0)], + rd, True, '', '') + hil_report.mark_report_abandoned(rd, 'the worker pool would not shut down.') + hil_report.accumulate_report([('a', 0, 0, [('a', {'t': 'OK'}, '2s')], 0)], + rd, False, '', '') + self.assertEqual(json.loads((rd / hil_report.REPORT_JSON).read_text())['caveat'], '') + + def test_rig_health_still_carries_across_the_retry(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.write_timeout_report(rd, [{'name': 's'}], 3600, + prefix='> **Rig note.** wedged\n') + hil_report.accumulate_report([('a', 0, 0, [('a', {'t': 'OK'}, '1s')], 0)], + rd, False, '', '') + doc = json.loads((rd / hil_report.REPORT_JSON).read_text()) + self.assertIn('Rig note', doc['banner']) + + def test_a_second_attempts_abandon_is_recorded(self): + """_already_abandoned matched a notice carried forward from an EARLIER attempt, so + a genuinely new abandon wrote nothing and the run's own failure vanished.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report( + [('a', 0, 0, [('a', {'t': 'OK'}, '1s')], 0)], rd, True, '', + '> **Rig note.** x\n', + caveat='**HIL run abandoned: worker pool timed out after 3600s.**\n') + hil_report.accumulate_report([('a', 0, 0, [('a', {'t': 'OK'}, '2s')], 0)], + rd, False, '', '') + hil_report.mark_report_abandoned(rd, 'the worker pool would not shut down.') + self.assertIn('would not shut down', + json.loads((rd / hil_report.REPORT_JSON).read_text())['caveat']) + + +class AMalformedSidecarNeverCostsTheReport(unittest.TestCase): + """hil_ci.sh now uploads a sidecar as the merge base, so a non-conforming one is + reachable from outside the harness.""" + + def _write(self, rd, doc): + (rd / hil_report.REPORT_JSON).write_text(json.dumps(doc)) + + def test_a_null_banner_does_not_kill_a_successful_run(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + self._write(rd, {'rows': [{'board': 'a', 'cells': {'t': 'pass'}, 'duration': '1s'}], + 'banner': None, 'caveat': None, 'scope': ''}) + hil_report.accumulate_report([('b', 0, 0, [('b', {'t': 'OK'}, '1s')], 0)], + rd, False, '', '') + self.assertTrue((rd / hil_report.REPORT_MD).is_file()) + + def test_a_null_cells_row_still_gets_its_pool_timeout_cell(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + self._write(rd, {'rows': [{'board': 'boardA', 'cells': None, 'duration': '61s'}], + 'banner': '', 'caveat': '', 'scope': ''}) + hil_report.write_timeout_report(rd, [{'name': 'boardA'}], 3600) + doc = json.loads((rd / hil_report.REPORT_JSON).read_text()) + v = hil_report.summarize({'boards': [{'name': 'boardA'}]}, ['boardA'], doc) + self.assertFalse(v['results'][0]['pass'], + 'a board that ate the whole pool guard was published as a pass') + + def test_an_awkward_sidecar_still_gets_the_abandon_stamp(self): + """Any raise inside the dict branch was swallowed and the markdown fallback was + unreachable, so the stamp was lost from BOTH artifacts.""" + for bad in ({'rows': [{'board': 'a', 'cells': {'t': 'p'}, 'duration': 120}], + 'banner': '', 'caveat': '', 'scope': ''}, + {'rows': [{'board': 'a', 'cells': {'t': ['x']}, 'duration': '1s'}], + 'banner': None, 'caveat': '', 'scope': ''}): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + self._write(rd, bad) + (rd / hil_report.REPORT_MD).write_text('**✅ 27 passed**\n') + hil_report.mark_report_abandoned(rd, 'the worker pool would not shut down.') + self.assertIn('abandoned', (rd / hil_report.REPORT_MD).read_text(encoding='utf-8'), + f'no stamp for {bad}') + + def test_a_malformed_roster_entry_still_leaves_an_artifact(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + import io + from contextlib import redirect_stdout + with redirect_stdout(io.StringIO()): + hil_report.write_timeout_report(rd, ['plainstring'], 3600) + self.assertTrue((rd / hil_report.REPORT_MD).is_file(), 'no artifact at all') + + +class PoolTimeoutOutranksAStaleLock(unittest.TestCase): + def test_a_wedge_is_not_published_as_lock_contention(self): + """locked was computed across every cell and short-circuited detail, so a board + that wedged the rig on the retry was reported as LOCKED -- and hil-validate.js + re-runs those, paying another pool guard on a board that just hung it.""" + doc = {'rows': [{'board': 'boardX', + 'cells': {'board-locked': 'fail', 'pool-timeout': 'fail'}, + 'duration': None}], 'banner': '', 'caveat': '', 'scope': ''} + r = hil_report.summarize({'boards': [{'name': 'boardX'}]}, ['boardX'], doc)['results'][0] + self.assertFalse(r['locked'], 'a wedge was published as lock contention') + self.assertFalse(r['pass']) + + +class NoBoardsExitRespectsFreshness(unittest.TestCase): + def test_a_fresh_run_does_not_republish_the_previous_rows(self): + """It is called BEFORE the fresh wipe, so it re-published last run's green table + under this run's red job -- the stale-table failure it exists to prevent.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report([('a', 0, 0, [('a', {'t': 'OK'}, '1s')], 0)], + rd, True, '', '') + hil_report.mark_report_no_boards(rd, 'filters emptied', fresh=True) + self.assertEqual(json.loads((rd / hil_report.REPORT_JSON).read_text())['rows'], []) + + def test_an_accumulate_run_keeps_them(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report([('a', 0, 0, [('a', {'t': 'OK'}, '1s')], 0)], + rd, True, '', '') + hil_report.mark_report_no_boards(rd, 'filters emptied', fresh=False) + self.assertEqual([r['board'] for r in json.loads( + (rd / hil_report.REPORT_JSON).read_text())['rows']], ['a']) + + def test_it_does_not_overwrite_an_abandon_notice(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report([('a', 0, 0, [('a', {'t': 'OK'}, '1s')], 0)], + rd, True, '', '') + hil_report.mark_report_abandoned(rd, 'the worker pool would not shut down.') + hil_report.mark_report_no_boards(rd, 'filters emptied', fresh=False) + self.assertIn('abandoned', + json.loads((rd / hil_report.REPORT_JSON).read_text())['caveat']) + + +class TheNoBoardsCallSiteIsWired(unittest.TestCase): + """The fresh/accumulate branches of mark_report_no_boards were tested by calling it + DIRECTLY, so both passed while hil_test.py's one real call site never passed the flag + at all -- an --accumulate run whose filter emptied still wiped the accumulated rows. + This drives hil_test.py itself; the no-boards exit needs only a config and a filter + that matches nothing, so it costs no hardware.""" + + def _run(self, *extra): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + (rd / 'cfg.json').write_text(json.dumps( + {'boards': [{'name': 'alpha', 'uid': '1', 'flasher': {'name': 'jlink', 'uid': '2'}}]})) + (rd / hil_report.REPORT_JSON).write_text(json.dumps( + {'rows': [{'board': 'earlier', 'cells': {'t': 'pass'}, 'duration': '1s'}], + 'banner': '', 'scope': '', 'caveat': ''})) + r = subprocess.run( + [sys.executable, str(Path(HIL_DIR) / 'hil_test.py'), + '--flasher', 'nonexistent', *extra, str(rd / 'cfg.json')], + capture_output=True, text=True, timeout=120, + env={**os.environ, 'HIL_REPORT_DIR': str(rd)}) + self.assertEqual(r.returncode, 1, r.stdout + r.stderr) + return json.loads((rd / hil_report.REPORT_JSON).read_text()) + + def test_an_accumulate_run_keeps_the_accumulated_rows(self): + doc = self._run('--accumulate') + self.assertEqual([r['board'] for r in doc['rows']], ['earlier'], + "the call site did not pass fresh=not args.accumulate") + self.assertIn('selected no boards', doc['caveat']) + + def test_a_fresh_run_does_not_republish_them(self): + doc = self._run() + self.assertEqual(doc['rows'], []) + self.assertIn('selected no boards', doc['caveat']) + + +class EveryWriterRendersBeforeItCommits(unittest.TestCase): + def test_accumulate_report_does_not_commit_json_then_fail_to_render(self): + """accumulate_report hand-rolled the write instead of calling write_report, so a + render failure left the sidecar ahead of the markdown -- the exact ordering + write_report's docstring forbids.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + (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') + + +class MissingSidecarDoesNotDestroyTheMarkdown(unittest.TestCase): + def test_an_absent_sidecar_keeps_the_prior_table(self): + """`recovered` was only cleared when the sidecar was TORN, not when it was absent + -- reachable from hil_ci.sh's asymmetric copy-back and build.yml's skip marker.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + (rd / hil_report.REPORT_MD).write_text('| Board | t |\n| a | OK |\n| b | OK |\n') + hil_report.write_timeout_report(rd, [{'name': 'stuck'}], 3600) + self.assertIn('| a | OK |', (rd / hil_report.REPORT_MD).read_text()) + + +class LoadIsTheOnlyTrustBoundary(unittest.TestCase): + """hil_ci.sh uploads a sidecar as the merge base, so these shapes arrive from OUTSIDE + the harness. Every one of these raised past a handler before.""" + + def _seed(self, rd, raw): + (rd / hil_report.REPORT_JSON).write_text(raw if isinstance(raw, str) + else json.dumps(raw)) + + def test_a_non_list_rows_does_not_raise(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + self._seed(rd, {'rows': 1, 'banner': '', 'caveat': '', 'scope': ''}) + hil_report.accumulate_report([('a', 0, 0, [('a', {'t': 'OK'}, '1s')], 0)], + rd, False, '', '') + self.assertTrue((rd / hil_report.REPORT_MD).is_file()) + + def test_an_unhashable_cell_value_does_not_raise(self): + """render_matrix does REPORT_CELL.get(v, v); an unhashable value raised TypeError + on the NORMAL accumulate path.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + self._seed(rd, {'rows': [{'board': 'a', 'cells': {'t': ['x'], 'u': 'pass'}, + 'duration': '1s'}], + 'banner': '', 'caveat': '', 'scope': ''}) + hil_report.accumulate_report([('b', 0, 0, [('b', {'t': 'OK'}, '1s')], 0)], + rd, False, '', '') + cells = {r['board']: r['cells'] + for r in json.loads((rd / hil_report.REPORT_JSON).read_text())['rows']} + self.assertNotIn('t', cells['a'], 'a corrupt cell must drop, not become a pass') + self.assertIn('u', cells['a']) + + def test_summarize_survives_a_malformed_sidecar_via_load(self): + """The CLI is the one reader an agent's verdict depends on.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + (rd / 'cfg.json').write_text(json.dumps({'boards': [{'name': 'a'}]})) + self._seed(rd, {'rows': [{'board': 1, 'cells': 'notadict'}, + {'board': 'a', 'cells': {'t': 'pass'}}], + 'banner': '', 'caveat': '', 'scope': ''}) + r = subprocess.run( + [sys.executable, str(Path(HIL_DIR) / 'helper' / 'hil_report.py'), + str(rd / 'cfg.json'), '-b', 'a', '--report-dir', str(rd)], + capture_output=True, text=True, timeout=60) + self.assertEqual(r.returncode, 0, r.stderr) + self.assertTrue(json.loads(r.stdout)['results'][0]['pass']) + + +class NoBoardsGuardOnlyAppliesWhenAccumulating(unittest.TestCase): + def test_a_fresh_run_carries_nothing_from_the_prior_sidecar(self): + """rows were reset on fresh but banner and scope were not, so a leftover or + uploaded sidecar republished a stale rig-health note and a stale scope line under + this run's notice.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report([('old', 0, 0, [('old', {'t': 'OK'}, '1s')], 0)], + rd, True, '3 board(s) — a, b, c', + '> **Rig note.** stale D-state holder\n') + hil_report.mark_report_no_boards(rd, 'filters emptied', fresh=True) + doc = json.loads((rd / hil_report.REPORT_JSON).read_text()) + self.assertEqual(doc['rows'], []) + self.assertEqual(doc['banner'], '', 'a stale rig-health banner was republished') + self.assertEqual(doc['scope'], '', 'a stale scope note was republished') + self.assertNotIn('Rig note', (rd / hil_report.REPORT_MD).read_text()) + + def test_an_accumulate_run_keeps_banner_and_scope(self): + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report([('old', 0, 0, [('old', {'t': 'OK'}, '1s')], 0)], + rd, True, '3 board(s) — a, b, c', + '> **Rig note.** real\n') + hil_report.mark_report_no_boards(rd, 'filters emptied', fresh=False) + doc = json.loads((rd / hil_report.REPORT_JSON).read_text()) + self.assertIn('Rig note', doc['banner']) + self.assertEqual([r['board'] for r in doc['rows']], ['old']) + + def test_a_fresh_run_is_not_blocked_by_a_prior_abandon(self): + """The guard runs BEFORE the fresh wipe, so guarding a fresh run left the previous + attempt's rows AND its abandon notice published as this run's.""" + td = TemporaryDirectory() + self.addCleanup(td.cleanup) + rd = Path(td.name) + hil_report.accumulate_report([('old', 0, 0, [('old', {'t': 'OK'}, '1s')], 0)], + rd, True, '', '') + hil_report.mark_report_abandoned(rd, 'the worker pool would not shut down.') + hil_report.mark_report_no_boards(rd, 'filters emptied', fresh=True) + doc = json.loads((rd / hil_report.REPORT_JSON).read_text()) + self.assertEqual(doc['rows'], []) + self.assertIn('selected no boards', doc['caveat']) + + +if __name__ == '__main__': + unittest.main() |