fix(disk-hygiene): gate PowerShell move/rename/overwrite spellings

[kyle-sexton]

Fixes #387

Extends the PowerShell lane mutation-spelling set with move, rename, overwrite, and truncation spellings.

Test plan

• python3 -m pytest plugins/disk-hygiene/skills/clean/scripts/test_hygiene.py -k powershell_deletion_spellings_force

Related

N/A

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[claude]

Claude finished @kyle-sexton's task in 2m 59s —— View job

---

Security review of PR #2470 (403b64d)

• Gather context (PR diff, changed files)
• Review destructive_guard.py changes for logic/regex flaws
• Review test_hygiene.py, plugin.json, CHANGELOG.md
• Post findings

Scope: only the 4 files changed in this PR. destructive_guard.py is a PreToolUse hook that gates the PowerShell lane by flagging known filesystem-mutation "spellings" and turning them into an ask/deny verdict (per its own docstring, this lane is a blocklist over a permissive default, not a fail-closed allowlist — unlike the Bash lane). Findings below are about gaps in the new mutation-detection logic added by this PR, which is exactly the security property this PR is trying to strengthen.

---

IMPORTANT — New-Item -Force truncation check can be bypassed with a multi-line command
destructive_guard.py#L942-L944

_POWERSHELL_NEW_ITEM_FORCE = re.compile(
    r"(?i)(?<![\w./\\-])new-item(?![\w-]).*-force\b"
)

Python's . does not match \n unless re.DOTALL is set, and this pattern is compiled with only the inline (?i) flag. command is passed through to powershell_decision verbatim (see _decide, L1210-L1214) with no newline normalization, and PowerShell itself supports splitting a single logical command across lines (backtick line-continuation, or a multi-statement -Command string). So:

New-Item C:\file.txt -ItemType File `
  -Force

is one valid PowerShell invocation that truncates an existing file, but .*-force\b cannot bridge the embedded \n between File and -Force, so _POWERSHELL_NEW_ITEM_FORCE.search() returns no match and the command silently defers (no ask, no deny) instead of hitting the guard this PR just added.
Confidence: Medium — the regex behavior is certain; real-world exploitability depends on the hook actually receiving multi-line command strings for PowerShell tool calls, which is a normal and expected shape for generated PowerShell scripts.

---

SUGGESTION — new mutation-word list omits standard aliases for the exact verbs this PR targets
destructive_guard.py#L933-L940

The new alternation covers move-item|rename-item|mv|move|ren|rename and set-content|out-file|add-content, but PowerShell ships two more built-in aliases for the same cmdlets that aren't listed: mi (Move-Item) and rni (Rename-Item). Given the existing list already includes single/two-letter aliases for other verbs (rm, rd, ri), the omission of mi/rni looks like an oversight rather than a deliberate exclusion.

Separately, Copy-Item -Force (aliases copy, cp, cpi) is a classic overwrite spelling — arguably the most common one — and isn't gated anywhere in this file (confirmed via grep -i copy-item across the changed files: no hits). The PR title and description explicitly scope this change to "move/rename/overwrite spellings," so this looks like a gap against the PR's own stated goal rather than an intentional scope boundary.

Because this lane is a blocklist by design (not fail-closed), these are coverage gaps rather than a broken security boundary, but they mean a generated mi C:\a C:\b, rni old new, or Copy-Item -Force src dst still silently defers with no ask prompt.
Confidence: High (verified by reading the regex and grepping the changed files for coverage).

---

No other issues found. The new _POWERSHELL_OUTPUT_REDIRECT regex ((?<![<>])>(?![=>]), L945) correctly excludes >> (append) and >=/n>&1-style comparisons aren't a concern here since PowerShell has no bare >/< comparison operators; it over-matches on things like 2>&1 (stderr redirection, not a file write) but that's a false-positive/UX issue, not a security gap, so it's out of scope for this review. No injection, credential, or workflow-permission issues in this diff (plugin.json is a version bump, CHANGELOG.md is prose).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 403b64dbfb

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 2m 22s —— View job

---

Security review of PR #2470 (62fbaa9)

• Gather context (PR diff via gh pr diff 2470, changed files)
• Review destructive_guard.py changes for logic/regex flaws
• Review test_hygiene.py, plugin.json, CHANGELOG.md
• Post findings

---

Scope: only the 4 files changed in this PR. destructive_guard.py is a PreToolUse hook for the PowerShell lane — per its own docstring this lane is a blocklist over a permissive default (not fail-closed like the Bash lane), so a bypass here means a flagged command silently defers with no ask/deny verdict, rather than a hard security-boundary break. The findings below are gaps in the new mutation-detection logic this PR adds, and they're still present at the current head (62fbaa9) — the diff content is unchanged from the earlier reviewed commit (403b64d), just re-SHA'd by a rebase.

---

IMPORTANT — module-qualified invocations of the newly added cmdlets still bypass the guard
destructive_guard.py#L953-L958

_POWERSHELL_QUALIFIED_DELETE only covers remove-item|clear-content|clear-recyclebin:

_POWERSHELL_QUALIFIED_DELETE = re.compile(
    r"(?i)[a-z][\w.]*\\(remove-item|clear-content|clear-recyclebin)(?![\w-])"
)

_POWERSHELL_MUTATION_WORDS (the word-boundary matcher this PR extends, L933-L940) has a lookbehind (?<![\w./\\-]) that rejects a preceding backslash, so a module-qualified call to any of the new verbs — e.g. Microsoft.PowerShell.Management\Move-Item, ...\Set-Content, ...\Out-File, ...\Rename-Item, or a qualified New-Item -Force — matches neither _POWERSHELL_MUTATION_WORDS nor _POWERSHELL_QUALIFIED_DELETE, and powershell_decision() (L961-L1023) returns None — no ask, no deny, the operation defers silently.
Confidence: High (verified by reading the regex; the same gap was already flagged on the prior commit and is unresolved at this head).

---

IMPORTANT — New-Item -Force truncation check can be bypassed with a multi-line command
destructive_guard.py#L942-L944

_POWERSHELL_NEW_ITEM_FORCE = re.compile(
    r"(?i)(?<![\w./\\-])new-item(?![\w-]).*-force\b"
)

Python's . doesn't match \n without re.DOTALL, and this pattern is compiled with only the inline (?i) flag. command reaches powershell_decision verbatim with no newline normalization (_decide, L1210-L1214), and PowerShell supports splitting a logical statement across lines via the backtick line-continuation. So:

New-Item C:\file.txt -ItemType File `
  -Force

is one valid invocation that truncates an existing file, but .*-force\b can't bridge the embedded \n, so the regex misses it and the command defers with no verdict.
Confidence: Medium — the regex behavior is certain; exploitability depends on the hook receiving multi-line command strings for PowerShell calls, which is a normal shape for generated scripts.

---

SUGGESTION — new mutation-word list omits standard aliases and leaves Copy-Item -Force (an overwrite spelling) completely ungated
destructive_guard.py#L933-L940

The new alternation (move-item|rename-item|mv|move|ren|rename, set-content|out-file|add-content) omits PowerShell's built-in mi (Move-Item) and rni (Rename-Item) aliases, even though the existing list already carries single/two-letter aliases for other verbs (rm, rd, ri). Separately, Copy-Item -Force (aliases copy, cp, cpi) — arguably the most common overwrite spelling — isn't matched anywhere in this file. The PR is explicitly scoped to "move/rename/overwrite spellings," so a generated mi C:\a C:\b, rni old new, or Copy-Item -Force src dst still silently defers with no ask prompt, which looks like a gap against the PR's own stated goal.
Confidence: High (verified by reading the regex and grepping the changed files — no copy-item/cpi/mi/rni coverage anywhere).

---

No other security issues found in this diff. _POWERSHELL_OUTPUT_REDIRECT (L945) over-matches on things like 2>&1, but that only makes the guard more conservative (extra ask prompts), not a security gap. plugin.json is a version bump and CHANGELOG.md is prose — no injection, credential, or workflow-permission issues there. (Doc drift between the changelog and safety-model.md/SKILL.md was already flagged by another reviewer — that's a documentation-accuracy issue, not a vulnerability, so it's out of scope for this security pass.)

[claude]

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

---

Security review of PR #2470 (d3de451)

• Gather context (PR diff via git diff origin/main...HEAD, changed files)
• Review destructive_guard.py changes for logic/regex flaws
• Review test_hygiene.py, plugin.json, CHANGELOG.md
• Post findings

Scope: only the 4 files changed in this PR. destructive_guard.py is a PreToolUse hook for the PowerShell lane — per its own docstring this lane is a blocklist over a permissive default (not fail-closed like the Bash lane), so a bypass here means a flagged command silently defers with no ask/deny verdict, rather than a hard security-boundary break.

The diff at the current head (d3de451) is unchanged from the previously reviewed commits (403b64d, 62fbaa9) — verified by diffing origin/main...HEAD and reading the live file. Both IMPORTANT findings from the earlier passes on this PR are still present and unaddressed:

---

IMPORTANT — module-qualified invocations of the newly added cmdlets still bypass the guard
destructive_guard.py#L953-L958

_POWERSHELL_QUALIFIED_DELETE = re.compile(
    r"(?i)[a-z][\w.]*\\(remove-item|clear-content|clear-recyclebin)(?![\w-])"
)

_POWERSHELL_MUTATION_WORDS (L933-L940) has a lookbehind (?<![\w./\\-]) that rejects a preceding backslash, so a module-qualified call to any of this PR's new verbs — e.g. Microsoft.PowerShell.Management\Move-Item, ...\Set-Content, ...\Out-File, ...\Rename-Item, or a qualified New-Item -Force — matches neither _POWERSHELL_MUTATION_WORDS nor _POWERSHELL_QUALIFIED_DELETE (which still only lists remove-item|clear-content|clear-recyclebin). powershell_decision() (L961-L1023) returns None — no ask, no deny, the operation defers silently.
Confidence: High (verified by reading the regex; also independently flagged by the Codex P1 review comment on this PR).

---

IMPORTANT — New-Item -Force truncation check can be bypassed with a multi-line command
destructive_guard.py#L942-L944

_POWERSHELL_NEW_ITEM_FORCE = re.compile(
    r"(?i)(?<![\w./\\-])new-item(?![\w-]).*-force\b"
)

Python's . doesn't match \n without re.DOTALL (confirmed: no re.DOTALL/re.S anywhere in this file), and command reaches powershell_decision verbatim with no newline normalization (_decide, L1211-L1213). PowerShell supports splitting a logical statement across lines via backtick line-continuation, so:

New-Item C:\file.txt -ItemType File `
  -Force

is one valid invocation that truncates an existing file, but .*-force\b can't bridge the embedded \n, so the regex misses it and the command defers with no verdict.
Confidence: Medium — the regex behavior is certain; exploitability depends on the hook receiving multi-line command strings for PowerShell calls, which is a normal shape for generated scripts.

---

SUGGESTION — new mutation-word list still omits standard aliases and leaves Copy-Item -Force (an overwrite spelling) ungated
destructive_guard.py#L933-L940

The new alternation (move-item|rename-item|mv|move|ren|rename, set-content|out-file|add-content) omits PowerShell's built-in mi (Move-Item) and rni (Rename-Item) aliases, even though the existing list already carries single/two-letter aliases for other verbs (rm, rd, ri). Separately, Copy-Item -Force (aliases copy, cp, cpi) — arguably the most common overwrite spelling — still isn't matched anywhere in this file (confirmed via grep: no hits for copy-item/cpi/mi/rni). The PR is explicitly scoped to "move/rename/overwrite spellings," so a generated mi C:\a C:\b, rni old new, or Copy-Item -Force src dst still silently defers with no ask prompt.
Confidence: High (verified by reading the regex and grepping the changed files).

---

No other security issues found in this diff. _POWERSHELL_OUTPUT_REDIRECT (L945) over-matches on things like 2>&1, but that only makes the guard more conservative (extra ask prompts), not a security gap. plugin.json is a version bump and CHANGELOG.md is prose — no injection, credential, or workflow-permission issues there. The changelog/safety-model.md/SKILL.md doc-drift already flagged by Codex is a documentation-accuracy issue, not a vulnerability, so it stays out of scope for this security pass.

Summary: same 2 IMPORTANT + 1 SUGGESTION findings as the prior two review passes on this PR — none have been addressed at the current head. The core gap is that _POWERSHELL_QUALIFIED_DELETE (the module-qualified-name matcher) was never extended to cover the cmdlets this PR adds.
· Branch

[claude]

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

---

Code review of PR #2470 (d3de451)

• Gather context (PR diff, changed files, CLAUDE.md/REVIEW.md)
• Review destructive_guard.py changes (correctness, maintainability)
• Review test_hygiene.py test coverage
• Review plugin.json, CHANGELOG.md
• Post findings

Scope: correctness, design, tests, and documentation for the 4 changed files, per this repo's REVIEW.md lane split — security-relevant regex bypasses (module-qualified calls, multi-line -Force, Copy-Item, mi/rni aliases) are the security lane's territory and are already covered in detail in the earlier security-lane comments on this PR; not repeated here.

Two inline findings posted on test_hygiene.py (both 🔴 Important, test-coverage gaps):

• Several of the regex alternatives this PR adds to _POWERSHELL_MUTATION_WORDS (mv/move/ren/rename, Add-Content, Format-Volume, Clear-Disk, Initialize-Disk) have zero test coverage anywhere in the suite — only Move-Item, Rename-Item, Set-Content, Out-File, New-Item -Force, and bare > are exercised.
• The kill-switch audit-only-mode counterpart test (test_powershell_deletion_spellings_denied_in_audit_only_mode, test_hygiene.py#L4457) wasn't extended alongside the ask-path test, so the new spellings' deny behavior under disk_hygiene_enabled=false is unverified.

---

🟡 Suggestion — reference docs still describe the pre-PR (unflagged) behavior, contradicting this release

This finding spans files this PR didn't touch, so it can't be anchored to a changed line:

• skills/clean/reference/safety-model.md#L181-L191 still states Move-Item/mv, Rename-Item, Set-Content/Out-File/>, New-Item -Force, and Format-Volume/Clear-Disk "reach the tool with no guard verdict at all, in audit-only mode included," and still carries TODO(#387): extend the flagged set to those spellings. — this PR is exactly that extension, closing disk-hygiene: PowerShell guard misses move/rename/overwrite/format spellings #387, but the TODO and the surrounding claim were left in place.
• skills/clean/SKILL.md#L385-L389 likewise still says "move, rename, overwrite, and volume-format spellings are not flagged at all."

CHANGELOG.md's new [0.17.3] entry (CHANGELOG.md#L6-L12) correctly describes the new behavior, so a reader who checks the changelog and then the safety model / skill doc gets directly contradictory answers about what the PowerShell lane guards. This was also flagged by the Codex review on this PR; independently confirmed here by reading both docs against the current destructive_guard.py behavior. (Codex's separate P1 finding about module-qualified invocations bypassing the new matcher is a security-lane concern, already covered there.)

---

No other correctness, design, or maintainability issues found in this diff. plugin.json's version bump (0.17.2 → 0.17.3) matches the changelog entry, and the _powershell_mutation_verdict message rename from "deletion spelling" to "mutation spelling" (destructive_guard.py#L993) is an accurate, low-risk terminology update now that the word list covers more than deletion.

[claude]

🔴 Important — several of the newly added regex alternatives have no test coverage at all

_POWERSHELL_MUTATION_WORDS (destructive_guard.py#L937-L939) adds mv|move|ren|rename (aliases for Move-Item/Rename-Item), add-content, and format-volume|clear-disk|initialize-disk — but this test only exercises Move-Item, Rename-Item, Set-Content, Out-File, New-Item -Force, and bare >. A grep of the whole test file turns up zero hits for Add-Content, Format-Volume, Clear-Disk, or Initialize-Disk in any PowerShell test, and no test exercises the mv/move/ren/rename aliases on the PowerShell lane (the one mv hit at line 2918 is an unrelated Bash-lane test). Since this is a blocklist-by-design lane, an untested alternative that's subtly wrong (typo, wrong precedence, word-boundary miss) would silently defer instead of prompting, and nothing in the suite would catch it.

Suggest adding one case per newly-added alternative (mv, move, ren, rename, Add-Content, Format-Volume, Clear-Disk, Initialize-Disk) to this loop.

[claude]

🔴 Important — audit-only (kill-switch) deny path isn't re-verified for any of the new spellings

test_powershell_deletion_spellings_denied_in_audit_only_mode (test_hygiene.py#L4457-L4472) is the counterpart to this test — it asserts enabled=False turns the same spellings into deny instead of ask (kill-switch B2 behavior called out in this module's own docstring at destructive_guard.py:970-972). This PR extends the ask-path list here but leaves that deny-path list untouched, so none of Move-Item, Rename-Item, Set-Content, Out-File, New-Item -Force, or output redirection are verified to actually deny in audit-only mode — only that they prompt when the kill switch is on. _powershell_mutation_verdict routes both branches through shared code today, but that symmetry is exactly the kind of thing a future refactor could break unnoticed without a test on both sides.

[github-actions]

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