fix(disk-hygiene): enforce disk_hygiene_enabled kill switch on PowerShell lane + via hook argv

[kyle-sexton]

Summary

The disk_hygiene_enabled=false kill switch ("audit-only mode") did not actually prevent deletions, for two independent reasons. Both are fixed here.

• B2 — the enabled flag never gated the PowerShell lane, so flagged deletion spellings still returned ask (and could be approved) with execution disabled.
• D3 — the kill switch was inert under the env-injection failure: the guard read the value from the hook process environment, which the runtime does not populate for a skill-frontmatter hook, so a configured false was silently overridden to enabled.

Fix

B2 — gate the PowerShell lane (skills/clean/scripts/destructive_guard.py). main() routed tool_name == "PowerShell" to powershell_decision() and returned before the enabled gate was computed (that computation was only reached on the Bash branch). The gate is now resolved once, before the tool-name branch, and threaded into powershell_decision(command, enabled). A flagged mutation spelling (Remove-Item/rm/del/::Delete/recycle-bin) resolves against the kill switch via new helper _powershell_mutation_verdict: ask when enabled (preserves the manual-handoff prompt), deny in audit-only mode. Engine (hygiene.py) calls stay hard-denied regardless.

D3 — deliver the value through the hook argv (skills/clean/SKILL.md + destructive_guard.py), mirroring the --authorized-data-root ${CLAUDE_PLUGIN_DATA} mechanism landed in #742 / #376. The frontmatter exec-form hook now passes --disk-hygiene-enabled ${user_config.disk_hygiene_enabled} in its args array. New resolve_disk_hygiene_enabled() reads the kill switch from that argv (generic _argv_flag_value extracted from _argv_authorized_data_root), treats a literal/unsubstituted placeholder or empty value as absent, and honors CLAUDE_PLUGIN_OPTION_DISK_HYGIENE_ENABLED only as a fallback. Absent every channel, the guard fails safe to enabled=true (guard active, every mutation still gated behind the final human prompt) — the pre-existing fail-safe default is preserved; the change is that a user who set false now has that value honored.

Per the plugins reference, ${user_config.KEY} is rejected only in shell-form fields; the sanctioned channel for exec-form hooks is args (or the env var) — exactly our hook shape.

Docs updated to match: SKILL.md audit-only note, reference/safety-model.md PowerShell paragraph (previously stated deletions are always ask), CHANGELOG.md, and plugin.json 0.4.3 → 0.4.4.

Verification

python -m unittest test_hygiene — 77 passed, 4 skipped (Linux-only platform gates). New tests:

• B2: test_powershell_deletion_spellings_denied_in_audit_only_mode — the five mutation spellings + ::Delete return deny when disabled. The pre-existing test_powershell_deletion_spellings_force_final_prompt (enabled → ask) still passes, proving the enabled path is unchanged.
• D3 / acceptance: test_kill_switch_blocks_every_lane_via_argv_without_env — the centerpiece: a configured false reaching the guard only via argv, with the env var UNSET (the exact env-injection failure) denies both a PowerShell Remove-Item and a Bash apply. test_kill_switch_enabled_via_argv_without_env_still_gates confirms an argv true (env unset) yields ask, proving the argv value — not a hardcoded default — drives the decision.
• Precedence/seam: test_resolve_disk_hygiene_enabled_precedence (argv-over-env both directions, case/whitespace-tolerant false, placeholder/empty → env fallback, no-channel → fail-safe enabled), test_argv_flag_value_parses_both_arg_spellings, and test_skill_hook_passes_disk_hygiene_enabled_flag_matching_constant (locks the frontmatter-args ↔ guard-constant seam).

Linters: ruff check — All checks passed; markdownlint-cli2 on the 3 changed markdown files — 0 errors; check-changelog-parity.sh --check and --check-bump origin/main — pass (0.4.4 entry present). (ruff format is not the repo's gate — origin/main is not ruff format-clean either, so it is not run to avoid unrelated churn.)

Caveat (inherited, unverifiable without the live harness): the != "false" comparison assumes a boolean userConfig serializes to the string "false". This is the same assumption the pre-existing env-var gate already made (CLAUDE_PLUGIN_OPTION_... was parsed identically), so it is no new regression; and live-harness substitution of ${user_config.disk_hygiene_enabled} into exec-form args rests on the plugins-reference guarantee (same substitution pass as ${CLAUDE_PLUGIN_DATA}, already proven to resolve in this exact position) and cannot be exercised without the running harness.

Closes #382

Related

• Source: disk-hygiene: disk_hygiene_enabled kill switch does not gate the PowerShell lane and is inert under the env-injection failure #382.
• disk-hygiene: skill-frontmatter guard reads CLAUDE_PLUGIN_DATA from env but never receives it -> engine lane fails closed on all platforms #376 & fix(disk-hygiene): guard resolves authorized data root from hook argv #742 — the env-injection root cause and the --authorized-data-root argv precedent this builds on.

---

🤖 Generated with Claude Code

[claude]

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

---

PR #968 Review — fix(disk-hygiene): enforce disk_hygiene_enabled kill switch

• Gather context and understand the request
• Read changed files
• Analyze correctness of B2 fix (PowerShell lane kill-switch gate)
• Analyze correctness of D3 fix (argv-based kill-switch delivery)
• Review test coverage for new paths
• Check security implications
• Check alignment with CLAUDE.md / project guidelines
• Post final review

---

Overall Assessment

Approve with minor observations. Both bugs are correctly identified, correctly fixed, and the fixes are well-tested. The code is clean, the failure modes are documented, and the security properties are preserved. Three low-severity observations follow — none block merge.

---

B2 Fix (PowerShell lane)

Correct. The root cause is clearly visible in the diff: enabled was only computed after the tool_name == "PowerShell" early-return branch in main(), so the kill switch never reached that lane. The fix resolves enabled unconditionally before the branch (line 377) and threads it into powershell_decision (line 380). The new _powershell_mutation_verdict helper correctly maps enabled=False → deny and enabled=True → ask, preserving the existing manual-handoff semantics on the enabled path.

Both code paths (_POWERSHELL_MUTATION_WORDS and _POWERSHELL_DOTNET_DELETE) are wired to the new helper, so all five test spellings (Remove-Item, rm, del, [IO.File]::Delete, [...FileSystem]::DeleteFile) are covered.

---

D3 Fix (argv-based kill-switch delivery)

Correct. The generalization of _argv_authorized_data_root into _argv_flag_value is clean and the original function becomes a thin wrapper — no behavior change on the existing path.

resolve_disk_hygiene_enabled() has the right precedence logic:

from_argv = _argv_flag_value(sys.argv[1:], _DISK_HYGIENE_ENABLED_FLAG)
if from_argv and from_argv != _DISK_HYGIENE_ENABLED_PLACEHOLDER:
    return from_argv.strip().lower() != "false"
return os.environ.get(_DISK_HYGIENE_ENABLED_ENV, "true").strip().lower() != "false"

Three edge cases are handled correctly:

• Unsubstituted placeholder ("${user_config.disk_hygiene_enabled}") — exact-string guard on line 168 → falls back to env.
• Empty string — falsy if from_argv check → falls back to env.
• No channel — env fallback defaults to "true" → enabled=True (fail-safe, guard stays active).

The minor .strip() addition on the env fallback path (not present in the old code) makes whitespace tolerance consistent across both channels — unambiguously correct.

---

Test Coverage

Coverage is solid and specifically targets the failure scenarios:

Test	What it verifies
test_powershell_deletion_spellings_denied_in_audit_only_mode	B2: all 5 PS spellings → deny when disabled (via env fallback)
test_kill_switch_blocks_every_lane_via_argv_without_env	B2+D3 acceptance: false via argv, env UNSET → both PS and Bash lanes deny
test_kill_switch_enabled_via_argv_without_env_still_gates	Confirms argv true → ask, not a hardcoded default
test_resolve_disk_hygiene_enabled_precedence	Full precedence matrix: argv-over-env, case/whitespace, placeholder fallback, no-channel
test_argv_flag_value_parses_both_arg_spellings	Both --flag value and --flag=value forms, absent-flag → None
test_skill_hook_passes_disk_hygiene_enabled_flag_matching_constant	Seam lock: frontmatter args array matches guard constants

The pre-existing test_powershell_deletion_spellings_force_final_prompt (enabled → ask) remains meaningful as a regression guard.

---

Observations (non-blocking)

1. run_guard_powershell_disabled implicitly relies on test-runner argv

test_hygiene.py line ~1322 — This helper sets the env var but does not mock sys.argv. resolve_disk_hygiene_enabled() reads sys.argv[1:] first; since the test runner's argv won't contain --disk-hygiene-enabled, from_argv is None and the env var correctly drives the result. This works, but it's a subtle implicit assumption. The parallel run_guard_enabled_argv helper (which does mock argv) could serve as a model if run_guard_powershell_disabled ever needs updating.

2. Seam test parses SKILL.md frontmatter as JSON — single-line dependency

test_hygiene.py line ~1573 — The seam test extracts args_line[args_line.index("["):args_line.rindex("]")+1] and parses it as JSON. This correctly handles the current single-line args: [...] format, but would silently fail (raise ValueError on index()) if the array is ever reformatted across multiple YAML lines. A low-priority brittleness — the test failing loudly if the format changes is actually acceptable behaviour for a seam lock, so this is a style note rather than a defect.

3. Placeholder exact-string comparison

destructive_guard.py line 168 — The unsubstituted-placeholder guard compares against "${user_config.disk_hygiene_enabled}" exactly. If the harness ever passes the literal in a different form (different whitespace, etc.), the check would miss it and interpret the placeholder as True (since "${...}".lower() != "false" is True). This is the same assumption as the --authorized-data-root precedent and is well-tested by the precedence test, but it is a documented caveat worth keeping in mind if the harness substitution behavior ever changes.

---

Security

• No new attack surface. The argv channel is model-unforgeable by the same argument as --authorized-data-root (exec-form hook, harness-controlled substitution).
• Fail-safe default preserved: unconfigured → enabled=True → guard active, every mutation still gated behind the human prompt.
• The kill switch value is now honored on all four lanes: Bash scan/preview (always allow), Bash apply (ask/deny), PowerShell mutation spellings (ask/deny), PowerShell .NET Delete (ask/deny).

---

Docs / Changelog

SKILL.md, safety-model.md, and CHANGELOG.md are consistent with the code changes. The 0.4.4 version bump in plugin.json is correct for a security fix. The CHANGELOG entry is clear and complete.

[claude]

Claude finished @kyle-sexton's task in 5m 17s —— View job

---

Security Review — PR #968

• Load tools
• Fetch PR diff
• Read changed source files
• Analyze for security vulnerabilities
• Post findings

---

Scope: destructive_guard.py, test_hygiene.py, SKILL.md, safety-model.md, plugin.json, CHANGELOG.md

Result: No critical or important security vulnerabilities found. The two fixes are correctly implemented and maintain the pre-existing fail-safe defaults. Two suggestion-level observations follow.

---

SUGGESTION — Confidence: High

Only the literal string "false" disables the kill switch; other common false-y representations are silently treated as enabled

destructive_guard.py L169 and L170:

return from_argv.strip().lower() != "false"
return os.environ.get(_DISK_HYGIENE_ENABLED_ENV, "true").strip().lower() != "false"

Any value that is not exactly "false" (case-insensitive, after stripping whitespace) — including "0", "no", "off", "disabled", or a YAML-serialized False that a harness might render differently — is treated as enabled=True. An administrator who sets disk_hygiene_enabled: no or disk_hygiene_enabled: 0 would find the kill switch silently inoperative with no error.

Security direction is correct: unexpected values fail safe to enabled=True (guard active, every mutation still gated behind the final human prompt). This is not a vulnerability — it's a usability trap that could cause audit-only mode to appear broken for non-false-string configurations. The same assumption was already made by the pre-existing env-var path, so this is no regression; the PR explicitly documents it as an inherited caveat.

---

SUGGESTION — Confidence: Medium

run_guard / run_guard_disabled / run_guard_powershell_disabled do not mock sys.argv, leaving a latent test fragility

test_hygiene.py L1061–L1085 and L1309–L1334:

These helpers patch os.environ but leave sys.argv at whatever the test-runner set it to. After this PR, resolve_disk_hygiene_enabled() reads sys.argv[1:] before falling back to the env var, so if the runner's argv happened to contain --disk-hygiene-enabled true, that would override the env-var mock and the run_guard_powershell_disabled-based tests would produce a misleading ask instead of the expected deny. Standard test-runner invocations (python -m unittest, pytest) never produce such argv, so this does not affect CI today.

The new run_guard_enabled_argv helper (added in this same PR) demonstrates the robust pattern: it mocks both sys.argv and os.environ together. Applying that same pattern to the existing helpers would fully seal the seam.

---

Notes on the core fixes

B2 (PowerShell lane never gated): enabled is now resolved once before the tool_name branch and threaded into powershell_decision. The helper _powershell_mutation_verdict gates flagged spellings against the kill switch. Engine calls remain hard-denied regardless of enabled, which is correct. No bypass.

D3 (env-injection failure): resolve_disk_hygiene_enabled() mirrors the pre-existing resolve_authorized_data_root() pattern: argv is the primary channel; env var is only the fallback. Placeholder and empty-string detection correctly handle the unsubstituted case and fall through to the env-var fallback. Fail-safe default ("true") is preserved. The exec-form hook args array means ${user_config.disk_hygiene_enabled} is substituted as a single OS-level argv token — no word-splitting occurs, so there is no argv-injection surface through the config value itself.

Fail-safe chain:

1. argv value "false" → enabled=False (user explicitly disabled)
2. argv value absent/placeholder/empty → env var
3. env var "false" → enabled=False
4. env var absent → default "true" → enabled=True (guard active)

All four paths are covered by the new tests. The acceptance test test_kill_switch_blocks_every_lane_via_argv_without_env with CLAUDE_PLUGIN_OPTION_DISK_HYGIENE_ENABLED explicitly unset is the correct centerpiece for D3.

[kyle-sexton]

Post-green review pass (work-2 / T5, 2026-07-22T08:24:56Z) — classification

CI green (functional gates + security-review=SUCCESS, no critical/important). Rule-10 diff-read done. Both SUGGESTIONs classified:

#	Finding	Class	Disposition
1	Only literal "false" disables; "0"/"no"/"off" treated as enabled	NON-ISSUE for the declared schema	No change. disk_hygiene_enabled is "type": "boolean" (default true) in plugin.json userConfig — the value can only be boolean true/false, rendered canonically (.strip().lower() handles "false"/"False"), so non-boolean spellings cannot occur. Residual is fail-SAFE (unexpected→enabled→guard active) and matches the pre-existing env path — no regression, as the reviewer notes.
2	run_guard/run_guard_disabled/run_guard_powershell_disabled don't mock sys.argv	VALID / deferred (minor test-fragility)	Latent only — standard runners never pass --disk-hygiene-enabled in argv, so CI is unaffected and the fix's correctness is unaffected. The new run_guard_enabled_argv helper already demonstrates the robust pattern. Deferred to #970 (test-hardening) to keep this defer-forbidden freeze-core fix moving.

No inline threads to resolve. Flipping to ready; handing to babysit. Merge remains human/merge-lane.

[claude]

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

---

PR #968 Review — fix(disk-hygiene): enforce disk_hygiene_enabled kill switch

• Gather context and understand the request
• Read changed files (destructive_guard.py, test_hygiene.py, SKILL.md, safety-model.md)
• Analyze correctness of B2 fix (PowerShell lane kill-switch gate)
• Analyze correctness of D3 fix (argv-based kill-switch delivery)
• Review test coverage for new paths
• Check security implications
• Check alignment with CLAUDE.md / project guidelines
• Post final review

---

Overall Assessment

Approve. Both bugs are correctly identified and fixed, the test matrix covers the exact failure paths, and the prior suggestions from the two automated review passes are correctly classified. Ready to merge.

---

B2 — PowerShell lane gate

Correct. The root cause was main() returning at the tool_name == "PowerShell" branch before enabled was computed; the kill switch never reached that lane. The fix computes enabled = resolve_disk_hygiene_enabled() unconditionally at line 377, before the branch. The _powershell_mutation_verdict helper at line 325 correctly maps enabled=False → deny and enabled=True → ask, preserving the manual-handoff semantics on the enabled path. Engine calls at line 306 remain hard-denied regardless of the kill switch — correct, because engine invocations must go through the Bash lane.

Both _POWERSHELL_MUTATION_WORDS and _POWERSHELL_DOTNET_DELETE paths flow through _powershell_mutation_verdict, so all five test spellings are covered end-to-end.

---

D3 — argv-based kill-switch delivery

Correct. The refactor from _argv_authorized_data_root into a generic _argv_flag_value at line 120 is clean and makes _argv_authorized_data_root a thin wrapper with no behavior change. The precedence logic in resolve_disk_hygiene_enabled() at line 154 correctly handles all four cases:

Input	Result
argv "false" (any case/whitespace)	enabled=False
argv is the literal unsubstituted placeholder	falls back to env
argv is empty string	falls back to env (falsy guard)
No channel	defaults to "true" → enabled=True (fail-safe, guard active)

The --flag value and --flag=value spellings are both handled at lines 129–133. The absent-flag case (returns None, then if from_argv is falsy → env fallback) is also correct.

SKILL.md frontmatter at line 13 passes --disk-hygiene-enabled ${user_config.disk_hygiene_enabled} in exec-form args — this is substituted as a single OS-level argv token with no shell parsing, mirroring the --authorized-data-root precedent from #742.

---

Prior suggestion classifications

Both suggestions from the automated security review pass are correctly classified:

"Only literal false disables" — Non-issue. The disk_hygiene_enabled userConfig is typed "boolean" in plugin.json line 21, so the harness can only emit the canonical string "false" or "true". Non-boolean spellings ("0", "no") are not a reachable input and the fail-safe direction (unexpected → enabled=True, guard stays active) is correct. Classification valid.

"Test helpers don't mock sys.argv" — Deferred to #970. run_guard_disabled and run_guard_powershell_disabled rely on the test runner's argv not containing --disk-hygiene-enabled, which holds for all standard runners. The new run_guard_enabled_argv helper (which patches both sys.argv and os.environ) demonstrates the robust pattern and already covers the B2+D3 acceptance tests. Deferral is reasonable.

---

Test coverage

The six new tests accurately target the exact failure scenarios:

Test	What it verifies
test_powershell_deletion_spellings_denied_in_audit_only_mode	B2: all 5 PS spellings → deny when kill switch is off via env fallback
test_kill_switch_blocks_every_lane_via_argv_without_env	B2+D3 acceptance: false via argv only, env var UNSET → both PS and Bash lanes deny
test_kill_switch_enabled_via_argv_without_env_still_gates	argv true without env → ask, confirming argv value drives the decision
test_resolve_disk_hygiene_enabled_precedence	Full precedence matrix including case/whitespace, placeholder, empty, no-channel
test_argv_flag_value_parses_both_arg_spellings	Both --flag value and --flag=value spellings; absent → None
test_skill_hook_passes_disk_hygiene_enabled_flag_matching_constant	Seam lock between frontmatter args and guard constant; prevents silent rename regression

The pre-existing test_powershell_deletion_spellings_force_final_prompt (enabled → ask) still passing means the enabled path is demonstrably unchanged.

---

Security

No new attack surface:

• Exec-form hook substitution prevents argv injection through the config value.
• Fail-safe default preserved: unconfigured → enabled=True → every mutation still behind the final human prompt.
• Kill switch now enforced across all four lanes: Bash scan/preview (allow), Bash apply (ask/deny), PS mutation spellings (ask/deny), PS .NET Delete (ask/deny).

[claude]

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

---

Security Review — PR #968

• Read PR diff
• Analyze destructive_guard.py (core logic changes)
• Analyze SKILL.md (hook argv injection surface)
• Analyze supporting changed files
• Post security findings

---

Scope: destructive_guard.py, test_hygiene.py, SKILL.md, safety-model.md, plugin.json, CHANGELOG.md

Result: No CRITICAL or IMPORTANT security vulnerabilities found. The two fixes are correctly implemented, the fail-safe default is preserved, and the argv-injection surface is not exploitable. Two SUGGESTION-level observations below.

---

Core fix analysis

B2 — PowerShell kill switch gate. enabled is now resolved unconditionally before the tool_name branch (L377), so powershell_decision(command, enabled) receives the correct value regardless of which tool path is taken. _powershell_mutation_verdict maps enabled=False → deny, enabled=True → ask. Engine calls ("hygiene.py" in command.casefold()) remain hard-denied regardless of enabled — correct.

D3 — argv channel. resolve_disk_hygiene_enabled() at L154–L170 follows the exact pattern of resolve_authorized_data_root(). The fail-safe chain is sound:

1. argv "false" → disabled
2. argv absent / placeholder / empty → env var
3. env var "false" → disabled
4. env var absent → default "true" → enabled (guard active)

Argv injection surface. The hook is exec-form (no shell), so ${user_config.disk_hygiene_enabled} is substituted as a single OS-level token before the process is launched. A config value containing spaces (e.g., "false --disk-hygiene-enabled true") is delivered as one token and cannot split into multiple argv entries. _argv_flag_value matches on exact token equality (token == flag) — no injection.

_argv_flag_value generalization. The refactor from _argv_authorized_data_root to the generic helper at L120 is a pure extraction — behavior on the data-root path is unchanged and both code paths remain covered by the existing test suite.

---

SUGGESTION — Confidence: High

Audit-only mode (enabled=False) still allows unknown PowerShell mutation spellings

destructive_guard.py L379–L383:

if tool_name == "PowerShell":
    verdict = powershell_decision(command, enabled)
    if verdict:
        print(json.dumps(decision(*verdict)))
    return 0

When powershell_decision returns None (no regex match), the guard exits without printing, which Claude Code treats as non-blocking — the command proceeds. This path is taken regardless of enabled. A PowerShell deletion command not in _POWERSHELL_MUTATION_WORDS (e.g., Invoke-Expression "Remove-Item foo", Start-Process cmd -ArgumentList '/c del foo', or a PS alias not in the wordlist) therefore bypasses the kill switch even in audit-only mode.

This is pre-existing behavior, not introduced by this PR, and is documented in safety-model.md as "a raised bar, not fail-closed." The PR correctly applies enabled to all known spellings. Noting it here because the PR description states the kill switch "blocks deletions on every lane" — that claim holds only for the enumerated regex set; a wordlist bypass is possible. No code change required; the caveat should be understood by security reviewers.

---

SUGGESTION — Confidence: Medium

Only the literal string "false" (case-insensitive, trimmed) disables the kill switch; other false-y forms are silently treated as enabled

destructive_guard.py L169–L170:

return from_argv.strip().lower() != "false"
return os.environ.get(_DISK_HYGIENE_ENABLED_ENV, "true").strip().lower() != "false"

Values "0", "no", "off", "disabled", or a YAML-boolean false serialized differently are all treated as enabled=True. The fail-safe direction is correct (unexpected → guard active), but an administrator who sets disk_hygiene_enabled: no would find audit-only mode silently inoperative. The userConfig schema declares the type as boolean so canonical serialization produces "true"/"false" — this is an inherent assumption that cannot be verified without the live harness. This finding and its risk are unchanged from the pre-existing env-var path; the PR explicitly documents it as an inherited caveat.

---