test(disk-hygiene): mock sys.argv in run_guard* helpers to seal the kill-switch argv seam

[kyle-sexton]

Summary

resolve_disk_hygiene_enabled() reads --disk-hygiene-enabled from sys.argv[1:] before the environment fallback (introduced in #382). Three helpers in plugins/disk-hygiene/skills/clean/scripts/test_hygiene.py — run_guard, run_guard_disabled, and run_guard_powershell_disabled — patched os.environ to drive the kill switch but left sys.argv unpatched. If a test runner's real invocation argv happened to carry --disk-hygiene-enabled <value>, it would silently override the env-var mock and a run_guard_powershell_disabled-based test would observe ask where deny is expected.

This is test-isolation fragility, not a live failure: standard unittest/pytest invocations never produce such argv, so CI is unaffected today. The fix seals the latent seam.

Change

Each of the three helpers now patches guard.sys.argv to a clean, flag-free argv ([str(SCRIPT_DIR / "destructive_guard.py")]) alongside its existing os.environ mock — the exact pattern the newer run_guard_enabled_argv helper already established. With no argv flag present, resolve_disk_hygiene_enabled() falls through to the environment mock, keeping the env var the sole channel under test. No production logic is touched; the underlying #968 fix is correct and security-reviewed clean.

Version bumped 0.4.4 → 0.4.5 (patch — pure test isolation, no behavior change) with a matching CHANGELOG.md entry.

Verification

• python -m unittest -v test_hygiene.py — 81 passed, 4 skipped (platform gates).
• ruff check on the touched file — All checks passed.
• markdownlint-cli2 on CHANGELOG.md — 0 errors.
• check-changelog-parity.sh --check and --check-bump origin/main — pass.
• scripts/validate-plugins.sh — manifests + catalog validated.

Note (out of scope, follow-up candidate)

run_guard_powershell (the enabled PowerShell helper) carries the identical env-only seam but is not named in #970, so it is deliberately left untouched here to keep this PR atomic to the issue's stated scope. Worth a follow-up to seal for full consistency.

Related

• fix(disk-hygiene): enforce disk_hygiene_enabled kill switch on PowerShell lane + via hook argv #968 — the security-review SUGGESTION this PR resolves.
• disk-hygiene: disk_hygiene_enabled kill switch does not gate the PowerShell lane and is inert under the env-injection failure #382 — the argv-reading precedence in resolve_disk_hygiene_enabled() that this test change seals against.

Closes #970

---

🤖 Generated with Claude Code

[claude]

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

---

PR Review

• Gather context and understand the request
• Read changed files
• Review code for correctness, security, and alignment with project guidelines
• Post review findings

---

Summary

This PR correctly seals a latent test-isolation seam where three run_guard* helpers in test_hygiene.py patched os.environ to drive the kill switch but left sys.argv unpatched. Since resolve_disk_hygiene_enabled() reads --disk-hygiene-enabled from sys.argv[1:] before the environment fallback, a stray argv flag from the test runner could silently override the env-var mock. The fix is minimal, correct, and consistent with the established pattern.

---

Correctness

The three new patches use mock.patch.object(guard.sys, "argv", [str(SCRIPT_DIR / "destructive_guard.py")]) — targeting guard.sys rather than sys globally. This is the right form: it patches the argv attribute on the exact sys module object the guard module uses, regardless of import order, and is the same pattern already established in run_guard_enabled_argv (line 1117) and run_guard_hook_argv (line 1408). No concerns here.

The clean argv [str(SCRIPT_DIR / "destructive_guard.py")] correctly leaves argv[1:] empty, so resolve_disk_hygiene_enabled() finds no flag and falls through to the env-var mock — exactly the intended isolation.

---

Known gap: run_guard_powershell still unpatched

run_guard_powershell carries the identical env-only seam and is deliberately left out of scope per the PR description. This is a reasonable call for atomicity. Worth noting for the follow-up: run_guard_powershell sets disk_hygiene_enabled=true in its env mock, so a stray --disk-hygiene-enabled false in a test runner's argv would flip a PowerShell deletion test's expected ask to deny. That's a softer failure mode than the _disabled path (the test would still pass with the wrong decision, just in the wrong direction), but it's the same structural fragility.

---

CHANGELOG and version

The 0.4.4 → 0.4.5 patch bump is appropriate for a pure test-isolation change with no runtime behavior change. The CHANGELOG entry under ### Changed (rather than ### Fixed) is defensible — there's no production bug to fix. The entry is accurate and clearly communicates the latent-not-live nature of the issue.

---

Minor observations (non-blocking)

• The mock.patch.dict("os.environ", ..., clear=False) in run_guard and run_guard_disabled means any real CLAUDE_PLUGIN_OPTION_DISK_HYGIENE_ENABLED already in the test process environment would bleed through. This is a pre-existing condition unrelated to this PR and does not affect the fix's validity, but it's worth noting as a difference from the clear=True pattern used in run_guard_enabled_argv and run_guard_hook_argv.

---

Verdict

Approve. The fix is correct, minimal, follows the established mock pattern, touches no production logic, and is well-documented. The only open item is the acknowledged run_guard_powershell follow-up.

[claude]

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

---

Security Review — PR #971

• Read changed files
• Analyze for security issues
• Post findings

---

No security issues found.

The three changed files are:

File	Change	Security verdict
plugin.json	Version 0.4.4 → 0.4.5	No implications
CHANGELOG.md	Documentation entry	No implications
test_hygiene.py	Add mock.patch.object(guard.sys, "argv", …) to three helpers	Clean — see below

Test-file analysis

The only substantive code change is adding one line to each of three test helpers in test_hygiene.py:

mock.patch.object(guard.sys, "argv", [str(SCRIPT_DIR / "destructive_guard.py")]),

• Scope — the patch is inside a with block; it only affects the test process for the lifetime of that context manager, then reverts. No production code path is touched.
• Value source — SCRIPT_DIR is Path(__file__).resolve().parent (line 19): a module-level constant derived from the test file's own location, not from any external or user-controlled input. No injection surface.
• No credentials or secrets — the patched argv contains only a file path to destructive_guard.py, which mirrors the pattern already used by the run_guard_enabled_argv helper that existed before this PR.
• No dependency or supply-chain changes — no new imports, no pinning changes, no action version changes.
• No GitHub Actions modifications — no workflow files touched.

The PR does exactly what it claims: it seals a latent test-isolation seam. The approach (patching guard.sys.argv via mock.patch.object rather than the bare sys.argv) is correct and consistent with the pre-existing run_guard_enabled_argv pattern.

[claude]

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

---

PR Review — #971

• Gather context and understand the request
• Read changed files and diff
• Review code for correctness, security, and alignment with guidelines
• Post review findings

---

Summary

This PR seals a latent test-isolation seam: resolve_disk_hygiene_enabled() reads --disk-hygiene-enabled from sys.argv[1:] before the env-var fallback, but three run_guard* helpers patched only os.environ, leaving sys.argv unguarded. The fix adds one mock.patch.object(guard.sys, "argv", …) line to each affected helper.

---

Correctness

All three additions follow the correct form — targeting guard.sys rather than the global sys:

• run_guard (line 1067)
• run_guard_disabled (line 1081)
• run_guard_powershell_disabled (line 1324)

Patching guard.sys (the sys module object the guard module imported) rather than the global sys is the right form — it's immune to import-order variation, and it's identical to the established pattern in run_guard_enabled_argv (line 1117) and run_guard_hook_argv (line 1408).

The patched argv [str(SCRIPT_DIR / "destructive_guard.py")] leaves argv[1:] empty, so resolve_disk_hygiene_enabled() finds no flag and falls through to the env-var mock — exactly the intended isolation.

---

Known gap: run_guard_powershell still unpatched

run_guard_powershell (lines 1298–1312) carries the same env-only seam. It is deliberately excluded per the PR's atomicity argument, which is reasonable. The failure mode is softer here (the env mock sets enabled=true; a stray --disk-hygiene-enabled false from a test runner would flip an expected ask to deny, causing a test to pass with the wrong decision rather than fail), but the structural fragility is identical. The follow-up is worth tracking.

---

Version and CHANGELOG

• 0.4.4 → 0.4.5 patch bump is appropriate for a pure test-isolation change with no runtime behavior change.
• CHANGELOG entry under ### Changed is accurate and defensible — there's no production bug being fixed, only test robustness improved.
• The entry's wording ("seals latent fragility rather than a live failure") correctly characterizes the risk level.

---

Pre-existing minor observation (non-blocking)

run_guard and run_guard_disabled use clear=False on their mock.patch.dict("os.environ", …). This means a real CLAUDE_PLUGIN_OPTION_DISK_HYGIENE_ENABLED already present in the test process environment would bleed through. This is a pre-existing condition unrelated to this PR and does not affect the fix's correctness, but it differs from the clear=True pattern used by run_guard_enabled_argv and run_guard_hook_argv.

---

Verdict

Approve. The fix is correct, minimal, touches no production logic, follows the established mock pattern, and is well-documented. The only open item is the acknowledged run_guard_powershell follow-up, which is deliberately out of scope.

[kyle-sexton]

Reviewed personally (rule 10): small, exactly matches the described fix, test-only, no production-logic drift. Independently confirmed live CI green (not just the worker's local run). Flipping to ready.

[claude]

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

---

Security Review — PR #971

• Gather context
• Read PR diff and changed files
• Analyze for security vulnerabilities
• Post findings

---

No security issues found.

The three changed files are:

File	Change	Security verdict
plugin.json	Version 0.4.4 → 0.4.5	No implications
CHANGELOG.md	Documentation entry	No implications
test_hygiene.py	Add mock.patch.object(guard.sys, "argv", …) to three helpers	Clean — see below

Test-file analysis

The only substantive code change adds one line to each of three test helpers:

mock.patch.object(guard.sys, "argv", [str(SCRIPT_DIR / "destructive_guard.py")]),

• Value source — SCRIPT_DIR is Path(__file__).resolve().parent (line 19 of the file): a module-level constant derived from the test file's own location, not from any external or user-controlled input. No injection surface.
• Scope — the patch lives inside a with block and reverts the moment the block exits. No persistent mutation of the process's sys.argv; no side-effects outside the test helper's lifetime.
• Target — guard.sys rather than the global sys is the correct and safe form: it patches the argv attribute on exactly the sys module object the guard module imported, with no broader blast radius.
• No credentials or secrets — the patched argv contains only a file path to destructive_guard.py, a known test script already present in the repo.
• No dependency or supply-chain changes — no new imports, no pinning changes, no action version bumps.
• No GitHub Actions modifications — no workflow files touched.

Pre-existing condition (not introduced by this PR)

run_guard_powershell retains the env-only seam (mock.patch.dict without a matching argv patch). The PR description deliberately defers this to a follow-up. This is a pre-existing latent isolation gap — not introduced here, not a security defect in production code, and carries no exploitability outside an adversarially-crafted test environment.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c2aeae81af

ℹ️ 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".