fix(disk-hygiene): match globs case-insensitively and complete the denial allow-list

[kyle-sexton]

Summary

Takes findings 1 and 5 of #1806. The issue bundles seven findings; all seven are verified in a comment on the issue, and this PR deliberately does not close it — five findings remain open, three of them needing a maintainer's call rather than an implementation.

Fix

Finding 1 — hint matching was misanchored and case-sensitive in the unsafe direction. has_protected_name() casefolds and matching_hints() did not, so on Windows and macOS — where both spellings name the same file — protection was case-robust while discovery was not.

All six fnmatchcase call sites now go through one glob_matches() helper: hints, consumer protection globs, and the protection re-checks in the preview, verify, and apply lanes. That is the part the issue flagged as needing deliberate handling — the protection globs move with the hints rather than by accident. Casefolding is the safe direction for both roles: a protection glob that matches more can only keep more, and a hint that matches more can only surface more for triage, since hints are discovery signals and never cleanup verdicts. The helper casefolds both operands and keeps fnmatchcase rather than switching to fnmatch, whose folding follows the host platform — a matcher whose verdict changes with where the scan runs is not one a protection can rest on.

A new atomic-write-staging-remnant hint (*.tmp.*, ceiling medium) covers the class the producer-specific hint's own reason already claimed: .tmp as an infix before a pid and random suffix, the standard write-temp-then-rename shape. The producer-specific hint still fires alongside it, since it carries a narrower reason.

Finding 5 — the Bash denial text under-reported the allow-list. It enumerated four engine subcommands and omitted the read-only kill-switch probe that _decide allows before the classifier ever runs. Since the documented bootstrap path is to submit a wrong shape so the denial teaches the grammar, a consumer learning the allow-list from the denial never learned the probe is permitted — and the probe is the step that lets the model state the kill-switch value honestly instead of assuming the default. The denial now names the probe and discloses the bundled engine's own path, the only route left when a rendered body's ${CLAUDE_PLUGIN_ROOT} arrives unexpanded and the exact-path identity check denies every guess.

The enumeration and the grammar are now one list, as suggested: classify_exact_engine_command rejects any subcommand outside _ALLOWED_ENGINE_SUBCOMMANDS before its own dispatch, and both bundled script paths come from a single accessor each, so the message cannot teach a grammar the classifier does not implement.

Verification

Running matching_hints() against the shipped baseline, before and after:

Name	Before	After
.rate-limits.json.tmp.1363789.17391	none	atomic-write-staging-remnant
settings.json.tmp.4	none	atomic-write-staging-remnant
.claude.json.tmp.9552.9bfba4e83eaa	claude-json-failed-atomic-write	both hints
thumbs.db	none	windows-explorer-metadata
TMP-build	none	common-temp-directory
Scratch.md	none	scratch-artifact

Thumbs.db, tmp-build, scratch.md, and foo.tmp are unchanged, so this widens matching without disturbing what already matched.

One correction to the issue's table: the .DS_Store row does not hold on Windows. macos-finder-metadata declares "os": ["macos"] and matching_hints() filters by current OS before matching, so neither spelling hints on a Windows host. That is correct OS filtering, not a defect; the case finding stands on the other five rows.

Gates:

• bash plugins/disk-hygiene/skills/clean/scripts/hygiene.test.sh — 228 tests, OK (4 skipped); 5 new tests covering case-insensitive hints, the staging-remnant class, case-insensitive consumer protection globs, the denial text naming every accepted shape, and the classifier rejecting a subcommand outside the shared list.
• bash scripts/check-changelog-parity.sh --check-order — pass.
• check-skill.sh clean — PASS, 0 errors; markdownlint-cli2 — 0 errors; ruff check — clean.

Related

• Refs disk-hygiene: hint matching is misanchored and case-sensitive, byte accounting cannot express "unknown", and five smaller findings from a live audit #1806 — stays open. This PR takes findings 1 and 5 only. Verified and still open there: finding 2 (byte qualification — its size_qualifiers mechanism is landing in PR fix(disk-hygiene): hard-protect cloud-sync placeholders and tenant sync roots #1818, and the rest should build on that rather than race it), finding 3 (a summarize surface — a new subcommand and a new shape in the guard's grammar, a design call), finding 4 (the Stop detector's marker — both remedies change when a safety-observability detector goes quiet, a maintainer call), finding 6 (the probe's environment channel — the probe's allow-list requires exactly two tokens, so the probe, the guard, and SKILL.md have to change together), and finding 7 (run-state retention — a durability policy call).
• Refs disk-hygiene: cloud-sync placeholders are unprotected, unhinted, and counted as reclaimable bytes — deleting one destroys the org's cloud copy #1804 / PR fix(disk-hygiene): hard-protect cloud-sync placeholders and tenant sync roots #1818 and disk-hygiene: the "provably a different file" escape added in #1640 lets the plugin's own stale engines bypass the kill switch #1805 / PR fix(disk-hygiene): deny the different-file escape to this plugin's own cache tree #1819 — the two CRITICALs from the same audit. All three are open against this manifest, so this PR takes 0.13.0 (0.11.0 and 0.12.0 are claimed by fix(disk-hygiene): hard-protect cloud-sync placeholders and tenant sync roots #1818 and fix(disk-hygiene): deny the different-file escape to this plugin's own cache tree #1819). Merged in issue order the changelog reads contiguously.

No linked issue

🤖 Generated with Claude Code

https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C

[claude]

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

---

Security Review — PR #1820

• Gather PR diff and changed files
• Read destructive_guard.py — command guard, allow-list, injection surfaces
• Read hygiene.py — glob matching, path traversal, OS-filter logic
• Read test_hygiene.py — test surface
• Read baseline-policy.json — new hint entries
• Read plugin.json — version bump only
• Analyze findings and write report

---

Summary

No security vulnerabilities found. The two functional changes in this PR are both conservative from a security standpoint — one tightens the guard's subcommand gate, the other widens only in the safe direction. Analysis by area:

---

glob_matches() — case-insensitive glob helper (hygiene.py:107)

Replaces five fnmatch.fnmatchcase(subject, pattern) calls with fnmatch.fnmatchcase(subject.casefold(), pattern.casefold()). Operates in two security-distinct roles:

Protection globs (consumer's additional_protected_path_globs, checked in scan_tree, preview, handoff_verify, apply_plan): wider matching is strictly conservative — a protection glob that matches more can only protect more paths from deletion, never fewer. The previous case-sensitive behavior was the hole: Deliverables/** not protecting deliverables/… on a case-insensitive filesystem (Windows/macOS) was an under-protection, not a precision.

Hint matching (matching_hints): hints are explicitly discovery signals with no deletion authority. A hint that fires on more names can only surface more entries for triage; it carries no consequence in the deletion pipeline.

No locale injection, no shell expansion: fnmatch.fnmatchcase does no subprocess execution and str.casefold() is locale-independent Unicode normalization.

Verdict: no issue.

---

_ALLOWED_ENGINE_SUBCOMMANDS early gate (destructive_guard.py:223 / 777)

Before this change, classify_exact_engine_command dispatched unknown subcommands through the if/elif chain and fell off the end returning None. The new code adds an explicit rejection at the entry point:

if tokens[2] not in _ALLOWED_ENGINE_SUBCOMMANDS:
    return None

This is strictly tightening — it makes the existing implicit rejection explicit and prevents a future subcommand added to the dispatch chain but not to the tuple (or vice versa) from causing undefined behavior. The tuple is the single authoritative list for both the classifier and the denial text, which closes the drift class the PR describes.

Verdict: no issue, strictly tighter.

---

Denial text disclosing bundled paths (destructive_guard.py:962)

_bash_denial_guidance() now includes _display_path(_engine_script_path()) and _display_path(_probe_script_path()) — absolute paths of the two bundled scripts. Both are derived from Path(__file__).resolve(), which is the interpreter's own view of where the running guard lives. Concerns evaluated:

• Path disclosure: The model session already has access to the plugin install directory (it launched the guard from there). The paths are not new information to a session that can run these scripts; the disclosure teaches the grammar to a session that cannot yet reach the engine.
• Tamper resistance: The paths come from __file__, not from environment variables or user input, so a hostile repo's env block cannot redirect them.
• Classifier / denial consistency: Both the is_exact_kill_switch_probe classifier and the denial now call the same _probe_script_path() accessor, so what the denial teaches the model to submit is exactly what the classifier will accept. The prior code had these inline independently, creating a drift surface.

Verdict: no issue.

---

atomic-write-staging-remnant hint (baseline-policy.json:99)

New name_glob hint with pattern *.tmp.*, ceiling medium. Hints have no deletion authority — the apply gate requires a human-approved plan with an approval token, and neither matching_hints nor validate_hint classify anything for deletion. A broad hint pattern can only surface more entries in the audit report for human triage.

Verdict: no issue.

---

No new subprocess or shell injection surfaces

All new code paths operate via string comparison (in, ==) and fnmatch.fnmatchcase. No new subprocess.run calls, no new shell interpolation, no new trust boundaries.

---

No findings to report.

[claude]

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

---

Security Review — PR #1820

• Gather PR diff and changed files
• Read destructive_guard.py — command guard, allow-list, injection surfaces
• Read hygiene.py — glob matching, path traversal, OS-filter logic
• Read test_hygiene.py — test surface
• Read baseline-policy.json — new hint entries
• Read plugin.json — version bump only
• Analyze findings and write report

---

Finding 1 — Test defect: Windows-specific hint asserted without OS mock (will fail on Linux CI)

test_hygiene.py lines 116–130 asserts that Thumbs.db and thumbs.db produce the windows-explorer-metadata hint:

for name, expected in (
    ("Thumbs.db", "windows-explorer-metadata"),
    ("thumbs.db", "windows-explorer-metadata"),
    ...
):
    matched = {hint["id"] for hint in hygiene.matching_hints(name, name, policy)}
    self.assertIn(expected, matched, name)

matching_hints() calls current_os = os_key() and skips any hint whose "os" list does not include the runtime platform. On Linux, windows-explorer-metadata ("os": ["windows"]) is filtered out, so the assertion "windows-explorer-metadata" in matched fails. The test has no mock.patch.object(hygiene, "os_key", return_value="windows") guard — unlike similar tests in the file that do mock the OS (lines 368, 405). The remaining four assertions (tmp-build, scratch.md, and their case variants, all "os": ["all"]) would pass on Linux.

Fix: Wrap the Windows-specific assertions in a mock.patch.object(hygiene, "os_key", return_value="windows") context, matching the pattern used elsewhere in the suite.

Fix this →

---

Finding 2 — Correctness: *.tmp.* pattern naming

baseline-policy.json line 103 introduces *.tmp.*. Under fnmatch, the trailing * matches zero or more characters, so foo.tmp. (with a trailing dot, zero characters after the dot) also matches. This is likely benign — a file ending in .tmp. is equally suspicious — but it's worth noting the pattern is broader than .tmp. followed by a non-empty suffix. The comment in the PR ("a .tmp infix before a pid and random suffix") implies non-empty content after the dot. If truly strict matching is desired, the pattern would be *.tmp.?*; as written the current behaviour is fine given the "safe direction" rationale (more hint matches = more for triage, never a verdict).

---

What looks correct

glob_matches() implementation (hygiene.py lines 107–123):

return fnmatch.fnmatchcase(subject.casefold(), pattern.casefold())

Casefolding both operands before using fnmatchcase is correct. fnmatch.fnmatch's platform-dependent folding is avoided, and the safe-direction rationale (protection globs match more → keep more; hint globs match more → surface more) is sound. casefold() never produces *, ?, [, or ] from any Unicode input, so there is no metacharacter-injection risk through case transformation. All five fnmatch.fnmatchcase call sites are replaced uniformly; discovery and protection now use the same logic rather than disagreeing on case.

_ALLOWED_ENGINE_SUBCOMMANDS single-source and pre-check (destructive_guard.py lines 223, 777): the tuple drives both the classifier's if tokens[2] not in _ALLOWED_ENGINE_SUBCOMMANDS: return None guard and the denial text's enumeration, so the two cannot silently diverge. The new pre-check at the top of classify_exact_engine_command means any subcommand added to the tuple but lacking a corresponding if tokens[2] == ... branch returns None (closed) rather than accidentally falling through.

_engine_script_path() / _probe_script_path() accessors (lines 226–235): both paths are computed once and shared between the classifier and the denial text, verified to resolve to existing files (hygiene.py in the same directory, kill_switch_probe.py at skills/setup/scripts/). This closes the gap where the denial could teach a path the classifier did not use, or vice versa.

_bash_denial_guidance now discloses the probe (lines 962–984): the probe runs before the classifier in _decide, so a consumer learning the grammar from the denial was previously missing the probe's allowed shape. Now disclosed.

test_bash_denial_names_every_shape_the_classifier_accepts locks the invariant: it asserts that every element of _ALLOWED_ENGINE_SUBCOMMANDS, the probe filename, and the engine's absolute path all appear in the denial string. This test would catch future drift.

test_classifier_rejects_a_subcommand_outside_the_shared_list verifies the pre-check: summarize (not in the tuple) correctly returns None.

---

Summary

One test defect that will cause CI failures on Linux (Finding 1); one non-blocking pattern note (Finding 2). The core logic changes — glob_matches(), _ALLOWED_ENGINE_SUBCOMMANDS, the new accessors, and the denial text update — are correct and tighten the invariants they target.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3ea124c1f5

ℹ️ 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 3m 25s —— View job

---

Security Review — PR #1820

• Initialize task list
• Get PR diff and identify changed files
• Read hygiene.py — glob matching, casefold logic, path handling
• Read destructive_guard.py — command guard, allow-list, injection surfaces, path disclosure
• Read baseline-policy.json — new hint entries
• Analyze findings and write report

---

No security vulnerabilities found. Analysis by area:

---

glob_matches() — case-insensitive glob helper (hygiene.py:107)

str.casefold() is applied to both subject and pattern before fnmatch.fnmatchcase. The key safety property: casefold() performs Unicode case normalization over letter code points only. The glob metacharacters * (U+002A), ? (U+003F), [ (U+005B), and ] (U+005D) are ASCII punctuation with no casing relationship in Unicode — no input character can casefold to any of them. There is no metacharacter-injection surface through case transformation.

On the protection side, wider matching is strictly conservative (a protection glob that matches more can only keep more paths from deletion, never fewer). On the hint side, wider matching is safe (hints are discovery signals with no deletion authority — the apply gate requires a human-approved plan with an approval token). All five prior fnmatch.fnmatchcase call sites are replaced uniformly; discovery and protection now use the same logic.

Verdict: no issue.

---

_ALLOWED_ENGINE_SUBCOMMANDS single-source gate (destructive_guard.py:223 / 777)

The new early check if tokens[2] not in _ALLOWED_ENGINE_SUBCOMMANDS: return None is inserted before the if/elif dispatch chain. Before this change, an unknown subcommand would fall through the entire chain, returning None implicitly. The behavior in _decide for a None return is identical — it reaches the deny branch. The change makes the existing implicit rejection explicit and prevents a future subcommand being added to the dispatch chain without a corresponding tuple entry (or vice versa) from causing undefined behavior. Strictly tightening.

Verdict: no issue, strictly tighter.

---

_engine_script_path() / _probe_script_path() shared accessors (destructive_guard.py:226–235)

Both paths are derived from Path(__file__).resolve() — the interpreter's own view of the running guard's location — not from environment variables or user-controlled input. A hostile repository's env block cannot redirect them. They are shared between the classifier (_script_path_key comparison) and the denial text, so what the denial teaches the model to submit is exactly what the classifier will accept. This closes the drift surface that existed when the paths were inlined independently.

The _probe_script_path() path construction (parents[2] / "setup" / "scripts" / "kill_switch_probe.py") navigates relative to the resolved guard location. This is unchanged in intent from the pre-existing inline path — only refactored to an accessor.

Verdict: no issue.

---

_display_path() in denial guidance (destructive_guard.py:238 / 962)

_display_path converts a Path to a forward-slash absolute string for the denial text. This output is a text payload returned to the Claude session as a denial reason — it is never passed to a shell or evaluated as code. The backslash-to-forward-slash substitution (os.fspath(path).replace("\\", "/")) has no injection surface because the string is consumed as prose, not as shell input. The absolute paths disclosed (plugin cache directory) are already accessible to the session that launched the hook; no new trust boundary is crossed.

Verdict: no issue.

---

*.tmp.* hint in baseline-policy.json (line 99)

The new hint pattern *.tmp.* is a discovery signal with no deletion authority. Hints are surfaced in scan output for human triage; the apply gate requires an explicit, human-approved plan with an approval token. A broader hint pattern can only surface more entries, which is the safe direction.

Verdict: no issue.

---

No new subprocess or shell execution surfaces

All new code paths operate through string comparison (in, ==) and fnmatch.fnmatchcase. No new subprocess.run calls, no new shell interpolation, no new trust boundaries are introduced in this PR.

---

No security findings to report.

[claude]

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

---

PR Review — #1820

• Post initial comment
• Get PR diff and review changed files
• Check if latest commit (384b6c8) fixed the Windows-mock test issue flagged in prior reviews
• Review hygiene.py — glob matching changes
• Review destructive_guard.py — allow-list and denial text changes
• Review test_hygiene.py — new test coverage
• Review baseline-policy.json — new hint entry
• Synthesize and post full review

---

Review

The prior Finding 1 (Windows-specific hint tested without OS mock) is correctly resolved by the follow-up commit. 384b6c8 splits Thumbs.db out of the OS-agnostic table into a new test_platform_scoped_hints_are_also_case_insensitive method that patches os_key for both "windows" and "macos" before asserting — exactly the pattern used elsewhere in the suite. The OS-agnostic table adds failed-write.tmp / FAILED-WRITE.TMP as its case-folding witness instead. Both the Codex and prior Claude review flagged this; it is cleanly addressed.

Prior Finding 2 (*.tmp. boundary) — confirmed intentional. fnmatch.fnmatchcase("foo.tmp.", "*.tmp.*") does match; the PR and review both acknowledge this. Given that hints are discovery-only (no deletion authority) and the "safe direction" rationale is explicitly documented in the hint's reason string, this is a reasonable call.

---

hygiene.py — glob_matches()

hygiene.py:107–123 — The implementation is correct. fnmatch.fnmatchcase is locale-independent; str.casefold() cannot introduce glob metacharacters; and all five former fnmatchcase call sites now route through the helper uniformly, including the protection re-checks in preview, handoff_verify, and apply_plan. The docstring is thorough and accurately explains why fnmatch.fnmatch is not a safe substitute.

One minor style observation: _display_path at line 238 is followed by only one blank line before the large comment block that precedes _MARKER_TOKEN_SPLIT (PEP 8 asks for two between top-level definitions). Since ruff check passes, this is a pre-existing or tolerated convention — no action needed.

---

destructive_guard.py — _ALLOWED_ENGINE_SUBCOMMANDS and accessors

line 223: The single-source tuple is the right structure. The pre-check at line 777 (if tokens[2] not in _ALLOWED_ENGINE_SUBCOMMANDS: return None) now makes the implicit fall-through explicit and prevents any future subcommand added to the dispatch chain but not the tuple from leaking through — strictly tighter.

_probe_script_path() at line 231: The parents[2] traversal from scripts/ resolves to skills/, giving skills/setup/scripts/kill_switch_probe.py. Verified: the file exists at that path. Both is_exact_kill_switch_probe and _bash_denial_guidance now call the same accessor, so the classifier and the denial cannot diverge.

_bash_denial_guidance at line 974: The ", ".join(tuple[:-1]) + ", and " + tuple[-1] construction is correct for the current four-element tuple. If the tuple ever grows to two elements this degenerates to ", and X" (empty prefix), which is still grammatical. If it shrinks to one, tuple[:-1] is empty and the join produces ", and only-element" — mildly odd but still parseable. Not a real risk given the stable set of subcommands, but noted.

---

test_hygiene.py — new tests

Five new tests, all well-targeted:

• test_hints_match_a_name_whatever_its_case — OS-agnostic rows only; the fix-commit correctly replaced the Windows row with failed-write.tmp / FAILED-WRITE.TMP, which is OS-agnostic ("os": ["all"]).
• test_platform_scoped_hints_are_also_case_insensitive — covers Windows (Thumbs.db, thumbs.db, THUMBS.DB) and macOS (.DS_Store, .ds_store) under pinned os_key. Good: THUMBS.DB adds a third casing the prior table lacked entirely.
• test_atomic_write_staging_remnants_are_hinted_as_a_class — verifies the three concrete examples from the PR body, plus the co-fire of the producer-specific hint alongside the class hint.
• test_consumer_protection_globs_match_whatever_the_case — an integration-level test: creates a real deliverables/report.tmp on disk, loads a policy with Deliverables/**, scans, and asserts consumer-protected-path in the entry's protected_reasons. This is the right level to test the protection side — it exercises scan_tree end-to-end rather than only the matcher.
• test_bash_denial_names_every_shape_the_classifier_accepts and test_classifier_rejects_a_subcommand_outside_the_shared_list — together they lock the invariant the PR is about. The rejection test uses "summarize" (the planned-but-not-yet-built subcommand from Finding 3) as the outsider, which is a thoughtful choice: it documents the gap while verifying the gate.

---

baseline-policy.json

New atomic-write-staging-remnant entry at line 99. Pattern *.tmp.*, ceiling medium, "os": ["all"]. Positioned before claude-json-failed-atomic-write so the specific hint immediately follows the class hint — good ordering for readability. Reason string is accurate and actionable.

---

Summary

Both functional changes are correct and conservative. The test suite adequately covers all new behavior. The prior test defect is properly fixed. No outstanding correctness, security, or alignment issues.

[claude]

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

---

Code Review — PR #1820

• Gather PR diff and changed files
• Review hygiene.py — glob matching changes
• Review destructive_guard.py — allow-list and denial text changes
• Review test_hygiene.py — new test coverage
• Review baseline-policy.json — new hint entry
• Check CLAUDE.md / AGENTS.md alignment
• Post findings

Security review is out of scope for this lane (handled by the dedicated security-review runs already posted on this PR, which found no issues). This pass is correctness / architecture / test coverage / maintainability only.

---

Correctness

Both changes do what they claim:

• glob_matches() (hygiene.py:138-153) is applied uniformly at all five former fnmatch.fnmatchcase sites (matching_hints, scan_tree, preview, handoff_verify, apply_plan) — no site was missed, so hints and every protection re-check lane now agree on case.
• _ALLOWED_ENGINE_SUBCOMMANDS (destructive_guard.py:275-281) is the single source for both the classifier's pre-check and the denial enumeration, and _engine_script_path() / _probe_script_path() are likewise shared between the classifier and the denial text — this closes the exact drift class the PR describes. Verified _probe_script_path()'s parents[2] traversal resolves to the real skills/setup/scripts/kill_switch_probe.py on disk.

Test coverage

The five new tests are well-targeted and the prior Windows-mock defect (flagged by Codex and an earlier automated review) is correctly fixed in 384b6c8 by splitting the platform-scoped assertions under a pinned os_key.

One gap, low severity: test_consumer_protection_globs_match_whatever_the_case (test_hygiene.py:194-228) exercises only the scan_tree protection re-check. The preview, handoff_verify, and apply_plan lanes got the identical one-line change (fnmatchcase → glob_matches) but have no dedicated case-insensitivity regression test of their own — coverage there is indirect, riding on glob_matches() being unit-tested plus scan_tree's coverage. This is a pre-existing pattern (those three lanes had no consumer-glob test before this PR either), so it's not a regression this PR introduces, just an opportunity the PR could have closed while it was already touching all four sites.

Maintainability (minor, non-blocking)

_display_path() (destructive_guard.py:293-295) is a new general helper (Path → Bash-friendly forward-slash string) introduced by this PR, but _display_python() (destructive_guard.py:87-93) — a few lines above where _display_path is used — does the identical os.fspath(...).replace("\\", "/") conversion inline instead of delegating to it:

def _display_python() -> str:
    try:
        runtime = Path(sys.executable).resolve(strict=True)
    except OSError:
        runtime = Path(sys.executable).absolute()
    return os.fspath(runtime).replace("\\", "/")

Since _display_path now exists, _display_python's last line could be return _display_path(runtime), removing the duplicated formatting logic. Not a bug — both produce the same output — just a small dedup this PR's own new helper makes possible.

Alignment with CLAUDE.md / AGENTS.md

No concerns. This change is plugin-internal logic (glob matching, allow-list enumeration) — it doesn't touch a plugin manifest, marketplace schema, or documented harness/official-docs surface, so the fresh-docs mandate doesn't apply here. The version bump to 0.13.0 and the changelog entry's "why the number skips" note correctly account for the two other open PRs (#1818, #1819) claiming 0.11.0/0.12.0 against the same manifest, consistent with the "Related" section's stated sequencing.

Note on test execution

I was not able to run bash plugins/disk-hygiene/skills/clean/scripts/hygiene.test.sh in this environment (the sandbox denied the command), so I reviewed the diff and existing test structure statically rather than re-confirming the 228/5-new-tests-pass claim from the PR description directly.

---

Summary: No correctness or architecture issues. One low-severity test-coverage gap (protection re-check case-insensitivity only tested at the scan_tree site) and one non-blocking maintainability nit (_display_python could delegate to the new _display_path). Both are optional polish, not blockers.

[claude]

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

---

Security Review — PR #1820

• Gather PR diff and changed files
• Read hygiene.py — glob matching changes
• Read destructive_guard.py — allow-list and denial text changes
• Read baseline-policy.json — new hint entry
• Read test_hygiene.py — new test coverage
• Analyze findings and post review

---

Finding — glob_matches()'s "casefold is always the safe direction" claim breaks for negated character-class ranges (IMPORTANT, confidence: medium)

hygiene.py:138-154:

return fnmatch.fnmatchcase(subject.casefold(), pattern.casefold())

The PR's entire safety argument for this helper — "a protection glob that matches more can only keep more" — assumes casefolding a pattern only widens what it matches. That holds for literal text and simple */? wildcards, but not for an fnmatch character class that names a case-specific range, especially a negated one like [!A-Z] or [!a-z].

fnmatch.translate() turns [!A-Z] into the regex [^A-Z] verbatim from the pattern text, and fnmatchcase compiles it without re.IGNORECASE — the case-sensitivity lives entirely in which letters appear in the class. Casefolding the pattern string before translation doesn't make the class "case-insensitive"; it silently swaps which case the class excludes:

• pattern.casefold() turns "[!A-Z]" into "[!a-z]" — i.e. [^A-Z] (exclude uppercase) becomes [^a-z] (exclude lowercase). That's a different class, not a case-folded version of the same one.

Concretely, for a consumer policy additional_protected_path_globs: ["[!A-Z]*"] (intended as "protect anything not starting with a capital letter"), on a file deliverables_report.txt:

• Before this PR (case-sensitive fnmatchcase): 'd' is not in A-Z → pattern [!A-Z]* matches → file is protected.
• After this PR: pattern casefolds to [!a-z]*; 'd' is in a-z → negated class no longer matches → file is not protected.

That's a narrowing, not a widening — exactly the direction the PR's safety reasoning rules out. Because glob_matches() backs the protection-glob checks in scan_tree, preview, handoff_verify, and apply_plan (hygiene.py:853, :1532, :1607, :1943), a consumer whose protection policy relies on a negated letter-range class could silently lose the protection guarantee for exactly the paths they meant to shield from apply_plan's deletion — in a plugin whose entire purpose is guarding against irreversible destructive deletion.

Scope/severity notes:

• Not exercised by the shipped baseline-policy.json — its protected_name_globs ("Dropbox (*)", "OneDrive - *") and hint patterns use no character classes, so this only affects consumer-supplied additional_protected_path_globs. The field's only documented example (README.md:135) is a plain ** glob, so the trigger condition is real but not the common case.
• has_protected_name() (unchanged by this PR, already casefolded pre-existing) likely has the identical flaw — worth a follow-up look, but it's out of this diff's scope.
• Non-negated classes (e.g. [A-Z]) don't have this problem — casefolding only ever widens them, consistent with the PR's stated invariant. It's specifically negation ([!...]) that flips the excluded set.
• I could not execute Python in this sandboxed review session to empirically confirm (command execution required approval unavailable here); the reasoning is derived directly from fnmatch.translate's documented handling of [!...] classes and fnmatchcase's case-sensitive compilation. Worth a quick empirical check: fnmatch.fnmatchcase('d', '[!A-Z]') → True, vs fnmatch.fnmatchcase('d'.casefold(), '[!A-Z]'.casefold()) → False.

Fix this →

---

Everything else reviewed — no issues

• _ALLOWED_ENGINE_SUBCOMMANDS single-source gate (destructive_guard.py:223, pre-check at :846): strictly tightens the classifier — an unknown subcommand now fails closed at the top of classify_exact_engine_command instead of implicitly falling through the if/elif chain. Both the classifier and the denial text drive off the same tuple, closing the drift class the PR targets.
• _engine_script_path() / _probe_script_path() / _display_path() (destructive_guard.py:277-299): both paths derive from Path(__file__).resolve() — not from environment variables or attacker-controlled input — so a hostile repo's env block can't redirect them. The classifier and the denial text now call the identical accessors, so what the denial teaches can't diverge from what the classifier accepts. _display_path is a pure string transform consumed as prose (a denial reason), never passed to a shell.
• Denial text disclosing bundled script paths (destructive_guard.py:1035-1048): both disclosed paths are things the session already has filesystem access to (it launched the guard from the same install directory); no new trust boundary is crossed.
• atomic-write-staging-remnant hint (baseline-policy.json:105-112): hints carry no deletion authority — apply_plan requires a human-approved plan with an approval token — so a broad *.tmp.* pattern can only surface more entries for triage, not authorize anything.
• No new subprocess/shell surfaces, no new environment-variable trust, no template/injection surfaces introduced by this diff.

Summary

One IMPORTANT finding: glob_matches()'s casefold-both-operands approach silently narrows (rather than only widens) matching for protection globs that use a negated fnmatch character-class range naming a specific letter case — the one case where the PR's "widening is always safe" invariant doesn't hold. Everything else in the diff (the subcommand allow-list gate, the shared path accessors, the denial-text disclosure, and the new hint) is correct and either neutral or strictly tightens existing behavior.

[github-actions]

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