fix(disk-hygiene): surface silent destructive-guard launch failures

[kyle-sexton]

Closes #1416

Summary

• Closes the observability half of disk-hygiene: the destructive-operation guard has never launched — 73 recorded runs, all hook_non_blocking_error #1416. A repo-operator investigation (issue comments,
  2026-07-25T23:46-23:47Z) found both originally-reported launch-refusal root causes already fixed
  and merged (disk-hygiene: fix(disk-hygiene): resolve kill switch by reading user settings directly #1242/0.9.0; repo-hygiene: fix(repo-hygiene): destructive-guard fail-open on Windows — shell-form hook launch + jq fail-closed degraded mode #1006), and split the one remaining live
  defect (a silent post-launch death) to disk-hygiene: destructive guard exits 1 with empty stderr after 17s — a live fail-open distinct from #1242 #1423, fixed separately by fix(disk-hygiene): destructive guard denies at exit 2 instead of failing open at exit 1 #1449. What disk-hygiene: the destructive-operation guard has never launched — 73 recorded runs, all hook_non_blocking_error #1416 kept as its
  own scope, per the operator's brief and its amendment: make a guard-launch/runtime failure loud,
  because "the guard denied nothing because it approved" and "the guard denied nothing because it
  never ran, or ran and died" were indistinguishable from outside the harness.
• New detector, plugins/disk-hygiene/skills/clean/scripts/guard_launch_monitor.py. A second,
  independent hook — stdlib-only, imports nothing from destructive_guard.py or lib/ — registered
  on Stop (not PreToolUse/PostToolUse) in hooks/hooks.json. Deliberately not per-tool-call:
  this repo already paid for that mistake once
  (docs/adr/0004-rightsize-instruction-surfaces-by-incumbent-first-arbitration.md's D-12, a
  guardrails PreToolUse hook costing 12-19s p50 on every Bash call). Per the
  hooks reference (fetched 2026-07-25), Stop fires once
  per turn — the guard, if it ran, ran synchronously before the guarded command, so its failure
  record is already in the transcript well before the turn ends. The read itself is a bounded
  byte-seek tail (2MB cap) so per-turn cost never scales with session length, and a once-per-session
  marker (keyed by the hook's own session_id input, never a field found inside transcript
  records — those can differ from the file's own session, confirmed empirically against real local
  transcripts) short-circuits the read entirely after the first warning.
• Satisfies the amended criteria 2/3 exactly. The emitted systemMessage states the most recent
  failure's exitCode and durationMs explicitly (labelled, not just embedded) alongside truncated
  stderr and the total failure count — verified both by the rendered-string test in
  test_guard_launch_monitor.py (fixture shaped like the real disk-hygiene: destructive guard exits 1 with empty stderr after 17s — a live fail-open distinct from #1242 #1423 record: exitCode: 1,
  durationMs: 17054) and by a manual smoke test against a genuine local transcript record (see Test
  plan) that reproduces exitCode: 1, durationMs: 11, and the real config-refusal stderr text.
• Never blocks, never emits permissionDecision or decision: block; on any transcript read/parse
  failure it exits 0 with no output. The once-per-session marker degrades toward re-warning, never
  toward silence, if its own bookkeeping write fails — over-warning is the safe direction for a module
  whose entire purpose is killing a silent-suppression defect class.
• plugin.json 0.9.4 → 0.9.5, CHANGELOG.md entry, README.md and
  skills/clean/reference/safety-model.md both state what's covered (only destructive_guard.py's
  own command string, current-session only) and what isn't (repo-hygiene's own guard — verified
  working separately; no retroactive scan of past sessions).

Test plan

• plugins/disk-hygiene/skills/clean/scripts/guard_launch_monitor.test.sh — 17/17 pass:
  the disk-hygiene: destructive guard exits 1 with empty stderr after 17s — a live fail-open distinct from #1242 #1423 shape (states exitCode: 1/durationMs: 17054 in the rendered string), the
  launch-refusal shape, empty-stderr placeholder rendering, a clean session with a different
  hook's failure present (proves the command-substring filter discriminates), a fully clean session,
  malformed/unreadable transcript, missing transcript_path, malformed stdin, once-per-session
  suppression (same session id) vs independent warnings (different session ids), tail-bounded read
  still finds a failure near the end of an oversized transcript, marker-write failure still emits the
  warning this run, and a direct assertion that no permissionDecision/decision: block is ever
  emitted.
• Manual smoke test against a genuine local transcript (copied outside the repo, not committed):
  piped a real hook_non_blocking_error record for destructive_guard.py
  (exitCode: 1, durationMs: 11, the real "Plugin option "disk_hygiene_enabled" isn't set"
  stderr) through the finished detector — emitted systemMessage names all three correctly.
• bash scripts/check-hook-userconfig-argv.sh — pass (new hook's args carry no ${user_config.*}
  token).
• bash scripts/check-changelog-parity.sh --check-bump origin/main — pass.
• node scripts/validate-plugin-contracts.mjs — pass (43 setup skills, 2101 plugin files).
• claude plugin validate plugins/disk-hygiene/ — pass.
• bash scripts/run-plugin-tests.sh (full repo, 149 *.test.sh files) — run locally; time-boxed
  partway through (29/149 files, 0 failures) given this change's isolation to new disk-hygiene-only
  files plus the repo-wide structural gates above already passing across all 2101 plugin files. CI
  runs the same script to completion as the authoritative full-repo gate.

Related

Refs #1423 — the live launch/runtime-death fail-open this issue was found alongside, fixed separately
by #1449 (open, unmerged as of this PR).

Refs #1449 — open PR, unmerged, also touches plugins/disk-hygiene/.claude-plugin/plugin.json,
CHANGELOG.md, and hooks/hooks.json for the #1423 fix. Both PRs edit the same three files; whoever
merges second should expect a straightforward rebase (this PR adds a new Stop hooks.json key and a
new CHANGELOG/version entry — no overlapping lines with #1449's PreToolUse-side edit, but git may
still want a manual pass).

🤖 Generated with Claude Code

This was generated by AI during work-loop execution.

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e2ff216f3a

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

[kyle-sexton]

🤖 Autonomous merge lane — threads validated, deliberately left unresolved. (AI-generated.)

The lane was authorized to clear review threads so this PR could merge. It did not, because both findings are VALID and neither fix is one an autonomous lane should pick. Nothing was pushed; head is unchanged at e2ff216f3ab5306e3e040b862e1ff6eec4634184. Both threads were replied to with evidence and left open on purpose — resolving them would have cleared the merge gate over live concerns.

P1 — shared-interpreter blind spot (hooks.json:9 and :28)

Both entries declare the identical literal "command": "python3". So the Stop detector dies with the guard on exactly the WindowsApps-alias fail-open that this plugin's own README:86-96 documents. The detector meant to surface a silent guard failure shares the guard's single point of failure.

Why the lane did not fix it: the remedy is an unsettled launcher design decision — a shell shim, absolute-interpreter resolution, or a non-Python detector. Those have materially different portability and maintenance costs, and the diff does not settle which this repo wants.

Independent sub-finding, cheap to fix, surfaced while validating: the PR's three "does not cover" lists (README, safety-model.md, CHANGELOG) all omit this shared-interpreter case. As written, documented coverage reads broader than the code delivers — worth correcting regardless of which launcher remedy is chosen.

P2 — telemetry convention gap (real, but not this PR's to close)

A genuine convention gap, but plugin-wide and pre-existing rather than introduced here: there are zero HOOK_TELEMETRY_SINK references anywhere in plugins/disk-hygiene, and the PreToolUse guard emits none either. Closing it here would make this the fleet's first Python emitter — all 10 current emitters use the bash hook::emit_telemetry. That is a fleet-level convention decision, not a PR-scoped fix. Note the fleet tracker #836 is closed, so there was no live tracker to defer this against; it may need a fresh one.

Both AI reviews failed to produce a verdict

claude-review and claude-security-review both failed with is_error: true and returned no verdict. On a destructive-guard PR, that means the security review is un-run — not clean. The lane re-ran both workflows; since no commit was pushed, they would otherwise have stayed un-run indefinitely.

Honest note on verification

The worker skipped its test step rather than claiming a pass: zero files were touched, so there were no touched paths to test. Nothing here is reported as verified green.

Merge stays BLOCKED, correctly. No labels changed, no force-push, nothing merged.

[claude]

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

---

PR Review: fix(disk-hygiene): surface silent destructive-guard launch failures

• Read and analyze changed files
• Review core detector logic (guard_launch_monitor.py)
• Review hook registration (hooks.json)
• Review test coverage
• Review documentation changes
• Assess the two open review threads (P1 shared-interpreter, P2 telemetry)
• Post final review

---

Summary

The implementation is solid and the design decisions are well-reasoned. The stdlib-only detector, deliberate fail-open posture, bounded tail read, and once-per-session marker are all sound. Test coverage is thorough (17 scenarios). Two findings require attention before this merges.

---

Findings

P1 — Shared-interpreter blind spot (confirms the open thread)

File: plugins/disk-hygiene/hooks/hooks.json:28

The Stop detector and the PreToolUse guard both declare "command": "python3" — identical interpreter strings. plugins/disk-hygiene/README.md:86-96 documents that on stock Windows this resolves to the WindowsApps Store alias stub, which never starts an interpreter and causes a non-blocking error. In that scenario both hooks fail the same way: the hook_non_blocking_error record for the guard is written to the transcript, the detector exits non-blocking, and no systemMessage is emitted. The new observability guarantee does not hold on the one platform the README highlights as the most prominent fail-open vector.

The autonomous lane's assessment is correct — the remedy (shell shim, absolute-path resolution mirroring what setup check already does, or a non-Python detector) is a design decision this diff does not settle. I'm not asking for that to be resolved in this PR.

The cheap, PR-scoped fix that IS worth doing: The three "does not cover" disclosures in README.md, safety-model.md, and CHANGELOG.md each list three exclusions (repo-hygiene's guard, a renamed guard script, prior sessions) but omit the shared-interpreter case. As written, documented coverage reads broader than the code delivers. Adding "the shared-interpreter fail-open documented in the Windows python3 gotcha section" as a fourth exclusion bullet to all three locations is a doc-only change that accurately represents the current scope without requiring the launcher design decision. Fix this →

P2 — Telemetry convention gap (confirms the open thread; not this PR's to close)

File: plugins/disk-hygiene/skills/clean/scripts/guard_launch_monitor.py:258

The gap is real: docs/conventions/hook-observability/README.md requires a HOOK_TELEMETRY_SINK envelope for each meaningful outcome, and a guard failure found and reported is a meaningful outcome. But the gap is plugin-wide and pre-existing — neither destructive_guard.py nor any other disk-hygiene hook emits one, and every current fleet emitter goes through the bash hook::emit_telemetry helper. Closing it here would stand up the fleet's first Python emitter and apply it to one of two wired hooks while the primary guard still emits nothing. The fleet issue tracker (#836) is closed with no replacement, so there's no live ticket to defer against. This should not block this PR — file a fresh tracker for disk-hygiene's full telemetry adoption (both hooks, chosen language) and note the convention requirement there.

---

Implementation notes (no action required)

• _build_message at line 213 formats exitCode: {exit_code} and durationMs: {duration_ms} where the dict values may be None if a malformed record omits those fields. The output would show exitCode: None, which is unusual but accurate. The test suite validates against real-shaped records that include them, and the bare BaseException catch in main() means even a malformed record can't break the detector. Acceptable.
• The str(session_id) coercion at line 234 is redundant since the value is already str | None after the or "unknown-session" default, but harmless.
• The tail-read approach means guard failures that happened early in a session whose transcript grew past 2MB before any failure would never be detected. This is the correct tradeoff — documented, bounded cost, once-per-session marker ensures the amortized cost stays O(1) after the first warning.
• The _write_marker → re-warning degradation design is correct. Over-warning is the right failure mode for a module whose purpose is to kill silent suppression.
• Test isolation of tempfile.gettempdir() at line 106 is important and correctly implemented — without it, once-per-session markers from test runs would pollute the real temp dir.

---

Verdict

Request changes on P1's documentation gap; defer P2 to a separate tracker. The implementation is correct and the test suite is comprehensive. The one fix that belongs in this PR is adding the shared-interpreter exclusion to the three "does not cover" lists — it's a doc-only change that takes five minutes and makes the stated coverage match the actual code. The launcher design question (how to break the interpreter coupling) should be tracked separately.

[claude]

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

---

Security Review: fix(disk-hygiene): surface silent destructive-guard launch failures

• Load tools and initialize
• Read PR diff and changed files
• Audit guard_launch_monitor.py — path traversal, injection, file read safety
• Audit hooks.json — hook registration, interpreter trust
• Audit shell test script — injection patterns
• Assess existing findings (P1 shared-interpreter, P2 telemetry)
• Post security findings

---

Findings

IMPORTANT · Confirmed — Detector and guard share the same interpreter; detector is blind exactly when needed

Both hooks in hooks.json declare the identical literal "command": "python3":

• Line 9 — PreToolUse guard (destructive_guard.py)
• Line 28 — Stop detector (guard_launch_monitor.py)

The plugin's own README.md:86-96 documents that on stock Windows python3 resolves to a zero-length WindowsApps\python3.exe App Execution Alias stub — the guard never starts, Claude Code treats the non-blocking result as approval, and the destructive command proceeds ungated. Because both hooks use the same interpreter resolution path, the detector dies by exactly the same mechanism: the failure record is written to the transcript and never read. The net result is the complete inversion of the security property this PR states it delivers — destructive operations proceed ungated and the failure is invisible. This is the already-escalated P1 finding, confirmed against the branch; noting it here as a security finding because the failure mode affects a destructive-action control gate.

The three "does not cover" disclosures added to README.md, safety-model.md, and CHANGELOG.md do not enumerate this case, so documented coverage reads broader than the code delivers.

---

SUGGESTION · Plausible — Arbitrary file read via stdin-supplied transcript_path

guard_launch_monitor.py:166–173

def _read_tail(transcript_path: str) -> str:
    path = Path(transcript_path)
    size = path.stat().st_size
    with path.open("rb") as handle:
        ...
    return raw.decode("utf-8", errors="replace")

The transcript_path value is read from stdin JSON at line 232 and passed to Path().stat() / Path().open() with no validation that it falls within any expected directory. The script will open any path the process user has read access to — up to 2 MB — and decode it. In normal hook execution, transcript_path is supplied by the Claude Code harness and can be considered trustworthy. The actual exfiltration surface is further limited by the JSONL parser: only lines matching the hook_non_blocking_error / destructive_guard.py structure expose data (the stderr field, ≤300 chars). However, the absence of any path restriction means the pattern provides no defense-in-depth if the trust boundary changes (e.g., a future harness change, a misconfigured hook runner, or direct script invocation with crafted stdin during CI or testing). Adding a check that transcript_path starts with an expected base directory (e.g., the parent of the session transcript dir) would close this.

---

SUGGESTION · Plausible — Sensitive guard stderr propagated to user-visible system message

guard_launch_monitor.py:200–228

The stderr content from the guard's transcript record is included verbatim (truncated at 300 chars) in the systemMessage emitted to the user. If the guard process writes sensitive material to stderr — partial config values, filesystem paths, credential-adjacent fragments — those appear in the system message. The 300-char cap limits but does not eliminate the surface. No sanitization beyond truncation is applied. The test fixtures confirm this is intentional and the PR body explicitly calls it out as a feature (exitCode: 1, durationMs: 11, and "the real config-refusal stderr text" confirmed in smoke test). This is an accepted design trade-off for observability; flagging it so reviewers can confirm the guard's stderr is not expected to carry sensitive data from its callers.

---

Items reviewed and not flagged

• Shell test script injection (guard_launch_monitor.test.sh:25): $FLOOR is interpolated into a Python -c command, but the upstream sed regex constrains output to digits and a dot only (\1.\2 where \1/\2 are [0-9]*). No injection vector.
• Marker file path traversal via session_id: session_id is sanitized to alphanumeric + -_ before use in filenames. No traversal possible.
• JSON injection in systemMessage output: json.dumps() handles all escaping; no injection surface in the final output.
• BaseException catch in main(): intentional design for detector reliability; exits 0 on all failure paths without blocking the turn.
• Telemetry gap (P2): noted by prior reviewers; this is a convention-compliance gap, not a security issue.

[claude]

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

---

PR Review: fix(disk-hygiene): surface silent destructive-guard launch failures

• Load tools and initialize
• Read changed files and new commit
• Verify P1 documentation gap fix (new commit 7209392)
• Review core detector (guard_launch_monitor.py)
• Review tests (test_guard_launch_monitor.py, guard_launch_monitor.test.sh)
• Review hook registration (hooks.json)
• Review documentation changes (README, CHANGELOG, safety-model)
• Assess open findings P1/P2 against current HEAD
• Post final review

---

Summary

Two commits under review:

• e2ff216 — original implementation of guard_launch_monitor.py, tests, hook registration, and documentation
• 7209392 — documentation-only follow-up that discloses the shared-interpreter blind spot across all three coverage-disclosure sites

The implementation is solid and the test suite is thorough. The new commit resolves the one PR-scoped fix that was blocking this PR. Both open threads are still unresolved by design — this review confirms the correct disposition for each.

---

P1 — Documentation gap: RESOLVED by 7209392

The previous review requested that README.md, safety-model.md, and CHANGELOG.md each add the shared-interpreter exclusion to their "does not cover" lists, since documented coverage read broader than what the code delivers.

7209392 delivers exactly that at all three sites:

• README.md (line 75-78): "It is also wired with the same literal python3 command as the guard it watches, so the interpreter-resolution failure below — the WindowsApps alias stub, or a missing/broken python3 — takes the detector down with the guard and goes unreported (fix(disk-hygiene): guard-launch detector shares the guard's interpreter, so it is blind to the interpreter fail-open #1504)."
• CHANGELOG.md (0.9.5 entry): "because both hooks are wired with the same literal python3 command — the interpreter-resolution fail-open documented in the README... takes the detector down with the guard, so that one vector stays unreported until the detector gets a launcher independent of the guarded interpreter (fix(disk-hygiene): guard-launch detector shares the guard's interpreter, so it is blind to the interpreter fail-open #1504)."
• safety-model.md: same disclosure with reference to fix(disk-hygiene): guard-launch detector shares the guard's interpreter, so it is blind to the interpreter fail-open #1504

Each disclosure also references the tracking issue (#1504) for the launcher design fix. The documented coverage now matches the actual code scope. This was the PR-scoped ask; it's done.

The code-level fix (breaking the interpreter coupling) is correctly deferred to #1504. That is the right call — a shell shim, absolute-path resolution, or a non-Python detector each carry distinct portability trade-offs that this diff does not settle.

---

P2 — Telemetry convention gap: confirmed deferred, correct disposition

Zero HOOK_TELEMETRY_SINK references anywhere in plugins/disk-hygiene/. The gap is plugin-wide, pre-existing, and fleet issue #836 (which covered OTel-envelope adoption) is closed. The previous reviews are right that closing it here would stand up the fleet's first Python emitter while the primary guard still emits nothing — half an adoption. Does not block this PR; needs a fresh tracker for full disk-hygiene telemetry coverage (both hooks).

---

Implementation notes (no action required)

guard_launch_monitor.py is well-structured:

• Stdlib-only, zero imports from destructive_guard.py or lib/ — correct, since a guard that can't launch can't report it didn't. line 26-28
• _MAX_TAIL_BYTES = 2_000_000 cap keeps per-turn cost O(constant), not O(session). line 88
• Once-per-session marker keyed by session_id from hook stdin (never from inside transcript records, which can carry different session IDs). line 62-65
• _write_marker degrades toward re-warning, never silence — correct failure direction. line 154-162
• BaseException catch in main() is intentional, documented with the noqa suppression. line 260
• str(session_id) coercion at line 234 is redundant but harmless.
• exitCode: None possible if a malformed record omits the field — output would show exitCode: None, which is unusual but accurate. The BaseException catch ensures even this case can't break the detector.

Tests are comprehensive: 17 scenarios covering the #1423 discriminating shape, launch-refusal shape, clean session with another hook's failure (proves command-substring filter discriminates), fully clean session, unreadable/missing transcript, malformed stdin, once-per-session suppression (same session), independent warnings (different session IDs), marker-write failure still warns, unsubstituted placeholder treated as absent, multiple failures with count and most-recent selection, tail-bounded read still finds a late failure in an oversized transcript (verifies the 2MB cap mechanism), non-attachment records ignored, malformed JSON line doesn't abort scan, and no permissionDecision/decision: block ever emitted. Test isolation via tempfile.gettempdir() mock and CLAUDE_PLUGIN_DATA env patch is correctly implemented.

Shell test wrapper: Parses Python version floor from hygiene.py directly rather than duplicating the number — correct single-source approach. No injection vector (the sed regex constrains $FLOOR to digits and a dot). guard_launch_monitor.test.sh

One minor observation not previously noted: The runtime systemMessage built by _build_message (line 219-228) still lists only three "does not cover" items and does not mention the shared-interpreter limitation — only the static documentation got that addition. This is not a blocker: the systemMessage fires only when the detector did successfully run (meaning python3 resolved and the interpreter started), at which point the shared-interpreter limitation is not the active failure mode. The disclosure belongs in the docs, which is where it now lives.

---

Verdict

Approved. The P1 documentation gap that was blocking this PR is resolved by 7209392. The implementation is correct, the test suite is comprehensive, and the two open review threads are at their correct dispositions — P1's code fix tracked in #1504, P2's telemetry gap needing a fresh tracker for a full disk-hygiene adoption (both hooks). Nothing remaining here warrants holding this PR.
| Branch

[claude]

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

---

Security Review: fix(disk-hygiene): surface silent destructive-guard launch failures

• Initialize and load tools
• Get PR diff and changed files
• Read core detector (guard_launch_monitor.py)
• Read hook registration (hooks.json)
• Read test files for security patterns
• Audit for injection surfaces, path traversal, logic flaws, auth gaps
• Post security findings

---

Overview

Reviewed the 8 changed files. The PR adds a new Stop-event hook (guard_launch_monitor.py) that reads the session transcript and emits a systemMessage when it detects a silent destructive_guard.py launch/runtime failure. The implementation is stdlib-only, fail-open on errors, and never emits blocking output.

Prior reviews identified two concerns; I independently verified both and add one new finding. Results below, severity- and confidence-tagged.

---

Finding 1 — IMPORTANT · Confirmed — Shared interpreter makes the detector blind on its highest-priority fail-open vector

Confidence: HIGH — directly readable in two places; no inference required.

Files: hooks.json:9 and hooks.json:28

Both hooks declare the identical literal "command": "python3":

// PreToolUse guard (line 9)
"command": "python3"

// Stop detector (line 28)
"command": "python3"

When python3 resolves to the Windows Store alias stub (documented at README.md:89-100) or is otherwise missing/broken, the guard fails non-blocking, the destructive command proceeds ungated, and the detector fails via the exact same mechanism before it can read the transcript. The security guarantee this PR introduces — that a silent guard failure becomes visible — does not hold on the platform the README identifies as its most prominent fail-open vector.

Status as of HEAD (7209392): The disclosure gap (prior reviewer's "cheap fix") is now addressed — README.md:75-78, CHANGELOG.md:27-31, and safety-model.md all enumerate the shared-interpreter case as an explicit coverage exclusion, and #1504 tracks the launcher fix. The code-level exposure remains open but is now correctly disclosed. No further action needed in this PR beyond what was done.

---

Finding 2 — SUGGESTION · Confirmed — Arbitrary file read via stdin-supplied transcript_path

Confidence: MEDIUM — exploitable only if harness stdin is controlled; exfiltration surface is narrow.

File: guard_launch_monitor.py:165–173

def _read_tail(transcript_path: str) -> str:
    path = Path(transcript_path)
    size = path.stat().st_size
    with path.open("rb") as handle:
        ...

transcript_path is taken from stdin JSON at line 232 with no validation that it falls within an expected directory, is not a symlink, or is not a device node. The script will open any path the process user can read, up to 2 MB. The actual exfiltration surface is narrow: only content from lines matching the hook_non_blocking_error + destructive_guard.py command structure surfaces in the output (≤ 300 chars of the stderr field). Most arbitrary files won't contain JSONL records with that shape, so the practical risk is low in normal operation.

In the normal hook execution model the harness controls stdin and transcript_path is trustworthy. The risk materializes if: (a) the script is invoked directly with crafted stdin during CI/testing, (b) the harness's hook invocation is subverted, or (c) the plugin runs in a multi-user environment where one user can influence another user's hook stdin. None of these are likely in a standard deployment; noting this as a defense-in-depth gap for the case of future harness changes or non-standard invocation paths.

Adding a check that transcript_path starts with an expected base directory (e.g., ~/.claude/) would close this without affecting normal operation. Consider fixing →

---

Finding 3 — SUGGESTION · Confirmed — Guard stderr propagated verbatim into systemMessage

Confidence: HIGH — the PR body explicitly confirms this is intentional.

File: guard_launch_monitor.py:200–228

The guard's stderr field (from the transcript record) is included verbatim, truncated at 300 chars, in the systemMessage emitted to Claude. If the guard process writes sensitive material to stderr — filesystem paths, partial credential fragments, usernames — those appear in the context window. The 300-char cap limits but doesn't eliminate the surface. The PR explicitly validates this behavior as a feature (manual smoke test confirms "real config-refusal stderr text" appears in the message).

This is a deliberate design trade-off for observability. Noting it so reviewers can confirm the guard's stderr is not expected to carry sensitive data from its callers.

---

Finding 4 — SUGGESTION · New — str(session_id) coercion on a non-string JSON value creates inconsistent marker-key semantics

Confidence: LOW — non-obvious, depends on harness sending a non-string session_id.

File: guard_launch_monitor.py:233

session_id = hook_input.get("session_id") or "unknown-session"
marker_paths = _marker_path_candidates(data_root, str(session_id))

hook_input.get("session_id") can return any JSON-parsed value. A truthy non-string (e.g., a large JSON object {"uuid": "..."}) passes the or guard, is coerced via str() to its Python repr ("{'uuid': '...'}""), then sanitized to alphanumeric+-_. Two different sessions whose session_id objects differ only in ordering or types could map to the same sanitized key, causing one session to inherit the "already warned" suppression of another — silencing the detector for a session where it should fire.

More consequentially: if a malicious actor could craft stdin such that session_id is a truthy value whose sanitized form matches a legitimate in-flight session's marker filename, they could pre-suppress the warning for that session. The practical risk is low given harness trust, but the str() path is unintentional: the field is documented to be a string. A isinstance(session_id, str) guard before use would make this explicit. Consider fixing →

---

Items audited and not flagged

• Shell test script injection (guard_launch_monitor.test.sh:25): $FLOOR is interpolated into an inline Python -c string. The upstream sed regex constrains output to \1.\2 where both groups are [0-9]* — only digits and a dot. No injection vector exists.
• Marker file path traversal via session_id: safe_session sanitizes to alphanumeric + -_, converting /, . and all other chars to _. No path traversal possible via session_id.
• JSON injection in systemMessage output: The final json.dumps({"systemMessage": message}) at line 258 handles all escaping. No injection surface in the output.
• permissionDecision/decision: block emission: The test at test_guard_launch_monitor.py:344-360 confirms these are never emitted, and the implementation confirms the only output key is systemMessage.
• BaseException catch: Intentional fail-open design; the detector never blocks a turn. Acceptable.
• _argv_flag_value flag parsing: Correctly handles both --flag value and --flag=value forms; no injection via argv since values come from the harness-substituted ${CLAUDE_PLUGIN_DATA}.
• data_root write surface: Marker writes using data_root from CLAUDE_PLUGIN_DATA write only files containing the string "1" to subdirectories of the supplied root. Requires controlling the env var; impact is minimal (small file writes).

---

Verdict

The implementation is sound. Finding 1 (shared-interpreter blind spot) is now correctly disclosed in the documentation by the latest commit and tracked for a proper fix in #1504 — no further action needed here. Findings 2–4 are SUGGESTION-level and low-severity; none block merge. The detector's fail-open design, bounded tail read, sanitized session key, and json.dumps output encoding are all correct. The test suite covers the security-relevant scenarios (no blocking output, marker suppression, malformed inputs, command-substring discrimination).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7209392dab

ℹ️ 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 43s —— View job

---

PR Review: fix(disk-hygiene): surface silent destructive-guard launch failures

• Gather context and review diff
• Analyze new commit a3976c6 (tail window fix)
• Assess prior findings against current HEAD
• Review guard_launch_monitor.py changes
• Post findings

---

New commit: a3976c6 — tail boundary fix

This commit addresses the Codex finding from the second automated review: "inspect the byte preceding the seek position and discard the first line only when the offset is actually mid-line."

The fix is correct. The previous unconditional handle.readline() would discard a whole record when size - _MAX_TAIL_BYTES landed exactly on a record's first byte — meaning the tail window started at a clean boundary, not mid-record, and the discard threw away a real record. That record could be the session's only guard failure, silencing the very report the detector exists to make.

The new implementation at guard_launch_monitor.py:170–177:

offset = size - _MAX_TAIL_BYTES
handle.seek(offset - 1)
starts_mid_record = handle.read(1) != b"\n"
if starts_mid_record:
    handle.readline()  # discard the truncated partial first line

• Seeks to offset - 1 (the byte immediately before the window), reads one byte.
• If that byte is \n, the window already starts at a record boundary — no discard needed.
• If it is not \n, the first byte at offset is mid-record — discard that partial line.
• After read(1), the file position is exactly at offset in both branches, so the subsequent handle.read() picks up from the right place with or without the conditional discard.

The minimum offset (size == _MAX_TAIL_BYTES + 1 → offset == 1) is handled correctly: handle.seek(0) reads the first file byte, and the logic applies without underflowing.

The test fixture at test_guard_launch_monitor.py:329–360 is well-constructed. It uses _filler_line(nbytes) which counts the trailing \n in its byte budget, so the arithmetic works out exactly: the padding line fills _MAX_TAIL_BYTES - len(failure) - 1 bytes on disk, leaving the failure record occupying the first len(failure) + 1 bytes of the retained window. The assertion confirms the boundary placement before the detector runs — a good defensive practice for a fixture whose whole point is byte-level precision.

---

Prior findings — disposition at HEAD a3976c6

Finding	Status
Codex P2 (tail boundary — discard when already on boundary)	Fixed by a3976c6
claude-review P1 (documentation gap — shared-interpreter exclusion missing)	Fixed by 7209392
Codex P1 / claude-review (shared-interpreter blind spot — code level)	Open, correctly deferred to #1504; documented at all three coverage-disclosure sites
Codex P2 / claude-review (telemetry convention gap)	Open, correctly deferred to #1505
Security Finding 2 (arbitrary file read via transcript_path)	Acknowledged, low severity, no action
Security Finding 3 (guard stderr in systemMessage)	Intentional design trade-off, no action
Security Finding 4 (str(session_id) on non-string JSON value)	Still present, SUGGESTION/LOW confidence, acceptable

---

Verdict

Approved. The new commit correctly fixes the tail boundary bug, the test validates the exact byte-level scenario, and the CHANGELOG entry is updated. All open threads are at their correct dispositions — the two code-level deferred items (#1504, #1505) have live trackers and are not regressions introduced by this PR. Nothing remaining here warrants holding.
| Branch

[claude]

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

---

Security Review: fix(disk-hygiene): surface silent destructive-guard launch failures

• Load tools and read PR diff
• Audit guard_launch_monitor.py — path traversal, injection, file read safety, logic flaws
• Audit hooks.json — hook registration, interpreter trust
• Audit test files for security patterns
• Review prior findings against HEAD (a3976c65)
• Post final security findings

---

HEAD reviewed: a3976c65f9f8103f5b951d8ed8535d37a4d2c95e
Scope: 8 changed files across 3 commits (e2ff216, 7209392, a3976c65).

Three previously-identified findings survive at current HEAD; one new edge-case observation on the latest commit. No injection, command-execution, or privilege-escalation surfaces found beyond the four items below.

---

IMPORTANT · Confirmed — Detector shares the guard's interpreter; blind on the most prominent fail-open vector

File: hooks/hooks.json:9 and hooks/hooks.json:28

Both hooks declare the identical literal "command": "python3". On stock Windows, that name resolves to the WindowsApps Store alias stub (documented at README.md:89–100), causing the guard to fail non-blocking and the destructive command to proceed ungated. Because the detector is launched through the same path, it dies by the same mechanism: the hook_non_blocking_error record is written to the transcript and never read. The security property this PR adds — guard failures become visible — does not hold on the platform the README identifies as its most prominent fail-open vector.

Status at HEAD: Code-level exposure remains open by design. Commit 7209392 added accurate disclosure at all three coverage-disclosure sites (README.md:75–78, CHANGELOG.md, safety-model.md), and #1504 tracks the launcher fix. The documented envelope now matches actual scope; the code fix is deferred correctly.

---

SUGGESTION · Plausible — Arbitrary file read via stdin-supplied transcript_path

File: skills/clean/scripts/guard_launch_monitor.py:165–173

def _read_tail(transcript_path: str) -> str:
    path = Path(transcript_path)
    size = path.stat().st_size
    with path.open("rb") as handle:
        ...

transcript_path is read from stdin JSON at line 238 with no validation that it falls within an expected directory or is not a symlink / device node. Up to 2 MB of any path the process user can read will be loaded. Practical exfiltration surface is narrow — only lines whose parsed shape matches type == "attachment" / attachment.type == "hook_non_blocking_error" / command containing "destructive_guard.py" propagate content to output. Most arbitrary files produce nothing. Risk materialises when the script is invoked directly with crafted stdin (CI, testing pipelines, non-standard hook runners). Adding a check that Path(transcript_path).resolve() starts with an expected base directory (e.g., Path.home()) would close this without affecting normal operation. Consider fixing →

---

SUGGESTION · Confirmed — Guard stderr propagated verbatim into systemMessage

File: skills/clean/scripts/guard_launch_monitor.py:206–212

The stderr field from the guard's transcript record (up to 300 chars, truncated) is included verbatim in the systemMessage emitted to Claude. If the guard writes sensitive material to stderr — filesystem paths, partial credential fragments, usernames — those appear in the context window. The PR body and smoke-test explicitly confirm this is intentional ("real config-refusal stderr text" confirmed in output). Noting it for reviewer confirmation that destructive_guard.py's stderr is not expected to carry sensitive data from its callers under any configuration.

---

SUGGESTION · Low confidence — Non-string session_id value bypasses the "unknown-session" fallback and produces an inconsistent marker key

File: skills/clean/scripts/guard_launch_monitor.py:239–240

session_id = hook_input.get("session_id") or "unknown-session"
marker_paths = _marker_path_candidates(data_root, str(session_id))

hook_input.get("session_id") can return any JSON-parsed value. A truthy non-string (e.g., a JSON object {"uuid": "..."}) passes the or guard, is coerced via str() to its Python repr, then sanitized to alphanumeric+-_. If a controlled stdin can supply a session_id whose sanitized str() form matches a legitimate in-flight session's marker filename, the detector can be pre-suppressed for that session. The attack requires controlling the hook stdin (harness trust assumption), and the practical impact is over-suppression (silence rather than harm), which is the safe-direction failure for most modules but is the opposite of the correct failure direction for a module whose purpose is killing silent suppression. An isinstance(session_id, str) guard before use would make this explicit. Consider fixing →

---

New commit (a3976c6) — no new security issues

The boundary-probe fix (offset - 1 seek before deciding whether to call readline()) is a pure correctness improvement. offset is size - _MAX_TAIL_BYTES and the enclosing if size > _MAX_TAIL_BYTES guard guarantees offset >= 1, so offset - 1 >= 0: no underflow, no invalid seek. From a security standpoint the fix closes a secondary suppression vector — the previous unconditional readline() could drop the only guard-failure record in the tail, silencing the detector for a real failure, which is the wrong failure direction for this module. The fix is correct.

---

Items audited and not flagged

• Shell test wrapper injection (guard_launch_monitor.test.sh:25): $FLOOR is interpolated inside an inline Python -c string. The upstream sed regex constrains output to digits and a dot only — no injection vector.
• Marker path traversal via session_id: safe_session sanitizes to alphanumeric + -_, converting /, . and all other characters to _. Path traversal via session_id is not possible.
• JSON injection in output: json.dumps({"systemMessage": message}) at line 264 handles all escaping. No injection surface in the emitted output.
• permissionDecision/decision: block emission: Implementation emits only {"systemMessage": ...} and never emits blocking vocabulary. Confirmed by test at test_guard_launch_monitor.py:379.
• data_root write surface: Marker writes under data_root (from CLAUDE_PLUGIN_DATA or --data-root) write only the string "1" to named subdirectories. Controlling the env var is a prerequisite; impact is a small file write, not code execution.
• BaseException catch in main(): Intentional fail-open design; exits 0 on all failure paths. Does not mask a security condition — the detector's own failure surface is near-zero by stdlib-only construction.
  | Branch

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a3976c65f9

ℹ️ 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 1m 26s —— View job

---

PR Review: fix(disk-hygiene): surface silent destructive-guard launch failures

• Read changed files at HEAD (abfbec9)
• Review latest commit — marker ordering fix
• Assess all prior findings against current HEAD
• Post final review

---

New commit: abfbec9 — marker-ordering fix

This commit addresses the Codex finding from the third automated review: the once-per-session marker was written before print(), so a broken pipe (or early termination) left the marker in place with nothing delivered, then the broad BaseException catch exited quietly — permanently silencing the detector for the session. The exact silent-suppression class this module exists to prevent.

The fix is correct. _run() now returns (message, marker_paths) and main() writes the marker only after both print() and sys.stdout.flush() succeed:

print(json.dumps({"systemMessage": message}))
sys.stdout.flush()          # raises BrokenPipeError if pipe is closed
_write_marker(marker_paths) # only reached if flush succeeds

If flush() raises, the BaseException block catches it, the marker is never written, and the next Stop in the same session finds no marker and fires again. The degradation is toward re-warning, which is the correct direction.

The test is well-constructed. _BrokenStdout inherits from io.StringIO (so print() succeeds into the buffer) but overrides flush() to raise BrokenPipeError. This accurately models the common broken-pipe scenario: the process writes to a userspace buffer, then flush triggers the OS send and discovers the other end is gone. The fixture then verifies the retry fires with the expected content. This is exactly the right scenario to cover.

One observation (no action required): If _write_marker() itself raises an OSError that bubbles past the internal except OSError: continue loop (not currently possible given the loop structure, but worth noting), the BaseException block in main() would catch it and the marker would not be written — re-warning on next Stop. This is the _write_marker docstring's stated contract ("Failure here must never suppress a real finding") and is already the correct degradation direction.

---

Prior findings — disposition at HEAD abfbec9

Finding	Status
Codex P1 / shared-interpreter blind spot (code)	Open, correctly deferred to #1504; all three disclosure sites enumerate it
Codex P1 / documentation gap	Fixed by 7209392
Codex P2 / tail boundary (unconditional readline)	Fixed by a3976c6
Codex P2 / marker written before delivery	Fixed by abfbec9
Telemetry convention gap (plugin-wide, pre-existing)	Correctly deferred to #1505
Security: transcript_path path restriction	SUGGESTION/low; no blocker
Security: guard stderr in systemMessage	Intentional design; documented
Security: str(session_id) on non-string JSON value	SUGGESTION/low; no blocker

---

Implementation notes (no action required)

The full implementation at HEAD is clean:

• _run() → tuple[str, list[Path]] | None — the signature change is the right abstraction; it makes the "caller marks, not callee" contract explicit in the type rather than relying only on the docstring.
• sys.stdout.flush() before _write_marker() — flush() is the right flush point; print() alone offers no delivery guarantee.
• test_undelivered_warning_is_not_marked_as_warned — precise fixture that covers the exact broken-pipe scenario; the two-phase structure (run with broken pipe, then retry and assert content) is the minimal and sufficient test.
• CHANGELOG.md entry updated to cover both the tail-boundary and marker-ordering fixes in 0.9.5.

---

Verdict

Approved. The Codex marker-ordering finding is correctly and cleanly fixed by abfbec9. All three per-commit correctness findings (documentation gap, tail boundary, marker ordering) are resolved. The two open review threads are at their correct dispositions — shared-interpreter launcher design tracked in #1504, telemetry adoption tracked in #1505. The implementation is sound, the test suite is comprehensive, and nothing remaining here warrants holding.
| Branch

[claude]

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

---

Security Review: fix(disk-hygiene): surface silent destructive-guard launch failures

• Read changed files (PR diff + full source)
• Audit guard_launch_monitor.py — path traversal, injection, file read, session handling, marker timing
• Audit hooks.json — interpreter trust, hook registration
• Audit shell test wrapper — injection patterns
• Verify prior findings against HEAD abfbec9
• Post final findings

---

HEAD reviewed: abfbec9d22d7437a2e5158b4fa1408f595a602e8
Scope: 8 changed files across 5 commits.

No injection surfaces, command execution vectors, credential exposure, or GitHub Actions issues found. Three findings carry over from prior reviews (two SUGGESTION, one IMPORTANT). One prior finding (marker-before-delivery) is confirmed fixed. Full accounting below.

---

IMPORTANT · Confirmed — Shared interpreter makes the detector blind on its most prominent fail-open vector

Confidence: HIGH

Files: hooks/hooks.json:9 and hooks/hooks.json:28

Both hooks declare the identical literal "command": "python3". On stock Windows, that name resolves to the WindowsApps Store alias stub documented at README.md:89–100, causing the guard to fail non-blocking and the destructive command to proceed ungated. Because the detector launches via the same path, it dies identically: the hook_non_blocking_error record is written to the transcript and never read. The security property this PR introduces does not hold on the platform the README identifies as its most prominent fail-open vector.

Status at HEAD: Code-level exposure is open by design and disclosed at all three coverage sites (README.md, CHANGELOG.md, safety-model.md) since commit 7209392. Tracked for a launcher fix in #1504. No further action needed in this PR.

---

SUGGESTION · Plausible — Arbitrary file read via stdin-supplied transcript_path

Confidence: MEDIUM

File: guard_launch_monitor.py:165–168

def _read_tail(transcript_path: str) -> str:
    path = Path(transcript_path)
    size = path.stat().st_size
    with path.open("rb") as handle:

transcript_path is taken from stdin JSON at line 244 with no validation that it falls within an expected directory or is not a symlink/device node. Up to 2 MB of any path the process user can read is loaded. The practical exfiltration surface is narrow — only content from lines whose parsed shape matches type == "attachment" / attachment.type == "hook_non_blocking_error" / command containing "destructive_guard.py" reaches output, and at most 300 chars of the stderr field at that. Most arbitrary files produce nothing. The risk materialises if the script is invoked directly with crafted stdin (CI pipelines, testing harnesses, non-standard hook runners). Adding a Path(transcript_path).resolve() prefix check against Path.home() before stat()/open() would close this without affecting normal operation.

---

SUGGESTION · Confirmed — Non-string session_id bypasses the "unknown-session" fallback and produces an unpredictable marker key

Confidence: LOW

File: guard_launch_monitor.py:245–246

session_id = hook_input.get("session_id") or "unknown-session"
marker_paths = _marker_path_candidates(data_root, str(session_id))

A truthy non-string JSON value (dict, list, int) passes the or guard. str() coerces it to its Python repr ("{'uuid': '...'}""), which after safe_session sanitization produces a deterministic (but unpredictable) marker filename. If controlled stdin can supply a session_id whose sanitised str() form matches an in-flight session's marker filename, the warning is pre-suppressed for that session. The practical impact is over-suppression (silence rather than harm), which is the wrong failure direction for a module whose purpose is killing silent suppression. An isinstance(session_id, str) guard before use would make this explicit.

---

VERIFIED FIXED — Marker written before delivery (Codex P2, commit a3976c65)

File: guard_launch_monitor.py:267–271

if found:
    message, marker_paths = found
    print(json.dumps({"systemMessage": message}))
    sys.stdout.flush()
    _write_marker(marker_paths)

The marker is now written only after flush() succeeds. If flush() raises BrokenPipeError, the BaseException handler at line 273 catches it, _write_marker is not called, and the next Stop invocation retries. Confirmed by test_undelivered_warning_is_not_marked_as_warned and independently by tracing the execution path. Prior concern eliminated.

---

Items audited and not flagged

• Shell test wrapper injection (guard_launch_monitor.test.sh:25): $FLOOR is interpolated into an inline Python -c string. The upstream sed regex constrains output to digits and a dot only ([0-9]*\.[0-9]*) — no injection vector.
• Marker path traversal via session_id: safe_session sanitises to alphanumeric + -_, converting /, ., and all other characters to _. Path traversal via session_id is not possible.
• JSON injection in output: json.dumps({"systemMessage": message}) at line 269 handles all escaping. No injection surface in the emitted output.
• No command execution: The script contains zero subprocess calls, eval(), exec(), or os.system() — stdlib-only, read/write only.
• permissionDecision/decision: block emission: Implementation emits only {"systemMessage": ...}. Confirmed by test_no_permission_decision_or_block_ever_emitted.
• data_root write surface: Marker writes under data_root write only the string "1" to named subdirectories. Impact is a small file write; all OSError cases are caught and silently skipped.
• BaseException catch: Intentional fail-open design; exits 0 on all failure paths. Does not mask a security condition.
• TOCTOU on _already_warned / _write_marker: Benign — concurrent double-warning is the safe over-warning direction for this module.
• No GitHub Actions changes: No workflow files modified.

---

Verdict

No blocking security issues introduced by this PR. The IMPORTANT shared-interpreter finding is real, pre-existing at the code level, correctly disclosed in documentation, and tracked for a fix in #1504. Both SUGGESTION-level findings are low-severity, require harness-level stdin control to exploit, and leave no capability beyond triggering over-suppression or reading non-matching file content through a narrow JSONL filter. The marker-timing defect found by Codex P2 is confirmed fixed.
| Branch

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: abfbec9d22

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