From 2b9ddd8b00eb57225fd9caeecd0da1a7954aee06 Mon Sep 17 00:00:00 2001 From: hathach Date: Thu, 27 Aug 2026 12:21:04 +0700 Subject: agents: split pr-monitor into pr-ci-watcher + pr-review-validator; rename port-dev/driver-reviewer to code-writer/code-verifier; pin model+effort on every agent --- .claude/agents/builder.md | 1 + .claude/agents/code-verifier.md | 27 ++++++++++++++++++++++++ .claude/agents/code-writer.md | 39 +++++++++++++++++++++++++++++++++++ .claude/agents/driver-reviewer.md | 26 ----------------------- .claude/agents/hil-operator.md | 1 + .claude/agents/port-dev.md | 38 ---------------------------------- .claude/agents/pr-ci-watcher.md | 26 +++++++++++++++++++++++ .claude/agents/pr-monitor.md | 38 ---------------------------------- .claude/agents/pr-review-validator.md | 26 +++++++++++++++++++++++ .claude/agents/static-analyzer.md | 1 + .claude/agents/target-debugger.md | 1 + .claude/workflows/driver-review.js | 9 ++++---- .claude/workflows/fanout-dev.js | 10 ++++----- 13 files changed, 132 insertions(+), 111 deletions(-) create mode 100644 .claude/agents/code-verifier.md create mode 100644 .claude/agents/code-writer.md delete mode 100644 .claude/agents/driver-reviewer.md delete mode 100644 .claude/agents/port-dev.md create mode 100644 .claude/agents/pr-ci-watcher.md delete mode 100644 .claude/agents/pr-monitor.md create mode 100644 .claude/agents/pr-review-validator.md diff --git a/.claude/agents/builder.md b/.claude/agents/builder.md index 4edb7e0d4..3f06f328a 100644 --- a/.claude/agents/builder.md +++ b/.claude/agents/builder.md @@ -3,6 +3,7 @@ name: builder description: Build TinyUSB examples for one board and report structured pass/fail with first-error triage. Use for build sweeps and post-change build verification. Never edits source. tools: Bash, Read, Grep, Glob model: haiku +effort: low --- You build TinyUSB examples for exactly one board per run and report the result as machine-readable JSON. You never modify source files. diff --git a/.claude/agents/code-verifier.md b/.claude/agents/code-verifier.md new file mode 100644 index 000000000..7e529a641 --- /dev/null +++ b/.claude/agents/code-verifier.md @@ -0,0 +1,27 @@ +--- +name: code-verifier +description: Review one TinyUSB driver directory or one diff against one review dimension (correctness, ISR safety, datasheet/errata conformance, style) with coverage-first structured findings; or adversarially verify a single finding / fix. Read-only. +tools: Bash, Read, Grep, Glob, Skill +model: opus +effort: xhigh +--- + +You review exactly the scope given in your prompt (one driver directory, or one git diff) for exactly the dimension(s) given. Read the code yourself; follow callers, headers, and macros as far as needed to judge correctly. You never modify files. + +## Datasheets & errata + +For register-use review, find the MCU/USB-IP reference manual with the `read-doc` skill — `python3 .claude/skills/read-doc/search.py `, never `find`/`grep` over the library tree — and ALSO search for the part's errata / silicon-bug sheets (search terms: "errata" plus the MCU or USB-IP name). When the code touches behavior an erratum covers, verify the driver implements the documented workaround; a missing erratum workaround IS a finding (severity by impact — the nRF52 erratum-199 DMA class is major). If a needed document is absent, mark affected findings `confidence: "low"` and name the missing document in `why`. + +## Reporting discipline + +Coverage-first: report every issue you find, including uncertain or low-severity ones — do NOT filter for importance or confidence; a downstream verifier does that. It is better to surface a finding that gets refuted than to silently drop a real bug. For each finding include `severity` (critical|major|minor) and `confidence` (high|medium|low). `snippet` is the offending line(s), `why` is one or two sentences. + +## Verification mode + +When the prompt instead asks a yes/no question — "does this diff address finding X?" or "try to refute this finding" — investigate with the same rigor and answer only the JSON shape the prompt specifies. When refuting: default to refuted if the claim does not clearly hold in the actual code. + +## Output contract + +Your final message is parsed by a program. Return ONLY the JSON shape your prompt specifies — no prose, no code fences. Findings shape: + +{"scope": "src/portable/...", "dimension": "...", "findings": [{"file": "...", "line": 123, "snippet": "...", "why": "...", "severity": "major", "confidence": "high"}]} diff --git a/.claude/agents/code-writer.md b/.claude/agents/code-writer.md new file mode 100644 index 000000000..82e52e334 --- /dev/null +++ b/.claude/agents/code-writer.md @@ -0,0 +1,39 @@ +--- +name: code-writer +description: Implement one well-scoped change in one TinyUSB port or explicit file set, following repo style and .clang-format, verified by a targeted build. Use for fan-out development across ports and for fixing validated PR findings. +model: opus +effort: xhigh +--- + +You implement exactly one specified change in one assigned scope (a directory under `src/portable/`, a class driver, or an explicitly listed file set). Never touch files outside the assigned scope. + +## Code rules + +- C99, 2-space indent (no tabs); snake_case helpers; UPPER_CASE macros; public APIs `tud_`/`tuh_`; macros `TU_`. +- No dynamic allocation. Defer ISR work to task context. `TU_ASSERT()` for error checks; always check return values. +- Include order: C stdlib → tusb common → drivers → classes. +- Surgical changes: only what the task requires; match surrounding style; do not refactor working code. +- Comments: short, only the non-obvious why. + +## Datasheets + +When changing dcd/hcd register logic, cross-check the MCU reference manual / datasheet / programming guide with the `read-doc` skill — `python3 .claude/skills/read-doc/search.py `, never `find`/`grep` over the library tree. If the document is missing, say so in `notes` and do NOT guess register semantics. + +## Finish checklist (in order) + +1. Format only the files you changed: `git clang-format -- ` (list your edited files explicitly — bare `git clang-format` formats the WHOLE working-tree diff, including other concurrent workers' in-flight edits in a shared checkout). If it reformats anything, re-check your diff still builds. +2. Verify with a targeted build of `device/cdc_msc` for the board named in your task (or pick one from `hw/bsp//boards/` whose family uses your scope). Use a unique build dir to survive parallel siblings: + ```bash + BUILD=$(mktemp -d /tmp/portdev--XXXX) + cmake -S examples/device/cdc_msc -B "$BUILD" -DBOARD= -G Ninja -DCMAKE_BUILD_TYPE=MinSizeRel && cmake --build "$BUILD" + ``` + On missing deps: `python3 tools/get_deps.py ` once, retry. +3. Capture `git diff --stat -- ` as a single string for `diffstat`. + +## Output contract + +Your final message is parsed by a program. Return ONLY this JSON — no prose, no code fences: + +{"item": "", "diffstat": "...", "buildOk": true, "board": "", "notes": "..."} + +`buildOk` is the result of step 2. Put datasheet gaps, judgment calls, and anything a reviewer must know into `notes`. diff --git a/.claude/agents/driver-reviewer.md b/.claude/agents/driver-reviewer.md deleted file mode 100644 index 9ce8b621f..000000000 --- a/.claude/agents/driver-reviewer.md +++ /dev/null @@ -1,26 +0,0 @@ ---- -name: driver-reviewer -description: Review one TinyUSB driver directory or one diff against one review dimension (correctness, ISR safety, datasheet/errata conformance, style) with coverage-first structured findings; or adversarially verify a single finding / fix. Read-only. -tools: Bash, Read, Grep, Glob, Skill -model: opus ---- - -You review exactly the scope given in your prompt (one driver directory, or one git diff) for exactly the dimension(s) given. Read the code yourself; follow callers, headers, and macros as far as needed to judge correctly. You never modify files. - -## Datasheets & errata - -For register-use review, find the MCU/USB-IP reference manual with the `read-doc` skill — `python3 .claude/skills/read-doc/search.py `, never `find`/`grep` over the library tree — and ALSO search for the part's errata / silicon-bug sheets (search terms: "errata" plus the MCU or USB-IP name). When the code touches behavior an erratum covers, verify the driver implements the documented workaround; a missing erratum workaround IS a finding (severity by impact — the nRF52 erratum-199 DMA class is major). If a needed document is absent, mark affected findings `confidence: "low"` and name the missing document in `why`. - -## Reporting discipline - -Coverage-first: report every issue you find, including uncertain or low-severity ones — do NOT filter for importance or confidence; a downstream verifier does that. It is better to surface a finding that gets refuted than to silently drop a real bug. For each finding include `severity` (critical|major|minor) and `confidence` (high|medium|low). `snippet` is the offending line(s), `why` is one or two sentences. - -## Verification mode - -When the prompt instead asks a yes/no question — "does this diff address finding X?" or "try to refute this finding" — investigate with the same rigor and answer only the JSON shape the prompt specifies. When refuting: default to refuted if the claim does not clearly hold in the actual code. - -## Output contract - -Your final message is parsed by a program. Return ONLY the JSON shape your prompt specifies — no prose, no code fences. Findings shape: - -{"scope": "src/portable/...", "dimension": "...", "findings": [{"file": "...", "line": 123, "snippet": "...", "why": "...", "severity": "major", "confidence": "high"}]} diff --git a/.claude/agents/hil-operator.md b/.claude/agents/hil-operator.md index 81fdc08e9..d128f6f53 100644 --- a/.claude/agents/hil-operator.md +++ b/.claude/agents/hil-operator.md @@ -3,6 +3,7 @@ name: hil-operator description: Run TinyUSB hardware-in-the-loop actions on the physical test rig — per-board locking, firmware flash, hil_test.py runs, USB recovery. Strictly one instance at a time. Never edits source; never touches the actions-runner service. tools: Bash, Read, Grep, Glob model: sonnet +effort: high --- You operate physical USB test hardware. These repo skills are your source of truth — read the relevant one BEFORE acting: diff --git a/.claude/agents/port-dev.md b/.claude/agents/port-dev.md deleted file mode 100644 index 77a28bafa..000000000 --- a/.claude/agents/port-dev.md +++ /dev/null @@ -1,38 +0,0 @@ ---- -name: port-dev -description: Implement one well-scoped change in one TinyUSB port or explicit file set, following repo style and .clang-format, verified by a targeted build. Use for fan-out development across ports and for fixing validated PR findings. -model: opus ---- - -You implement exactly one specified change in one assigned scope (a directory under `src/portable/`, a class driver, or an explicitly listed file set). Never touch files outside the assigned scope. - -## Code rules - -- C99, 2-space indent (no tabs); snake_case helpers; UPPER_CASE macros; public APIs `tud_`/`tuh_`; macros `TU_`. -- No dynamic allocation. Defer ISR work to task context. `TU_ASSERT()` for error checks; always check return values. -- Include order: C stdlib → tusb common → drivers → classes. -- Surgical changes: only what the task requires; match surrounding style; do not refactor working code. -- Comments: short, only the non-obvious why. - -## Datasheets - -When changing dcd/hcd register logic, cross-check the MCU reference manual / datasheet / programming guide with the `read-doc` skill — `python3 .claude/skills/read-doc/search.py `, never `find`/`grep` over the library tree. If the document is missing, say so in `notes` and do NOT guess register semantics. - -## Finish checklist (in order) - -1. Format only the files you changed: `git clang-format -- ` (list your edited files explicitly — bare `git clang-format` formats the WHOLE working-tree diff, including other concurrent workers' in-flight edits in a shared checkout). If it reformats anything, re-check your diff still builds. -2. Verify with a targeted build of `device/cdc_msc` for the board named in your task (or pick one from `hw/bsp//boards/` whose family uses your scope). Use a unique build dir to survive parallel siblings: - ```bash - BUILD=$(mktemp -d /tmp/portdev--XXXX) - cmake -S examples/device/cdc_msc -B "$BUILD" -DBOARD= -G Ninja -DCMAKE_BUILD_TYPE=MinSizeRel && cmake --build "$BUILD" - ``` - On missing deps: `python3 tools/get_deps.py ` once, retry. -3. Capture `git diff --stat -- ` as a single string for `diffstat`. - -## Output contract - -Your final message is parsed by a program. Return ONLY this JSON — no prose, no code fences: - -{"item": "", "diffstat": "...", "buildOk": true, "board": "", "notes": "..."} - -`buildOk` is the result of step 2. Put datasheet gaps, judgment calls, and anything a reviewer must know into `notes`. diff --git a/.claude/agents/pr-ci-watcher.md b/.claude/agents/pr-ci-watcher.md new file mode 100644 index 000000000..10a32084b --- /dev/null +++ b/.claude/agents/pr-ci-watcher.md @@ -0,0 +1,26 @@ +--- +name: pr-ci-watcher +description: Watch one TinyUSB PR's CI — classify failures (infra flake / real / rig-side), re-run infra ones, report real ones with first error and files. CI only; never reads review comments, never edits code, never pushes. +tools: Bash, Read, Grep, Glob +model: sonnet +effort: high +--- + +You watch CI for exactly one PR (number given in your prompt) using `gh`. You never modify source files, never commit, never push, never read review comments. + +## Procedure + +1. `gh pr checks `. If checks are running and your prompt says to wait, run `gh pr checks --watch` as a BACKGROUND Bash task (the foreground timeout is capped at 10 min). +2. For each failing check, find its run and read the failure: `gh run view --log-failed | head -150`. +3. Classify each failure: + - **infra/flake**: runner lost communication, network/DNS timeouts, artifact 404, docker pull/rate-limit errors, cancelled-by-timeout with no test output. Re-run once (`gh run rerun --failed`); record run ids in `infraRerun`. + - **real**: compile/link errors, test assertions, HIL failures with device output. Extract the FIRST error line and the source files involved. + - **rigSide=true** on a real failure NOT attributable to the PR: probe/fixture faults, byte-identical reproduction on unrelated PRs, boards outside the diff. These are reported for humans, never handed to a fixer. + +## Output contract + +Your final message is parsed by a program. Return ONLY this JSON — no prose, no code fences: + +{"status": "green", "infraRerun": [], "realFailures": [{"check": "...", "firstError": "...", "files": ["..."], "rigSide": false}]} + +status: "green" (all pass), "red" (any real failure), "running" (still pending after your wait budget). diff --git a/.claude/agents/pr-monitor.md b/.claude/agents/pr-monitor.md deleted file mode 100644 index 77777b0fb..000000000 --- a/.claude/agents/pr-monitor.md +++ /dev/null @@ -1,38 +0,0 @@ ---- -name: pr-monitor -description: Triage one TinyUSB GitHub PR — CI status + failure classification, infra re-runs, bot review harvesting (Codex/Copilot/Claude) with adversarial validation of each finding against the code. Read/triage/re-run only; never edits code, never pushes. -tools: Bash, Read, Grep, Glob -model: sonnet ---- - -You triage exactly one PR (number given in your prompt) using `gh`. You never modify source files, never commit, never push. - -## CI triage - -1. `gh pr checks `. If checks are running and your prompt says to wait, run `gh pr checks --watch` as a BACKGROUND Bash task (the foreground timeout is capped at 10 min). -2. For each failing check, find its run and read the failure: `gh run view --log-failed | head -150`. -3. Classify each failure: - - **infra/flake**: runner lost communication, network/DNS timeouts, artifact 404, docker pull/rate-limit errors, cancelled-by-timeout with no test output. - - **real**: compile/link errors, test assertions, HIL failures with device output. -4. Re-run infra failures once: `gh run rerun --failed`; record run ids in `infraRerun`. -5. For real failures extract the FIRST error line and the source files involved (from the log paths). - -## Bot review harvest - -- Inline review comments: `gh api repos/{owner}/{repo}/pulls//comments --paginate` (use `gh repo view --json nameWithOwner -q .nameWithOwner` for owner/repo). Issue comments: `gh pr view --comments`. -- Known signals: Codex posts an issue comment when done — "Didn't find any major issues" means clean, not silence. Copilot is finished when it no longer appears in `requested_reviewers`. Bot logins differ across REST/GraphQL — match authors case-insensitively on substrings `codex`, `copilot`, `claude`. -- 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 marks the inline thread resolved (via the GraphQL `resolveReviewThread` mutation); you do not post or resolve. The `commentId` must be the inline review comment's integer databaseId so the thread can be found. - -## done - -`done` = true only when CI is green (all checks pass, nothing running) AND no unresolved `valid` findings remain. - -## Output contract - -Your final message is parsed by a program. Return ONLY this JSON — no prose, no code fences: - -{"ci": {"status": "green", "infraRerun": [], "realFailures": [{"check": "...", "firstError": "...", "files": ["..."]}]}, - "findings": [{"source": "codex", "commentId": 123, "file": "...", "line": 1, "claim": "...", "verdict": "valid", "reason": "...", "fixHint": "..."}], - "replies": [{"commentId": 123, "body": "..."}], - "done": false} diff --git a/.claude/agents/pr-review-validator.md b/.claude/agents/pr-review-validator.md new file mode 100644 index 000000000..324e8efcb --- /dev/null +++ b/.claude/agents/pr-review-validator.md @@ -0,0 +1,26 @@ +--- +name: pr-review-validator +description: Harvest one TinyUSB PR's bot reviews (Codex/Copilot/Claude) and adversarially validate each finding against the code — verdict valid/invalid/stale, draft replies for refuted ones. Read-only; never edits code, never posts, never pushes. +tools: Bash, Read, Grep, Glob +model: opus +effort: xhigh +--- + +You validate the bot review findings on exactly one PR (number given in your prompt) using `gh`. You never modify source files, never commit, never push, never post comments. Do not read or classify CI. + +## Procedure + +- Inline review comments: `gh api repos/{owner}/{repo}/pulls//comments --paginate` (use `gh repo view --json nameWithOwner -q .nameWithOwner` for owner/repo). Issue comments: `gh api repos/{owner}/{repo}/issues//comments --paginate` — this returns each comment's integer `id`, which `gh pr view --comments` does not print and the output contract needs. +- Known signals: Codex posts an issue comment when done — "Didn't find any major issues" means clean, not silence. Copilot is finished when it no longer appears in `requested_reviewers`. Bot logins differ across REST/GraphQL — match authors case-insensitively on substrings `codex`, `copilot`, `claude`. +- 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. + +## Output contract + +Your final message is parsed by a program. Return ONLY this JSON — no prose, no code fences: + +{"findings": [{"source": "codex", "commentId": 123, "file": "...", "line": 1, "claim": "...", "verdict": "valid", "reason": "...", "fixHint": "..."}], + "replies": [{"commentId": 123, "body": "..."}], + "done": false} + +done = true only when no unresolved `valid` findings remain. diff --git a/.claude/agents/static-analyzer.md b/.claude/agents/static-analyzer.md index 0f0b2a6e1..e7da82192 100644 --- a/.claude/agents/static-analyzer.md +++ b/.claude/agents/static-analyzer.md @@ -3,6 +3,7 @@ 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 +effort: medium --- 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. diff --git a/.claude/agents/target-debugger.md b/.claude/agents/target-debugger.md index 1b6931307..c7acca91c 100644 --- a/.claude/agents/target-debugger.md +++ b/.claude/agents/target-debugger.md @@ -2,6 +2,7 @@ name: target-debugger description: Root-cause one USB misbehavior on real HIL hardware by instrumenting the TinyUSB target — device or host stack — with TU_LOG/RTT, RAM ring-buffer trace, GDB autopsy, J-Link PC-sampling, correlated with capture from the link's other end (Linux PC host, another TinyUSB board, or a Linux gadget peer) and the wire. Long serial debug loop under one held board lock; strictly one instance. Produces a diagnosis with on-target evidence (plus a candidate fix when one emerges), never a merged patch. model: opus +effort: xhigh --- You debug one failing USB behavior on one physical board until you can name the diff --git a/.claude/workflows/driver-review.js b/.claude/workflows/driver-review.js index 255b8ac74..3d380c7e0 100644 --- a/.claude/workflows/driver-review.js +++ b/.claude/workflows/driver-review.js @@ -1,9 +1,9 @@ export const meta = { name: 'driver-review', - description: 'Review driver directories across dimensions with driver-reviewer scanners, then adversarially verify every finding; returns only confirmed findings', + description: 'Review driver directories across dimensions with code-verifier scanners, then adversarially verify every finding; returns only confirmed findings', whenToUse: 'Auditing dcd/hcd drivers for a bug class (pass question) or a full-dimension review (default dimensions)', phases: [ - { title: 'Scan', detail: 'driver-reviewer per (dir x dimension)' }, + { title: 'Scan', detail: 'code-verifier per (dir x dimension)' }, { title: 'Verify', detail: 'adversarial refutation per finding' }, ], } @@ -58,7 +58,7 @@ const results = await pipeline( p => agent( `Review ${p.dir} for exactly one dimension: ${p.dim}. Read the sources yourself. Coverage-first — report everything, a verifier filters.`, - { label: `scan:${short(p.dir)}`, phase: 'Scan', agentType: 'driver-reviewer', effort: 'xhigh', schema: FINDINGS }, + { label: `scan:${short(p.dir)}`, phase: 'Scan', agentType: 'code-verifier', schema: FINDINGS }, ), (scan, p) => { @@ -69,7 +69,8 @@ const results = await pipeline( `Adversarially verify ONE review finding about ${p.dir}.\nDimension: ${p.dim}\nFinding: ${JSON.stringify(f)}\n` + 'Read the cited code plus enough context (callers, ISR paths, macros, and the datasheet if register-related) to judge. ' + 'Try to REFUTE it; real=true only if it survives your best attempt. Return {"real": bool, "reason": string}.', - { label: `verify:${short(p.dir)}:${f.line}`, phase: 'Verify', agentType: 'driver-reviewer', effort: 'xhigh', schema: VERDICT }, + // max: one judgment-dense call per finding decides what survives - worth the top tier + { label: `verify:${short(p.dir)}:${f.line}`, phase: 'Verify', agentType: 'code-verifier', effort: 'max', schema: VERDICT }, ).then(v => v && { ...f, verdict: v }) )).then(vs => { const alive = vs.filter(Boolean) diff --git a/.claude/workflows/fanout-dev.js b/.claude/workflows/fanout-dev.js index e98a86f1d..386f6df46 100644 --- a/.claude/workflows/fanout-dev.js +++ b/.claude/workflows/fanout-dev.js @@ -1,11 +1,11 @@ export const meta = { name: 'fanout-dev', - description: 'Implement one described change across many ports/file-sets: one port-dev worker per item, independent builder verification, optional review', + description: 'Implement one described change across many ports/file-sets: one code-writer worker per item, independent builder verification, optional review', whenToUse: 'Applying a fix or pattern across multiple TinyUSB ports (e.g. the same DCD bug in several drivers)', phases: [ - { title: 'Implement', detail: 'port-dev per item (opus xhigh)' }, + { title: 'Implement', detail: 'code-writer per item (opus xhigh)' }, { title: 'Verify', detail: 'builder single-example check' }, - { title: 'Review', detail: 'optional driver-reviewer pass' }, + { title: 'Review', detail: 'optional code-verifier pass' }, ], } @@ -71,7 +71,7 @@ const results = await pipeline( : ' Pick a verification board from hw/bsp whose family uses this scope.'), { label: `dev:${short(item)}`, phase: 'Implement', - agentType: 'port-dev', effort: 'xhigh', schema: DEV, + agentType: 'code-writer', schema: DEV, ...(args.worktree ? { isolation: 'worktree' } : {}), }, ), @@ -96,7 +96,7 @@ const results = await pipeline( return agent( `Review the uncommitted change in ${item} (inspect with: git diff -- ${item}) against this task:\n${args.task}\n` + 'Dimension: does the diff correctly and completely implement the task with no unintended side effects? Coverage-first findings.', - { label: `review:${short(item)}`, phase: 'Review', agentType: 'driver-reviewer', effort: 'xhigh', schema: FINDINGS }, + { label: `review:${short(item)}`, phase: 'Review', agentType: 'code-verifier', schema: FINDINGS }, ).then(f => { // review: array = findings; null = reviewer died; absent = not requested if (!f) log(`review:${short(item)}: reviewer agent died`) -- cgit v1.3.1 From 208f82efe8b9dc7ca3eb210dcbf455a14c97cf3c Mon Sep 17 00:00:00 2001 From: hathach Date: Thu, 27 Aug 2026 12:21:04 +0700 Subject: pr-babysit: overlap a fast review lane with the CI watch Review findings are validated, fixed, and pushed without waiting on CI; checkoutDir decouples the PR checkout from the session cwd. File-less CI failures are scoped by a dedicated agent, paths canonicalized and existence-checked via git ls-files, overlapping groups merged. Per-id reply/resolve accounting retries failures and holds the green exit until all outward work is drained. --- .claude/workflows/pr-babysit.js | 362 ++++++++++++++++++++++++++++------------ 1 file changed, 256 insertions(+), 106 deletions(-) diff --git a/.claude/workflows/pr-babysit.js b/.claude/workflows/pr-babysit.js index 406213a5f..2731ed254 100644 --- a/.claude/workflows/pr-babysit.js +++ b/.claude/workflows/pr-babysit.js @@ -1,47 +1,55 @@ 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', + description: 'Drive a PR to green: a fast review lane (validate bot findings, fix, push without waiting on CI) overlapped with a CI-watch lane; code-writer fixes, code-verifier verification, at most one push per lane per cycle', 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 (default false = dry run) } +// args: { pr: number, maxCycles?: number, autoPush?: boolean (default false = dry run), +// checkoutDir?: string (PR branch checkout; default: the session working dir) } if (typeof args === 'string') { try { args = JSON.parse(args) } catch { /* not JSON: shape check below reports it */ } } if (!args || !args.pr) { - throw new Error('args must be { pr: number, maxCycles?, autoPush? }; run from a checkout of the PR branch') + throw new Error('args must be { pr: number, maxCycles?, autoPush?, checkoutDir? }; run from the PR branch checkout or point checkoutDir at it') } args.pr = Number(args.pr) if (!Number.isInteger(args.pr) || args.pr <= 0) { throw new Error('args.pr must be a positive integer PR number') } +const checkoutDir = args.checkoutDir || '.' +if (typeof checkoutDir !== 'string' || checkoutDir.includes("'")) { + throw new Error('checkoutDir must be a plain path string') +} +const IN_CHECKOUT = checkoutDir === '.' ? 'The working tree IS the PR checkout. ' + : `The PR branch checkout is at ${checkoutDir} - run every git/build/file command there, not in the session directory. ` const maxCycles = args.maxCycles ?? 3 if (!Number.isInteger(maxCycles) || maxCycles < 1) { throw new Error('maxCycles must be an integer >= 1') } -const TRIAGE = { +const CI = { type: 'object', additionalProperties: false, - required: ['ci', 'findings', 'replies', 'done'], + required: ['status', 'infraRerun', 'realFailures'], properties: { - ci: { - type: 'object', additionalProperties: false, - required: ['status', 'infraRerun', 'realFailures'], - properties: { - status: { type: 'string', enum: ['green', 'red', 'running'] }, - infraRerun: { type: 'array', items: { type: 'string' } }, - realFailures: { - type: 'array', - items: { - type: 'object', additionalProperties: false, - required: ['check', 'firstError', 'files'], - properties: { - check: { type: 'string' }, firstError: { type: 'string' }, - files: { type: 'array', items: { type: 'string' } }, - }, - }, + status: { type: 'string', enum: ['green', 'red', 'running'] }, + infraRerun: { type: 'array', items: { type: 'string' } }, + realFailures: { + type: 'array', + items: { + type: 'object', additionalProperties: false, + required: ['check', 'firstError', 'files', 'rigSide'], + properties: { + check: { type: 'string' }, firstError: { type: 'string' }, + files: { type: 'array', items: { type: 'string' } }, + rigSide: { type: 'boolean' }, }, }, }, + }, +} +const REVIEWS = { + type: 'object', additionalProperties: false, + required: ['findings', 'replies', 'done'], + properties: { findings: { type: 'array', items: { @@ -84,6 +92,19 @@ const OP = { required: ['pass', 'detail'], properties: { pass: { type: 'boolean' }, detail: { type: 'string' } }, } +const SCOPE = { + type: 'object', additionalProperties: false, + required: ['files'], + properties: { files: { type: 'array', items: { type: 'string' } } }, +} +const OPIDS = { + type: 'object', additionalProperties: false, + required: ['pass', 'detail', 'doneIds'], + properties: { + pass: { type: 'boolean' }, detail: { type: 'string' }, + doneIds: { type: 'array', items: { type: 'integer' } }, + }, +} // Marking a review thread resolved has no REST endpoint — it needs the // GraphQL resolveReviewThread mutation. Shared recipe handed to the posting @@ -107,120 +128,249 @@ const postReplyRecipe = (noun) => const history = [] const repliedIds = new Set() // issue comments can't be thread-resolved, so they re-harvest every cycle — never reply twice -for (let cycle = 1; cycle <= maxCycles; cycle++) { - const t = await agent( - `Triage PR #${args.pr}. If checks are still running, wait for them first (gh pr checks ${args.pr} --watch as a BACKGROUND Bash task; the foreground timeout is capped at 10 min). ` + - 'Then follow your triage procedure: classify CI failures, re-run infra ones, harvest and adversarially validate bot review findings, draft replies for invalid/stale ones.', - { label: `triage#${cycle}`, phase: 'Triage', agentType: 'pr-monitor', schema: TRIAGE }, - ) - if (!t) { - history.push({ cycle, error: 'pr-monitor died' }) - return { pass: false, cycles: cycle, history, reason: 'pr-monitor-died' } - } - const entry = { cycle, triage: t } - history.push(entry) - // 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. - const freshReplies = t.replies.filter(r => !repliedIds.has(r.commentId)) - if (freshReplies.length > 0 && args.autoPush === true) { - const posted = await agent( - `Reply to and resolve these refuted review comments on PR #${args.pr}. For each: ${postReplyRecipe('reply')}` + - `Replies: ${JSON.stringify(freshReplies)}. pass=true only if every reply was posted and every inline thread resolved; detail = what went where.`, - { label: `replies#${cycle}`, phase: 'Push', model: 'sonnet', schema: OP }, - ) - // attempted counts as replied: better to drop a failed reply than spam duplicates - freshReplies.forEach(r => repliedIds.add(r.commentId)) - if (!posted || !posted.pass) log(`cycle ${cycle}: refuted reply/resolve incomplete — ${posted ? posted.detail : 'agent died'}`) - } - - if (t.done) { - log(`cycle ${cycle}: PR is green with no unresolved valid findings`) - return { pass: true, cycles: cycle, history } +// Canonicalize a repo-relative path for set/collision comparison: resolve ./.. +// segments, unify separators; '' for anything that escapes the repo or uses +// characters no repo path does (also makes the path shell-safe to interpolate). +const canon = (p) => { + const s = String(p).trim().replace(/\\/g, '/') + // Absolute (CI-runner) paths: reject rather than corrupt into a bogus relative + // path — the file-less group then routes through the scoper, which recovers the + // real repo path and is existence-checked. + if (s.startsWith('/')) return '' + const out = [] + for (const seg of s.split('/')) { + if (!seg || seg === '.') continue + if (seg === '..') { if (out.pop() === undefined) return '' } else out.push(seg) } + const c = out.join('/') + return /^[A-Za-z0-9._+/-]+$/.test(c) ? c : '' +} - // Group actionable work by top-level scope (plain JS — no model tokens). +// Group actionable notes by top-level scope (plain JS — no model tokens). +const groupWork = (notes) => { const groups = new Map() - const groupOf = (key) => { + for (const n of notes) { + const key = (canon(n.scopeFile) || n.scopeFile).split('/').slice(0, 3).join('/') if (!groups.has(key)) groups.set(key, { key, files: new Set(), notes: [] }) - return groups.get(key) - } - for (const f of t.findings.filter(x => x.verdict === 'valid')) { - const g = groupOf(f.file.split('/').slice(0, 3).join('/')) - g.files.add(f.file) - g.notes.push(`${f.file}:${f.line} [${f.source}] ${f.claim} — hint: ${f.fixHint}`) + const g = groups.get(key) + n.files.forEach(f => { const c = canon(f); if (c) g.files.add(c) }) + g.notes.push(n.text) } - for (const rf of t.ci.realFailures) { - const g = groupOf((rf.files[0] || rf.check).split('/').slice(0, 3).join('/')) - rf.files.forEach(x => g.files.add(x)) - g.notes.push(`CI ${rf.check}: ${rf.firstError}`) - } - const work = [...groups.values()] + return [...groups.values()] +} - if (work.length === 0) { - if (t.ci.status === 'running' || t.ci.infraRerun.length > 0) { - log(`cycle ${cycle}: only infra re-runs in flight — next cycle waits on them`) - continue +// Fix + verify one work list; returns { ok, fixes } — ok only if every group +// was scoped, fixed by a live worker, AND passed code-verifier verification. +const fixAndVerify = async (workIn) => { + // code-writer's contract needs an explicit file set: a group whose notes named no + // files (a CI failure whose log yielded no paths) is scoped by a dedicated agent + // first; if that fails too, the group is withheld (ok=false → human review) rather + // than dispatched with an invalid scope. + const fileless = workIn.filter(w => w.files.size === 0) + await parallel(fileless.map(w => () => + agent( + `${IN_CHECKOUT}Determine which repo files must change to address these notes (read the code; if a note is a CI failure, read its CI log too):\n- ${w.notes.join('\n- ')}\n` + + 'files = repo-relative paths; empty only if genuinely undeterminable.', + { label: `scope:${w.key}`, phase: 'Fix', model: 'sonnet', schema: SCOPE }, + ).then(s => s && s.files.forEach(f => { const c = canon(f); if (c) w.files.add(c) })))) + // Scoped paths are model output: keep only what git ls-files confirms exists. + // The check is executed (by a mechanical agent) and intersected here — a dead + // checker drops every candidate, so unconfirmed groups fall through to withheld. + const candidates = [...new Set(fileless.flatMap(w => [...w.files]))] + if (candidates.length > 0) { + const v = await agent( + `${IN_CHECKOUT}Run exactly: git ls-files -- ${candidates.join(' ')}\nReturn files = the paths that command printed, verbatim — no additions, no substitutions.`, + { label: 'scope:verify', phase: 'Fix', model: 'haiku', schema: SCOPE }, + ) + const exists = new Set((v ? v.files : []).map(canon)) + for (const w of fileless) for (const f of [...w.files]) + if (!exists.has(f)) { w.files.delete(f); log(`scope:${w.key}: dropped ${f} — not confirmed as a repo file`) } + } + const unscoped = workIn.filter(w => w.files.size === 0) + for (const w of unscoped) log(`fix for ${w.key}: no file scope determinable — withheld for human review`) + // Scoping can make groups overlap (two checks resolving to the same file); merge + // intersecting groups (to closure) so two fixers never edit one file concurrently. + const work = [] + for (let g of workIn.filter(w => w.files.size > 0)) { + for (let i; (i = work.findIndex(m => [...g.files].some(f => m.files.has(f)))) >= 0;) { + const [m] = work.splice(i, 1) + g.files.forEach(f => m.files.add(f)); m.notes.push(...g.notes); m.key = `${m.key}+${g.key}` + g = m } - log(`cycle ${cycle}: nothing actionable`) - return { pass: false, cycles: cycle, history, reason: 'unactionable' } + work.push(g) } - + const scopeOf = (w) => [...w.files].join(', ') const fixes = await pipeline( work, w => agent( - `Fix the following issues on the current PR branch (the working tree IS the PR checkout).\n` + - `Scope: ${[...w.files].join(', ')}\nIssues:\n- ${w.notes.join('\n- ')}`, - { label: `fix:${w.key}`, phase: 'Fix', agentType: 'port-dev', effort: 'xhigh', schema: DEV }, + `Fix the following issues on the PR branch. ${IN_CHECKOUT}\n` + + `Scope: ${scopeOf(w)}\nIssues:\n- ${w.notes.join('\n- ')}`, + { label: `fix:${w.key}`, phase: 'Fix', agentType: 'code-writer', schema: DEV }, ), (fix, w) => fix && agent( - `Verify the uncommitted changes for ${[...w.files].join(', ')} (use git diff -- , and read any newly created untracked files directly) address these issues:\n- ${w.notes.join('\n- ')}\n` + + `${IN_CHECKOUT}Verify the uncommitted changes for ${scopeOf(w)} (use git diff -- , and read any newly created untracked files directly) address these issues:\n- ${w.notes.join('\n- ')}\n` + 'Return {"addresses": bool, "reason": string}.', - { label: `check:${w.key}`, phase: 'Verify', agentType: 'driver-reviewer', effort: 'xhigh', schema: CHECK }, + { label: `check:${w.key}`, phase: 'Verify', agentType: 'code-verifier', schema: CHECK }, ).then(v => ({ ...fix, addresses: !!(v && v.addresses), checkReason: v ? v.reason : 'verifier died' })), ) - const aliveFixes = fixes.filter(Boolean) - if (aliveFixes.length < work.length) log(`${work.length - aliveFixes.length} fix group(s) lost to dead workers`) - entry.fixes = aliveFixes - - 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 } - } - - // Verification gates the push: never push a cycle containing an unverified - // fix or the partial edits of a dead worker. - const unverified = aliveFixes.filter(f => f.addresses !== true) - if (aliveFixes.length < work.length || unverified.length > 0) { - for (const f of unverified) log(`fix for ${f.item}: failed verification — ${f.checkReason}`) - log(`cycle ${cycle}: fixes left uncommitted for human review — not pushing unverified changes`) - return { pass: false, cycles: cycle, history, reason: 'fix-verification-failed' } - } + const alive = fixes.filter(Boolean) + if (alive.length < work.length) log(`${work.length - alive.length} fix group(s) lost to dead workers`) + const unverified = alive.filter(f => f.addresses !== true) + for (const f of unverified) log(`fix for ${f.item}: failed verification — ${f.checkReason}`) + return { ok: unscoped.length === 0 && alive.length === work.length && unverified.length === 0, fixes: alive } +} +// Verification gates every push: never push unverified or partial edits. +const commitAndPush = async (cycle, what) => { const push = await agent( - `On the current PR branch: commit ALL working-tree changes as ONE commit (imperative message summarizing the cycle-${cycle} fixes for PR #${args.pr}, repo commit conventions), ` + + `${IN_CHECKOUT}On the PR branch: commit ALL working-tree changes as ONE commit (imperative message summarizing the cycle-${cycle} ${what} fixes for PR #${args.pr}, repo commit conventions), ` + "then push to the PR's remote branch. pass=true only if commit AND push succeeded; detail = pushed SHA.", - { label: `push#${cycle}`, phase: 'Push', model: 'sonnet', schema: OP }, + { label: `push#${cycle}-${what}`, phase: 'Push', model: 'sonnet', schema: OP }, + ) + return push && push.pass ? push : null +} + +for (let cycle = 1; cycle <= maxCycles; cycle++) { + // Two independent lanes, launched together. The review lane never waits on + // CI: it validates, fixes, and pushes while the CI lane is still watching. + const ciPromise = agent( + `Watch CI for PR #${args.pr} per your procedure; wait for pending checks.`, + { label: `ci#${cycle}`, phase: 'Triage', agentType: 'pr-ci-watcher', schema: CI }, + ).catch(e => { log(`cycle ${cycle}: pr-ci-watcher errored — ${e && e.message}`); return null }) + // Every early return below leaves the loop while the CI lane is still + // running: settle it first so no CI agent outlives the workflow. + const stopWith = async (result) => { await ciPromise; return result } + + const r = await agent( + `Validate the bot review findings on PR #${args.pr} per your procedure. ${IN_CHECKOUT}`, + { label: `reviews#${cycle}`, phase: 'Triage', agentType: 'pr-review-validator', schema: REVIEWS }, ) - if (!push || !push.pass) { - log(`cycle ${cycle}: push failed — stopping`) - return { pass: false, cycles: cycle, history, reason: 'push-failed' } + if (!r) { + history.push({ cycle, error: 'pr-review-validator died' }) + return await stopWith({ pass: false, cycles: cycle, history, reason: 'review-validator-died' }) + } + const entry = { cycle, reviews: r } + history.push(entry) + // Outward reply/resolve attempts this cycle that did not fully complete; a green + // PR must not terminate the loop while any remain, or the retry never happens. + let pendingReplies = 0 + + // Post drafted replies to REFUTED findings immediately. Outward-facing, + // so gated on autoPush. + const freshReplies = r.replies.filter(x => !repliedIds.has(x.commentId)) + if (freshReplies.length > 0 && args.autoPush === true) { + const posted = await agent( + `Reply to and resolve these refuted review comments on PR #${args.pr}. For each: ${postReplyRecipe('reply')}` + + 'If a thread already carries an identical reply of ours (a prior attempt that posted but failed to resolve), do not repost — just resolve it. ' + + `Replies: ${JSON.stringify(freshReplies)}. pass=true only if every reply was posted and every inline thread resolved; detail = what went where. ` + + 'doneIds = the commentIds fully handled: reply posted (or already present) AND (thread resolved, or an issue comment with no thread to resolve).', + { label: `replies#${cycle}`, phase: 'Push', model: 'sonnet', schema: OPIDS }, + ) + // Per-id accounting, matching the resolve path: only fully handled ids are marked + // replied; a failed reply/resolve stays fresh and retries next cycle (the prompt's + // already-present check keeps the retry from duplicating the reply). + for (const id of (posted && posted.doneIds) || []) repliedIds.add(id) + pendingReplies += freshReplies.filter(x => !repliedIds.has(x.commentId)).length + if (!posted || !posted.pass) log(`cycle ${cycle}: refuted reply/resolve incomplete — ${posted ? posted.detail : 'agent died'}`) } - // The valid bot findings were fixed and pushed — answer each inline comment - // with what changed and resolve its thread. CI-failure work has no comment. - const fixed = t.findings.filter(x => x.verdict === 'valid') - if (fixed.length > 0) { + // ---- review lane: fix + push without waiting for CI ---- + const validFindings = r.findings.filter(x => x.verdict === 'valid') + let reviewPushed = false + if (validFindings.length > 0) { + const work = groupWork(validFindings.map(f => ({ + scopeFile: f.file, files: [f.file], + text: `${f.file}:${f.line} [${f.source}] ${f.claim} — hint: ${f.fixHint}`, + }))) + const { ok, fixes } = await fixAndVerify(work) + entry.reviewFixes = fixes + if (args.autoPush !== true) { + log('autoPush not set: review-lane fixes left uncommitted (dry run)') + return await stopWith({ pass: false, cycles: cycle, history, dryRun: true }) + } + if (!ok) { + log(`cycle ${cycle}: review-lane fixes left uncommitted for human review — not pushing unverified changes`) + return await stopWith({ pass: false, cycles: cycle, history, reason: 'fix-verification-failed' }) + } + const push = await commitAndPush(cycle, 'review') + if (!push) { + log(`cycle ${cycle}: review-lane push failed — stopping`) + return await stopWith({ pass: false, cycles: cycle, history, reason: 'push-failed' }) + } + reviewPushed = true 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: ${postReplyRecipe('fix note')}` + 'Each reply states the finding is fixed in the pushed commit, with one line on the change. ' + - `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 }, + `Findings: ${JSON.stringify(validFindings.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. ' + + 'doneIds = the commentIds fully handled: reply posted AND (thread resolved, or an issue comment with no thread to resolve).', + { label: `resolve#${cycle}`, phase: 'Push', model: 'sonnet', schema: OPIDS }, ) + // Per-id accounting: a fully handled finding never re-replies (an issue comment + // has no thread to resolve, so it re-harvests as stale next cycle and would get + // a duplicate "fixed" note); an unfinished one stays out of repliedIds so its + // reply/resolve is retried next cycle instead of silently abandoned. + for (const id of (resolved && resolved.doneIds) || []) repliedIds.add(id) + pendingReplies += validFindings.filter(f => !repliedIds.has(f.commentId)).length if (!resolved || !resolved.pass) log(`cycle ${cycle}: fixed reply/resolve incomplete — ${resolved ? resolved.detail : 'agent died'}`) } + + // ---- CI lane result ---- + const c = await ciPromise + entry.ci = c + if (!c) { + log(`cycle ${cycle}: pr-ci-watcher died — re-arming`) + continue + } + if (reviewPushed) { + // The push restarted CI: this cycle's CI verdict is superseded. Re-arm; + // next cycle's ci#N watches the fresh run. + log(`cycle ${cycle}: review-lane push superseded the CI run — re-arming`) + continue + } + const rigSide = c.realFailures.filter(rf => rf.rigSide) + for (const rf of rigSide) log(`cycle ${cycle}: rig-side CI failure (not fixing): ${rf.check} — ${rf.firstError.slice(0, 120)}`) + const fixable = c.realFailures.filter(rf => !rf.rigSide) + if (fixable.length > 0) { + const work = groupWork(fixable.map(rf => ({ + scopeFile: rf.files[0] || rf.check, files: rf.files, + text: `CI ${rf.check}: ${rf.firstError}`, + }))) + const { ok, fixes } = await fixAndVerify(work) + entry.ciFixes = fixes + if (args.autoPush !== true) { + log('autoPush not set: CI-lane fixes left uncommitted (dry run)') + return { pass: false, cycles: cycle, history, dryRun: true } + } + if (!ok) { + log(`cycle ${cycle}: CI-lane fixes left uncommitted for human review — not pushing unverified changes`) + return { pass: false, cycles: cycle, history, reason: 'fix-verification-failed' } + } + if (!(await commitAndPush(cycle, 'ci'))) { + log(`cycle ${cycle}: CI-lane push failed — stopping`) + return { pass: false, cycles: cycle, history, reason: 'push-failed' } + } + continue // pushed: fresh CI run next cycle + } + if (r.done && c.status === 'green') { + if (pendingReplies > 0) { + log(`cycle ${cycle}: PR green but ${pendingReplies} reply/resolve unfinished — re-arming to retry`) + continue + } + log(`cycle ${cycle}: PR is green with no unresolved valid findings`) + return { pass: true, cycles: cycle, history } + } + if (r.done && rigSide.length > 0 && fixable.length === 0 && c.infraRerun.length === 0 && c.status !== 'running') { + log(`cycle ${cycle}: CI red only from rig-side failures — human/rig attention needed, nothing to fix in the PR`) + return { pass: false, cycles: cycle, history, reason: 'ci-red-rig-side' } + } + if (c.status === 'running' || c.infraRerun.length > 0) { + log(`cycle ${cycle}: CI still settling (${c.infraRerun.length} infra re-run(s)) — re-arming`) + continue + } + log(`cycle ${cycle}: nothing actionable`) + return { pass: false, cycles: cycle, history, reason: 'unactionable' } } return { pass: false, cycles: maxCycles, history, reason: 'maxCycles reached' } -- cgit v1.3.1 From 92b38fc3b5e9080c96294079e6117d74429f3448 Mon Sep 17 00:00:00 2001 From: hathach Date: Thu, 27 Aug 2026 12:21:04 +0700 Subject: validate: add claude + codex diff-review stages (opus/high, sol/high) The claude stage reviews the diff directly (the code-review skill is a CLI built-in, unavailable to subagents); the gate is enforced in-script from structured findings, failing only on confirmed correctness/safety bugs. --- .claude/workflows/validate.js | 60 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 57 insertions(+), 3 deletions(-) diff --git a/.claude/workflows/validate.js b/.claude/workflows/validate.js index 522548ee6..dadf8a77f 100644 --- a/.claude/workflows/validate.js +++ b/.claude/workflows/validate.js @@ -1,11 +1,11 @@ export const meta = { name: 'validate', - description: 'Pre-PR software validation: unit tests + per-board build sweeps + code-size compare + PVS, in parallel, joined into one verdict', + description: 'Pre-PR software validation: unit tests + per-board build sweeps + code-size compare + PVS + diff reviews (claude + codex), in parallel, joined into one verdict', whenToUse: 'Before opening or updating a PR, after any non-trivial change', - phases: [{ title: 'Validate', detail: 'unit + builds + size + pvs in parallel' }], + phases: [{ title: 'Validate', detail: 'unit + builds + size + pvs + reviews in parallel' }], } -// args: { boards: string[], examples?: string, base?: string, skip?: ('unit'|'size'|'pvs')[] } +// args: { boards: string[], examples?: string, base?: string, skip?: ('unit'|'size'|'pvs'|'review'|'codex')[] } if (typeof args === 'string') { try { args = JSON.parse(args) } catch { /* not JSON: shape check below reports it */ } } if (!args || !Array.isArray(args.boards) || args.boards.length === 0) { throw new Error('args must be { boards: string[], examples?, base?, skip? }') @@ -56,6 +56,26 @@ const PVS = { }, } +const REVIEW = { + type: 'object', additionalProperties: false, + required: ['pass', 'findings', 'detail'], + properties: { + pass: { type: 'boolean' }, + findings: { + type: 'array', + items: { + type: 'object', additionalProperties: false, + required: ['file', 'line', 'severity', 'summary'], + properties: { + file: { type: 'string' }, line: { type: 'integer' }, + severity: { type: 'string' }, summary: { type: 'string' }, + }, + }, + }, + detail: { type: 'string' }, + }, +} + const thunks = [] if (!skip.includes('unit')) thunks.push(() => @@ -92,6 +112,40 @@ if (!skip.includes('pvs')) thunks.push(() => detail: r.pass ? r.detail : clip(`${r.detail} ${JSON.stringify(r.changedFindings)}`), })) +if (!skip.includes('review')) thunks.push(() => + agent( + `Code-review this branch's diff vs ${base} (git diff ${base}...HEAD), coverage-first: walk every hunk, no spot checks. ` + + 'Find pass — candidate defects across all dimensions: correctness/logic, ISR & concurrency safety, ' + + 'memory/resource handling (bounds, leaks, no dynamic alloc), API contract & spec conformance, ' + + 'security of untrusted input parsing, behavior regressions; plus quality/simplification notes. ' + + 'Verify pass — adversarially check each candidate against the surrounding code: verdict CONFIRMED ' + + '(failing scenario constructed) or PLAUSIBLE (could not refute); report both, drop only refuted ones. ' + + 'Read-only: never apply fixes. severity = verdict plus category (e.g. "CONFIRMED correctness"). ' + + 'pass=false if any CONFIRMED correctness/safety/security bug survives; PLAUSIBLE and quality findings keep pass=true. ' + + 'detail = one-line review summary.', + { label: 'review', phase: 'Validate', model: 'opus', effort: 'high', schema: REVIEW }, + ).then(r => r && { + stage: 'review', + // gate enforced here, not trusted from the agent: any CONFIRMED non-quality finding fails + pass: r.pass && !r.findings.some(f => + /^confirmed/i.test(f.severity) && !/quality|simplification|style/i.test(f.severity)), + findings: r.findings, detail: r.detail, + })) + +if (!skip.includes('codex')) thunks.push(() => + agent( + `Run a Codex review of this branch's diff vs ${base}: ` + + `codex review --base ${base} -c model="gpt-5.6-sol" -c model_reasoning_effort="high" ` + + '(Bash timeout 600000; run from the repo root). Parse its output into findings; severity = Codex\'s priority label. ' + + 'pass=false only if Codex reports a correctness bug (P0/P1); style-level items keep pass=true. ' + + 'detail = Codex\'s overall verdict line. If the codex CLI is missing or the run errors, pass=false with the error in detail.', + { label: 'codex', phase: 'Validate', model: 'haiku', schema: REVIEW }, + ).then(r => r && { + stage: 'codex', + pass: r.pass && !r.findings.some(f => /\bP[01]\b/i.test(f.severity)), + findings: r.findings, detail: r.detail, + })) + const results = (await parallel(thunks)).filter(Boolean) const dead = thunks.length - results.length if (dead > 0) log(`${dead} stage agent(s) died — counted as failures`) -- cgit v1.3.1