fix(disk-hygiene): accept braces inside quoted words in the engine gate

[kyle-sexton]

Closes #5641

Summary

The disk-hygiene engine gate refused any command containing { or }, so a quoted WSL distro path such as 'C:/Users/me/AppData/Local/wsl/{673ac4db-...}' could never form an exact engine call. The denial also named the brace as a substitution or expansion.

Fix

In destructive_guard.py, { and } are dropped from whole-word single or double quoted spans before the forbidden-character check (_without_quoted_braces), in both _literal_shell_words and _unparsable_reason. Unquoted braces and every other expansion or operator character, $ included, are still refused. Version 0.41.3 with a CHANGELOG entry.

Verification

• python3 -m unittest plugins/disk-hygiene/skills/clean/scripts/test_hygiene.py: 679 tests, OK (1 skipped), including new GUID-path accept and unquoted/$ reject cases.
• scripts/check-changelog-parity.sh --check, --check-order, --check-bump origin/main: pass.
• scripts/validate-plugins.sh: all manifests validated.

Related

Observed in #5228 probe; denial wording from #5519.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-01T01:26:54.176075Z	e5ad74d	Draft marked ready

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

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

---

Security review complete

• Gather context (PR diff, custom instructions)
• Invoke /review:security-review for PR fix(disk-hygiene): accept braces inside quoted words in the engine gate #5644 (HEAD e5ad74d)
• Post findings

Scope: plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py (the only security-relevant change) — _without_quoted_braces, and its use in _literal_shell_words and _unparsable_reason.

No security issues found.

This hook is a fail-closed PreToolUse gate for destructive disk-cleanup commands, so I treated it as touching the irreversible-action protected class in the instruction exception register and reviewed the narrowed blocklist accordingly rather than waving it through as a prose-only change:

• The new exception only neutralizes {/} inside a span the regex _WHOLE_WORD_QUOTED recognizes as a whole quoted word (quote at word start, no nested quote, same quote closing immediately before a space/end). I traced this against _literal_shell_words's own quote-parsing loop (L204-L236) and the two enforce identical boundary conditions, so there's no case where the regex strips a brace that the real word-parser would not also treat as fully inside one literal quoted word.
• Quoting a span really does make {/} inert for both target shells: Bash suppresses brace expansion inside single or double quotes, and PowerShell has no analogous brace-expansion feature to begin with. So the exception can't let a real expansion through while the guard believes it's looking at a literal.
• Every other forbidden character ($, backticks, operators, unquoted/boundary-crossing braces) is still rejected regardless of quoting — confirmed in the diff and in the added tests (test_literal_parser_accepts_braces_only_inside_whole_word_quotes, test_an_unquoted_or_expanding_brace_target_is_refused), which specifically exercise adjacent-quote ("a '{b}' x'y'"), cross-boundary ("a '{b}' {c}"), and inside-quote-$/backtick/${}/$() cases and expect rejection.
• No secrets, auth, or injection surface elsewhere in the diff (plugin.json version bump, CHANGELOG.md entry, and test additions are not security-relevant).

GitHub Actions hardening is out of scope for this lane (zizmor's), and this PR doesn't touch any workflow file regardless.
· branch

[claude]

Claude finished @kyle-sexton's task in 6m 54s —— View job

---

Code review complete

• Gather context (PR diff, changed files, CLAUDE.md/AGENTS.md conventions)
• Review destructive_guard.py changes for correctness
• Review test coverage in test_hygiene.py
• Review CHANGELOG/version bump
• Post findings

Summary

Traced _without_quoted_braces and its two call sites (_literal_shell_words, _unparsable_reason) against the manual quote-tokenizing loop later in _literal_shell_words to check the two stay in agreement on what counts as "whole-word quoted." They do: the regex's word-boundary lookbehind/lookahead ((?<![^ ])...(?= |$)) matches exactly the cases the tokenizer itself treats as a bare quoted word (adjacent quotes, a trailing non-space char after the close, or an unterminated quote all correctly fall through to the forbidden-char deny instead of being silently brace-stripped). $, backticks, parens, etc. are deliberately left unstripped even inside quotes, so '$x{b}'-style inputs are still refused — consistent with the PR's stated scope. The new tests in test_hygiene.py exercise these boundary cases directly and look correct on manual trace. (I could not execute python3 -m unittest in this sandbox to independently run the suite — the author's test-run numbers are author-claimed and unverified by me.)

One completeness gap found and adversarially verified against a second, independent reading of the code:

destructive_guard.py — the fix is incomplete for the /usr/bin/[ ... ] bracket-test belt

claude-code-plugins/plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py

Line 1676 in e5ad74d

if any(value in _SHELL_EXPANSION_OR_OPERATOR_CHARS for value in interior):

_absolute_bracket_test_words (used by is_exact_readonly_supporting_command, the read-only ls/stat/du/test/find inspection belt from #2591/#2618) scans its interior string for forbidden characters directly (any(value in _SHELL_EXPANSION_OR_OPERATOR_CHARS for value in interior)) and never routes it through the new _without_quoted_braces. Concretely:

/usr/bin/[ -d 'C:/Users/me/AppData/Local/wsl/{673ac4db-a2e3-459e-882c-1ec71b253aa2}' ]

is denied solely because of the quoted {} — the identical quoted-GUID shape this PR now accepts everywhere else in this file. Remove the braces from the same command and it's allowed (reaches _trusted_system_readonly_head and returns True). This is a real allow→deny flip caused by the brace, not a hypothetical.

Severity-wise this is narrow rather than blocking: the non-bracket spelling of the same trusted command, /usr/bin/test -d '...{guid}...' (and stat/ls/du on the same path), already goes through _literal_shell_words on the whole command and does pick up the fix, so the identical inspection is reachable today under a different spelling. Only the [ ... ] bracket spelling of test is left behind. (The bare, unqualified [ ... ] form at _parse_bracket_test_words, line 1448, has the same unrouted check, but it's dead code for this purpose — its caller denies bare-bracket form unconditionally regardless of what that function returns, so no behavior change there.)

Suggested fix: scan _without_quoted_braces(interior) in _absolute_bracket_test_words (and, for consistency even though it's currently inert, in _parse_bracket_test_words) instead of interior directly.

Everything else — the version bump, CHANGELOG entry, and engine_grammar-level tests confirming a quoted GUID --target classifies identically to an unquoted one — checked out.

[claude]

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

---

Code review complete

• Gather context (PR diff, changed files, CLAUDE.md/AGENTS.md conventions)
• Invoke /review:code-review for PR fix(disk-hygiene): accept braces inside quoted words in the engine gate #5644 (HEAD b09cd97)
• Post findings

Summary

Since the last review round (e5ad74d), the only new commit is b09cd97, which touches just CHANGELOG.md (blank-line fix) and one test's GUID string (C:/Users/x/... → D:/wsl/..., a machine-path lint fix). destructive_guard.py is byte-for-byte unchanged from the prior round, so I re-verified the completeness gap flagged by the earlier code-review pass rather than re-deriving it from scratch, and it is still present and still unaddressed.

destructive_guard.py — the fix doesn't cover the /usr/bin/[ ... ] bracket-test belt
L1676

_absolute_bracket_test_words (feeds is_exact_readonly_supporting_command, the read-only inspection belt) still scans interior directly against _SHELL_EXPANSION_OR_OPERATOR_CHARS and never routes through the new _without_quoted_braces:

if any(value in _SHELL_EXPANSION_OR_OPERATOR_CHARS for value in interior):
    return None

versus the fixed call sites at L197-200 and L2193. Concretely:

/usr/bin/[ -d 'D:/wsl/{673ac4db-a2e3-459e-882c-1ec71b253aa2}' ]

is still denied solely because of the quoted {} — the same quoted-GUID shape this PR now accepts everywhere else. Drop the braces from the same command and it's allowed. This is a real allow→deny flip caused by quoting, not a hypothetical, and I traced it statically against _trusted_system_readonly_head/is_exact_readonly_supporting_command to confirm the interior check is the only gate standing in the way.

It's narrow rather than blocking — /usr/bin/test -d '...{guid}...' (and stat/ls/du on the same path) already goes through _literal_shell_words on the whole command and does pick up the fix, so the identical inspection is reachable today under a different spelling. Only the [ ... ] bracket-test spelling is left behind, and no test in this PR's test_hygiene.py diff exercises _absolute_bracket_test_words or is_exact_readonly_supporting_command with a quoted brace either way.

Suggested fix: scan _without_quoted_braces(interior) in _absolute_bracket_test_words (line 1676), and for consistency in _parse_bracket_test_words (line 1448) too, even though that one is currently dead code for this purpose — its caller at line 1700 denies the bare [ ... ] form unconditionally regardless of what it returns.

Fix this →

Everything else in this round checked out: the _WHOLE_WORD_QUOTED regex and its two now-fixed call sites (_literal_shell_words, _unparsable_reason) stay in agreement with the manual quote-tokenizing loop, the CHANGELOG/version-bump edits are cosmetic and correct, and the test_hygiene.py additions correctly exercise the boundary cases they target (adjacent quotes, cross-boundary braces, $/backtick/${}/$() inside quotes). No new regressions introduced by b09cd97.

I did not execute the test suite in this sandbox (no tool access to run python3); the author's and prior reviewer's test-run results are author-claimed and unverified by me — this finding is from static tracing of the code paths only.
· branch

[claude]

Claude finished @kyle-sexton's task in 48s —— View job

---

Security review complete

• Gather context (PR diff, custom instructions)
• Invoke /review:security-review for PR fix(disk-hygiene): accept braces inside quoted words in the engine gate #5644 (HEAD b09cd97)
• Post findings

Skip gate hit (item 3): this head already has a successful security review that still applies. The only commit added since the prior review (e5ad74d, posted above) is b09cd97, "satisfy markdown and machine-path lint for the quoted-brace change." I diffed e5ad74d...b09cd97 directly:

plugins/disk-hygiene/CHANGELOG.md                         | 3 ++-
plugins/disk-hygiene/skills/clean/scripts/test_hygiene.py | 2 +-

• destructive_guard.py — the only security-relevant file in this PR — is byte-for-byte unchanged since the prior review.
• test_hygiene.py: the one-line change swaps the test fixture's GUID path (C:/Users/x/AppData/Local/wsl/{...} → D:/wsl/{...}), a shorter literal for a machine-path lint rule. Same shape, same assertion, no new behavior under test.
• CHANGELOG.md: the same path literal updated in prose, plus a blank-line fix. No code or policy content.

Neither touches the quoted-brace exception logic (_without_quoted_braces, _literal_shell_words, _unparsable_reason) reviewed at e5ad74d, so that review's conclusion — no security issues found, including the instruction-exception-register pass on the narrowed forbidden-character blocklist — still applies at b09cd97. Per the skip gate, no further findings are posted.

(Unrelated to security: the code-reviewer's completeness-gap finding on _absolute_bracket_test_words not routing through _without_quoted_braces is a fail-safe over-denial, not a security issue — it blocks a trusted read-only command rather than permitting an unsafe one — so it's out of scope here and is already tracked in that review's comment.)
· branch

[claude]

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

---

Code review complete

• Gather context (PR diff, changed files, CLAUDE.md/AGENTS.md conventions)
• Invoke /review:code-review for PR fix(disk-hygiene): accept braces inside quoted words in the engine gate #5644 (HEAD f23d668)
• Post findings

Summary

Since the last review round (b09cd97), one new commit landed: e2f9b96, "accept quoted braces in the absolute bracket-test belt." It directly resolves the completeness gap both prior code-review passes flagged — _absolute_bracket_test_words and _parse_bracket_test_words previously scanned interior directly against _SHELL_EXPANSION_OR_OPERATOR_CHARS without routing through _without_quoted_braces, so /usr/bin/[ -d '...{guid}...' ] was denied solely for the quoted brace even though the equivalent /usr/bin/test spelling was already accepted.

The fix at L1678-1680 and L1448-1450 now runs _SHELL_EXPANSION_OR_OPERATOR_CHARS.intersection(_without_quoted_braces(interior)) at both call sites — semantically equivalent to the any(... for value in _without_quoted_braces(...)) form already used in _literal_shell_words and _unparsable_reason. I traced the bookend/strip logic in both functions and the frozenset-intersection idiom is a correct, if stylistically distinct, way to express the same "any forbidden char present" check.

Test coverage added for the previously-missing case: test_hygiene.py L11179-11182 exercises is_exact_readonly_supporting_command with the quoted-GUID bracket-test form (accept) and the unquoted form (still reject). _parse_bracket_test_words's own fix has no dedicated new test, but that function remains dead code for the accept path (its only caller, is_exact_readonly_supporting_command at L1704, returns False unconditionally whenever it parses successfully), so this is a non-issue — correctly called out as "for consistency" in the commit message rather than claimed as a behavior fix.

The core quoted-brace machinery (_WHOLE_WORD_QUOTED, _without_quoted_braces, and the _literal_shell_words/_unparsable_reason call sites) is byte-for-byte unchanged from the prior two review rounds, which already confirmed it correct against the manual quote-tokenizer and the full forbidden-character set. No new issues there.

No blocking findings. The gap identified across both earlier review rounds is now closed, and no new issues were introduced by e2f9b96. CHANGELOG (0.42.1) and version bump are consistent with the fix.

I did not execute the test suite in this sandbox (no permitted tool access to run python3 here); the author's test-run results are author-claimed and unverified by me. This review is from static tracing of the code paths and diff only.
· branch

[claude]

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

---

Security review complete

• Gather context (PR diff, custom instructions)
• Invoke /review:security-review for PR fix(disk-hygiene): accept braces inside quoted words in the engine gate #5644 (HEAD f23d668)
• Post findings

No security issues found.

Scope: the HEAD moved from b09cd97 (last reviewed) to f23d668 via a merge of origin/main. The merge itself pulls in an unrelated feature (b906919, catalog/ownership-investigation), but git diff origin/main...HEAD confirms that feature's files aren't part of this PR's diff — the PR still touches only plugin.json, CHANGELOG.md, destructive_guard.py, and test_hygiene.py. The merge also carries one new PR commit, e2f9b96, "accept quoted braces in the absolute bracket-test belt," which is security-relevant and wasn't covered by the prior reviews at e5ad74d/b09cd97, so I didn't rely on the skip gate this round.

e2f9b96 closes the completeness gap the earlier code-review pass flagged (quoted-brace GUID paths were still denied through the /usr/bin/[ ... ] bracket-test spelling even though the equivalent test/ls/stat/du spelling was accepted). It routes _without_quoted_braces — the same helper already reviewed and found sound at e5ad74d — into the two remaining raw interior scans:

• _parse_bracket_test_words L1437-L1452
• _absolute_bracket_test_words L1655-L1683

This is the same exception, applied consistently, not a new carve-out — so the earlier instruction-exception-register analysis (quoting makes {/} inert for both target shells, $/backticks/parens/pipes/etc. still refused regardless of quoting) still covers it. I re-verified directly against this new code:

• _absolute_bracket_test_words.intersection(_without_quoted_braces(interior)) is a correct, equivalent rewrite of the old any(c in SET for c in interior) scan (frozenset .intersection() over a string iterates its characters the same way) — no characters silently stop being checked.
• The widened acceptance only affects is_exact_readonly_supporting_command, the read-only ls/stat/du/test/find inspection belt; accepted commands still pass through _literal_shell_words/_trusted_system_readonly_head, so a quoted brace can let a literal GUID-shaped word through but still can't smuggle $, backticks, parens, ;, |, or an unquoted/boundary-crossing brace — same deny set as every other call site.
• _parse_bracket_test_words (L1437) picking up the same fix is cosmetic/consistency-only: its sole caller (L1704) denies the bare [ ... ] form unconditionally whenever this returns non-None, and the fallback path still has [/] themselves (untouched by _without_quoted_braces, which only strips {/}) blocking _literal_shell_words on that branch — so this function's return value can't flip an accept either way.
• New test is_exact_readonly_supporting_command coverage (test_hygiene.py, the D:/wsl/{guid} bracket-test case) exercises exactly this path and matches the fix.

plugin.json (0.42.0 → 0.42.1, correctly rebased past main's version bump) and the CHANGELOG.md entry are prose/metadata only, not security-relevant.

GitHub Actions hardening is out of scope for this lane (zizmor's); this PR doesn't touch any workflow file.
· branch