fix(disk-hygiene): restore the README and evals #2635 never got back (#2828) - #2829
Conversation
PR #2635 changed seven files. The stale-base squash in #2639 deleted them all; #2714 restored four and #2803 restored four, and the overlap left README.md and skills/clean/evals/evals.json unrestored on main. The silent-revert canary cannot see this: the deleting commit is already an acknowledged incident, and the canary has no mechanism for noticing that a recorded incident's restoration was only partial. The README's overview, approval-contract bullet, and deletion-report paragraph lead with tidiness again, and eval 12 (empty-directories-remain-first-class-tidiness-findings) is back alongside eval 1's provenance-first expectation. The approval bullet is rewritten rather than restored verbatim. #2635's own hunk was malformed: it rewrote the bullet while leaving the original trailing lines in place and duplicating the following "Filesystem roots..." bullet in truncated form. The replacement matches SKILL.md:305-307, which is the authority on what the approval table names. Closes #2828 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RPRAvSLuRmR6rPQeCvdok6
|
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 finished @kyle-sexton's task in 1m 34s —— View job Code review: restore README and evals #2635 never got back
No correctness or maintainability issues found. This is a clean, low-risk documentation/eval restoration:
I wasn't able to execute |
|
Claude finished @kyle-sexton's task in 46s —— View job Security Review — No Findings
Skip gate: change has no security-relevant surface. All four changed files are documentation, changelog, an eval-prompt JSON fixture, and a semver bump:
None of this touches authentication/authorization, input handling, command/path/template construction, secrets or tokens, or GitHub Actions workflow permissions/triggers. The PR itself is explicit that the engine ( No security issues found. |
|
Last security-reviewed head: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f527131e7
ℹ️ 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 has reviewed this PR 3 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
Review found that eval 12 as #2635 wrote it marks the correct gated response as a failure. Its prompt targeted `C:\` directly, but SKILL.md requires an OS-managed volume root to be addressed through `--root-children` with explicit `--root-child` names after the confirmation gate clears, and the engine rejects a direct root scan. An eval whose expected output is only reachable by bypassing that gate rewards bypassing it. Point the prompt at five explicitly selected children so the tidiness ranking is reachable inside the documented safe flow, and add a fourth expectation pinning that the ranking request is never authority to skip the gate. Root gating itself stays eval 11's subject. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RPRAvSLuRmR6rPQeCvdok6
|
Claude finished @kyle-sexton's task in 1m 12s —— View job Code review: restore README and evals #2635 never got back
No correctness or maintainability issues found on the current head ( The prior Codex review flagged eval 12 ( Checked beyond that:
No further findings to report. |
|
Claude finished @kyle-sexton's task in 47s —— View job Todo
Security Review — No FindingsSkip gate: change has no security-relevant surface. This PR was already reviewed at Across the full diff (
No No security issues found. |
…hangelog The entry said eval 12 "is back" while scoping the not-restored-verbatim caveat to the README approval bullet alone. Three of the four restored surfaces are corrected rather than restored: the approval bullet, the deletion-report enumeration, and eval 12's prompt. Only eval 1's expectation is byte-identical to #2635. In a repo that keeps scripts/silent-revert-incidents.txt precisely because "restored" records went unaudited, a changelog that misstates what was restored is the failure mode this change exists to correct. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RPRAvSLuRmR6rPQeCvdok6
|
Claude finished @kyle-sexton's task in 1m 40s —— View job Code review: restore README and evals #2635 never got back
No correctness or maintainability issues found on the current head ( This is the third automated review pass on this PR; the one real defect a prior pass caught (eval 12's
No inline findings to post. |
…ts corpus (#2986) #2917 item 2 named the same unhedged `31h28m` claim in two files. #2930 corrected `scripts/check-silent-revert.sh` and left `scripts/silent-revert-incidents.txt` byte-identical, so since #2930 the two files have actively contradicted each other on the one question the whole restoration assertion turns on. This finishes item 2 and clears the two smaller prose defects the same review pass found. ## 1. The surviving claim in the corpus file `scripts/silent-revert-incidents.txt` said: > It says nothing about whether the content that commit deleted is on main today, > and that gap has already cost this repo 31h28m: the content #2828 is about was > missing from main while the f603880 row below printed `ok ... fires as > recorded` and the replay exited 0. The defect is that "that gap has already cost this repo 31h28m" charges the whole absence to a mechanism that existed for only part of it, and it does so in the same breath as "the replay exited 0" — so the sentence reads as thirty-one hours of a running, silent replay. It was not. The canary did not exist for most of that window. Rewritten to separate the two windows in the same terms the script now uses, and to keep the point the passage was carrying — that the replay's silence was never a statement about content presence: > The content #2828 is about was absent from main for 31h28m: f603880 removed it > and 534eac1 (#2829) put it back. > > 31h28m is the CONTENT-ABSENCE window, not a window this replay ran through in > silence -- the canary did not exist for most of it. It merged part-way in, at > 7b47d22 (#2808), 6h13m before the restore, so the replay covered only the tail > of the absence. Over that tail the f603880 row below printed > `ok ... fires as recorded` and the replay exited 0 while the content was still > gone: the row was never wrong about what it asserted, it asserted the other > question. Two re-lands (#2714, #2803) each missed it, and a hand audit caught it, > not this file. Both figures are closed historical intervals between two fixed commits, so neither can rot — that is why they are stated rather than replaced with a hedge. Neither is a running total. ### What the two figures measure Both are committer-date deltas, re-derived here rather than carried over: | interval | endpoints | seconds | figure | | --- | --- | --- | --- | | content absence | `f603880da` (1786752447) → `534eac138` (1786865745) | 113298 | 31h28m18s → **31h28m** | | replay presence | `7b47d2253` (1786843322) → `534eac138` (1786865745) | 22423 | 6h13m43s → **6h13m** | `7b47d2253` (#2808, the commit that merged this canary) is an ancestor of `534eac138` and its timestamp falls inside the absence interval, so the canary genuinely merged part-way through. The replay therefore covered 22423 of 113298 seconds — under a fifth — and the prose says "only the tail" rather than quoting that fraction, matching the script. ## 2. The broken antecedent in `check-silent-revert.sh` In the `The restoration assertion (#2855)` comment block, one paragraph establishes that marker text is matched with `grep -F` and therefore "may contain any character -- no separator is reserved inside it". The next sentence a reader needs is "The ONE reserved position is a LEADING `[`". #2930 inserted its ~30-line uniqueness block ("a marker must occur exactly ONCE in the file it binds to", plus the definition/use two-row design and the corpus-review obligation) between them, and did so without even a blank comment line before "The ONE reserved position", so that sentence ran straight out of a paragraph about counting occurrences and its antecedent sat about thirty lines uphill. Fixed by relocating the uniqueness block below the disposition paragraphs, which restores the original adjacency: whitespace/any-character → `[` is the one reserved position → the disposition needs a non-empty reason → the disposition ends at the first `]`. The uniqueness block now closes the section. Two connective repairs were needed because the block no longer follows the sentence it used to lean on, and both preserve what is asserted: - `For the same reason a marker must occur exactly ONCE` → `Literal matching is also why a marker must occur exactly ONCE`. "The same reason" was the looseness of literal substring matching, which the block's own next sentence (``grep -qF` answers "is this string anywhere in this file"`) immediately restates, so naming it costs nothing and removes the anaphor. - `Nothing enforces this:` → `Nothing enforces the once-rule:`, since "this" now has no adjacent referent. Nothing else in the block changed — the diff is a pure relocation apart from those two clauses. ## 3. The distributing parenthetical Both files claimed that both `f603880da` markers occur in the same three sibling sites: > Both markers on the f603880 row also occur elsewhere in the same plugin on > current main -- the engine script, its test file, CHANGELOG.md That reads as three sites applying to both markers. Re-measured from scratch on `origin/main` with `git grep -F` per literal, no pathspec: **README marker** (bound to `plugins/disk-hygiene/README.md`) — 5 hits: - `plugins/disk-hygiene/README.md:6` (the bound file) - `plugins/disk-hygiene/skills/clean/scripts/hygiene.py:2807` - `plugins/disk-hygiene/skills/clean/scripts/hygiene.py:3601` - `plugins/disk-hygiene/skills/clean/scripts/test_hygiene.py:566` - `scripts/silent-revert-incidents.txt:107` (the marker row itself) No CHANGELOG hit. **evals marker** (bound to `plugins/disk-hygiene/skills/clean/evals/evals.json`) — 3 hits: - `plugins/disk-hygiene/skills/clean/evals/evals.json:138` (the bound file) - `plugins/disk-hygiene/CHANGELOG.md:139` - `scripts/silent-revert-incidents.txt:108` (the marker row itself) No engine-script or test hit. So the sites do not distribute: the engine script and its test belong to the README marker only, CHANGELOG.md to the evals marker only. The load-bearing claim survives untouched — each literal does occur outside the file it binds to, which is why a repo-wide grep would report the incident restored on a tree where it is not — and the prose now says that in the per-marker form. Neither rewrite quotes either literal. Doing so would have added an occurrence of the very string whose occurrence set the sentence enumerates; the prose refers to them by role ("the README marker", "the evals marker"), which is what the script already did. Nor does either rewrite assert exhaustiveness, since both literals also appear on the corpus page itself — a fact that file already states separately. ### The provenance sentence had to move with it The script's old follow-on read: > Stated in the present tense on purpose: the sibling copies are a property of main > as it stands now, not of the tree during the #2828 window. Measurement contradicts that for the README half, so it could not be left standing next to a corrected parenthetical: - The evals half holds. At `f603880da` the evals literal occurred nowhere in the tree, and a `git log -S` walk over the whole `f603880da..534eac1` range returns exactly one commit — `534eac138` itself. So `plugins/disk-hygiene/CHANGELOG.md` acquired its copy at the restore, and the interval is measured rather than interpolated between endpoints: a repo-wide grep for that literal matched nothing at any point during the absence. - The README half does not. `git log -S` over `f603880da..534eac1` returns `94ae28728` (#2803, one of the two re-lands the passage already names as having missed the README) and then the restore. `94ae28728` put the string into `hygiene.py` and `test_hygiene.py` and never into `README.md`, and it landed 2026-08-15T20:37:17-04:00, which is 44m45s before `7b47d2253` merged this canary. Grepping at `7b47d2253` confirms it: three hits, all in the engine script and its test, none in `README.md`. So for the entire tail of the absence that the replay actually covered, a repo-wide grep for the README marker matched on a tree whose bound file did not have the content. The passage now says that, which turns the path-scoping argument from a hypothetical into the answer this very incident would have produced. ## Scope note The parenthetical defect is called out in #2981 against `check-silent-revert.sh`, but `silent-revert-incidents.txt` carried its own shorter copy of the same claim. Fixing only one would have re-created exactly the two-files-disagree condition this issue exists to close, so both were corrected — the corpus version kept shorter, to that file's register. ### A third site carries the same defect, and is deliberately NOT touched here Verification swept `31h28m` across the whole tree rather than only the two files in scope, and turned up a site neither #2917 nor #2981 names — `.github/workflows/silent-revert-canary.yml:154–157`, the workflow that actually runs the replay: > Both are needed, and the gap between them has already cost 31h28m: the content > #2828 is about was missing from main while the replay printed `ok` and exited 0 Same unhedged construction, charging all 31h28m to a period in which "the replay printed `ok`". It is pre-existing — `git diff origin/main` on that path is empty — and it is outside the file scope agreed for this change, so it is left alone here rather than folded in unannounced. It should be filed and fixed, and the correction is already derived above: the content was absent for 31h28m, of which this replay was present only for the final 6h13m, and over that tail it printed `ok` and exited 0 while the content was still gone. Recording it here so the remaining inconsistency is a known, measured follow-up rather than something a later reader rediscovers as evidence that this fix was incomplete. ## Verification CI is the verification of record; the shell suites were deliberately not run locally (they behave badly on Windows). Everything asserted above was measured with `git log`, `git rev-parse`, `git merge-base`, `git log -S`, and `git grep -F`, at explicit revs. A fresh-context verifier subagent then re-derived every figure independently, with the reasoning above withheld: both committer-date windows and their arithmetic, the ancestry and interval placement of `7b47d2253` and `94ae28728`, the complete per-literal occurrence sets on `origin/main` and on the branch tree (confirming the edits added no new occurrence of either literal), the adjacency repair in part 2 and that the relocated block's claims are unchanged, that every `31h28m` mention in the two edited files is now hedged and window-separated, and that the corpus file contains no stray non-comment line that would make the parser exit 2. It returned PASS on all sixteen claims, and its repo-wide sweep is what surfaced the workflow site noted above. The edits also add no line over 80 columns to either file — the over-80 counts are identical to `origin/main`'s (61 in the script, 13 in the corpus, all pre-existing). ## Related Closes #2981 - #2917 — the parent issue, whose item 2 this completes - #2930 — corrected the script half and left the corpus half behind - #2855 — added the restoration markers and the comment block part 2 repairs - #2847 — the standard requiring a figure that does not rot Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…nt (#3066) ## Summary `.github/workflows/silent-revert-canary.yml` was the third site of the claim #2917 set out to remove. Its comment charged the whole `31h28m` content-absence window to the replay's silence — so the workflow contradicted the two `scripts/` files the job it documents actually invokes. This brings the last of the three sites into agreement. ## Fix Comment text only, in one hunk. No `run:`, `uses:`, `with:`, or any other executable YAML was touched (`git diff --numstat` = `6 3` on that one file; every `+`/`-` line begins with ` #`). Before: > incidents deleted is on the tree TODAY. Both are needed, and the gap > between them has already cost 31h28m: the content #2828 is about was > missing from main while the replay printed `ok` and exited 0, two > re-lands each missed it, and a hand audit found it, not this lane. After: > incidents deleted is on the tree TODAY. Both are needed, and #2828 > is the gap made real: its content sat off main for 31h28m, from > f603880 to the 534eac1 (#2829) restore. This lane covered only > the tail -- it merged at 7b47d22 (#2808), 6h13m out from the > restore -- and across that tail the replay still printed `ok` and > exited 0 with the content gone: it answered the other question. Two > re-lands each missed it, and a hand audit found it, not this lane. The load-bearing point is preserved rather than deleted: the replay's silence still did not mean the content was present. What changes is which window that silence is charged to. "Both are needed" is kept deliberately — the `ORDER IS LOAD-BEARING` paragraph immediately below reads off it. The wording is the workflow's own rather than a lift from either sibling; in particular it keeps this file's "not this lane" voice instead of the scripts' "not this canary" / "not this file". ## Verification Both figures re-derived here from committer dates (`git show -s --format=%ct`), not carried over from the issue: | window | from | to | seconds | rendered | | --- | --- | --- | --- | --- | | content absence | `f603880da` `1786752447` | `534eac138` `1786865745` | `113298` | 31h28m | | replay presence | `7b47d2253` `1786843322` | `534eac138` `1786865745` | `22423` | 6h13m | `113298s` = 31h28m18s and `22423s` = 6h13m43s; both are floored to the minute, matching the convention the two sibling files already use. `7b47d2253` is an ancestor of `main` (`git merge-base --is-ancestor` exit 0) and its committer epoch falls strictly inside the absence interval, which is what makes "only the tail" true. Repo-wide sweep for further sites: swept all tracked files for `31h28m`, `31h`, `6h13m`, `113298`, `22423`, the three incident shas, and numberless restatements (`exited 0 for`, ``printed `ok` ``, `while the replay`, `absence window`). **No fourth site exists.** The only near-hit outside the three known files is `plugins/claude-ops/skills/lanes/scripts/restart-consumer.test.sh:572` — "`lock-held` and exited 0 forever" about a Task Scheduler consumer — an unrelated sentence that merely shares the words. No follow-up issue needed. The change is comment-only, so no test asserts it; CI is the verification of record for the surrounding workflow, and the shell suites were not run locally (they run badly on this machine's platform). ## The other two sites Both siblings are already on `main` and untouched here — #2930 corrected `scripts/check-silent-revert.sh`, and #2986 (merged as `0e9ed8eb0`) corrected `scripts/silent-revert-incidents.txt`. This branch is merged up to a `main` that carries both, so all three passages can be read side by side in one tree. The workflow was the last holdout. ## Related - #2917 — the parent issue that named this claim - #2930 — corrected `scripts/check-silent-revert.sh` - #2986 — corrected `scripts/silent-revert-incidents.txt` - #2847 — the standard this follows: state the claim that stays true rather than renumbering a figure that rots Closes #2988 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Summary
PR #2635 changed seven files. The stale-base squash in #2639 deleted them all 13 minutes later —
the incident already recorded in
scripts/silent-revert-incidents.txt. PR #2714 restored four andPR #2803 restored four; the overlap left
plugins/disk-hygiene/README.mdandplugins/disk-hygiene/skills/clean/evals/evals.jsonunrestored onmain.The silent-revert canary cannot see this. It detects deleting commits, and the deleting commit here
(
f603880da) is already an acknowledged incident — it has no mechanism for noticing that a recordedincident's restoration was only partial.
What changed
README.md— all three feat(disk-hygiene): prioritize tidiness over reclaimable bytes in reports #2635 hunks:secondary signal;
risk, with byte counts last;
evals.json— eval 12empty-directories-remain-first-class-tidiness-findingsrestored, andeval 1's third expectation restored to the provenance-first wording.
0.20.10with a CHANGELOG entry.Three of the four surfaces are corrected, not restored verbatim
Only eval 1's expectation is byte-identical to #2635. The other three each deviate deliberately, and
each deviation is disclosed in the CHANGELOG as well as here.
1. The README approval bullet — #2635's own hunk was malformed. It rewrote the bullet while
leaving the original two trailing lines in place and duplicating the following "Filesystem roots..."
bullet in truncated form. Restoring it byte-for-byte would re-introduce a broken list. The
replacement matches
SKILL.md§5, which is the authority on what the approval table names:That is also why
and its logical byte countsurvives in the bullet rather than being dropped theway #2635 dropped it — the approval genuinely still names byte counts, last.
2. The README deletion-report paragraph keeps its skip enumeration. #2635 collapsed "removed,
locked, changed, protected, needs-elevation, and unverified outcomes" to the single word "skips".
This PR keeps the enumeration, which matches
SKILL.md§6.3. Eval 12's prompt is retargeted through the documented
--root-childrenselection. As #2635wrote it, the prompt scanned
C:\directly.SKILL.mdis explicit that--root-childrenis theonly way to address an OS-managed volume root, and the engine has always enforced it —
hygiene.pyraises
OS-managed roots are not valid audit targets, and--root-childrenwithout names returnsroot-children-selection-required. Both were already true at #2635's own commit. So the eval'sexpected output was reachable only by bypassing the confirmation gate, which means the eval graded
the correct gated refusal as a failure. The prompt now names five explicitly selected children, and
a fourth expectation pins that a ranking request is never authority to skip the gate. Root gating
itself stays eval 11's subject.
Scope note
The engine half is already complete on
mainand is not touched here. Verified against mergedcontent, not PR state:
empty_directory_count(hygiene.py:852, emitted at:1446and:3432),preview
empty_directory/empty_directories(:2639,:2653), applypaths_removed/empty_directories_removed(:3170-3171), andvalidate_plan'sprovenance/riskenforcement(
:1609-1652). The four tidiness tests were executed againstorigin/maincontent and pass (twoskip on a documented Linux-only platform gate).
Test plan
scripts/validate-plugins.sh— all manifests and the catalog validatescripts/check-changelog-parity.sh --check— passscripts/check-changelog-parity.sh --check-order— pass, 78 changelogs newest-firstscripts/check-changelog-parity.sh --check-bump origin/main— passscripts/check-changelog-parity.sh --check-preserved origin/main— pass, 64 headings comparedplugins/skill-quality/scripts/check-evals-quality.sh— pass (1 pre-existing warning on eval 9)evals.jsonvalidates againstplugins/skill-quality/reference/evals.schema.json— 0 errorsmarkdownlint-cli2onREADME.mdandCHANGELOG.md— 0 errorsbiome checkon both changed JSON files — cleanRelated
Closes #2828. Refs #2590, #2635, #2639, #2691, #2714, #2803.