fix(disk-hygiene): deny when the guard's decision cannot be written to stdout

[kyle-sexton]

Summary

Follow-up to #1449, which closed six fail-open paths in destructive_guard.py but not this one — none of its in-process tests could reach the failure.

_decide writes its decision with print, which only buffers. A closed or lost stdout pipe therefore raises nowhere inside main — the failure surfaces at interpreter shutdown, and CPython reports a failed shutdown flush by replacing the exit status with 120.

Measured on merged efb6c271 (deny decision, stdout wired to a pipe whose reader is closed):

deny + broken stdout -> exit 120   "Exception ignored while flushing sys.stdout"

120 is "any other exit code" under the PreToolUse contract — non-blocking, so the destructive command runs even though the guard had decided to deny it. Same shape as #1423, reached from the one stream #1449's stderr work did not cover.

The module tail now flushes stdout itself, so an undeliverable decision is caught while there is still a decision to make: it denies at exit 2 with a diagnostic, because a decision the host never received is not a decision. It then flushes stderr best-effort and os._exits the resolved code, so a shutdown flush can no longer rewrite it. #1449's null-device fallback is reused rather than duplicated — extracted from _write_diagnostic as _discard_stream.

A follow-up commit then removed that same _discard_stream(sys.stdout) call from main's deny handler once re-verification showed the module tail's explicit flush — not the discard call — was what actually closed the exit-120 path; the discard call only changed which of two true diagnostics was emitted.

Test plan

• test_undeliverable_stdout_decision_denies_at_exit_2_in_a_real_process spawns the real script against a genuinely closed stdout pipe and asserts exit 2 with the diagnostic; it exits 120 against the pre-fix tail.
• Re-verified the redundant-discard-removal commit: deny with stdout wired to a pipe whose reader is closed still exits 2 with "the decision could not be written to stdout"; confirmed neither this module nor killswitch_config registers an atexit handler or __del__, which the tail's os._exit would otherwise skip.
• 212 tests pass.

Related

Closes #1423 (the remaining stream not covered by #1449).

Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 26c0b7f886

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

[kyle-sexton]

🤖 Released by autonomous babysit lane (fable-autopilot). PR merged as 0a0e2fa (squash). The one open review thread was verified and resolved; both rate-limited AI review jobs were re-run to genuine completion first, since security-review / security-review is a required context and had reported success without the review actually running.

[claude]

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

---

PR Review

• Gather context (diff, changed files)
• Understand the request
• Review the changed code
• Post findings

---

Scope: One commit, one file — 189658a adds env.pop("PYTHONUNBUFFERED", None) to the real-process stdout-failure test introduced in 2de5f36.

The fix is correct and complete

The Codex P2 comment identifies the exact problem this commit resolves. When the test suite runs under PYTHONUNBUFFERED=1 (e.g., python3 -m unittest in a CI harness that sets it), the child inherits the variable and Python's text streams run unbuffered. In that configuration the print in _decide raises immediately — BrokenPipeError on POSIX, OSError errno 22 on Windows — inside main, which the outer except BaseException catches and handles via the generic internal-error diagnostic. The subprocess exits 2, but with the wrong message: internal error ... BrokenPipeError, not could not be written to stdout.

That path is correct behaviour, but it's not the path this test was written to exercise. The test exists to verify the module-tail flush handler that was the actual fix for the exit-120 regression. With the inherited variable, that handler is never reached, and the diagnostic assertion at line 3999 fails.

Dropping PYTHONUNBUFFERED from the child's environment is the right fix — not widening the assertion, because widening it would collapse the regression signal onto a platform guess. The child buffers as normal, the buffer flush fails at interpreter shutdown, the module tail catches it, and the diagnostic is the one the assertion expects.

The comment at lines 3970–3974 explains this correctly and concisely.

Sibling tests are not affected

The two other real-process tests that copy os.environ (lines 4027, 4140) both use stdout=subprocess.PIPE — a live, connected pipe — so PYTHONUNBUFFERED cannot make their stdout writes fail. The scoping to this test only is correct.

Minor observation (no action required)

Line 3998: assertNotEqual(120, proc.returncode) is logically redundant once assertEqual(2, proc.returncode) on line 3997 passes. It reads as a regression label rather than a guard — fine as documentation, and the PR description calls this out explicitly.

Verdict

Clean, minimal, correctly targeted test fix. No correctness, security, or architecture concerns. Addresses the Codex P2 finding fully.

[claude]

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

---

Security Review — PR #1588

• Gather PR diff and context
• Read changed files
• Perform security analysis
• Post findings

---

Reviewed commits: 2de5f36, 26c0b7f, 189658a
Scope: destructive_guard.py (core fix + redundant-call removal), test_hygiene.py (test environment fix)

---

No exploitable security issues found

The changes close a genuine, previously-verified fail-open path and do so correctly. Below is a trust-boundary walk for each new piece.

---

Module-tail flush + os._exit (core fix)

if __name__ == "__main__":
    _exit_code = main()
    try:
        sys.stdout.flush()
    except BaseException:
        _write_diagnostic("...denying by default.")
        _discard_stream(sys.stdout)
        _exit_code = 2
    with contextlib.suppress(BaseException):
        sys.stderr.flush()
    os._exit(_exit_code)

Property verified: If stdout delivery fails at flush time, the exit code is forced to 2 (deny) before os._exit is called. os._exit bypasses CPython's own shutdown flush sequence, so it cannot be rewritten to 120 afterward. The only two reachable exit codes after this tail are 0 (delivered allow) and 2 (deny — either deliberate or because delivery failed). This is the correct invariant for a security gate.

Watchdog race: _watchdog_fire calls os._exit(2) from a background thread on deadline. If it races with the module tail, whichever wins first exits with 2 — the deny is still delivered. No fail-open race.

atexit skip: os._exit skips registered atexit handlers. The PR description and its commit message document that neither destructive_guard nor killswitch_config registers any — this was verified by inspection. This is the correct tradeoff given that the alternative (CPython's shutdown replacing the exit code) is the vulnerability.

---

_discard_stream helper

def _discard_stream(stream: object) -> None:
    with contextlib.suppress(BaseException):
        null_fd = os.open(os.devnull, os.O_WRONLY)
        try:
            os.dup2(null_fd, stream.fileno())
        finally:
            os.close(null_fd)

Callers: sys.stdout and sys.stderr only — both trusted interpreter-owned objects. No external input reaches this function. os.devnull is a constant. No injection surface.

fd leak: The try/finally ensures null_fd is closed even when os.dup2 or stream.fileno() raises. contextlib.suppress(BaseException) ensures the function itself never raises to its callers. No fd leak path.

Double-null open: If both os.open(os.devnull, ...) calls race (watchdog + module tail calling _discard_stream on the same fd simultaneously), the result is two fd-2 replacements both pointing at /dev/null, which is idempotent and harmless.

---

Redundant-discard removal (26c0b7f)

Removes _discard_stream(sys.stdout) from main's except handler. The commit documents why this call was ineffective (the broken-pipe exception from _decide's print reaches main's handler only when unbuffered; buffered, it escapes the try-block entirely into the module tail). Removing it reduces the mechanism count and leaves a single authoritative owner for the stdout deadline. No security regression — the tail's flush is what closes the vulnerability in both buffered and unbuffered modes.

---

Test environment fix (189658a)

env = dict(os.environ)
env.pop("PYTHONUNBUFFERED", None)

This modifies a shallow copy of os.environ, not os.environ itself. The removed key prevents the child guard process from inheriting unbuffered mode, keeping the test pinned to the buffered-shutdown path it is designed to exercise. No capability expansion, no injection risk.

---

Summary

All three trust boundaries in the new code — the module tail's flush-and-exit, the _discard_stream fd redirect, and the test's env sanitization — are implemented correctly. The fix correctly closes the exit-120 fail-open path reported in #1423, and the test correctly isolates the buffered code path from runner-environment variation. No security issues to report.