fix: silent-revert canary keeps the last row and rejects a trailing comma

[kyle-sexton]

Closes #2874
Closes #2875

Summary

The silent-revert canary's self-proof lane could report a green replay after verifying nothing. verify_known_incidents used while read without keeping a last line that lacks a trailing newline, and it had no row-count floor, so an empty file, a comments-only file, or a one-row file whose only row was dropped all printed "Canary reproduces every recorded incident" and exited 0. The same read idiom dropped the last acknowledgment-file row (fail-closed). Attribution grammar from #2843 is already on main; a trailing comma in [sha=n,] was still accepted because word splitting under IFS=',' drops a trailing empty field, against the file's own "exactly that grammar / exit 2" doctrine.

Fix

In scripts/check-silent-revert.sh:

• verify_known_incidents and ack_reason now use while read … || [[ -n "$var" ]] so a final unterminated row is processed rather than dropped.
• verify_known_incidents counts verified rows and dies (exit 2) when the count is zero — an empty or comments-only incident file is a broken input, not a pass.
• parse_attribution_field rejects a field that starts with, ends with, or contains a doubled comma before splitting, so a trailing comma is exit 2 like every other empty-entry position.

Test plan

bash scripts/check-silent-revert.test.sh — 112 passed, 0 failed. New cases pin the empty-file / comments-only floor, newline vs no-newline two-row parity (fixtures written with printf so the trailing newline is real), a one-row file with no trailing newline, an unterminated last ack row, and all four comma positions.

Verification

• bash scripts/check-silent-revert.test.sh — 112 passed, 0 failed
• shellcheck -x scripts/check-silent-revert.sh scripts/check-silent-revert.test.sh — clean
• New coverage:
  • empty and comments-only incident files exit 2 (not the success banner)
  • a two-row file without a trailing newline reaches the same FAIL as the same file with one (fixtures differ by exactly one trailing byte)
  • a one-row file with no trailing newline still verifies that row
  • an acknowledgment file whose last row has no trailing newline still clears
  • all four empty-entry comma positions ([sha=n,], [,sha=n], [sha=n,,sha=m], [,]) exit 2

Related

Refs #2843 (attribution grammar already merged; this PR only closes the remaining reader defects).
Refs #2833 (why the attribution field exists).
Refs #2808 (canary and incident corpus).

[claude]

Claude finished @kyle-sexton's task in 57s —— View job

---

Security review — no findings

• Gather context (PR diff, changed files)
• Read changed files (scripts/check-silent-revert.sh, scripts/check-silent-revert.test.sh)
• Apply /review:security-review criteria (skip gate + hunt)
• Post findings or clean bill of health

Skip gate hit: condition 2 — no security-relevant surface.

This PR only touches two bash read loops and a comma-grammar check in scripts/check-silent-revert.sh:

• ack_reason / verify_known_incidents (L446-460, L864-958): while read ... || [[ -n "$var" ]] now processes a final line lacking a trailing newline, instead of silently dropping it.
• verify_known_incidents gains a verified row counter and refuses with die (exit 2) when zero rows were verified (L962-968), closing the false-green path where an empty/comments-only/fully-dropped incident file printed a success banner.
• parse_attribution_field now rejects [sha=n,] / [,sha=n] / [sha=n,,sha=m] / [,] via an explicit glob check before the IFS=',' split (L838-847), closing the trailing-comma gap where word splitting drops a trailing empty field.

None of INCIDENTS_FILE / ACK_FILE are attacker-controlled — they're repo-committed data files read by a CI self-check (silent-revert-canary.yml), not derived from PR body/title, issue text, or any other untrusted input. The sha/culprit values extracted from those files are passed as literal arguments to git rev-parse --verify --quiet "${sha}^{commit}" (no eval, no unquoted expansion into a command position), so there's no injection surface even though the values come from a data file. The changes are stricter/fail-closed in every direction: more input shapes now die with exit 2 rather than silently passing.

No CRITICAL / IMPORTANT / SUGGESTION findings.

[github-actions]

Last security-reviewed head: 19a76b4d7b754c97ca1f4380111fb30e1b00b7e6. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

[claude]

Claude finished @kyle-sexton's task in 3m 52s —— View job

---

Claude finished the code review

• Gather context (PR body, changed files)
• Understand the request (invoke /review:code-review)
• Review scripts/check-silent-revert.sh diff
• Review scripts/check-silent-revert.test.sh diff
• Run test suite if possible
• Post findings

This repo also carries a claude-security-review workflow, so this lane's review is scoped to correctness/maintainability only; security-relevant findings (none applicable here) go to that lane.

Note on verification: the sandbox for this lane only allows gh/git/read-only tools, not arbitrary bash invocation, so I could not run bash scripts/check-silent-revert.test.sh myself. The PR's "111 passed, 0 failed" claim is author-reported and unverified by me — my findings below are from reading the diff and script logic directly.

Findings

One inline comment posted on scripts/check-silent-revert.test.sh:

• Test doesn't actually exercise the "with trailing newline" case it claims to (lines 1178–1183): with_nl="$(printf ... '\n')" — command substitution strips all trailing newlines from captured output, so with_nl never actually ends in \n. The following without_nl="${with_nl%$'\n'}" is then a no-op. Both inc-nl.txt and inc-nonl.txt end up written without a trailing newline, so the assertion "a two-row file without a trailing newline reaches the same FAIL as the same file with one" is comparing the no-newline case against itself rather than against a file that actually has one. This is the regression test guarding the exact read fix in verify_known_incidents, so it's worth fixing so it verifies what it claims — see the inline comment for a suggested fix.

Everything else checked out

• ack_reason and verify_known_incidents's while read ... || [[ -n "$var" ]] fix correctly closes the dropped-last-line gap, and is consistent with the pattern parse_incidents_file already used at line 1068 — no drift between the three readers of the same file format.
• The zero-row floor (((verified > 0)) || die ...) only counts fires/clean rows (marker rows and comments correctly excluded via continue before the increment), so it can't misfire on a legitimate marker-only-looking file that would already be invalid input for this subcommand.
• The trailing/leading/doubled-comma rejection in parse_attribution_field (case "$field" in *, | ,* | *,,*)) correctly rejects all four shapes in the PR's test matrix ([sha=n,], [,sha=n], [sha=n,,sha=m], [,]); the per-entry '') die ... check added alongside it is unreachable once the field-level case fires first, but it's harmless defensive redundancy, not a bug.
• No other while read loop in the script (lines 580, 716, 1309) shares the defect — those all read from internally-generated pipelines/temp files that are guaranteed to end in a newline (awk/printf output), not user-edited corpora, so they're correctly out of scope for this fix.

Branch: cursor/2874-2875-canary-rows-cfcf

[github-actions]

Claude has reviewed this PR 3 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[claude]

Claude finished @cursor[bot]'s task in 1m 27s —— View job

---

Code review — no new findings

• Gather context (PR diff, changed files)
• Review scripts/check-silent-revert.sh diff
• Review scripts/check-silent-revert.test.sh diff (incl. fix for prior inline comment)
• Post findings

This push (5ac7064) only touches scripts/check-silent-revert.test.sh, fixing the one finding from the prior review round on this PR: the "with trailing newline" fixture in t_replay_refuses_a_zero_row_or_truncated_file was being built via $(printf ...), which strips trailing newlines, so both fixtures ended up newline-less.

The fix (lines 1178–1183) now writes both fixtures directly with printf — printf '%s\n%s\n' ... >inc-nl.txt and printf '%s\n%s' ... >inc-nonl.txt — so inc-nl.txt genuinely carries the trailing newline that inc-nonl.txt lacks. It also adds a byte-count assertion (nl_bytes - nonl_bytes == 1, lines 1184–1189) that pins the two fixtures to differ by exactly the one trailing byte, so this specific class of regression (round-tripping through a $(...) that silently strips the newline) can't go quiet again. This is exactly right — it verifies what the test now claims.

The sh script itself (19a76b4) is unchanged since the last review pass and was already found clean: the while read ... || [[ -n "$var" ]] fix in ack_reason/verify_known_incidents, the zero-row floor, and the comma-grammar rejection in parse_attribution_field (with the SC2249 default *) arm now present) all read correctly and consistently.

Not independently verified: this lane's sandbox only permits gh/git/read-only tools, not arbitrary bash execution, so I could not run bash scripts/check-silent-revert.test.sh or shellcheck myself — same limitation as the prior pass. The PR's "112 passed, 0 failed" and shellcheck-clean claims are author-reported and unverified by me.

No CRITICAL / IMPORTANT / SUGGESTION findings on this diff.

Branch: cursor/2874-2875-canary-rows-cfcf

[claude]

Claude finished @cursor[bot]'s task in 46s —— View job

---

Code review — no new findings (skip gate)

• Gather context (PR body, changed files)
• Invoke /review:code-review and follow its instructions
• Confirm whether the diff changed since the last review on this lane
• Post findings

Skip gate hit: condition 4 — this head already has a successful review from this lane that still applies.

The current HEAD (abd925b) is a merge commit that only pulls in unrelated commits from main (138c545 test(planning) fixtures, f6b2bcb markdown-format doc fix). git diff 5ac7064 abd925b touches only docs/upstream/aihero-course.md and files under plugins/planning/— nothing in scripts/check-silent-revert.sh or scripts/check-silent-revert.test.sh, and no other file from this PR's changed-files list (scripts/check-silent-revert.sh, scripts/check-silent-revert.test.sh).

The prior review pass on 5ac7064 (this PR's actual content) already covered both files end-to-end:

• The while read ... || [[ -n "$var" ]] fix in ack_reason/verify_known_incidents
• The zero-row floor (((verified > 0)) || die ...)
• The comma-grammar rejection in parse_attribution_field (including the SC2249 default *) arm)
• The test-fixture fix for the trailing-newline regression test, plus its byte-count pin

That review found one issue (the $(printf ...) newline-stripping bug in the test fixture), which was fixed and re-reviewed clean. No new content to review here.

No CRITICAL / IMPORTANT / SUGGESTION findings.