From 994282a6bb0fe3013e9c4101a8b5543af78d3dd5 Mon Sep 17 00:00:00 2001 From: Vivek Date: Thu, 30 Jul 2026 11:59:05 +0530 Subject: [PATCH 1/2] docs: tier review round 1 by surface, drop forced minimum rounds The self-review loop charged every PR a fixed minimum of deep-review (7 to 24 agents), a forced second round even after a clean deep-review, and a standalone prose pass. Round 1 is now tiered by the diff's paths (deep-review only when shipped source is touched, one broad reviewer otherwise), a clean round 1 converges the loop by itself, and the converging round's prose-tier findings are applied without re-review in place of the separate prose pass. deep-review defaults trim to 16 agents and three dynamic lenses. Closes #1180. --- .claude/hooks/route-skills.sh | 7 ++-- .claude/skills/webjs-start-work/SKILL.md | 44 ++++++++++++------------ .claude/workflows/deep-review.js | 14 ++++---- test/hooks/review-loop-exit.test.mjs | 41 ++++++++++++++-------- 4 files changed, 60 insertions(+), 46 deletions(-) diff --git a/.claude/hooks/route-skills.sh b/.claude/hooks/route-skills.sh index f01bbdbf7..7d6fe0567 100755 --- a/.claude/hooks/route-skills.sh +++ b/.claude/hooks/route-skills.sh @@ -178,8 +178,9 @@ fi # Triggers: review the PR/diff/branch/changes, code review, look it over # for bugs. Reviewing every change before it is marked ready is a standing # expectation, and a review is a LOOP with the substantive-gate exit the -# webjs-start-work skill defines (loop on substantive findings, then one -# bounded prose pass). Keep this directive in lockstep with that skill's +# webjs-start-work skill defines (loop on substantive findings; the +# converging round's prose findings apply without re-review). Keep this +# directive in lockstep with that skill's # exit condition; test/hooks/review-loop-exit.test.mjs asserts the # pre-#1171 clean-round absolute stays gone. # code-review is a built-in Claude Code skill (no in-repo SKILL.md, so the @@ -187,7 +188,7 @@ fi if has '(review|audit) (the |my |this )?(pr|diff|branch|change|changes|code|commit)' \ || has 'code ?review' \ || has '(review|look) .{0,20}(over )?for (bug|issue|correctness|regression)'; then - add_match "code-review: the request is to review code. Invoke the code-review skill (it reviews the diff for correctness bugs plus reuse and simplification). A review of a PR runs the webjs-start-work self-review loop, whose round 1 is the deep-review workflow; the code-review skill's findings feed that loop as auxiliary input, not as a round of it. Treat review as a LOOP with that skill's exit: after fixing findings, re-review until a round finds nothing SUBSTANTIVE (shipped source, a test's ability to observe the defect it claims to cover, or a factual runtime claim in docs), then one bounded prose pass ends it, and a budget of 5 substantive rounds bounds the whole loop (over budget, stop and report rather than continuing). Never report done off a round that found something substantive. Findings from the code-review skill itself carry no tier tag (its fixed result schema), so it is the one reviewer kind whose findings you classify yourself: doubt resolves to substantive, and every prose classification is recorded like a downgrade, a one-sentence reason on the finding's thread. An untagged finding from any tag-capable reviewer is simply treated as substantive." + add_match "code-review: the request is to review code. Invoke the code-review skill (it reviews the diff for correctness bugs plus reuse and simplification). A review of a PR runs the webjs-start-work self-review loop, whose round 1 is the deep-review workflow when the diff touches shipped source (packages/*/src, packages/cli/lib, packages/cli/templates) and one broad fresh reviewer otherwise; the code-review skill's findings feed that loop as auxiliary input, not as a round of it. Treat review as a LOOP with that skill's exit: after fixing findings, re-review until a round finds nothing SUBSTANTIVE (shipped source, a test's ability to observe the defect it claims to cover, or a factual runtime claim in docs); that round's prose-tier findings are applied without re-review and end the loop, and a budget of 5 substantive rounds bounds the whole loop (over budget, stop and report rather than continuing). Never report done off a round that found something substantive. Findings from the code-review skill itself carry no tier tag (its fixed result schema), so it is the one reviewer kind whose findings you classify yourself: doubt resolves to substantive, and every prose classification is recorded like a downgrade, a one-sentence reason on the finding's thread. An untagged finding from any tag-capable reviewer is simply treated as substantive." fi # --- verify: prove the change works by running the app ------------------ diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index dad7b2eb4..b21654179 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -102,7 +102,7 @@ The user's request typically names an issue by number (e.g. `#112`) or by descri The title MUST carry a conventional-commit prefix from the first moment (feat/fix/perf/breaking appear in the changelog; chore/docs/test/refactor do not), because a single-commit PR squashes on the COMMIT subject and a multi-commit PR on the TITLE. Refine the title/body as the change takes shape; the draft is a living document. Capture the issue URL/number for `Closes #` (already in the body). - From here on, the PR exists, so: commit per logical unit and push after each (the commits stream onto the PR); post design-rationale / decision / follow-up context comments to the PR as those discussions happen (do not hoard them for the end); and run every self-review round ON the PR. The PR is marked **ready for review** (`gh pr ready `) only at the very end, AFTER the Definition of done is satisfied and the self-review loop has converged (a round finding nothing substantive, then the prose pass). Opening late and dumping everything at the end is the anti-pattern this step exists to kill. + From here on, the PR exists, so: commit per logical unit and push after each (the commits stream onto the PR); post design-rationale / decision / follow-up context comments to the PR as those discussions happen (do not hoard them for the end); and run every self-review round ON the PR. The PR is marked **ready for review** (`gh pr ready `) only at the very end, AFTER the Definition of done is satisfied and the self-review loop has converged (a round finding nothing substantive). Opening late and dumping everything at the end is the anti-pattern this step exists to kill. 7. **Report back briefly.** One short message to the user: issue title + number, new branch name, draft PR URL, "project card moved to In progress". Then continue with the actual work the user asked for. @@ -310,7 +310,7 @@ Beyond review findings, proactively record the *reasoning* behind a PR as commen ### How the loop works -The draft PR is already open (step 6), so reviews post to it from the first round. Do NOT mark it ready for review or report "ready for merge" yet. Run rounds of self-review until ONE round finds nothing SUBSTANTIVE (the substantive gate below defines the term); a single bounded prose pass (step 4) then ends the loop. +The draft PR is already open (step 6), so reviews post to it from the first round. Do NOT mark it ready for review or report "ready for merge" yet. Run rounds of self-review until ONE round finds nothing SUBSTANTIVE (the substantive gate below defines the term); that round's prose-tier findings are then applied without re-review and the loop ends (step 4). **A round BUDGET bounds the loop: 5 substantive rounds, raised only by the user's instruction for the task.** When a round would exceed it, stop instead: post that round's findings as usual, withhold the flip to ready for review, and report per `### Reporting after the loop`, ending the loop as OVER BUDGET rather than converged. An over-budget loop is signal about the CHANGE (too big, too entangled) or its FIXES (shipped unproven), and the user deciding what happens next is cheaper than more autonomous rounds; a measured 18-round loop spent its last four rounds catching regressions its own mid-loop fixes introduced. @@ -319,10 +319,10 @@ The draft PR is already open (step 6), so reviews post to it from the first roun - **No full test suite inside the loop.** After a fix, run only the specific test file(s) covering the line you just changed (`node --test `), which is also what the commit rule needs (a logical unit commits when its own tests pass). Do NOT run the e2e suite, the full Node suite, the browser suite, the Bun matrix, or the four-app dogfood boot check between rounds. Those run ONCE, after the loop converges (step 4), and the Definition of done still requires every layer the change touches. This is a timing rule, not permission to skip a layer. **The per-fix scope INCLUDES proof, not just a green run:** a mid-loop fix that changes shipped-source behavior gets the same commit-first red/green counterfactual toggle the Definition of done mandates for the original change (seconds per fix), and a fix touching behavior a prior test claims to cover re-runs THAT test's counterfactual too, because a fix can make an older test non-discriminating without failing it. Unproven mid-loop fixes are how repair regressions buy extra rounds, which costs far more than the toggle. - **Never wait on CI between rounds.** Pushing is fire-and-forget: push, then start the next round immediately. No `gh pr checks --watch` mid-loop. Read CI once, after the loop converges. A round that finds something means another push anyway, which supersedes the run you would have been watching, and merge is gated on green CI regardless, so waiting mid-loop buys nothing. -**Shape the rounds for wall clock: deep-review first, then delta-scoped verification.** Sequential broad rounds re-AUDIT the same surface N times; this shape audits it once, in parallel, then each later round asks only about what changed (while still holding the full changed files as evidence). +**Shape the rounds for wall clock: one tier-matched audit first, then delta-scoped verification.** Sequential broad rounds re-AUDIT the same surface N times; this shape audits it once (in parallel on the deep tier), then each later round asks only about what changed (while still holding the full changed files as evidence). -- **Round 1 is the committed deep-review workflow, for EVERY task that enters the loop** (`.claude/workflows/deep-review.js`, run via the Workflow tool as `{ name: "deep-review", args: "" }`). What it does: a scout reads the diff and proposes up to six dynamic lenses tailored to the PR's nature, which run in parallel alongside six fixed ones (correctness, security, blast radius, test adequacy, invariants and doc drift, fresh eyes; at most THREE reviewers per run are pinned to fable and the rest to opus, so two model families always read the diff at a fixed fable cost), then an adversarial jury refutes each finding. The whole run is HARD-CAPPED at 24 agents by default (`{ pr, maxAgents }` overrides, clamped 8 to 60): dynamic lenses trim first, jury sizes shrink next, and a finding the run cannot verify comes back in an `unverified` list, never silently dropped: raise `maxAgents` when the jury budget fell short, and re-run after fixes when the fixed per-run verification cap truncated, since a bigger `maxAgents` cannot recover a capped finding. Its CONFIRMED findings are the round's findings, the ones you act on; the jury-REJECTED ones come back too, carrying their refuters' reasons, but only for the audit trail (post them with the round per step 2's record rule; they are already adjudicated, need no action, and do not by themselves force another round). Uniform on purpose: its cost self-scales (juries convene per finding, so a quiet PR is cheap), and a uniform rule cannot be misjudged the way a by-risk bifurcation can, which matters because a `packages/*/src` bug ships into every end-user app. The whole run counts as ONE round, clean only when it confirms nothing; its confirmed SUBSTANTIVE findings feed this same loop (fix, post as one review object, delta round), while a run whose confirmed findings are all prose-tier exits the substantive loop per steps 3 and 4 (being round 1, it still gets the delta-shaped pass the two-round minimum requires before the prose pass). A deep-review run that dies or errors is a failed spawn under the liveness rules below: re-run it, and never substitute an inline pass. -- **Every later round is delta-scoped: ONE reviewer whose QUESTION is what the last round changed** (or, when a round produced NO fix commits, the riskiest section of the original diff). Delta-scoped narrows the question, never the evidence: the reviewer still receives the full changed files and MUST trace the fix's blast radius, grepping every symbol, rule, or concept the fix touches across the whole PR surface and comparing each other occurrence, since a small fix can break something far from its own hunk. What it does not do is re-audit the unchanged surface from scratch, which is the all-pairs reasoning that makes broad rounds slow. If it finds something, fix and repeat the delta shape. Re-run deep-review in full, not a delta, when the surface round 1 audited is no longer the surface under review: a fix rewrote something well outside the original diff (for shipped source, a fix spanning files is reason enough), OR the PR's SCOPE changed after round 1, new work folded in, a user-directed pivot, an issue absorbed mid-review. A delta round verifies fixes to an audited surface; it cannot stand in for the audit of a surface round 1 never saw. +- **Round 1's shape is decided by the SURFACES in the PR's diff, a path check, never a judgment call.** Read the touched paths (`gh pr diff --name-only`). A diff touching SHIPPED SOURCE, any path under `packages/*/src/`, `packages/cli/lib/`, or `packages/cli/templates/` (the code and templates that land in every end-user app), takes the committed deep-review workflow as round 1 (`.claude/workflows/deep-review.js`, run via the Workflow tool as `{ name: "deep-review", args: "" }`). Every other diff (docs, website, blog, skills, hooks, tests-only, examples) takes ONE broad fresh reviewer as round 1: the delta-verifier template below with the FULL diff as its question. A mixed diff takes the deep tier (one shipped-source path is enough), and the USER asking for a deep review always forces it. The tier is path-based for the same reason the substantive gate is surface-based: membership is checkable from the diff, so the agent motivated to go light cannot misjudge it, and the deep tier's cost lands exactly where a bug ships into every end-user app. What deep-review does: a scout reads the diff and proposes up to three dynamic lenses tailored to the PR's nature, which run in parallel alongside six fixed ones (correctness, security, blast radius, test adequacy, invariants and doc drift, fresh eyes; at most THREE reviewers per run are pinned to fable and the rest to opus, so two model families always read the diff at a fixed fable cost), then an adversarial jury refutes each finding. The whole run is HARD-CAPPED at 16 agents by default (`{ pr, maxAgents }` overrides, clamped 8 to 60): dynamic lenses trim first, jury sizes shrink next, and a finding the run cannot verify comes back in an `unverified` list, never silently dropped: raise `maxAgents` when the jury budget fell short, and re-run after fixes when the fixed per-run verification cap truncated, since a bigger `maxAgents` cannot recover a capped finding. Its CONFIRMED findings are the round's findings, the ones you act on; the jury-REJECTED ones come back too, carrying their refuters' reasons, but only for the audit trail (post them with the round per step 2's record rule; they are already adjudicated, need no action, and do not by themselves force another round). Either round-1 shape counts as ONE round, clean only when it confirms nothing substantive; substantive findings feed this same loop (fix, post as one review object, delta round), while a round 1 whose findings are all prose-tier exits the substantive loop per steps 3 and 4. A round-1 reviewer that dies or errors is a failed spawn under the liveness rules below: re-run it, and never substitute an inline pass. +- **Every later round is delta-scoped: ONE reviewer whose QUESTION is what the last round changed** (or, when a round produced NO fix commits, the riskiest section of the original diff). Delta-scoped narrows the question, never the evidence: the reviewer still receives the full changed files and MUST trace the fix's blast radius, grepping every symbol, rule, or concept the fix touches across the whole PR surface and comparing each other occurrence, since a small fix can break something far from its own hunk. What it does not do is re-audit the unchanged surface from scratch, which is the all-pairs reasoning that makes broad rounds slow. If it finds something, fix and repeat the delta shape. Re-run round 1 in full, not a delta, when the surface round 1 audited is no longer the surface under review: a fix rewrote something well outside the original diff (for shipped source, a fix spanning files is reason enough), OR the PR's SCOPE changed after round 1, new work folded in, a user-directed pivot, an issue absorbed mid-review. A scope change that pulls shipped source into a light-tier PR re-runs round 1 at the DEEP tier. A delta round verifies fixes to an audited surface; it cannot stand in for the audit of a surface round 1 never saw. - **Starve reviewers of context they do not need.** Each gets the PR diff, the PR title and body (the author's claims, which the review checks against), the touched files, and the rule files it judges against (`AGENTS.md`, `CONVENTIONS.md`), nothing more. NEVER feed them prior PR comments or reviews (measured on #1159: the comment payload alone reached 171 KB by the fifth round and dominated its cost), and never a growing already-handled exclusion list. Dedupe repeat findings yourself: a duplicate costs you a second of reading, while the exclusion list costs every reviewer minutes of ingestion. **The substantive gate: which findings keep the loop open.** Findings come in two tiers, and only the substantive tier forces another round. Measured need (#1171): a 14-round loop stopped touching the shipped guard after round 6, then spent rounds 10 to 13 relitigating wording about a transpiler, because "a round that finds nothing" never arrives on a surface where prose always yields another nit. @@ -333,16 +333,16 @@ The draft PR is already open (step 6), so reviews post to it from the first roun - **Prose-tier findings are still findings.** They are posted, and each is fixed, rejected, or deferred like any other (step 2), but their fixes are applied WITHOUT a re-review round and they never force one. One exception: a prose fix that itself touches a substantive surface (a code example inside a docstring, a doc claim about runtime behavior) gets one delta-scoped check of THAT FIX ALONE. - **Never publish a rule about a third-party tool's internals** (a transpiler's constant folding, an engine's optimization behavior) as a fix or a doc claim: those internals are unversioned and unobservable, so the rule cannot stay true. The measured incident is four successive constant-folding rules for one Bun version, each falsified by the next round. Fix such a finding by removing the claim, not by refining it. -The gate changes only WHEN the loop exits, never WHETHER a surface is reviewed: every finding of both tiers is still posted and resolved, and the prose pass in step 4 is mandatory, not optional. +The gate changes only WHEN the loop exits, never WHETHER a surface is reviewed: every finding of both tiers is still posted and resolved, and the converging round's prose-tier fixes are still applied (step 4). Each round must: -1. **Run the round's reviewer: the round-1 deep-review workflow, or the single delta verifier** (the delta verifier via the Agent tool with `subagent_type: "general-purpose"` and `run_in_background: true`; round 1 via the Workflow tool, which also runs in the background and reports through its completion notification). A fresh reviewer has no prior context on the decisions you made, so it's less likely to share your blind spots. That is also why reviewers are ONE-SHOT, never persistent teammates: a teammate that keeps context across rounds re-derives its own prior conclusions, which is the blind-spot sharing this rule exists to kill. Teammates fit collaborating work; reviewing is the one job where accumulated context is the failure mode. **Async plus a watchdog, never an UNBOUNDED wait in either direction.** A synchronous spawn blocks the whole turn, so a hung reviewer is indistinguishable from a working one for as long as the harness takes to give up (a delta round that should cost 1 to 5 minutes once ate 20 this way, unwatchable from inside the blocked call). So spawn the reviewer in the background AND, immediately, start a background watchdog on the task's output file. Take the path from the spawn's own tool result or task notification, NEVER a guess (a watchdog once aimed at the newest file in the tasks directory and watched its own output). The watchdog has TWO MODES, decided by what that file actually is: +1. **Run the round's reviewer: round 1's reviewer at the diff's tier, or the single delta verifier** (the broad round-1 reviewer and the delta verifier via the Agent tool with `subagent_type: "general-purpose"` and `run_in_background: true`; a deep-tier round 1 via the Workflow tool, which also runs in the background and reports through its completion notification). A fresh reviewer has no prior context on the decisions you made, so it's less likely to share your blind spots. That is also why reviewers are ONE-SHOT, never persistent teammates: a teammate that keeps context across rounds re-derives its own prior conclusions, which is the blind-spot sharing this rule exists to kill. Teammates fit collaborating work; reviewing is the one job where accumulated context is the failure mode. **Async plus a watchdog, never an UNBOUNDED wait in either direction.** A synchronous spawn blocks the whole turn, so a hung reviewer is indistinguishable from a working one for as long as the harness takes to give up (a delta round that should cost 1 to 5 minutes once ate 20 this way, unwatchable from inside the blocked call). So spawn the reviewer in the background AND, immediately, start a background watchdog on the task's output file. Take the path from the spawn's own tool result or task notification, NEVER a guess (a watchdog once aimed at the newest file in the tasks directory and watched its own output). The watchdog has TWO MODES, decided by what that file actually is: - **Transcript mode**, when the file demonstrably grows: probe its mtime every 30 seconds and fire on about 2 minutes of stall or 10 minutes total. On a fire, peek at harness evidence only (the task's status, whether the file is still growing; the words in any `tail` are NEVER read as a prose claim of liveness), stop a stalled reviewer (`TaskStop`) and re-spawn it per the re-spawn bullet, leave a progressing one alone. - **Stub mode**, when the file is a static stub whose mtime never moves, which is the PERMANENT HEALTHY state of every worktree-isolated reviewer, and therefore the mode for every reviewer you spawn directly (two healthy isolated ones were once killed on exactly that misread): there is NO mid-flight activity signal, so the watchdog is the 10-minute hard timer alone, the harness's completion or killed notification is the only earlier news, and the wait is blind but BOUNDED, which is the point. Kill only on the hard cap or a killed/errored task status, nothing softer. When the reviewer completes first, stop the watchdog. (Backgrounding does not protect you from a declined spawn: a refusal lands on the tool call itself, and a Workflow call can be declined the same way.) - **Reviewer liveness (non-negotiable).** The harness status is the ONLY liveness signal. For BOTH reviewer kinds, an Agent spawn and a Workflow run alike, the completion signal is the harness notification and the result it carries, and the mid-flight signal is the watchdog in whichever mode the task's output file supports, an activity probe on a real transcript or the bare hard timer on a stub (`TaskStop` works on either task id), so neither kind is ever an UNBOUNDED wait. A run whose harness status is killed or errored is a dead round exactly like a dead spawn. Transcript-file activity (mtime, growth) is harness evidence where the file is a real transcript, and no signal at all where it is a static stub; the WORDS in a transcript are an agent's own prose and count for nothing in either mode. A spawn that was declined at the permission prompt, returned an internal error, or came back with an empty result means NOTHING is running and no review happened. Never read an agent's own prose as evidence it is alive: a background reviewer once signed off with "Yes, it's running. I have the diff and I'm partway through the careful pass" while its harness status read `killed`, and taking that at face value is exactly what turns a dead round into an unbounded wait. If a background reviewer is in play anyway (a `/code-review` agent, or a named agent from an earlier turn), its status is the harness completion notification for that task, which arrives on its own and carries the agent's real result, never what the agent last said, and a `killed` or errored status is terminal. Do not poll it, do not wait on it, do not re-read its message hoping for a different answer. `TaskList` is the shared to-do list, NOT a registry of running subagents, so an empty `TaskList` is evidence of nothing here. A dead STRAY background agent (a `/code-review` agent, a named agent from an earlier turn) is not by itself a blocked loop: it was never the round's reviewer, so write it off and run the round's real reviewer, the deep-review workflow for round 1 or the delta verifier after it. Only the round's own reviewer being unproducible blocks the loop. The rules below follow: recognise the failure, re-spawn it per the re-spawn bullet, and only a reviewer that cannot be produced at all blocks the loop. + **Reviewer liveness (non-negotiable).** The harness status is the ONLY liveness signal. For BOTH reviewer kinds, an Agent spawn and a Workflow run alike, the completion signal is the harness notification and the result it carries, and the mid-flight signal is the watchdog in whichever mode the task's output file supports, an activity probe on a real transcript or the bare hard timer on a stub (`TaskStop` works on either task id), so neither kind is ever an UNBOUNDED wait. A run whose harness status is killed or errored is a dead round exactly like a dead spawn. Transcript-file activity (mtime, growth) is harness evidence where the file is a real transcript, and no signal at all where it is a static stub; the WORDS in a transcript are an agent's own prose and count for nothing in either mode. A spawn that was declined at the permission prompt, returned an internal error, or came back with an empty result means NOTHING is running and no review happened. Never read an agent's own prose as evidence it is alive: a background reviewer once signed off with "Yes, it's running. I have the diff and I'm partway through the careful pass" while its harness status read `killed`, and taking that at face value is exactly what turns a dead round into an unbounded wait. If a background reviewer is in play anyway (a `/code-review` agent, or a named agent from an earlier turn), its status is the harness completion notification for that task, which arrives on its own and carries the agent's real result, never what the agent last said, and a `killed` or errored status is terminal. Do not poll it, do not wait on it, do not re-read its message hoping for a different answer. `TaskList` is the shared to-do list, NOT a registry of running subagents, so an empty `TaskList` is evidence of nothing here. A dead STRAY background agent (a `/code-review` agent, a named agent from an earlier turn) is not by itself a blocked loop: it was never the round's reviewer, so write it off and run the round's real reviewer, round 1's reviewer at its tier or the delta verifier after it. Only the round's own reviewer being unproducible blocks the loop. The rules below follow: recognise the failure, re-spawn it per the re-spawn bullet, and only a reviewer that cannot be produced at all blocks the loop. - **A failed spawn means the round did not happen.** Do not count it as a round, do not report it as a round, and do not let it advance the loop toward convergence. A failed spawn is not something to stop and report on its own, though: re-spawn it per the re-spawn bullet. Do still run the repo-health check below: a spawn that dies after its isolated worktree was created is the likeliest leaker of all, so that check keys off the SPAWN, not off a round completing. - **A reviewer that returns without reviewing is also a failed round.** A subagent can come back successfully and still have reviewed nothing ("I could not fetch the diff", a refusal, an answer to some other question, a summary of the PR body with no reading of the code). The prompt tells it to report `BLOCKED` plus what it is missing when it CANNOT review, which covers the could-not-fetch case; a refusal or an off-topic answer will not carry the sentinel, so do not rely on it alone: any answer that is not the reviewer's expected result shape is the same state, and the absence of findings is NOT a clean round. The expected shape is per reviewer kind: the delta verifier returns a finding list or the literal `CLEAN` (a list with missing tier tags is still a valid round; per the substantive gate an untagged finding is treated as substantive); a deep-review run returns its structured result with `confirmed` and `rejected` lists, and anything else from it (prose, a partial result, nothing) means the round did not happen. Treat it exactly like a failed spawn: no round happened, so re-spawn per the re-spawn bullet, feeding it whatever the reviewer said it was missing (the diff, a path, the base branch). - **NEVER downgrade to an inline self-review.** The fresh subagent is required, not preferred, because reviewing your own work re-derives the same assumptions that produced the bug. That downgrade has already shipped three real bugs into a PR that two inline rounds had called clean (an unguarded `.action=${fn}` property path, a test-less branch the suite would not have missed, and a factually wrong commit message), every one of them caught by the first genuine subagent round that followed. An inline pass is strictly worse than no review, because it looks like one. @@ -357,15 +357,15 @@ Each round must: - In the TASK's worktree, `git rev-parse --abbrev-ref HEAD` is still the feature branch and `git status` is clean (`git -C ...`; the worktree itself holds the branch, so a checkout there cannot collide). In the PRIMARY checkout the healthy HEAD is `main`; a primary sitting on anything else means something mutated it, so put it back with `git checkout -f main`. Never try `git checkout -f ` in the primary: the task worktree holds that branch and git refuses. The merge / commits are always safe on GitHub regardless; this only repairs the LOCAL repo. Nothing is lost. If you cannot cleanly repair, `git checkout -f main && git pull` to resync, then continue. - For the DELTA verifier (round 1's prompts live inside the deep-review workflow), pass a prompt that: + For the DELTA verifier and the light tier's broad round 1 (a deep-tier round 1's prompts live inside the deep-review workflow), pass a prompt that: - Names the PR number and branch. - - Tells it to fetch the diff (`gh pr diff --repo webjsdev/webjs`) and read the changed files in context. Those plus the PR title and body (`gh pr view --repo webjsdev/webjs --json title,body`) and the rule files, never prior PR comments or reviews, per the starve rule above. A delta round keeps the same evidence but makes the fix commits' diff its question, with a blast-radius instruction per the round-shape rules; a delta pass following a round with no fix commits takes the riskiest section of the original diff as its question instead. + - Tells it to fetch the diff (`gh pr diff --repo webjsdev/webjs`) and read the changed files in context. Those plus the PR title and body (`gh pr view --repo webjsdev/webjs --json title,body`) and the rule files, never prior PR comments or reviews, per the starve rule above. A delta round keeps the same evidence but makes the fix commits' diff its question, with a blast-radius instruction per the round-shape rules; a delta pass following a round with no fix commits takes the riskiest section of the original diff as its question instead; a light-tier round 1 makes the FULL diff its question. - Instructs it to look for: bugs, regressions, security issues, missed edge cases, broken invariants, doc drift, test gaps, stylistic problems against `AGENTS.md` and `CONVENTIONS.md` (root and per-package). - Asks for a numbered list with `file:line` references, each finding tagged `substantive` or `prose` per the substantive gate's three surfaces (the template below carries the definition, so the tag arrives with the finding and you only verify it). Problems only, no suggestions. - Contains, verbatim: "If you find nothing genuinely wrong, say exactly `CLEAN` on its own line and stop." plus a no-padding instruction. Keep `CLEAN` and its "on its own line" exactly as written; the loop tests for that literal sentinel, so a paraphrase in the prompt makes a clean round unrecognisable. - Tells it to say `BLOCKED` plus what it is missing if it CANNOT review (no diff, no access, wrong repo), instead of falling through to `CLEAN`. Without this the two sanctioned outputs are a finding list or `CLEAN`, so a reviewer that never saw the diff has found nothing genuinely wrong and reports itself clean, which is precisely the non-review the liveness rules must catch. -2. **For each finding the subagent reports**, do exactly ONE of these three. There is no fourth option, and "mention it and move on" is not allowed. One gate first, for a DELTA round's findings on shipped source (round-1 findings skip it, since deep-review's juries already refuted them): a finding whose fix would be expensive or behavior-changing gets an adversarial REFUTER before you act, a fresh subagent handed the same evidence and told to DISPROVE the claim (does the scenario reproduce in the code as written, is the behavior actually intended, is it already guarded or tested somewhere the finder did not look). A finding the refuter kills is a rejection with the refuter's reason; the rest proceed. The refuter is a reviewer spawn like any other: step 1's working-tree-safety rules (`isolation: "worktree"`, read-only git, the per-spawn repo-health check) and liveness rules (re-spawn a failed one) apply to it in full, and if a refuter cannot be produced, act on the finding ungated, since the gate is an optimization and must fail toward treating the finding as real. This costs one cheap spawn and saves the expensive failure, a confident false positive that buys a wrong "fix" plus the delta rounds to unwind it. Trivial or obviously-real findings skip the gate. +2. **For each finding the subagent reports**, do exactly ONE of these three. There is no fourth option, and "mention it and move on" is not allowed. One gate first, for findings on shipped source from any round OTHER than a deep-review run (deep-review's confirmed findings skip it, since its juries already refuted them): a finding whose fix would be expensive or behavior-changing gets an adversarial REFUTER before you act, a fresh subagent handed the same evidence and told to DISPROVE the claim (does the scenario reproduce in the code as written, is the behavior actually intended, is it already guarded or tested somewhere the finder did not look). A finding the refuter kills is a rejection with the refuter's reason; the rest proceed. The refuter is a reviewer spawn like any other: step 1's working-tree-safety rules (`isolation: "worktree"`, read-only git, the per-spawn repo-health check) and liveness rules (re-spawn a failed one) apply to it in full, and if a refuter cannot be produced, act on the finding ungated, since the gate is an optimization and must fail toward treating the finding as real. This costs one cheap spawn and saves the expensive failure, a confident false positive that buys a wrong "fix" plus the delta rounds to unwind it. Trivial or obviously-real findings skip the gate. - **Fix it** on the branch (commit + push to update the PR), OR - **Reject it** explicitly with a one-sentence reason, stated in your reply to the user and recorded on the finding's thread (`rejected because `, per the posting rules; not in the PR body). Rejection has to be defensible (e.g. "the agent flagged X as a security issue but X runs server-side only and never reaches user input"). False positives are real; reject them on the merits, don't just hand-wave. OR - **Defer it** when it is genuine but out of scope for THIS PR (a pre-existing bug, an unrelated dependency/hygiene problem, a separate feature). A finding on code this PR adds or changes is NEVER out of scope, whatever its size: it is fixed or rejected, then and there, and deferral cannot be the path around either. "Out of scope" is NOT a reason to drop a finding, but the loop NEVER files a follow-up issue on its own: the owner has said unfiled work piling up from review loops is unaffordable, so the loop's job ends at making the finding durable and visible. Record the deferral on the finding's thread (one sentence on why it is out of scope) and carry it into the end-of-loop report, where the USER decides what, if anything, gets filed; invoke `webjs-file-issue` only on their explicit go-ahead. **The loop's FINAL summary review on the PR also carries a deferral ledger** whenever anything was deferred: a short "Deferred findings" list, each entry naming the finding and its one-sentence reason, so a cold reader of the PR sees every deferral in one place without walking the threads (a loop with no deferrals says nothing). A deferred finding with no thread record and no line in the report is a dropped finding, which is the mistake this clause exists to prevent. If unsure whether something is in-scope, default to fixing it in this PR; only defer when it is clearly separable and fixing it here would mean scope creep. @@ -374,15 +374,15 @@ Each round must: Post rejected findings and false positives too, so the reasoning is auditable on the PR. A round that reviewed and found nothing posts a short summary review saying so. Do this as part of the loop, not as an afterthought. Build the review JSON with a real serializer, never by interpolating into a shell string: review text is dense with backticks, and an unquoted shell string runs them as command substitution. That is exactly how a review on #1115 lost every code reference and had to be deleted and reposted. (Reminder: em-dashes and the other banned glyphs in AGENTS.md invariant 11 apply to every comment body, since they go through the same tooling.) -3. **If the round produced any actionable SUBSTANTIVE findings (a deep-review run's CONFIRMED list, or anything a delta verifier reports, with tags verified per the substantive gate, including findings YOU then reject, since your own rejection is unadjudicated judgment in a way a jury's is not)**, resolve them and run a delta-scoped round: ONE fresh reviewer whose question is the fix commits' diff, with the full changed files as evidence and the blast-radius grep the round-shape rules mandate (when the round produced no fix commits, the question is the riskiest section instead). Prose-tier findings from the same round are resolved too, under the gate's no-re-review rule, and do not by themselves trigger the delta round. Fresh matters: on #1159, three consecutive rounds each found problems INTRODUCED by the previous round's fix, which is exactly what a re-used reviewer, already invested in that fix, is worst at seeing. If the delta round finds something substantive, fix and repeat the delta shape; deep-review re-runs only when the round-shape rules call for it (a fix well outside the original diff, or a scope change). +3. **If the round produced any actionable SUBSTANTIVE findings (a deep-review run's CONFIRMED list, or anything a broad or delta verifier reports, with tags verified per the substantive gate, including findings YOU then reject, since your own rejection is unadjudicated judgment in a way a jury's is not)**, resolve them and run a delta-scoped round: ONE fresh reviewer whose question is the fix commits' diff, with the full changed files as evidence and the blast-radius grep the round-shape rules mandate (when the round produced no fix commits, the question is the riskiest section instead). Prose-tier findings from the same round are resolved too, under the gate's no-re-review rule, and do not by themselves trigger the delta round. Fresh matters: on #1159, three consecutive rounds each found problems INTRODUCED by the previous round's fix, which is exactly what a re-used reviewer, already invested in that fix, is worst at seeing. If the delta round finds something substantive, fix and repeat the delta shape; round 1 re-runs in full only when the round-shape rules call for it (a fix well outside the original diff, or a scope change, at the tier the new surface calls for). -4. **If the round confirms nothing substantive** (the literal `CLEAN`, a deep-review run that confirms nothing, or findings that are all prose-tier once the tags are verified), the substantive loop is done, with one exception: a round 1 that exits here, whether clean or prose-only, still gets the delta-shaped pass the two-round minimum requires. Then run the ONE prose pass: a single fresh reviewer (delta shape, under the same isolation, liveness, and posting rules) whose question is ONLY the prose-tier surfaces of the whole PR (wording, docstrings and comments, the PR body). Apply its fixes WITHOUT re-review and stop; the pass is exactly one round, because prose always yields another nit and re-reviewing prose is the non-termination the substantive gate exists to kill. Its one exception is the gate's: a prose fix that touches a substantive surface gets one delta-scoped check of that fix alone. The prose pass is mandatory and runs after every loop; it never counts toward the round total or the two-round minimum. Its reviewer is subject to the same liveness and re-spawn rules as any round's reviewer, and a prose-pass reviewer that cannot be produced at all ends the loop BLOCKED per `## Failure handling`, never as converged. Once it is done, NOW run everything the loop deferred: the full suites for every layer the change touches (e2e, Node, browser, Bun matrix, the four-app dogfood boot check, per the Definition of done), and only now read CI. Run them in PARALLEL: launch each suite as its own background Bash task in one batch, plus the CI read as a background watch (an `until` loop that exits when no check is pending), then collect EVERY launched task's result before reporting anything. The set is independent work, so its wall clock is the slowest suite rather than the sum, and a background task you forget to collect is a silently skipped layer, so the report waits for all of them. This changes only the shape; the WHEN-not-WHETHER rule above still decides what runs. +4. **If the round confirms nothing substantive** (the literal `CLEAN`, a deep-review run that confirms nothing, or findings that are all prose-tier once the tags are verified), the loop is converged: apply that round's prose-tier fixes WITHOUT re-review and stop. There is no separate prose sweep and no forced extra round: every reviewer already reports and tags prose findings alongside substantive ones, so the converging round's prose-tier findings ARE the sweep, and re-reviewing prose is the non-termination the substantive gate exists to kill. The one exception is the gate's: a prose fix that touches a substantive surface gets one delta-scoped check of that fix alone. Once the prose fixes are committed, NOW run everything the loop deferred: the full suites for every layer the change touches (e2e, Node, browser, Bun matrix, the four-app dogfood boot check, per the Definition of done), and only now read CI. Run them in PARALLEL: launch each suite as its own background Bash task in one batch, plus the CI read as a background watch (an `until` loop that exits when no check is pending), then collect EVERY launched task's result before reporting anything. The set is independent work, so its wall clock is the slowest suite rather than the sum, and a background task you forget to collect is a silently skipped layer, so the report waits for all of them. This changes only the shape; the WHEN-not-WHETHER rule above still decides what runs. -The minimum is TWO rounds: deep-review, then at least one delta verify. A deep-review that confirms nothing substantive is rare and usually means the lenses were too shallow for this PR; before believing it, run one delta-shaped pass with the riskiest section as its question anyway. +The minimum is ONE round. A round 1, either tier, that finds nothing substantive converges the loop by itself: on the deep tier its findings were already adversarially jury-refuted, and on the light tier nothing in the diff is shipped source, so forcing an extra pass on exactly the cleanest case was pure length (the two-round minimum this rule replaces). A fix always buys a fresh delta round; a clean round never does. -**The LAST round must find nothing substantive, always. A fix is never the end of the loop.** The moment a round surfaces a genuine substantive finding and you fix it (or reject it), you have NOT finished: the fix changed the branch, so it needs its own round. Run another round on the new HEAD and keep going until one round finds nothing substantive; the prose pass (step 4) then ends the loop. Do NOT report "fixed it" or "ready to merge" after a round that found something substantive, even if the fix is obviously correct and tests pass. The recurring failure this prevents: review finds X, you fix X and immediately report done, having never re-reviewed the fix. The mirror failure is looping on prose (the rounds-10-to-13 relitigation the substantive gate describes), which the gate ends. If you ever notice you are about to report a result where the most recent review round found a substantive finding, stop and run another round first. TWO cases end the loop without converging, and neither is ever reported as done: a reviewer that cannot be produced at all, meaning it cannot be spawned or never returns an actual review despite the re-spawns (see the liveness rules in step 1), ends it as BLOCKED; a spent round budget (the budget rule at the top of this section) ends it as OVER BUDGET. +**The LAST round must find nothing substantive, always. A fix is never the end of the loop.** The moment a round surfaces a genuine substantive finding and you fix it (or reject it), you have NOT finished: the fix changed the branch, so it needs its own round. Run another round on the new HEAD and keep going until one round finds nothing substantive; that round's prose fixes are applied and the loop ends (step 4). Do NOT report "fixed it" or "ready to merge" after a round that found something substantive, even if the fix is obviously correct and tests pass. The recurring failure this prevents: review finds X, you fix X and immediately report done, having never re-reviewed the fix. The mirror failure is looping on prose (the rounds-10-to-13 relitigation the substantive gate describes), which the gate ends. If you ever notice you are about to report a result where the most recent review round found a substantive finding, stop and run another round first. TWO cases end the loop without converging, and neither is ever reported as done: a reviewer that cannot be produced at all, meaning it cannot be spawned or never returns an actual review despite the re-spawns (see the liveness rules in step 1), ends it as BLOCKED; a spent round budget (the budget rule at the top of this section) ends it as OVER BUDGET. -**A standalone "review the PR" request IS the loop, not a one-shot, and it ALWAYS starts with a fresh deep-review run.** When the user asks you to review an existing PR (separately from the post-`gh pr create` flow), you re-enter this exact loop from round 1: a full deep-review, regardless of how many review cycles the PR has already been through, because the ask itself says the existing trail is not trusted or not current. An explicit ask also overrides the trivial-change skip below: when the user asks for a review, they get one, however small the diff. Then, if it finds anything substantive, fix/reject/defer it and run delta rounds until one finds nothing substantive, then close with the prose pass, BEFORE you report back. "I reviewed it and fixed one thing" is not an acceptable stopping point; "I reviewed it in a loop, the last round found nothing substantive, and the prose pass ran" is. The user should never have to ask "did the loop converge?" because you should not have stopped until it did. Arriving here skips step 1's preamble, so read it before spawning, BOTH halves. The working-tree-safety block still applies (worktree isolation for reviewers you spawn directly, the read-only git prohibition for every reviewer including deep-review's internal agents, and the repo-health check after each spawn or workflow run, including failed ones), and so do the liveness rules and the round budget: the only reasons to stop short of convergence are the two sanctioned exits, a reviewer that cannot be produced (reported as blocked, never papered over with an inline pass of your own) and a spent round budget (reported as over budget). +**A standalone "review the PR" request IS the loop, not a one-shot, and it ALWAYS starts a fresh round 1 at the tier the diff's paths call for.** When the user asks you to review an existing PR (separately from the post-`gh pr create` flow), you re-enter this exact loop from round 1 (deep-review for a shipped-source diff, the single broad reviewer otherwise, and an explicit ask for a DEEP review forces the deep tier), regardless of how many review cycles the PR has already been through, because the ask itself says the existing trail is not trusted or not current. An explicit ask also overrides the trivial-change skip below: when the user asks for a review, they get one, however small the diff. Then, if it finds anything substantive, fix/reject/defer it and run delta rounds until one finds nothing substantive, apply that round's prose findings, and only then report back. "I reviewed it and fixed one thing" is not an acceptable stopping point; "I reviewed it in a loop and the last round found nothing substantive" is. The user should never have to ask "did the loop converge?" because you should not have stopped until it did. Arriving here skips step 1's preamble, so read it before spawning, BOTH halves. The working-tree-safety block still applies (worktree isolation for reviewers you spawn directly, the read-only git prohibition for every reviewer including deep-review's internal agents, and the repo-health check after each spawn or workflow run, including failed ones), and so do the liveness rules and the round budget: the only reasons to stop short of convergence are the two sanctioned exits, a reviewer that cannot be produced (reported as blocked, never papered over with an inline pass of your own) and a spent round budget (reported as over budget). ### When to skip the loop @@ -392,15 +392,15 @@ Skip only for PRs that change a single line of trivially-correct content (a doc After the loop converges and the deferred suites plus the CI read (step 4) are done, report exactly this shape to the user: -> PR # is up at . Self-review loop ran rounds plus the prose pass; last round found nothing substantive. Issues found and fixed during the loop: . Out-of-scope findings, recorded on the PR and awaiting your call on filing: . Ready to merge. +> PR # is up at . Self-review loop ran rounds; the last found nothing substantive. Issues found and fixed during the loop: . Out-of-scope findings, recorded on the PR and awaiting your call on filing: . Ready to merge. When any finding was deferred, the chat report does not stop at that one-line list: expand each deferral right there, mirroring the PR ledger (the finding, its one-sentence reason, its thread), and END WITH A DIRECT QUESTION, on its own line, asking whether to file follow-up issues for these deferrals and which ones. The question is mandatory, not a courtesy: filing happens only on the user's answer, silence is never consent, and a deferral report that does not ask has not finished reporting. The user reads the deferral summary and answers the filing question in the chat without opening the PR; the PR ledger exists for the cold reader later. -**Only a round in which fresh subagents actually REVIEWED counts toward ``, and a deep-review run, all its lenses and juries together, is ONE round. The prose pass is reported by name and never counts toward `` or the two-round minimum.** A spawn that was declined, errored, was killed, or returned nothing did not produce a round, so it can be neither the converging last round nor part of the total, and an inline pass you did yourself is not a round at all (see the liveness rules in step 1). Neither does a spawn that returned a result without reviewing: a completed call and a real subagent result are NOT the test, because a reviewer that says "I could not fetch the diff" satisfies both and reviewed nothing. The test is that it came back in its expected result shape: findings or the literal `CLEAN` from a delta verifier (tags are expected, but a list with missing tier tags is still a valid round, its untagged findings treated as substantive per the gate), the structured `confirmed`/`rejected` result from a deep-review run. A loop whose reviewer never reviewed has run ZERO rounds, and the honest report there is that the review is blocked and why, not a count and not "ready to merge". +**Only a round in which fresh subagents actually REVIEWED counts toward ``, and a deep-review run, all its lenses and juries together, is ONE round.** A spawn that was declined, errored, was killed, or returned nothing did not produce a round, so it can be neither the converging last round nor part of the total, and an inline pass you did yourself is not a round at all (see the liveness rules in step 1). Neither does a spawn that returned a result without reviewing: a completed call and a real subagent result are NOT the test, because a reviewer that says "I could not fetch the diff" satisfies both and reviewed nothing. The test is that it came back in its expected result shape: findings or the literal `CLEAN` from a broad or delta verifier (tags are expected, but a list with missing tier tags is still a valid round, its untagged findings treated as substantive per the gate), the structured `confirmed`/`rejected` result from a deep-review run. A loop whose reviewer never reviewed has run ZERO rounds, and the honest report there is that the review is blocked and why, not a count and not "ready to merge". **An OVER-BUDGET loop reports the rounds run, every open finding with its PR-review link, and your read on why it was not converging** (repair regressions, a genuinely deep surface, reviewer quality), and never says "ready to merge". The findings stay posted and unresolved on the PR, which stays a draft. The deferred suites and the CI read are converged-path steps that gate the flip to ready, so an over-budget stop does not run them, and the report says so. -If you cannot honestly say "the last round found nothing substantive and the prose pass ran", you cannot say "ready to merge". If a finding is rejected as a false positive, mention it in the report so the user can second-guess the rejection. Every finding the loop surfaced must be accounted for in this report as fixed, rejected-with-reason, or deferred-as-out-of-scope (recorded on its thread, listed here AND in the final summary review's deferral ledger per step 2's defer rule, filed only on the user's go-ahead); a deferred finding missing from the thread, this report, or that ledger is a dropped finding, and you have not finished the loop. Every genuine finding must ALSO appear as a comment on the PR (see step 2), so the report and the PR agree. +If you cannot honestly say "the last round found nothing substantive", you cannot say "ready to merge". If a finding is rejected as a false positive, mention it in the report so the user can second-guess the rejection. Every finding the loop surfaced must be accounted for in this report as fixed, rejected-with-reason, or deferred-as-out-of-scope (recorded on its thread, listed here AND in the final summary review's deferral ledger per step 2's defer rule, filed only on the user's go-ahead); a deferred finding missing from the thread, this report, or that ledger is a dropped finding, and you have not finished the loop. Every genuine finding must ALSO appear as a comment on the PR (see step 2), so the report and the PR agree. **Merge is gated on green CI, enforced at the branch level, not by trust.** A PR must not merge until all CI checks pass. `main` branch protection requires the five `ci.yml` checks (Conventions, Unit+integration, Browser, E2E, Build) before any merge; if `gh api repos/webjsdev/webjs/branches/main/protection` shows `required_status_checks: null`, run `bash scripts/protect-main.sh` once (needs repo admin) to restore it. Do not work around a red or pending check; wait for green. @@ -409,7 +409,7 @@ If you cannot honestly say "the last round found nothing substantive and the pro ### Subagent prompt template ``` -Review PR # (branch ``) at https://github.com/webjsdev/webjs/pull/ for bugs, regressions, security issues, missed edge cases, broken invariants, doc drift, test gaps, and style violations against the project's AGENTS.md and CONVENTIONS.md (root + per-package). +Review PR # (branch ``) at https://github.com/webjsdev/webjs/pull/ for bugs, regressions, security issues, missed edge cases, broken invariants, doc drift, test gaps, and style violations against the project's AGENTS.md and CONVENTIONS.md (root + per-package). HARD CONSTRAINT, read first: you are running against a repository the main session is actively using, and every worktree of it shares ONE `.git` directory, so a git write here reaches the main session's checkout even from an isolated worktree. You are a READ-ONLY reviewer. Do NOT run any command that changes git branch, HEAD, the index, or the working tree: no `git checkout`, `git switch`, `git reset`, `git restore`, `git stash`, `git pull`, `git fetch` that moves refs, `git merge`, `git rebase`, `git clean`, `git branch -f`, or `git worktree`. Any of these silently corrupts the main session's checkout (it moved HEAD off the branch and looked like lost work, and a stray worktree op once flipped the shared repo's `core.bare` to `true`). You do NOT need to switch branches to review. Use `gh pr diff ` and `gh pr view ` for the diff and metadata, and read any file at its PR-branch state with `gh api repos///contents/?ref= --jq .content | base64 -d`. All of those read from GitHub, so they work whether or not the branch exists locally, which matters because a PR you were asked to review may not be checked out here at all. If the branch does happen to be the one checked out, reading files in place is fine too. The only git you may run is read-only inspection (`git log`, `git show`, `git diff` WITHOUT changing state, `git status`, `git blame`). If you think you need to change git state to do the review, you are wrong; report what you found instead. @@ -419,7 +419,7 @@ You start with no prior context on this PR. Steps: 2. Run `gh pr view --repo webjsdev/webjs --json title,body` to see what the author claims it does. 3. Read every file the diff touches in its current state (not just the diff hunks) so you see edits in context. 4. Read root AGENTS.md, the per-package AGENTS.md for each touched package, and CONVENTIONS.md if a scaffolded template was touched. -5. Specifically check: . +5. Specifically check: . Report findings as a numbered list with file:line references. Tag each finding `substantive` or `prose`: `substantive` when it touches shipped source, a test's ability to OBSERVE the defect it claims to cover (a tautological assertion that stays green with the bug present is substantive), or a factual claim about runtime behavior in docs (a number in docs that states runtime behavior, a default, a limit, a condition list, is this surface, not prose); `prose` for everything else (wording, PR-body counts, comment style, review artifacts). Problems only. No suggestions, no nits about style if the rule isn't enforceable. If you find nothing genuinely wrong, say exactly `CLEAN` on its own line and stop. Do not pad with "looks good overall" or summaries. @@ -480,5 +480,5 @@ A merge updates `main` and npm, but the four in-repo apps deployed to Railway (` - If the issue is already in `In progress` (someone else's work, or a prior branch left open): report this and ask the user whether to continue on the existing branch, branch off a fresh main, or pick a different issue. - If the TASK worktree regressed mid-loop (its HEAD detached or off the feature branch, work seemingly "gone"): a review subagent mutated shared git state. In the PRIMARY, HEAD on `main` is the healthy state, not a regression. Do NOT panic or redo work. The local feature-branch ref and `origin/` still point at the latest commit (every logical unit was pushed). Recover with `git -C checkout ` (anchored: run bare from the primary it would succeed and park the PRIMARY on the feature branch, since a detached worktree no longer holds it); confirm with `git log --oneline origin/main..HEAD` and `git status` clean. The PR on GitHub was never affected (the GitHub-reading reviewer still saw correct content), so no re-push or force-push is needed. - If the round budget (default 5 substantive rounds, user-raisable per task) is spent before convergence: stop the loop as OVER BUDGET per the budget rule in `### How the loop works`. Post the final round's findings as usual, withhold the flip to ready for review, and report the rounds run, the open findings, and your read on the non-convergence cause. The branch, the commits, and the card all stay exactly as they are. -- If the round's reviewer, or the prose-pass reviewer, cannot be produced (a deep-review run or a delta spawn that is declined at the permission prompt, errors, dies, hangs past its watchdog, or returns without its expected result shape): the round, or the pass, did not happen. Re-run it (re-invoke the workflow, re-spawn the verifier), varying the approach after a few identical failures, and do NOT stop mid-loop to report the failure or ask how to proceed: recovering costs seconds and interrupting costs the loop its momentum. Only a reviewer that cannot be produced at all blocks the loop, which is one of its two exits other than convergence (the other is the round budget's OVER BUDGET stop, above); then withhold the flip to ready for review and say once, at the end, that the review did not run and why. Keep that out of the PR body and PR comments, since a spawn that could not run is session tooling rather than a fact about the change. Do NOT review it yourself inline and count that as the round; an inline pass is what let three real bugs through a supposedly clean PR. The branch, the commits, and the card all stay exactly as they are. Full rules in step 1 of the self-review loop. +- If the round's reviewer cannot be produced (a deep-review run, a light-tier broad reviewer, or a delta spawn that is declined at the permission prompt, errors, dies, hangs past its watchdog, or returns without its expected result shape): the round did not happen. Re-run it (re-invoke the workflow, re-spawn the verifier), varying the approach after a few identical failures, and do NOT stop mid-loop to report the failure or ask how to proceed: recovering costs seconds and interrupting costs the loop its momentum. Only a reviewer that cannot be produced at all blocks the loop, which is one of its two exits other than convergence (the other is the round budget's OVER BUDGET stop, above); then withhold the flip to ready for review and say once, at the end, that the review did not run and why. Keep that out of the PR body and PR comments, since a spawn that could not run is session tooling rather than a fact about the change. Do NOT review it yourself inline and count that as the round; an inline pass is what let three real bugs through a supposedly clean PR. The branch, the commits, and the card all stay exactly as they are. Full rules in step 1 of the self-review loop. - If the `gh project item-edit` call fails (auth scope, missing field): report the failure clearly and offer to do the move manually via the web UI. The branch creation still stands. diff --git a/.claude/workflows/deep-review.js b/.claude/workflows/deep-review.js index a387cce7a..49e94addc 100644 --- a/.claude/workflows/deep-review.js +++ b/.claude/workflows/deep-review.js @@ -1,10 +1,10 @@ export const meta = { name: 'deep-review', description: 'Deep multi-agent PR review: parallel finder lenses, then adversarial verification; confirmed findings are actionable, jury-rejected ones return for the audit trail', - whenToUse: 'The round-1 review for any PR entering a review loop, or a heavyweight pass on demand. Works on any repo. Pass the PR number as args, "owner/repo#123" for another repository, or { pr, repo, lenses, maxAgents } where explicit lenses skip the scout and maxAgents caps the whole run (default 24).', + whenToUse: 'The round-1 review for a PR whose diff touches shipped source (packages/*/src, packages/cli/lib, packages/cli/templates), or a heavyweight pass on demand. Works on any repo. Pass the PR number as args, "owner/repo#123" for another repository, or { pr, repo, lenses, maxAgents } where explicit lenses skip the scout and maxAgents caps the whole run (default 16).', phases: [ { title: 'Scope', detail: 'a scout reads the diff and proposes dynamic lenses for this PR' }, - { title: 'Find', detail: 'six fixed lenses plus up to six dynamic ones, in parallel, at most three pinned to fable and the rest opus' }, + { title: 'Find', detail: 'six fixed lenses plus up to three dynamic ones, in parallel, at most three pinned to fable and the rest opus' }, { title: 'Verify', detail: 'adversarial refuters per finding, majority rules' }, ], } @@ -31,7 +31,7 @@ const givenLenses = (args && typeof args === 'object' && Array.isArray(args.lens // silently dropped. Two distinct causes with two distinct remedies: a jury // budget shortfall (raise maxAgents) and the fixed per-run verification cap // (CAP below; re-run after fixes, maxAgents cannot recover it). -const rawMax = (args && typeof args === 'object' && Number.isFinite(Number(args.maxAgents))) ? Number(args.maxAgents) : 24 +const rawMax = (args && typeof args === 'object' && Number.isFinite(Number(args.maxAgents))) ? Number(args.maxAgents) : 16 const MAX_AGENTS = Math.min(60, Math.max(8, Math.floor(rawMax))) if (!pr) throw new Error('Pass the PR number as args, e.g. Workflow({ name: "deep-review", args: "123" }), or "owner/repo#123", or { pr, repo, lenses, maxAgents }') @@ -107,7 +107,7 @@ const LENS_PROPOSALS = { properties: { lenses: { type: 'array', - maxItems: 6, + maxItems: 3, items: { type: 'object', required: ['key', 'prompt'], @@ -124,21 +124,21 @@ phase('Scope') let spent = 0 let dynamic = [] if (givenLenses) { - dynamic = givenLenses.slice(0, 6).map((l) => ({ key: String(l.key), prompt: String(l.prompt) })) + dynamic = givenLenses.slice(0, 3).map((l) => ({ key: String(l.key), prompt: String(l.prompt) })) log(`using ${dynamic.length} caller-provided dynamic lenses, scout skipped`) } else { spent += 1 const scout = await agent( `${SAFETY} -You are the SCOUT for a deep review of PR ${pr}. Read the diff and the PR body, understand what kind of work this PR actually does, and propose ZERO to six ADDITIONAL review lenses tailored to it. Six fixed lenses already run regardless, so never duplicate their ground: correctness of the changed logic, security, repo-wide ripple effects of touched symbols, test adequacy and counterfactuals, invariant and doc drift, and a broad fresh-eyes pass. +You are the SCOUT for a deep review of PR ${pr}. Read the diff and the PR body, understand what kind of work this PR actually does, and propose ZERO to three ADDITIONAL review lenses tailored to it. Six fixed lenses already run regardless, so never duplicate their ground: correctness of the changed logic, security, repo-wide ripple effects of touched symbols, test adequacy and counterfactuals, invariant and doc drift, and a broad fresh-eyes pass. Propose a lens only when THIS PR's nature earns it. Examples of the kind of thing that earns one: concurrency or race conditions if the diff touches async coordination; serialization compatibility if a wire format changed; migration or data-loss paths if storage schemas moved; API backward compatibility if public signatures changed; performance if a hot path was rewritten; accessibility if UI semantics changed; prompt-injection surfaces if agent-facing prose or tooling changed. Zero is a fine answer for a PR whose nature the fixed six already cover. Each proposal is a key plus a one-paragraph reviewer charter written like an order: what to hunt, where in this diff, and what evidence would confirm it.`, { label: 'scout:lenses', phase: 'Scope', schema: LENS_PROPOSALS }, ) - dynamic = ((scout && scout.lenses) || []).slice(0, 6) + dynamic = ((scout && scout.lenses) || []).slice(0, 3) log(`scout proposed ${dynamic.length} dynamic lens(es)${dynamic.length ? ': ' + dynamic.map((l) => l.key).join(', ') : ''}`) } // Budget: fixed lenses always run; dynamic ones fit in what remains after diff --git a/test/hooks/review-loop-exit.test.mjs b/test/hooks/review-loop-exit.test.mjs index 2b16d0a81..8309ad83c 100644 --- a/test/hooks/review-loop-exit.test.mjs +++ b/test/hooks/review-loop-exit.test.mjs @@ -39,21 +39,26 @@ test('the skill carries the substantive gate and its two-tier exit', () => { // and the one tagless-reviewer kind the orchestrator classifies itself. assert.match(skill, /arrives UNTAGGED from a reviewer whose result shape carries the tag is treated as substantive/); assert.match(skill, /structurally cannot tag/); - // The bounded prose pass and its non-round status. - assert.match(skill, /never counts toward the round total or the two-round minimum/); + // The exit: the converging round's prose findings apply without a + // re-review round, and there is no standalone prose pass to relitigate. + assert.match(skill, /apply that round's prose-tier fixes WITHOUT re-review and stop/); + assert.match(skill, /converging round's prose-tier findings ARE the sweep/); + assert.ok(!skill.includes('prose pass'), 'the standalone prose pass must not resurface in the skill'); + // A prose fix that touches a substantive surface still gets its one + // delta-scoped check (the gate's exception survives the pass removal). + assert.match(skill, /gets one delta-scoped check of THAT FIX ALONE/); // The re-tag license is asymmetric and downgrades are auditable. assert.match(skill, /DOWNGRADE to prose is treated like a rejection/); assert.match(skill, /recorded on the finding's thread/); - // The two-round minimum survives a prose-only round 1, in step 4 AND in - // the round-1 bullet that restates the exit. - assert.match(skill, /whether clean or prose-only, still gets the delta-shaped pass/); - assert.match(skill, /still gets the delta-shaped pass the two-round minimum requires before the prose pass/); + // Round 1 is tiered by the diff's PATHS (never by judged importance), a + // mixed diff escalates, and the minimum is one round, so a clean round 1 + // converges without a forced extra pass. + assert.match(skill, /Round 1's shape is decided by the SURFACES in the PR's diff, a path check/); + assert.match(skill, /one shipped-source path is enough/); + assert.match(skill, /The minimum is ONE round/); + assert.match(skill, /pulls shipped source into a light-tier PR re-runs round 1 at the DEEP tier/); // An untagged list is still a valid round (round counting stays sane). assert.match(skill, /untagged findings treated as substantive per the gate/); - // An unproducible prose-pass reviewer is BLOCKED, not converged, and the - // failure taxonomy covers the prose pass explicitly. - assert.match(skill, /prose-pass reviewer that cannot be produced at all ends the loop BLOCKED/); - assert.match(skill, /or the prose-pass reviewer, cannot be produced/); // The round budget bounds the loop: 5 substantive rounds, OVER BUDGET is // a non-converged exit with its own report, and the failure taxonomy // carries it. @@ -91,6 +96,12 @@ test('deep-review findings carry a required tier with the gate wording', () => { assert.match(workflow, /a number in docs that states runtime behavior/); // The tier is orthogonal to severity, stated so dedup changes stay honest. assert.match(workflow, /surface classification, not a severity judgment/); + // The trimmed defaults hold: 16 agents, at most three dynamic lenses, + // and whenToUse scopes round 1 to shipped-source diffs. + assert.match(workflow, /\? Number\(args\.maxAgents\) : 16/); + assert.match(workflow, /maxItems: 3/); + assert.match(workflow, /ZERO to three ADDITIONAL review lenses/); + assert.match(workflow, /whose diff touches shipped source/); // Tier outranks severity everywhere findings compete: the same-line // dedup collision, the tier-first sort that drives the CAP slice and // the jury-budget walk, and a missing tier fails OPEN (substantive). @@ -122,14 +133,16 @@ test('the routed review directive states the substantive exit, not the old absol // relitigation; it must not resurface in the injected directive. assert.ok(!hook.includes('until a round is clean'), 'route-skills.sh reverted to the pre-#1171 exit'); assert.match(hook, /nothing SUBSTANTIVE/); - assert.match(hook, /prose pass/); + assert.ok(!hook.includes('prose pass ends it'), 'route-skills.sh reverted to the standalone prose pass'); + assert.match(hook, /prose-tier findings are applied without re-review/); assert.match(hook, /found something substantive/); // The tagless-reviewer clause stays in lockstep with the skill's gate: // orchestrator-classified, doubt to substantive, prose recorded. assert.match(hook, /recorded like a downgrade/); - // The directive and the skill prescribe the SAME round-1 reviewer: the - // deep-review workflow, with code-review findings as auxiliary input. - assert.match(hook, /round 1 is the deep-review workflow/); + // The directive and the skill prescribe the SAME tiered round-1 reviewer, + // with code-review findings as auxiliary input. + assert.match(hook, /round 1 is the deep-review workflow when the diff touches shipped source/); + assert.match(hook, /one broad fresh reviewer otherwise/); assert.match(hook, /auxiliary input/); // Self-classification is scoped to the code-review skill's own findings; // untagged findings from tag-capable reviewers stay substantive. From 0b840df55df9d5891266846dbc89a369f053d192 Mon Sep 17 00:00:00 2001 From: Vivek Date: Thu, 30 Jul 2026 12:07:52 +0530 Subject: [PATCH 2/2] docs: close the guard test's prose-pass observability hole The negative anchor checked only the two-word spelling, so a partial revert reintroducing the hyphenated prose-pass reference stayed green. Both spellings are now rejected in the skill and the hook, the new failure-handling wording is pinned, the liveness enumeration names the broad round-1 reviewer, and the report template drops the rounds-2+ relic. Hunk-level counterfactuals verified red for the skill and hook. --- .claude/skills/webjs-start-work/SKILL.md | 4 ++-- test/hooks/review-loop-exit.test.mjs | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index b21654179..3a692190b 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -344,7 +344,7 @@ Each round must: **Reviewer liveness (non-negotiable).** The harness status is the ONLY liveness signal. For BOTH reviewer kinds, an Agent spawn and a Workflow run alike, the completion signal is the harness notification and the result it carries, and the mid-flight signal is the watchdog in whichever mode the task's output file supports, an activity probe on a real transcript or the bare hard timer on a stub (`TaskStop` works on either task id), so neither kind is ever an UNBOUNDED wait. A run whose harness status is killed or errored is a dead round exactly like a dead spawn. Transcript-file activity (mtime, growth) is harness evidence where the file is a real transcript, and no signal at all where it is a static stub; the WORDS in a transcript are an agent's own prose and count for nothing in either mode. A spawn that was declined at the permission prompt, returned an internal error, or came back with an empty result means NOTHING is running and no review happened. Never read an agent's own prose as evidence it is alive: a background reviewer once signed off with "Yes, it's running. I have the diff and I'm partway through the careful pass" while its harness status read `killed`, and taking that at face value is exactly what turns a dead round into an unbounded wait. If a background reviewer is in play anyway (a `/code-review` agent, or a named agent from an earlier turn), its status is the harness completion notification for that task, which arrives on its own and carries the agent's real result, never what the agent last said, and a `killed` or errored status is terminal. Do not poll it, do not wait on it, do not re-read its message hoping for a different answer. `TaskList` is the shared to-do list, NOT a registry of running subagents, so an empty `TaskList` is evidence of nothing here. A dead STRAY background agent (a `/code-review` agent, a named agent from an earlier turn) is not by itself a blocked loop: it was never the round's reviewer, so write it off and run the round's real reviewer, round 1's reviewer at its tier or the delta verifier after it. Only the round's own reviewer being unproducible blocks the loop. The rules below follow: recognise the failure, re-spawn it per the re-spawn bullet, and only a reviewer that cannot be produced at all blocks the loop. - **A failed spawn means the round did not happen.** Do not count it as a round, do not report it as a round, and do not let it advance the loop toward convergence. A failed spawn is not something to stop and report on its own, though: re-spawn it per the re-spawn bullet. Do still run the repo-health check below: a spawn that dies after its isolated worktree was created is the likeliest leaker of all, so that check keys off the SPAWN, not off a round completing. - - **A reviewer that returns without reviewing is also a failed round.** A subagent can come back successfully and still have reviewed nothing ("I could not fetch the diff", a refusal, an answer to some other question, a summary of the PR body with no reading of the code). The prompt tells it to report `BLOCKED` plus what it is missing when it CANNOT review, which covers the could-not-fetch case; a refusal or an off-topic answer will not carry the sentinel, so do not rely on it alone: any answer that is not the reviewer's expected result shape is the same state, and the absence of findings is NOT a clean round. The expected shape is per reviewer kind: the delta verifier returns a finding list or the literal `CLEAN` (a list with missing tier tags is still a valid round; per the substantive gate an untagged finding is treated as substantive); a deep-review run returns its structured result with `confirmed` and `rejected` lists, and anything else from it (prose, a partial result, nothing) means the round did not happen. Treat it exactly like a failed spawn: no round happened, so re-spawn per the re-spawn bullet, feeding it whatever the reviewer said it was missing (the diff, a path, the base branch). + - **A reviewer that returns without reviewing is also a failed round.** A subagent can come back successfully and still have reviewed nothing ("I could not fetch the diff", a refusal, an answer to some other question, a summary of the PR body with no reading of the code). The prompt tells it to report `BLOCKED` plus what it is missing when it CANNOT review, which covers the could-not-fetch case; a refusal or an off-topic answer will not carry the sentinel, so do not rely on it alone: any answer that is not the reviewer's expected result shape is the same state, and the absence of findings is NOT a clean round. The expected shape is per reviewer kind: the broad round-1 reviewer and the delta verifier return a finding list or the literal `CLEAN` (a list with missing tier tags is still a valid round; per the substantive gate an untagged finding is treated as substantive); a deep-review run returns its structured result with `confirmed` and `rejected` lists, and anything else from it (prose, a partial result, nothing) means the round did not happen. Treat it exactly like a failed spawn: no round happened, so re-spawn per the re-spawn bullet, feeding it whatever the reviewer said it was missing (the diff, a path, the base branch). - **NEVER downgrade to an inline self-review.** The fresh subagent is required, not preferred, because reviewing your own work re-derives the same assumptions that produced the bug. That downgrade has already shipped three real bugs into a PR that two inline rounds had called clean (an unguarded `.action=${fn}` property path, a test-less branch the suite would not have missed, and a factually wrong commit message), every one of them caught by the first genuine subagent round that followed. An inline pass is strictly worse than no review, because it looks like one. - **Re-spawn it. Do not interrupt the user over a failed spawn.** A spawn that broke mechanically (an internal error, a timeout, a returned-without-reviewing result) is harness noise, so just spawn it again, adjusting the prompt if the reviewer said what it was missing. Keep re-spawning until one takes. A handful of consecutive identical failures means something structural is wrong, so vary the approach (a smaller scope, a different focus, a fresh prompt) rather than firing the same call a tenth time. Recovering costs a few seconds; asking the user costs the whole loop its momentum. **Never** stop mid-loop to report a failed spawn, ask how to proceed, or hand back a half-finished loop. The user asked for a converged review, not a status update, and the loop is not done until it converges. - **A genuinely unrecoverable reviewer is the only FAILURE stopping point, and even that is reported at the end** (the round budget is the other sanctioned stop, and it is not a failure). If re-spawning cannot produce a working reviewer at all, the loop has not converged, so withhold the flip to ready for review and say plainly what happened, once, when you report back. Keep it in the conversation: a spawn that could not run is session tooling, not a fact about the change, so it does NOT go into the PR body or a PR comment (the PR records the review, not the harness). What you must never do is substitute an inline pass to keep the loop moving, which is the failure this whole block exists to prevent. @@ -392,7 +392,7 @@ Skip only for PRs that change a single line of trivially-correct content (a doc After the loop converges and the deferred suites plus the CI read (step 4) are done, report exactly this shape to the user: -> PR # is up at . Self-review loop ran rounds; the last found nothing substantive. Issues found and fixed during the loop: . Out-of-scope findings, recorded on the PR and awaiting your call on filing: . Ready to merge. +> PR # is up at . Self-review loop ran rounds; the last found nothing substantive. Issues found and fixed during the loop: . Out-of-scope findings, recorded on the PR and awaiting your call on filing: . Ready to merge. When any finding was deferred, the chat report does not stop at that one-line list: expand each deferral right there, mirroring the PR ledger (the finding, its one-sentence reason, its thread), and END WITH A DIRECT QUESTION, on its own line, asking whether to file follow-up issues for these deferrals and which ones. The question is mandatory, not a courtesy: filing happens only on the user's answer, silence is never consent, and a deferral report that does not ask has not finished reporting. The user reads the deferral summary and answers the filing question in the chat without opening the PR; the PR ledger exists for the cold reader later. diff --git a/test/hooks/review-loop-exit.test.mjs b/test/hooks/review-loop-exit.test.mjs index 8309ad83c..c05f2ae05 100644 --- a/test/hooks/review-loop-exit.test.mjs +++ b/test/hooks/review-loop-exit.test.mjs @@ -43,7 +43,10 @@ test('the skill carries the substantive gate and its two-tier exit', () => { // re-review round, and there is no standalone prose pass to relitigate. assert.match(skill, /apply that round's prose-tier fixes WITHOUT re-review and stop/); assert.match(skill, /converging round's prose-tier findings ARE the sweep/); - assert.ok(!skill.includes('prose pass'), 'the standalone prose pass must not resurface in the skill'); + assert.ok(!/prose[- ]pass/i.test(skill), 'the standalone prose pass must not resurface in the skill (either spelling)'); + // The failure taxonomy names the light-tier reviewer instead of the + // removed prose-pass reviewer. + assert.match(skill, /a light-tier broad reviewer, or a delta spawn/); // A prose fix that touches a substantive surface still gets its one // delta-scoped check (the gate's exception survives the pass removal). assert.match(skill, /gets one delta-scoped check of THAT FIX ALONE/); @@ -133,7 +136,7 @@ test('the routed review directive states the substantive exit, not the old absol // relitigation; it must not resurface in the injected directive. assert.ok(!hook.includes('until a round is clean'), 'route-skills.sh reverted to the pre-#1171 exit'); assert.match(hook, /nothing SUBSTANTIVE/); - assert.ok(!hook.includes('prose pass ends it'), 'route-skills.sh reverted to the standalone prose pass'); + assert.ok(!/prose[- ]pass/i.test(hook), 'route-skills.sh reverted to the standalone prose pass (directive or comment)'); assert.match(hook, /prose-tier findings are applied without re-review/); assert.match(hook, /found something substantive/); // The tagless-reviewer clause stays in lockstep with the skill's gate: