feat(disk-hygiene): attended deep inventory with justified KEEP reasons

[kyle-sexton]

Refs: #5221

Summary

/disk-hygiene:clean gains an attended, report-only deep inventory. A home-directory target (so a bare /disk-hygiene:clean ~), or any target with --deep, runs the read-only inventory subcommand before any scan. It lists every entry with name, extension, size, mtime, owner, producer, category, disposition and reason. Every KEEP needs a specific reason: an empty reason always fails the report, and a category-only reason ("tool-managed", "OS-owned", "managed by ") fails unless it names a tool ("managed by ") that the row's evidence shows still references the entry.

Fix

• Engine: inventory subcommand, row schema, named categories (superseded versions, unreferenced plugin cache versions, /tmp by producer, orphaned transcript dirs, dangling symlinks) and the KEEP-reason validator.
• Validator: an empty KEEP reason fails even when the evidence dict carries a tool and a reference, because an empty reason names no tool. "tool-managed" and "OS-owned" never pass, with or without evidence. "managed by " passes only when the evidence's tool is that named tool and its references is set. Tests cover the empty reason with evidence, a reason that names no tool with evidence, and a different tool's evidence.
• Destructive guard: inventory joins _READONLY_ENGINE_SUBCOMMANDS (scan, inventory, preview, handoff-verify, catalog) beside main's _MUTATING_ENGINE_SUBCOMMANDS (apply, handoff-apply), and the kill-switch denial text names it. A test pins the denial text.
• Docs: SKILL.md names --deep in argument-hint and Arguments, and its Deep inventory section says a bare home target runs inventory first. The large-root paragraph links to it, so the bounded --max-depth 1 scan is the step that follows for removal candidates, not the first step. reference/scan-flags.md holds the columns, categories and the KEEP rule; reference/safety-model.md holds the report-only boundary. The --sizes-only text in both follows main (fix(disk-hygiene): gate --sizes-only behind the large-scan confirmation and keep no per-path entries #5587): it goes through the large-scan question. SKILL.md is 498 lines (cap 500). The plugin README.md documents --deep.
• Category fixes: a release outranks its own prerelease; a version a symlink points at (beside the versions, in their parent, or in ~/.local/bin and ~/bin) is KEEP; a plugin cache candidate carries its .orphaned_at marker age and whether it is past the 14-day sweep window, and says when the registry records no install so the sweep does not run; tmp-producer covers /tmp itself, not $TMPDIR. ORPHAN_SWEEP_DAYS and PROJECT_NAME_CAP carry a verification stamp and a recheck trigger.
• Where /proc is unreadable (macOS, Windows), running_paths returns None and rows that would be CANDIDATE on "no running process uses it" are UNKNOWN, with a reason that says the table was not read.
• --execute, the low-signal keep rule and the confirmation gates are unchanged; any removal still goes through scan, preview and the removal approval.
• repo-hygiene:clean gets a one-line pointer to deep mode and no scanner.
• Eval ids 16 (--deep ~ lists with justified KEEP, deletes nothing) and 17 (bare ~ runs the deep inventory first, no bounded scan first).
• disk-hygiene 0.36.0 to 0.37.0 (main took 0.35.0 to 0.36.0 for handoff-apply, two fixes and /disk-hygiene:audit while this was open) and repo-hygiene 0.18.0 to 0.18.1, each with a CHANGELOG entry.

Known limits, all report-only (deletion stays gated): a version selected by a file rather than a symlink (an nvm alias, .tool-versions) is not seen, so its row can be a CANDIDATE for a version in use; the tmp-producer category yields rows only when the target is /tmp or contains it (so a home inventory has none; --deep /tmp covers it) and none on macOS, where /tmp is a link and /private/tmp an OS-managed root.

Verification

Run on the head b22f973b0, which merges current origin/main (disk-hygiene 0.36.0); the only conflict was the disk-hygiene CHANGELOG, resolved by renumbering this release to 0.37.0:

• The seven disk-hygiene *.test.sh wrappers: all pass (hygiene.test.sh 638 tests, 1 skipped; deep_inventory.test.sh 50 tests). All repo-hygiene *.test.sh wrappers pass too.
• scripts/run-ruff.sh check plugins/disk-hygiene: all checks passed; format --check on deep_inventory.py, test_deep_inventory.py, destructive_guard.py and engine_grammar.py: already formatted.
• scripts/check-changed-skills.sh "$(git merge-base origin/main HEAD)": 3 skills checked, 0 failed.
• markdownlint-cli2 on SKILL.md, scan-flags.md, safety-model.md, README.md and the disk-hygiene CHANGELOG: 0 issues.
• scripts/check-changelog-parity.sh --check, --check-order and --check-bump origin/main: pass.
• scripts/validate-plugins.sh: all manifests and the catalog validated.
• Every shebang file in the diff is mode 100755.

CI: lint and ci-status were red on d0f85a3fb for the executable bit on deep_inventory.py and test_deep_inventory.py and for Linux user paths in the test fixtures. Both are fixed, and on 6e852df64 lint, lint-2, test-linux (0-3), hook-utils, managed-files-guard, ci-lanes and ci-status pass.

Related

Closes is not used because two items of the owner's decision on #5221 are still the owner's call:

• Item 3, reuse the feat(claude-ops): report unreferenced plugin cache versions in audit-install-state #5420 logic: plugin_cache_versions in deep_inventory.py applies the audit-install-state registry rule (_install_paths, _resolve, the registry-doubt rules) as a plugin-local copy, because a plugin cannot import a sibling. It is not in scripts/cross-plugin-source-registry.txt and no sync-*.sh gate covers it, so nothing catches drift between the two copies; the tests here pin this copy only. The owner decides between vendoring it through a sync-*.sh script plus a registry entry and accepting the separate copy.
• Item 6, coordination with disk-hygiene: whole-machine audit driven by an orchestrator and subagents (no model-invocable audit, 250k cap needs manual splitting, guard and output gaps) #5214 and disk-hygiene: add a gated managed-state lane with a bundled owner registry #4006: they are to reuse the row schema defined in deep_inventory.py and documented in reference/scan-flags.md#--deep; their scope is not folded in. No comment was posted on either issue from this PR, so a note there is the owner's call.

Flipping this PR to ready also stays with the owner.

🤖 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-09-30T19:20:36.745545Z	3b8e1dd	Draft marked ready

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

---

CI Code Review

• Gather context (PR diff, CLAUDE.md, REVIEW.md)
• Review engine changes (deep_inventory.py, engine_grammar.py, destructive_guard.py, hygiene.py)
• Review tests (test_deep_inventory.py, deep_inventory.test.sh, test_hygiene.py)
• Review docs (SKILL.md, scan-flags.md, safety-model.md, README.md, CHANGELOG.md)
• Adversarially verify candidate findings
• Post findings (inline + summary)

This repo runs a separate security lane (.github/workflows/claude-security-review-hosted.yml), so security-scoped findings are intentionally left to that lane and omitted here.

Scope of this review: git diff origin/main...HEAD (17 files, +1978/-21) at 3b8e1ddbb81ebb7ff9048c0e72040e3fe27b03c6, focused on the new inventory subcommand, deep_inventory.py's categorization/validator logic, the destructive-guard wiring, and the doc/eval/changelog updates.

Findings

One inline finding posted on plugins/disk-hygiene/skills/clean/scripts/hygiene.py (lines 4949–4978, run_inventory):

🔴 Important — the two related report files (rows_path, summary_path) aren't written with the atomicity convention this same script already uses for its other two-related-files case. catalog writes its json/markdown pair through write_text_atomic (temp file + os.replace); run_inventory instead streams rows directly to rows_path's final name over the course of the whole walk, and summary_path via a direct (non-renamed) write_json. If the walk raises partway through a long --deep ~ scan (caught by main()'s broad except (OSError, subprocess.SubprocessError)), rows_path is left on disk as a truncated JSONL file with no matching summary and no marker that it's incomplete — and it will later be walked as an ordinary unclassified entry by a subsequent inventory. Detail and a suggested fix are in the inline comment.

What I verified statically (not executed)

I traced the validate_report KEEP-reason validator (_category_only, _shows_reference, _NAMED_TOOL/_CATEGORY_PHRASE regexes) against every test case in test_deep_inventory.py by hand, including the substring-false-match case ("managed by promise" vs. tool "mise") and the multi-clause splitting (;, ,, ., and) — all check out. I also traced superseded_versions (release-outranks-prerelease ordering, symlink/running-process rescue, running=None → UNKNOWN propagation), plugin_cache_versions (registry-doubt branching, .orphaned_at sweep-window math), tmp_entries, project_transcripts/decode_project, the destructive-guard wiring (_READONLY_ENGINE_SUBCOMMANDS, grammar-derived argparse), and the doc cross-references (SKILL.md line cap, scan-flags.md/safety-model.md anchors, version bumps vs. CHANGELOG headers). No other correctness issues found.

Note: I was not able to execute the Python test suites or scripts/run-ruff.sh in this sandboxed environment (tool execution was denied), so the PR body's own "all pass" test/lint claims are author-reported and unverified by me — I relied on static reading of the code and tests instead.

[claude]

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

---

Security review complete

• Gather context (PR diff, changed files)
• Review deep_inventory.py (new inventory engine)
• Review destructive_guard.py / engine_grammar.py changes (readonly/mutating subcommand gating)
• Review hygiene.py changes (inventory wiring)
• Check instruction-surface deletions (SKILL.md, safety-model.md, scan-flags.md)
• Post findings

No security issues found.

Scope: the new inventory subcommand and its wiring — deep_inventory.py, the run_inventory addition in hygiene.py, and the guard/grammar extensions in destructive_guard.py and engine_grammar.py.

What I checked:

• Report-only boundary holds. inventory only reads (os.lstat/os.scandir with follow_symlinks=False, so size walks never cross a symlink) and writes its own JSONL report. Nothing in the new code deletes, moves, or mutates. preview/apply still require a snapshot/plan, and INVENTORY_REPORT_KIND keeps the inventory report from being mistaken for one.
• Guard/grammar stay in lock-step. inventory is added to both engine_grammar.SUBCOMMANDS and destructive_guard._READONLY_ENGINE_SUBCOMMANDS; a test (test_only_the_mutating_subcommands_are_left_off_the_read_only_allowance) pins that every grammar subcommand except apply/handoff-apply is in the read-only allowance, and another pins the updated kill-switch denial text. The new subcommand takes no --execute/--report/--plan, confirmed by test_inventory_takes_deep_but_never_an_execute_flag.
• State containment unchanged. run_inventory writes through the existing state_output_path, which still fails closed without --data-root and refuses any path outside the guard-validated data root or inside the plugin install directory (hygiene.py#L365-L379). --data-root on inventory still carries the external_check the guard supplies, same as every other state-writing subcommand.
• No new injection surface. No subprocess/shell calls were added; all new I/O is direct filesystem access (os, pathlib) on a target the existing has_linkish_component/is_os_managed_target checks still gate before any walk.
• KEEP-reason validator regexes (_NAMED_TOOL, _CATEGORY_PHRASE) are bounded ({0,2} repetition, no nested unbounded quantifiers), so no ReDoS concern.
• Instruction-surface diff (SKILL.md, safety-model.md, scan-flags.md, README.md): purely additive — a new "Deep inventory is report-only" section reinforces the existing report-only/no-mutation boundary rather than loosening it. No guardrail was removed, narrowed, or softened.

Out of scope per this lane's charter (and not re-reported here): GitHub Actions/workflow hardening (zizmor's lane — no workflow files changed in this PR), and general code quality/style (/review:code-review's lane).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3b8e1ddbb8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

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

---

Code Review (/review:code-review)

• Gather context (PR diff, CLAUDE.md, AGENTS.md, rules)
• Invoke /review:code-review and follow its procedure exactly
• Post inline findings
• Post summary

---

Scope: commit 619360e (fix(disk-hygiene): stop the inventory at bind mounts, count open files, write the report atomically), the only content pushed since the lane's last successful review on 3b8e1dd. Isolated to this PR's own diff via git diff 7106c28..619360e (matches the PR's file list, +2038/-21) since origin/main moved past this branch's merge base and the local checkout's history is shallow/partial for a direct origin/main...HEAD diff.

This commit is a direct response to the two Codex P1/P2 findings and the Claude atomicity finding from the prior review round:

• Bind mounts on the same device are now caught via /proc/self/mountinfo (new mounts param threaded through deep_inventory.inventory_rows/_tree_size), not just st_dev.
• Open file descriptors are now included in running_paths() (scans /proc/<pid>/fd alongside exe/cwd), so a /tmp entry a process holds open no longer reads as an unused candidate.
• Atomicity: rows_path now streams to a *.tmp sibling and is os.replaced into place only once the walk finishes (cleaned up via finally: partial.unlink(missing_ok=True) on any exception), and summary_path goes through the existing write_text_atomic helper instead of the non-atomic write_json. Verified against the new test_an_interrupted_walk_leaves_no_partial_report test, which confirms no file lands in <data-root>/inventory/ when the walk raises partway through. All three fixes check out as intended and are each covered by a new, correctly-targeted test.

Findings

One inline finding on plugins/disk-hygiene/skills/clean/scripts/hygiene.py (line 4951, run_inventory):

🔴 Important — mounts = frozenset(str(p) for p in linux_mount_points()[0]) takes only the points and discards linux_mount_points()'s second return value (the error string). On Linux, when /proc/self/mountinfo can't be read, that call returns (set(), "cannot read ..."); here the error is silently dropped, mounts becomes empty, and the walk falls back to st_dev-only detection — reintroducing the same-device bind-mount gap this very commit fixes, with the summary still reporting status: inventory-complete. Every other caller of linux_mount_points() in this file (apply_plan's initial and per-removal checks) treats a non-empty error as something that must raise or produce a visible "mount-state-unverified" outcome rather than be silently swallowed; scan --sizes-only's new rollup_precision: partial field follows the same convention for this exact error. No test covers run_inventory with a mount-read error (the new test_mount_point_on_the_same_device_is_not_entered only mocks the success case). Detail and a suggested direction (raise, or surface a mount_state field, matching the running=None → UNKNOWN pattern already used for the process table) are in the inline comment.

What I verified statically (not executed)

Traced running_paths()'s new /proc/<pid>/fd scan against test_open_descriptors_count_as_use (including the non-path socket:[123] case, which the docstring correctly notes can never match a real row) and the pre-existing PermissionError-on-another-user's-process behavior, now explicitly documented in the docstring. Traced the mounts threading through _tree_size and inventory_rows's not_entered branch against test_mount_point_on_the_same_device_is_not_entered. Traced the atomic-write path (partial open → replace → finally unlink) against test_an_interrupted_walk_leaves_no_partial_report, including the redundant-but-harmless unlink(missing_ok=True) after a successful os.replace (file is already gone by then, so it's a no-op). Confirmed the CHANGELOG (0.38.0) and scan-flags.md/README.md doc updates in this commit's own diff match the new behavior. Did not re-verify the KEEP-reason validator, category logic, or destructive-guard wiring reviewed in the prior round, since this commit doesn't touch them.

I was not able to execute the Python test suite or scripts/run-ruff.sh in this environment (no tool access to run them here), so the earlier round's author-reported "all pass" claims remain unverified by me for this commit too — I relied on static reading of the diff and the new tests.

[claude]

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

---

Security review complete

• Gather context (PR diff, CLAUDE.md, custom instructions)
• Invoke /review:security-review per custom instructions
• Post findings

Scope: commit range 3b8e1dd...619360e (the fix commit "stop the inventory at bind mounts, count open files, write the report atomically" merged since the last security review on this PR), focused on deep_inventory.py (bind-mount detection, fd-based process-use), hygiene.py (run_inventory's new atomic-write and mount-detection wiring), and the accompanying tests/docs.

Findings

One inline finding posted on plugins/disk-hygiene/skills/clean/scripts/hygiene.py#L4951:

🟠 IMPORTANT — the new bind-mount stop silently fails open when /proc/self/mountinfo is unreadable. mounts = frozenset(str(p) for p in linux_mount_points()[0]) discards the error half of linux_mount_points()'s (points, error) return. When mountinfo can't be read, that call returns (set(), "cannot read ..."), and this line quietly treats the machine as mount-free — collapsing back to the device-only check that Codex's P1 finding on this PR showed misses a same-device bind mount. Every other mount-consuming call site in this file (apply_plan, its mid-removal recheck, scan_tree) either fails closed or surfaces the error into the report; run_inventory is the only one that swallows it, with no test covering that path. Detail and a suggested fix (surface the error into the inventory summary, mirroring scan_tree) are in the inline comment.

What I verified

• The atomic-write fix for rows_path/summary_path (streaming to a secrets.token_hex(4)-suffixed temp file, os.replace into place, finally: unlink(missing_ok=True)) correctly mirrors the pre-existing write_text_atomic pattern used by catalog — this resolves the prior review's finding about a truncated report surviving an interrupted walk. The temp-file naming uses a random token, so it isn't practically guessable/symlink-attackable ahead of creation.
• running_paths()'s new /proc/<pid>/fd scan is read-only, bounded by normal process-permission visibility (another user's /proc/<pid>/fd raises OSError and is skipped), and a non-path descriptor target (socket:[...], pipe:[...]) can only produce a harmless false-miss in _in_use's prefix match, never a false keep of an unrelated path.
• _tree_size's new mounts parameter and inventory_rows's mount-point check are otherwise correctly wired (string-keyed comparison against linux_mount_points()'s absolute paths, consistent with the existing mount_state convention elsewhere in the file).
• No new subprocess/shell surface, no new injection surface, and the report-only boundary (no delete/move/write outside the guarded data root) still holds.

Everything else in the diff (docs, the merge-brought-in files from origin/main, test additions) is non-functional or already covered by other lanes.

[claude]

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

---

Code Review (/review:code-review)

• Gather context (PR diff, CLAUDE.md, prior review rounds)
• Determine scope: what changed since the last reviewed commit
• Review the new changes
• Adversarially verify candidate findings
• Post findings (inline + summary)

---

Scope: this lane already completed two full review rounds on this PR's own logic, at 3b8e1dd and again at 619360e (the bind-mount/fd-use/atomicity fix commit). Comparing 619360e against the current head 84156c9, this sync added exactly two commits to the branch:

• e8a4228 — already merged into origin/main independently as #5615 (the /disk-hygiene:check skill); it is not part of this PR's own diff (git diff origin/main...HEAD excludes it, matching the PR's file list).
• 84156c9 — a merge of origin/main into this branch, which only touched this PR's own files to renumber the version/changelog around that already-merged commit: plugin.json 0.38.0 → 0.39.0, a CHANGELOG.md reflow (the disk-hygiene entry becomes 0.39.0, with 0.38.0 now documenting #5615's /disk-hygiene:check), and a README.md wording tweak pointing to /disk-hygiene:check. No Python, test, or skill-logic file in this PR changed.

I confirmed hygiene.py, deep_inventory.py, destructive_guard.py, engine_grammar.py, and every repo-hygiene file are byte-identical between 619360e and 84156c9 (git diff 619360e..84156c9 touches none of them), and the renumbered CHANGELOG.md/plugin.json stay internally consistent (top entry 0.39.0 matches the manifest version).

No new findings. There is no new review surface: the substantive code was already reviewed in full at 619360e, and this sync is a mechanical version/changelog renumbering from the merge, not a logic change.

For completeness: the mounts = frozenset(str(p) for p in linux_mount_points()[0]) finding this lane already raised twice against hygiene.py:4951 (silently discarding linux_mount_points()'s error, which can fail the bind-mount detection open) is unchanged and still outstanding in this unmodified file — not re-reported here since the code hasn't moved since it was flagged, to avoid duplicating an existing thread. See hygiene.py#L4951.

[claude]

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

---

Security review complete

• Gather context (PR diff since last reviewed head, changed files)
• Determine what's new since the last security review on this PR
• Review new/changed security-relevant code
• Check instruction-surface diffs
• Post findings

Scope: git diff origin/main...HEAD at 84156c929ab64b7fbe404cd55a945ddd84953840 — 17 files, +2038/-21, which matches this PR's own diff exactly. This synchronize event brought in no new PR-owned content: the only commits added since the last security-review pass (on 619360e) are e8a4228 (an already-merged, independently-reviewed main PR, #5615) and the merge commit 84156c9 that pulls it in. git diff --stat origin/main...HEAD is identical in file list and line counts to the PR's own metadata both before and after this sync, confirming nothing new to this branch was introduced.

Findings

One finding, still open from the prior review round — the diff at this location is byte-for-byte unchanged since it was first flagged on commit 619360e:

🟠 IMPORTANT — hygiene.py#L4951 (run_inventory) silently fails open when /proc/self/mountinfo is unreadable.

mounts = frozenset(str(p) for p in linux_mount_points()[0])

This discards the error half of linux_mount_points()'s (points, error) return. When mountinfo can't be read, mounts silently becomes empty — exactly as if the host had no bind mounts — and the walk falls back to the device-only check alone, reopening the same-device bind-mount gap that Codex's P1 finding on this PR required fixing in the first place, with status: inventory-complete still reported and no test covering this path. Every other linux_mount_points() caller in this file (apply_plan, its mid-removal recheck, scan_tree) surfaces or fails closed on this error; run_inventory is the only one that swallows it. Posted inline with detail and a suggested fix (surface the error into the summary or raise, mirroring the existing running=None → UNKNOWN convention used for the process table).

This is the same finding reported on the prior round for commit 619360e (before the two Codex findings and the atomicity finding were fixed in that same commit) — those three were addressed, this one was not, and the code is unchanged at this line on the current head.

What else I checked

Since the diff is unchanged from the last full pass, I re-confirmed rather than re-deriving: the report-only boundary still holds (no delete/move/write outside the guarded data root), the atomic-write fix for rows_path/summary_path and the /proc/<pid>/fd open-descriptor fix are both still correctly in place and unaffected by the merge, the destructive-guard/grammar wiring (inventory in _READONLY_ENGINE_SUBCOMMANDS) is unchanged, and no new subprocess/shell surface or injection vector was introduced. The instruction-surface diffs (SKILL.md, safety-model.md, scan-flags.md, README.md) remain purely additive — no guardrail removed, narrowed, or softened.

Out of scope per this lane's charter: GitHub Actions/workflow hardening (zizmor's lane — no workflow files changed) and general code quality/style (/review:code-review's lane).

[claude]

Claude finished @kyle-sexton's task in 52s —— View job

---

Security review complete

• Gather context (PR diff, CLAUDE.md, custom instructions)
• Determine scope: what changed since the last reviewed commit (84156c9)
• Review new/changed security-relevant code
• Check instruction-surface diffs
• Post findings

No security issues found.

Scope: git diff origin/main...HEAD at 2fb30c0370fdabb188b20f196005efa7e50343f8 — 17 files, +2059/-21, matching this PR's own diff exactly. This lane already completed three full review rounds on this PR (at 3b8e1dd, 619360e, 84156c9). Since 84156c9, the branch gained two commits: 27206c9 (already merged into origin/main independently as a plugins/planning fix, and excluded from this PR's own diff — confirmed by git diff origin/main...HEAD --stat matching the PR's file list exactly) and 19eb0aa (this PR's own fix commit), plus the merge commit 2fb30c0 that pulls 27206c9 in.

19eb0aa — "report an unreadable mountinfo in the inventory summary" is the direct fix for the one finding this lane has now raised three times against hygiene.py: run_inventory was silently discarding linux_mount_points()'s error half, which let the bind-mount detection fail open with no indication in the report. The fix:

• Captures both return values: mount_points, mount_error = linux_mount_points() (hygiene.py#L4951).
• Surfaces mount_error as a new mount_state_error field in the inventory summary (hygiene.py#L4981), null when mountinfo was read successfully.
• Documents the field in scan-flags.md: "when that cannot be read the summary carries the reason in mount_state_error."
• Two new tests (test_unreadable_mountinfo_is_reported_not_hidden, test_readable_mountinfo_reports_no_error) cover both the error and success paths by mocking linux_mount_points.

This resolves the silent-failure concern: a reader of the report can no longer mistake a walk that ran without bind-mount detection for one that ran with it. The walk still proceeds and still reports status: inventory-complete when there are no row-validation failures (it doesn't fail closed), but that's a report-only inventory tool surfacing a data-quality caveat rather than a security control being bypassed — deletion stays gated behind scan/preview/apply regardless of what inventory reports, so there's no exploit path through this. No new issue introduced by the change itself (no new subprocess/shell surface, no new injection surface, report-only boundary unaffected).

Everything else in the diff is unchanged since the last full pass at 84156c9 (already reviewed) or is the independently-merged #5470 planning fix, which is not part of this PR's own diff.

Out of scope per this lane's charter: GitHub Actions/workflow hardening (zizmor's lane — no workflow files changed) and general code quality/style (/review:code-review's lane).

[claude]

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

---

Code Review (/review:code-review)

• Gather context (PR diff, CLAUDE.md, prior review rounds)
• Determine scope: what changed since the last reviewed commit
• Review the new changes
• Adversarially verify candidate findings
• Post findings (inline + summary)

---

Scope: this lane has now reviewed this PR's own diff across four prior rounds (3b8e1dd, 619360e, and confirmed-unchanged at 84156c9 twice). Comparing 84156c9 against the current head 2fb30c0, the merge added one commit relevant to this PR's own files:

• 19eb0aa — fix(disk-hygiene): report an unreadable mountinfo in the inventory summary, touching hygiene.py (+4/-1), reference/scan-flags.md (+6/-2), and test_hygiene.py (+17). 27206c9 (a planning plugin fix, fix(planning): one escaped named-field ledger row grammar #5470) and the bulk of the merge's file list are from origin/main and outside this PR's own diff — confirmed via git diff origin/main...HEAD --stat, which still matches the PR's reported 17 files / +2059/-21 exactly.

19eb0aa is a direct fix for the one finding this lane raised on three consecutive rounds (619360e, and twice more unchanged through 84156c9): run_inventory's mounts = frozenset(str(p) for p in linux_mount_points()[0]) was discarding linux_mount_points()'s error half, so an unreadable /proc/self/mountinfo silently fell back to device-only bind-mount detection with no indication in the report.

What changed and why it resolves the finding

hygiene.py now captures both elements of the tuple:

mount_points, mount_error = linux_mount_points()
mounts = frozenset(str(p) for p in mount_points)

and the summary dict gains a mount_state_error field (None on success, the reason string otherwise), populated from mount_error regardless of whether the walk itself hit any validation failures. This is exactly the "surface it in the summary" direction this lane suggested each round (the alternative being to fail closed, which the author reasonably didn't choose given inventory is report-only and a reader can now see the caveat). reference/scan-flags.md documents the new field and clarifies "only a mount on another device is skipped" when mountinfo can't be read.

I traced this against linux_mount_points(): it always returns a (set, str|None) tuple (catches OSError/UnicodeError/ValueError internally, and returns (set(), None) outright on non-Linux), so run_inventory can't raise from this call — mount_error is reliably populated or None. The two new tests (test_unreadable_mountinfo_is_reported_not_hidden, test_readable_mountinfo_reports_no_error) correctly mock linux_mount_points for both branches and assert on summary["mount_state_error"].

No new findings. The fix is narrowly scoped, correctly wired, and matches the fix direction requested across the prior rounds. I did not re-derive findings on the rest of the diff (bind-mount stop, fd-based process-use, atomic report writes, KEEP-reason validator, category logic, destructive-guard wiring) since none of that code changed in this round — those were already reviewed and confirmed clean in the earlier rounds on this PR.

Note on execution

I was not able to execute the Python test suite (test_hygiene.py::InventoryCommandTests) or scripts/run-ruff.sh in this sandboxed environment — tool execution was denied. I relied on static reading of the diff, the two new tests, and linux_mount_points()'s implementation, as in every prior round of this review.