Skip to content

fix(disk-hygiene): make the confirmation gate answerable and the alias probe reachable - #2016

Merged
kyle-sexton merged 4 commits into
mainfrom
fix/disk-hygiene-stranded-review-threads
Aug 8, 2026
Merged

fix(disk-hygiene): make the confirmation gate answerable and the alias probe reachable#2016
kyle-sexton merged 4 commits into
mainfrom
fix/disk-hygiene-stranded-review-threads

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Three stranded review findings against disk-hygiene, all in normative prose. No behavior change to
any script; the diff is four files and the plugin manifest bump.

What was wrong

The confirmation gate applied one acceptance bar to four different questions. The bar demanded an
answer "naming exactly the tier and path list just shown". Two of the four questions it governs have
shown neither at the point they are asked — the no-target prompt asks the user to supply a
directory, and §1's large-scan confirmation is asked before the walk that would produce a tier or a
path list. Neither could be cleared by any reply a human could give, and one of them gates an
unbounded whole-volume walk. The surface rule and the answer floor are genuinely common to all four,
so they stay shared; what an answer must name is now stated per question in a table. The gate also
now states the obligation that produced the defect in the first place: ask each question so it shows
what its row requires the answer to name.

The gate fell back to an inline question only when AskUserQuestion was absent. Per
permissions (fetched 2026-08-08), a bare tool name in
permissions.deny "removes the tool from Claude's context entirely", while dontAsk mode denies
AskUserQuestion "even if you've allowed them" — the tool stays visible and every call fails.
Absence and denial are distinct states, and the skill named dontAsk as a cause while keying the
trigger on the one state it does not produce. An agent that checks its tool list would see the tool,
skip the inline branch, call it, be denied, and find no recovery in the text. The fallback now
triggers on absent, denied, or otherwise unusable, including a denial discovered only by calling it.

The python3 alias probe was routed through an interpreter that may be unable to run it. setup
step 1(b) launches the bundled inspect-only probe with "any interpreter already proven real" and
gates the PowerShell escape hatch on no such interpreter existing. But the probe carries
from __future__ import annotations (optional release 3.7.0b1) and PEP 604 unions, and CPython
raises SyntaxError at compile time for a future feature it does not define — so on Python 3.6 the
probe dies before emitting any JSON. The same section already names 3.6 as an interpreter that
rejects guard-shaped code, then routed an identically-shaped probe through it. A 3.6 interpreter is
not absent, so the literal reading never reached PowerShell and the check failed closed with no
detail to remediate from. The fallback now also covers a chosen interpreter that emits no verdict.

Scope

A fourth finding on this branch (_discard_stream re-closing the descriptor it just repaired) was
already discharged on main before this branch was reopened: the module tail is
os._exit(_exit_code), so the exit-status leak the thread reported no longer reproduces regardless
of what the fallback does. The commit that had added a fix and a test for it is reverted here rather
than carried — the residual descriptor defect is real but maps to no finding this branch owns.

The dontAsk defect has an exact twin at plugins/repo-hygiene/skills/clean/SKILL.md:85, from the
same original PR. It is deliberately not fixed here: changelog parity is per-plugin, and folding
a second plugin in would require its own bump and entry. Reported separately for its own change.

Verification

  • Both permissions-doc quotes read verbatim off the live page this session, not from recall.
  • The 3.7 floor and CPython's compile-time rejection of an undefined future feature were read off a
    live interpreter (__future__.annotations.getOptionalRelease()), not asserted.
  • check-changelog-parity.sh --check / --check-order / --check-bump, check-changed-skills.sh,
    check-shell-portability.sh, and markdownlint-cli2 all pass locally.
  • test_hygiene.py 242 pass, python3_alias_probe.test.sh 10 pass — unchanged, as expected for a
    prose-only diff.
  • 0.15.0 steps over 0.14.0, which is claimed by open PR fix(disk-hygiene): qualify unknown and non-reclaimable byte counts #1870 against the same manifest.

Related

Addresses review threads on #1124, #1738.

No linked issue

kyle-sexton and others added 3 commits August 6, 2026 18:24
Four stranded post-merge review findings, three of them the same bug class: a
condition keyed on a surface being *absent* when the real trigger is
present-but-unusable.

- The confirmation gate held every question to one bar -- an affirmative naming
  exactly the tier and path list just shown -- including the no-target prompt and
  the large-scan confirmation, neither of which has shown a tier or a path list.
  The surface rule and the answer floor stay common to all four questions; what
  an answer must name is now stated per question, and the two cross-references
  that asserted the deletion bar applies unchanged now name their row.

- The gate fell back to an inline question only when AskUserQuestion was absent.
  Permission mode dontAsk denies the tool even when an allow rule names it,
  leaving it visible while every call fails; only a bare-name deny rule removes
  it from context. The fallback now triggers on absent or denied.

- setup step 1(b) routed to the PowerShell signal only when no alternate
  interpreter existed at all. A real-but-incompatible launcher -- Python 3.6,
  which the same section already names as unable to run a future-import -- is not
  absent, so the check had no path to a verdict. The fallback now also covers a
  chosen interpreter that emits no verdict.

- _discard_stream re-closed the descriptor it had just repaired: os.open returns
  the lowest free fd, so a target fd closed outright comes back on its own
  number, dup2 is a no-op, and the unconditional close in the finally undoes the
  repair. The module's os._exit tail means no exit-status regression rode on
  this, so this is defense in depth; a new test asserts fd 2 is writable after
  the repair rather than the exit code that survives either way.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Drop the `_discard_stream` change and its test. The review thread that
prompted them is already discharged on main: the module tail is
`os._exit(_exit_code)`, so the exit-status leak the thread reported no
longer reproduces regardless of what the fallback does. The remaining
descriptor defect is real but unrelated to any finding this branch
carries, and belongs in its own change.

Close two enumerations that a literal reader could fall out of:

- The question-surface rule listed absence and denial as the only two
  unusable states. It now reads absent, denied, or otherwise unusable,
  including a denial discovered only by calling the tool.
- The per-question table said what an answer must name but never
  obliged the question to show it, which is the exact shape of the
  defect the table was written to fix.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cursor

cursor Bot commented Aug 8, 2026

Copy link
Copy Markdown

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 Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

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


Security review — PR #2016

  • Fetch PR diff (gh pr diff 2016) and read all four changed files
  • Review plugin.json version bump
  • Review CHANGELOG.md prose
  • Review skills/clean/SKILL.md confirmation-gate rewrite
  • Review skills/setup/SKILL.md python3-probe fallback rewrite
  • Assess whether either rewrite weakens the destructive-action confirmation gate or the Windows alias-stub check
  • Post findings

Scope confirmed: all four changed files are prose/manifest — plugin.json (version bump only),
CHANGELOG.md, and two SKILL.md files. No shell/Python/JS executed by the plugin changed, and no
GitHub Actions workflow is touched, matching the PR description ("No behavior change to any script").

No security issues found.

This PR's two substantive edits both touch security-relevant logic (the destructive-action
confirmation gate in clean/SKILL.md, and the Windows python3-App-Execution-Alias probe in
setup/SKILL.md), but in both cases the change closes a gap rather than opening one:

  • Confirmation gate (plugins/disk-hygiene/skills/clean/SKILL.md, "Confirmation gate" section):
    the old text keyed the inline-question fallback on AskUserQuestion being absent. Under
    dontAsk permission mode the tool stays visible but every call is denied — a state the old text
    never named, so an agent that checked its tool list could call the tool, get denied, and have no
    documented recovery for a gate that's supposed to block destructive deletion. The new text widens
    the fallback trigger to "absent, denied, or otherwise unusable" and states explicitly that "a
    denied call is an unanswered question, never an answer." That's a strictly wider (safer) trigger
    condition — the four questions it governs (no-target prompt, large-scan confirmation, removal
    approval, unsupported-platform handoff) all still funnel through the same floor ("the user's own
    answer... never supply, infer, or fabricate it... on rejection, stop"). The new per-question table
    only varies what the content of a valid answer must name — the two non-destructive questions
    (target selection, scan scope) get an achievable bar, while the two questions that gate actual file
    removal (§5 removal approval, §6 manual handoff) keep the original strict "exactly the one tier and
    the exact path list just shown" bar unchanged. No weakening of the deletion-approval bar.

  • python3 alias probe (plugins/disk-hygiene/skills/setup/SKILL.md, step 1): the PowerShell
    fallback (which detects the zero-length WindowsApps Microsoft-Store-stub alias before anything
    executes it) previously triggered only when no alternate interpreter existed. A real-but-incompatible
    interpreter (e.g. Python 3.6, which the same section documents as unable to run the guard's own
    from __future__ import annotations source) would silently emit no JSON verdict — but per the
    unchanged rule two paragraphs later ("Only verdict ok passes; fail closed on everything else"),
    that non-verdict already failed closed rather than open. The fix (triggering the fallback on "no
    such interpreter exists or the one you chose emits no JSON verdict") turns an unremediable
    fail-closed into an actionable one — it does not change the fail-closed default. No new trust is
    extended to an unproven interpreter.

  • plugin.json / CHANGELOG.md: version bump and documentation only, no logic.

No injection surface, credential handling, permission-widening, or supply-chain change is present in
this diff.

@claude

claude Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 4m 50s —— View job


Review complete

This is a prose-only fix to three stranded review findings against disk-hygiene's clean and setup
skills, plus the changelog/version bump. No script or hook logic changed (confirmed: only
.claude-plugin/plugin.json, CHANGELOG.md, and the two SKILL.md files are touched; python3_alias_probe.py
itself is unmodified). Per this repo's CLAUDE.md, this is a skill prose-body edit, not a frontmatter/contract
change, so the fresh-docs mandate doesn't gate it — the author's permissions-doc citations are a nice-to-have
here, not an obligation.

Verified independently:

  • Read python3_alias_probe.py directly: it does carry from __future__ import annotations (line 31) and
    PEP 604 unions throughout, so the PR's central claim — that Python 3.6 dies with a SyntaxError before
    emitting any JSON — checks out against the actual script, not just the PR description's assertion.
  • The confirmation-gate table's new per-row "what must be named" column is internally consistent with the
    cross-references in §1 (scan-scope row) and §6 (removal row) that this PR updated to point at it.

Not independently verified (sandboxed environment blocked script execution/approval for
check-changelog-parity.sh, check-changed-skills.sh, markdownlint-cli2, and the test suites; and outbound
gh pr view on other PRs was also blocked) — the PR body's test/lint/parity results and the "0.14.0 claimed by
#1870" version-collision claim are author-claimed and unverified by me.

Finding (Suggestion — consistency gap in the refactor itself):

The PR's whole point is that a call site should point at the confirmation gate's per-question row rather than
re-stating the "what must be named" bar inline (that's exactly what §1 and §6 were changed to do here). §5
was left out of that treatment — it still inlines the bar text instead of referencing the row:

When status is `ready-for-explicit-approval`, show a table naming every path, the single tier, logical
bytes, and the preview's approval token, then pass the [confirmation gate](#confirmation-gate) — the
approval must name **exactly that tier and list**. Process another tier only with a new plan, preview,
and question.

then pass the [confirmation gate](#confirmation-gate) — the
approval must name **exactly that tier and list**. Process another tier only with a new plan, preview,
and question.

Compare to the new §6 wording a few lines later (the [confirmation gate](#confirmation-gate)'s removal row).
§5 is the section that most directly owns that table row (Removal approval (§5) and manual handoff (§6)),
so leaving its own cross-reference un-migrated is the one loose end in an otherwise thorough sweep — worth a
one-line touch-up (e.g. pass the confirmation gate's removal row) so the three call sites are uniform. Not a
functional bug: the restated text is still accurate, just duplicated instead of centralized, which is the
class of drift this very PR exists to eliminate elsewhere.

No other correctness, design, or maintainability issues found in the diff. The markdown table renders
correctly, all #confirmation-gate anchor links resolve, and the setup/SKILL.md fallback-trigger wording
("no such interpreter exists or ... emits no JSON verdict") is precise — it doesn't conflate "no verdict"
with "non-ok verdict," so it won't accidentally reroute a legitimate indeterminate/not-found result.

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

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

…very site

The handoff gate's §6 row requires an answer naming exactly one tier, but the
manual-handoff step asked only for an exact path list against a report that is
multi-tier by construction. States the single-tier presentation there, so the
rule holds at every question it governs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

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


Security review — PR #2016 (re-run at ea12ae6)

  • Fetch PR diff (gh pr diff 2016) and confirm scope against HEAD ea12ae6
  • Diff this commit against the last-reviewed commit (4739892) to isolate what's new
  • Re-review clean/SKILL.md confirmation gate for security-relevant wording changes
  • Re-review setup/SKILL.md python3-probe fallback (unchanged since last review — confirmed)
  • Check plugin.json / CHANGELOG.md for anything beyond version/prose
  • Post findings

No security issues found.

Scope of this re-run: this branch was already given a full security review at 4739892 (found no
issues). Commit ea12ae6 is the only thing new since then, and it touches exactly one file —
plugins/disk-hygiene/skills/clean/SKILL.md, 9 insertions / 6 deletions, all inside the confirmation-gate
table and the §6 unsupported-platform-handoff paragraph. setup/SKILL.md, CHANGELOG.md, and
plugin.json are byte-identical to what was already reviewed.

What changed and why it doesn't weaken anything:

  • Confirmation-gate table (clean/SKILL.md
    L92-100):
    wording-only refinement of which row requires the answer to name something the skill itself
    produced. The removal/manual-handoff row still requires "exactly the one tier and the exact path
    list just shown" — unchanged bar.
  • §6 unsupported-platform handoff
    (L265-269):
    this is the one substantive change, and it's a tightening, not a loosening. The prior wording
    said the human "approves an exact path list" against a §3 report that spans every tier; the new
    wording closes that ambiguity by requiring the path list be "drawn from one tier" and explicitly
    instructing the report be narrowed to a single tier and shown before asking. That removes a
    plausible path to a mixed-tier approval on the platforms (Windows/macOS) where no engine-side
    --confirm-tier argument backstops the approval — i.e., it closes a gap in the one lane that relies
    entirely on the prose gate rather than engine validation.
  • Everything downstream of the gate in the manual-handoff lane — per-path handoff-verify
    revalidation, the "verify one path per deletion, not one batch" staleness rule, the ban on
    container-wide deletion spellings (Clear-RecycleBin/empty-Trash), and the kill-switch/guard-hook
    description — is byte-identical to what was already reviewed and confirmed sound.

No injection surface, credential handling, permission-widening, or supply-chain change is present in
this diff. Confirms the conclusion of the prior security-review pass on this PR.

@claude

claude Bot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

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


Review complete

  • Gather context (diff, changed files, CLAUDE.md)
  • Review plugin.json version bump
  • Review CHANGELOG.md
  • Review skills/clean/SKILL.md confirmation-gate rewrite
  • Review skills/setup/SKILL.md python3-probe fallback rewrite
  • Post findings

Scope: prose/manifest only (plugin.json version bump, CHANGELOG.md, and the two SKILL.md files). No script/hook logic changed — destructive_guard.py and test_hygiene.py are not in this diff (the earlier commit that touched them was reverted, per the PR description). Security-relevant assessment is out of scope for this lane per REVIEW.md's split; correctness/maintainability only below.

Independently verified (not just author-claimed):

  • python3_alias_probe.py:31 does carry from __future__ import annotations, so the central claim — a pre-3.7 interpreter dies with SyntaxError before emitting JSON — checks out against the actual script.
  • destructive_guard.py:1327 — the module tail is os._exit(_exit_code) — confirms the PR's scope claim that the _discard_stream finding is already discharged on main independent of this branch.
  • The new confirmation-gate table (clean/SKILL.md lines 96–100) is internally consistent with its two updated cross-references: §1's scan-scope row (line 131) and §6's removal row (line 268), both of which now show the required tier/path list before asking, matching the table's "just shown" requirement.

Not independently verified (gh pr view on other PRs was blocked in this sandbox): the 0.14.0 claimed-by-#1870 version-collision note, and the test/lint/parity results in the PR body.

Finding — carried over from an earlier review pass on this same PR, still unaddressed (Suggestion, not a bug):

The PR's stated goal is that a call site should point at the confirmation gate's per-question row rather than restate the "what must be named" bar inline — that's exactly what §1 (line 131) and §6 (line 268) were migrated to do here, including in the latest commit (ea12ae6) which explicitly set out to "satisfy the ask-shows-what-the-row-names rule at every site." §5 was left out of that migration; it still inlines the bar text:

When status is `ready-for-explicit-approval`, show a table naming every path, the single tier, logical
bytes, and the preview's approval token, then pass the [confirmation gate](#confirmation-gate) — the
approval must name **exactly that tier and list**. Process another tier only with a new plan, preview,
and question.

then pass the [confirmation gate](#confirmation-gate) — the
approval must name **exactly that tier and list**. Process another tier only with a new plan, preview,
and question.

This isn't a functional gap — §5's plan is single-tier by construction (§4: "never mix tiers"), and it already shows the table before asking, so it satisfies the rule in substance. It's the one remaining textual inconsistency across the three call sites the table's intro paragraph describes (Removal approval (§5) and manual handoff (§6)), and it's the section that most directly owns that row. A one-line touch-up (e.g. pass the confirmation gate's removal row) would make all three call sites uniform, closing out the consistency sweep this PR is otherwise thorough about. This line isn't part of this PR's diff, so it can't carry an inline comment — noting it here per the "no changed line to anchor to" case.

No other correctness, design, or maintainability issues found. The table renders correctly, all #confirmation-gate anchors resolve, and the setup/SKILL.md fallback-trigger wording ("no such interpreter exists or ... emits no JSON verdict") is precise — a legacy Python 2.x also hits a SyntaxError on the same from __future__ import annotations line, so it's correctly covered by "emits no JSON verdict" without needing its own clause.
·

@kyle-sexton
kyle-sexton merged commit 417ac6d into main Aug 8, 2026
33 checks passed
@kyle-sexton
kyle-sexton deleted the fix/disk-hygiene-stranded-review-threads branch August 8, 2026 21:57
kyle-sexton added a commit that referenced this pull request Aug 12, 2026
…2255)

## Summary

Mirror the #2016 `disk-hygiene` confirmation-gate prose fix in
`repo-hygiene`: the inline-question fallback must trigger when
`AskUserQuestion` is absent, denied, or otherwise unusable — not only
when it is absent.

## Fix

- Update `plugins/repo-hygiene/skills/clean/SKILL.md` confirmation gate
question-surface rule to match the corrected `dontAsk` / deny /
disallowed-tools wording.
- Bump plugin to 0.10.1 with CHANGELOG entry.

## Verification

Prose-only change; no shell/python suites map to this file. Reviewed
against the live permissions docs claim cited in #2088 and the merged
`disk-hygiene` sibling wording.

## Related

Fixes #2088 (completes the three-item sweep alongside #2252 and #2254).

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant