fix(scripts): read revert subjects and assert the canary's replay attributions

[kyle-sexton]

Closes #2837. Closes #2833.

Summary

Three defects in the silent-revert canary, plus one unfiled harness-safety fix
in a file this change already owns. Branched off 2de57a379 (#2832), which had
already merged.

Fix

1. declares_removal() could not read the only revert subject that merges here (#2837)

The detector accepted three intent forms. This repo is squash-only with
squash_merge_commit_title: PR_TITLE, so the squash subject is the PR title,
and .github/workflows/pr-title.yml gates every title through a required
Conventional-Commits check whose default type list is all-lowercase — it admits
revert: and contains nothing a Revert "…" subject could match. So a
deliberate revert reached main wearing a subject the detector could not read.

declares_removal() now also accepts the Conventional-Commits revert type,
anchored at the start of the subject, requiring the literal lowercase token,
its optional (scope) and/or !, its colon, and a non-empty description:

^revert(\([^()]+\))?!?:[[:space:]]*[^[:space:]]

Never a substring search for "revert" — kept exactly as constrained as the three
forms beside it.

Before / after on the repo's one real deliberate revert

1d1fca6e8 — revert: remove Cursor dual-target marketplace manifests (#1835) (#1839)

Before (at 2de57a379, shipped detector, thresholds unmodified):

$ bash scripts/check-silent-revert.sh --commit 1d1fca6e8

SILENT REVERT SUSPECTED

  removed by   1d1fca6e8  revert: remove Cursor dual-target marketplace manifests (#1835) (#1839)
               2026-07-30 19:41:47 -0400
  content from b6c4b58f3  feat: add Cursor dual-target marketplace manifests (#1835)
               2026-07-30 18:17:36 -0400  (1 commit(s) earlier on main)
  lines lost   3361  (threshold 200, window 40 commits)
  [... 102 files, sample block and "What to do" block elided; 130 lines total ...]

EXIT=1

After:

$ bash scripts/check-silent-revert.sh --commit 1d1fca6e8
declared 1d1fca6e8 revert: remove Cursor dual-target marketplace manifests (#1835) (#1839)
         removal is declared: the subject carries the Conventional-Commits revert type
EXIT=0

Suppression is not widened over anything the corpus records

• No recorded incident has a revert-prefixed subject — f603880da
fix(disk-hygiene): …, 9239f1541 feat(disk-hygiene): …, cc58cbc53
fix(repo-fleet-hygiene): …, c8470efd0 docs(conventions): …. All four rows
  still hold (green run below).
• Neither acknowledgment-file commit is revert-prefixed either — 6f0a31109
fix(repo-fleet-hygiene): …, 91e77fc16 fix(hook-utils): … — so no ack row
  goes dead now that declares_removal() short-circuits ahead of ack_reason().
• The header's "fires on 5 commits — 1%" calibration figure is therefore
  unchanged; none of those five is revert-prefixed.

New tests pin both directions. revert:, revert(scope):, revert!: and
revert(scope)!: suppress; feat: do not revert the alpha guard (#99),
reverted: drop the alpha guard (#99), Revert: drop the alpha guard (#99) and
a bare revert: with no description all still fire. The pre-existing case only
covered a body mention of "revert"; the subject is what the new form reads, so
that is where a substring bug would widen.

2. verify_known_incidents asserted only "something fired" (#2833)

A fires row passed on scan_commit's exit status while the note beside it
named a specific culprit and a specific line count that nothing checked. On
cc58cbc53, whose deletions trace to two culprits, losing the eda5ae5ed
attribution entirely would still have printed ok on the surviving bfb66beb8
finding — the canary announcing a reproduction it did not perform.

A fires row may now carry a bracketed attribution expectation after its sha:

fires <sha> [<culprit-full-sha>=<blamed-lines>,<culprit-full-sha>=<blamed-lines>] <note>

and the replay asserts the run's findings are exactly that set — same
culprits, same per-culprit counts, no extras, no omissions. Full 40-character
culprit shas only, the same discipline silent-revert-acknowledged.txt uses.

Counts come from a new FINDINGS_SINK file that report_finding appends
<full-culprit-sha> <count> to, not from scraping the human report — the report
prints a 9-character abbreviation, which is not enough sha to assert on. Nothing
else sets FINDINGS_SINK, so --commit and range mode are byte-identical.

Every recorded figure was measured, not transcribed from the notes — the
sink was wired first and the observed values recorded:

f603880da -> a95f240f7502ff9f1e5411602e79ccbd4b9056c0 346
9239f1541 -> f603880dad4003477ec7cac27654fce92c75eec8 451
cc58cbc53 -> bfb66beb8c1a6cdb52957a9a2614c6d2593739dd 853
             eda5ae5ed5c2b54fdf2db5fd3b620a9272c46da5 298

Three of the four agree with the notes #2832 corrected; the fourth is 298 rather
than 301, for the reason in section 5. A malformed field is exit 2 (cannot
run), never a FAIL and never a pass — a silently misread expectation is the same
false green this file exists to remove. The field is optional so a row can be
pinned before its attribution is measured, but a new well-formedness assertion
requires every shipped fires row to carry one.

3. The workflow header stated a reason that was not true (unfiled)

.github/workflows/silent-revert-canary.yml asserted "There is no
pull_request trigger, so it can never gate a PR" — while its own on: block
has a paths-filtered pull_request trigger, added by #2808 to run the detector's
unit tests, and explained at length 20 lines further down in the same file. The
conclusion holds for a different reason: both scan steps are gated
if: github.event_name != 'pull_request', and the lane sits outside ci.yml
and its ci-status aggregate. Wording corrected to the actual reason.

No trigger and no if: changed. git diff on that file is comment lines
only — one hunk, @@ -13,6 +13,9 @@, entirely inside the # header.

4. The detector's line counts depended on ambient git config (unfiled, found by #2833's new assertion)

The first CI run of this branch went red, and the failure is the most valuable
thing in this PR. The runner reported the eda5ae5ed attribution as 298
lines where my machine measured 301:

FAIL cc58cbc53 fires, but NOT as recorded
     recorded attribution:   eda5ae5ed…  301
     what the detector reported:  eda5ae5ed…  298

Cause: attribute_file called bare git diff --unified=0, so it inherited
whatever diff.algorithm the caller's config carried. I have
diff.algorithm = histogram set globally; CI has nothing set and therefore uses
git's default myers. The algorithm changes which lines a hunk calls deleted,
so it changes the per-culprit counts this canary thresholds on. Reproduced
directly:

$ GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=diff.algorithm GIT_CONFIG_VALUE_0=myers \
    FINDINGS_SINK=… scripts/check-silent-revert.sh --commit cc58cbc53…
bfb66beb8…  853
eda5ae5ed…  298      # 301 under histogram

Three lines is harmless in itself. The principle is not: the same drift can
carry a count across the 200-line threshold, so a commit could fire on one
machine and stay silent on another, and the header's "fires on 5 commits over
500 — 1%" calibration only ever described one algorithm.

attribute_file now pins --diff-algorithm=myers -M explicitly, and the file
enumeration pins -M too. Both are git's defaults, so this does not change
what CI detects today — CI already had no diff.algorithm set. It makes a
local run match CI, not the reverse. -M covers the same exposure for rename
detection, which the existing header calls "load-bearing rather than
incidental": diff.renames = false in a developer's config would decompose a
git mv into delete + add and make relocating a large recent file fire.

The recorded figure is now 298, and the prose figures in the script header
and the corpus are corrected with the refutation attached, so nobody
re-measuring on a histogram machine "corrects" it back to 301.

Worth stating plainly: nothing asked for this. #2833's exact-count assertion
turned a silent, config-dependent divergence into a red build on its first run —
which is precisely the argument for asserting attributions instead of exit
status.

5. The test harness could commit the developer's work as test <t@t.test> (unfiled)

Found the hard way while developing this. mk_repo is called as
repo="$(mk_repo)", so a return 1 inside the command substitution cannot abort
the suite — the caller just gets "". And "" is not inert: git -C "" is
documented as a no-op, so the next add -A + commit staged and committed my
uncommitted work into the checkout, authored test <t@t.test>. Such a commit
cannot be pushed here — it fails required_signatures with no_user.

mk_repo now yields a path derived from SELF_DIR that does not exist, so every
git call against it fails loudly and the assertions go red — fail-closed, which
is what a harness that cannot build its fixture should do. Scoped to this file
only; if the same repo="$(mk_repo)" shape exists in sibling harnesses that is a
separate follow-up.

Verification

scripts/check-silent-revert.sh --verify-known-incidents, run on a machine with
diff.algorithm = histogram set globally — it now agrees with CI exactly,
because the flags are pinned:

ok   f603880da fires as recorded, 1 attribution(s) reproduced exactly  (#2639 dropped #2635 (346 blamed lines), 13 minutes later)
ok   9239f1541 fires as recorded, 1 attribution(s) reproduced exactly  (#2641 dropped #2639 (451 blamed lines), 10 minutes later)
ok   cc58cbc53 fires as recorded, 2 attribution(s) reproduced exactly  (#2633 dropped #2644's rollups (853 blamed lines) and #2642's GraphQL merge evidence (298); already recorded in #2656)
ok   c8470efd0 stays clean as recorded  (docs(conventions) rewrote 129 lines of a doc #2679 had just added)

Canary reproduces every recorded incident at the shipped settings.

Injected failure against the shipped corpus (853 changed to 852 in a
copy) — the mechanism is proven on real rows, not only on synthetic fixtures:

$ SILENT_REVERT_INCIDENTS=<copy with 298 -> 297> scripts/check-silent-revert.sh --verify-known-incidents
ok   f603880da fires as recorded, 1 attribution(s) reproduced exactly  (...)
ok   9239f1541 fires as recorded, 1 attribution(s) reproduced exactly  (...)
FAIL cc58cbc53 fires, but NOT as recorded  (#2633 dropped #2644's rollups (853 blamed lines) and #2642's GraphQL merge evidence (298); already recorded in #2656)
     recorded attribution:
       bfb66beb8c1a6cdb52957a9a2614c6d2593739dd 853
       eda5ae5ed5c2b54fdf2db5fd3b620a9272c46da5 297
     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.
ok   c8470efd0 stays clean as recorded  (...)

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

The commit still fires — exit status alone would have passed this row. Note the
row is red on a one-line discrepancy in one of two attributions, which is
exactly the regression #2833 describes.

scripts/check-silent-revert.test.sh: 49 passed, 0 failed, including
replay fails when the finding is attributed to a different culprit,
replay fails when the recorded line count no longer reproduces, and
replay fails when one of two recorded attributions stops reproducing. So the
assertion is proven by permanent tests, not only by a one-off injection.

6. Review follow-up: an unterminated attribution field read as absent

Both automated review lanes independently flagged the same real gap, and they
were right. A fires row whose field opened with [ but never closed it failed
the [[ "$rest" == \[*\]* ]] glob, so attribution stayed empty, the remainder
became free-text note, and the row fell back to passing on exit status alone —
reintroducing the exact pre-#2833 gap by the one route nobody would look at, and
contradicting the contract documented directly above it.

A leading [ now COMMITS the row to carrying an attribution; unterminated takes
the malformed path. Reproduced against the real corpus with the closing bracket
stripped from the f603880da row:

check-silent-revert: unterminated attribution field for f603880dad… (no closing ']'): [a95f240f75…=346 #2639 dropped #2635 …
EXIT=2

The four malformed shapes already pinned all carried a closing ], so this one
was untested; [<sha>=40 and a bare [ are now pinned too. The shipped corpus
was never at risk — t_shipped_data_files_are_wellformed's regex covers it —
but that is a separate layer and does not hold for a custom
SILENT_REVERT_INCIDENTS.

7. Verification follow-up: git blame was still ambient-config dependent

Fresh-context verification of section 4 found that fix was only half of one.
Pinning the diff flags left attribute_file's git blame call bare, so
blame.ignoreRevsFile — an ordinary setting in any repo carrying a
bulk-reformat commit — still decided the per-culprit counts the replay now
asserts on. On the real corpus, with that setting naming bfb66beb8:

culprit	pinned	with blame.ignoreRevsFile
bfb66beb8…	853	259
eda5ae5ed…	298	322

On a synthetic fixture it is worse than a wrong number. With the pin absent and
that config present, the detector reports no finding at all on a genuine
silent revert:

	clean config	hostile config
pinned	culprit 40	culprit 40
unpinned	culprit 40	(nothing — the canary goes silent)

That is a false green reached through the developer's own gitconfig — the
precise failure this canary exists to remove.

The obvious fix does not work, and the comment says so.
-c blame.ignoreRevsFile= does not clear it: the documented "an empty file
name resets the list" applies to the option, and the -c form was measured
leaving the hostile value fully in effect (853 → 259 with the reset supposedly
applied). Only --no-ignore-revs-file actually resets. The symmetry with the
-c pins above is wrong here and is deliberately not used.

t_counts_are_immune_to_ambient_git_config pins the property: the same fixture
scanned twice, once under a hostile GIT_CONFIG_GLOBAL setting
blame.ignoreRevsFile, diff.algorithm and diff.renames, asserting identical
exit status and identical per-culprit findings. Confirmed discriminating —
with the blame pin stripped it fails, and it fails because the hostile run
reports nothing at all.

shellcheck, actionlint, typos, bash -n and
scripts/check-shell-portability.sh all pass.

Test plan

Run from a clean checkout of this branch, with unset GIT_DIR GIT_WORK_TREE:

1. bash scripts/check-silent-revert.test.sh — the detector's own unit suite.
   Expect 49 passed, 0 failed. Covers all four revert: spellings, the
   four negative subject cases (feat: do not revert ..., reverted:,
   Revert:, bare revert:), the wrong-culprit / wrong-count / missing-
   attribution replay regressions, all six malformed-field shapes, and
   t_counts_are_immune_to_ambient_git_config.
2. bash scripts/check-silent-revert.sh --verify-known-incidents — the real
   corpus. Expect exit 0 with all three fires rows reporting
   attribution(s) reproduced exactly and the clean row staying clean.
3. Negative control for step 2: edit scripts/silent-revert-incidents.txt to
   inject a wrong culprit sha (leaving the count correct, so the commit still
   fires) and, separately, a wrong line count. Each must exit 1 with
   fires, but NOT as recorded. Restore the file afterwards.
4. Set diff.algorithm = histogram in global git config and repeat steps 1-2.
   The numbers must not move — that is what the new flag pins buy.
5. git diff origin/main...HEAD -- .github/workflows/silent-revert-canary.yml
   must show comment-only changes; no executable YAML line may differ.

Related

• Refs ci: add a post-merge silent-revert canary #2808 — the PR that merged the canary and its three intent forms.
• Refs fix(scripts): correct the silent-revert incident corpus attribution #2832 / silent-revert canary: the incident corpus misattributes the #2633 revert to a PR that never merged #2831 — the corpus-attribution correction this branches off;
  silent-revert canary: verify_known_incidents proves only that a commit still fires, not which culprit or how many lines #2833 was raised in its review.
• Refs Stale-base squash merges silently reverted two merged fixes (CI green, issues left closed) #2691 — the original silent-revert audit the corpus is built from.
• Refs revert: remove Cursor dual-target marketplace manifests (#1835) #1839 — the deliberate revert (1d1fca6e8) used as the real-history
  fixture for silent-revert canary: declares_removal() does not recognize a revert: subject, the only revert spelling the PR-title gate permits #2837.

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[claude]

Claude finished @kyle-sexton's task in 1m 38s —— View job

---

Security review complete

• Gather context (PR diff, changed files)
• Review scripts/check-silent-revert.sh changes for security-relevant logic
• Review scripts/check-silent-revert.test.sh changes
• Review .github/workflows/silent-revert-canary.yml changes
• Review scripts/silent-revert-incidents.txt changes
• Post findings

No security issues found.

Summary of what was reviewed:

• scripts/check-silent-revert.sh: the new revert:/revert(scope):/revert!: matching in declares_removal() (around L217-L239) is an anchored regex against a commit subject sourced from local git history — no substring widening, no ReDoS-prone construct, no eval/dynamic command execution anywhere in the file. The new parse_attribution_field() and verify_known_incidents() logic (L512-L620) validates shas/counts with strict regexes before use, and all variables ($sha, $culprit, $note, etc.) are quoted in git/printf/test contexts — no injection surface. FINDINGS_SINK is only ever set internally via mktemp, never from an externally influenced value.
• .github/workflows/silent-revert-canary.yml: diff is comment-only (header prose correcting the stated rationale for the pull_request trigger). No change to on:, permissions: (contents: read, unchanged), trigger gating, or if: conditions — the pull_request lane still only runs unit tests, gated off from the scan steps, and stays outside ci-status.
• scripts/check-silent-revert.test.sh: the mk_repo fail-closed fix (routes a mktemp -d/init failure to a guaranteed-nonexistent path derived from SELF_DIR instead of returning "", which previously risked git -C "" operating on the invoker's own working tree) is a harness-safety improvement, not a regression — it closes a real (if CI-internal, not attacker-reachable) footgun rather than introducing one.
• scripts/silent-revert-incidents.txt: data-only change (new bracketed attribution fields and corrected prose), no executable content.

This canary runs post-merge on push to main only (requires write access to trigger the scan path) and processes only the repo's own trusted git history — there's no untrusted external input in the code paths this PR touches.

[claude]

Claude finished @kyle-sexton's task in 5m 1s —— View job

---

Code review complete

• Read trigger instructions and gather PR context
• Invoke /review:code-review (skill tool errored in this session — followed its SKILL.md instructions manually) and read the four changed files
• Post findings as inline comments where anchorable to a changed line
• Post summary review comment

Scope

Reviewed all four changed files against origin/main...HEAD: the two declares_removal()/verify_known_incidents changes in scripts/check-silent-revert.sh, the matching corpus rows in scripts/silent-revert-incidents.txt, the new/updated tests in scripts/check-silent-revert.test.sh, and the comment-only fix in .github/workflows/silent-revert-canary.yml. This repo carries .github/workflows/claude-security-review.yml, so per REVIEW.md this lane omits security findings — none were relevant here regardless.

Findings

One inline finding posted on scripts/check-silent-revert.sh:559 (🟡 Nit): the attribution-field bracket detection ([[ "$rest" == \[*\]* ]]) only recognizes an attribution attempt when the row contains a closing ] somewhere. A row whose attribution is typo'd with no closing bracket silently degrades to "no attribution, pass on exit status alone" instead of the documented "malformed ⇒ exit 2, never a pass" guarantee — a shape the four new malformed-field tests don't cover (they all include the closing bracket). Narrow impact: the shipped corpus is incidentally protected by the separate t_shipped_data_files_are_wellformed well-formedness regex, so this doesn't threaten the current corpus, only a hypothetical future/custom $INCIDENTS_FILE.

Everything else checked out on read-through:

• The new Conventional-Commits revert:/revert(scope):/revert!:/revert(scope)!: regex (scripts/check-silent-revert.sh:220-238) is correctly anchored at the subject start, case-sensitive, and requires a non-empty description — verified it rejects reverted:, Revert:, and a bare revert:, matching the new negative tests.
• verify_known_incidents's attribution parsing/comparison (sort + cmp -s against FINDINGS_SINK output) is sound; report_finding's sink write is correctly scoped so --commit/range mode are unaffected when FINDINGS_SINK isn't set.
• The mk_repo fail-closed fix in the test harness (scripts/check-silent-revert.test.sh:50-80) correctly keeps the real tmp dir in TMPDIRS for cleanup even on the git_init_test_repo failure path, and every caller already treats repo="$(mk_repo)" as unconditionally succeeding, so no caller depended on the old nonzero-return semantics.
• The workflow header diff (.github/workflows/silent-revert-canary.yml) is comment-only, as claimed — confirmed via git diff origin/main...HEAD -- .github/workflows/silent-revert-canary.yml.

Not independently verified

This checkout is shallow (2 commits of history), so I could not run scripts/check-silent-revert.sh --verify-known-incidents or scripts/check-silent-revert.test.sh myself, and shellcheck/gh/git fetch all required approval this session denied. The PR body's reported command output (the before/after repro, the 46-passed test run, the injected-failure demo, and the measured attribution counts) is author-claimed and unverified by me — flagging per review policy rather than restating it as independently confirmed. If it's useful to have these actually re-run, granting this lane shellcheck/bash/unattended git fetch would let a future pass do so.
· Branch

[github-actions]

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d8e43cfd31

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[github-actions]

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

[claude]

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

---

Code review complete

• Read trigger instructions and gather PR context
• Load /review:code-review skill instructions
• Check skip gate — PR open, not draft, not trivial; this head (167d5aa) has not been reviewed by this lane before (prior reviews in the thread are pinned to d8e43cfd, the commit before the last push)
• Diff PR against origin/main, read all four changed files
• Review against CLAUDE.md/REVIEW.md criteria (correctness + maintainability; security stays with the dedicated claude-security-review lane per REVIEW.md's split)
• Post inline finding
• Post summary

Scope

Reviewed the full diff against origin/main...HEAD (395/-28 across 4 files), with particular attention to what the latest commit (167d5aa, "pin the canary's diff flags…") changed since the two earlier review passes on this PR, both pinned to the prior head d8e43cfd.

Findings

One inline finding, posted on scripts/check-silent-revert.sh:591 — this reiterates a finding both Codex and this lane's own previous pass already raised on this PR, since the latest commit didn't touch this code path and it remains unaddressed:

[[ "$rest" == \[*\]* ]] only recognizes an attribution field when a closing ] appears somewhere in the row's remainder. A fires row whose attribution is truncated or typo'd with no closing bracket silently degrades to "no attribution, pass on exit status alone" instead of the documented "malformed ⇒ exit 2, never a pass" guarantee. The four new malformed-field test cases (scripts/check-silent-revert.test.sh:592) all include a closing bracket, so this shape still isn't pinned by a test. Narrow blast radius — the shipped corpus is protected indirectly by t_shipped_data_files_are_wellformed's separate regex — so this only threatens a hypothetical custom $INCIDENTS_FILE.

Everything else checked out on read-through

• declares_removal()'s new Conventional-Commits revert:/revert(scope):/revert!:/revert(scope)!: regex (scripts/check-silent-revert.sh:225-239): correctly anchored at the subject start, case-sensitive, requires a non-empty description. Traced through the new positive/negative tests (t_conventional_revert_subject_is_declared, the expanded t_prose_mentioning_revert_still_fires) and they match the claimed behavior — reverted:, Revert:, and a bare revert: correctly still fire.
• The --diff-algorithm=myers -M pinning added in the latest commit (scripts/check-silent-revert.sh:280-337, scripts/check-silent-revert.sh:421): both flags are stated and confirmed to be git's actual defaults, so this is a no-op for what CI detects and only removes dependence on ambient diff.algorithm/diff.renames config — sound fix, correctly reasoned in the header/inline comments.
• verify_known_incidents's attribution parsing/comparison (parse_attribution_field, sort + cmp -s against FINDINGS_SINK): validates full 40-char shas and numeric counts before use, dies via die() (not a subshell) as documented, and report_finding's sink write is correctly scoped so --commit/range mode stay unaffected when FINDINGS_SINK isn't set.
• mk_repo's fail-closed fix (scripts/check-silent-revert.test.sh:62-80): on mktemp/git_init_test_repo failure, returns a guaranteed-nonexistent path immediately (via return 0 inside the failure branch, not falling through the rest of the function), and correctly keeps a successfully-created tmpdir in TMPDIRS for cleanup even when the later git_init_test_repo call fails.
• .github/workflows/silent-revert-canary.yml: confirmed via git diff that the change is comment-only — no on:/if:/permissions: changes — and the corrected prose matches the file's actual trigger/gating structure.
• scripts/silent-revert-incidents.txt: data-only change; the corrected 298 figure (from 301) is consistent with the code-side pinning fix and the accompanying prose correctly documents why the number changed rather than silently rewriting it.

Not independently verified

Did not re-run scripts/check-silent-revert.sh --verify-known-incidents or scripts/check-silent-revert.test.sh myself in this session — the PR body's reported command output (before/after repro, 46-passed test run, injected-failure demo, measured attribution counts) is author-claimed and unverified by me here. The bracket-detection issue above was confirmed by static/logical analysis of the bash glob semantics ([[ "$rest" == \[*\]* ]] requires a ] to exist in $rest, which a truncated field lacks), not by executing the harness in this session.

[claude]

Claude finished @kyle-sexton's task in 1m 19s —— View job

---

Security review complete

• Gather context (PR diff, changed files)
• Invoke /review:security-review and follow its instructions
• Post findings

No security issues found.

Scope

This is a synchronize re-review. The last security-reviewed head was d8e43cfd3165658; the incremental diff since then is a single follow-up commit (167d5aa, "pin the canary's diff flags so its line counts stop depending on ambient config"), touching only scripts/check-silent-revert.sh and scripts/silent-revert-incidents.txt.

What changed

• scripts/check-silent-revert.sh#L338 and #L424: attribute_file() and scan_commit() now pin --diff-algorithm=myers -M explicitly on the git diff calls used to compute blame attributions and rename-aware file enumeration, instead of inheriting whatever diff.algorithm/diff.renames a caller's global git config happens to set.
• scripts/silent-revert-incidents.txt: the recorded attribution for the cc58cbc53 incident's second culprit changes from 301 to 298 lines, matching the re-measurement under the now-pinned myers algorithm (301 was the same commit measured under diff.algorithm = histogram).
• Both flags being pinned (myers, -M) are already git's own defaults — this changes what a run inherits, not what CI actually computes; it makes local reproduction match CI rather than the reverse.

Assessment

Skip-gate criterion 2 applies: no security-relevant surface. Both git diff invocations already operated on trusted local shas/filenames sourced from the repo's own history (unchanged in this commit — only two literal flags were added, no new variable interpolation or command construction), so there's no new injection surface, no permission or trust-boundary change, and no untrusted input newly in play. The rest of the diff is comments and a corpus data-file constant. This lane's earlier full review of the prior head (d8e43cfd3) also found nothing, and nothing in this incremental commit changes that conclusion.
· Branch

[kyle-sexton]

Both review lanes raised the same finding at check-silent-revert.sh:591 (5 threads, one finding). It is correct and now fixed.

A fires row whose attribution field opened with [ but never closed it failed the [[ "$rest" == \[*\]* ]] glob, so attribution stayed empty, the remainder became free-text note, and the row fell back to passing on exit status alone — reintroducing the exact pre-#2833 gap this PR closes, by the one route nobody would think to look at, and contradicting the contract documented a few lines above ("A malformed field is exit 2 ... never a FAIL and never a pass").

A leading [ now COMMITS the row to carrying an attribution; an unterminated field takes the malformed path:

if [[ "$rest" == \[* ]]; then
  [[ "$rest" == \[*\]* ]] ||
    die "unterminated attribution field for $sha in $INCIDENTS_FILE (no closing ']'): $rest"
  ...
fi

Reproduced against the real corpus with the closing bracket stripped from the f603880da row:

$ SILENT_REVERT_INCIDENTS=<copy, closing ] removed> scripts/check-silent-revert.sh --verify-known-incidents
check-silent-revert: unterminated attribution field for f603880dad4003477ec7cac27654fce92c75eec8 in ... (no closing ']'): [a95f240f7502ff9f1e5411602e79ccbd4b9056c0=346 #2639 dropped #2635 ...
EXIT=2

Two malformed shapes added to t_replay_asserts_the_recorded_attribution covering exactly this: [<sha>=40 (unterminated) and a bare [. The reviewers were right that the four existing cases all carried a closing ], so the shape was unpinned — and right that t_shipped_data_files_are_wellformed's regex only covered the shipped corpus, not a custom SILENT_REVERT_INCIDENTS.

[claude]

Claude finished @kyle-sexton's task in 1m 28s —— View job

---

Code review complete

• Read trigger instructions and gather PR context
• Invoke /review:code-review skill and follow its instructions
• Diff PR against origin/main, read all four changed files
• Post inline findings (none — see below)
• Post summary

Scope

This is a synchronize re-review. The last review of this lane covered head 167d5aa. The only change since then is the new commit b478989 ("treat an unterminated attribution field as malformed, not absent"), touching scripts/check-silent-revert.sh and scripts/check-silent-revert.test.sh — nothing else in the PR changed.

What changed

This commit fixes the exact bracket-detection gap that both Codex and this lane's two prior passes raised on check-silent-revert.sh (previously anchored around line 591):

• scripts/check-silent-revert.sh#L588-L605: the field-detection condition changes from [[ "$rest" == \[*\]* ]] (silently falls through to "no attribution" when the closing ] is missing) to a two-step check — [[ "$rest" == \[* ]] alone now commits the row to carrying an attribution, and a missing ] after that takes the die() path with a clear message, rather than silently degrading to a free-text note and a pass-on-exit-status-alone.
• scripts/check-silent-revert.test.sh#L586-L603: two new malformed shapes added to the loop — "[$culprit=40" (unterminated with content) and "[" (bare bracket) — both previously unpinned, now asserting rc=2.

Verification (static read-through)

• Confirmed die() (scripts/check-silent-revert.sh#L195-L198) exits 2, matching the documented "malformed ⇒ exit 2, never FAIL, never pass" contract.
• Confirmed the while read ... done <"$INCIDENTS_FILE" loop (scripts/check-silent-revert.sh#L579, #L654) is a plain file redirection, not a subshell/pipeline — so die() inside it correctly terminates the whole script rather than only the loop.
• Traced both new test shapes through the new logic by hand: "[$culprit=40" and "[" each start with [ and contain no ], so both now hit the new die() branch — this is exactly the shape the prior reviews (this lane, twice, and Codex) flagged as unpinned.
• No other logic in verify_known_incidents or parse_attribution_field was touched by this commit; the rest of the PR (the declares_removal() revert-subject matching, the --diff-algorithm=myers -M pinning, the mk_repo fail-closed fix, and the workflow header comment fix) is unchanged since the prior full passes on this PR, which found those sound.

No new findings. The previously-raised finding (bracket-detection trapdoor) is fixed correctly and is now covered by a permanent test.

Not independently verified

Could not execute bash scripts/check-silent-revert.test.sh or --verify-known-incidents in this session (no tool access to run shell commands here) — the fix above is verified by static/logical read-through of the glob semantics and the surrounding control flow, not by running the harness.
· Branch

[claude]

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

---

Security review complete

• Read trigger instructions and gather PR context
• Invoke /review:security-review and follow its instructions
• Post findings

No security issues found.

Scope

This is a synchronize re-review. The last security-reviewed head was 167d5aa2ba61c5862df24f0368ec55874f6653ff; the incremental diff since then is the single new commit b478989 ("treat an unterminated attribution field as malformed, not absent"), touching only scripts/check-silent-revert.sh and scripts/check-silent-revert.test.sh.

What changed

• scripts/check-silent-revert.sh#L586-L604: fixes the bracket-detection bug both automated lanes raised earlier on this PR. The [[ "$rest" == \[*\]* ]] check (requiring a closing ] before treating a row as carrying an attribution at all) is replaced with a two-step check — [[ "$rest" == \[* ]] now commits the row to carrying an attribution, and die() fires with a descriptive message if no closing ] is found, instead of silently falling back to "no attribution, pass on exit status alone."
• scripts/check-silent-revert.test.sh#L584-L596: adds two new malformed-shape cases ([$culprit=40 and a bare [) to t_replay_asserts_the_recorded_attribution, pinning the previously-untested unterminated-bracket path.

Assessment

Skip-gate criterion 2 applies: no security-relevant surface. die() (scripts/check-silent-revert.sh#L195-L198) only echos $* to stderr and exits — the newly-added die "unterminated attribution field for $sha in $INCIDENTS_FILE (no closing ']'): $rest" call introduces no eval/dynamic execution, and $sha/$INCIDENTS_FILE/$rest are all quoted, so there's no injection surface. This change tightens validation (a malformed field now correctly fails closed with exit 2 instead of silently falling back to a pass) — it closes a validation gap rather than opening one, and it operates on the same trusted local-file input ($INCIDENTS_FILE, sourced from the repo or a SILENT_REVERT_INCIDENTS override under the operator's own control) already in scope for the prior full review of this file, which also found nothing.
· Branch

[claude]

Claude finished @kyle-sexton's task in 6m 1s —— View job

---

Code review complete

• Read trigger instructions and gather PR context
• Invoke /review:code-review skill and follow its instructions (skill tool errored in this session again; followed plugins/review/skills/code-review/SKILL.md directly, as prior passes on this PR did)
• Diff PR against origin/main, read all four changed files
• Post inline findings (none — see below)
• Post summary

Scope

This is a synchronize re-review. The last review of this lane (and the security lane) covered head b478989. The only change since then is the new commit 5e5a91a — "pin git blame too, so ambient config cannot silence the canary" — touching only scripts/check-silent-revert.sh and scripts/check-silent-revert.test.sh (+90/-11). Nothing else in the PR changed since the prior full passes, which already found the declares_removal() revert-subject matching, the --diff-algorithm=myers -M diff pinning, the bracket-detection fix, the mk_repo fail-closed fix, and the workflow header comment fix sound.

What this commit does

Extends the ambient-config immunity already established for git diff (167d5aa) to git blame: attribute_file() now calls git blame --no-ignore-revs-file --line-porcelain ... (scripts/check-silent-revert.sh#L385-L386), so a caller's blame.ignoreRevsFile (which reassigns authorship away from listed commits) can no longer change the per-culprit counts the replay asserts on. A new test, t_counts_are_immune_to_ambient_git_config (scripts/check-silent-revert.test.sh#L264-L304), pins the property directly by scanning the same fixture twice — once clean, once under GIT_CONFIG_GLOBAL pointed at a hostile config setting blame.ignoreRevsFile, diff.algorithm=histogram, and diff.renames=false — and asserting identical exit status and identical FINDINGS_SINK contents.

Read-through verification

• The --no-ignore-revs-file flag addition is a minimal, correctly-scoped change to an existing call; option ordering and quoting are unaffected.
• The new test correctly reuses the established run_canary/prefix-env-var idiom already documented in this file (FINDINGS_SINK="..." SILENT_REVERT_THRESHOLD=20 run_canary ...) for GIT_CONFIG_GLOBAL, which is sound for the same reason: the prefix assignment on a shell-function call is inherited by the child process the function launches.
• culprit is captured as the commit that adds the block later dropped, matching how blame.ignoreRevsFile's target needs to be identified; the hostile config's ignore-revs.txt correctly lists that sha.
• The assertion isn't vacuous: [[ -s "$clean_sink" ]] requires the clean run to have actually produced findings before comparing, so a bug that suppressed output in both runs wouldn't silently pass.
• FINDINGS_SINK is genuinely mode-agnostic (report_finding appends to it whenever set, regardless of --commit/range/--verify-known-incidents), so using it here against a plain --commit HEAD scan is a legitimate, already-established use, not a new code path.
• The new prose in attribute_file()'s comment block (scripts/check-silent-revert.sh#L319-L339) cites the same 390/340 figures for 6f0a31109/91e77fc16 as the unchanged header section above it (#L139, #L143) — internally consistent, no stale cross-reference introduced.

No new findings.

Not independently verified

Could not execute scripts/check-silent-revert.test.sh or run isolated git blame probes in this session — shell commands beyond simple read-only git diff/git log/git show on the actual checkout required approval this session didn't grant (consistent with prior passes on this PR hitting the same restriction for shellcheck/gh/git fetch). In particular, the commit's central factual claim — that -c blame.ignoreRevsFile= leaves a GIT_CONFIG_GLOBAL-sourced blame.ignoreRevsFile value in effect while --no-ignore-revs-file clears it — is plausible under git's documented list-config accumulation semantics (an auto-generated --no-<opt> clears an OPT_STRING_LIST-style option, which --ignore-revs-file is), but I could not run git myself to confirm it, so it stands as author-measured and unverified by me. The PR body's other reported command output (repro transcripts, the 48-passed test run) is likewise author-claimed and unverified here.

[claude]

Claude finished @kyle-sexton's task in 1m 5s —— View job

---

Security review complete

• Read trigger instructions and gather PR context
• Invoke /review:security-review (skill tool errored again in this session — followed its SKILL.md instructions manually, as in the prior passes on this PR)
• Post findings

No security issues found.

Scope

This is a synchronize re-review. The last security-reviewed head was b47898957d0bfa3dbef87ad3b00298aa808f2654; the incremental diff to the current head (5e5a91a48dfdd6691609e07333de815c07e5e935) is the single new commit 5e5a91a ("pin git blame too, so ambient config cannot silence the canary"), touching only scripts/check-silent-revert.sh and scripts/check-silent-revert.test.sh.

What changed

• scripts/check-silent-revert.sh#L385-L386: the git blame call in attribute_file() now passes --no-ignore-revs-file explicitly, alongside the already-pinned --diff-algorithm=myers -M on the preceding git diff call. The accompanying comment documents a measured exposure: a caller with blame.ignoreRevsFile set globally reassigns blamed lines away from the listed commit, which would swing a real recorded attribution (bfb66beb8...) from 853 down to 259 lines and cause a FAIL ... NOT as recorded on an otherwise-clean tree. The comment also correctly notes -c blame.ignoreRevsFile= does not reset the setting (only the negated option does), so the fix uses the option form.
• scripts/check-silent-revert.test.sh#L252-L305: new test t_counts_are_immune_to_ambient_git_config runs the canary once with a clean environment and once with GIT_CONFIG_GLOBAL pointed at a scratch config carrying hostile-but-legitimate blame.ignoreRevsFile / diff.algorithm / diff.renames settings, then asserts the FINDINGS_SINK output is byte-identical between the two runs.

Assessment

Skip-gate criterion 2 applies: no new security-relevant surface.

• Both git diff/git blame invocations operate on trusted local shas/filenames already in scope from the prior review of this file ($parent, $commit/$sha, $file) — this commit only adds two literal flags (--no-ignore-revs-file, unchanged quoting elsewhere), no new variable interpolation or command construction, no injection surface.
• This change is a hardening fix, not a new risk: it closes a real detection-evasion path where a developer's (or, in principle, an attacker's) ambient git config could silently cause the silent-revert canary to under-attribute a large deletion — the opposite direction of a vulnerability.
• The new test's use of GIT_CONFIG_GLOBAL is scoped to that single subshell invocation via env-var assignment on the command line (GIT_CONFIG_GLOBAL="$cfg" ... run_canary ...), not an actual mutation of the invoking user's global git config, and all paths/variables are quoted. It operates entirely within mk_repo's disposable scratch checkout.
• .github/workflows/silent-revert-canary.yml and scripts/silent-revert-incidents.txt are unchanged since the last full review of this PR, which also found nothing.
  · Branch

[claude]

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

---

Code review complete

• Read trigger instructions and gather PR context
• Invoke /review:code-review skill and follow its instructions (skill tool errored in this session again; followed plugins/review/skills/code-review/SKILL.md directly, as prior passes on this PR did)
• Diff PR against origin/main, read all four changed files
• Post inline findings (none — see below)
• Post summary

Scope

This is a synchronize re-review. The last review of this lane covered head 5e5a91a. The only change since then is the new commit 39ecf30 — "docs(workflows): say what the PR lane actually runs" — touching only .github/workflows/silent-revert-canary.yml (+11/-11, all within # comment lines). Nothing else in the PR changed since the prior full passes, which already reviewed declares_removal()'s revert-subject matching, the --diff-algorithm=myers -M / --no-ignore-revs-file pinning, the bracket-detection fix (both automated lanes' shared finding, now fixed and covered by a test), the mk_repo fail-closed fix, and found them sound.

What this commit does

Corrects the header prose this PR itself rewrote earlier (in b3f1fa1). That rewrite replaced a false claim ("no pull_request trigger") with one that was itself slightly imprecise — that the trigger "exists solely to run the detector's own unit tests" and that "both scan steps" are gated off. This commit tightens the wording to match the actual step list.

Verified by reading the current file (.github/workflows/silent-revert-canary.yml):

• On a pull_request event, two steps run unconditionally: "Test the silent-revert detector" (L92-93, unit tests) and "Replay the recorded incidents" (L100-101, --verify-known-incidents — a corpus reproduction against real history, not a unit test). The new prose correctly distinguishes these ("its own unit tests and its recorded corpus") where the old prose collapsed them into one.
• Two different steps carry if: github.event_name != 'pull_request': "Resolve the pushed range" (L111-113) and "Scan the merged commits for silent reverts" (L139-140). The new prose's "the range-resolution and scan steps are both gated" (L17-18, L43) matches this exactly — the old prose's "both scan steps" was imprecise since range-resolution doesn't scan anything.
• The load-bearing conclusion — no PR is ever inspected for silent reverts, because the two gated steps are the only ones that touch check-silent-revert.sh in scan mode — is unchanged and still correct.

Confirmed comment-only: every changed line in git show 39ecf30 begins with #; no on:, if:, permissions:, or step body differs from the prior head.

No new findings. This is a small, accurate self-correction to prose this same PR introduced — verified against the actual workflow structure, not just read for plausibility.

Not independently verified

Did not execute scripts/check-silent-revert.test.sh or --verify-known-incidents in this session — this commit doesn't touch either script, so re-running wasn't necessary to verify its claims, which are checkable directly against the YAML's step list and if: conditions (done above).
· Branch

[claude]

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

---

Security review complete

• Read trigger instructions and gather PR context
• Invoke /review:security-review (skill tool errored again in this session — followed its SKILL.md instructions manually, as prior passes on this PR)
• Post findings

No security issues found.

Scope

This is a synchronize re-review. The last security-reviewed head was 5e5a91a48dfdd6691609e07333de815c07e5e935; the incremental diff to the current head (39ecf30887fde9caebda297c22be171a95acc287) is a single new commit, 39ecf30 ("docs(workflows): say what the PR lane actually runs"), touching only .github/workflows/silent-revert-canary.yml.

What changed

Two comment blocks in the workflow header are reworded — from "the pull_request trigger exists solely to run the detector's own unit tests" / "both scan steps are gated" to the more precise "it only exercises the detector against its own tests and its recorded corpus" / "the range-resolution and scan steps are both gated". Confirmed via git diff 5e5a91a...39ecf30 that every changed line begins with # — this is a prose-only correction of the comment's description of trigger/gating behavior, not a behavior change.

Assessment

Skip-gate criterion 2 applies: no security-relevant surface.

• No line in on:, permissions:, jobs:, or any if: condition changed — confirmed by reading the diff hunk-by-hunk against the file at L13-L49. The pull_request trigger still only runs the detector's unit tests + corpus replay, still gated away from scanning a PR's own content (if: github.event_name != 'pull_request' on the range-resolution and scan steps, unchanged), and the lane still sits outside ci.yml's ci-status aggregate.
• The corrected wording doesn't loosen or misstate any trust boundary — if anything it's more precise about what the pull_request trigger does (unit tests + recorded-corpus replay, not just unit tests), and the load-bearing security claim ("no PR is ever inspected for silent reverts") is unchanged and still accurate.
• scripts/check-silent-revert.sh, scripts/check-silent-revert.test.sh, and scripts/silent-revert-incidents.txt are unchanged since the last full review of this PR, which also found nothing.
  · Branch

[claude]

Claude finished @kyle-sexton's task in 1m 19s —— View job

---

Security review complete

• Read trigger instructions and gather PR context
• Invoke /review:security-review (skill tool errored again in this session — followed its SKILL.md instructions manually, as in every prior pass on this PR)
• Post findings

No security issues found.

Scope

This is a synchronize re-review. The last security-reviewed head was 39ecf30887fde9caebda297c22be171a95acc287; the incremental diff to the current head (74ea838b57f6da30e8c9ac666c5b36d5fda8c76e) is two new commits — 4769fe8 ("pin the git flags a caller's config can move, and stop overclaiming which") and 74ea838 ("fail loudly when a fixture cannot be built, and prove each pin discriminates") — touching only scripts/check-silent-revert.sh and scripts/check-silent-revert.test.sh. .github/workflows/silent-revert-canary.yml and scripts/silent-revert-incidents.txt are unchanged since the last full review, which also found nothing.

What changed

• scripts/check-silent-revert.sh#L217, #L580 and the report_finding git log calls: add --no-show-signature, so a caller's log.showSignature = true can no longer make git prepend a GPG verification verdict onto stdout ahead of the commit subject $(...) captures — this repo requires signed commits, so an unpinned run could misread a signed revert's subject as Good "git" signature for ... and lose intent detection.
• scripts/check-silent-revert.sh#L355-L368: the git diff call in attribute_file() adds --no-ext-diff and --no-textconv, closing two more ambient-config paths (diff.external, diff.<name>.textconv) that could replace or transform the diff output the canary's counts are computed from.
• scripts/check-silent-revert.sh#L465: the git blame call adds --no-textconv alongside the existing --no-ignore-revs-file.
• The rest of the diff is comment/prose expansion (a pin table stating precisely what each flag defends and what it deliberately does not cover) and a printf newline fix in scan_commit()'s declared output — no behavior change to counting logic beyond the flags above.
• scripts/check-silent-revert.test.sh: adds CANARY_SCRIPT support and a fixture-build guard to run_canary() (an unbuilt fixture now fails loudly with RC=99 instead of silently behaving like a passing rc=1), a strip_pin() helper that writes a de-pinned copy of the detector into the test fixture's own throwaway checkout and fails loudly if the sed edit matched nothing, and four new t_*_pin_is_load_bearing cases plus t_declared_note_ends_its_line.

Assessment

Skip-gate criterion 2 applies: no security-relevant surface.

• All three new flags (--no-show-signature, --no-ext-diff, --no-textconv) are pins on existing git log/git diff/git blame invocations that already operated on trusted local shas/paths in scope from prior reviews of this file — no new variable interpolation, no new command construction, no injection surface. Every one of them removes a way ambient/developer git config could silently change what the canary reports (misread subjects, suppressed diff output, transformed content), which tightens the detector rather than loosening anything.
• The strip_pin()/CANARY_SCRIPT machinery in the test harness operates entirely inside mk_repo's disposable scratch checkout ($repo/scripts/...), never touches the real repo's scripts/check-silent-revert.sh, and takes no externally-controlled input — it's local test infrastructure comparing a shipped script against a sed-derived throwaway copy of itself.
• t_show_signature_pin_is_load_bearing's hand-crafted gpgsig header (grafted via awk + git hash-object/update-ref) creates a deliberately-invalid signature on a disposable fixture commit to exercise log.showSignature output — no signing key, agent, or network involved, and the object never leaves the fixture's throwaway .git.
• No .github/workflows/* file changed in this increment (confirmed via git diff — zero output), so no permission or trigger surface moved.
  · Branch

[claude]

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

---

Security review complete

• Read trigger instructions and gather PR context
• Invoke /review:security-review (skill tool errored again in this session — followed its SKILL.md instructions manually, as in every prior pass on this PR)
• Post findings

No security issues found.

Scope

This is a synchronize re-review. The last security-reviewed head was 74ea838b57f6da30e8c9ac666c5b36d5fda8c76e; the incremental diff to the current head (866e93fb153f9294ff9daad943964f960d4b8814) is a single new commit, 866e93f ("test(scripts): make the --no-textconv fixture actually exercise the pin"), touching only scripts/check-silent-revert.test.sh (+81/-26). scripts/check-silent-revert.sh, scripts/silent-revert-incidents.txt, and .github/workflows/silent-revert-canary.yml are unchanged since the last full review, which also found nothing.

What changed

Fixes t_textconv_pin_is_load_bearing, which previously passed vacuously (and then failed outright on CI) because its fixture couldn't actually exercise the --no-textconv pin:

• scripts/check-silent-revert.test.sh#L509-L516: the fake textconv driver changes from a helper script under the fixture's tmp directory ($repo/shift-textconv.sh, invoked by absolute path) to the bare PATH-resolved command sed p. The commit message explains this is a Windows-CI portability fix (git spawns textconv via the platform's native process API, which can't resolve the MSYS-style path mktemp -d returns for a script file), not a security-motivated change.
• The driver's transformation changes from prepending 10 filler lines (which shifts a hunk's offset but not its length — so it never actually changed any count) to sed p (which doubles every line, so it does).
• scripts/check-silent-revert.test.sh#L531-L541: adds a self-check that runs raw git blame --line-porcelain with and without the driver config and fails the case (distinctly from a pin-inert failure) if the driver isn't actually active.
• Adds a third diff-textconv-unpinned arm and a 150-line fixture prefix so the "mixed" pin-stripped arms are asserted to go silent (empty sink), not merely produce a different count.

Assessment

Skip-gate criterion 2 applies: no security-relevant surface.

• This is test-only code. All of it runs inside mk_repo's disposable scratch checkout, and GIT_CONFIG_GLOBAL="$cfg" is scoped to individual command invocations (never the invoking user's real global git config), consistent with the pattern already reviewed in the prior t_counts_are_immune_to_ambient_git_config pass.
• The sed p "textconv driver" is a fixed literal string written into a throwaway $repo/textconv-gitconfig file — no variable interpolation, no externally-influenced content, and it only ever processes the fixture's own self-generated feature.md.
• No production code (scripts/check-silent-revert.sh) changed in this commit — the --no-textconv pin itself was already reviewed and is unchanged; this commit only fixes the test that verifies it's load-bearing.
• No .github/workflows/* file changed in this increment.

[claude]

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

---

Security review complete

• Read trigger instructions and gather PR context
• Invoke /review:security-review (skill tool errored again in this session — followed its SKILL.md instructions manually, as in every prior pass on this PR)
• Post findings

No security issues found.

Scope

This is a synchronize re-review. The last security-reviewed head was 866e93fb153f9294ff9daad943964f960d4b8814; the incremental diff to the current head (55e2f4c5d875505f123c3abb1cdca64467d2762c) is a single new commit, 55e2f4c ("test(scripts): close a fail-open in strip_pin and measure the both-stripped arm"), touching only scripts/check-silent-revert.sh and scripts/check-silent-revert.test.sh. .github/workflows/silent-revert-canary.yml and scripts/silent-revert-incidents.txt are unchanged since the last full review, which also found nothing.

What changed

• scripts/check-silent-revert.sh#L458-L471: comment-only rewrite of the prose above the git blame --no-ignore-revs-file --no-textconv --line-porcelain call, replacing stale prototype figures with the measured figures from the now-fixed t_textconv_pin_is_load_bearing. No executable line differs.
• scripts/check-silent-revert.test.sh#L361-L382: strip_pin() now strips #-comment lines from both the shipped detector and its sed-derived copy (via grep -v '^[[:space:]]*#' into two mktemp files) before comparing, instead of cmp-ing the whole files. This closes a fail-open where a sed expression that only touched prose (e.g. matched a flag name appearing in a comment above the real call, not the call itself) would still register as "the edit changed something" and let a pin-discrimination test pass vacuously.
• scripts/check-silent-revert.test.sh#L588-L603: t_textconv_pin_is_load_bearing's "both pins stripped" arm now asserts the reported count is exactly clean_n * 2 (matching the sed p duplication semantics), rather than merely asserting it differs from the clean count — a tighter, more specific assertion, not a behavior change to production code.

Assessment

Skip-gate criterion 2 applies: no security-relevant surface.

• The only change to scripts/check-silent-revert.sh is comment text — confirmed via the diff above, no git/shell invocation, flag, or control-flow line moved.
• strip_pin() and the tightened arithmetic assertion are test-harness-only code, operating entirely inside mk_repo's disposable scratch checkout ($repo/scripts/$name.sh) on the test's own mktemp scratch files, with no externally-controlled input and no new eval/dynamic execution — consistent with the pattern already reviewed in prior passes on this file's harness helpers.
• This is a correctness/rigor tightening of the test suite (closing a way a discrimination test could pass without discriminating), not a change to any trust boundary, permission, or injection surface.
• No .github/workflows/* file changed in this increment.
  · Branch

[claude]

Claude finished @kyle-sexton's task in 2m 8s —— View job

---

Security review complete

• Read trigger instructions and gather PR context
• Invoke /review:security-review and follow its instructions (skill tool errored again in this session — followed its SKILL.md instructions manually, as in every prior pass on this PR)
• Post findings

No security issues found — skip gate applied (criterion 3: this head already has a successful security review that still applies).

Why this head is skipped

The last security-reviewed head was 55e2f4c5d875505f123c3abb1cdca64467d2762c. Since then, two merge commits landed on this branch — 6dfc6c8 ("Merge origin/main into the silent-revert intent branch", bringing in #2873) and c5e6845 ("Merge origin/main to pick up #2888") — but neither adds any new commit of this branch's own.

Verified this concretely rather than assuming it from the merge commit messages:

• git diff origin/main...HEAD --numstat at the current head (c5e68454a7f7bde3d2efadf1ae8b2a0de054d300) reports exactly +12/-9 (workflow), +306/-20 (check-silent-revert.sh), +659/-6 (check-silent-revert.test.sh), +39/-8 (silent-revert-incidents.txt) — identical to the changed_files totals already supplied for this task, and identical to every stat reported across the prior review passes on this PR.
• The four changed files' current blob shas (efd459f, 52f917b, 360ececb, 197deced) match the changed_files list's SHAs exactly — this is the same content already reviewed, not new content that happens to be the same size.
• The two merges only pulled in #2873 and #2888, both already independently merged to origin/main — so their content is common to both sides of origin/main...HEAD and doesn't appear in this PR's diff at all, regardless of the fact that #2873 touches the same file (scripts/check-silent-revert.sh) this PR also touches. The merge commit messages themselves confirm this was a clean reconciliation ("the reconciliation resolved in the previous merge stands unchanged").

So there is nothing new to review here. Every commit that makes up this PR's actual +1016/-43 diff — the revert:-subject Conventional-Commits matching in declares_removal(), the bracketed-attribution assertions in verify_known_incidents (including the bracket-detection fix both automated lanes flagged and the author fixed), the --diff-algorithm=myers -M / --no-ignore-revs-file / --no-textconv / --no-ext-diff / --no-show-signature ambient-git-config pins, the mk_repo fail-closed harness fix, and the workflow header comment corrections — was already reviewed across the prior passes on this thread and found to have no security-relevant surface: no injection paths, no trust-boundary changes, no permission widening in the workflow (contents: read unchanged, no on:/if: changes), and no untrusted external input in scope (the canary only ever processes this repo's own trusted git history).
· Branch