fix(symcheck): make the round-1-only gap visible at the gate (#99) - #135
Conversation
The symbol-check pass is derived and dispatched in round 1 only, so its count covers the document as the secondaries first read it — not the ready-to-paste code the author adds afterwards while fixing the findings this review raised. That is the least-reviewed code in the change by construction, and round 1's clean `<M>/<M>` was all the gate rendered: a review that added unchecked code was byte-identical at the gate to one that added none. Every round N >= 2 now records `> [symcheck-added: <K>]`, and `gate-summary` renders it under the coverage line. Zero is recorded too — "I checked and nothing was added" and "nobody looked" are different facts, and an unrecorded later round now reads NO RECORD rather than as silence. Option 3 of the three in #99, chosen deliberately over option 2 (dispatch the pass again when new blocks exist). Option 2 needs a stable per-block identity that does not exist: rows are positional (`B<n>`) and carry line ranges that shift on any edit above them, so "which blocks are new" is not a set-difference over row lines. `<K>` is therefore a COUNT, not an identity diff — a round that swaps one block for another nets zero — and the instruction, the docs and the zero-case gate wording all say so rather than implying a precision it lacks. Round-1-only behaviour is unchanged; its two existing entries are untouched. Also corrects a docs sentence that predates this: the symcheck section said "Each round records a durable coverage line", which was never true of a round-1-only pass and would now contradict the new record. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019vdNEXiS6JU1dRzvzXYS2n
Four findings from this PR's own multi-review gate (fable; codex clean), all agreed. r1 (med) — the record sat in fan-out step 8, which runs BEFORE the primary edits the body, so the count predated the round's own fixes; on the final round those are precisely the blocks nobody checks. Worse than reported: ROUND 1's fixes were not counted at all, because the gate's NO RECORD branch was keyed on round > 1 — so a review that converges at round 1, the commonest shape, rendered as though everything had been checked. The record now happens at the END of the primary turn, every round including round 1, and the gate no longer looks at the round (which also drops gate-summary's core.sh call). r2 (med) — the derivation hung two `0` parentheticals off `<M>`, so the first read as `<K> = 0` for the no-blocks-then-gained-blocks case the code comment says the line exists for. `<M>` is now defined first, then `<K>` as the current count minus `<M>`, floored at `0`. r3 (low) — four rendering branches, three entries, and the uncovered one was the zero branch whose wording is what stops the gate overclaiming. Added `star/gate-symcheck-added-zero` and `star/gate-symcheck-added-scoped-to-coverage` (silence where the pass never applied), and corrected a table comment that asserted a completeness it did not have. r4 (low) — the paragraph claimed "ROUND N >= 2 ONLY" from inside a step whose header says to skip it on round N >= 2. Resolved by r1's move; the packaging assertions are now windowed on the Primary turn section, so the placement itself is what they check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019vdNEXiS6JU1dRzvzXYS2n
…ll-clear Three findings from round 2 of this PR's own multi-review gate (fable; codex clean both rounds), all agreed and all reproduced. fable-rd2-r1 (med) — the added-count block rendered on EVERY PR scratch. `rows` exits 3 on one by construction, so the round records `not applicable`, which is non-empty and reaches the block: the coverage enclosure never kept scratches out. The primary never edits a PR diff, so no block can be added and the warning could never be true. Reproduced on this PR's own scratch. The sharper half of the finding was about my own work: the SA0 test's premise and the `scoped-to-coverage` entry's recorded reason BOTH claimed a PR scratch has no coverage line. It has one, so that entry's rationale was false — the same class as a false SURVIVES-BY-DESIGN. Replaced with a `_doc_flavor` branch, a real PR-scratch test, and `star/gate-symcheck-added-dormant-on-pr`. fable-rd2-r2 (med) — the record carries no round and the reader is `tail -1`, so a round that forgot to record rendered an earlier round's count as current: #99's own gap moved one round down, and a stale count is worse than none because it reads as an all-clear. Reproduced on a round-3 doc holding one round-1 record. Fewer records than the marker's round now renders STALE, and the instruction states the one-per-round rule the check keys off. fable-rd2-r3 (low) — my edit created a contradiction: I corrected one of two identical false "Each round records a durable coverage line" sentences, leaving the doc asserting opposite cadences for two passes that are round-1-only for the same reason. Fixed the crossref one too; neither copy carries it now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019vdNEXiS6JU1dRzvzXYS2n
kevin-agrology
left a comment
There was a problem hiding this comment.
Commented inline (2)
Multi-review
Agreed findings (3)
🟠 med — The added-count block renders on every PR scratch, where it can never mean anything: symcheck.sh rows exits 3 on a PR scratch by construction (no **Files:** sections — I ran it on this very scratch, rc=3), so step 8 records [symcheck-coverage: not applicable], sx is non-empty, and the gate prints — round 1 only: NO RECORD of whether blocks were added since on a document whose body the primary never edits and to which no block can be added. Reproduced: a - **PR:**-headed doc with [symcheck-coverage: not applicable] renders the NO RECORD line. The only way a PR review avoids the warning is a ritual rows | wc -l that is always 0, recorded as [symcheck-added: 0] every round. The plan-lint block directly below already branches on _doc_flavor "$doc" == pr for exactly this reason (#118); this block does not. Note too that the scoped-to-coverage mutation comment ("hoisting it out would put a NO RECORD on every PR scratch") and the SA0 test comment ("a PR scratch, where the pass never applies") both assume a PR scratch has no coverage line — it has one, so the enclosure does not keep PR scratches silent and the recorded reason for that entry is not the reason it works. — risk: every PR gate carries a false "unchecked blocks" warning (or a meaningless ritual zero), diluting the signal the change exists to create
🟠 med — The record carries no round and _symcheck_added is tail -1, so a later round that FORGOT to record renders the earlier round's count as current. Now that round 1 always records (the round-2 change), NO RECORD fires only when the primary skipped the step in EVERY round; skip it in round 3 alone and the gate confidently prints round 1's no net change in block count since for a review whose round-3 fixes may have added the very blocks nobody checked. Reproduced: a converged · round 3/5 doc with [symcheck-coverage: 4/4 rows verdicted] and a single [symcheck-added: 0] renders the zero wording, not NO RECORD. "I checked and nothing was added" and "nobody looked" are the two facts the PR description says the design keeps distinct — but only for round 1; the gate cannot tell a stale record from a current one, which is the #99 visibility gap moved one round down. The information to detect it is already in the doc: the marker round is N and the instruction says exactly one record per round, so fewer than N records (or a record count not matching the round) is the "nobody looked" case and can render as such. — risk: a stale zero renders as a current all-clear at the gate — the confidently-wrong output this repo's visibility rule exists to prevent
🟡 low — The PR corrects "Each round records a durable coverage line" in the symcheck section because a round-1-only pass never records per round — but the identical sentence stands, unchanged, at the head of the crossref section (docs/multi-review.md:292, and the vendored .agents/skills/multi-review/protocol/multi-review.md:292), and the crossref pass is round 1 only for the same reason (command step 2: "ROUND 1 ONLY"). Same falsehood, same rationale, one of two occurrences fixed. Outside the changed hunks, so no anchor — raising it because the PR's own "Scope held" paragraph gives the reason it should go too. — risk: the protocol doc contradicts itself on the crossref pass's cadence after this PR made the symcheck sentence say the opposite
Fixed during review (4)
🟠 med — [symcheck-added: <K>] is recorded in fan-out step 8, which runs BEFORE this round's primary turn — so the count never includes the blocks the primary adds while addressing this round's findings. On the final round (the common 2-round case) the gate renders a <K> that predates the last set of fixes, and a 0 renders as "no net change in block count since" for a document that gained blocks after the record was written. — risk: the gate states a checked fact ("I checked and nothing was added") about a doc revision that was never counted — the confidently-wrong reading #99 set out to remove — ✅ Moved to the end of the primary turn, every round, at head 1e76331; the gate's round check is gone with it.
🟠 med — The derivation sentence is ambiguous about WHICH quantity the first 0 names: "that count minus the <M> recorded in round 1's symcheck-coverage line (0 if that line said not applicable, and 0 if the subtraction goes negative)". The second parenthetical unambiguously defines <K>; by parallel structure the first reads as <K> = 0 when round 1 said not applicable — which would record symcheck-added: 0 for exactly the case the star.sh comment calls out ("a round-1 doc with no blocks that GAINS one is exactly this gap"). — risk: a primary following the literal reading records 0 for a doc that went from no blocks to several, and the gate renders "no net change" — the opposite of the truth, in the one case the code comment says the line exists for — ✅ <M> is defined first, then <K> as the current count minus <M> floored at 0, at head 1e76331.
🟡 low — The mutation table adds three star/gate-symcheck-added-* entries and the comment says "Three branches, three assertions", but the rendering has FOUR output branches: positive, zero, NO RECORD, and dormant. The zero-branch echo ("no net change in block count since (a one-for-one swap would not show here)") — the anti-overclaim wording the PR description leans on hardest — has no entry, so the sweep does not prove gate-summary: zero added-count rendered nothing or overclaimed can go red. (Reading the test it plainly can, so this is a table-completeness gap, not a falsifiability one.) — risk: the one guard whose wording is load-bearing for honesty at the gate is the one the sweep never touches, and the comment beside the table claims a completeness it does not have — ✅ star/gate-symcheck-added-zero added at head 1e76331; the branch count in that table comment has now been corrected twice, and the comment says so.
🟡 low — The new paragraph is titled "ROUND N ≥ 2 ONLY, and never skipped" but sits inside step 8, whose opening rule (line 845, unchanged) is "On round N ≥ 2, skip this step entirely". A primary that obeys the step header never reaches the paragraph that overrides it. The plan-lint block already lives with the same tension, and the gate renders NO RECORD when the record is missing, so this fails visibly rather than silently — but the instruction is now internally contradictory in the direction that produces the NO RECORD. — risk: NO RECORD at the gate on every multi-round review run by a primary that reads step 8's header literally, i.e. the feature degrades to its own failure notice — ✅ The paragraph left step 8 entirely at head 1e76331, and the packaging window is now the Primary turn section.
———
🤖 Posted by AI agents (claude-opus-5[1m] + claude-fable-5-1) via multi-review star review.
| # secondaries first read it — not the blocks the author added afterwards while fixing their | ||
| # findings, which are the least-reviewed code in the change by construction. Say which. | ||
| # Rendered for the not-applicable case too: a round-1 doc with no blocks that GAINS one is | ||
| # exactly this gap, and the coverage line alone would read as "nothing to check". | ||
| # | ||
| # NOT conditioned on the round (fable-rd1-r1). The primary records this at the END of its own | ||
| # turn, so EVERY round has one to record — round 1 included, whose fixes are unchecked for | ||
| # exactly the same reason. Gating the NO RECORD on round > 1 hid the commonest case of the gap: | ||
| # a review that converges at round 1 and never records anything rendered as if all was checked. | ||
| # |
There was a problem hiding this comment.
🟠 med — The added-count block renders on every PR scratch, where it can never mean anything: symcheck.sh rows exits 3 on a PR scratch by construction (no **Files:** sections — I ran it on this very scratch, rc=3), so step 8 records [symcheck-coverage: not applicable], sx is non-empty, and the gate prints — round 1 only: NO RECORD of whether blocks were added since on a document whose body the primary never edits and to which no block can be added. Reproduced: a - **PR:**-headed doc with [symcheck-coverage: not applicable] renders the NO RECORD line. The only way a PR review avoids the warning is a ritual rows | wc -l that is always 0, recorded as [symcheck-added: 0] every round. The plan-lint block directly below already branches on _doc_flavor "$doc" == pr for exactly this reason (#118); this block does not. Note too that the scoped-to-coverage mutation comment ("hoisting it out would put a NO RECORD on every PR scratch") and the SA0 test comment ("a PR scratch, where the pass never applies") both assume a PR scratch has no coverage line — it has one, so the enclosure does not keep PR scratches silent and the recorded reason for that entry is not the reason it works. — risk: every PR gate carries a false "unchecked blocks" warning (or a meaningless ritual zero), diluting the signal the change exists to create — 🤖 multi-review star review (claude-fable-5-1 + claude-opus-5[1m])
| _symcheck_added() { # <doc> -> "<K>" | ||
| review_section "$1" | strip_fences /dev/stdin \ | ||
| | grep -oE '^> \[symcheck-added: [0-9]+\]' \ | ||
| | sed -E 's/^> \[symcheck-added: ([0-9]+)\]$/\1/' \ | ||
| | tail -1 | ||
| } |
There was a problem hiding this comment.
🟠 med — The record carries no round and _symcheck_added is tail -1, so a later round that FORGOT to record renders the earlier round's count as current. Now that round 1 always records (the round-2 change), NO RECORD fires only when the primary skipped the step in EVERY round; skip it in round 3 alone and the gate confidently prints round 1's no net change in block count since for a review whose round-3 fixes may have added the very blocks nobody checked. Reproduced: a converged · round 3/5 doc with [symcheck-coverage: 4/4 rows verdicted] and a single [symcheck-added: 0] renders the zero wording, not NO RECORD. "I checked and nothing was added" and "nobody looked" are the two facts the PR description says the design keeps distinct — but only for round 1; the gate cannot tell a stale record from a current one, which is the #99 visibility gap moved one round down. The information to detect it is already in the doc: the marker round is N and the instruction says exactly one record per round, so fewer than N records (or a record count not matching the round) is the "nobody looked" case and can render as such. — risk: a stale zero renders as a current all-clear at the gate — the confidently-wrong output this repo's visibility rule exists to prevent — 🤖 multi-review star review (claude-fable-5-1 + claude-opus-5[1m])
TL;DR: The symbol-check pass runs in round 1 only, so any ready-to-paste code the author adds
afterwards — while fixing the very findings the review raised — is never checked against the repo.
The gate showed round 1's clean count and said nothing about the gap, so a review that added
unchecked code looked identical to one that added none. Later rounds now record what the pass
didn't see, and the gate renders it.
Closes #99.
What and why
The pass is derived and dispatched in round 1 only (
commands/multi-review.mdstep 2), inheritedfrom the cross-reference pass where the rule is well-founded. The consequence is that the code
written during the review — which the standing note at every terminal state already calls out as
unreviewed — is exactly what this pass is switched off for.
Every round N ≥ 2 now records:
<K>is the doc's currentsymcheck.sh rowscount minus the<M>round 1 recorded.gate-summaryrenders it under the existing coverage line:
Zero is recorded and rendered too — "I checked and nothing was added" and "nobody looked" are
different facts. A later round that recorded nothing at all renders
— round 1 only: NO RECORD of whether blocks were added since, and the line stays dormant on asingle-round review, which has no later round to report on.
Why option 3, not option 2
The issue offers three options and says "(3) is the minimum; (2) is probably the right end state."
This is option 3, chosen deliberately after finding what option 2 actually costs.
Option 2 needs a stable per-block identity, and none exists. A row is
B1<TAB>Task 1<TAB>bash:12-20<TAB>…— an ordinal plus a line range. Any edit above a block shiftsits range and renumbers it, so "which blocks are new since round 1" is not a set-difference over
row lines. Making option 2 work means adding a content-hash column to
rows, which the reviewerprompt,
symcheck.sh check, and two existing mutation entries all read. That is a change to apass's output contract, and it is not the change #99's stated invariant asks for.
The invariant actually violated is the visibility one — this repo's rule that a pass which does
not run must never be indistinguishable from one that ran clean. Option 3 closes exactly that, in
the idiom already used for all three coverage lines, with no new script coupling.
So
<K>is a COUNT, not an identity diff, and a round that replaces one block with another netszero. The instruction, the protocol doc, and the zero-case gate wording each say so explicitly
rather than implying a precision the number lacks — and there is a mutation entry
(
command/symcheck-added-count-not-identity) whose whole job is to keep that admission in the text.Scope held
Round-1-only behaviour is unchanged, and its two existing entries
(
command/symcheck-round-1-only,command/symcheck-skip-later-rounds) are untouched — I re-probedboth anyway, since the new text sits next to them.
One pre-existing docs error is corrected because the change makes it self-contradictory: the
symcheck section said "Each round records a durable coverage line", which was never true of a
round-1-only pass.
What this PR's own review gate changed
Two rounds,
codex+fable. 7 findings, all agreed, none disputed — codex reported[no-findings]both rounds; every finding came from fable, and every one was about code this PRadded. Each was reproduced before being accepted.
Round 1 — the design was wrong in a way I had not seen:
the count predated the round's own fixes — and on the final round those are exactly the blocks
nobody checks. Worse, found while fixing it: I had keyed NO RECORD on
round > 1, so a reviewthat converges at round 1 — the commonest shape — recorded nothing and rendered as if all was
checked. The feature was blind to its most common case. Moved to the end of the primary turn,
every round; the gate's round check went with it, which also drops a
core.shcall.0parentheticals off<M>, so the first read as<K> = 0for the very no-blocks-then-gained-blocks case the code comment says the line exists for.
whose wording is what stops the gate overclaiming. My table comment asserted a completeness it did
not have.
on round N ≥ 2.
Round 2 (re-fanned on the new-logic trigger, its one allowed use) — two more real defects in the
round-1 fix:
rowsexits 3 on one byconstruction, so the round records
not applicable— which is non-empty and reaches the block.Reproduced on this PR's own scratch. The sharper half was about my own work: the test's premise
and the mutation entry's recorded reason both claimed a PR scratch has no coverage line. It has
one, so that entry's rationale was false — the same class as a false
SURVIVES-BY-DESIGN.Now branches on
_doc_flavor, with a real PR-scratch test and a correctly-reasoned entry.tail -1, so a round that forgot torecord rendered an earlier round's count as current — symcheck runs round 1 only, so code blocks the author adds between rounds are never checked #99's own gap moved one round down, and
a stale count is worse than none because it reads as an all-clear. Fewer records than the marker's
round now renders
STALE.leaving the doc asserting opposite cadences for two passes that are round-1-only for the same
reason. Fixed the crossref one too.
Converged at round 2 (no
highs, trigger spent). The round-2 fixes are themselves unreviewed —true of every terminal state, and what the human gate is for.
Verification evidence
Written test-first; each assertion was watched failing for the expected reason before the code
existed. The two packaging assertions that were authored after their prose (so never naturally RED)
were verified by removing the text they pin and confirming both go red, then restoring it.
gate-summary: round-1-only gap is silent — 4/4 reads as fully checkedgate-summary: zero added-count rendered nothing or overclaimedgate-summary: an unrecorded later round is silentcommand: no round records blocks added after round 1 — the gate cannot show the gapcommand: nothing says how <K> is derived, so the count is left to inventioncommand: <K> is presented as if it detected any change, which it does notgate-summary: a PR scratch was warned about unchecked blocks it cannot havegate-summary: a round-1 record rendered as current on a round-3 reviewgate-summary: a single-round review hid the gap — its own fixes are uncheckedLocal gate, all green: both suite legs (bash 5 and
/bin/bash3.2), shellcheck incl..githooks/pre-push, version check (1.35.0 → 1.36.0),--verify-table(396 entries), and--onlyon all elevensymcheck-addedentries plus the two adjacent pre-existing round-1-onlyones — all caught.
The reviewer bundle was rebuilt (
docs/multi-review.mdis vendored into it).🤖 Generated with Claude Code
https://claude.ai/code/session_019vdNEXiS6JU1dRzvzXYS2n