fix(disk-hygiene): flag the parent-folder send-to-Recycle-Bin spelling - #2860
Conversation
The Shell.Application rule shipped for issue 2595 required the literal Recycle
Bin folder id (NameSpace(10) / NameSpace(0xa)) AND a MoveHere/InvokeVerb
action, so the ordinary way to send a named item to the bin --
$sh.NameSpace('<parent folder>').ParseName('victim').InvokeVerb('delete'),
which addresses the item through its containing folder and never mentions the
bin -- returned no verdict at all. The belt raised no prompt, and because the
gate keyed on a literal token rather than on the deletion, the same command
could be made to prompt by adding an inert NameSpace(10) call.
The widened rule keys that shape on the delete VERB instead of on the folder
id: an InvokeVerb / InvokeVerbEx call whose literal verb argument names the
delete verb now returns "ask" (and "deny" in audit-only mode), whatever
namespace the FolderItem was obtained from -- consistent with every other
spelling in this set, which are all context-free token matches. InvokeVerbEx
is covered by the same token because a word boundary closed immediately after
InvokeVerb never matched the suffixed spelling.
What the rule deliberately still does NOT catch, each still deferring:
- MoveHere into an ordinary (non-bin) folder: that is a MOVE, not a deletion.
Its fixture still asserts assertIsNone, and now carries CopyHere beside it.
- Non-delete verbs (InvokeVerb('open')) and the omitted default verb
("typically open" per the FolderItem.InvokeVerb reference).
- An opaque verb argument (InvokeVerb($verb)) -- the verb is unknowable from
the command text. Widening to it, like widening _POWERSHELL_MUTATION_WORDS,
is its own change.
The delete-verb set is enumerated, not identity-checked, and the pattern set
now says so: the verb argument "must be one of the values returned by the
item's FolderItemVerb.Name property" (Microsoft's FolderItem.InvokeVerb
reference, fetched 2026-08-16), i.e. a name out of the item's own verb
collection, not a stable canonical token -- so a verb named anything else (a
localized name) is not covered and completeness is not implied.
The test note claiming Move-Item is the catch-all for these COM spellings is
corrected: _POWERSHELL_MUTATION_WORDS matches neither MoveHere (the "move"
entry's trailing boundary rejects the "here") nor InvokeVerb, verified by
execution rather than by reading.
Closes #2850
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 52s —— View job Security review complete ✅
Scope: No security issues found. The change is a purely additive heuristic fix to a PowerShell-command classifier used by disk-hygiene's guard hook: it adds a second, independent detection path (
No |
|
Last security-reviewed head: |
|
Claude finished @kyle-sexton's task in 2m 14s —— View job Claude's PR reviewTodo list
SummaryI reviewed the diff (
No correctness, security, or maintainability issues found — I have no inline comments to add. One very minor, non-blocking observation: the audit-only-mode denial test ( Note on verification: the Bash tool in this environment blocked |
|
Claude has reviewed this PR 3 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…erdict
Fresh-context review of the widened Recycle Bin rule raised three points, all
about the rule describing itself accurately rather than about what it matches:
- The verb-keyed match is context-free by design, so a command that never
mentions Shell.Application (a FolderItem obtained some other way, or any
other COM object exposing InvokeVerb) got a reason naming a library absent
from the command. The reason now says "a COM shell delete verb".
- _shell_verb_name_deletes documented the accelerator strip but not the
whitespace strip, which is what resolves a padded literal (' delete ') to
the enumerated verb.
- The CopyHere non-catch is qualified: only CopyHere into an ORDINARY folder
is stated, since a CopyHere into the bin namespace is untested here and its
behavior is not established from a primary source.
The leading `(?<![\w])` is left as-is and the reason recorded: a hyphen-
prefixed relative (My-InvokeVerb('delete'), a wrapper) still matches on
purpose. Tightening it to the cmdlet list's `(?<![\w./\-])` would buy
precision by making a deleting wrapper go silent, which is the failure this
rule exists to close.
No verdict changes: the probe set and the plugin's PowerShell-lane tests
return the same decisions as before this commit.
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 51s —— View job Security review complete ✅
Skip gate: no security-relevant surface in the new commits. The only commits since the last successful security review (
No No security issues found. |
|
Claude finished @kyle-sexton's task in 3m 1s —— View job Code review complete
Scope: FindingsOne non-blocking doc nit posted inline: VerificationHand-traced both regexes (
Also confirmed:
No correctness, security, or maintainability blockers found. This is a narrowly-scoped, well-documented widening of an existing detection rule with no regressions to the bin-id branch it builds on. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…cation Review found the docstring still promised a Shell.Application Recycle Bin spelling while the second branch is deliberately context-free: it matches a literal delete verb passed to InvokeVerb on any COM object, with no Shell.Application or NameSpace call required. A reader with only the docstring — not the commit history — could reasonably infer a context that is not required. Behavior is unchanged; the reason string already said "COM shell delete verb" rather than naming Shell.Application, for this same reason. 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 20s —— View job Security review complete ✅
Note: the No security issues found. Scope of the delta since the last successful security review ( For completeness, re-verified the full PR diff from a security lens:
No |
|
Claude finished @kyle-sexton's task in 3m 17s —— View job Claude's PR reviewTodo list
SummaryPer I independently hand-traced (this sandbox blocks No correctness, security, or maintainability issues found. No inline comments to add — the diff is clean and the one substantive finding raised on the prior head has already been fixed. Other checks:
Note on verification: as in prior passes on this PR, this environment does not permit executing |
1 participant
Summary
The
Shell.ApplicationRecycle Bin rule shipped for issue 2595 required the literal bin folder id(
NameSpace(10)/NameSpace(0xa)) and aMoveHere/InvokeVerbaction. The ordinary way tosend a named item to the bin addresses the item through its parent folder and never mentions
the bin at all:
That spelling returned no verdict, so the belt raised no prompt — and because the gate keyed on the
presence of a literal token rather than on the deletion, the same command could be made to prompt by
adding an inert
NameSpace(10).Items().Countcall.This keys that shape on the delete verb instead of on the folder id. The bin-id branch is still
evaluated first and keeps its verdict text, so commands satisfying both are unaffected; its action
pattern gains only the
InvokeVerbExsuffix described below.What it now catches
InvokeVerb/InvokeVerbExwhose literal verb argument names the delete verb, in single ordouble quotes, with or without a menu accelerator (
'&Delete','De&lete'), from any namespace —the match is context-free, like every other spelling in this pattern set. Requiring a
Shell.Applicationcontext would be a second, trivially-defeated way of doing the same test.InvokeVerbExspecifically: a word boundary closed immediately afterInvokeVerbnever matchedthe suffixed spelling, which is the stem-vs-suffixed-relative gap this repo has been bitten by
before.
InvokeVerbExis "similar to InvokeVerb, but it allows you to specify arguments to thecommand as well as the command itself"
(ShellFolderItem.InvokeVerbEx,
fetched 2026-08-16).
What it deliberately still does NOT catch
Each of these still defers, and each is now locked by a fixture:
MoveHereinto an ordinary (non-bin) folder — a MOVE, not a deletion. The existingassertIsNonefixture is untouched.CopyHereinto an ordinary folder — copies; the original is untouched.CopyHereinto thebin namespace also defers, exactly as it did before this PR; it is left alone rather than widened
because no primary source establishes what it does there.
InvokeVerb('open')) and the omitted verb (the default, "typically open").InvokeVerb($verb)) — unknowable from the command text. Widening toit is its own change, as is widening
_POWERSHELL_MUTATION_WORDS.The delete-verb set is enumerated, not identity-checked, and the pattern set now says so with
its source: the verb argument "must be one of the values returned by the item's
FolderItemVerb.Name property. If no verb is specified, the default verb will be invoked"
(FolderItem.InvokeVerb,
fetched 2026-08-16), and
FolderItemVerb.Namemerely "Contains the verb's name"(reference, fetched
2026-08-16). There is no canonical-token identity test available for a COM shell verb, so a verb
named anything else (a localized name) is not covered and completeness is not implied.
The test note claiming
Move-Itemis the catch-all for these COM spellings is corrected:_POWERSHELL_MUTATION_WORDSmatches neitherMoveHere(themoveentry's trailing(?![\w-])rejects the
here) norInvokeVerb— verified by execution, printed below, not by reading.Verified by execution
powershell_decision(command, enabled=True)driven directly against the pre-fix module (theorigin/mainblob042b7051, copied out and hash-verified) and against the patched module:$sh.NameSpace('C:\some\parent').ParseName('victim').InvokeVerb('delete')(send form)ask(New-Object -ComObject Shell.Application).NameSpace(10).MoveHere($path)askask$shell = New-Object -ComObject Shell.Application; $shell.NameSpace(10).MoveHere($path)askask$shell.NameSpace(0xa).ParseName($path).InvokeVerb('delete')askask(New-Object -ComObject Shell.Application).NameSpace('C:\tmp').MoveHere($path)(move)...ParseName('victim').InvokeVerbEx('delete')ask...ParseName('victim').InvokeVerb('&Delete')ask...ParseName('victim').InvokeVerb('open')...ParseName('victim').InvokeVerb()NameSpace('C:\tmp').CopyHere($path)NameSpace(10).Items()...ParseName('victim').Verbs()_POWERSHELL_MUTATION_WORDS.search(...)returnsNonefor both the folder-pathMoveHereand theInvokeVerb('delete')command, before and after.The three new fixture sets were run against the pre-fix guard module and fail there
(
test_powershell_deletion_spellings_force_final_prompt,test_powershell_deletion_spellings_denied_in_audit_only_mode, and the newtest_powershell_shell_app_send_to_bin_via_parent_folder_prompts); the deferral test passes bothbefore and after, so its added cases lock existing behavior rather than change it.
Deliberate over-coverage
A fresh-context reviewer flagged that the leading
(?<![\w])lets a hyphen-prefixed relativethrough:
My-InvokeVerb('delete')(a wrapper function) returnsask. That is kept on purpose andthe reason is now recorded in the pattern comment — tightening it to the cmdlet list's
(?<![\w./\\-])would buy precision by making a deleting wrapper go silent, which is the exactfailure this rule exists to close. Prompting once on a wrapper is the cheaper error. Text that
merely mentions the spelling (
Select-String "InvokeVerb('delete')") likewise prompts, which is thesame behavior every other spelling in this file already has for a mention of
Remove-Item.Gates run locally
check-changelog-parity.sh --check,--check-order,--check-bump origin/main,--check-preserved origin/main,validate-plugins.sh,markdownlint-cli2on the CHANGELOG,run-ruff.sh check/format --diffon both touched Python files (no new findings; the onlypre-existing ones are untouched regions), and the plugin's Python suites. Six failures in the local
Windows run (
test_stash_must_exist_in_an_independent_checkout,test_preview_allows_root_children_os_managed_snapshot, two intest_guard_launch_monitor, two intest_hook_telemetry) reproduce identically on a pristineorigin/mainworktree and are unrelatedto this change.
Related
This PR widens the rule that shipped there; it does not revert any of it.
onto
origin/mainat2d20a277and every cited line was re-derived from that content.