Skip to content

docs: two-tier review-loop exit, substantive gate then one prose pass #1171

Description

@vivek7405

Problem

The self-review loop's termination condition is "loop until a round finds nothing" (.claude/skills/webjs-start-work/SKILL.md, the "The LAST round must be clean, always" absolute, ~L369 at origin/main 16198d17). #1159 fixed round shape and liveness but deliberately left this condition alone, and it cannot terminate on prose surfaces, because prose always yields another nit.

Measured on a live 14-round loop (a security-fix PR reviewed 2026-07-28/29). Rounds ~5 to 10 produced the findings that mattered: two test files whose assertions checked for the absence of a secret that could never have appeared in the searched string (so they passed for free, leaving the guard unproven), a streaming-drain test that stayed green on a real leak three separate times for three different reasons, and a docs claim that a secret in an outer const is safe, which is false on Bun. After round ~6 not one finding touched the shipped guard or its commit sites. Rounds 10 to 13 produced four successive rules about Bun's constant folding, each measured, each falsified by the next round, much of it the loop correcting its own previous round's overcorrection. The loop had moved from reviewing the fix to reviewing the review artifacts (test docstrings, PR-body counts, prose about a transpiler), and on that surface "a round that finds nothing" never arrives.

Design / approach

Replace the single exit with a two-tier exit built on a SURFACE-based gate.

  1. Loop until a round finds nothing substantive. A finding is substantive when it touches any of three checkable surfaces: (a) shipped source, (b) 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 (c) a factual claim about runtime behavior in docs (for example "this value is safe" on a runtime where it is not). Everything else (PR-body counts, docstring wording, prose style, review-artifact nits) is prose-tier and does not keep the substantive loop open.
  2. Then run exactly ONE bounded prose pass. Its fixes are applied and NOT re-reviewed, with one narrow exception: a prose fix that itself touches a substantive surface (a code example inside a docstring, say) gets one delta-scoped check of that fix alone.

Why surface-based rather than the first-proposed "changes shipped behavior or a user-facing guarantee": an importance gate is judged by the same agent that is motivated to terminate, and it would have excluded the tautological-test findings, which the retrospective itself rated the most valuable of the whole loop. Surface membership is checkable from the finding itself (which file, what kind of claim), so it resists motivated classification.

Guard on the classifier: the REVIEWER tags each finding substantive-or-prose against the surface definition, and the loop-running agent VERIFIES the tag rather than assigning it. Otherwise the motivated-termination problem the gate closes reopens through the classification step.

One adjacent content rule, same file, one sentence: never publish a rule about a third-party tool's internals (a transpiler's constant folding, an engine's optimization behavior) as a review finding or a doc claim. The four falsified Bun constant-folding rules are the incident. This is a review-content rule, not a termination rule, and stands on its own.

This changes only WHEN the loop exits, never WHETHER a surface gets reviewed. The prose pass is mandatory, not optional. An earlier loosening attempt (#1157, per the #1159 body) read as permission to skip a layer, so the text must say this outright.

Implementation notes (for the implementing agent)

  • Work from origin/main (16198d17, the docs: make the self-review loop fast, parallel, and stall-proof #1159 merge), not a stale local copy. At filing time the primary checkout's main was one commit behind and its skill copy predates docs: make the self-review loop fast, parallel, and stall-proof #1159. Verify with git log --oneline -1 origin/main -- .claude/skills/webjs-start-work/SKILL.md before editing.
  • Where to edit, all in .claude/skills/webjs-start-work/SKILL.md under ### How the loop works (line refs against origin/main):
    • Step 3 (~L363, "If the round produced any actionable findings"): the delta-round trigger becomes "any actionable SUBSTANTIVE findings". A round whose findings are all prose-tier ends the substantive loop and enters the prose pass.
    • Step 4 (~L365, "If the round reports CLEAN"): the exit gains the prose pass between "loop is done" and running the deferred suites.
    • The absolute at ~L369 ("The LAST round must be clean, always"): rewrite around the two-tier exit. The BLOCKED path (the one non-clean exit, end of that paragraph) stays untouched.
    • The standalone-review entry (~L371) and ## Failure handling (~L464): both cross-reference the exit condition and must say the new one.
    • The delta-verifier prompt template (~L346 and ~L394): the reviewer's expected result shape ("a finding list or the literal CLEAN") gains the per-finding substantive/prose tag plus the three-surface definition, so the tag arrives with the finding. The BLOCKED sentinel and liveness rules are untouched.
    • The report template and round counting (~L381 to ~L385): report as "N substantive rounds plus the prose pass". The prose pass never counts toward <K> and never substitutes for the minimum-two-rounds rule (~L367), whose "clean" also becomes "nothing substantive".
  • .claude/workflows/deep-review.js: the FINDINGS schema (L45 to L56, required ['file', 'line', 'title', 'detail', 'severity']) gains the tag (for example a required surface enum) with the three-surface definition in the finder prompts, so round-1 confirmed findings classify the same way as delta findings. Sanity-check with node --check .claude/workflows/deep-review.js.
  • Landmines:
    • docs: make the self-review loop fast, parallel, and stall-proof #1159 restructured this exact section days ago. Its bullets are cross-referenced by NAME rather than ordinal so an insertion cannot silently truncate the list for a literal reader. Preserve that property.
    • Both copies of the skill MUST end byte-identical: in-repo .claude/skills/webjs-start-work/SKILL.md and ~/.claude/skills/webjs-start-work/SKILL.md (the docs: make the self-review loop fast, parallel, and stall-proof #1159 rule, verify with diff -q).
    • AGENTS.md invariant 11 (banned prose glyphs, WebJs casing) applies to the skill file, enforced by .claude/hooks/block-prose-punctuation.sh on new content.
    • A review loop may be live in another session while this lands (a 14-round loop was running when this was filed). The skill copy in ~/.claude is read at spawn time, so landing mid-loop is safe but the running loop keeps its old rules.
  • Tests + docs surfaces: node --test test/hooks/route-skills.test.mjs must stay green (it asserts every routed skill is committed in-repo). No packages/*/src change and no user-facing surface, so the docs site / scaffold / marketing surfaces are N/A, same rationale as docs: make the self-review loop fast, parallel, and stall-proof #1159.

Acceptance criteria

  • The loop exits in two tiers: substantive rounds until one finds nothing substantive, then exactly one prose pass whose fixes are applied unreviewed, except a prose fix touching a substantive surface, which gets one delta check of that fix alone
  • The substantive definition is surface-based and lists exactly the three surfaces (shipped source, a test's ability to observe its claimed defect, factual runtime claims in docs); importance-based wording ("changes shipped behavior", "user-facing guarantee") appears nowhere as the gate
  • The reviewer classifies and the orchestrator verifies: the delta prompt template and the deep-review FINDINGS schema both carry the tag
  • The report template counts substantive rounds and names the prose pass separately; the prose pass does not count toward <K> or the two-round minimum
  • The third-party-internals content rule is present as its own sentence
  • The BLOCKED path, liveness rules, and minimum-two-rounds rule survive intact, with "clean" consistently meaning "nothing substantive" at every cross-reference (steps 3 and 4, the ~L369 absolute, the standalone entry, Failure handling)
  • The text states this changes WHEN the loop exits, never WHETHER surfaces are reviewed, and that the prose pass is mandatory
  • node --check .claude/workflows/deep-review.js passes and node --test test/hooks/route-skills.test.mjs is green
  • In-repo and ~/.claude skill copies are byte-identical after the change

Metadata

Metadata

Assignees

Labels

documentationImprovements or additions to documentation

Type

No type

Projects

Status
Done

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions