silent-revert canary: verify_known_incidents passes with zero rows verified, and a file with no trailing newline silently loses its last row

[kyle-sexton]

Surfaced by fresh-context verification of #2843. Both defects are in the
row-reading loops, not in the detector, so they are independent of the
attribution work that PR adds.

Refs measured at. Findings 1 and 3 are pre-existing on main
(5958e1faf) — the same while read … done < FILE idiom sits at
check-silent-revert.sh:449 (verify_known_incidents) and :233
(ack_reason) there. Finding 2's demonstration needs the per-row
attribution assertion, which exists only on #2843's branch, so that transcript
pair was taken at 39ecf3088 (branch ccp-2843-fix), where the functions sit
at :595 and :264 and parse_attribution_field at :575. The underlying
newline behavior is identical on both refs; only the observable differs.

All repros use throwaway files outside the checkout via
SILENT_REVERT_INCIDENTS / SILENT_REVERT_ACK
(check-silent-revert.sh:192-193). The shipped corpus and acknowledgment
files were not touched.

1. There is no row-count floor: zero rows verified is a green run

verify_known_incidents loops the incident file and reports rc at the end.
Nothing anywhere counts how many rows it actually verified, so a file that
yields no rows falls straight through to the success banner:

$ SILENT_REVERT_INCIDENTS=<empty file> bash scripts/check-silent-revert.sh --verify-known-incidents

Canary reproduces every recorded incident at the shipped settings.
EXIT=0

A comments-only file is the same:

$ SILENT_REVERT_INCIDENTS=<file of two '#' lines> bash scripts/check-silent-revert.sh --verify-known-incidents

Canary reproduces every recorded incident at the shipped settings.
EXIT=0

Zero ok lines printed, and the run still asserts in its own words that the
canary "reproduces every recorded incident". The blank line above the banner is
the only visible difference from a real green run — the exit status, the
sentence, and the check row are identical.

This is the canary's self-proof lane, so the failure direction is a false
green in the one place a false green is least recoverable: the mechanism that
exists to certify the detector still works reports success when it verified
nothing at all.

2. A file with no trailing newline silently loses its last row

while read -r … done < FILE discards a final line that is not newline-
terminated: read returns non-zero at EOF even though it populated the
variables, so the loop body never runs for that row. The row is not reported,
not counted, not warned about — it simply does not exist as far as the replay
is concerned.

Demonstrated with a two-row temp incidents file whose row 2 carries
deliberately wrong attribution counts (=1 and =2 against the real 853
and 298). Two files, byte-identical except for the final newline:

$ wc -c two-rows-nl.txt two-rows-nonl.txt
277 two-rows-nl.txt
276 two-rows-nonl.txt

$ cmp -l two-rows-nl.txt two-rows-nonl.txt
cmp: EOF on two-rows-nonl.txt after byte 276

WITHOUT the trailing newline (276 bytes):

$ SILENT_REVERT_INCIDENTS=two-rows-nonl.txt bash scripts/check-silent-revert.sh --verify-known-incidents
ok   f603880da fires as recorded, 1 attribution(s) reproduced exactly  (row 1, correct)

Canary reproduces every recorded incident at the shipped settings.
EXIT=0

WITH the trailing newline (277 bytes), the same bytes plus \n:

$ SILENT_REVERT_INCIDENTS=two-rows-nl.txt bash scripts/check-silent-revert.sh --verify-known-incidents
ok   f603880da fires as recorded, 1 attribution(s) reproduced exactly  (row 1, correct)
FAIL cc58cbc53 fires, but NOT as recorded  (row 2, deliberately WRONG counts)
     recorded attribution:
       bfb66beb8c1a6cdb52957a9a2614c6d2593739dd 1
       eda5ae5ed5c2b54fdf2db5fd3b620a9272c46da5 2
     what the detector reported:
       bfb66beb8c1a6cdb52957a9a2614c6d2593739dd 853
       eda5ae5ed5c2b54fdf2db5fd3b620a9272c46da5 298
     A row that fires for the wrong reason is not a reproduction.
     Do NOT edit the row to match; find out why the attribution moved.

The canary no longer reproduces the incidents it was built for.
Do not relax the recorded expectations to make this pass.
EXIT=1

One byte is the difference between a caught regression and a green run. This
compounds with finding 1: dropping the only row of a one-row file lands
exactly on the zero-rows-verified green above.

3. ack_reason drops the acknowledgment file's last row too — fail-closed

ack_reason (:264 on the branch, :233 on main) uses the same idiom
against $ACK_FILE. Same two-file demonstration, 131 vs 130 bytes, with the
acked commit 6f0a31109 as row 2:

$ SILENT_REVERT_ACK=ack-nl.txt bash scripts/check-silent-revert.sh --commit 6f0a3110942375fb292112f5df5e2c39bbf56cb0
acknowledged 6f0a31109 fix(repo-fleet-hygiene): restore GraphQL merge evidence and rollups after #2633 (#2640)
             reviewed and cleared: row 2, the ack under test
EXIT=0

$ SILENT_REVERT_ACK=ack-nonl.txt bash scripts/check-silent-revert.sh --commit 6f0a3110942375fb292112f5df5e2c39bbf56cb0

SILENT REVERT SUSPECTED

  removed by   6f0a31109  fix(repo-fleet-hygiene): restore GraphQL merge evidence and rollups after #2633 (#2640)
  content from cc58cbc53  fix(repo-fleet-hygiene): report bare repos with live working trees (#2633)
  lines lost   447  (threshold 200, window 40 commits)
  [... finding body elided ...]
EXIT=1

Lower severity, deliberately stated: this direction is fail-closed. The
lost row means an acknowledged commit fires anyway — noise a human dismisses,
not a missed detection. It is filed with the other two because it is the same
one-line defect in the same file, and fixing one loop while leaving its twin is
how the second one gets forgotten.

What already mitigates this — and where the mitigations stop

• t_shipped_data_files_are_wellformed in scripts/check-silent-revert.test.sh
  enforces n >= 2 pinned rows, and on fix(scripts): read revert subjects and assert the canary's replay attributions #2843's branch additionally requires
  every shipped fires row to carry a well-formed attribution field. That
  closes findings 1 and 2 for the shipped corpus only — it reads
  $SELF_DIR/silent-revert-incidents.txt by path, never whatever
  SILENT_REVERT_INCIDENTS names.

• CI's editorconfig lane enforces insert_final_newline = true for [*]
  (.editorconfig:18; the only unset override is the lockfile section at
  :63-67), which keeps the shipped files newline-terminated in the tree.

  The nuance that matters: that lane lives in .github/workflows/ci.yml
  (:92-97), and silent-revert-canary.yml is deliberately not in
  ci.yml and not in its ci-status aggregate — the workflow's own header
  says so and explains why ("Adding it there would make a detection heuristic
  able to block merges"). So the newline guard is a property of a different
  workflow that the canary neither runs nor depends on. The canary's own lane
  enforces nothing about final newlines.

• Neither mitigation reaches a custom path. SILENT_REVERT_INCIDENTS and
  SILENT_REVERT_ACK are unbounded by both: no row floor, no newline
  enforcement, no grammar check. Every transcript above is a run at the shipped
  settings with only that variable changed.

Suggested fix

Both are one-liners, and the row floor is the one that matters:

• Count verified rows in verify_known_incidents and die (or fail) when the
  count is zero — an incident file that pins nothing is a broken input, not a
  pass. Consider asserting >= 1 fires row specifically, since a clean-only
  file also proves nothing about detection.
• Use while read -r … || [[ -n "$var" ]] (or equivalent) in both loops so a
  final unterminated row is processed rather than dropped.
• Worth a test in check-silent-revert.test.sh for each: a zero-row incident
  file must not report green, and a two-row file without a trailing newline
  must reach the same verdict as the same file with one.

Open threads

Two questions raised while measuring this that are not resolved here and
should not be folded into a fix silently.

a. Can GitHub's squash path produce a multi-line first paragraph?

declares_removal reads the subject as git log -1 --format=%B | head -n 1
(:216-219), while every report line prints --format=%s (:401, :501).
Those two differ in exactly one case: %s joins all lines of the first
paragraph with spaces, whereas %B | head -1 takes only the first physical
line. A two-line first paragraph is therefore the only way the detector's view
of the subject can diverge from the one the report shows a human.

Measured across all first-parent commits of origin/main at 5958e1faf —
comparing %s against the first line of %B in a single git log pass:

TOTAL_FIRST_PARENT=1541
DIVERGENCES=0

(The in-file comment at :216 cites 1527 for a related count; the history has
grown since.) So it is unreachable in this repo's history today. But the
squash message is hand-editable in GitHub's merge dialog, so nothing structural
prevents it — the mechanism is not disproved, only unobserved. Failure
direction is toward firing (the detector would fail to see a declaration
the report displays), which is safe, which is why this is a thread rather than
a finding.

b. Does a .gitattributes diff=<driver> reach the same code path as diff.external?

The script pins --no-ext-diff and check-silent-revert.test.sh:398-434
proves that pin is load-bearing against diff.external specifically. Nothing
in the script or its tests mentions .gitattributes, diff=<driver>, or
textconv, and a scan of #2843's comments turned up no answer either. Whether
a per-path diff driver declared in .gitattributes can reach the same
false-green the diff.external test demonstrates is recorded here as an open
question — not asserted in either direction.

Related

• Refs silent-revert canary: verify_known_incidents proves only that a commit still fires, not which culprit or how many lines #2833 — the replay validating only exit status per row. Orthogonal:
  that issue is about a single row asserting too little; this one is about
  zero rows and about a row that is never read at all. fix(scripts): read revert subjects and assert the canary's replay attributions #2843 is the fix in
  flight for silent-revert canary: verify_known_incidents proves only that a commit still fires, not which culprit or how many lines #2833 and does not touch either loop's read semantics.
• Refs fix(scripts): read revert subjects and assert the canary's replay attributions #2843 — the PR whose verification surfaced this; findings 1 and 3
  predate it, finding 2's transcript is taken on its branch.
• Refs silent-revert canary: the pinned clean row is not the closest sub-threshold miss, and its note names the wrong PR #2865 — sibling corpus-accuracy defects found the same way.
• Refs silent-revert canary: the incident corpus misattributes the #2633 revert to a PR that never merged #2831 — closed; prior art for corpus-data defects in this same file.
• Refs ci: add a post-merge silent-revert canary #2808 — the PR that introduced the canary and its replay.

[kyle-sexton]

Correction — transcript provenance.

Every transcript in this issue, including findings 1 and 3, was produced at
39ecf3088 on the #2843 branch, not at 5958e1faf. The opening paragraph
reads as though findings 1 and 3 were run on main; they were not. What is
pre-existing on main is the defect in both read loops
(check-silent-revert.sh:449 in verify_known_incidents and :233 in
ack_reason there) — the printed numbers are the branch's.

That matters for two figures a triager would otherwise read as contradictions:

• Finding 3 reports lines lost 447 for 6f0a31109, while the corpus and
  fix(scripts): re-derive the overlap calibration from the pinned detector's counts #2847 record 390 for the same commit. The difference is this branch's
  5e5a91a48 ("pin git blame too, so ambient config cannot silence the
  canary") plus the diff-flag pins — a differently-pinned measurement, not a
  changed measurement of the same thing.
• Finding 2's eda5ae5ed…=298 is the branch corpus's value for that
  attribution; main's silent-revert-incidents.txt records 301. Same
  cause.

Findings 1 and 3 reproduce on main as well — the loop idiom is identical
there — only the printed line counts differ. Finding 2's transcript pair is
branch-only as already stated, because the FAIL it demonstrates depends on the
per-row attribution assertion #2843 adds.

One clarification on open thread (b): #2843 carries 19 comments/reviews;
none of them mention .gitattributes, diff=, --no-ext-diff, textconv, or
a diff driver, so the question is genuinely unanswered rather than unchecked.

[kyle-sexton]

Second provenance note, and an update on open thread (b).

The worktree was dirty. The runs above were taken in a checkout at
39ecf3088 that also carried uncommitted edits from a parallel lane to
scripts/check-silent-revert.sh (+9/-10) and scripts/check-silent-revert.test.sh.
Stating it rather than leaving it inferred:

• Those edits do not touch verify_known_incidents, ack_reason, or
  parse_attribution_field. They add --no-show-signature to three git log
  calls, --no-ext-diff --no-textconv to attribute_file's git diff,
  --no-textconv to its git blame, and fix a missing \n in
  scan_commit's declared line. The read-loop defects in findings 1-3 are
  untouched by all of it.
• Every line number cited in the issue body — :192-193, :216, :264,
  :268, :401, :560-562, :569-570, :575, :580-581, :595, :603 —
  was verified against git show 39ecf3088:scripts/check-silent-revert.sh and
  matches that ref exactly. (In the dirty tree parse_attribution_field and
  verify_known_incidents sit one line lower.)
• The attribution counts are unaffected by the added pins: diff.bash.textconv
  and diff.markdown.textconv are both unset, and diff.external is unset, so
  --no-textconv / --no-ext-diff are no-ops here. The 853 / 298 the run
  reported match the branch corpus's own recorded values for cc58cbc53,
  which is the corroboration that matters.

Open thread (b) has moved — recording it rather than leaving it stale.

The issue body says nothing in the script or its tests mentions
.gitattributes or diff=<driver>. That was accurate at 39ecf3088, and the
underlying question turns out to be less hypothetical than it read:

.gitattributes:14-15 declares diff=bash for *.sh and *.bash — the exact
file class this canary scans — and :13 declares diff=markdown for *.md.
So per-path diff drivers are not a hypothetical this repo would have to opt
into; they are already declared. What makes them inert today is that neither
diff.bash.textconv nor diff.markdown.textconv is configured, so no textconv
runs. A developer (or a tool installer) setting either turns them live.

The parallel lane's in-flight uncommitted work in this worktree adds
--no-textconv to both the git diff and the git blame in attribute_file,
which is the pin that would close this. Flagging it here so the thread is
resolved deliberately in that work rather than left open in this issue — if
that change lands, thread (b) can be struck.

Thread (a) is unaffected by any of the above: it was measured with a single
git log pass over origin/main at 5958e1faf, not through the detector.

[kyle-sexton]

Claimed by lane 2874+2875 (same script: scripts/check-silent-revert.sh). Pairing with #2875.