diff options
| -rw-r--r-- | .claude/agents/hil-operator.md | 9 | ||||
| -rw-r--r-- | .claude/agents/pr-review-validator.md | 20 | ||||
| -rw-r--r-- | .claude/skills/etm-trace/SKILL.md | 6 | ||||
| -rw-r--r-- | .claude/skills/hil-pool-check/SKILL.md | 4 | ||||
| -rw-r--r-- | .claude/skills/hil/SKILL.md | 41 | ||||
| -rw-r--r-- | .claude/skills/rtt/SKILL.md | 12 | ||||
| -rw-r--r-- | .claude/skills/usbtest/SKILL.md | 10 | ||||
| -rw-r--r-- | CLAUDE.md | 2 | ||||
| -rw-r--r-- | docs/superpowers/followup/pr3840-skill-md-no-boards-drift.md | 38 |
9 files changed, 65 insertions, 77 deletions
diff --git a/.claude/agents/hil-operator.md b/.claude/agents/hil-operator.md index d128f6f53..4fd483a73 100644 --- a/.claude/agents/hil-operator.md +++ b/.claude/agents/hil-operator.md @@ -47,9 +47,10 @@ The GitHub Actions runner keeps running during your work. Per-board flock locks schedule them — it round-robins boards across host controllers and budgets simultaneous flashes and usbtest batteries per controller. Those budgets live in one process, so a second `hil_test.py` alongside the first does not share them and the rig sees double the configured - width. (Do not read that as the cause of a dead card: hil_lock.py:128-131 records that every - observed uPD720201 death traced to a marginal DUT port bouncing under concurrent batteries, - and that lowering the widths does not fix a bad port — fix the port or pull the board.) + width. (A dead uPD720201 card is not that: every observed death traced to a marginal DUT + port bouncing under concurrent batteries, and lowering the widths does not fix a bad port — + fix the port or pull the board; the concurrency note above `FLASH_PARALLEL` in `hil_lock.py` + keeps the record.) - On test failure, retry ONCE, with `-v` for diagnosis. Retry from the spec the run just wrote — `<config>.failed`, which already begins with `--accumulate` and restricts each board to its failed tests via `-bt`. If you compose the retry by hand you MUST pass `--accumulate` yourself: @@ -76,6 +77,6 @@ python3 test/hil/helper/hil_report.py <config> -b BOARD [-b BOARD...] # from t `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. +rather than a phrase, so any of it re-derived by hand has come out wrong before. `wedged` is yours — the boards your run left unresponsive, usually none — and the only field you author. diff --git a/.claude/agents/pr-review-validator.md b/.claude/agents/pr-review-validator.md index 4069a856f..a731cd774 100644 --- a/.claude/agents/pr-review-validator.md +++ b/.claude/agents/pr-review-validator.md @@ -10,8 +10,22 @@ You validate the bot review findings on exactly one PR (number given in your pro ## Procedure -- Inline review comments: `gh api repos/{owner}/{repo}/pulls/<N>/comments --paginate` (use `gh repo view --json nameWithOwner -q .nameWithOwner` for owner/repo). Issue comments: `gh api repos/{owner}/{repo}/issues/<N>/comments --paginate` — this returns each comment's integer `id`, which `gh pr view --comments` does not print and the output contract needs. PR reviews (the Copilot/Claude verdict bodies): `gh api repos/{owner}/{repo}/pulls/<N>/reviews --paginate` — compare each review's `commit_id` to the head SHA from `gh pr view <N> --json headRefOid -q .headRefOid` to tell a review of the current push from an older one. -- Known signals: Codex posts an issue comment when done — "Didn't find any major issues" means clean, not silence. It can also signal a clean pass with no comment at all: a 👍 (`+1`) reaction on the PR description (`gh api "repos/{owner}/{repo}/issues/<N>/reactions?content=%2B1&per_page=100" --paginate`, author matching `codex`; without `--paginate` a fresh reaction can fall off the first page and Codex looks pending forever) — settled when the reaction's `created_at` postdates the head push time defined below. Its body carries a `**Reviewed commit:** <short sha>` line: Codex is settled only when that short SHA prefix-matches the head SHA, otherwise the comment is a verdict for an older push and Codex is still pending. Its "Something went wrong" comment has no Reviewed-commit line, so correlate that one by time instead — against the moment the SHA *became* the head, `gh api repos/{owner}/{repo}/commits/<headSha>/check-suites --jq '[.check_suites[].created_at] | min'` (the suites are created when the push lands; fall back to `gh api repos/{owner}/{repo}/commits/<headSha> --jq .commit.committer.date` only if the SHA has no check suites). The committer date alone is when the commit was written, which can precede the push by hours and make a leftover error comment look fresh. An error/quota comment settles Codex only when its `created_at` postdates that push time, or when it arrives as a PR review whose `commit_id` is the head SHA. An older one is a leftover from an earlier push — Codex is still pending. Copilot submits a PR review whose body opens with a verdict header (`### 🟢 Approval recommended` / `### 🟡 Changes recommended`) and leaves `requested_reviewers` once submitted. The Claude bot posts a PR review, or its `claude-review` check run for the head SHA reaches `status: completed` — ask for that check by name, `gh api "repos/{owner}/{repo}/commits/<headSha>/check-runs?check_name=claude-review"`, since the unfiltered listing is paginated and drops it on a PR with more than a page of checks. A bot reporting a usage/quota limit counts as settled once that report postdates the head push time above (the check-suite timestamp, not the committer date) — do not wait on it. Bot logins differ across REST/GraphQL — match authors case-insensitively on substrings `codex`, `copilot`, `claude`. +- Fetch with the REST API (owner/repo via `gh repo view --json nameWithOwner -q .nameWithOwner`; head SHA via `gh pr view <N> --json headRefOid -q .headRefOid`): + - inline review comments: `gh api repos/{owner}/{repo}/pulls/<N>/comments --paginate` + - issue comments: `gh api repos/{owner}/{repo}/issues/<N>/comments --paginate` — this returns each comment's integer `id`, which `gh pr view --comments` does not print and the output contract needs + - PR reviews (the Copilot/Claude verdict bodies): `gh api repos/{owner}/{repo}/pulls/<N>/reviews --paginate` +- A reviewer is **settled** for the current head SHA when its verdict artifact is bound to that SHA (a review's `commit_id`, a check run for the SHA, Codex's `**Reviewed commit:** <short sha>` line prefix-matching it) or, for artifacts that carry no SHA (reactions, error and quota comments), when its `created_at` postdates the head push time. Anything bound to or dated before an older push is a leftover from that push: the reviewer is still pending. A bot reporting a usage/quota limit settles the same way — do not wait on it. +- Head push time = when the SHA *became* the head: `gh api repos/{owner}/{repo}/commits/<headSha>/check-suites --jq '[.check_suites[].created_at] | min'` (suites are created when the push lands). Fall back to `gh api repos/{owner}/{repo}/commits/<headSha> --jq .commit.committer.date` only if the SHA has no check suites: the committer date is when the commit was written, which can precede the push by hours and make a leftover error comment look fresh. +- Where each bot's verdict lives (match authors case-insensitively on the substrings `codex`, `copilot`, `claude` — logins differ across REST/GraphQL): + +| Bot | Verdict artifact | Bound to head by | +| ------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------ | +| Codex | issue comment when done ("Didn't find any major issues" = clean, not silence); body carries `**Reviewed commit:** <short sha>` | short SHA prefix-matches head | +| Codex | 👍 (`+1`) reaction on the PR description = a clean pass with no comment, author matching `codex`: `gh api "repos/{owner}/{repo}/issues/<N>/reactions?content=%2B1&per_page=100" --paginate` (without `--paginate` a fresh reaction can fall off the first page and Codex looks pending forever) | reaction `created_at` postdates head push time | +| Codex | "Something went wrong" comment (no Reviewed-commit line), or a PR review | comment `created_at` postdates head push time; review `commit_id` = head | +| Copilot | PR review whose body opens with `### 🟢 Approval recommended` / `### 🟡 Changes recommended`; it leaves `requested_reviewers` once submitted | review `commit_id` = head | +| Claude | PR review, or the `claude-review` check run reaching `status: completed` — ask for it by name, `gh api "repos/{owner}/{repo}/commits/<headSha>/check-runs?check_name=claude-review"` (the unfiltered listing is paginated and drops it on a PR with more than a page of checks) | review `commit_id` = head; the check run is per SHA | + - For EACH unresolved bot finding: open the file at the cited line in the current checkout and judge the claim adversarially. `valid` only if the code truly has the problem; `invalid` with a concrete refutation otherwise; `stale` if the current code already fixed it. - Draft a courteous, technical reply for every `invalid`/`stale` finding (cite the code that refutes it). Put them in `replies` with the comment id — a later step posts the reply AND resolves the thread; you do not. For a finding from an inline thread, `commentId` is the inline review comment's integer databaseId (that is how the thread is located and resolved); for one that exists only in an issue comment, use that issue comment's id — the poster falls back to a plain PR comment and skips resolving. @@ -25,6 +39,6 @@ Your final message is parsed by a program. Return ONLY this JSON — no prose, n done = true only when no unresolved `valid` findings remain AND every auto-reviewer has settled for the current head SHA: its verdict is posted (Copilot review header, -Codex verdict comment, Claude review or concluded check) or it reported hitting a +Codex verdict comment or 👍 reaction, Claude review or concluded check) or it reported hitting a usage/quota limit. A reviewer that has not reported since the last push is pending — return done = false so the caller re-checks next cycle. diff --git a/.claude/skills/etm-trace/SKILL.md b/.claude/skills/etm-trace/SKILL.md index 43cf6a2a5..5d9b51bcf 100644 --- a/.claude/skills/etm-trace/SKILL.md +++ b/.claude/skills/etm-trace/SKILL.md @@ -196,8 +196,8 @@ both. Timing semantics and clock columns are explained at the top of that file. ## References - Ozone manual (UM08025, automation socket §6.7, project commands §7): - <https://www.segger.com/downloads/jlink/UM08025_Ozone.pdf> — V3.50, same as - the installed Ozone (web is rev 1 vs the local copy's rev 0; the local PDF - under /opt/SEGGER/Ozone_V350/Doc remains the offline fallback). + <https://www.segger.com/downloads/jlink/UM08025_Ozone.pdf>; the installed + Ozone ships its own revision of it under its `Doc/` directory (offline + fallback — section numbers can differ between revisions). - J-Link / J-Trace manual (UM08001, trace ch. 10, timing troubleshooting): <https://kb.segger.com/UM08001_J-Link_/_J-Trace_User_Guide> diff --git a/.claude/skills/hil-pool-check/SKILL.md b/.claude/skills/hil-pool-check/SKILL.md index 65e28b65f..dd128f755 100644 --- a/.claude/skills/hil-pool-check/SKILL.md +++ b/.claude/skills/hil-pool-check/SKILL.md @@ -130,7 +130,9 @@ removes VBUS, so there is nothing further to try. The user-facing answer to a pool check IS the tool's summary table: paste the complete per-board table (and footer counts) verbatim — never truncate rows or reduce it to a prose digest like -"27/27 healthy"; at most one line of commentary below it. +"27/27 healthy". Commentary below it covers only what the table cannot show: which mode ran +and why when it was not the full check, the escalation note below, a board that needs a +physical replug. When an escalation above was needed, add a short note under the table naming: which boards needed it, which root port was cycled (or that a replug was needed instead), which siblings bounced, and diff --git a/.claude/skills/hil/SKILL.md b/.claude/skills/hil/SKILL.md index 03a462ce6..0257c42c1 100644 --- a/.claude/skills/hil/SKILL.md +++ b/.claude/skills/hil/SKILL.md @@ -60,14 +60,15 @@ fi Read `full`, never `args` alone: `args` is empty for BOTH `full: true` (run the whole matrix — a broad or unclassified change) and "nothing selected" (skip). Skip only when `full` is false AND `args` is empty. -Unit suites (no hardware), all five run by the `hil-test`/`ci-select-test` pre-commit -hooks: `test_ci_select.py` covers only selection, `test_ci_metrics.py` only the code-size -plumbing. The containment work --- bounded reads, the kill ladders, the build and pool -guards --- lives in `test_hil_bounded.py`, `test_hil_health.py` and `test_hil_util.py`, so -run all five when changing `test/hil`: -`for f in test/hil/test/test_*.py; do python3 "$f"; done` (~48s, of which -`test_hil_bounded.py` is ~30s of deliberate hang/timeout simulation; the two `test_ci_*` -suites are ~4s together). +Unit suites (no hardware) live in `test/hil/test/test_*.py`; the `hil-test` pre-commit hook +runs every `test_hil*.py`, `ci-select-test` the two `test_ci_*` suites plus +`test_hil_util.BottomLayer`. `test_ci_select.py` covers only selection, `test_ci_metrics.py` +only the code-size plumbing; the containment work --- bounded reads, the kill ladders, the +build and pool guards --- lives in `test_hil_bounded.py`, `test_hil_health.py` and +`test_hil_util.py`; `test_hil_report.py` covers the report document and `test_hil_rtt.py` +the RTT console. Run them all when changing `test/hil`: +`for f in test/hil/test/test_*.py; do python3 "$f"; done` (about a minute, half of it +`test_hil_bounded.py`'s deliberate hang/timeout simulation). ## Pre-flight rig health check @@ -134,23 +135,29 @@ the run before its own guard can write a report. NEVER cancel early. ## Reporting The user-facing answer to a HIL run IS the tool's summary table: paste the complete per-board -table (and footer counts) verbatim — never truncate rows or reduce it to a prose digest; at most -one line of commentary below it. +table (and footer counts) verbatim — never truncate rows or reduce it to a prose digest. +Commentary below it covers only what the table cannot show: a banner verdict from the list +below, a retry, a wedged board. **First check what sits above the table.** Six banners can appear there; match on a PREFIX, since each carries trailing detail and two are blockquotes: -- `**HIL run abandoned: worker pool timed out after …s.**` — no results were collected this - attempt, so any table below is a PREVIOUS attempt's. Report the abandonment, never those - rows, and never `"pass": true`. -- `**HIL run aborted: a worker raised …**` — same rule: a worker crashed before results - were collected; any table below is stale. Report the abort, never the rows. +- `**HIL run abandoned: worker pool timed out after …s.**` and + `**HIL run aborted: a worker raised …**` — the pool guard fired, or a worker crashed. The + banner counts what happened: "N board(s) below finished and are this run's; K never + reported and are NOT in the table: <names>". The N finished boards' rows are this run's: + report them. The K named boards are not this run's whatever the table shows — on a fresh + run they have no row, on an `--accumulate` retry a previous attempt's row survives under + the banner and `hil_report.py` still folds it into `results` as ran — so name them as not + run; the `<config>.failed` re-run spec covers them. Never `"pass": true`. - `**HIL run abandoned: the worker pool would not shut down.**` — DIFFERENT: the table below IS this run's, but the pool could not be shut down afterwards (the job exits non-zero even if every board passed). Report the results AND the abandonment; never `"pass": true`. -- `**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`. +- `**HIL run selected no boards.**` — the filters intersected to nothing. A fresh run shows + no table; an `--accumulate` run keeps the previous attempt's rows under the notice, and + they are not this run's. Report the empty selection (and the filter shown), never + `"pass": true`. - `> **Rig note.**` — a process was in D state when the run started. This is NOT a wedge: a healthy in-flight testusb is uninterruptible for most of every case, and the rig supports a dev run alongside CI. On its own it is never `wedged: true` and never turns a diff --git a/.claude/skills/rtt/SKILL.md b/.claude/skills/rtt/SKILL.md index 5e14ae84c..58d7312a3 100644 --- a/.claude/skills/rtt/SKILL.md +++ b/.claude/skills/rtt/SKILL.md @@ -169,11 +169,13 @@ then `savebin <file> <pBuffer> <SizeOfBuffer>`.) RTT console output is NOT lossless under load; for high-bandwidth streams size the buffer up (SystemView needs 2048–8192) and watch for overflow. - Non-ARM ports must supply `SEGGER_RTT_LOCK/UNLOCK`: the vendored generic - RISC-V lock uses `mstatus` CSRs that trap (mcause=2) on WCH QingKe. Worked - port on branch `claude/add-systemview-debug`: `hw/bsp/ch583/ - sysview_rtt_lock_wch.h` (brace-scoped save/restore of CSR 0x800), and the - shared `hw/bsp/sysview_rtt_conf_wch.h` that ch32v20x/ch32v30x family.cmake - force-include to win the include-guard race against the vendored conf. + RISC-V lock uses `mstatus` CSRs that trap (mcause=2) on WCH QingKe. A + working WCH port exists but is unmerged (maintainer branch + `claude/add-systemview-debug`, not on master): + `hw/bsp/ch583/sysview_rtt_lock_wch.h` (brace-scoped save/restore of CSR + 0x800), and the shared `hw/bsp/sysview_rtt_conf_wch.h` that + ch32v20x/ch32v30x family.cmake force-include to win the include-guard race + against the vendored conf. ## Common mistakes diff --git a/.claude/skills/usbtest/SKILL.md b/.claude/skills/usbtest/SKILL.md index 606b379b5..c851f85c4 100644 --- a/.claude/skills/usbtest/SKILL.md +++ b/.claude/skills/usbtest/SKILL.md @@ -41,11 +41,11 @@ python3 test/hil/usbtest.py --serial <uid> --keep-binding --tests 29 # one case actions runner. It keeps running; the per-board flock is what arbitrates (see the `hil` skill). Never start a battery by hand next to a running one: `hil_test.py` budgets 2 concurrent batteries per host controller (`HIL_USBTEST_PARALLEL`). The width itself is a profiled throughput/bandwidth - trade, not a safety ceiling (hil_lock.py:122-127) — but a battery outside the budget is a real - hazard, and the hazard is recorded: unbudgeted concurrent batteries have hard-frozen the rig with - a fatal PCIe error on a VFIO-passed xHCI, and a marginal DUT port bouncing under concurrent - batteries has killed a uPD720201 outright, which lowering the widths does not fix - (hil_lock.py:130-132). + trade, not a safety ceiling (the concurrency note above `FLASH_PARALLEL` in `hil_lock.py`) — but + a battery outside the budget is a real hazard: unbudgeted concurrent batteries have hard-frozen + the rig with a fatal PCIe error on a VFIO-passed xHCI, and a marginal DUT port bouncing under + concurrent batteries has killed a uPD720201 outright, which lowering the widths does not fix + (that note records every such death). ## Porting ladder — new MCU/DCD to 30/30 @@ -8,7 +8,7 @@ Reference these instructions first; fall back to search/bash only when reality d Bias toward caution over speed. For trivial tasks, use judgment. -- **Think first** — state assumptions; ask if unclear; present alternatives instead of picking silently. +- **Think first** — state assumptions; ask if unclear; when a choice matters, name it and recommend one rather than picking silently or surveying every option. - **Simplicity** — no features, abstractions, flexibility, or error handling beyond what was asked. If 200 lines could be 50, rewrite. - **Surgical changes** — touch only what the task requires; match existing style; don't refactor working code; mention unrelated dead code rather than deleting it. Remove only orphans *your* changes created. - **Goal-driven** — turn tasks into verifiable goals ("write failing test, make it pass"). For multi-step work, state a brief `step → verify` plan. diff --git a/docs/superpowers/followup/pr3840-skill-md-no-boards-drift.md b/docs/superpowers/followup/pr3840-skill-md-no-boards-drift.md deleted file mode 100644 index a039a8c12..000000000 --- a/docs/superpowers/followup/pr3840-skill-md-no-boards-drift.md +++ /dev/null @@ -1,38 +0,0 @@ -# `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. |