Fix review findings and add static-analyzer agent

Review-fix batch (owner-confirmed) on the multi-agent harness:

- board_lock: detach holder stdio so a captured `hold` cannot hang on the
  daemon's inherited pipe; probe locks by holder-pid liveness instead of a
  momentary flock, which could spuriously fail a concurrent acquirer
  (storm-tested: 1 winner in 10, 0/15 acquire failures under probe storm)
- hil_test: locked board renders a visible board-locked fail row so the
  report matches the exit code; stale marker cleared on a real re-run
- pr-babysit: autoPush now opt-in (default dry run); resolve recipe
  paginates reviewThreads; post-push resolve gets issue-comment fallback
- validate: size stage honors non-default base via --base-branch; pvs
  stage delegated to the new agent
- new static-analyzer agent (sonnet): PVS-Studio SAST+MISRA for one
  board, structured findings gated on files changed vs base

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rn1AN5DsTdFhRwhugfgKZi
This commit is contained in:
hathach
2026-07-10 23:28:09 +07:00
parent e3dd9245ef
commit 0557655afb
7 changed files with 124 additions and 35 deletions

View File

@ -0,0 +1,43 @@
---
name: static-analyzer
description: Run PVS-Studio static analysis (SAST + MISRA C:2023/C++:2008) on TinyUSB for one board and report structured findings, gated on diagnostics in files changed vs a base ref. Read-only; never edits source.
tools: Bash, Read, Grep, Glob
model: sonnet
---
You run PVS-Studio over the TinyUSB examples build for exactly one board per run and report machine-readable findings. You never modify source files.
## Procedure
1. **Build with an exported compile DB.** Running solo, the wrapper does build + analyze + report in one step:
```bash
.claude/skills/pvs/run_pvs.sh <BOARD> # uses examples/cmake-build-<BOARD>
```
When the prompt says parallel build agents are running (or asks for a dedicated build dir), do NOT share `cmake-build-<BOARD>` — build your own and analyze manually:
```bash
cd examples && cmake -B cmake-build-pvs -DBOARD=<BOARD> -G Ninja -DCMAKE_BUILD_TYPE=MinSizeRel . && cmake --build cmake-build-pvs
cd .. && pvs-studio-analyzer analyze -f examples/cmake-build-pvs/compile_commands.json \
-R .PVS-Studio/.pvsconfig -o pvs-report.log -j"$(nproc)" \
--security-related-issues --misra-c-version 2023 --misra-cpp-version 2008 --use-old-parser
plog-converter -a GA:1,2 -t errorfile pvs-report.log
```
2. **Gate on changed files.** The prompt names a base ref (default `master`). Compute `git diff --name-only <base>...HEAD` plus uncommitted changes (`git diff --name-only <base>`), then match diagnostics against that set. `pass=false` only when GA:1 diagnostics exist in changed files — or when the tool itself failed (build, license, analyzer error); say which in `detail`.
## Recovery rules
- License missing (`pvs-studio-analyzer lic-info` fails): register from `$PVS_STUDIO_CREDENTIALS` (`read -r n k <<< "$PVS_STUDIO_CREDENTIALS"; pvs-studio-analyzer credentials "$n" "$k"`); if unset, report the failure — do not hunt for keys.
- Missing dependency errors (`lib/...` or `hw/mcu/...` not found): run `python3 tools/get_deps.py <FAMILY>` once, then retry.
- `.pvsconfig` already excludes vendored code and accepted MISRA deviations — never add suppressions yourself; surviving findings are real.
- Build + analysis take minutes — use generous Bash timeouts (>= 10 min).
## Output contract
Your final message is parsed by a program. Return ONLY this JSON — no prose, no code fences:
{"pass": true, "ga1": 3, "ga2": 17, "changedFindings": [{"file": "src/portable/x/dcd_x.c", "line": 123, "rule": "V547", "level": 1, "message": "..."}], "detail": "GA:1=3 GA:2=17 total; 0 diagnostics in files changed vs master"}
`ga1`/`ga2` = total GA level 1/2 diagnostic counts. `changedFindings` = every GA:1 and GA:2 diagnostic located in a changed file (`level` = 1 or 2). `pass` = no GA:1 in changed files and the tool ran clean.

View File

@ -1,11 +1,11 @@
export const meta = {
name: 'pr-babysit',
description: 'Drive a PR to green: pr-monitor triage (CI + bot reviews), port-dev fixes for validated findings, driver-reviewer verification, one commit+push per cycle',
whenToUse: 'After opening a PR, from a checkout of the PR branch. Invoking with autoPush enabled authorizes pushes to that branch.',
whenToUse: 'After opening a PR, from a checkout of the PR branch. Default is a dry run (fixes left uncommitted, nothing posted); passing autoPush: true is the explicit authorization for pushes and PR comments.',
phases: [{ title: 'Triage' }, { title: 'Fix' }, { title: 'Verify' }, { title: 'Push' }],
}
// args: { pr: number, maxCycles?: number, autoPush?: boolean }
// args: { pr: number, maxCycles?: number, autoPush?: boolean (default false = dry run) }
if (typeof args === 'string') args = JSON.parse(args) // tolerate stringified invocation args
if (!args || !args.pr) {
throw new Error('args must be { pr: number, maxCycles?, autoPush? }; run from a checkout of the PR branch')
@ -84,8 +84,8 @@ const OP = {
const RESOLVE_RECIPE =
'To resolve the review thread for an inline review comment (its integer databaseId is the commentId): ' +
'get owner/repo via `gh repo view --json nameWithOwner -q .nameWithOwner`; find the thread node id with ' +
'`gh api graphql -f query=\'query($o:String!,$r:String!,$p:Int!){repository(owner:$o,name:$r){pullRequest(number:$p){reviewThreads(first:100){nodes{id isResolved comments(first:50){nodes{databaseId}}}}}}}\' -F o=OWNER -F r=REPO -F p=' + args.pr + '` ' +
'(paginate with the endCursor if there are more than 100 threads), pick the thread whose comments contain that databaseId, then resolve it with ' +
'`gh api graphql -f query=\'query($o:String!,$r:String!,$p:Int!,$c:String){repository(owner:$o,name:$r){pullRequest(number:$p){reviewThreads(first:100,after:$c){pageInfo{hasNextPage endCursor}nodes{id isResolved comments(first:50){nodes{databaseId}}}}}}}\' -F o=OWNER -F r=REPO -F p=' + args.pr + '` ' +
'(while hasNextPage is true and the comment is not found yet, re-run with -F c=<endCursor>), pick the thread whose comments contain that databaseId, then resolve it with ' +
'`gh api graphql -f query=\'mutation($id:ID!){resolveReviewThread(input:{threadId:$id}){thread{isResolved}}}\' -F id=THREAD_ID`. ' +
'Issue comments (the 404 fallback case) have no thread — do not try to resolve those.'
@ -106,7 +106,7 @@ for (let cycle = 1; cycle <= maxCycles; cycle++) {
// Post drafted replies to REFUTED findings as soon as triage produces them —
// decoupled from fixing/pushing so done/unactionable cycles still post.
// Reply AND resolve the thread. Outward-facing, so gated on autoPush.
if (t.replies.length > 0 && args.autoPush !== false) {
if (t.replies.length > 0 && args.autoPush === true) {
const posted = await agent(
`Reply to and resolve these refuted review comments on PR #${args.pr}. For each: post the reply with ` +
`gh api repos/{owner}/{repo}/pulls/${args.pr}/comments/{commentId}/replies -f body=<body> ` +
@ -168,8 +168,8 @@ for (let cycle = 1; cycle <= maxCycles; cycle++) {
if (aliveFixes.length < work.length) log(`${work.length - aliveFixes.length} fix group(s) lost to dead workers`)
entry.fixes = aliveFixes
if (args.autoPush === false) {
log('autoPush=false: fixes left uncommitted in the working tree (dry run)')
if (args.autoPush !== true) {
log('autoPush not set: fixes left uncommitted in the working tree (dry run)')
return { pass: false, cycles: cycle, history, dryRun: true }
}
@ -199,8 +199,9 @@ for (let cycle = 1; cycle <= maxCycles; cycle++) {
const resolved = await agent(
`The fixes for PR #${args.pr}'s valid review findings were just committed and pushed (${push.detail}). ` +
'For each finding below: post a threaded reply to its inline comment via ' +
`gh api repos/{owner}/{repo}/pulls/${args.pr}/comments/{commentId}/replies -f body=<body>, stating it is fixed in the pushed commit and one line on the change, ` +
`then mark its thread resolved. ${RESOLVE_RECIPE} ` +
`gh api repos/{owner}/{repo}/pulls/${args.pr}/comments/{commentId}/replies -f body=<body>, stating it is fixed in the pushed commit and one line on the change; ` +
`if that 404s, the id is an issue comment — post a regular PR comment instead (gh pr comment ${args.pr} --body <quote the original point, then the fix note>) and skip resolving. ` +
`After replying to an inline comment, mark its thread resolved. ${RESOLVE_RECIPE} ` +
`Findings: ${JSON.stringify(fixed.map(f => ({ commentId: f.commentId, file: f.file, line: f.line, claim: f.claim, fixHint: f.fixHint })))}. ` +
'pass=true only if every reply was posted and every thread resolved; detail = what went where.',
{ label: `resolve#${cycle}`, phase: 'Push', model: 'sonnet', schema: OP },

View File

@ -36,6 +36,25 @@ const BUILD = {
},
},
}
const PVS = {
type: 'object', additionalProperties: false,
required: ['pass', 'ga1', 'ga2', 'changedFindings', 'detail'],
properties: {
pass: { type: 'boolean' }, ga1: { type: 'integer' }, ga2: { type: 'integer' },
changedFindings: {
type: 'array',
items: {
type: 'object', additionalProperties: false,
required: ['file', 'line', 'rule', 'level', 'message'],
properties: {
file: { type: 'string' }, line: { type: 'integer' }, rule: { type: 'string' },
level: { type: 'integer' }, message: { type: 'string' },
},
},
},
detail: { type: 'string' },
},
}
const thunks = []
@ -57,7 +76,7 @@ for (const b of args.boards) thunks.push(() =>
if (!skip.includes('size')) thunks.push(() =>
agent(
`Compare TinyUSB code size against ${base}: python3 tools/metrics_compare_base.py -b ${args.boards[0]} -e device/cdc_msc . ` +
`Compare TinyUSB code size against ${base}: python3 tools/metrics_compare_base.py --base-branch ${base} -b ${args.boards[0]} -e device/cdc_msc . ` +
'The report lands in cmake-metrics/<board>/metrics_compare.md. pass=false only if the tool itself errors; ' +
'detail = the flash/RAM delta summary from the report (mention any example that grew).',
{ label: 'size', phase: 'Validate', model: 'haiku', schema: STAGE },
@ -65,15 +84,13 @@ if (!skip.includes('size')) thunks.push(() =>
if (!skip.includes('pvs')) thunks.push(() =>
agent(
'Run PVS-Studio static analysis per .claude/skills/pvs/SKILL.md, but with a DEDICATED build dir so you do not collide with parallel build agents: ' +
`cd examples && cmake -B cmake-build-pvs -DBOARD=${args.boards[0]} -G Ninja -DCMAKE_BUILD_TYPE=MinSizeRel . && cmake --build cmake-build-pvs. ` +
'Then: pvs-studio-analyzer analyze -f examples/cmake-build-pvs/compile_commands.json -R .PVS-Studio/.pvsconfig -o pvs-report.log -j12 ' +
'--security-related-issues --misra-c-version 2023 --misra-cpp-version 2008 --use-old-parser ' +
'and view with: plog-converter -a GA:1,2 -t errorfile pvs-report.log. ' +
`pass=false only if GA:1 diagnostics exist in files changed vs ${base} (git diff --name-only ${base}...HEAD). ` +
'detail = GA:1/GA:2 counts plus any diagnostics in changed files.',
{ label: 'pvs', phase: 'Validate', model: 'sonnet', effort: 'low', schema: STAGE },
).then(r => r && { stage: 'pvs', ...r }))
`Run PVS-Studio static analysis for board ${args.boards[0]}, gating on files changed vs ${base}. ` +
'Parallel build agents are running — use your dedicated build dir, never cmake-build-<board>.',
{ label: 'pvs', phase: 'Validate', agentType: 'static-analyzer', effort: 'low', schema: PVS },
).then(r => r && {
stage: 'pvs', pass: r.pass,
detail: r.pass ? r.detail : clip(`${r.detail} ${JSON.stringify(r.changedFindings)}`),
}))
const results = (await parallel(thunks)).filter(Boolean)
const dead = thunks.length - results.length

View File

@ -57,3 +57,11 @@ pr-babysit {pr: 3761, maxCycles: 1, autoPush: false} (wf_361c9e0a-d0e) + direct
- Remaining: Task 18 verdicts, final whole-branch review, memory note update for the lock protocol.
Stop-gate extra (done this session): board_lock `cmd_hold` holder-signaled success via pipe (c326eaacc), storm-tested 10/10 exactly-one-winner.
## Post-review fixes (2026-07-10, owner-confirmed batch)
- board_lock: holder daemon detaches stdio (captured `hold` returned in 30 ms; pre-fix hung on the inherited pipe); `is_locked()` re-done as pid-liveness probe — never touches the flock, so status/pre-check storms can no longer fail a concurrent acquirer (0/15 failures under a 300-probe storm; hold storm still 1-winner-in-10).
- hil_test: locked board now emits a visible `board-locked` ❌ report row (report matches exit code); `accumulate_report` clears the stale marker once the board runs for real (3-scenario logic test green). failed-tests stays empty so re-runs repeat the whole board.
- pr-babysit: `autoPush` now opt-in (default dry run; `autoPush: true` is the explicit push/comment authorization); RESOLVE_RECIPE paginates reviewThreads (`pageInfo` + cursor); post-push resolve step gained the same issue-comment 404 fallback as the refuted-replies step.
- validate: size stage passes `--base-branch <base>`; pvs stage now calls the new `static-analyzer` agent (sonnet, structured `{pass, ga1, ga2, changedFindings[], detail}`).
- NEW agent `static-analyzer` (PVS-Studio SAST+MISRA, read-only) — mirrored to the launch-dir registry; registers next session (harness fact 1), so the validate pvs stage is unsmokable until then.

View File

@ -30,8 +30,9 @@ Layered: **agents** (who does the work, with baked-in domain knowledge) ×
### Worker agents — `.claude/agents/*.md`
Tiered models (owner revision 2026-07-09; originally all-opus): `port-dev`
and `driver-reviewer` on **opus** at **xhigh**; `hil-operator` and
`pr-monitor` on **sonnet**; `builder` on **haiku** (mechanical, log-heavy).
and `driver-reviewer` on **opus** at **xhigh**; `hil-operator`, `pr-monitor`
and `static-analyzer` on **sonnet**; `builder` on **haiku** (mechanical,
log-heavy).
| Agent | Effort | Role |
|---|---|---|
@ -40,17 +41,18 @@ and `driver-reviewer` on **opus** at **xhigh**; `hil-operator` and
| `driver-reviewer` | xhigh | Review one dcd/hcd directory against dimensions: correctness, ISR safety, register use vs. datasheet AND MCU errata (calibre library; missing erratum workarounds are findings), style. Returns structured findings `{file, line, snippet, why, severity, confidence}` — coverage-first (report everything; filtering happens downstream). |
| `hil-operator` | default | All rig interaction — the actions-runner service is NEVER stopped; per-board flock locks arbitrate with concurrent CI. `hil_test.py` runs rely on its per-board self-locking; manual hardware work (JLink/GDB, usbtest, serial) is wrapped in `test/hil/board_lock.py hold/release`; rig-wide ops (uhubctl, pci-rebind) require `hold --all`; on wedge `usb_recover.sh` + dmesg. Used strictly serially — never two instances concurrently. |
| `pr-monitor` | default | Triage one GitHub PR via `gh`: check CI status (`gh pr checks`), read failing run logs and classify each failure infra/flake vs real; re-run infra failures (`gh run rerun --failed`); harvest automated review comments (Codex/Copilot/Claude bots — knows their signals: Codex posts a "Didn't find any major issues" issue comment when clean; Copilot drops out of `requested_reviewers` when done; bot logins differ across APIs); adversarially validate each finding against the actual code. Returns structured triage `{ci: {status, infraRerun[], realFailures[]}, findings: [{source, file, line, claim, verdict, fixHint}]}`. Read/triage/re-run/reply only — never edits code. |
| `static-analyzer` | low | Run PVS-Studio (SAST + MISRA C:2023/C++:2008) for one board: build with exported `compile_commands.json` (via `run_pvs.sh` solo, or a dedicated `cmake-build-pvs` dir when parallel builders run), analyze against `.PVS-Studio/.pvsconfig`, gate on diagnostics in files changed vs a base ref. Returns `{pass, ga1, ga2, changedFindings[], detail}`; `pass=false` only on GA:1 in changed files or tool failure. Read-only. |
### Workflows — `.claude/workflows/*.js`
| Workflow | Args | Shape |
|---|---|---|
| `validate.js` | `{boards[], examples?, base?, skip?: ('unit'\|'size'\|'pvs')[]}` | One parallel stage: unit tests (ceedling) + one `builder` per board + code-size compare (`tools/metrics_compare_base.py` vs `base`, default master) + PVS analyze. Join → plain-JS verdict `{pass, failures[]}`. Barrier is correct here: the verdict needs all results. |
| `validate.js` | `{boards[], examples?, base?, skip?: ('unit'\|'size'\|'pvs')[]}` | One parallel stage: unit tests (ceedling) + one `builder` per board + code-size compare (`tools/metrics_compare_base.py` vs `base`, default master) + PVS analyze (`static-analyzer` agent). Join → plain-JS verdict `{pass, failures[]}`. Barrier is correct here: the verdict needs all results. |
| `fanout-dev.js` | `{task, items[], board?, review?, worktree?}` | `pipeline(items)`: `port-dev` per item → `builder` verify → optional `driver-reviewer` pass. Workers share the tree by default (ports are disjoint directories); `worktree: true` switches on per-agent worktree isolation for collision-prone tasks. Returns per-item results. |
| `driver-review.js` | `{dirs[], dimensions?, question?}` | Supersedes `port-audit.js`. Scan stage per (dir × dimension) → adversarial verify per finding (verifier prompted to refute) → confirmed findings only. |
| `hil-validate.js` | `{boards[], force?}` | Strictly serial `for` loop of `hil-operator` calls; each board is protected by `hil_test.py`'s own per-board flock, so the actions-runner keeps running throughout. Boards found locked (a concurrent CI job mid-test) are retried once at the end of the loop; boards still locked are returned in `locked[]` for a user force/wait/accept decision. `force: true` (user-authorized only) bypasses locks via `HIL_NO_BOARD_LOCK=1`. Returns per-board `{board, pass, detail}` plus `wedged[]` and `locked[]`. |
| `full-check.js` | `{boards[], ...}` | Thin composer: `workflow('validate', ...)` → only if green → `workflow('hil-validate', ...)`. Single nesting level (children do not nest further). |
| `pr-babysit.js` | `{pr, maxCycles?, autoPush?}` | Cycle until CI green + review threads resolved, or `maxCycles` (default 3): `pr-monitor` triage (blocks on `gh pr checks --watch` while CI runs) → valid findings + real CI failures grouped by file/port → `port-dev` fix per group (pipeline) → `driver-reviewer` verifies each fix addresses its finding → one commit + push per cycle. Every actioned inline comment is both **replied to and marked resolved** (GraphQL `resolveReviewThread`): refuted findings get the refutation, fixed findings get a "fixed in <sha>" note. `autoPush` defaults true; **invoking this workflow is the explicit push authorization** for follow-up commits on that PR branch (scoped exception to the hold-pushes-until-told rule). |
| `pr-babysit.js` | `{pr, maxCycles?, autoPush?}` | Cycle until CI green + review threads resolved, or `maxCycles` (default 3): `pr-monitor` triage (blocks on `gh pr checks --watch` while CI runs) → valid findings + real CI failures grouped by file/port → `port-dev` fix per group (pipeline) → `driver-reviewer` verifies each fix addresses its finding → one commit + push per cycle. Every actioned inline comment is both **replied to and marked resolved** (GraphQL `resolveReviewThread`): refuted findings get the refutation, fixed findings get a "fixed in <sha>" note. `autoPush` defaults **false** (dry run: fixes stay uncommitted, nothing posted); **passing `autoPush: true` is the explicit push authorization** for follow-up commits and PR comments on that branch (scoped exception to the hold-pushes-until-told rule). |
### Board lock protocol — `test/hil/` (repo code)

View File

@ -49,17 +49,22 @@ def read_info(board: str):
def is_locked(board: str) -> bool:
"""True if some live process currently holds the flock."""
path = lock_path(board)
if not os.path.exists(path):
"""True if the recorded holder process is still alive.
Deliberately never touches the flock: even a momentary probe lock would
make a concurrent acquirer's LOCK_NB attempt fail spuriously. The flock
taken by acquirers themselves stays the only authority."""
info = read_info(board)
pid = info.get('pid') if isinstance(info, dict) else None
if not isinstance(pid, int) or pid <= 0:
return False
with open(path) as f:
try:
fcntl.flock(f, fcntl.LOCK_EX | fcntl.LOCK_NB)
fcntl.flock(f, fcntl.LOCK_UN)
return False
except OSError:
return True
try:
os.kill(pid, 0)
except ProcessLookupError:
return False
except PermissionError:
return True # alive but owned by another user (e.g. the CI runner)
return True
def cmd_hold(boards, reason):
@ -91,6 +96,13 @@ def cmd_hold(boards, reason):
os._exit(0)
# holder (grandchild): acquire all flocks, signal the parent, sleep until killed
os.close(r_fd)
# Detach stdio: a `hold` whose output is captured must see EOF when the
# front-end exits — the immortal holder must not keep that pipe open.
devnull = os.open(os.devnull, os.O_RDWR)
for std_fd in (0, 1, 2):
os.dup2(devnull, std_fd)
if devnull > 2:
os.close(devnull)
try:
handles = []
for b in boards:

View File

@ -1704,7 +1704,9 @@ def test_board(board: Board) -> tuple[str, int, list[str], list]:
_lock_fh = acquire_board_lock(name)
except RuntimeError as e:
log_line(f'{name:25} {STATUS_FAILED}: {e}')
return name, 1, [], []
# visible report row so the ❌ matches the exit code; failed-tests stays
# empty so a re-run repeats the whole board (no bogus -bt test filter)
return name, 1, [], [(name, {'board-locked': 'fail'})]
try:
# default to all tests
test_list = []
@ -1831,7 +1833,11 @@ def accumulate_report(mret: list, report_dir: Path, fresh: bool) -> str:
pass # corrupt/old sidecar: start fresh
# merge this run: current cells override prior for boards/tests that ran
for _, _, _, rows in mret:
for name, _, _, rows in mret:
if rows and not any('board-locked' in cells for _, cells in rows):
# board ran for real this time: clear a stale lock-failure cell
# (its row is keyed by board name; test rows may be variant names)
acc.get(name, {}).pop('board-locked', None)
for row_label, cells in rows:
acc.setdefault(row_label, {}).update(cells)