fix(disk-hygiene): re-land tidiness-first reporting and correct the belt's documented posture - #2714
Merged
Merged
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 encountered an error after 0s —— View job I'll analyze this and get back to you. |
Contributor
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Warning Automated security review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
The check is green on purpose, and it is not evidence. It certifies that a security pass ran, and this one did not complete — but the cause is outside this PR's control, so merging is deliberately left unblocked rather than locking every merge for the length of the outage. Nothing was reviewed at this head. Where this check is required, it is satisfied without that evidence; a human should review security-sensitive changes here before merging. Re-run the job, or workflow_dispatch this workflow with the PR number, to retry the review (ci-workflows#227). A new push also retries it only if the caller's Re-running does NOT help for every class:
|
![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: e4ecf26b39
ℹ️ 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".
…elt's documented posture Re-lands the report-ordering fix from #2635 (reverted by #2639's stale base) and the session-lifetime honesty from #2639 (reverted by #2641's stale base), then corrects three further documentation defects in the clean skill's two operator- facing markdown surfaces. F1 (#2590) — reports are ordered by tier and evidence strength, never by byte size: provenance/what-it-is/why-removable/risk lead, bytes come last; empty directories are first-class findings distinguished from not-walked coverage gaps; `provenance` and `risk` return to the plan schema; §5's preview table and §6's apply summary lead with tidiness rather than bytes. F5(a) (#2618) — the frontmatter PreToolUse belt is session-lifetime, not skill-scoped. Both markdown sites now say so, sourced from the skills reference ("registers when the skill is invoked and keeps running for the rest of the session"), and name the allowed-tools/hooks asymmetry that made the narrower claim plausible. The third site (destructive_guard.py's resolve_mode docstring) is covered by a sibling PR. F6 (#2618) — SKILL.md claimed the belt "still launches in exec form via python3", contradicting its own frontmatter and safety-model.md: it has been shell form since 0.17.9 (#2568). The false statement and the conclusions drawn from it (a silently-inert belt, defense-in-depth "lost, not preserved", #2568 unconverted) are removed rather than reworded; safety-model.md's accurate account, including the real residual fail-open, is the single copy. F4 (#2618) — step 6.2 listed three ways reversible removal silently becomes permanent but omitted path length, which fails differently: beyond MAX_PATH (260) a path cannot reach the Recycle Bin at all, so the only fallback is a permanent delete through a long-path API. That fallback now requires its own explicit irreversible-action approval instead of inheriting the tier approval given for reversible removals. F3 (#2618) — §3 now states that relocation is out of scope: the skill offers keep-or-delete only, and a move is the operator's own action outside the workflow. F7 (#2618) — the Gotchas section carried ~56 lines of harness mechanics already documented in full by reference/safety-model.md; hand-maintained duplication is how F6's stale bullet survived a fix to the reference. Those bullets are replaced with load-when pointers, leaving the engine-behavior and operator- actionable gotchas in place. Gotchas 77 -> 35 lines; SKILL.md 490 -> 495 net, the other findings having added required content, and back under the 500-line skill-quality cap it had 10 lines of headroom against. Also corrects safety-model.md's claim that Move-Item/Rename-Item "reach the tool with no guard verdict at all" — stale in the unsafe-sounding direction since #2470 gated move/rename/overwrite/volume spellings and closed #387. The lane is still enumerated rather than fail-closed, so the residuals are named concretely. Closes #2590 Refs #2618 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cursor
Bot
force-pushed
the
docs/2590-2618-report-order-and-doc-honesty
branch
from
August 15, 2026 19:53
e4ecf26 to
88c3191
Compare
This was referenced Aug 15, 2026
Contributor
|
Claude finished @cursor[bot]'s task in 23s —— View job Task list
Result: review skipped
If a re-review is needed for this change, it should target it while open, or a fresh review can be requested against the merged commit via a new PR/branch. |
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
This was referenced Aug 15, 2026
Closed
kyle-sexton
added a commit
that referenced
this pull request
Aug 16, 2026
## Summary #2590 stayed open: #2714 restored skill prose, but the engine half from #2635 (`empty_directory_count`, plan `provenance`/`risk`, preview/apply tidiness fields) was still absent on `main`. Re-lands that engine work and tests. ## Test plan - [x] `python3 -m unittest test_hygiene.HygieneTests` — 94/94 pass (focused empty-dir / provenance / apply cases included) ## Related Closes #2590 Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
kyle-sexton
added a commit
that referenced
this pull request
Aug 16, 2026
…2828) (#2829) ## 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 and PR #2803 restored four; the overlap left `plugins/disk-hygiene/README.md` and `plugins/disk-hygiene/skills/clean/evals/evals.json` unrestored on `main`. 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 recorded incident's restoration was only **partial**. ## What changed - **`README.md`** — all three #2635 hunks: - the overview now states that safe tidiness is the primary objective and reclaimable bytes are a secondary signal; - the approval-contract bullet now names provenance, what the entry is, why it is removable, and risk, with byte counts last; - the deletion-report paragraph now leads with tidiness outcomes and records bytes as secondary. - **`evals.json`** — eval 12 `empty-directories-remain-first-class-tidiness-findings` restored, and eval 1's third expectation restored to the provenance-first wording. - Version bumped to `0.20.10` with 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: > show a table naming every path with provenance, what it is, why removable, risk, whether it is an > empty directory, the single tier, and only then logical / reclaimable bytes That is also why `and its logical byte count` survives in the bullet rather than being dropped the way #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-children` selection.** As #2635 wrote it, the prompt scanned `C:\` directly. `SKILL.md` is explicit that `--root-children` is the only way to address an OS-managed volume root, and the engine has always enforced it — `hygiene.py` raises `OS-managed roots are not valid audit targets`, and `--root-children` without names returns `root-children-selection-required`. Both were already true at #2635's own commit. So the eval's expected 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 `main` and is not touched here. Verified against merged content, not PR state: `empty_directory_count` (`hygiene.py:852`, emitted at `:1446` and `:3432`), preview `empty_directory` / `empty_directories` (`:2639`, `:2653`), apply `paths_removed` / `empty_directories_removed` (`:3170-3171`), and `validate_plan`'s `provenance`/`risk` enforcement (`:1609-1652`). The four tidiness tests were executed against `origin/main` content and pass (two skip on a documented Linux-only platform gate). ## Test plan - [x] `scripts/validate-plugins.sh` — all manifests and the catalog validate - [x] `scripts/check-changelog-parity.sh --check` — pass - [x] `scripts/check-changelog-parity.sh --check-order` — pass, 78 changelogs newest-first - [x] `scripts/check-changelog-parity.sh --check-bump origin/main` — pass - [x] `scripts/check-changelog-parity.sh --check-preserved origin/main` — pass, 64 headings compared - [x] `plugins/skill-quality/scripts/check-evals-quality.sh` — pass (1 pre-existing warning on eval 9) - [x] `evals.json` validates against `plugins/skill-quality/reference/evals.schema.json` — 0 errors - [x] `markdownlint-cli2` on `README.md` and `CHANGELOG.md` — 0 errors - [x] `biome check` on both changed JSON files — clean ## Related Closes #2828. Refs #2590, #2635, #2639, #2691, #2714, #2803. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Aug 17, 2026
…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>
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 #2590
Summary
Re-lands tidiness-first reporting (#2590) and corrects the clean skill's documented belt posture for the #2618 findings that belong on the markdown surfaces (
SKILL.md,reference/safety-model.md). Cut from currentorigin/mainso this does not silently revert under the #2691 stale-base squash-merge defect.Fix
provenance/riskrestored to the plan schema; preview/apply lead with tidiness.disk-hygiene:clean: frontmatter Bash belt is session-lifetime, not skill-scoped (+6 findings from a post-use audit) #2618 docs lane): session-lifetime honesty, delete the false exec-form claim, long-path Recycle Bin caveat, relocation out of scope, thin Gotchas intosafety-model.mdpointers.safety-model.md's stale claim thatMove-Item/Rename-Itemreach the tool ungated (fixed since fix(disk-hygiene): gate PowerShell move/rename/overwrite spellings #2470).destructive_guard.py/hygiene.py— sibling lane (fix(disk-hygiene): re-land the guard allowlist and harden its trust anchor #2706).Deliberately not
Closes #2618: remaining findings belong to the guard lane (#2706). Closing it from here would falsely close it a second time.Verification
npx markdownlint-cli2 --config .markdownlint-cli2.jsoncon both files → 0 errorstypos --config _typos.toml→ exit 0editorconfig-checker→ exit 0scripts/check-changed-skills.sh origin/main→SKILL.mdunder 500-line cap; base-ref trigger phrases preserved0.20.6with changelog entry so cached installs receive the restored skillRelated
disk-hygiene:clean: frontmatter Bash belt is session-lifetime, not skill-scoped (+6 findings from a post-use audit) #2618 — docs-lane remediation only; guard half is fix(disk-hygiene): re-land the guard allowlist and harden its trust anchor #2706