fix(disk-hygiene): reason about volume-root rejection instead of blanket-denying non-OS drives

[kyle-sexton]

Note

Supersedes #1026 — same fully reviewed, CI-green content rebuilt as a single commit on current
main (version reconciled to 0.6.4 over #1014's 0.6.3). See #1026 for the complete review history;
all its review threads were resolved there.

Summary

disk-hygiene's clean skill rejected any filesystem/volume root as an audit target purely
structurally, with no reasoning about the volume's purpose. This wrongly blocked a legitimate non-OS
volume such as a Windows Dev Drive (D:\, ReFS, no OS content) with no path to override,
interrogate, or explain. Root classification is now reasoned: a genuine OS-managed root stays
denied; a non-OS volume root becomes a valid target that composes with the large-target scan gate.

Fixes #984

Root cause (corrected from the issue's premise)

The issue cited hard_protection() (the path.parent == path branch) as the blanket rejection.
Empirically, that branch is dead code for drive roots in the live call graph — candidates are
validated non-root descendants of the target, so it never fires on a drive root. The real
user-facing block on Windows was the mount-point check in main() scan: every Windows drive
letter is os.path.ismount() == True, so D:\ was rejected as a "mount point" before the
root check the issue points at (verified live: os.path.ismount('D:/') == True; scanning D:\
returned {"error": "mount points are not valid audit targets"}). Fixing only the cited line would
have left the bug unfixed. preview() applied the same checks in a different order, a latent
inconsistency.

Fix

• Reasoned OS-managed classification (is_os_managed_target): a root is denied when it is within
  an OS-managed root (full system_roots() set) or holds an existing OS-install marker
  (os_drive_markers). The OS-install markers deliberately exclude the per-volume metadata every
  Windows volume carries (System Volume Information, $Recycle.Bin) — a Dev Drive has those too,
  so counting them would misclassify every drive root as OS-managed. WINDOWS_VOLUME_SYSTEM_NAMES is
  split into WINDOWS_OS_DRIVE_MARKERS ∪ WINDOWS_PER_VOLUME_METADATA; the union is unchanged, so
  per-entry protection of those folders on every drive is preserved.
• Mount rejection scoped to non-root mounts (mounted and not is_volume_root(target)): a
  volume-root target (inherently a mount point) falls through to the reasoned root logic; nested and
  bind mounts stay hard-blocked — the real cross-boundary danger.
• Scan and preview unified to one unverified → OS-managed → non-root-mount target-check order,
  removing the latent ordering inconsistency.
• Composes with the large-target gate (disk-hygiene: no engine-level large-target confirmation gate; relies entirely on the invoking agent remembering --max-depth #985 / feat(disk-hygiene): gate unbounded scans of known-large targets at the engine #1010), reusing its vocabulary — no parallel gate.
  large_scan_reasons now names a non-OS volume root a known-large root (reason
  non-os-volume-root), so an unbounded whole-volume walk returns large-target-confirmation-required
  (exit 5) unless bounded with --max-depth or confirmed with --confirmed-large-scan. No
  destructive_guard change is needed — feat(disk-hygiene): gate unbounded scans of known-large targets at the engine #1010 already permits that flag.
• hard_protection's (dead-for-drive-roots) branch is made reasoned rather than blanket, for
  faithfulness, and locked with a direct unit test.

Deletion safety is unchanged: per-entry OS-managed / mount / VCS / identity protections, the preview,
and per-tier approval all still gate any removal. The design degrades safely — if OS-drive
markers were somehow absent on a real OS drive, the result is confirmation-required plus full
downstream gating, never a silent dangerous delete. A stray non-OS D:\Windows folder classifies the
volume as OS-managed (conservative false-positive deny), which is acceptable.

Test evidence

• test_hygiene.py: 101 tests pass (added classification, os_drive_markers metadata-exclusion,
  hard_protection reasoning, non-root-mount rejection, OS-managed deny in scan+preview, and
  non-OS-volume-root large-target-gate tests; existing system_roots protection test stays green).
• ruff check clean; python -m py_compile clean.
• Repo gates green: check-changelog-parity --check-bump, check-changed-skills (skill-quality
  static contract — 0 errors), validate-plugins (manifests + catalog), check-orphaned-fixtures.
• Live verification on the audited machine: C:\ → denied (OS-managed roots are not valid audit targets); D:\ (Dev Drive) with no bound → large-target-confirmation-required (reason
  non-os-volume-root); D:\ --max-depth 1 --confirmed-large-scan → scan-complete.

Independent review

Reviewed by a fresh-context reviewer (rationale withheld, adversarial). Verdict: classification core
sound — every branch hand-traced, the pathlib root-identity edge verified empirically, no path where
an OS drive slips through as non-OS or a Dev Drive is wrongly denied, and no deletion-safety
regression. No code changes required. Three low-confidence advisories, noted for transparency (no
action taken):

1. hard_protection's volume-root branch is unreachable on the live call path (candidates are
   validated non-root descendants) — kept as defense-in-depth and locked by a direct unit test.
2. system_roots() and os_drive_markers() each call windows_drive_roots(), so a scan makes a
   couple of redundant GetLogicalDrives syscalls per invocation — negligible, not hot-path.
3. Known limitation: a stray Recovery (or other OS-install-marker-named) folder on a non-boot drive
   would classify that volume as OS-managed. This fails safe (over-restrictive deny, never an unsafe
   allow) and is the conservative side to err on.

Fresh-docs note

Per the repo's fresh-docs mandate: this change is Python engine logic that reuses existing status
vocabulary and adds no new plugin component type, manifest field, hook, or skill surface — no official
Claude Code plugin-schema page is load-bearing here, so none is cited. The only external behavior
reused (--confirmed-large-scan, large-target-confirmation-required) is this plugin's own #1010
contract, referenced directly.

Related

• disk-hygiene: guard hook fails to launch on skill-frontmatter hooks referencing ${CLAUDE_PLUGIN_DATA} — live fail-open on 0.4.6 #983, disk-hygiene: no engine-level large-target confirmation gate; relies entirely on the invoking agent remembering --max-depth #985, disk-hygiene: no handoff pointer to source-control:worktree for VCS worktree directories #986 — sibling issues from the same disk-hygiene audit, addressed in parallel PRs.
• disk-hygiene: no engine-level large-target confirmation gate; relies entirely on the invoking agent remembering --max-depth #985 (PR feat(disk-hygiene): gate unbounded scans of known-large targets at the engine #1010) — the companion large-target scan gate; this PR builds on its
  large_scan_reasons / --confirmed-large-scan mechanism rather than adding a parallel gate.

🤖 Generated with Claude Code

https://claude.ai/code/session_01SFq1q99cNeZjKhDzHv2BQw

[claude]

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

---

PR Review: fix(disk-hygiene): reason about volume-root rejection

• Gather context and read changed files
• Review hygiene.py — classification logic, mount/root reasoning
• Review test_hygiene.py — coverage and correctness
• Review documentation changes (SKILL.md, safety-model.md, evals.json)
• Review manifest and changelog
• Synthesize findings and post review

---

Overview

This PR is well-designed and correctly addresses the root cause. The bug was not where the original issue assumed (hard_protection()'s parent == path branch), but in main()'s mount-point check firing on every Windows drive letter before the root check could be reached. Both the issue diagnosis and the fix are sound.

The implementation has no blocking defects. Three low-severity observations below, all of which were already noted in the independent review (none require action).

---

Classification core (hygiene.py)

WINDOWS_OS_DRIVE_MARKERS / WINDOWS_PER_VOLUME_METADATA split (lines 32–46)

The split is semantically correct. The union is unchanged (WINDOWS_VOLUME_SYSTEM_NAMES), so per-entry protection of $Recycle.Bin and System Volume Information on every drive is fully preserved. Only the OS-drive-classification set is narrowed.

is_os_managed_target() (lines 293–326)

The two-direction logic is correct:

1. Target inside a system root: uses the full system_roots() set (catches C:\Windows\Temp, /etc/x, etc.)
2. OS marker exists inside target: uses the narrow os_drive_markers() set with an existence gate (marker_abs.exists()). The existence gate is the critical piece — os_drive_markers() synthesizes per-drive paths for all logical drives without probing disk, so C:\Windows and D:\Windows are both synthesized; the existence check ensures only the marker present on the target drive is counted.

Cross-drive contamination is also prevented: is_within(Path('C:/Windows'), Path('D:/')) raises ValueError in os.path.commonpath (different drives), which is_within() catches and returns False. Verified correct.

os_drive_markers() on POSIX (lines 257–285)

On non-Windows, it delegates to system_roots(). That makes the second check in is_os_managed_target() a redundant superset of the first for POSIX. Not a bug — / is correctly classified OS-managed (both /bin ∈ system_roots and /bin exists in /), and /home/user is not (no system root is within it) — just a harmless extra loop.

Mount-rejection scoping (scan line 1693, preview line 1273)

if mounted and not is_volume_root(target):
    raise HygieneError("mount points are not valid audit targets")

Correct. A volume root is inherently a mount point (os.path.ismount on Windows, appears in /proc/self/mountinfo on Linux); gating on not is_volume_root lets it fall through to the reasoned OS-managed check while preserving the nested/bind mount rejection.

The hard_protection() mount exemption for admitted volume roots (lines 344–358) is also correct: when walking up from a child to a volume-root target, hitting current == target with is_volume_root(target) True skips both nested-mount-point and target-is-mount-point reasons. This prevents every descendant of an admitted Dev Drive scan from being flagged. Verified by test_hard_protection_exempts_admitted_volume_root_from_mount_reason.

Scan/preview ordering is now unified — both follow unverified → OS-managed → non-root-mount, removing the latent ordering inconsistency noted in the PR.

---

Test coverage (test_hygiene.py)

Coverage for the new functionality is comprehensive:

Test	What it pins
test_os_drive_markers_exclude_per_volume_metadata	$Recycle.Bin/SVI absent from OS-drive markers
test_os_managed_target_flags_drive_holding_os_install	Marker existence → OS-managed
test_os_managed_target_ignores_per_volume_metadata_only	No OS-install marker → not OS-managed (the Dev Drive case)
test_os_managed_target_flags_path_within_a_system_root	Target inside a system root → OS-managed
test_hard_protection_reasons_about_volume_root_purpose	Branch is reasoned, not structural
test_hard_protection_exempts_admitted_volume_root_from_mount_reason	No spurious mount reason on children
test_non_os_volume_root_without_bound_requires_large_confirmation	End-to-end: exit 5, non-os-volume-root reason
test_non_os_volume_root_with_confirmed_flag_scans	End-to-end: --confirmed-large-scan proceeds
test_large_scan_reasons_flags_non_os_volume_root	Both OS and non-OS volume-root cases
test_scan_denies_os_managed_root	OS drive still denied
test_scan_rejects_non_root_mount_point	Non-root mount still rejected
test_preview_denies_os_managed_root_snapshot	Preview re-checks OS-managed status
test_preview_allows_non_os_volume_root_snapshot	Preview passes non-OS volume root through to candidate eval

The _non_os_volume_root_patches() helper correctly mocks is_volume_root, is_os_managed_target, and mount_state to simulate a Windows Dev Drive on Linux CI without needing actual Windows APIs.

---

Low-severity observations (no action required)

1. Docstring ambiguity in is_os_managed_target() (line 309)

"The containing direction deliberately uses the narrow marker set"

"Containing direction" is slightly ambiguous — a reader might think it refers to the first check (is target contained in a root?) rather than the second (does target contain a marker?). The code is correct and the tests verify it, but "marker-contains direction" or "target-contains-marker direction" would remove the ambiguity.

2. Redundant GetLogicalDrives calls for non-OS volume roots (acknowledged as advisory #2 in the PR)

For a non-OS volume root, windows_drive_roots() is called 4× per scan invocation: twice inside the is_os_managed_target() call in main() (once from system_roots(), once from os_drive_markers()), and twice more from the is_os_managed_target() inside large_scan_reasons(). Negligible for a non-hot-path scan entry point.

3. hard_protection() volume-root branch unreachability (acknowledged as advisory #1 in the PR)

The is_volume_root(path) and is_os_managed_target(path) branch in hard_protection() (line 338) is dead code on the live call path (candidates are validated non-root descendants). Keeping it reasoned rather than blanket is the right choice for defense-in-depth, and locking it with a direct unit test (test_hard_protection_reasons_about_volume_root_purpose) is the right mitigation.

---

Documentation

SKILL.md: The "Arguments and boundaries" section now correctly names D:\ as a valid target subject to the large-target gate. The distinction between OS-managed root (denied outright) vs. non-OS volume root (valid but confirmation-required) is clear. Lines 27–30

safety-model.md: Non-overridable checks section accurately reflects the new distinction. Lines 20–22

evals.json: Eval #11 covers the correct expected behavior — the non-OS volume root is valid, hits the large-target gate, and an OS-managed root is still denied. Lines 127–135

CHANGELOG.md: Accurate and complete.

plugin.json: Version bump to 0.6.4 is correct.

---

Verdict

Approved. The classification core is sound, every branch has been hand-traced, deletion safety is unchanged, and the PR degrades safely under failure modes (stray OS-marker folder on a non-boot drive → conservative over-restrictive deny, not an unsafe allow). No blocking issues.

[claude]

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

---

Security Review

• Gathering PR diff and changed files
• Reviewing hygiene.py — OS classification logic
• Reviewing test_hygiene.py — test surface
• Reviewing docs/manifests
• Synthesizing findings

---

Reviewed the seven changed files. No critical vulnerabilities found. Two findings worth noting:

---

IMPORTANT — TOCTOU race window in is_os_managed_target() | Confidence: PLAUSIBLE

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

for marker in markers if markers is not None else os_drive_markers():
    marker_abs = marker.absolute()
    if is_within(marker_abs, target_abs) and marker_abs.exists():   # ← TOCTOU
        return True

The marker_abs.exists() call introduces a non-atomic window between classification and use. An adversary who already has write access to the drive root (e.g., administrator or a compromised privileged process) could transiently rename C:\Windows → C:\Windows.bak during the instant is_os_managed_target() is evaluating, causing the OS drive to be classified as a valid non-OS target.

The blast radius is substantially limited by layered defenses: the drive still triggers the large_scan_reasons gate (exit 5, requires --confirmed-large-scan), the scan itself is read-only, and deletion needs a separate preview + per-tier approval with a fresh token. An attacker capable of transiently renaming C:\Windows already has privileges sufficient to cause more direct harm by other means.

No code change required — the conservative failure mode described in the PR ("degrades safely — if OS-drive markers were somehow absent on a real OS drive, the result is confirmation-required plus full downstream gating, never a silent dangerous delete") holds. This is an inherent property of existence-gated classification rather than a design flaw.

---

SUGGESTION — os_drive_markers() inherits unchecked ambient environment variables | Confidence: LOW

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

env_roots = (
    os.environ.get("SystemRoot"),
    os.environ.get("ProgramFiles"),
    os.environ.get("ProgramFiles(x86)"),
    os.environ.get("ProgramData"),
)
markers.extend(Path(value).absolute() for value in env_roots if value)
drive_roots = windows_roots if windows_roots is not None else windows_drive_roots()
for drive_root in drive_roots:
    markers.extend(drive_root / name for name in WINDOWS_OS_DRIVE_MARKERS)

If environment variables are missing or adversarially set (e.g., SystemRoot pointing to a non-OS path), the env-var-derived markers may not cover the real OS directory. However, the structural per-drive loop (lines 282–284) independently synthesizes <drive>\Windows, <drive>\Program Files, etc. for every mounted drive, without relying on env vars. Because is_os_managed_target() gates on .exists(), an OS installation in a standard location will always be caught by the structural loop regardless of env var state. Not exploitable as long as the OS folders physically exist under their standard names.

No action required unless hardening against a severely contaminated environment is a stated goal.

---

No findings in remaining surface

• The WINDOWS_VOLUME_SYSTEM_NAMES union (WINDOWS_OS_DRIVE_MARKERS | WINDOWS_PER_VOLUME_METADATA) is preserved — per-entry protection of $Recycle.Bin and System Volume Information is unchanged on every drive.
• The reordering of target checks in main() and preview() (unverified → OS-managed → non-root-mount) is logically consistent and fails closed on mount-error in both paths.
• hard_protection()'s while is_within(current, target) loop correctly skips target-is-mount-point only when current == target and is_volume_root(target) — nested and bind mounts below the target still get nested-mount-point.
• is_within() uses os.path.commonpath + normcase which handles Windows drive-letter cross-drive paths correctly (ValueError returns False).
• No shell injection surface: subprocess calls throughout the script use list form, not string interpolation of user-controlled input.
• No new egress, remote MCP, or third-party trust delegation introduced.