fix(repo-fleet-hygiene): warn on ignored files in reclaimable worktrees - #2632
Closed
kyle-sexton wants to merge 1 commit into
Closed
fix(repo-fleet-hygiene): warn on ignored files in reclaimable worktrees#2632kyle-sexton wants to merge 1 commit into
kyle-sexton wants to merge 1 commit into
Conversation
|
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. |
Contributor
|
Claude finished @kyle-sexton's task in 1m 56s —— View job Security Review
No security issues found. This PR extends
One judgment call worth a human look (not a security finding, so not reported inline): the regenerable-prefix allowlist in |
kyle-sexton
enabled auto-merge (squash)
August 14, 2026 22:58
|
Last security-reviewed head: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e038e8abdb
ℹ️ 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".
Force --untracked-files=normal on the ignored-aware status probe so local status.showUntrackedFiles=no cannot hide destroyable content. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
cursor
Bot
force-pushed
the
cursor/fix-fleet-reclaimable-ignored-f8b0
branch
from
August 14, 2026 23:12
e038e8a to
2b9391b
Compare
Contributor
Author
Contributor
Author
|
Superseded by #2637 — reclaimable-worktree retired from the fleet collector. |
auto-merge was automatically disabled
August 14, 2026 23:19
Pull request was closed
Contributor
|
Claude finished @cursor[bot]'s task in 33s —— View job Code Review SkippedPR is closed. Per the
If this PR is reopened, re-trigger |
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
cursor Bot
pushed a commit
that referenced
this pull request
Aug 16, 2026
Detects a merge that silently deleted content another recently-merged commit had just added, with nothing in its own message saying it meant to. This is item 4 of #2691. The canary blames the lines each merge removed against its parent and reports when a large block traces to a single commit inside a recency window. Volume, recency, constrained intent markers, and a non-blocking post-merge-only trigger keep the firing rate at 1% over the last 500 first-parent commits of main. Two designs were measured and rejected first. Merge-base staleness exonerates all three real incidents, because every reverting branch was up to date with main in HISTORY and stale only in CONTENT -- `git merge-base --is-ancestor f603880 refs/pull/2641/head` is true while that branch's tree carried zero occurrences of #2639's markers. Curated marker strings only catch what somebody pre-registered, and nobody had registered the fixes that were lost. That evidence also corrects the issue's premise: this was not a stale-BASE failure, so `strict_required_status_checks_policy` would have passed all three merges, and so would a merge queue (CI was green -- the tests were deleted alongside the code). No ruleset is touched here. Calibration surfaced a third, previously unfiled incident: #2633's squash dropped #2632's rollups (853 lines), which #2640 restored by hand. Refs #2691 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018S8a1S71VxhLTRWBtMuEvp
kyle-sexton
added a commit
that referenced
this pull request
Aug 16, 2026
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 f603880 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" f603880 -- 'plugins/disk-hygiene/**' f603880:plugins/disk-hygiene/skills/clean/SKILL.md:1 f603880: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. **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 cc58cbc fix(repo-fleet-hygiene): report bare repos with live working trees (#2633) FIRE 451 9239f15 feat(disk-hygiene): verify redundant checkout evidence (#2641) FIRE 390 6f0a311 fix(repo-fleet-hygiene): restore GraphQL merge evidence and rollups after #2633 FIRE 346 f603880 fix(disk-hygiene): session-honest belt, read-only allowlist, ... (#2639) FIRE 340 91e77fc 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 f603880 fires as recorded (#2639 dropped #2635 (346 lines), 13 minutes later) ok 9239f15 fires as recorded (#2641 dropped #2639 (451 lines), 10 minutes later) ok cc58cbc fires as recorded (#2633 dropped #2632's rollups (853 lines) -- unfiled until now) ok c8470ef 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 a95f240~1..9239f15 ok a95f240 feat(disk-hygiene): prioritize tidiness over reclaimable bytes in reports (#2635) ok b7793d3 fix(repo-fleet-hygiene): degrade non-repo paths under --root (#2630) SILENT REVERT SUSPECTED removed by f603880 fix(disk-hygiene): session-honest belt, ... (#2639) content from a95f240 feat(disk-hygiene): prioritize tidiness over ... (#2635) lines lost 346 (threshold 200, window 40 commits) ok eda5ae5 feat(repo-fleet-hygiene): gather merge evidence via aliased GraphQL (#2642) SILENT REVERT SUSPECTED removed by 9239f15 feat(disk-hygiene): verify redundant checkout evidence (#2641) content from f603880 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](https://github.com/melodic-software/claude-code-plugins/actions/runs/31918399785/job/95094057983), 12s): ``` Test the silent-revert detector check-silent-revert.test.sh: 27 passed, 0 failed Replay the recorded incidents ok f603880 fires as recorded (#2639 dropped #2635 (346 lines), 13 minutes later) Replay the recorded incidents ok 9239f15 fires as recorded (#2641 dropped #2639 (451 lines), 10 minutes later) Replay the recorded incidents ok cc58cbc fires as recorded (#2633 dropped #2632's rollups (853 lines) -- unfiled until now) Replay the recorded incidents ok c8470ef 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 #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. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
This was referenced Aug 16, 2026
kyle-sexton
added a commit
that referenced
this pull request
Aug 16, 2026
…2832) Closes #2831. The silent-revert canary merged in #2808 ships an incident corpus that misattributes one of its three incidents. Since `--verify-known-incidents` replays that corpus on every `push: main`, the corpus is the guard's own honesty proof, and a wrong row costs it the credibility it exists to earn. The guard already contradicts its own fixture. Running the shipped detector on the incident commit prints: ``` $ scripts/check-silent-revert.sh --commit cc58cbc content from bfb66be feat(repo-fleet-hygiene): add finding rollups and scalable handoff plans (#2644) lines lost 853 content from eda5ae5 feat(repo-fleet-hygiene): gather merge evidence via aliased GraphQL (#2642) lines lost 301 ``` while the corpus recorded that commit as `#2633 dropped #2632's rollups (853 lines) -- unfiled until now`. ## What was wrong, and what it is now **The named victim never merged.** PR #2632 is CLOSED, not merged: `mergeCommit` null, `mergedAt` null, closed unmerged 2026-08-14T23:19:00Z, head `2b9391be` not an ancestor of `origin/main`, and zero occurrences of `rollup` in its diff. **There were two victims, and neither was #2632.** Attributing each of the 1165 lines `cc58cbc53` (#2633) deleted to the commit `git blame` credits it to: | culprit | PR | lines | | --- | --- | ---: | | `bfb66beb8` | #2644 — finding rollups and handoff plans | 853 | | `eda5ae5ed` | #2642 — aliased GraphQL merge evidence | 301 | | 11 other commits, ≤4 lines each | — | 11 | | **total** | | **1165** | 1165 matches the squash diffstat exactly (`6 files changed, 549 insertions(+), 1165 deletions(-)`). #2642 was not mentioned in the corpus at all, despite the detector reporting it as a separate finding. Corroboration: `plugins/repo-fleet-hygiene/skills/audit/scripts/audit-fleet.sh` went 2178 lines / 8 `graphql` / 9 `rollup` at `bfb66beb8` → 1700 / 0 / 0 at `cc58cbc53` → 2348 with both restored at `6f0a31109` (#2640) → 2933 on current `origin/main`. **853 was correct but unqualified.** It is a blame attribution against one culprit — not the deleting squash's diffstat total (1165) and not what #2644 added (942 insertions in its own squash commit, per `git diff --shortstat d55ffbf bfb66be`). Both 853 and 301 now say what they measure, in the fixture and in the script header. Worth noting for anyone re-measuring: `gh pr view 2644 --json additions` says 947, not 942, because the PR-level count is three-dot against the merge base while the commit diffstat is two-dot against the parent, and main moved under the branch in between. #2640 diverges the same way (`+1281/−463` as a commit, `1269/451` as a PR). The branch text avoids the trap by citing no lines-added figure at all — it just says the blame counts are not that. **"Unfiled until now" was false.** #2656 recorded this merge event, pinned to the same commits, roughly 23 hours before #2808 merged — from the test-coverage angle rather than as a silent revert. The corpus now cites it and frames the canary's contribution as detection speed and automation rather than discovery. ## Two judgement calls worth reviewing **The `#2632` string still appears twice**, in a new note recording that the row previously named it and why that was wrong. That is a refutation, not an attribution; it is there so the error is not silently reintroduced. Say the word if you would rather it be dropped entirely. **`check-silent-revert.sh` said the canary "fires 5 times" over 500 commits.** Measured per commit: `f603880da` 1 finding, `9239f1541` 1, `cc58cbc53` 2, `6f0a31109` 1 (pre-acknowledgment, 390 lines), `91e77fc16` 1 (pre-acknowledgment, 340 lines) — 5 commits, 6 findings. The number was counting commits, so the count is unchanged; the units are now stated so the 6-vs-5 gap is not read as an error. ## Blast radius Comments and free-text fixture notes only. - `diff <(git show HEAD~1:scripts/check-silent-revert.sh | grep -v '^\s*#') <(grep -v '^\s*#' scripts/check-silent-revert.sh)` is empty — no non-comment line of the detector changed. - No threshold, window, or expectation moved. The fixture parser reads `expect sha note`; only `note` text changed, and both `expect` and `sha` are untouched. - `shellcheck`, `bash -n`, `actionlint`, and `typos` all pass. `scripts/check-silent-revert.sh --verify-known-incidents` after the change: ``` ok f603880 fires as recorded (#2639 dropped #2635 (346 lines), 13 minutes later) ok 9239f15 fires as recorded (#2641 dropped #2639 (451 lines), 10 minutes later) ok cc58cbc fires as recorded (#2633 dropped #2644's rollups (853 blamed lines) and #2642's GraphQL merge evidence (301); already recorded in #2656) ok c8470ef 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. ``` ## Related - Refs #2808 — the PR that merged the canary and introduced the misattributed corpus row. - Refs #2691 — the original silent-revert audit the corpus is built from. Its two incidents (#2639/#2635 and #2641/#2639) were already recorded correctly and are untouched here. - Refs #2656 — the pre-existing record of this merge event, now cited by the corpus in place of the "unfiled until now" claim. Stays CLOSED/COMPLETED. - Refs #2644, #2642, #2633, #2640 — the merges measured above. Refs #2632, which is not one of them: it never merged, which is the whole point. - Refs #2833 — follow-up raised in review here: the replay checks only that a recorded commit still fires, never which culprit or how many lines, so a row can keep printing a reproduction it no longer performs. Pre-existing, and out of scope for a change constrained to leave the detector untouched. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_018S8a1S71VxhLTRWBtMuEvp --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2601
Summary
reclaimable-worktreerested only on emptygit status --porcelain, so ignored files were invisible and would be destroyed without warning on removal.Fix
Account for ignored content when classifying reclaimable worktrees — do not present a safe-to-delete finding when ignored files would be destroyed without warning.
Verification
See collector tests on PR checks.
Related
Refs #2597 — fleet hygiene epic.
Refs #2605 — overlap with source-control stranded-work axis.