ci: add a post-merge silent-revert canary

[kyle-sexton]

No linked issue

Summary

A post-merge canary for the failure class in #2691: a merge that silently deletes content another recently-merged commit had just added, leaving no Revert: marker and no failing test. Item 4 of that issue. Detection only — it runs on push to main, is not in ci.yml, and is not wired into ci-status, so it can never gate a merge.

It also corrects the issue's premise. This was not a stale-base failure, which means strict_required_status_checks_policy would not have prevented it. Details below.

Fix

scripts/check-silent-revert.sh blames the lines each merge deleted against its own parent and reports when a large block traces to a single commit inside a recency window. Plus scripts/check-silent-revert.test.sh (26 hermetic cases), two data files, and .github/workflows/silent-revert-canary.yml.

Why blame-of-deleted-lines, and not the alternatives

Three designs were measured against the real history before one was chosen.

Merge-base staleness — tested and rejected on evidence. It exonerates all three real incidents:

$ git merge-base --is-ancestor f603880d refs/pull/2641/head && echo YES
YES
$ git grep -c "read-only supporting allowlist" refs/pull/2641/head -- 'plugins/disk-hygiene/**'
(no output — zero occurrences)
$ git grep -c "read-only supporting allowlist" f603880d -- 'plugins/disk-hygiene/**'
f603880d:plugins/disk-hygiene/skills/clean/SKILL.md:1
f603880d:plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py:1

PR #2641's head had #2639 in its ancestry and zero occurrences of #2639's content in its tree. #2639 shows the identical shape against #2635. These branches were up to date with main in history and stale only in content — a bad conflict resolution or a force-push from an older worktree.

So strict would have passed all three merges, and a merge queue would have too (CI was green — the tests were deleted alongside the code). That reframes the canary: for this class it is not defence in depth behind a real fix, it is the only control that fires at all. No ruleset is touched here and nothing in github-iac changes; ADR 0001 stands as written.

This also matters for #2799, which merged while this was in flight. That PR landed scripts/check-stale-base-overlap.sh as a required PR gate (it is in ci-status's needs), and its header cites these very incidents: "claude-code-plugins#2691: #2635 then #2639 lost inside ten minutes". Its rule is merge-base(HEAD, base) == tip(base) → fresh, exit 0. I ran the merged script against the actual PR branches:

$ git worktree add --detach <tmp> refs/pull/2641/head
$ git branch -f base2641 eda5ae5ed          # main's tip when #2641 merged
$ bash ./check-stale-base-overlap.sh --check base2641
check-stale-base-overlap: HEAD is up to date with base2641
exit 0

$ git checkout --detach refs/pull/2639/head
$ git branch -f base2639 b7793d3b4          # main's tip when #2639 merged
$ bash ./check-stale-base-overlap.sh --check base2639
check-stale-base-overlap: HEAD is up to date with base2639
exit 0

It exits 0 — "fresh" — on both branches that did the reverting, because both were genuinely up to date with main. That is not a bug in #2799; it correctly detects the stale-base class it targets. But its header claims these incidents as its motivating case, and for those it is a false negative. The two guards are complementary rather than redundant: #2799 prevents a stale base pre-merge, this catches a stale tree post-merge, and only the second fires on what actually happened. Worth correcting #2799's header comment either way.

Curated marker strings (the issue's own suggestion 3) — rejected. Only catches what someone pre-registered, and nobody had registered #2632, #2635 or #2639. Registration happens after you know a fix matters, which is the knowledge the incident destroys.

PR-creation-time overlap — rejected as non-discriminating. At the 17-concurrent-PR rate ADR 0001 records, nearly every PR has siblings landing while it is open.

False-positive strategy

A canary that cries wolf gets disabled, which is worse than none.

1. Volume, aggregated per culprit commit — summing across culprits would re-admit ordinary iteration.
2. Recency window (40 first-parent commits). Stated limitation: content reverted from outside the window is missed by design.
3. Intent, in constrained forms only — a Revert " subject, a This reverts commit <sha> line, or an explicit Intentional-removal: trailer. Deliberately not a substring search for "revert": a body reading "this does not revert X" would silence a real finding.
4. Non-blocking — post-merge only, outside ci-status.

Measured, not assumed — and measured by running the shipped script itself over the last 500 first-parent commits of main, not a stand-in:

FIRE    853 cc58cbc53 fix(repo-fleet-hygiene): report bare repos with live working trees (#2633)
FIRE    451 9239f1541 feat(disk-hygiene): verify redundant checkout evidence (#2641)
FIRE    390 6f0a31109 fix(repo-fleet-hygiene): restore GraphQL merge evidence and rollups after #2633
FIRE    346 f603880da fix(disk-hygiene): session-honest belt, read-only allowlist, ... (#2639)
FIRE    340 91e77fc16 fix(hook-utils): stop a NUL in a payload value from voiding two blocking guards
MEASUREMENT COMPLETE over 500 commits

5 fires in 500 merges — 1%, zero errors. Three are the confirmed incidents. The other two are real and are not detector bugs: #2640 (390) is the manual restore of #2633's revert, and #2135 (340) is a deliberate merge reconciliation the author argues at length in the PR body. Both are pre-recorded in the acknowledgment file so main starts green.

Rename detection is deliberately left ON. An earlier revision passed --no-renames, which silently made the shipped detector a different detector from the calibrated one: without detection a git mv decomposes into delete + add, the delete side reaches the --diff-filter=MD enumeration as a whole-file removal, and relocating a large file a recent commit had added would fire. In a repo that restructures skills and docs this often, that is a live false-positive class. With detection on, the shipped script reproduces the calibration corpus exactly (the five rows above), and a test pins the rename case. The narrow cost, stated rather than hidden: content gutted in the same commit that renames its file is not attributed.

The uncomfortable part, stated plainly: 340 is the largest false positive and 346 is the smallest true one. No threshold separates them. Picking a number inside that 2% gap would be overfitting, so the threshold is 200 — which costs nothing (200 and 300 fire on the identical five commits) and leaves headroom for a smaller future revert. Precision is traded for recall because the miss is expensive and the fire is cheap.

Cheap requires a disposition path in both directions in time, so there are two: the prospective Intentional-removal: trailer (one line in the PR body, which GitHub carries into the squash message), and scripts/silent-revert-acknowledged.txt for a fire that can only be judged after the fact — a commit message cannot be amended post-merge. Without the second, one legitimate fire leaves the canary permanently red, and a permanently red canary is one on its way to being deleted. It matches changelog-parity-baseline.txt in shape, matches full 40-char SHAs only, and requires a recorded reason, so it is an audit trail rather than a mute button.

A third incident, previously unfiled

Calibration surfaced one #2691 never identified: #2633's squash dropped #2632's finding rollups (853 lines), which #2640 restored by hand the same night. Nobody filed it. That is the clearest argument for automating the detection.

Verification

Real output, all from this branch.

Catches the actual incident (requirement 1) — replayed against the real merges, and pinned in scripts/silent-revert-incidents.txt so CI re-proves it on every run:

$ scripts/check-silent-revert.sh --verify-known-incidents
ok   f603880da fires as recorded  (#2639 dropped #2635 (346 lines), 13 minutes later)
ok   9239f1541 fires as recorded  (#2641 dropped #2639 (451 lines), 10 minutes later)
ok   cc58cbc53 fires as recorded  (#2633 dropped #2632's rollups (853 lines) -- unfiled until now)
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.

Range mode over the incident window — both fire, the interleaved unrelated merges stay clean:

$ scripts/check-silent-revert.sh a95f240f~1..9239f154
ok a95f240f7 feat(disk-hygiene): prioritize tidiness over reclaimable bytes in reports (#2635)
ok b7793d3b4 fix(repo-fleet-hygiene): degrade non-repo paths under --root (#2630)
SILENT REVERT SUSPECTED
  removed by   f603880da  fix(disk-hygiene): session-honest belt, ... (#2639)
  content from a95f240f7  feat(disk-hygiene): prioritize tidiness over ... (#2635)
  lines lost   346  (threshold 200, window 40 commits)
ok eda5ae5ed feat(repo-fleet-hygiene): gather merge evidence via aliased GraphQL (#2642)
SILENT REVERT SUSPECTED
  removed by   9239f1541  feat(disk-hygiene): verify redundant checkout evidence (#2641)
  content from f603880da  fix(disk-hygiene): session-honest belt, ... (#2639)
  lines lost   451  (threshold 200, window 40 commits)

Actionable when it fires (requirement 4) — it names what disappeared, which commit removed it, which commit added it, the per-file split, and the sample quotes back the very content #2691 reported as lost:

  by file:
     235  plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py
     156  plugins/disk-hygiene/skills/clean/scripts/test_hygiene.py
      26  plugins/disk-hygiene/skills/clean/SKILL.md
      ...
  sample of the removed content:
      | - The skill-frontmatter guard is a fail-closed allowlist. It permits canonical bundled scan/preview
      | calls made from literal shell words, a small read-only supporting set for cleanup inspection

Tests (requirement 5) — scripts/check-silent-revert.test.sh, hermetic synthetic repos, following the scripts/*.test.sh + test-git-helpers.sh convention. 27 cases: attribution, per-culprit aggregation, the recency window, the pure-rename negative, every intent form, the "prose mentioning revert must still fire" negative, the empty-trailer negative, abbreviated-SHA rejection, whole-file deletion, the shipped default in both directions, and fail-closed exit 2 on an unresolvable range, a malformed range, and an unreachable pinned commit.

$ bash scripts/check-silent-revert.test.sh
check-silent-revert.test.sh: 27 passed, 0 failed

Verified in real CI on this PR, not just locally. The lane runs on pull_request (scoped by paths to the canary's own files) so the detector is exercised before it lands — the scan step is gated off for PR events, so nothing on a PR inspects that PR. From the actual run log (job, 12s):

Test the silent-revert detector    check-silent-revert.test.sh: 27 passed, 0 failed
Replay the recorded incidents      ok   f603880da fires as recorded  (#2639 dropped #2635 (346 lines), 13 minutes later)
Replay the recorded incidents      ok   9239f1541 fires as recorded  (#2641 dropped #2639 (451 lines), 10 minutes later)
Replay the recorded incidents      ok   cc58cbc53 fires as recorded  (#2633 dropped #2632's rollups (853 lines) -- unfiled until now)
Replay the recorded incidents      ok   c8470efd0 stays clean as recorded  (docs(conventions) rewrote 129 lines ...)
Replay the recorded incidents      Canary reproduces every recorded incident at the shipped settings.

That run matters for a specific reason. The blame-header pattern originally used an ERE interval ({40}), and interval support is an awk-implementation variable — the runner's default awk is mawk, development machines run gawk. Had it not matched, attribution would emit nothing and every commit would report ok: a false green, the exact failure class this canary exists to remove. It is now interval-free and proven against the runner's own awk above.

Cannot block a merge — verified, not just asserted. The required contexts on main are:

$ gh api repos/melodic-software/claude-code-plugins/rules/branches/main \
    --jq '.[] | select(.type=="required_status_checks") | .parameters.required_status_checks[].context'
pr-title / pr-title
pr-issue-linkage / pr-issue-linkage
do-not-merge / do-not-merge
ci-status

Silent-revert canary is not among them, and its only appearance in ci.yml is the workflow_schema filename list — never ci-status's needs. A non-required check cannot gate a merge.

Repo gates, all run locally on this branch: shellcheck --rcfile=.shellcheckrc -x (exit 0, and this repo enables require-double-brackets and add-default-case), check-shell-portability.sh --paths and --all (exit 0), zizmor (no findings), actionlint (exit 0), typos (exit 0), editorconfig-checker (exit 0), check-jsonschema --builtin-schema vendor.github-workflows (ok), check-silent-skips.sh (exit 0). The new workflow is registered in ci.yml's workflow_schema file list; the shell scripts are committed 100755.

Design notes for review

• No cancelling concurrency group, unlike ci.yml — a cancelled canary run is a silently missed detection.
• Fail-closed on an unusable push range. github.event.before is all-zeros on a first push or history rewrite; the workflow falls back to the head commit and emits a ::warning:: saying earlier commits were not scanned, rather than reporting a clean scan of nothing. An unresolvable range exits 2, never 0.
• Self-test runs before every scan, so a broken detector cannot mask a regression behind a green canary — the same never-skip, self-test-first shape the ci.yml gates use.
• What runs on a PR is the detector's unit tests, never detection. The pull_request trigger is paths-scoped to the canary's own five files, so it is inert on every other PR, and both scan steps carry if: github.event_name != 'pull_request'.
• Remaining limitations, stated rather than hidden: content reverted from outside the 40-commit recency window is missed by design; content gutted in the same commit that renames its file is not attributed; and no threshold separates a large deliberate rewrite from a silent revert, which is what the acknowledgment file exists to absorb.

Related

Refs #2691 — this is item 4; the issue covers more and stays open.
Refs #2799 — item 2, the merge-time stale-base gate, merged while this was in flight. Complementary, not redundant: verified above to exit 0 on both reverting branches, so its header's claim to cover these incidents needs correcting.
Refs #2713 — the docs-only silent-revert blind spot, same class from the test-coverage side.
Refs melodic-software/github-iac docs/adr/0001-relax-strict-required-status-checks.md — unchanged; the evidence above argues it was never the relevant control for this failure mode.

[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 29s —— View job

---

Claude finished the security review

Task: Run /review:security-review for PR #2808 (fix/silent-revert-canary → main, HEAD a90501d).

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

---

Security review

Reviewed the two security-relevant surfaces in this diff: the new .github/workflows/silent-revert-canary.yml workflow and the shell it calls (scripts/check-silent-revert.sh, scripts/check-silent-revert.test.sh). No vulnerabilities found.

Specifically checked and cleared:

• Trigger surface — silent-revert-canary.yml: push to main + workflow_dispatch only. No pull_request_target or workflow_run, so untrusted PR content never runs with this workflow's token.
• Permissions — L61-62: scoped to contents: read, and checkout sets persist-credentials: false (L70-73) — no token available to scripts even if something were compromised downstream.
• Script-injection via the github context — the workflow correctly threads github.event.before / github.event.after through env: (EVENT_BEFORE / EVENT_AFTER) rather than interpolating them into the run: body (L100-108), and both values are validated with git rev-parse --verify before use. This is exactly the pattern that avoids the classic "expression injection in a run: block" class of finding.
• Command/argument injection in the shell scripts — filenames from git diff --name-only -z are read with -d ''/read -r (null-safe) and every git invocation that takes a filename uses a -- separator before the path (check-silent-revert.sh attribute_file()), so a maliciously-named file (e.g. one starting with -) can't be interpreted as a flag. No eval, no unsanitized interpolation into a shell command, no network calls (curl/wget) anywhere in the new scripts.
• Untrusted content handled safely — commit subjects/bodies (declares_removal()) and the acknowledgment file (ack_reason()) are matched with case/grep -E and read -r, never eval'd or sourced.
• Supply chain — actions/checkout is pinned to a full commit SHA with a version comment (already zizmor's lane, but confirmed present and correctly pinned).

This lane is scoped to security only — no code-quality/style/test-coverage commentary is included here.
· Branch: fix/silent-revert-canary

[github-actions]

Last security-reviewed head: 253aa544552527a4877cca98bb4af3a691d91233. 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: a90501d38d

ℹ️ 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".

[claude]

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

---

Claude finished the security review

Task: Run /review:security-review for PR #2808 (fix/silent-revert-canary → main, HEAD 253aa54).

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

---

Security review

This head (253aa54) had already been security-reviewed once at a90501d with no findings. Since then, one commit ("ci: keep rename detection on and test the canary pre-merge") landed, touching 3 files. I reviewed that delta plus re-confirmed the previously-cleared surfaces still hold. No vulnerabilities found.

New surface in this commit — the pull_request trigger added to silent-revert-canary.yml:

• It's pull_request, not pull_request_target — runs with the PR's own workflow definition and no secret access, the safe pattern for untrusted branches.
• Scoped via paths: to only the canary's own files (L58-64), so it's inert on unrelated PRs.
• permissions: contents: read (L67-68) is unchanged and applies to this trigger too — no token available even to a malicious PR-supplied step.
• The two steps that run on pull_request (self-test, --verify-known-incidents, L89-98) don't interpolate any github.event.* value into their run: bodies, so there's no script-injection surface there. The range-resolve and scan steps (the ones that do use github.event.before/after via env:) are gated if: github.event_name != 'pull_request' (L108-110, L136-137), so they never run on this trigger at all.
• This is the same pull_request + contents: read posture already used by ci.yml's own top-level trigger — not a new pattern being introduced into the repo.

scripts/check-silent-revert.sh changes — this commit drops --no-renames from both git diff invocations in attribute_file()/scan_commit() to fix the rename-mishandling correctness bug flagged by Codex's review, and loosens the git blame --line-porcelain header regex from a {40} interval to [0-9a-f]+ [0-9]+ [0-9]+ with an explicit length($1) == 40 check (portability across mawk/gawk, not a security change). Neither touches trust boundaries, external input handling, or credential handling; both are pure detection-logic changes over already-trusted repo content (git blame/git diff output of the checked-out repo itself).

scripts/check-silent-revert.test.sh only gained a new hermetic test case (t_rename_is_not_a_removal) exercising the fix above — no security-relevant content.

.github/workflows/ci.yml's one-line change just adds silent-revert-canary.yml to the existing workflow_schema file list already reviewed at a90501d — no change in behavior.

No CRITICAL, IMPORTANT, or SUGGESTION findings.
· Branch: fix/silent-revert-canary

[claude]

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

---

Code review

• Gather context (PR state, diff, REVIEW.md/CLAUDE.md scope)
• Apply /review:code-review skip gate
• Post result

---

This PR is merged (state: MERGED). Per the /review:code-review skip gate, a closed/merged PR is not reviewed — stopping here with no inline findings posted.

For reference, this PR already went through a full inline security review (/review:security-review, no findings on either reviewed head) and a Codex P2 review comment (rename-attribution false positive), which the author acknowledged as addressed in a follow-up commit before merge.
· Branch: fix/silent-revert-canary

[github-actions]

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