fix(disk-hygiene): guard resolves authorized data root from hook argv

[kyle-sexton]

Summary

The disk-hygiene clean skill declares destructive_guard.py as a PreToolUse
skill-frontmatter hook. The guard read its authorized data root only from the
CLAUDE_PLUGIN_DATA environment variable, but the runtime does not inject that
variable into a skill-frontmatter hook's process environment. As a result
--data-root never validated and the scan/preview/apply engine lane failed
closed on every guarded invocation — on all platforms — making the plugin's core
engine unreachable through its sanctioned Bash lane.

Fix

Root cause: the guard depended on environment-variable injection, but per the
plugins reference inline
placeholder substitution (${CLAUDE_PLUGIN_DATA}) resolves in hook arguments
"anywhere the placeholder appears," and the reference explicitly recommends exec
form with args so each path is passed as one argument. Inline substitution was
already proven to work in this exact frontmatter context (${CLAUDE_PLUGIN_ROOT}
resolves — the guard script path loads and executes); only environment injection
was missing.

• skills/clean/SKILL.md frontmatter hook now passes the root as a
  runtime-substituted argument: --authorized-data-root ${CLAUDE_PLUGIN_DATA}.
• destructive_guard.py resolves its authority from that hook argv
  (resolve_authorized_data_root), honoring CLAUDE_PLUGIN_DATA only as a
  fallback. The authority is resolved once in main() and threaded explicitly
  down the classification chain instead of being read from os.environ deep in
  the leaf validator.
• Doc sentences that asserted the false "the hook process received
  CLAUDE_PLUGIN_DATA" mechanism are corrected in SKILL.md and
  reference/safety-model.md to describe the runtime-substituted argument.

Security property is unchanged: the authority comes from the guard's own
process argv (harness-substituted, model-unforgeable), a channel distinct from
the model-supplied --data-root inside the gated Bash command; the latter is
validated against the former and can never become its own authority.

Verification

• python -m unittest test_hygiene — 71 passed, 4 skipped (Linux-only platform
  gates). Adds 4 tests: argv authorizes matching --data-root with
  CLAUDE_PLUGIN_DATA absent from the environment; mismatch denied; deny when
  neither argv nor env supplies authority; argv-over-env precedence with
  literal-placeholder fallback.
• Scripted end-to-end proof (real subprocess, real argv, CLAUDE_PLUGIN_DATA
  removed from the environment — the exact failing scenario):
  • with --authorized-data-root <root> → permissionDecision: allow
  • same command without the argv (old wiring) → permissionDecision: deny,
    citing the now-accurate "neither the --authorized-data-root hook argument
    nor CLAUDE_PLUGIN_DATA" guidance.
• scripts/check-changelog-parity.sh --check and --check-bump origin/main — pass
  (version bumped 0.4.0 → 0.4.1, ## [0.4.1] entry added).
• markdownlint-cli2 on the three changed markdown files — 0 errors.

Not covered by units: live-harness end-to-end substitution of
${CLAUDE_PLUGIN_DATA} in a skill-frontmatter hook's args. This rests on the
plugins-reference guarantee (same inline-substitution pass as
${CLAUDE_PLUGIN_ROOT}, which empirically resolves in this exact position) — it
cannot be exercised without the running harness.

Closes #376

Related

• Same env-injection root cause, deliberately not folded here (separately
  tracked): disk-hygiene: disk_hygiene_enabled kill switch does not gate the PowerShell lane and is inert under the env-injection failure #382 (the disk_hygiene_enabled kill switch reads
  CLAUDE_PLUGIN_OPTION_DISK_HYGIENE_ENABLED from the hook env, also absent —
  ~2 lines once this argv plumbing exists), disk-hygiene: doc corrections — Windows mislabeled 'full', false CLAUDE_PLUGIN_DATA premise repeated, PowerShell-lane gaps #386 (the broader doc sweep beyond
  the two sentences this change falsifies), disk-hygiene: setup:check reports healthy while the engine is dead (misses #376 env-injection and bare-python failures) #385 (setup:check liveness probe).
• Umbrella: convention: false-green checks — surfaces report success while the capability is dead or findings invisible (liveness-assertion umbrella) #532 (false-green / dead-capability convention). This is the engine-
  liveness instance; opened as a draft pending that umbrella's decision.

[claude]

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

---

PR Review

• Gather context and understand the request
• Read and diff the changed files
• Analyze destructive_guard.py for correctness and security
• Analyze test_hygiene.py for coverage
• Review SKILL.md and safety-model.md doc changes
• Review plugin.json and CHANGELOG.md
• Post final review

---

Summary

The root-cause diagnosis is correct, and the fix is well-reasoned. The CLAUDE_PLUGIN_DATA environment variable is not injected into skill-frontmatter hook processes, so the guard was resolving None for its authority and failing closed on every engine invocation. Passing the value as a harness-substituted argument (--authorized-data-root ${CLAUDE_PLUGIN_DATA}) is the right channel — it's the same inline-substitution pass that already resolves ${CLAUDE_PLUGIN_ROOT} in this exact position.

The trust model is preserved: the argv side is harness-substituted and model-unforgeable; the --data-root in the guarded Bash command is model-supplied and validated against it. These channels cannot contaminate each other.

The seam-lock test test_skill_hook_passes_authorized_data_root_flag_matching_constant is particularly well-designed — it parses SKILL.md's frontmatter args array and asserts both that _AUTHORIZED_DATA_ROOT_FLAG is present and that ${CLAUDE_PLUGIN_DATA} immediately follows it, so a rename on either side breaks the test loudly rather than silently failing closed.

---

Issues

1. Coverage gap — preview/apply with --data-root not exercised through main() via hook argv (medium)

test_guard_authorizes_data_root_from_hook_argv_without_env only exercises the scan shape through run_guard_hook_argv. The preview and apply shapes also accept optional --data-root (see _consume_optional_pairs for preview and L241-L244 for apply), but there is no integration path test through main() → classify_exact_engine_command → _consume_optional_pairs with the argv-based authority for those shapes. The existing test_guard_preview_accepts_optional_authorized_data_root (line 1470) uses run_guard which falls back to os.environ, not run_guard_hook_argv. In the real hook wiring, those shapes are also guarded by the same argv authority path.

Fix this →

2. test_guard_denies_data_root_without_hook_authority doesn't mock sys.argv (low)

Line 1301-1325 removes CLAUDE_PLUGIN_DATA from the environment but relies on the test runner's real sys.argv not containing --authorized-data-root. This works in practice, but the newer run_guard_hook_argv(None) pattern in test_guard_denies_data_root_when_argv_and_env_both_absent is strictly more correct — it explicitly mocks argv to exclude the flag. The pre-existing test is fine but worth updating for consistency and robustness if the test helpers are being touched.

3. is_exact_engine_apply signature changed but function has no callers (low)

Line 250-251 changes is_exact_engine_apply from (command: str) -> bool to (command: str, authority: str | None) -> bool. It has no callers in the repo and is not exercised by any test directly. If it's intended as part of the importable API surface (e.g. for future consumer use), the signature change is a breaking one and worth documenting in the changelog. If it's dead code, it could be removed.

4. resolve_authorized_data_root reads sys.argv directly (style / low)

Line 131-140: resolve_authorized_data_root() calls _argv_authorized_data_root(sys.argv[1:]) internally. _argv_authorized_data_root already accepts argv: list[str] cleanly; the outer function could follow the same pattern (def resolve_authorized_data_root(argv: list[str] | None = None, env: ...)). Tests currently mock guard.sys.argv to get at it, which works but is heavier than passing a list. Not a correctness issue.

---

Nits

• The "${" not in from_argv guard in resolve_authorized_data_root is a good defensive move. Worth noting it would also correctly fall through if the runtime substitutes the placeholder to an empty string (the if from_argv branch is False), so the fail-closed posture holds for that edge case too.

• _bash_denial_guidance correctly embeds _AUTHORIZED_DATA_ROOT_FLAG instead of a hardcoded string literal in the denial message — that keeps the constant as the single source of truth.

• CHANGELOG entry is clear and accurate; patch version bump is appropriate for a bug fix.

---

Verdict

Approve with the coverage gap noted. The fix is correct, the security property is preserved, and the seam-lock test is a strong guard against regression. The main ask is adding run_guard_hook_argv-based tests for the preview and apply shapes with --data-root (finding #1) — those shapes are live in the real hook wiring and should be covered the same way scan now is.

[claude]

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

---

PR Review

• Gather context and understand the request
• Review the diff (origin/main...HEAD) — focusing on commit 53ee8a3
• Analyze new test coverage for preview/apply shapes via hook argv
• Verify previous review findings are addressed
• Post final review

---

Summary

Commit 53ee8a3 directly resolves finding #1 from the prior review. test_guard_authorizes_preview_data_root_from_hook_argv_without_env and test_guard_authorizes_apply_data_root_from_hook_argv_without_env both use run_guard_hook_argv — the same in-process driver that strips CLAUDE_PLUGIN_DATA from the environment and injects --authorized-data-root via mocked argv — so the missing main() → classify_exact_engine_command → _consume_optional_pairs coverage for those two shapes is now present.

A few observations on the new tests:

Apply correctly expects "ask", not "allow" — line 1411–1413 checks for "ask" on the authorized path, mirroring the main() branch at line 353–362. The approval token "a" * 24 satisfies the [0-9a-f]{24} regex. Both choices are correct.

run_guard_hook_argv is well-placed — it's on GuardTests as a helper method, not a free function, so it stays co-located with the integration tests that need it. The clear=True on mock.patch.dict is essential: it ensures the entire environment is replaced, not just the keyed entries.

---

Status of prior findings

#	Severity	Status
1 — preview/apply not exercised via hook argv	medium	Fixed by commit 53ee8a3
2 — test_guard_denies_data_root_without_hook_authority uses real sys.argv	low	Not addressed
3 — is_exact_engine_apply signature change, no callers or direct tests	low	Not addressed
4 — resolve_authorized_data_root reads sys.argv directly	style/low	Not addressed

Findings 2–4 were all low/style in the prior review and are still accurate. No regression introduced by this commit. The one residual item worth noting:

is_exact_engine_apply has no callers — line 250 changed signature from (command: str) -> bool to (command: str, authority: str | None) -> bool. It remains uncalled and untested directly. If this is intentionally exported API, a test_is_exact_engine_apply_* pair plus a CHANGELOG note would clarify the contract. If it's dead code it can be removed.

Fix this →

---

Verdict

Approve. The primary medium finding from the prior review is fully resolved. The implementation is correct, the security property is intact, and the test structure matches the real hook wiring. Remaining items are all low/style and do not block merge.

[claude]

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

---

Security Review

• Gather context and understand the request
• Read PR diff and changed files
• Analyze destructive_guard.py for security issues
• Analyze SKILL.md hook wiring for injection surfaces
• Review authority resolution trust model
• Post findings

---

Overview

The PR changes one security-critical function: how destructive_guard.py resolves its authority (the authorized data root it validates model-supplied --data-root values against). The old code read this from os.environ["CLAUDE_PLUGIN_DATA"]; the new code reads it from the guard's own sys.argv, injected as a harness-substituted placeholder in the skill frontmatter, with env as a fallback.

The core trust model claim is: the guard's own process argv is a model-unforgeable channel, because it is set by the harness from the skill-frontmatter args definition before the process starts. The model controls the Bash command delivered via stdin; it cannot reach the guard's sys.argv.

This claim is correct. The two channels are genuinely disjoint:

• sys.argv → set by harness at process spawn, from frontmatter args: ["...", "--authorized-data-root", "${CLAUDE_PLUGIN_DATA}"]
• sys.stdin → the JSON hook payload containing the model-authored Bash command

There is no code path by which a value in tool_input.command (stdin) could influence sys.argv. _argv_authorized_data_root never reads stdin; classify_exact_engine_command never reads argv. The fix is architecturally sound.

---

Findings

SUGGESTION — PowerShell engine-detection relies on filename string, not script identity

Confidence: low

destructive_guard.py L273:

if "hygiene.py" in command.casefold():

The PowerShell guard detects engine invocations by checking whether the string hygiene.py appears in the command. This is a filename-string check, not an identity check. A symlink to hygiene.py under a different name (e.g. /tmp/engine) would not be caught by this branch and would fall through to the mutation-word scan.

Mitigations that limit this in practice: (a) the model cannot create a symlink without a Bash command, which goes through the fail-closed exact-match lane; (b) the Bash lane is the only authoritative engine lane — PowerShell engine invocations cannot produce a successful apply result because the apply gate requires Linux-specific O_NOFOLLOW/descriptor semantics that don't exist on Windows (where PowerShell is typical); (c) even if somehow invoked via PowerShell, the engine script itself validates all inputs independently. This is a defense-in-depth gap, not a direct bypass of any destructive operation.

No action required before merge; tracking as a future hardening opportunity.

---

Pre-existing issue (not introduced by this PR) — kill-switch always reads as enabled in hook context

destructive_guard.py L342:

enabled = os.environ.get("CLAUDE_PLUGIN_OPTION_DISK_HYGIENE_ENABLED", "true").lower() != "false"

CLAUDE_PLUGIN_OPTION_DISK_HYGIENE_ENABLED is read from the hook process environment. Per the PR description (and issue #382), the hook process does not receive plugin option env vars via injection — so this variable is absent and the default "true" always wins. A consumer who sets disk_hygiene_enabled: false in userConfig cannot currently disable apply through this code path.

This is acknowledged and tracked separately. It pre-dates this PR; the diff here touches main() but does not change the enabled line. Worth flagging because the PR modifies main(), but it is not a regression introduced here.

---

What was checked

Area	Result
Authority channel separation (argv vs stdin)	Sound — no cross-contamination path exists
${ guard on unsubstituted placeholder	Correct — falls through to env; fails closed if neither is set
~ in model-supplied --data-root	Blocked — ~ is in _SHELL_EXPANSION_OR_OPERATOR_CHARS; parser rejects it
Path canonicalization in _data_root_key	Correct — expanduser + resolve(strict=False) + normcase on both sides
_is_current_python check	Correct — requires both normcase equality and samefile identity; symlink aliases are rejected
_script_path_key for hygiene.py	Correct — resolve(strict=True) requires real file; canonical path comparison
JSON payload parsing from stdin	Fine — model controls command; harness controls delivery format
GitHub Actions files	Not changed in this PR
Approval-token regex [0-9a-f]{24}	Correct — only hex, exact length
PowerShell mutation-word patterns	Not changed; no new bypass surface

Verdict: no blocking security issues. The argv-authority channel is architecturally correct and model-unforgeable as claimed. The one SUGGESTION (PowerShell filename-string check) is pre-existing defense-in-depth rather than a new vulnerability.

[kyle-sexton]

Post-green review pass complete (rule 8). Security review (this repo's new security-review gate): no blocking issues. Authority-channel separation (argv vs stdin) confirmed architecturally sound and model-unforgeable, unsubstituted-placeholder fallback confirmed fail-closed, path canonicalization/symlink checks all correct. One low-confidence SUGGESTION (PowerShell filename-string detection, pre-existing defense-in-depth gap, not a new vuln) — tracked, not fixed here. One pre-existing unrelated issue noted (kill-switch env-injection gap, already tracked as #382, not touched by this diff).

No threads to resolve, CI fully green (17/17).

Flipping to ready for review; handing off to the babysit lane for merge.

[claude]

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

---

PR Review

• Gather context and understand the request
• Read the full diff (origin/main...HEAD)
• Review destructive_guard.py — argv authority resolution, trust model, edge cases
• Review test_hygiene.py — coverage completeness for all 6 new tests across both commits
• Review SKILL.md and safety-model.md doc changes
• Review plugin.json and CHANGELOG.md
• Verify prior review findings are fully addressed
• Post final review

---

Summary

The fix is architecturally correct and all three previous reviews' findings are in the expected state. The two commits together are mergeable.

What the fix does: resolve_authorized_data_root() now reads the authorized data root from the guard's own sys.argv (via _argv_authorized_data_root), falling back to os.environ.get("CLAUDE_PLUGIN_DATA") only when argv doesn't provide it or when the placeholder is unsubstituted. The SKILL.md hook wiring adds "--authorized-data-root", "${CLAUDE_PLUGIN_DATA}" to the guard's args array, which the plugins reference guarantees will be substituted inline — the same pass that resolves ${CLAUDE_PLUGIN_ROOT} in that same position.

Trust model: The two channels are genuinely disjoint. sys.argv is set by the harness at process spawn from the frontmatter args definition; sys.stdin carries the model-authored Bash command JSON payload. There is no code path by which a value in tool_input.command can influence sys.argv. The authority the guard validates against is model-unforgeable.

---

Detailed findings

_argv_authorized_data_root — correct. Handles both space-separated (--authorized-data-root VALUE) and equals-form (--authorized-data-root=VALUE) variants. Handles the edge case where the flag is the last token with no value (--authorized-data-root at argv end → None).

resolve_authorized_data_root — the "${" not in from_argv guard is the right defensive check. If the runtime fails to substitute the placeholder, from_argv would be "${CLAUDE_PLUGIN_DATA}", which contains "${", so the condition is False and the env fallback applies. If the runtime substitutes to an empty string, if from_argv is False and the env fallback applies. Both edges fail closed correctly.

run_guard_hook_argv test helper — well-constructed. mock.patch.dict("os.environ", environment, clear=True) replaces the entire process environment, not just the specified keys, so CLAUDE_PLUGIN_DATA cannot leak in from the outer test runner. mock.patch.object(guard.sys, "argv", argv) patches the sys reference the module actually holds, which is the correct approach (vs. mock.patch("sys.argv") which would also work but is less precise). CLAUDE_PLUGIN_OPTION_DISK_HYGIENE_ENABLED is set to "true" explicitly so the enabled check in main() doesn't gate the apply result.

Scan/preview/apply shape coverage — complete. All three engine shapes are now exercised via the hook-argv path (no CLAUDE_PLUGIN_DATA in env, authority via argv only): scan → allow, preview → allow, apply authorized → ask, apply unauthorized → deny. The "ask" expectation on the authorized apply path is correct — main() returns "ask" when command_kind == "apply" and enabled.

Seam-lock test — well-designed. test_skill_hook_passes_authorized_data_root_flag_matching_constant parses the SKILL.md frontmatter args JSON inline array, confirms _AUTHORIZED_DATA_ROOT_FLAG appears, and asserts "${CLAUDE_PLUGIN_DATA}" is the immediately following element. A rename of the constant on either side fails the test loudly rather than silently losing authority.

_bash_denial_guidance — the absence message now references _AUTHORIZED_DATA_ROOT_FLAG as a variable rather than a string literal, keeping the constant as the single source of truth. test_guard_denies_data_root_when_argv_and_env_both_absent checks that the denial reason contains "--authorized-data-root", which remains true after any future constant rename only if the test is updated together — the two are appropriately coupled.

---

Status of all prior findings

#	Severity	Status
1 — preview/apply not exercised via hook argv	medium	Fixed in commit 53ee8a3
2 — test_guard_denies_data_root_without_hook_authority uses real sys.argv	low	Not addressed (still accurate, no regression)
3 — is_exact_engine_apply signature changed, no callers or tests	low	Not addressed (still accurate, no regression)
4 — resolve_authorized_data_root reads sys.argv directly	style/low	Not addressed (still accurate, no regression)

Finding #3 note: is_exact_engine_apply at L250 now accepts authority: str | None but has no callers and no direct test. If it is intended as a public API, the signature change is a breaking one (no test will catch the incompatibility) — worth a test_is_exact_engine_apply_* pair and a CHANGELOG line if this is ever exported. If it's dead code, removal is cleaner. Neither path blocks merge.

Fix finding #3 →

---

Verdict

Approve. Fix is correct, security property is intact, test coverage for all engine shapes via the hook-argv path is present, and the seam-lock test is a strong regression guard. All prior findings are in the expected state — the one medium item (#1) is fully resolved; the remaining items are all low/style and do not block merge.

[claude]

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

---

Security Review

• Gather context and understand the request
• Read PR diff (origin/main...HEAD)
• Analyze destructive_guard.py for security issues
• Analyze SKILL.md hook wiring for injection surfaces
• Review authority resolution trust model
• Check for dangerous patterns in changed files
• Post final findings

---

Overview

This PR changes one security-critical mechanism: how destructive_guard.py resolves its authority (the authorized data root against which model-supplied --data-root values are validated). The old code read CLAUDE_PLUGIN_DATA from os.environ; the new code reads it from the guard's own sys.argv as a harness-substituted placeholder, with env as a fallback.

The core trust model claim is: the guard's sys.argv is a model-unforgeable channel — it is set by the harness from the skill-frontmatter args definition before the process starts. The model controls tool_input.command delivered via sys.stdin; it cannot reach sys.argv.

This claim is correct. The two channels are genuinely disjoint. No code path in the guard causes stdin content to influence sys.argv. _argv_authorized_data_root never reads stdin; classify_exact_engine_command never reads argv. The fix is architecturally sound.

---

Findings

No CRITICAL or IMPORTANT security issues were found in this diff.

---

SUGGESTION — resolve_authorized_data_root reads sys.argv directly (confidence: medium, impact: low)

destructive_guard.py L131-L140

def resolve_authorized_data_root() -> str | None:
    from_argv = _argv_authorized_data_root(sys.argv[1:])
    if from_argv and "${" not in from_argv:
        return from_argv
    return os.environ.get("CLAUDE_PLUGIN_DATA")

_argv_authorized_data_root already accepts argv: list[str] as a parameter. resolve_authorized_data_root could follow the same pattern (def resolve_authorized_data_root(argv: list[str] | None = None, env: Mapping | None = None)) so that both sources are injectable without reaching into the process globals. The current design requires tests to mock guard.sys.argv (heavier) and means any future caller has no way to pass an alternate argv slice without monkey-patching.

This is a style/testability point, not an exploitable vulnerability — the production call site is main(), which reads the correct sys.argv.

---

SUGGESTION — ${ guard covers the harness placeholder form but not $VARNAME (confidence: low, impact: low)

destructive_guard.py L138

if from_argv and "${" not in from_argv:
    return from_argv

The check guards against the literal ${CLAUDE_PLUGIN_DATA} placeholder (brace form) being passed un-substituted. It would not catch $CLAUDE_PLUGIN_DATA (bare $VAR form without braces). Since the SKILL.md frontmatter uses ${CLAUDE_PLUGIN_DATA}, this is sufficient for the current harness. No action needed unless the harness ever emits the un-braced form.

---

Pre-existing (not introduced here) — kill switch always enabled in hook context

destructive_guard.py L342

enabled = os.environ.get("CLAUDE_PLUGIN_OPTION_DISK_HYGIENE_ENABLED", "true").lower() != "false"

Hook processes do not receive injected plugin option env vars, so this variable is always absent and enabled is always True. A consumer who configures disk_hygiene_enabled: false in userConfig believing it prevents the apply step from reaching the human-confirmation prompt will be wrong — the guard still emits "ask". The human-in-the-loop "ask" gate is still required, so no silent deletion occurs, but the kill switch is non-functional as a gate. Tracked as #382; pre-dates this PR and is not changed here.

---

What was checked

Area	Result
Authority channel separation (sys.argv vs sys.stdin)	Sound — no cross-contamination path
${ guard for unsubstituted placeholder	Correct; if from_argv also catches empty string
_data_root_key canonicalization (expanduser + resolve)	Both sides resolved equally; equality check prevents prefix bypass
_literal_shell_words metacharacter rejection	Blocks ~, $, {, ;, `
_is_current_python samefile + normcase identity check	Correct; symlink aliases and bare python rejected
_script_path_key resolve(strict=True)	Correct; non-existent path returns None and fails closed
Approval-token regex [0-9a-f]{24}	Unchanged; correct
is_exact_engine_apply signature change, no callers	Dead code concern (tracked in prior review), not a security issue
JSON payload parsing from sys.stdin	Standard json.load; command validated as str
SKILL.md frontmatter args array form	Correct two-element split: harness delivers --authorized-data-root and <value> as separate argv items
GitHub Actions / workflow files	Not changed in this PR

Verdict: no blocking security issues. The argv-authority channel is architecturally correct and model-unforgeable as claimed. Both SUGGESTIONS are low-impact style points that do not affect the security property. The one pre-existing concern (kill switch env injection, #382) is noted but pre-dates this diff.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 53ee8a3506

ℹ️ 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 2m 53s —— View job

---

Security Review

• Gather context and understand the request
• Read PR diff (origin/main...HEAD) — all 3 commits
• Analyze destructive_guard.py — placeholder-check fix, authority resolution, injection surfaces
• Analyze SKILL.md hook wiring and test_hygiene.py for security-relevant changes
• Post final security findings

---

Overview

This pass covers the full PR (commits 94a2127 → 53ee8a3 → b7e048a), with emphasis on b7e048a — the latest commit that fixes the Codex-identified issue where "${" not in from_argv rejected any valid authority path that merely contained the substring ${.

The core trust model claim is unchanged from prior reviews: the guard's sys.argv is a model-unforgeable channel. The model's input arrives in sys.stdin as a JSON payload; there is no code path by which tool_input.command content can influence sys.argv. The two channels are genuinely disjoint and that property is unaffected by the changes in this PR.

---

Finding: No CRITICAL or IMPORTANT security issues

---

Analysis of the b7e048a change specifically

_AUTHORIZED_DATA_ROOT_PLACEHOLDER construction:

_CLAUDE_PLUGIN_DATA_ENV = "CLAUDE_PLUGIN_DATA"
_AUTHORIZED_DATA_ROOT_PLACEHOLDER = f"${{{_CLAUDE_PLUGIN_DATA_ENV}}}"

The triple-brace f-string is correct: {{ → literal {, }} → literal }, giving "${CLAUDE_PLUGIN_DATA}". The exact-equality check in resolve_authorized_data_root correctly distinguishes the literal unsubstituted placeholder from a real data-root path that happens to contain ${ characters.

Fail-closed posture with ${}-bearing authority paths:

The commit message's reasoning is correct. For an authority path like /data/${weird}/root:

• _data_root_key("/data/${weird}/root") = Path(...).expanduser().resolve(strict=False) → Python does not perform shell ${} expansion; ${weird} stays literal
• The model's --data-root goes through _literal_shell_words first, which rejects any command containing $, {, or } (all in _SHELL_EXPANSION_OR_OPERATOR_CHARS)
• Therefore a model-supplied --data-root can never produce a path containing ${, and a ${}-bearing authority can never be matched by the model

Fail-closed posture is intact for this edge case.

Empty-string substitution edge:

If the runtime substitutes ${CLAUDE_PLUGIN_DATA} to "", from_argv is falsy → if from_argv fails → falls through to env. Correct.

--authorized-data-root=VALUE equals-form:

token[len(prefix):] returns "" when the value after = is empty → falsy → env fallback. Correct.

---

Pre-existing items (not introduced by this PR, unchanged)

Area	Status
Kill switch (CLAUDE_PLUGIN_OPTION_DISK_HYGIENE_ENABLED) always enabled in hook context	Pre-existing; tracked as #382
PowerShell filename-string detection ("hygiene.py" in command.casefold()) won't catch symlink aliases	Pre-existing defense-in-depth gap; noted in prior security review
is_exact_engine_apply signature changed, no callers or direct tests	Noted in prior code review; no security consequence

---

What was checked

Area	Result
sys.argv vs sys.stdin channel separation	Sound — no cross-contamination path
_AUTHORIZED_DATA_ROOT_PLACEHOLDER f-string correctness	Correct — evaluates to "${CLAUDE_PLUGIN_DATA}"
Exact-equality vs substring check for placeholder	Correct fix — only the literal unsubstituted token is rejected
Fail-closed with ${}-bearing authority path	Holds — model channel rejects $/{/} via _SHELL_EXPANSION_OR_OPERATOR_CHARS
Fail-closed with empty-string substitution	Holds — if from_argv is False for ""
_data_root_key expanduser on model-supplied values	Safe — ~ rejected upstream by _literal_shell_words
_bash_denial_guidance output	No injection risk — result is JSON-serialized by json.dumps
SKILL.md args array wiring	Correct two-element split: "--authorized-data-root" and "${CLAUDE_PLUGIN_DATA}" as separate entries
GitHub Actions / workflow files	Not changed in this PR

Verdict: no blocking security issues. The placeholder-check fix is correct, the fail-closed posture is preserved, and the authority channel remains model-unforgeable.

[claude]

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

---

PR Review

• Gather context and understand the request
• Read the new commit diff (b7e048a) — Codex finding fix
• Analyze destructive_guard.py — placeholder-token check change
• Analyze test_hygiene.py — coverage for ${-bearing path edge case
• Verify prior findings status
• Post final review

---

Summary

Commit b7e048a resolves the Codex finding kyle-sexton classified as VALID in the inline review thread: resolve_authorized_data_root was discarding any argv authority whose path merely contained the substring ${, not just the exact unsubstituted ${CLAUDE_PLUGIN_DATA} token. The fix is correct, minimal, and the fail-closed posture is fully preserved.

---

What changed

destructive_guard.py L111–113:

_CLAUDE_PLUGIN_DATA_ENV = "CLAUDE_PLUGIN_DATA"
_AUTHORIZED_DATA_ROOT_PLACEHOLDER = f"${{{_CLAUDE_PLUGIN_DATA_ENV}}}"

Two new module-level constants make the env-var name and the exact placeholder token single sources of truth. _AUTHORIZED_DATA_ROOT_PLACEHOLDER is derived from _CLAUDE_PLUGIN_DATA_ENV, so if the env var name ever changes, the placeholder and the os.environ.get() call update together.

destructive_guard.py L140, L142:

# before
if from_argv and "${" not in from_argv:
    return from_argv
return os.environ.get("CLAUDE_PLUGIN_DATA")

# after
if from_argv and from_argv != _AUTHORIZED_DATA_ROOT_PLACEHOLDER:
    return from_argv
return os.environ.get(_CLAUDE_PLUGIN_DATA_ENV)

The exact-equality check is strictly more precise than the substring check. A path like /home/user/.data/${project}/root no longer incorrectly falls through to the env fallback.

Security preservation: The fail-closed posture for the model channel is unchanged. _SHELL_EXPANSION_OR_OPERATOR_CHARS (L13) includes $, {, and } — model-supplied --data-root values containing those characters are rejected by _literal_shell_words before they reach _is_authorized_data_root. Path(...).expanduser().resolve() does not interpret ${VAR} syntax, so a ${}-bearing authority path cannot expand unexpectedly. A model-supplied value can never match a ${}-bearing authority path.

---

Test coverage

test_hygiene.py L1469–1479 — added inside the existing test_resolve_authorized_data_root_precedence block:

with mock.patch.object(
    guard.sys,
    "argv",
    [script, "--authorized-data-root", "/data/${weird}/root"],
):
    self.assertEqual(
        "/data/${weird}/root",
        guard.resolve_authorized_data_root(),
        "only the exact placeholder is rejected; a real path that merely "
        "contains ${ must be preserved as the argv authority",
    )

This directly exercises the Codex finding: a path with ${ in it is now returned as the argv authority rather than silently dropped. Placement inside the existing precedence test is correct — CLAUDE_PLUGIN_DATA is in the environment from the enclosing mock.patch.dict, so the env fallback would have returned /from-env before the fix, but now returns the correct /data/${weird}/root.

One minor note (not a blocker): The seam-lock test at L1508 still uses the string literal "${CLAUDE_PLUGIN_DATA}" rather than guard._AUTHORIZED_DATA_ROOT_PLACEHOLDER. Using the constant would make the assertion self-correcting if the placeholder ever changes. However, since that test's purpose is to verify what the frontmatter literally contains (and the harness substitutes the literal string, not the constant), the string form is also defensible. No action needed.

---

Status of all prior findings

#	Severity	Status
Codex — ${ substring check too broad	medium	Fixed in commit b7e048a
1 — preview/apply not exercised via hook argv	medium	Fixed in commit 53ee8a3
2 — test_guard_denies_data_root_without_hook_authority uses real sys.argv	low	Not addressed (no regression)
3 — is_exact_engine_apply signature changed, no callers or direct tests	low	Not addressed (no regression)
4 — resolve_authorized_data_root reads sys.argv directly (testability)	style/low	Not addressed (no regression)

---

Verdict

Approve. The Codex finding is fully resolved. The fix is correct, the security property is intact, and the new test directly exercises the previously broken edge case. All prior medium findings are addressed; remaining items are low/style with no regression introduced by any commit in this branch.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b7e048aca0

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