From 2046d5f1de7be162d9b26746be9542b83971efc9 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 29 Jul 2026 19:03:08 +0530 Subject: [PATCH 01/12] docs: exit the review loop on a substantive gate plus one prose pass The loop's exit was 'a round that finds nothing', which cannot terminate on prose surfaces: a 14-round loop stopped touching the shipped guard after round 6 and spent rounds 10 to 13 relitigating wording about a transpiler. The exit is now two-tier: loop while a round finds anything 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 whose fixes are applied without re-review. The gate is surface-based, not importance-based, so the agent motivated to terminate cannot classify its way out; reviewers tag each finding and the orchestrator verifies the tag, with untagged findings treated as substantive. Also bans publishing rules about a third-party tool's internals (four successive falsified Bun constant-folding rules). --- .claude/skills/webjs-start-work/SKILL.md | 44 +++++++++++++++--------- .claude/workflows/deep-review.js | 5 +-- 2 files changed, 30 insertions(+), 19 deletions(-) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index bd3f78deb..966152aea 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 to a clean round. 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, then the prose pass). 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. @@ -308,12 +308,12 @@ 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 zero new issues. +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. **Keep the loop fast. A round is a reading pass over the diff and should cost seconds, so nothing slow belongs between rounds.** Two rules, and both change WHEN work happens, never WHETHER: -- **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 last clean round, and the Definition of done still requires every layer the change touches. This is a timing rule, not permission to skip a layer. -- **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 last clean round. 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. +- **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. +- **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). @@ -321,6 +321,16 @@ The draft PR is already open (step 6), so reviews post to it from the first roun - **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. - **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. + +- **A finding is `substantive` when it touches any of three surfaces:** 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 even though it changes no shipped behavior); or a factual claim about runtime behavior in docs (for example "this value is safe" on a runtime where it is not). Everything else is `prose`: PR-body counts, docstring and comment wording, naming, style, and any nit about the review artifacts themselves. +- **The gate is surface-based on purpose, never importance-based.** Do not substitute a test like "changes shipped behavior" or "affects a user-facing guarantee": importance is judged by the same agent motivated to end the loop, and that exact wording would have ejected the tautological-test findings above, the most valuable findings of the loop that motivated this gate. Surface membership is checkable from the finding itself (which file, what kind of claim). +- **The reviewer classifies, you verify.** Every reviewer tags each finding `substantive` or `prose` against the three surfaces (the tag is part of its expected result shape; the delta prompt template and deep-review's finding schema both carry it). Check each tag against the surface definition and re-tag a mislabeled finding; never assign the tags from scratch yourself, or the motivated-termination problem the gate closes reopens through the classification step. A finding that arrives UNTAGGED is treated as substantive, the direction that keeps the loop open. +- **Prose-tier findings are still findings.** They are posted, and each is fixed, rejected, or filed 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. + 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: @@ -329,10 +339,10 @@ Each round must: 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. - - **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 "last round clean". 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 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 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. - - **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 a round comes back clean. + - **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 stopping point, and even that is reported at the end.** 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. **Working-tree safety (non-negotiable).** A review agent runs against the repository THIS session is using, and every worktree of a repo shares ONE `.git` directory, so its git writes reach the main checkout even from an isolated worktree. A reviewer that runs `git checkout` / `switch` / `reset` / `stash` / `rebase` silently moves the main session's HEAD off the branch (it has happened: a reviewer ran `git checkout main` mid-loop and the local checkout regressed; commits were safe only because they were already pushed). Two defenses: @@ -347,7 +357,7 @@ Each round must: - 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. - 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. Problems only, no suggestions. + - 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. @@ -360,15 +370,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 findings (a deep-review run's CONFIRMED list, or anything a delta verifier reports, 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). 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, 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 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). -4. **If the round reports `CLEAN`** (a deep-review run is clean only when it confirms nothing), the loop is done, with one exception: a clean round 1 still gets the delta-shaped pass the minimum-two-rounds rule requires. Once 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 substantive loop is done, with one exception: a clean round 1 still gets the delta-shaped pass the minimum-two-rounds rule 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. 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. The minimum is TWO rounds: deep-review, then at least one delta verify. A clean deep-review 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 LAST round must be clean, always. A fix is never the end of the loop.** The moment a round surfaces a genuine 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 zero issues. Do NOT report "fixed it" or "ready to merge" after a round that found something, 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. If you ever notice you are about to report a result where the most recent review round found a finding, stop and run another round first. The ONE case that ends the loop without a clean round is 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), and that ends it as BLOCKED, never as done. +**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. The ONE case that ends the loop without converging is 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), and that ends it as BLOCKED, never as done. -**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, fix/reject/file it and run delta rounds until the last one is clean, BEFORE you 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 was clean" is. The user should never have to ask "was the last review clean?" because you should not have stopped until it was. 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: the sole reason to stop short of clean is a reviewer that cannot be produced, which you report as blocked and never paper over with an inline pass of your own. +**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/file 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: the sole reason to stop short of convergence is a reviewer that cannot be produced, which you report as blocked and never paper over with an inline pass of your own. ### When to skip the loop @@ -378,11 +388,11 @@ 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; last round clean. Issues found and fixed during the loop: . Out-of-scope findings filed as follow-ups: . Ready to merge. +> 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 filed as follow-ups: . 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 clean 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, 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. 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: tagged findings or the literal `CLEAN` from a delta verifier, 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". -If you cannot honestly say "last round clean", 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 filed-with-issue-number; if you reported an out-of-scope finding to the user but cannot point to its issue number, 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 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 filed-with-issue-number; if you reported an out-of-scope finding to the user but cannot point to its issue number, 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. @@ -403,7 +413,7 @@ You start with no prior context on this PR. Steps: 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: . -Report findings as a numbered list with file:line references. 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. +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; `prose` for everything else (wording, 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. If you CANNOT review (you could not fetch the diff, you have no access to the repo or PR, the branch does not resolve), say exactly `BLOCKED` on its own line followed by one line naming what you are missing. Do NOT report `CLEAN` in that case: `CLEAN` means you looked and found nothing, and reporting it for a review you could not perform is the single worst outcome here, because it ends the loop on a review that never happened. ``` @@ -461,5 +471,5 @@ A merge updates `main` and npm, but the four in-repo apps deployed to Railway (` - If the TASK worktree's `git status` is dirty at start (a prior session died mid-work in it): stop and ask the user to commit, stash, or abandon that work. Never silently lose changes. A dirty PRIMARY checkout is not a blocker and not yours to fix; the worktree cuts from `origin/main` regardless. - 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's 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 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 its one exit that is not a clean round; 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 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 its one exit other than convergence (a round finding nothing substantive, then the prose pass); 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 6243d4a49..3e4d3aa08 100644 --- a/.claude/workflows/deep-review.js +++ b/.claude/workflows/deep-review.js @@ -47,13 +47,14 @@ const FINDINGS = { type: 'array', items: { type: 'object', - required: ['file', 'line', 'title', 'detail', 'severity'], + required: ['file', 'line', 'title', 'detail', 'severity', 'tier'], properties: { file: { type: 'string', description: 'repo-relative path' }, line: { type: 'number', description: '1-indexed line at the PR head' }, title: { type: 'string', description: 'one-sentence statement of the defect' }, detail: { type: 'string', description: 'concrete failure scenario: inputs/state leading to wrong behavior' }, severity: { type: 'string', enum: ['critical', 'major', 'minor'] }, + tier: { type: 'string', enum: ['substantive', 'prose'], description: "substantive when the finding touches shipped source, a test's ability to observe the defect it claims to cover, or a factual claim about runtime behavior in docs; prose for everything else (wording, counts, style)" }, }, }, }, @@ -158,7 +159,7 @@ log(`deep-review of PR ${pr}${repo ? ` in ${repo}` : ''}: ${ALL_LENSES.length} l const found = await parallel(ALL_LENSES.map((l) => () => agent( - `${SAFETY}\n\nYou are ONE review lens over PR ${pr}. Your single charter:\n${l.prompt}\n\nReport only genuine problems with concrete failure scenarios. No style nits, no suggestions, no padding. Return an empty findings array if you find nothing real.`, + `${SAFETY}\n\nYou are ONE review lens over PR ${pr}. Your single charter:\n${l.prompt}\n\nReport only genuine problems with concrete failure scenarios. No style nits, no suggestions, no padding. Return an empty findings array if you find nothing real.\n\nTag each finding's tier: "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; "prose" for everything else (wording, counts, comment style, review artifacts). The tier is a surface classification, not a severity judgment.`, { label: `find:${l.key}`, phase: 'Find', schema: FINDINGS, ...(l.model ? { model: l.model } : {}) }, ))) From eadc64a01eb629b7ac2609dba03010b203bc38c7 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 29 Jul 2026 19:17:18 +0530 Subject: [PATCH 02/12] docs: close review-found gaps in the gate exit; add its guard test The two-round-minimum carve-out now covers a prose-only round 1, the re-tag license is asymmetric (downgrades recorded on the thread), the round-1 bullet and round-counting rule are tier-consistent, and an unproducible prose-pass reviewer ends the loop BLOCKED. The skill router's injected review directive states the new exit instead of the old clean-round absolute it still carried. Both reviewer-facing prompts qualify counts (a number in docs stating runtime behavior is surface three). test/hooks/review-loop-exit.test.mjs pins the load-bearing anchors in all three files so a revert or typo-drift reds CI. --- .claude/hooks/route-skills.sh | 8 ++- .claude/skills/webjs-start-work/SKILL.md | 14 ++--- .claude/workflows/deep-review.js | 2 +- test/hooks/review-loop-exit.test.mjs | 68 ++++++++++++++++++++++++ 4 files changed, 82 insertions(+), 10 deletions(-) create mode 100644 test/hooks/review-loop-exit.test.mjs diff --git a/.claude/hooks/route-skills.sh b/.claude/hooks/route-skills.sh index 19b7d665d..2bceba707 100755 --- a/.claude/hooks/route-skills.sh +++ b/.claude/hooks/route-skills.sh @@ -177,13 +177,17 @@ fi # --- code-review: review the diff before a PR is ready ------------------ # 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 (re-review until a round is clean). +# 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 +# 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 # portability test that guards project skills does not cover it). 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). Treat review as a LOOP: after fixing findings, re-review until a round is clean. Never report done off a round that found something." + 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). Treat review as a LOOP with the webjs-start-work 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. Never report done off a round that found something substantive. Findings this skill reports carry no tier tag, so classify them against those three surfaces yourself, treating a doubtful one 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 966152aea..89fcdb812 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -317,7 +317,7 @@ The draft PR is already open (step 6), so reviews post to it from the first roun **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). -- **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 budget cannot verify comes back in an `unverified` list, never silently dropped, so raise the cap or re-run when that list is non-empty. 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, and its confirmed findings feed this same loop (fix, post as one review object, delta round). 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. +- **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 budget cannot verify comes back in an `unverified` list, never silently dropped, so raise the cap or re-run when that list is non-empty. 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 into the prose pass per the substantive gate and steps 3 and 4. 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. - **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. @@ -325,7 +325,7 @@ The draft PR is already open (step 6), so reviews post to it from the first roun - **A finding is `substantive` when it touches any of three surfaces:** 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 even though it changes no shipped behavior); or a factual claim about runtime behavior in docs (for example "this value is safe" on a runtime where it is not). Everything else is `prose`: PR-body counts, docstring and comment wording, naming, style, and any nit about the review artifacts themselves. - **The gate is surface-based on purpose, never importance-based.** Do not substitute a test like "changes shipped behavior" or "affects a user-facing guarantee": importance is judged by the same agent motivated to end the loop, and that exact wording would have ejected the tautological-test findings above, the most valuable findings of the loop that motivated this gate. Surface membership is checkable from the finding itself (which file, what kind of claim). -- **The reviewer classifies, you verify.** Every reviewer tags each finding `substantive` or `prose` against the three surfaces (the tag is part of its expected result shape; the delta prompt template and deep-review's finding schema both carry it). Check each tag against the surface definition and re-tag a mislabeled finding; never assign the tags from scratch yourself, or the motivated-termination problem the gate closes reopens through the classification step. A finding that arrives UNTAGGED is treated as substantive, the direction that keeps the loop open. +- **The reviewer classifies, you verify.** Every reviewer tags each finding `substantive` or `prose` against the three surfaces (the tag is part of its expected result shape; the delta prompt template and deep-review's finding schema both carry it). Check each tag against the surface definition and re-tag a mislabeled finding; never assign the tags from scratch yourself, or the motivated-termination problem the gate closes reopens through the classification step. The license is asymmetric: an UPGRADE to substantive is always yours to make, while a DOWNGRADE to prose is treated like a rejection, made only when the finding plainly matches the prose enumeration and recorded on the finding's thread with a one-sentence reason, so the ejection is auditable. A finding that arrives UNTAGGED is treated as substantive, the direction that keeps the loop open. - **Prose-tier findings are still findings.** They are posted, and each is fixed, rejected, or filed 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. @@ -372,9 +372,9 @@ Each round must: 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). -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 clean round 1 still gets the delta-shaped pass the minimum-two-rounds rule 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. 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 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 minimum-two-rounds rule 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. -The minimum is TWO rounds: deep-review, then at least one delta verify. A clean deep-review 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 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 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. The ONE case that ends the loop without converging is 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), and that ends it as BLOCKED, never as done. @@ -390,7 +390,7 @@ After the loop converges and the deferred suites plus the CI read (step 4) are d > 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 filed as follow-ups: . 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. 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: tagged findings or the literal `CLEAN` from a delta verifier, 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. 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". 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 filed-with-issue-number; if you reported an out-of-scope finding to the user but cannot point to its issue number, 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. @@ -413,7 +413,7 @@ You start with no prior context on this PR. Steps: 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: . -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; `prose` for everything else (wording, 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. +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. If you CANNOT review (you could not fetch the diff, you have no access to the repo or PR, the branch does not resolve), say exactly `BLOCKED` on its own line followed by one line naming what you are missing. Do NOT report `CLEAN` in that case: `CLEAN` means you looked and found nothing, and reporting it for a review you could not perform is the single worst outcome here, because it ends the loop on a review that never happened. ``` @@ -471,5 +471,5 @@ A merge updates `main` and npm, but the four in-repo apps deployed to Railway (` - If the TASK worktree's `git status` is dirty at start (a prior session died mid-work in it): stop and ask the user to commit, stash, or abandon that work. Never silently lose changes. A dirty PRIMARY checkout is not a blocker and not yours to fix; the worktree cuts from `origin/main` regardless. - 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's 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 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 its one exit other than convergence (a round finding nothing substantive, then the prose pass); 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, 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 its one exit other than convergence (a round finding nothing substantive, then the prose pass); 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 3e4d3aa08..0783556a3 100644 --- a/.claude/workflows/deep-review.js +++ b/.claude/workflows/deep-review.js @@ -159,7 +159,7 @@ log(`deep-review of PR ${pr}${repo ? ` in ${repo}` : ''}: ${ALL_LENSES.length} l const found = await parallel(ALL_LENSES.map((l) => () => agent( - `${SAFETY}\n\nYou are ONE review lens over PR ${pr}. Your single charter:\n${l.prompt}\n\nReport only genuine problems with concrete failure scenarios. No style nits, no suggestions, no padding. Return an empty findings array if you find nothing real.\n\nTag each finding's tier: "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; "prose" for everything else (wording, counts, comment style, review artifacts). The tier is a surface classification, not a severity judgment.`, + `${SAFETY}\n\nYou are ONE review lens over PR ${pr}. Your single charter:\n${l.prompt}\n\nReport only genuine problems with concrete failure scenarios. No style nits, no suggestions, no padding. Return an empty findings array if you find nothing real.\n\nTag each finding's tier: "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). The tier is a surface classification, not a severity judgment.`, { label: `find:${l.key}`, phase: 'Find', schema: FINDINGS, ...(l.model ? { model: l.model } : {}) }, ))) diff --git a/test/hooks/review-loop-exit.test.mjs b/test/hooks/review-loop-exit.test.mjs new file mode 100644 index 000000000..75699b5aa --- /dev/null +++ b/test/hooks/review-loop-exit.test.mjs @@ -0,0 +1,68 @@ +// Guards the review loop's substantive-gate exit (#1171) across the three +// files that carry it: the webjs-start-work skill (the normative rules), +// the deep-review workflow (the tier tag round-1 findings arrive with), +// and the skill-routing hook (the injected review directive, which once +// kept the OLD "until a round is clean" absolute after the skill had moved +// on, steering standalone reviews back into prose relitigation). +// +// These are static assertions over committed text on purpose: the gate is +// prose executed by agents, so the counterfactual for "the gate silently +// reverted" is the text no longer carrying its load-bearing anchors. Each +// assertion fails if its hunk is reverted or typo-drifted. + +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, resolve } from 'node:path'; + +const here = dirname(fileURLToPath(import.meta.url)); +const skill = readFileSync(resolve(here, '../../.claude/skills/webjs-start-work/SKILL.md'), 'utf8'); +const workflow = readFileSync(resolve(here, '../../.claude/workflows/deep-review.js'), 'utf8'); +const hook = readFileSync(resolve(here, '../../.claude/hooks/route-skills.sh'), 'utf8'); + +test('the skill carries the substantive gate and its two-tier exit', () => { + assert.match(skill, /The substantive gate: which findings keep the loop open/); + // The three surfaces, verbatim anchors of the definition. + assert.match(skill, /ability to OBSERVE the defect it claims to cover/); + assert.match(skill, /factual claim about runtime behavior in docs/); + // Fail-open direction for untagged findings. + assert.match(skill, /arrives UNTAGGED is treated as substantive/); + // The bounded prose pass and its non-round status. + assert.match(skill, /never counts toward the round total or the two-round minimum/); + // 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. + assert.match(skill, /whether clean or prose-only, still gets the delta-shaped pass/); + // An unproducible prose-pass reviewer is BLOCKED, not converged. + assert.match(skill, /prose-pass reviewer that cannot be produced at all ends the loop BLOCKED/); +}); + +test('deep-review findings carry a required tier with the gate wording', () => { + assert.match(workflow, /required: \['file', 'line', 'title', 'detail', 'severity', 'tier'\]/); + assert.match(workflow, /tier: \{ type: 'string', enum: \['substantive', 'prose'\]/); + // The finder prompt states the same three-surface definition. + assert.match(workflow, /Tag each finding's tier/); + assert.match(workflow, /ability to OBSERVE the defect it claims to cover/); + // The tier is orthogonal to severity, stated so dedup changes stay honest. + assert.match(workflow, /surface classification, not a severity judgment/); +}); + +test('deep-review.js stays valid in the async workflow context', () => { + // Workflow scripts run as an async function body (top-level return and + // await are legal there but not in a plain module, so `node --check` + // cannot cover this file). + const body = workflow.replace(/^export /m, ''); + const AsyncFunction = Object.getPrototypeOf(async function () {}).constructor; + assert.doesNotThrow(() => new AsyncFunction('args', 'agent', 'parallel', 'pipeline', 'phase', 'log', 'budget', 'workflow', body)); +}); + +test('the routed review directive states the substantive exit, not the old absolute', () => { + // The old exit steered standalone reviews into never-terminating prose + // 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.match(hook, /found something substantive/); +}); From 58c690a70994631bd8649c4a51f6e1c24c0639dc Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 29 Jul 2026 19:29:43 +0530 Subject: [PATCH 03/12] docs: keep the router directive and dedup in lockstep with the gate The gate now defines the one reviewer kind that structurally cannot tag (a fixed result schema, the built-in code-review shape): the orchestrator classifies it alone, doubt resolving to substantive and prose classifications recorded like downgrades, and the router directive states that instead of a freestanding self-classify license. Deep-review dedup prefers a substantive finding at a file:line collision so a prose finding can never flip the exit signal. The round-1 bullet no longer reads as skipping the mandatory delta pass. The guard test anchors the definition bullet uniquely (its shared phrases also live in the delta template, so alone they were vacuous) and pins the previously unpinned hunks. --- .claude/hooks/route-skills.sh | 2 +- .claude/skills/webjs-start-work/SKILL.md | 4 +-- .claude/workflows/deep-review.js | 6 +++- test/hooks/review-loop-exit.test.mjs | 40 +++++++++++++++++++----- 4 files changed, 40 insertions(+), 12 deletions(-) diff --git a/.claude/hooks/route-skills.sh b/.claude/hooks/route-skills.sh index 2bceba707..43e20656b 100755 --- a/.claude/hooks/route-skills.sh +++ b/.claude/hooks/route-skills.sh @@ -187,7 +187,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). Treat review as a LOOP with the webjs-start-work 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. Never report done off a round that found something substantive. Findings this skill reports carry no tier tag, so classify them against those three surfaces yourself, treating a doubtful one 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). Treat review as a LOOP with the webjs-start-work 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. Never report done off a round that found something substantive. Findings this skill reports carry no tier tag (a fixed result schema), the one reviewer kind the gate has 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." 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 89fcdb812..e19769f76 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -317,7 +317,7 @@ The draft PR is already open (step 6), so reviews post to it from the first roun **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). -- **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 budget cannot verify comes back in an `unverified` list, never silently dropped, so raise the cap or re-run when that list is non-empty. 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 into the prose pass per the substantive gate and steps 3 and 4. 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. +- **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 budget cannot verify comes back in an `unverified` list, never silently dropped, so raise the cap or re-run when that list is non-empty. 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. - **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. @@ -325,7 +325,7 @@ The draft PR is already open (step 6), so reviews post to it from the first roun - **A finding is `substantive` when it touches any of three surfaces:** 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 even though it changes no shipped behavior); or a factual claim about runtime behavior in docs (for example "this value is safe" on a runtime where it is not). Everything else is `prose`: PR-body counts, docstring and comment wording, naming, style, and any nit about the review artifacts themselves. - **The gate is surface-based on purpose, never importance-based.** Do not substitute a test like "changes shipped behavior" or "affects a user-facing guarantee": importance is judged by the same agent motivated to end the loop, and that exact wording would have ejected the tautological-test findings above, the most valuable findings of the loop that motivated this gate. Surface membership is checkable from the finding itself (which file, what kind of claim). -- **The reviewer classifies, you verify.** Every reviewer tags each finding `substantive` or `prose` against the three surfaces (the tag is part of its expected result shape; the delta prompt template and deep-review's finding schema both carry it). Check each tag against the surface definition and re-tag a mislabeled finding; never assign the tags from scratch yourself, or the motivated-termination problem the gate closes reopens through the classification step. The license is asymmetric: an UPGRADE to substantive is always yours to make, while a DOWNGRADE to prose is treated like a rejection, made only when the finding plainly matches the prose enumeration and recorded on the finding's thread with a one-sentence reason, so the ejection is auditable. A finding that arrives UNTAGGED is treated as substantive, the direction that keeps the loop open. +- **The reviewer classifies, you verify.** Every reviewer tags each finding `substantive` or `prose` against the three surfaces (the tag is part of its expected result shape; the delta prompt template and deep-review's finding schema both carry it). Check each tag against the surface definition and re-tag a mislabeled finding; never assign the tags from scratch yourself, or the motivated-termination problem the gate closes reopens through the classification step. The license is asymmetric: an UPGRADE to substantive is always yours to make, while a DOWNGRADE to prose is treated like a rejection, made only when the finding plainly matches the prose enumeration and recorded on the finding's thread with a one-sentence reason, so the ejection is auditable. A finding that arrives UNTAGGED from a reviewer whose result shape carries the tag is treated as substantive, the direction that keeps the loop open. One reviewer kind structurally cannot tag (a fixed result schema with no tier field, the built-in code-review skill's shape): for that reviewer alone you classify, under the same asymmetry, doubt resolving to substantive and every prose classification recorded like a downgrade. - **Prose-tier findings are still findings.** They are posted, and each is fixed, rejected, or filed 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. diff --git a/.claude/workflows/deep-review.js b/.claude/workflows/deep-review.js index 0783556a3..c348dcf3e 100644 --- a/.claude/workflows/deep-review.js +++ b/.claude/workflows/deep-review.js @@ -168,10 +168,14 @@ const found = await parallel(ALL_LENSES.map((l) => () => const all = found.filter(Boolean).flatMap((r) => r.findings) const byKey = new Map() const rank = { critical: 0, major: 1, minor: 2 } +// Tier outranks severity at a file:line collision: the tier decides the +// loop's exit, so a prose finding must never evict a substantive one at +// the same line. Severity orders within a tier. +const subst = (f) => f.tier === 'substantive' for (const f of all) { const key = `${f.file}:${f.line}` const prev = byKey.get(key) - if (!prev || rank[f.severity] < rank[prev.severity]) byKey.set(key, f) + if (!prev || (subst(f) && !subst(prev)) || (subst(f) === subst(prev) && rank[f.severity] < rank[prev.severity])) byKey.set(key, f) } const deduped = [...byKey.values()].sort((a, b) => rank[a.severity] - rank[b.severity]) const CAP = 12 diff --git a/test/hooks/review-loop-exit.test.mjs b/test/hooks/review-loop-exit.test.mjs index 75699b5aa..d7966ff58 100644 --- a/test/hooks/review-loop-exit.test.mjs +++ b/test/hooks/review-loop-exit.test.mjs @@ -23,30 +23,51 @@ const hook = readFileSync(resolve(here, '../../.claude/hooks/route-skills.sh'), test('the skill carries the substantive gate and its two-tier exit', () => { assert.match(skill, /The substantive gate: which findings keep the loop open/); - // The three surfaces, verbatim anchors of the definition. - assert.match(skill, /ability to OBSERVE the defect it claims to cover/); - assert.match(skill, /factual claim about runtime behavior in docs/); - // Fail-open direction for untagged findings. - assert.match(skill, /arrives UNTAGGED is treated as substantive/); + // The normative definition bullet, anchored by a phrase unique to it (the + // shared three-surface wording below also appears in the delta prompt + // template, so alone it would pass with the definition bullet deleted). + assert.match(skill, /A finding is `substantive` when it touches any of three surfaces/); + // The three surfaces appear in BOTH the definition and the delta prompt + // template, so each shared phrase must occur at least twice. + for (const phrase of [/ability to OBSERVE the defect it claims to cover/g, /factual claim about runtime behavior in docs/g]) { + assert.ok((skill.match(phrase) || []).length >= 2, `${phrase} must anchor both the definition and the template`); + } + // A number in docs stating runtime behavior is surface three, not prose. + assert.match(skill, /a number in docs that states runtime behavior/); + // Fail-open direction for untagged findings from a tag-capable reviewer, + // 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 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. + // 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/); - // An unproducible prose-pass reviewer is BLOCKED, not converged. + assert.match(skill, /still gets the delta-shaped pass the two-round minimum requires before the prose pass/); + // 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/); }); test('deep-review findings carry a required tier with the gate wording', () => { assert.match(workflow, /required: \['file', 'line', 'title', 'detail', 'severity', 'tier'\]/); assert.match(workflow, /tier: \{ type: 'string', enum: \['substantive', 'prose'\]/); - // The finder prompt states the same three-surface definition. + // The finder prompt states the same three-surface definition, including + // the docs-number qualifier. assert.match(workflow, /Tag each finding's tier/); assert.match(workflow, /ability to OBSERVE the defect it claims to cover/); + 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/); + // Dedup must never let a prose finding evict a substantive one at the + // same file:line (the tier decides the loop's exit). + assert.match(workflow, /subst\(f\) && !subst\(prev\)/); }); test('deep-review.js stays valid in the async workflow context', () => { @@ -65,4 +86,7 @@ test('the routed review directive states the substantive exit, not the old absol assert.match(hook, /nothing SUBSTANTIVE/); assert.match(hook, /prose pass/); 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/); }); From 12e6b26c372a440e67dfce88d6e14dd8f1eee4db Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 29 Jul 2026 19:39:38 +0530 Subject: [PATCH 04/12] docs: extend tier precedence to the CAP slice and jury walk Tier now outranks severity everywhere findings compete, not only at the same-line dedup collision: the sort that feeds the CAP slice and the jury-budget walk is tier-first, the CAP tail is returned in unverified instead of only logged, and a missing tier fails open to substantive. The router directive names the same round-1 reviewer as the skill (the deep-review workflow, code-review findings as auxiliary input), and the docs-number qualifier now lives in the normative definition the verify and downgrade steps audit against, not only in the reviewer prompts. --- .claude/hooks/route-skills.sh | 2 +- .claude/skills/webjs-start-work/SKILL.md | 2 +- .claude/workflows/deep-review.js | 18 +++++++++++------- test/hooks/review-loop-exit.test.mjs | 18 ++++++++++++++---- 4 files changed, 27 insertions(+), 13 deletions(-) diff --git a/.claude/hooks/route-skills.sh b/.claude/hooks/route-skills.sh index 43e20656b..670f56ac2 100755 --- a/.claude/hooks/route-skills.sh +++ b/.claude/hooks/route-skills.sh @@ -187,7 +187,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). Treat review as a LOOP with the webjs-start-work 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. Never report done off a round that found something substantive. Findings this skill reports carry no tier tag (a fixed result schema), the one reviewer kind the gate has 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." + 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. Never report done off a round that found something substantive. Findings arriving without a tier tag (a fixed result schema, the code-review shape) are the one reviewer kind 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." 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 e19769f76..48705ea31 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -323,7 +323,7 @@ The draft PR is already open (step 6), so reviews post to it from the first roun **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. -- **A finding is `substantive` when it touches any of three surfaces:** 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 even though it changes no shipped behavior); or a factual claim about runtime behavior in docs (for example "this value is safe" on a runtime where it is not). Everything else is `prose`: PR-body counts, docstring and comment wording, naming, style, and any nit about the review artifacts themselves. +- **A finding is `substantive` when it touches any of three surfaces:** 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 even though it changes no shipped behavior); or a factual claim about runtime behavior in docs (for example "this value is safe" on a runtime where it is not; a number in docs that states runtime behavior, a default, a limit, a condition list, is this surface too). Everything else is `prose`: PR-body counts, docstring and comment wording, naming, style, and any nit about the review artifacts themselves. - **The gate is surface-based on purpose, never importance-based.** Do not substitute a test like "changes shipped behavior" or "affects a user-facing guarantee": importance is judged by the same agent motivated to end the loop, and that exact wording would have ejected the tautological-test findings above, the most valuable findings of the loop that motivated this gate. Surface membership is checkable from the finding itself (which file, what kind of claim). - **The reviewer classifies, you verify.** Every reviewer tags each finding `substantive` or `prose` against the three surfaces (the tag is part of its expected result shape; the delta prompt template and deep-review's finding schema both carry it). Check each tag against the surface definition and re-tag a mislabeled finding; never assign the tags from scratch yourself, or the motivated-termination problem the gate closes reopens through the classification step. The license is asymmetric: an UPGRADE to substantive is always yours to make, while a DOWNGRADE to prose is treated like a rejection, made only when the finding plainly matches the prose enumeration and recorded on the finding's thread with a one-sentence reason, so the ejection is auditable. A finding that arrives UNTAGGED from a reviewer whose result shape carries the tag is treated as substantive, the direction that keeps the loop open. One reviewer kind structurally cannot tag (a fixed result schema with no tier field, the built-in code-review skill's shape): for that reviewer alone you classify, under the same asymmetry, doubt resolving to substantive and every prose classification recorded like a downgrade. - **Prose-tier findings are still findings.** They are posted, and each is fixed, rejected, or filed 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. diff --git a/.claude/workflows/deep-review.js b/.claude/workflows/deep-review.js index c348dcf3e..c0cbc4c3e 100644 --- a/.claude/workflows/deep-review.js +++ b/.claude/workflows/deep-review.js @@ -168,19 +168,23 @@ const found = await parallel(ALL_LENSES.map((l) => () => const all = found.filter(Boolean).flatMap((r) => r.findings) const byKey = new Map() const rank = { critical: 0, major: 1, minor: 2 } -// Tier outranks severity at a file:line collision: the tier decides the -// loop's exit, so a prose finding must never evict a substantive one at -// the same line. Severity orders within a tier. -const subst = (f) => f.tier === 'substantive' +// Tier outranks severity EVERYWHERE findings compete: the tier decides +// the loop's exit, so a prose finding must never evict a substantive one, +// at a same-line dedup collision, in the CAP slice, or in the jury-budget +// walk (the latter two follow this sort). Severity orders within a tier. +// A missing tier counts as substantive, the gate's fail-open direction. +const subst = (f) => f.tier !== 'prose' for (const f of all) { const key = `${f.file}:${f.line}` const prev = byKey.get(key) if (!prev || (subst(f) && !subst(prev)) || (subst(f) === subst(prev) && rank[f.severity] < rank[prev.severity])) byKey.set(key, f) } -const deduped = [...byKey.values()].sort((a, b) => rank[a.severity] - rank[b.severity]) +const deduped = [...byKey.values()].sort((a, b) => (Number(subst(b)) - Number(subst(a))) || (rank[a.severity] - rank[b.severity])) const CAP = 12 const toVerify = deduped.slice(0, CAP) -if (deduped.length > CAP) log(`capping verification at ${CAP} of ${deduped.length} deduped findings (dropped ${deduped.length - CAP} lowest-severity; re-run after fixes to catch them)`) +// The CAP tail is returned in `unverified`, never silently dropped. +const capDropped = deduped.slice(CAP) +if (capDropped.length) log(`capping verification at ${CAP} of ${deduped.length} deduped findings (${capDropped.length} returned unverified; raise maxAgents or re-run after fixes)`) log(`${all.length} raw findings, ${deduped.length} after dedup, verifying ${toVerify.length}`) if (toVerify.length === 0) return { pr, repo, confirmed: [], rejected: [], unverified: [], stats: { raw: all.length, deduped: deduped.length, verified: 0, lenses: ALL_LENSES.length, dynamic: DYNAMIC.map((l) => l.key), maxAgents: MAX_AGENTS, fable: ALL_LENSES.filter((l) => l.model === 'fable').length }, note: 'no findings survived dedup; treat as a clean deep pass' } @@ -231,7 +235,7 @@ return { repo, confirmed, rejected, - unverified, + unverified: [...unverified, ...capDropped], stats: { raw: all.length, deduped: deduped.length, verified: allocated.length, lenses: ALL_LENSES.length, dynamic: DYNAMIC.map((l) => l.key), maxAgents: MAX_AGENTS, fable: ALL_LENSES.filter((l) => l.model === 'fable').length }, note: 'Confirmed findings feed the normal review loop: fix, post as one review object, then a delta round. Rejected ones carry their refuters\' reasons for the audit trail.', } diff --git a/test/hooks/review-loop-exit.test.mjs b/test/hooks/review-loop-exit.test.mjs index d7966ff58..e7da6a1e1 100644 --- a/test/hooks/review-loop-exit.test.mjs +++ b/test/hooks/review-loop-exit.test.mjs @@ -32,8 +32,9 @@ test('the skill carries the substantive gate and its two-tier exit', () => { for (const phrase of [/ability to OBSERVE the defect it claims to cover/g, /factual claim about runtime behavior in docs/g]) { assert.ok((skill.match(phrase) || []).length >= 2, `${phrase} must anchor both the definition and the template`); } - // A number in docs stating runtime behavior is surface three, not prose. - assert.match(skill, /a number in docs that states runtime behavior/); + // A number in docs stating runtime behavior is surface three, not prose, + // in the normative definition AND the delta prompt template. + assert.ok((skill.match(/a number in docs that states runtime behavior/g) || []).length >= 2, 'docs-number qualifier must anchor both the definition and the template'); // Fail-open direction for untagged findings from a tag-capable reviewer, // 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/); @@ -65,9 +66,14 @@ 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/); - // Dedup must never let a prose finding evict a substantive one at the - // same file:line (the tier decides the loop's exit). + // 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). assert.match(workflow, /subst\(f\) && !subst\(prev\)/); + assert.match(workflow, /const subst = \(f\) => f\.tier !== 'prose'/); + assert.match(workflow, /Number\(subst\(b\)\) - Number\(subst\(a\)\)/); + // The CAP tail is returned in unverified, never silently dropped. + assert.match(workflow, /\.\.\.unverified, \.\.\.capDropped/); }); test('deep-review.js stays valid in the async workflow context', () => { @@ -89,4 +95,8 @@ test('the routed review directive states the substantive exit, not the old absol // 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/); + assert.match(hook, /auxiliary input/); }); From a6d735807c0df5e15bd4c9667228960f7d4ea02e Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 29 Jul 2026 19:47:41 +0530 Subject: [PATCH 05/12] docs: state the CAP remedy truthfully; fail a dead jury slot open The verification cap is a fixed 12, so its unverified tail is re-run-recoverable only; the log line, header comment, and the skill's round-1 bullet now say which remedy fits which cause instead of pointing every shortfall at maxAgents. A finding whose whole jury task resolves falsy now fails open to CONFIRMED like the empty jury, so every allocated finding lands in a returned list. The jury-walk comment matches the tier-first sort, and the router directive scopes self-classification to the code-review skill's own findings. --- .claude/hooks/route-skills.sh | 2 +- .claude/skills/webjs-start-work/SKILL.md | 2 +- .claude/workflows/deep-review.js | 20 +++++++++++++------- test/hooks/review-loop-exit.test.mjs | 12 +++++++++++- 4 files changed, 26 insertions(+), 10 deletions(-) diff --git a/.claude/hooks/route-skills.sh b/.claude/hooks/route-skills.sh index 670f56ac2..1ccca72c0 100755 --- a/.claude/hooks/route-skills.sh +++ b/.claude/hooks/route-skills.sh @@ -187,7 +187,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. Never report done off a round that found something substantive. Findings arriving without a tier tag (a fixed result schema, the code-review shape) are the one reviewer kind 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." + 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. 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), the one reviewer kind 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 48705ea31..6afacccdf 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -317,7 +317,7 @@ The draft PR is already open (step 6), so reviews post to it from the first roun **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). -- **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 budget cannot verify comes back in an `unverified` list, never silently dropped, so raise the cap or re-run when that list is non-empty. 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. +- **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. - **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. diff --git a/.claude/workflows/deep-review.js b/.claude/workflows/deep-review.js index c0cbc4c3e..a387cce7a 100644 --- a/.claude/workflows/deep-review.js +++ b/.claude/workflows/deep-review.js @@ -27,8 +27,10 @@ else if (typeof args === 'string' && args.trim()) { const givenLenses = (args && typeof args === 'object' && Array.isArray(args.lenses)) ? args.lenses : null // Hard agent budget for the WHOLE run (scout + finders + jurors). Degrades // gracefully: dynamic lenses are trimmed first, jury sizes shrink next, and a -// finding the budget cannot verify is returned marked unverified, never -// silently dropped. +// finding the run cannot verify is returned marked unverified, never +// 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 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 }') @@ -184,7 +186,7 @@ const CAP = 12 const toVerify = deduped.slice(0, CAP) // The CAP tail is returned in `unverified`, never silently dropped. const capDropped = deduped.slice(CAP) -if (capDropped.length) log(`capping verification at ${CAP} of ${deduped.length} deduped findings (${capDropped.length} returned unverified; raise maxAgents or re-run after fixes)`) +if (capDropped.length) log(`capping verification at ${CAP} of ${deduped.length} deduped findings (${capDropped.length} returned unverified; the cap is fixed, so re-run after fixes to catch them)`) log(`${all.length} raw findings, ${deduped.length} after dedup, verifying ${toVerify.length}`) if (toVerify.length === 0) return { pr, repo, confirmed: [], rejected: [], unverified: [], stats: { raw: all.length, deduped: deduped.length, verified: 0, lenses: ALL_LENSES.length, dynamic: DYNAMIC.map((l) => l.key), maxAgents: MAX_AGENTS, fable: ALL_LENSES.filter((l) => l.model === 'fable').length }, note: 'no findings survived dedup; treat as a clean deep pass' } @@ -195,9 +197,11 @@ phase('Verify') // on a major finding survives (fail-open toward treating it as real). const juries = { critical: 3, major: 2, minor: 1 } -// Allocate jurors within the remaining budget, severity-first (toVerify is -// already severity-sorted). A finding allocated zero jurors is returned -// UNVERIFIED rather than silently dropped. +// Allocate jurors within the remaining budget in toVerify's order, which +// is tier-first with severity within a tier (the sort above), so +// substantive findings claim jurors before prose ones. A finding +// allocated zero jurors is returned UNVERIFIED rather than silently +// dropped. spent += ALL_LENSES.length let juryBudget = Math.max(0, MAX_AGENTS - spent) const allocated = [] @@ -225,7 +229,9 @@ const verified = await parallel(allocated.map(({ f, take }) => () => return { ...f, confirmed: !dead, jury: cast.length, refutes, reasons: cast.map((v) => v.reason) } }))) -const results = verified.filter(Boolean) +// A dead jury SLOT (the whole parallel task resolved falsy) gets the same +// fail-open the empty jury does: the finding stays CONFIRMED, never lost. +const results = allocated.map(({ f }, i) => verified[i] || { ...f, confirmed: true, jury: 0, refutes: 0, reasons: [] }) const confirmed = results.filter((r) => r.confirmed) const rejected = results.filter((r) => !r.confirmed) log(`confirmed ${confirmed.length}, refuted ${rejected.length}`) diff --git a/test/hooks/review-loop-exit.test.mjs b/test/hooks/review-loop-exit.test.mjs index e7da6a1e1..56f3ab4c8 100644 --- a/test/hooks/review-loop-exit.test.mjs +++ b/test/hooks/review-loop-exit.test.mjs @@ -72,8 +72,14 @@ test('deep-review findings carry a required tier with the gate wording', () => { assert.match(workflow, /subst\(f\) && !subst\(prev\)/); assert.match(workflow, /const subst = \(f\) => f\.tier !== 'prose'/); assert.match(workflow, /Number\(subst\(b\)\) - Number\(subst\(a\)\)/); - // The CAP tail is returned in unverified, never silently dropped. + // The CAP tail is returned in unverified, never silently dropped, and + // its remedy is stated correctly (the cap is fixed; maxAgents cannot + // recover it). assert.match(workflow, /\.\.\.unverified, \.\.\.capDropped/); + assert.match(workflow, /the cap is fixed, so re-run after fixes/); + assert.match(skill, /cannot recover a capped finding/); + // A dead jury slot fails open to CONFIRMED instead of vanishing. + assert.match(workflow, /verified\[i\] \|\| \{ \.\.\.f, confirmed: true, jury: 0/); }); test('deep-review.js stays valid in the async workflow context', () => { @@ -99,4 +105,8 @@ test('the routed review directive states the substantive exit, not the old absol // deep-review workflow, with code-review findings as auxiliary input. assert.match(hook, /round 1 is the deep-review workflow/); 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. + assert.match(hook, /from the code-review skill itself/); + assert.match(hook, /any tag-capable reviewer is simply treated as substantive/); }); From 48ac4d988d41fc9b53022564507ac4b7dfd163e7 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 29 Jul 2026 19:58:00 +0530 Subject: [PATCH 06/12] docs: apply the prose pass The router directive's appositive attaches the reviewer kind to the skill rather than its findings, step 4 uses the same name for the two-round minimum as its other mentions, and the guard test's CAP comment says which file its third assertion targets. --- .claude/hooks/route-skills.sh | 2 +- .claude/skills/webjs-start-work/SKILL.md | 2 +- test/hooks/review-loop-exit.test.mjs | 5 +++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/.claude/hooks/route-skills.sh b/.claude/hooks/route-skills.sh index 1ccca72c0..033de2271 100755 --- a/.claude/hooks/route-skills.sh +++ b/.claude/hooks/route-skills.sh @@ -187,7 +187,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. 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), the one reviewer kind 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; 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. 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 6afacccdf..5b7388d7f 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -372,7 +372,7 @@ Each round must: 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). -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 minimum-two-rounds rule 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 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. 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. diff --git a/test/hooks/review-loop-exit.test.mjs b/test/hooks/review-loop-exit.test.mjs index 56f3ab4c8..530306588 100644 --- a/test/hooks/review-loop-exit.test.mjs +++ b/test/hooks/review-loop-exit.test.mjs @@ -73,8 +73,9 @@ test('deep-review findings carry a required tier with the gate wording', () => { assert.match(workflow, /const subst = \(f\) => f\.tier !== 'prose'/); assert.match(workflow, /Number\(subst\(b\)\) - Number\(subst\(a\)\)/); // The CAP tail is returned in unverified, never silently dropped, and - // its remedy is stated correctly (the cap is fixed; maxAgents cannot - // recover it). + // its remedy is stated correctly in both the workflow log and (third + // assertion, against the SKILL file) the skill's round-1 bullet: the + // cap is fixed, maxAgents cannot recover it. assert.match(workflow, /\.\.\.unverified, \.\.\.capDropped/); assert.match(workflow, /the cap is fixed, so re-run after fixes/); assert.match(skill, /cannot recover a capped finding/); From 51abd5096e7599aac727b46a169818e6cbca676d Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 29 Jul 2026 20:18:55 +0530 Subject: [PATCH 07/12] docs: fold in #1176, budget the loop and stop auto-filing follow-ups Five substantive rounds bound the loop by default (OVER BUDGET is a third exit with its own report, next to converged and BLOCKED), a mid-loop fix that changes shipped behavior runs the same red/green counterfactual toggle as the original change (an unproven fix once made an older test non-discriminating and bought two extra rounds), counterfactual claims are dated to the commit they were proven at, and the loop defers out-of-scope findings (thread record + report line) instead of filing issues, since the owner pays for every filed issue and one 18-round loop filed five. Folded from #1176 at the owner's direction rather than left as a follow-up, the #1159 precedent. --- .claude/hooks/route-skills.sh | 2 +- .claude/skills/webjs-start-work/SKILL.md | 17 ++++++++++++----- test/hooks/review-loop-exit.test.mjs | 18 ++++++++++++++++++ 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/.claude/hooks/route-skills.sh b/.claude/hooks/route-skills.sh index 033de2271..f01bbdbf7 100755 --- a/.claude/hooks/route-skills.sh +++ b/.claude/hooks/route-skills.sh @@ -187,7 +187,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. 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; 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." 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 5b7388d7f..455b62e5a 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -123,6 +123,8 @@ Doc drift is the #1 way a framework rots. Documentation MUST stay in sync with c - **Do NOT `git checkout ` to "undo" a counterfactual while the fix is still uncommitted.** `git checkout` restores the file to HEAD, which (pre-commit) has NO fix, so it silently throws the whole fix away, not just the temporary neutering. Commit first; then `git checkout ` restores the COMMITTED fix, which is what you want. - **Do NOT neuter a guard by `sed`-rewriting the source to a sentinel like `''`.** Shell-quoted escapes land as a literal control byte (a NUL/0x01) inside the file, which renders like a space in an editor but breaks the comparison and makes `grep` treat the file as binary (silent empty matches). Verify any byte-level edit with `od -c` on the changed line and `tr -d '\000' | wc -c` for stray NULs. Prefer the Edit tool (toggle the guard, run, toggle back) or `git stash`/`git stash pop` of the committed source over `sed` for this. The clean loop: commit fix+test, run test green, `git stash push -- ` (or Edit out the guard), run test red, `git stash pop` (or Edit the guard back), run test green again. The test having gone red in the middle is the proof. + + **A counterfactual CLAIM decays.** "Reverting X reds Y" is true of a commit, not a branch: a later commit touching the same mechanism can make it false while every test stays green (a review-loop fix once made an older test non-discriminating exactly this way). So date the claim to the commit it was proven at, and when a later commit touches that mechanism, re-run the toggle and restate or correct the claim. This applies to mid-loop fix commits too (see the loop-speed rule in the self-review loop). - **Integration** (server-level through `createRequestHandler`, SSR pipeline, scaffolds): behaviour across modules without a browser. - **Browser** (`*/test/**/browser/*.test.js`, run via `npm run test:browser` / `wtr`): anything touching hydration, client render, DOM, slots, the client router, custom-element upgrade. - **E2E** (`test/e2e/e2e.test.mjs`, run via `WEBJS_E2E=1`): full-stack behaviour observable only in a real browser against the running blog example, including **network probes** (was a module fetched or not), navigation, and streaming. @@ -310,9 +312,11 @@ Beyond review findings, proactively record the *reasoning* behind a PR as commen 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. +**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. + **Keep the loop fast. A round is a reading pass over the diff and should cost seconds, so nothing slow belongs between rounds.** Two rules, and both change WHEN work happens, never WHETHER: -- **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. +- **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). @@ -364,7 +368,7 @@ Each round must: 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. - **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 - - **File it** as a tracked issue when it is genuine but out of scope for THIS PR (a pre-existing bug, an unrelated dependency/hygiene problem, a separate feature). "Out of scope" is NOT a reason to drop a finding. INVOKE the `webjs-file-issue` skill rather than hand-rolling `gh issue create`: it gates on grounding the body in the real codebase first, which is exactly what an out-of-scope review finding needs and exactly what a hurried two-command version omits. Capture the issue number. Verify it landed (`gh project item-list 1 --owner webjsdev --limit 20000`, the board is past 200 items so the default cap of 30 reports a false negative). A finding you call out-of-scope without an issue number is an unfiled finding, which is the exact mistake this clause exists to prevent. If unsure whether something is in-scope, default to fixing it in this PR; only file-and-defer when it is clearly separable and fixing it here would mean scope creep. + - **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). "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. 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. **Record every genuine finding ON THE PR, exactly the way a person reviewing on GitHub would.** The review trail must live on the PR, not only in your reply to the user, since a finding that exists only in the chat transcript is invisible to anyone reading the PR later. **Post it through the mechanics in `### Every PR review is posted ON the PR` and `### Follow the real review flow`, which are authoritative; do not improvise a different shape here.** In short: the round's summary and all its inline `file:line` findings go up as ONE review object, each finding states the problem only, and the disposition (`fixed in ` / `rejected because ` / `filed as #`) goes in a threaded reply, after which the thread is resolved. That is what a real reviewer does on GitHub, and it is the only shape that renders as a grouped review rather than scattered boxes. @@ -376,7 +380,7 @@ Each round must: 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 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. The ONE case that ends the loop without converging is 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), and that ends it as BLOCKED, never as done. +**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. **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/file 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: the sole reason to stop short of convergence is a reviewer that cannot be produced, which you report as blocked and never paper over with an inline pass of your own. @@ -388,11 +392,13 @@ 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 filed as follow-ups: . Ready to merge. +> 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. **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". -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 filed-with-issue-number; if you reported an out-of-scope finding to the user but cannot point to its issue number, 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. +**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. + +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, filed only on the user's go-ahead per step 2's defer rule); a deferred finding missing from both the thread and this report 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. @@ -471,5 +477,6 @@ A merge updates `main` and npm, but the four in-repo apps deployed to Railway (` - If the TASK worktree's `git status` is dirty at start (a prior session died mid-work in it): stop and ask the user to commit, stash, or abandon that work. Never silently lose changes. A dirty PRIMARY checkout is not a blocker and not yours to fix; the worktree cuts from `origin/main` regardless. - 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 its one exit other than convergence (a round finding nothing substantive, then the prose pass); 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/test/hooks/review-loop-exit.test.mjs b/test/hooks/review-loop-exit.test.mjs index 530306588..404b426a1 100644 --- a/test/hooks/review-loop-exit.test.mjs +++ b/test/hooks/review-loop-exit.test.mjs @@ -54,6 +54,22 @@ test('the skill carries the substantive gate and its two-tier exit', () => { // 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. + assert.match(skill, /5 substantive rounds, raised only by the user's instruction/); + assert.match(skill, /ends it as OVER BUDGET/); + assert.match(skill, /An OVER-BUDGET loop reports the rounds run/); + assert.match(skill, /round budget \(default 5 substantive rounds, user-raisable per task\) is spent/); + // Mid-loop fixes carry proof: the red\/green toggle inside the loop-speed + // bullet, the non-discriminating-test re-run, and claim decay. + assert.match(skill, /make an older test non-discriminating without failing it/); + assert.match(skill, /A counterfactual CLAIM decays/); + assert.match(skill, /true of a commit, not a branch/); + // The loop never files follow-up issues on its own; deferral is recorded + // on the PR and filing is the user's call from the report. + assert.match(skill, /the loop NEVER files a follow-up issue on its own/); + assert.match(skill, /awaiting your call on filing/); }); test('deep-review findings carry a required tier with the gate wording', () => { @@ -110,4 +126,6 @@ test('the routed review directive states the substantive exit, not the old absol // untagged findings from tag-capable reviewers stay substantive. assert.match(hook, /from the code-review skill itself/); assert.match(hook, /any tag-capable reviewer is simply treated as substantive/); + // The directive states the round budget the skill mandates. + assert.match(hook, /budget of 5 substantive rounds bounds the whole loop/); }); From 0cc0a0d282bfbd9276a31eefa3ff23a5c9299486 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 29 Jul 2026 20:25:23 +0530 Subject: [PATCH 08/12] docs: align every exit and disposition seam with the fold The three-exit taxonomy (converged, BLOCKED, OVER BUDGET) and the defer disposition now read the same at every cross-reference: the failure-handling reviewer bullet, the standalone-review entry, the liveness block's stopping-point sentence, the prose-tier bullet, and the threaded-reply disposition list. An over-budget stop skips the deferred suites and CI read, stated where the report shape is defined. --- .claude/skills/webjs-start-work/SKILL.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index 455b62e5a..38aa8a4f0 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -330,7 +330,7 @@ The draft PR is already open (step 6), so reviews post to it from the first roun - **A finding is `substantive` when it touches any of three surfaces:** 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 even though it changes no shipped behavior); or a factual claim about runtime behavior in docs (for example "this value is safe" on a runtime where it is not; a number in docs that states runtime behavior, a default, a limit, a condition list, is this surface too). Everything else is `prose`: PR-body counts, docstring and comment wording, naming, style, and any nit about the review artifacts themselves. - **The gate is surface-based on purpose, never importance-based.** Do not substitute a test like "changes shipped behavior" or "affects a user-facing guarantee": importance is judged by the same agent motivated to end the loop, and that exact wording would have ejected the tautological-test findings above, the most valuable findings of the loop that motivated this gate. Surface membership is checkable from the finding itself (which file, what kind of claim). - **The reviewer classifies, you verify.** Every reviewer tags each finding `substantive` or `prose` against the three surfaces (the tag is part of its expected result shape; the delta prompt template and deep-review's finding schema both carry it). Check each tag against the surface definition and re-tag a mislabeled finding; never assign the tags from scratch yourself, or the motivated-termination problem the gate closes reopens through the classification step. The license is asymmetric: an UPGRADE to substantive is always yours to make, while a DOWNGRADE to prose is treated like a rejection, made only when the finding plainly matches the prose enumeration and recorded on the finding's thread with a one-sentence reason, so the ejection is auditable. A finding that arrives UNTAGGED from a reviewer whose result shape carries the tag is treated as substantive, the direction that keeps the loop open. One reviewer kind structurally cannot tag (a fixed result schema with no tier field, the built-in code-review skill's shape): for that reviewer alone you classify, under the same asymmetry, doubt resolving to substantive and every prose classification recorded like a downgrade. -- **Prose-tier findings are still findings.** They are posted, and each is fixed, rejected, or filed 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. +- **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. @@ -347,7 +347,7 @@ Each round must: - **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. - **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 stopping point, and even that is reported at the end.** 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. + - **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. **Working-tree safety (non-negotiable).** A review agent runs against the repository THIS session is using, and every worktree of a repo shares ONE `.git` directory, so its git writes reach the main checkout even from an isolated worktree. A reviewer that runs `git checkout` / `switch` / `reset` / `stash` / `rebase` silently moves the main session's HEAD off the branch (it has happened: a reviewer ran `git checkout main` mid-loop and the local checkout regressed; commits were safe only because they were already pushed). Two defenses: - For every reviewer YOU spawn directly (the delta verifier, a refuter): `isolation: "worktree"`, so its WORKING TREE is its own throwaway checkout and a stray `git checkout` cannot move the files under this session. It does NOT wall off the repo: the worktree shares this repo's `.git`, so ref and config writes still land on the shared repository. (A read-only reviewer needs no shared state; `gh pr diff` reads from GitHub.) @@ -370,7 +370,7 @@ Each round must: - **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). "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. 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. - **Record every genuine finding ON THE PR, exactly the way a person reviewing on GitHub would.** The review trail must live on the PR, not only in your reply to the user, since a finding that exists only in the chat transcript is invisible to anyone reading the PR later. **Post it through the mechanics in `### Every PR review is posted ON the PR` and `### Follow the real review flow`, which are authoritative; do not improvise a different shape here.** In short: the round's summary and all its inline `file:line` findings go up as ONE review object, each finding states the problem only, and the disposition (`fixed in ` / `rejected because ` / `filed as #`) goes in a threaded reply, after which the thread is resolved. That is what a real reviewer does on GitHub, and it is the only shape that renders as a grouped review rather than scattered boxes. + **Record every genuine finding ON THE PR, exactly the way a person reviewing on GitHub would.** The review trail must live on the PR, not only in your reply to the user, since a finding that exists only in the chat transcript is invisible to anyone reading the PR later. **Post it through the mechanics in `### Every PR review is posted ON the PR` and `### Follow the real review flow`, which are authoritative; do not improvise a different shape here.** In short: the round's summary and all its inline `file:line` findings go up as ONE review object, each finding states the problem only, and the disposition (`fixed in ` / `rejected because ` / `deferred as out of scope because `, an issue number appearing only if the user later says to file) goes in a threaded reply, after which the thread is resolved. That is what a real reviewer does on GitHub, and it is the only shape that renders as a grouped review rather than scattered boxes. 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.) @@ -382,7 +382,7 @@ The minimum is TWO rounds: deep-review, then at least one delta verify. A deep-r **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. -**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/file 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: the sole reason to stop short of convergence is a reviewer that cannot be produced, which you report as blocked and never paper over with an inline pass of your own. +**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). ### When to skip the loop @@ -396,7 +396,7 @@ After the loop converges and the deferred suites plus the CI read (step 4) are d **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". -**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. +**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, filed only on the user's go-ahead per step 2's defer rule); a deferred finding missing from both the thread and this report 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. @@ -478,5 +478,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 its one exit other than convergence (a round finding nothing substantive, then the prose pass); 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, 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 `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. From b225f84428ea297750eadea0acd7c6c7a98f1455 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 29 Jul 2026 20:37:37 +0530 Subject: [PATCH 09/12] docs: a finding on the PR's own code is never deferrable Deferral exists for findings the diff did not create (a pre-existing bug, an unrelated problem, a separate feature). A finding on code the PR adds or changes is fixed or rejected in the loop, whatever its size, so deferral cannot become the path around either. --- .claude/skills/webjs-start-work/SKILL.md | 2 +- test/hooks/review-loop-exit.test.mjs | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index 38aa8a4f0..3aefeb83d 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -368,7 +368,7 @@ Each round must: 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. - **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). "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. 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. + - **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. 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. **Record every genuine finding ON THE PR, exactly the way a person reviewing on GitHub would.** The review trail must live on the PR, not only in your reply to the user, since a finding that exists only in the chat transcript is invisible to anyone reading the PR later. **Post it through the mechanics in `### Every PR review is posted ON the PR` and `### Follow the real review flow`, which are authoritative; do not improvise a different shape here.** In short: the round's summary and all its inline `file:line` findings go up as ONE review object, each finding states the problem only, and the disposition (`fixed in ` / `rejected because ` / `deferred as out of scope because `, an issue number appearing only if the user later says to file) goes in a threaded reply, after which the thread is resolved. That is what a real reviewer does on GitHub, and it is the only shape that renders as a grouped review rather than scattered boxes. diff --git a/test/hooks/review-loop-exit.test.mjs b/test/hooks/review-loop-exit.test.mjs index 404b426a1..1629f4984 100644 --- a/test/hooks/review-loop-exit.test.mjs +++ b/test/hooks/review-loop-exit.test.mjs @@ -67,9 +67,11 @@ test('the skill carries the substantive gate and its two-tier exit', () => { assert.match(skill, /A counterfactual CLAIM decays/); assert.match(skill, /true of a commit, not a branch/); // The loop never files follow-up issues on its own; deferral is recorded - // on the PR and filing is the user's call from the report. + // on the PR and filing is the user's call from the report. Deferral can + // never swallow a finding on the PR's own changed code. assert.match(skill, /the loop NEVER files a follow-up issue on its own/); assert.match(skill, /awaiting your call on filing/); + assert.match(skill, /NEVER out of scope, whatever its size/); }); test('deep-review findings carry a required tier with the gate wording', () => { From 30d7634059ecb4b1434ea5379b042d4eb720bbbd Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 29 Jul 2026 20:43:41 +0530 Subject: [PATCH 10/12] docs: the final summary review carries a deferral ledger When a loop deferred anything, its final summary review on the PR lists each deferred finding with its one-sentence reason, so a cold reader sees every deferral in one place without walking the threads. The report accounting treats a deferral missing from the thread, the report, or the ledger as a dropped finding. --- .claude/skills/webjs-start-work/SKILL.md | 4 ++-- test/hooks/review-loop-exit.test.mjs | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index 3aefeb83d..7dfcf042c 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -368,7 +368,7 @@ Each round must: 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. - **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. 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. + - **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. **Record every genuine finding ON THE PR, exactly the way a person reviewing on GitHub would.** The review trail must live on the PR, not only in your reply to the user, since a finding that exists only in the chat transcript is invisible to anyone reading the PR later. **Post it through the mechanics in `### Every PR review is posted ON the PR` and `### Follow the real review flow`, which are authoritative; do not improvise a different shape here.** In short: the round's summary and all its inline `file:line` findings go up as ONE review object, each finding states the problem only, and the disposition (`fixed in ` / `rejected because ` / `deferred as out of scope because `, an issue number appearing only if the user later says to file) goes in a threaded reply, after which the thread is resolved. That is what a real reviewer does on GitHub, and it is the only shape that renders as a grouped review rather than scattered boxes. @@ -398,7 +398,7 @@ After the loop converges and the deferred suites plus the CI read (step 4) are d **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, filed only on the user's go-ahead per step 2's defer rule); a deferred finding missing from both the thread and this report 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 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. **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. diff --git a/test/hooks/review-loop-exit.test.mjs b/test/hooks/review-loop-exit.test.mjs index 1629f4984..4ce9fbbe0 100644 --- a/test/hooks/review-loop-exit.test.mjs +++ b/test/hooks/review-loop-exit.test.mjs @@ -72,6 +72,8 @@ test('the skill carries the substantive gate and its two-tier exit', () => { assert.match(skill, /the loop NEVER files a follow-up issue on its own/); assert.match(skill, /awaiting your call on filing/); assert.match(skill, /NEVER out of scope, whatever its size/); + // The final summary review on the PR carries the deferral ledger. + assert.match(skill, /FINAL summary review on the PR also carries a deferral ledger/); }); test('deep-review findings carry a required tier with the gate wording', () => { From cd746df184db267b3f3121d127be9e1d10b81c4b Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 29 Jul 2026 20:47:52 +0530 Subject: [PATCH 11/12] docs: the chat report expands the deferral ledger The end-of-loop chat report mirrors the PR's deferral ledger instead of a bare name list (each finding, its reason, its thread) and ends by asking which, if any, should be filed, so the user reads the summary without opening the PR. --- .claude/skills/webjs-start-work/SKILL.md | 2 ++ test/hooks/review-loop-exit.test.mjs | 4 +++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index 7dfcf042c..431845c33 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -394,6 +394,8 @@ After the loop converges and the deferred suites plus the CI read (step 4) are d > 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. +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 by asking which, if any, should be filed, since filing happens only on the user's answer. The user reads the deferral summary 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". **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. diff --git a/test/hooks/review-loop-exit.test.mjs b/test/hooks/review-loop-exit.test.mjs index 4ce9fbbe0..24c5b3e8f 100644 --- a/test/hooks/review-loop-exit.test.mjs +++ b/test/hooks/review-loop-exit.test.mjs @@ -72,8 +72,10 @@ test('the skill carries the substantive gate and its two-tier exit', () => { assert.match(skill, /the loop NEVER files a follow-up issue on its own/); assert.match(skill, /awaiting your call on filing/); assert.match(skill, /NEVER out of scope, whatever its size/); - // The final summary review on the PR carries the deferral ledger. + // The final summary review on the PR carries the deferral ledger, and + // the chat report expands the same ledger instead of a bare name list. assert.match(skill, /FINAL summary review on the PR also carries a deferral ledger/); + assert.match(skill, /expand each deferral right there, mirroring the PR ledger/); }); test('deep-review findings carry a required tier with the gate wording', () => { From e609e0c1db1a36b1c1eff48b7f8d485e34e6e147 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 29 Jul 2026 20:51:07 +0530 Subject: [PATCH 12/12] docs: the deferral report must end by asking whether to file The chat report's filing question is mandatory: it ends the report on its own line, silence is never consent, and a deferral report that does not ask has not finished reporting. --- .claude/skills/webjs-start-work/SKILL.md | 2 +- test/hooks/review-loop-exit.test.mjs | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/.claude/skills/webjs-start-work/SKILL.md b/.claude/skills/webjs-start-work/SKILL.md index 431845c33..dad7b2eb4 100644 --- a/.claude/skills/webjs-start-work/SKILL.md +++ b/.claude/skills/webjs-start-work/SKILL.md @@ -394,7 +394,7 @@ After the loop converges and the deferred suites plus the CI read (step 4) are d > 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. -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 by asking which, if any, should be filed, since filing happens only on the user's answer. The user reads the deferral summary in the chat without opening the PR; the PR ledger exists for the cold reader later. +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". diff --git a/test/hooks/review-loop-exit.test.mjs b/test/hooks/review-loop-exit.test.mjs index 24c5b3e8f..2b16d0a81 100644 --- a/test/hooks/review-loop-exit.test.mjs +++ b/test/hooks/review-loop-exit.test.mjs @@ -76,6 +76,9 @@ test('the skill carries the substantive gate and its two-tier exit', () => { // the chat report expands the same ledger instead of a bare name list. assert.match(skill, /FINAL summary review on the PR also carries a deferral ledger/); assert.match(skill, /expand each deferral right there, mirroring the PR ledger/); + // The chat report must END by directly asking whether to file follow-ups. + assert.match(skill, /END WITH A DIRECT QUESTION/); + assert.match(skill, /silence is never consent/); }); test('deep-review findings carry a required tier with the gate wording', () => {