docs: exit the review loop on a substantive gate plus one prose pass by vivek7405 · Pull Request #1173 · webjsdev/webjs

vivek7405 · 2026-07-29T13:33:40Z

Closes #1171
Closes #1176

The self-review loop's exit was "a round that finds nothing", which cannot terminate on prose surfaces because prose always yields another nit. Measured on a live 14-round loop: not one finding after round 6 touched the shipped guard, and rounds 10 to 13 relitigated wording about a transpiler's internals, each round falsifying the previous round's rule. #1159 fixed round shape and liveness but left the termination condition alone; this is the follow-up that condition needed.

What changed

In .claude/skills/webjs-start-work/SKILL.md, the ### How the loop works section changes as follows.

A substantive gate. Findings now carry a tier. A finding is substantive when it touches shipped source, a test's ability to OBSERVE the defect it claims to cover (a tautological assertion is substantive even though it changes no shipped behavior), or a factual claim about runtime behavior in docs. Everything else is prose. Only substantive findings force another round.
Surface-based on purpose, never importance-based. A gate like "changes shipped behavior" is judged by the same agent motivated to terminate, and would have ejected the tautological-test findings that were the most valuable output of the loop that motivated this change. Surface membership is checkable from the finding itself.
The reviewer classifies, the orchestrator verifies. The tag arrives with the finding (the delta prompt template and deep-review's finding schema both carry it); the orchestrator checks tags against the surface definition rather than assigning them, and an untagged finding is treated as substantive, the direction that keeps the loop open.
One bounded prose pass ends the loop. After a round finds nothing substantive, a single fresh reviewer sweeps only the prose surfaces; its fixes are applied without re-review. The one exception: a prose fix that itself touches a substantive surface gets one delta-scoped check of that fix alone. The pass is mandatory, and it never counts toward the round total or the two-round minimum.
A content rule alongside the termination rule: 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 doc claim; fix such a finding by removing the claim, not refining it. Four successive constant-folding rules for one Bun version, each falsified by the next round, is the incident.
Every cross-reference of the old exit updated to the new one: the loop intro, steps 3 and 4, the last-round absolute, the standalone-review entry, the report template and round counting, the step-6 ready-for-review gate, the loop-speed bullets, and ## Failure handling. The BLOCKED sentinel, liveness rules, and worktree-safety rules are untouched.

In .claude/workflows/deep-review.js, the FINDINGS schema gains a required tier enum (substantive | prose) and the finder-lens prompt carries the three-surface definition, so round-1 confirmed findings classify identically to delta findings. Dedup is tier-aware (a substantive finding survives a same-line collision with a prose one; severity orders within a tier), and the tier rides through the jury unchanged.

The gate changes only WHEN the loop exits, never WHETHER a surface is reviewed: both tiers are still posted and resolved on the PR, and the deferred suites still run in full after convergence.

Folded in: #1176, the round budget and mid-loop fix proofs

Filed as a follow-up and folded here at the owner's direction (the same-file split would drift, the #1159 precedent). Three additions, all in the same files this PR already edits:

A round budget bounds the loop. Five substantive rounds by default, raised only by the user's instruction for the task. A spent budget ends the loop OVER BUDGET: the final round's findings post as usual, the flip to ready is withheld, and the report carries the rounds run, the open findings, and a read on the non-convergence cause. The exit taxonomy is now converged, BLOCKED, or OVER BUDGET. Measured need: an 18-round loop whose last four rounds were spent catching regressions its own mid-loop fixes introduced.
Mid-loop fixes carry proof. A fix that changes shipped-source behavior runs the same commit-first red/green counterfactual toggle the original change gets, stated inside the loop-speed bullet so speed cannot be read as license to skip it; a fix touching behavior a prior test claims to cover re-runs that test's counterfactual, because a fix can make an older test non-discriminating without failing it. The Definition of done's counterfactual block adds claim decay: "reverting X reds Y" is true of a commit, not a branch, so the claim is dated and re-verified when a later commit touches the mechanism.
The loop never files follow-up issues on its own. The old rule filed every genuine out-of-scope finding, which produced five issues from one loop. Now such findings are DEFERRED: recorded on the finding's thread, listed in the end-of-loop report, and filed only on the user's explicit go-ahead.

The router directive and the guard test carry the budget and the deferral rule so they cannot silently drift.

Process note on this fold: the loop had already converged when the fold landed, and the owner's explicit cost directive was to verify the addition with a single delta-scoped round rather than the full deep-review re-run a scope change normally triggers. That deviation is deliberate and this sentence is its record.

Test plan

Running node --test test/hooks/route-skills.test.mjs gives 16/16 pass, including "every skill the hook can route to is committed in-repo".
deep-review.js syntax validated as an async function body (the context the Workflow runtime executes scripts in). node --check is inapplicable to workflow scripts: the pre-existing top-level return is legal in the runtime's async context but not in a plain module, so the acceptance criterion naming it was wrong as written and this is the equivalent check.
Both copies of the skill (in-repo and ~/.claude) verified byte-identical with diff -q after the change.

Doc surfaces

Tests: the router test above. Markdown / docs site / scaffold / MCP / editor plugins / marketing / dogfood apps / version bumps: N/A because nothing in packages/*/src or any user-facing surface changed; this edits a Claude Code skill file and the committed review workflow, which ship to agents working this repo, not to anything the framework publishes.

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).

vivek7405

The gate itself holds up under adversarial reading, and the four scenario-style objections I checked do not reproduce (severity-only dedup shadowing and the CAP=12 truncation are pre-existing round-1-only behavior that the mandatory delta pass covers; the code-example classification the prose-pass exception names is granted by its own text; committed executable tooling is shipped source in the gate's sense). But the exit condition has three real gaps and a handful of wording inconsistencies, inline below plus two that sit outside the diff:

.claude/hooks/route-skills.sh:186 still injects the OLD exit ("re-review until a round is clean. Never report done off a round that found something") on exactly the standalone review prompts whose exit this PR redefines, and the injection announces itself as authoritative for the prompt. Left as is, a review session gets steered straight back into the prose relitigation this change ends. Fixing on this PR.
The round-1 deep-review bullet (.claude/skills/webjs-start-work/SKILL.md:320) still says "clean only when it confirms nothing, and its confirmed findings feed this same loop (fix, post as one review object, delta round)" with no tier qualification, so a prose-only confirmed list reads as feeding a delta round, the opposite of steps 3 and 4.

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.

vivek7405

The fixes are consistent with each other except at three seams, two of them worth calling substantive:

.claude/workflows/deep-review.js:171 (unchanged lines, so noting it here): dedup keys on file:line and keeps the winner by severity alone, so a prose-major finding evicts a substantive-minor one at the same line and the round can present as all-prose while a lens raised a substantive defect. Tier now decides the loop's exit, so dedup has to prefer substantive at a collision; the body's "rides through dedup unchanged" sentence goes stale with the fix.
The last commit's message says the new test pins the load-bearing anchors, but several of that commit's own hunks are unpinned (the round-1 bullet tier split, the counting parenthetical, the failure-handling prose-pass entry, both counts qualifiers). Widening the test to cover them makes the claim true going forward.

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.

vivek7405

The tier-aware dedup fixed the collision case but left both severity-ordered paths downstream of it tier-blind, which is the same eviction one statement later. Both fixed together with a tier-first sort, and the CAP-dropped tail now lands in unverified so nothing is silently excluded from the returned result.

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.

vivek7405

The tier-first sort landed right (each widened assertion verifiably reds on revert), but the CAP-tail rerouting left its own documentation wrong, and the dataflow trace surfaced one pre-existing leak the never-silently-dropped guarantee does not cover:

.claude/workflows/deep-review.js:228 (unchanged line): a finding whose whole jury task resolves falsy is discarded by verified.filter(Boolean) and lands in no returned list. The empty-jury fail-open covers dead refuters inside a live slot, not a dead slot. Every allocated finding has to land somewhere; the consistent direction is the same fail-open the empty jury already gets.
.claude/workflows/deep-review.js:198 (unchanged line): the jury-walk comment still says "severity-first (toVerify is already severity-sorted)", the opposite of the sort the last fix installed two statements up.

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.

vivek7405

Went back over the last fixes (the dead-slot fail-open index alignment, the two-cause unverified wording, the directive scoping, the new assertions for vacuity) and found nothing left to flag. The exit-condition story now reads consistently across the skill, the workflow, and the router.

vivek7405

Final wording sweep. Two items in the PR body alongside the inline ones: three code-span left-hand sides followed by colon-then-prose (the invariant-11 definition-list shape) get rephrased verb-led, and the third What-changed bullet names the same actor orchestrator in one sentence and loop-runner in the next; unifying on orchestrator. No banned glyphs or casing violations anywhere in the added content.

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.

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.

vivek7405

The fold left stale cross-references at the seams, inline where the lines are in the diff and here for the rest:

Step 1's liveness block still opens "A genuinely unrecoverable reviewer is the only stopping point"; the round budget is now a second sanctioned stop.
The record rule's threaded-reply dispositions still enumerate "filed as #n", which the defer rule makes unusable at loop time (no issue number exists until the user says to file) and which omits the deferral disposition.
The reporting section's gating sentence covers only the converged case; whether the deferred suites and CI read run on an over-budget stop is unstated (they should not: they gate the flip to ready, which an over-budget loop withholds).

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.

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.

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.

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.

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.

vivek7405 self-assigned this Jul 29, 2026