Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion plugins/disk-hygiene/.claude-plugin/plugin.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"$schema": "https://json.schemastore.org/claude-code-plugin-manifest.json",
"name": "disk-hygiene",
"version": "0.17.1",
"version": "0.17.2",
"description": "Context-aware disk hygiene for arbitrary directory trees: inventories orphaned and temporary artifacts, classifies evidence into review tiers, and offers exact-path cleanup only after a fresh safety preview and explicit per-tier approval. The target is read-only by default; OS-managed paths, links and mount points, VCS-tracked content, changed entries, and live-handle uncertainty fail closed.",
"author": {
"name": "Melodic Software",
Expand Down
10 changes: 10 additions & 0 deletions plugins/disk-hygiene/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,16 @@
All notable changes to the `disk-hygiene` plugin are documented here. Format follows
[Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning.

## [0.17.2]

### Fixed

- **`_discard_stream` no longer re-closes the fd it just repaired.** When stderr's fd was closed
outright, `os.open(os.devnull)` could return that same fd number; `dup2` was then a no-op and the
unconditional `close(null_fd)` left fd 2 closed again, defeating the null-device redirect. The
guard now skips closing `null_fd` when it is the target fd. Covered by a unit test that closes fd
2 before calling `_discard_stream`.

## [0.17.1]

### Changed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1145,11 +1145,13 @@ def _discard_stream(stream: object) -> None:
stream), and there is nowhere left to report a failure to report.
"""
with contextlib.suppress(BaseException):
target_fd = stream.fileno()
null_fd = os.open(os.devnull, os.O_WRONLY)
try:
os.dup2(null_fd, stream.fileno())
os.dup2(null_fd, target_fd)
finally:
os.close(null_fd)
if null_fd != target_fd:
os.close(null_fd)
Comment thread
kyle-sexton marked this conversation as resolved.


def _watchdog_fire(deadline: float) -> None:
Expand Down
61 changes: 51 additions & 10 deletions plugins/disk-hygiene/skills/clean/scripts/test_hygiene.py
Original file line number Diff line number Diff line change
Expand Up @@ -4851,6 +4851,49 @@ def test_undeliverable_stdout_decision_denies_at_exit_2_in_a_real_process(
self.assertNotEqual(120, proc.returncode)
self.assertIn("could not be written to stdout", stderr)

def test_discard_stream_does_not_leak_when_fileno_raises(self) -> None:
"""fileno() failure must not open null_fd and leak it (#2088).

``_ClosedPipeStderr`` (a ``StringIO`` stand-in) is the in-process shape:
``fileno()`` raises ``UnsupportedOperation``. Resolving ``target_fd``
after ``os.open`` leaves an opened null fd whose ``finally`` never runs
``os.close`` because ``target_fd`` stays unbound.
"""

class _NoFdStream:
def fileno(self) -> int:
raise io.UnsupportedOperation("fileno")

with (
mock.patch.object(guard.os, "open") as open_mock,
mock.patch.object(guard.os, "close") as close_mock,
):
guard._discard_stream(_NoFdStream())
open_mock.assert_not_called()
close_mock.assert_not_called()

def test_discard_stream_keeps_closed_stderr_fd_open(self) -> None:
"""When fd 2 is closed outright, _discard_stream must repair it, not re-close it.

POSIX allocates the lowest free descriptor, so ``os.open(os.devnull)``
can return fd 2 when stderr's fd was closed. ``dup2(2, 2)`` is then a
no-op; closing ``null_fd`` without checking the target re-closes fd 2
and defeats the repair (#1526, #2088).
"""
saved = os.dup(2)
try:
os.close(2)

class _ClosedFdStream:
def fileno(self) -> int:
return 2

guard._discard_stream(_ClosedFdStream())
os.write(2, b"")
finally:
os.dup2(saved, 2)
os.close(saved)

def test_stderr_fd_closed_outright_still_denies_at_exit_2_in_a_real_process(
self,
) -> None:
Expand All @@ -4860,22 +4903,20 @@ def test_stderr_fd_closed_outright_still_denies_at_exit_2_in_a_real_process(
device and `dup2`s it onto the broken fd. When fd 2 is closed
outright rather than left open with a dead reader, `os.open` can
return fd 2 itself (POSIX allocates the lowest free descriptor),
making the `dup2` a no-op -- and the `finally: os.close(null_fd)`
that follows then re-closes fd 2, undoing the very repair it just
made. Against the pre-#1524 module tail (`raise SystemExit(main())`),
making the `dup2` a no-op -- and the `finally: os.close(null_fd)` that
followed then re-closed fd 2, undoing the very repair it just made.
`_discard_stream` now skips that close when `null_fd` is the target fd
(#2088). Against the pre-#1524 module tail (`raise SystemExit(main())`),
the interpreter's own shutdown flush then hits that closed fd and
CPython rewrites the exit status to 120: non-blocking under
PreToolUse, so the destructive command would run even though the
guard had decided to deny it (#1526, reproduced against merged
`efb6c271`).

This module's tail no longer depends on `_discard_stream` actually
repairing the fd for the exit code to survive: every path now ends in
`os._exit`, which skips the interpreter's normal shutdown flush --
the mechanism `120` comes from -- entirely. So the #1526 trigger is
closed as a structural side effect of the #1524 fix, not by touching
`_discard_stream` itself (which still has the self-undoing dup2/close
pattern described above; nothing downstream depends on it working).
This module's tail ends in `os._exit`, which skips the interpreter's
normal shutdown flush -- the mechanism `120` comes from -- so the exit
code no longer depends on `_discard_stream` working; the subprocess
case below still guards both the deny path and the fd repair.
"""
env = dict(os.environ)
script = str(SCRIPT_DIR / "destructive_guard.py")
Expand Down
Loading