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

[kyle-sexton]

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

[kyle-sexton]

🔒 merge-drive lane claiming this PR for on-branch fix work this cycle (amendment-round: 16, worker tier). Will fix clear branch-owned CI/bot findings, defer non-DEFER-FORBIDDEN items with reply+resolve, push, then re-check the merge gate.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7ae651119a

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

---

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

• Gather context and read changed files
• Validate Codex P1 finding (mount blocking on admitted volume roots) — confirmed, fixed
• Review classification logic (is_os_managed_target, is_volume_root)
• Review mount-rejection scoping change
• Review hard_protection reasoning changes
• Review scan/preview unification
• Check test coverage
• Implement fix for confirmed P1 bug
• Push fix — blocked (GitHub App lacks push permissions to this repo)

---

Classification core — sound

is_os_managed_target checks two orthogonal directions with the right root sets:

• "target within a system root" uses system_roots() — correctly catches sub-paths like C:\Windows\Temp.
• "an OS marker exists within the target" uses os_drive_markers() (the narrow set, deliberately excluding $Recycle.Bin and System Volume Information) and gates on .exists(). The existence gate is essential: os_drive_markers() synthesises paths for every drive without checking presence; without the gate, every drive root including D:\ would be denied because D:\Windows is in the markers list even if no Windows install is there.

The WINDOWS_OS_DRIVE_MARKERS / WINDOWS_PER_VOLUME_METADATA split is correct and well-motivated. The union WINDOWS_VOLUME_SYSTEM_NAMES is preserved for per-entry protection, so no entry-level deletability changes from the split. The order of checks in both main() and preview() is unified and consistent: mount-error → OS-managed → non-root-mount.

Confirmed P1 bug — fixed in this review

The Codex finding is correct and confirmed by code trace.

Root cause: hard_protection L344–350: the ancestor walk climbs from each candidate path up to target. When current == target and target is a volume root (e.g. D:\), mount_state(target) returns (True, None) — every Windows drive letter satisfies os.path.ismount. The pre-fix code appended "target-is-mount-point" unconditionally. In scan_tree, any directory with a non-empty protections list is truncated (line 719). So every top-level directory under the admitted non-OS volume root was truncated; the scan saw immediate children but never recursed. In preview, the same hard_protection call added "target-is-mount-point" to each candidate's blockers, making the status "blocked" and the approval_token None — no plan could be approved.

Why the tests missed it: test_non_os_volume_root_with_confirmed_flag_scans placed only a single file in the target (truncation logic applies only to directories); test_preview_allows_non_os_volume_root_snapshot didn't assert result["status"] or result["approval_token"].

Fix applied (in commit 35d12bd):

hygiene.py — exempt the admitted volume root from the mount signal in hard_protection:

elif mounted:
    if current == target and is_volume_root(target):
        pass  # admitted volume root; mount-point status is inherent, not a blocker
    else:
        reasons.append(
            "target-is-mount-point" if current == target else "nested-mount-point"
        )

Nested mounts (current != target) continue to add "nested-mount-point" as before.

test_hygiene.py — two additions:

1. test_non_os_volume_root_scan_recurses_into_directories — creates a target with a subdirectory containing a file, uses a mount_state side-effect that returns (True, None) only for the target itself (reflecting the real Windows invariant), and asserts "projects" is absent from truncated_paths and "projects/notes.txt" appears in the snapshot entries.

2. Strengthened test_preview_allows_non_os_volume_root_snapshot — switched from global return_value=(True, None) to a side-effect (same target-only logic), and added assertions on result["status"] == "ready-for-explicit-approval", result["approval_token"] non-None, and result["candidates"][0]["blockers"] == [].

Remaining review notes (no code change required)

Advisory 1 (PR-acknowledged): hard_protection's volume-root branch (is_volume_root(path) and is_os_managed_target(path) at L338) is unreachable in the live call graph — candidates are validated non-root descendants of the target, so no candidate's path.parent == path. Kept as defence-in-depth and locked by an existing unit test; the new exemption doesn't change that.

Advisory 2 (PR-acknowledged): system_roots() and os_drive_markers() each call windows_drive_roots() — two GetLogicalDrives syscalls per scan invocation. Negligible for this workload.

Advisory 3 (PR-acknowledged): A stray Recovery or Windows folder on a non-OS drive misclassifies it as OS-managed. Conservative false-positive deny — never an unsafe allow.

Push status

The commit (35d12bd) is ready locally but the GitHub App token on this runner does not have write access to melodic-software/claude-code-plugins. The branch owner (kyle-sexton) can cherry-pick or apply the diff directly:

# hygiene.py L347-350: replace the elif mounted block with:
elif mounted:
    if current == target and is_volume_root(target):
        pass  # admitted volume root; mount-point status is inherent, not a blocker
    else:
        reasons.append(
            "target-is-mount-point" if current == target else "nested-mount-point"
        )

Plus the two test additions described above (or see full diff with git show 35d12bd).

[claude]

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

---

Security Review — PR #1026

• Load tools
• Fetch PR diff
• Read changed files (hygiene.py, test_hygiene.py)
• Analyze for security issues
• Post findings

---

Scope reviewed: changed files only — hygiene.py, test_hygiene.py, plugin.json, docs, evals. No GitHub Actions workflow files were modified; no supply-chain or Actions-injection surface to review.

Injection / credential / egress: No shell execution, subprocess calls, SQL, template rendering, or credential handling in the new code. No new MCP server, remote endpoint, or egress path introduced. Clean on all of these.

---

IMPORTANT · HIGH confidence — hard_protection() ancestor walk adds target-is-mount-point to every candidate under a non-OS volume root, making the feature non-functional

Severity: IMPORTANT (logic flaw — fails safe, no dangerous delete path, but the core feature does not work)
Confidence: HIGH (traced code path completely; Codex P1 independently identified the same issue)

Where:
hygiene.py lines 340–352

The flaw:

main() and preview() were correctly updated to allow a non-OS volume root (D:\) to pass the target-level mount check:

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

But the per-entry hard_protection() ancestor walk was not updated. For every candidate D:\foo passed to hard_protection(path=D:\foo, target=D:\, ...), the while loop walks ancestors until it reaches current == target:

current = path          # D:\foo
while is_within(current, target):   # target = D:\
    ...
    mounted, mount_error = mount_state(current, ...)
    elif mounted:
        reasons.append(
            "target-is-mount-point" if current == target else "nested-mount-point"
        )
    if current == target:
        break
    current = current.parent        # eventually reaches D:\

D:\ is always os.path.ismount() == True on Windows. When current reaches the target drive root, mount_state returns (True, None) → "target-is-mount-point" is appended. This happens for every descendant of the admitted volume root.

Consequences:

• scan_tree(): every top-level directory child of D:\ has protections = ["target-is-mount-point", ...] → the elif protections: truncated.append(relative) branch fires → subtree not walked. The snapshot is almost empty.
• preview(): every candidate has "target-is-mount-point" in blockers → blocked = True → status = "blocked" → no approval token → no deletion possible.

A D:\ Dev Drive with --confirmed-large-scan passes all gate checks, produces a scan, but can never delete anything.

Why the tests don't catch it:

test_non_os_volume_root_with_confirmed_flag_scans patches mount_state globally to return_value=(True, None). Under this patch every intermediate ancestor is also "mounted", so "nested-mount-point" is added in addition to "target-is-mount-point". The test only asserts code == 0 and status == "scan-complete" — it never checks whether the snapshot entries are actually deletable.

test_preview_allows_non_os_volume_root_snapshot asserts result["candidates"] contains "orphan.tmp" — candidates are always listed, even when blocked. It does not assert result["status"] == "ready-for-explicit-approval". Under the same global mount_state patch, "target-is-mount-point" and "nested-mount-point" are in every candidate's blockers, yet the assertion passes.

Fix direction (matches Codex P1 suggestion):

Inside hard_protection, exempt the target-level mount check when current == target and is_volume_root(target). Only nested mounts (intermediate ancestors) should remain hard-blocked:

elif mounted:
    if current == target and is_volume_root(target):
        pass   # admitted volume root — mount-point at target is expected
    elif current == target:
        reasons.append("target-is-mount-point")
    else:
        reasons.append("nested-mount-point")

Fix this →

---

No other security issues found

• No injection surfaces (command, path, template, SQL) in new or modified code.
• No authorization bypass: the is_os_managed_target → mount-check ordering in main() correctly denies OS drives before allowing volume roots through, with no reachable path where a real OS drive slips to non-OS classification (per-drive hardcoded markers are the safety net even if env vars are absent).
• No dangerous pull_request_target/workflow_run patterns; no workflow files changed.
• No supply-chain risk; no new action pins or dependency loosening.
• The WINDOWS_OS_DRIVE_MARKERS / WINDOWS_PER_VOLUME_METADATA split is logically correct and preserves the union WINDOWS_VOLUME_SYSTEM_NAMES for all per-entry deletion guards.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 08c8fd1264

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

---

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

• Gather context and understand the request
• Read the full diff (git diff origin/main...HEAD)
• Verify the P1 fix in hard_protection() (admitted volume-root exemption)
• Review classification logic (is_os_managed_target, marker split, is_volume_root)
• Review scan/preview unification and mount-rejection scoping
• Review new tests for correctness and coverage
• Review docs, changelog, evals for accuracy
• Security review of new code paths
• Post final review

---

Overall

Sound PR. The P1 bug (identified by Codex and the prior security review, confirmed in the latest commit 0e813f4) is now fixed. The classification core is logically correct. I have one advisory on test quality and a handful of minor observations — no blocking issues.

---

P1 Fix — hard_protection ancestor-walk exemption

hygiene.py L347–358 — the fix is correct:

elif mounted:
    if current != target:
        reasons.append("nested-mount-point")
    elif not is_volume_root(target):
        reasons.append("target-is-mount-point")

The logic covers all three cases correctly:

• current != target → always nested-mount-point (unchanged — real cross-boundary danger)
• current == target and is_volume_root(target) → skip (volume roots are inherently mounted, and the target has already been admitted by the OS-managed check upstream)
• current == target and not is_volume_root(target) → target-is-mount-point (unchanged — non-root mount that should never have been admitted, or became a mount after snapshot)

Note on the exemption not checking is_os_managed_target: The fix only checks is_volume_root(target) without re-calling is_os_managed_target(target). This is a reasonable choice — calling is_os_managed_target on every candidate's ancestor walk would add two windows_drive_roots() syscalls × the number of files. The defense-in-depth check at L338–339 (is_volume_root(path) and is_os_managed_target(path)) handles the case where a path itself is an OS-managed volume root; the ancestor-walk exemption is the right place to skip the mount signal, not to re-run OS-managed classification.

The regression test is properly written. test_hard_protection_exempts_admitted_volume_root_from_mount_reason uses side_effect (not a global return_value) for mount_state, so only the target itself looks mounted — exactly reflecting the real Windows invariant. It covers both the "child is not blocked" and "nested mount stays blocked" cases. ✓

---

Classification logic — sound

WINDOWS_OS_DRIVE_MARKERS / WINDOWS_PER_VOLUME_METADATA split is correctly motivated. $Recycle.Bin and System Volume Information are on every Windows volume including a Dev Drive; Windows, Program Files, ProgramData, Recovery are OS-install markers. WINDOWS_VOLUME_SYSTEM_NAMES = OS_DRIVE_MARKERS | PER_VOLUME_METADATA preserves the union for per-entry deletion protection. ✓

is_os_managed_target direction semantics — I traced all meaningful cases:

Target	Direction 1 (within system_roots?)	Direction 2 (marker exists in target?)	Result
C:\ (OS drive, has C:\Windows)	C:\ not within C:\Windows → False	C:\Windows within C:\, exists → True	OS-managed ✓
D:\ (Dev Drive, no D:\Windows)	D:\ not within any root → False	D:\Windows within D:\, not exists → False	Not OS-managed ✓
/etc/x (POSIX sub-path)	/etc/x within /etc → True	—	OS-managed ✓
/ (POSIX root)	/ not within /bin (reversed) → False	/bin within /, exists → True	OS-managed ✓
/home/user	Not within any POSIX sysroot → False	None of /bin,/etc… within /home/user → False	Not OS-managed ✓

The .exists() gate on direction 2 is essential: os_drive_markers() synthesizes D:\Windows for every detected drive, so without it, every drive root would be denied regardless of what's actually on the drive.

os_drive_markers() on non-Windows delegates to system_roots() — correct; every POSIX system directory is a genuine OS-install marker, not per-volume metadata.

---

Test coverage advisory

test_preview_allows_non_os_volume_root_snapshot (L566–591) has weak assertions. The mount_state mock uses a global return_value=(True, None), which means orphan.tmp itself also looks like a mount point. In hard_protection, when current = orphan.tmp and current != target, the code appends "nested-mount-point" — so orphan.tmp.blockers = ["nested-mount-point"] and preview returns status = "blocked". The test only asserts:

self.assertEqual("high", result["tier"])
self.assertEqual(["orphan.tmp"], [item["path"] for item in result["candidates"]])

Both pass because candidates are always listed even when blocked. The test does not assert status == "ready-for-explicit-approval" or blockers == [], so it provides no evidence that the preview flow produces an approvable plan for a non-OS volume root.

The test_hard_protection_exempts_admitted_volume_root_from_mount_reason unit test (from the P1 fix commit) directly covers the correct behavior. But the higher-level test_preview_allows_non_os_volume_root_snapshot is misleading — it would pass even without any fix. Consider strengthening it:

# Use side_effect so only the target is mounted, not orphan.tmp
mock.patch.object(hygiene, "mount_state",
    side_effect=lambda p, *a: (Path(p) == target_resolved, None)),
# Then assert:
self.assertEqual("ready-for-explicit-approval", result["status"])
self.assertIsNotNone(result.get("approval_token"))
self.assertEqual([], result["candidates"][0]["blockers"])

Fix this →

Similarly, test_non_os_volume_root_with_confirmed_flag_scans creates only a single file (scratch.tmp), so truncation (the scan-side manifestation of the P1 bug) is never triggered. The new direct unit test fills this gap, but an end-to-end scan test with a subdirectory would give stronger integration coverage.

---

Minor observations (no action required)

Redundant path in os_drive_markers() on Windows — env-var paths (%SystemRoot%, %ProgramFiles%, %ProgramData%) appear twice in the markers list: once from the env-var block and once synthesized from WINDOWS_OS_DRIVE_MARKERS per drive root. For a typical Windows install where %SystemRoot% = C:\Windows, both C:\Windows (from env) and C:\Windows (from C:\ + "Windows") are added. The .exists() gate means two True results rather than one. Harmless, and consistent with how system_roots() was already structured.

is_os_managed_target + large_scan_reasons each call windows_drive_roots() — acknowledged in the PR. Two GetLogicalDrives syscalls per scan; large_scan_reasons also calls is_os_managed_target internally (adding two more). Negligible for this workload, and the result could be memoized trivially if it ever matters.

is_volume_root is a one-liner — path.parent == path is a well-known Python idiom. Extracting it as a named function is the right call here because it appears in three different contexts (L338, L350, L671) with slightly different semantics, and the name communicates intent for the volume-root classification narrative.

---

Scan/preview unification — correct

Both main() (L1691–1694) and preview() (L1271–1274) now use the same order: OS-managed → non-root-mount. The previous code had these reversed in preview(). The fix is correct and the ordering makes semantic sense (OS-managed is the harder/earlier veto; non-root-mount is the structural veto for paths that somehow passed the OS check).

---

Security

No new injection surfaces, subprocess calls with user-controlled data, or egress paths. The os_drive_markers() env-var reads (%SystemRoot%, %ProgramFiles%) are the same env vars system_roots() has always used. If those env vars are controlled by an attacker, the machine is already compromised; a missing env var results in a shorter marker list (more conservative, never more permissive). Deletion remains gated by snapshot identity, preview, and per-tier approval regardless of classification.

---

Docs

• CHANGELOG.md (0.6.3): accurate and thorough. The reference to "0.5.0" for the large-target gate would be clearer as "0.5.1" or "feat(disk-hygiene): gate unbounded scans of known-large targets at the engine #1010" but it's not wrong.
• SKILL.md: correctly updated — non-OS volume root described as valid but known-large, OS-managed root still denied outright.
• safety-model.md: "non-root mount target" wording is correct; the parenthetical clarifying volume roots are governed by OS-managed reasoning, not the structural mount veto, is worth keeping.
• evals.json eval fix(markdown-formatter): harden hook + simplify plumbing #11: well-specified. The expected output correctly names large-target-confirmation-required and distinguishes non-OS-volume-root from OS-managed-root behavior.
• plugin.json version 0.6.2 → 0.6.3: appropriate for a bug fix.

[claude]

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

---

Security Review — PR #1026

• Load tools
• Fetch PR diff (HEAD: 0e813f4)
• Read changed files (hygiene.py, test_hygiene.py)
• Analyze for security issues
• Post findings

---

Scope reviewed: changed files only — hygiene.py (all commits in this branch), test_hygiene.py. No GitHub Actions workflow files were modified; no supply-chain or Actions-injection surface to review.

Injection / credential / egress: No shell execution with shell=True. All six subprocess.run calls use list form. No SQL, template rendering, or credential handling in new or modified code. No new MCP server, remote endpoint, or egress path introduced.

---

SUGGESTION · PLAUSIBLE — is_volume_root returns True for Windows UNC share roots, admitting them where mount-check previously blocked them

Where: hygiene.py line 288–290 (definition), used at line 1693

The issue:

def is_volume_root(path: Path) -> bool:
    return path.parent == path

In Python's pathlib on Windows, a UNC share root (Path("//server/share")) is its own parent — Path("//server/share").parent == Path("//server/share") is True. This means is_volume_root returns True for UNC roots.

Combined with the new gate in main():

if is_os_managed_target(target):
    raise HygieneError("OS-managed roots are not valid audit targets")
if mounted and not is_volume_root(target):
    raise HygieneError("mount points are not valid audit targets")

For \\server\share:

• is_os_managed_target: system_roots() and os_drive_markers() enumerate only local drives via windows_drive_roots() (A–Z letters), so no OS marker maps to a UNC root → returns False
• mounted and not is_volume_root(target): True and False → condition not triggered

Before this PR: the blanket if mounted: raise caught all mount points, including UNC shares.

After this PR: \\server\share passes both checks — it is admitted as a non-OS volume root and falls through to large_scan_reasons, which adds non-os-volume-root and requires --confirmed-large-scan. With that flag, scan proceeds. The preview + apply path is then available for file deletion on the network share.

Why this is low severity: The user must explicitly specify the UNC path as --target, add --confirmed-large-scan, create a deletion plan, and explicitly approve it — multiple intentional steps. This is a CLI tool used by an operator who already has network-share access. The risk is unintentional behavior, not privilege escalation.

Mitigation option (if network shares should remain blocked): Add an explicit guard in is_volume_root or in the target-validation block:

# In main() / preview(), after the is_volume_root exemption:
if is_volume_root(target) and not target.drive.endswith("\\"):
    # UNC share root (drive is e.g. \\server\share, not C:)
    raise HygieneError("UNC share roots are not valid audit targets")

Or use Path.anchor / Path.drive to distinguish C:\ (drive ends in :) from \\server\share (drive is \\server\share).

---

No other security issues found

• OS-drive classification (is_os_managed_target): sound. The drive-root enumeration in os_drive_markers and system_roots iterates all local drive letters and synthesizes marker paths for each, providing a backstop even if env vars (SystemRoot, ProgramFiles, etc.) are cleared or redirected. Env var manipulation can only over-classify (deny more drives) — it cannot cause an OS drive with real OS markers to be classified as non-OS.
• hard_protection exemption (latest commit 0e813f4): correctly exempts only the admitted volume-root target (current == target and is_volume_root(target)). Nested mounts (current != target) still append nested-mount-point. A non-volume-root target that becomes mounted after admission still gets target-is-mount-point. Logic is correct.
• Subprocess calls (lines 530, 818, 1109, 1144, 1209): all use list form, not shell=True. Path arguments go in as str(path) list elements — no shell injection surface.
• State write path safety (state_output_path): unchanged; is_within guards prevent writes outside CLAUDE_PLUGIN_DATA.
• No authorization bypass: the OS-managed check fires before the mount exemption in both main() and preview(). No reachable path where a real OS drive slips through as non-OS (the per-drive marker loop catches existing C:\Windows even without env vars).
• No dangerous GitHub Actions patterns: no workflow files changed.
• WINDOWS_OS_DRIVE_MARKERS / WINDOWS_PER_VOLUME_METADATA split: correct and preserves the union for per-entry deletion guards.

[claude]

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

---

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

• Gather context and understand prior reviews
• Read current diff (origin/main...HEAD)
• Review changed files: hygiene.py, test_hygiene.py
• Check advisory items from prior reviews (weak tests, UNC share concern)
• Verify merge-commit reconciliation didn't introduce regressions
• Post comprehensive final review

---

Overall

Sound PR. The P1 fix from commit 0e813f4 (the hard_protection ancestor-walk exemption) is correctly implemented in the current HEAD. The classification logic is correct. Two test advisories from the prior round remain open — neither is a blocker, but both leave integration-level false confidence. The UNC share edge case (raised in the second security review) is also unaddressed. No regressions from the 540d247 merge. No blocking issues.

---

P1 Fix — verified correct in current HEAD

hygiene.py L347–358

elif mounted:
    if current != target:
        reasons.append("nested-mount-point")
    elif not is_volume_root(target):
        reasons.append("target-is-mount-point")

All three cases handled correctly:

• current != target → nested-mount-point (unchanged — real cross-boundary danger)
• current == target and is_volume_root(target) → skip (volume root is inherently mounted; already admitted upstream)
• current == target and not is_volume_root(target) → target-is-mount-point (non-root mount, unchanged)

The unit test test_hard_protection_exempts_admitted_volume_root_from_mount_reason is correctly written — uses side_effect so only the target itself is mounted, covers both the "child clean" and "nested mount blocked" cases.

---

Classification logic — sound

is_os_managed_target two-direction check is correct and traced at all meaningful cases:

Target	Within system_roots?	OS marker exists in target?	Result
C:\ (OS drive)	No	C:\Windows exists → Yes	OS-managed ✓
D:\ (Dev Drive, no D:\Windows)	No	D:\Windows not present → No	Not OS-managed ✓
/etc/x	Yes (within /etc)	—	OS-managed ✓
/ (POSIX root)	No (not within /bin)	/bin exists → Yes	OS-managed ✓

The .exists() gate on direction 2 is load-bearing — os_drive_markers() synthesizes marker paths for every enumerated drive without checking presence. Without it, every drive root would be denied.

WINDOWS_OS_DRIVE_MARKERS / WINDOWS_PER_VOLUME_METADATA split is correctly motivated: per-volume metadata ($Recycle.Bin, System Volume Information) that every Windows volume carries is excluded from OS-drive classification. The union WINDOWS_VOLUME_SYSTEM_NAMES is preserved for per-entry deletion guards — no entry-level deletability changes.

---

Advisory 1 — test_preview_allows_non_os_volume_root_snapshot still has weak assertions

test_hygiene.py L566–589

The test patches mount_state globally with return_value=(True, None). Under this patch, when hard_protection walks from orphan.tmp up to target, mount_state(orphan.tmp) also returns (True, None) — since current (orphan.tmp) != target, it appends "nested-mount-point". The candidate ends up blocked and result["status"] is NOT "ready-for-explicit-approval", but the test doesn't assert either of these:

self.assertEqual("high", result["tier"])                                  # passes — always
self.assertEqual(["orphan.tmp"], [item["path"] for item in ...])         # passes — candidates listed even when blocked

The test would pass even if the P1 fix were absent. The direct unit test covers the underlying behavior correctly, but this integration-level test provides false confidence that the preview produces an approvable plan.

Suggested fix — switch to side_effect and add meaningful assertions:

with (
    mock.patch.object(hygiene, "is_volume_root",
        side_effect=lambda p: Path(p).resolve() == root.resolve()),
    mock.patch.object(hygiene, "is_os_managed_target", return_value=False),
    mock.patch.object(hygiene, "mount_state",
        side_effect=lambda p, *a: (Path(p).resolve() == root.resolve(), None)),
    ...
):
    result = hygiene.preview(snapshot, plan)
self.assertEqual("ready-for-explicit-approval", result["status"])
self.assertIsNotNone(result.get("approval_token"))
self.assertEqual([], result["candidates"][0]["blockers"])

Fix this →

---

Advisory 2 — directory recursion not exercised end-to-end

test_non_os_volume_root_with_confirmed_flag_scans (L475) places only a single file (scratch.tmp) — truncation (the scan-side consequence of the P1 bug, where a protected directory is not walked) is never triggered. The test verifies code == 0 and status == "scan-complete" but passes even if directories under the target would be truncated.

The test mentioned in the first-round review (test_non_os_volume_root_scan_recurses_into_directories) was written in local commit 35d12bd but never pushed and is absent from the current branch. The direct unit test for hard_protection covers the right behavior, but an end-to-end scan test with a subdirectory would give stronger integration coverage.

---

Advisory 3 — UNC share roots admitted as non-OS volume roots (carried from security review)

hygiene.py L288–290

def is_volume_root(path: Path) -> bool:
    return path.parent == path

On Windows, Path("//server/share").parent == Path("//server/share") is True — UNC share roots are their own parent in pathlib. Combined with is_os_managed_target only enumerating local drive letters (windows_drive_roots() uses A–Z), a UNC share root (\\server\share) would:

• Pass is_os_managed_target → not OS-managed
• Pass mounted and not is_volume_root(target) → exempt from mount check

Before this PR: blanket mount rejection caught all mount points including UNC roots. After this PR: UNC roots pass both checks, are admitted as non-OS volume roots, fall through to the large-target gate, and are scannable with --confirmed-large-scan.

This is low severity (multiple intentional steps required; user already has share access) and fails safe via the large-target gate. But it's a behavioral change from the pre-PR behavior worth explicitly deciding on. Mitigation if desired:

def is_volume_root(path: Path) -> bool:
    if path.drive.startswith("\\\\"):  # UNC share root
        return False
    return path.parent == path

---

Scan/preview unification — correct

Both main() (L1691–1694) and preview() (L1271–1274) now use the same order: OS-managed → non-root-mount. The previous ordering inconsistency in preview() is resolved.

---

Merge commit (540d247) — clean

The merge brings in #1014 (guard hook fail-open on skill-hook data-root token) and related hook-utils changes. These are unrelated to the disk-hygiene volume-root logic. No conflicts or regressions in the disk-hygiene path.

---

Security

No new injection surfaces, subprocess calls with user-controlled data, or egress paths. The os_drive_markers() env-var reads (%SystemRoot%, %ProgramFiles%) are the same env vars system_roots() has always used; env-var manipulation can only over-classify (deny more), never under-classify. Deletion remains gated by snapshot identity, preview, per-tier approval regardless of classification. All six subprocess.run calls use list form.

---

Summary

No blocking issues. The P1 fix is correct and unit-tested. The classification core is sound. The two open test advisories (weak test_preview_allows_non_os_volume_root_snapshot assertions; missing directory-recursion integration test) are worth addressing before merge to replace false-confidence tests with meaningful ones, but they don't affect production behavior. The UNC share edge case is a deliberate decision point — either document the new behavior or add a guard.

[claude]

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

---

Security Review — PR #1026 (HEAD: 540d247)

• Load tools
• Fetch PR diff (current HEAD vs origin/main)
• Read changed files (hygiene.py, test_hygiene.py)
• Analyze for security issues
• Post findings

---

Scope reviewed: all files changed in this PR vs origin/main — hygiene.py (+117/−25), test_hygiene.py (+282/0), plugin.json, CHANGELOG.md, SKILL.md, evals.json, safety-model.md. No GitHub Actions workflow files modified; no supply-chain surface to review.

Delta since last security review (commit 0e813f4): The current HEAD (540d247) is a merge commit that incorporates origin/main (commit 0f04bd8 — guard hook fail-open fix in separate files). hygiene.py is byte-for-byte identical to 0e813f4 — no new security surface was introduced in this reconciliation merge.

Injection / credential / egress: No shell execution with shell=True. All subprocess.run calls use list form (unchanged). No SQL, template rendering, credential handling, MCP server, or remote endpoint in new or modified code.

---

SUGGESTION · PLAUSIBLE — is_volume_root returns True for Windows UNC share roots, admitting them past the mount-rejection gate

Carried forward from the 0e813f4 review — not addressed in the current HEAD.

Where: hygiene.py L288–290 (definition); applied at main() L1693 and preview() L1273

def is_volume_root(path: Path) -> bool:
    return path.parent == path

On Windows, Path("//server/share").parent == Path("//server/share") is True — a UNC share root is its own parent in Python's pathlib, so is_volume_root returns True for it.

Combined with the new gate in main() and preview():

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

For \\server\share: is_os_managed_target returns False (UNC paths are not reachable from windows_drive_roots()'s A–Z letter enumeration). mounted and not is_volume_root(target) evaluates to True and False — the mount rejection is skipped. The UNC root is admitted, classified as a non-OS volume root by large_scan_reasons, and subject only to the --confirmed-large-scan gate.

Before this PR: the blanket if mounted: raise caught UNC paths because Windows treats them as mount points. After this PR: they pass through.

Why low severity: the user must explicitly specify \\server\share as --target, supply --confirmed-large-scan, generate a preview plan, and explicitly approve deletion tier by tier — all intentional, supervised steps. There is no privilege escalation; the operator already holds the network share access. The risk is unintentional scope extension (auditing a network share when a drive root was intended), not silent unauthorized deletion.

Mitigation option (if network shares should remain blocked): In the target-validation blocks, distinguish a local drive root (drive ends in :, e.g. C:) from a UNC share root (drive is \\server\share):

# In main() / preview(), alongside the is_volume_root exemption:
if is_volume_root(target) and not str(target.drive).endswith(":"):
    raise HygieneError("UNC share roots are not valid audit targets")

Or alternatively, add a is_local_volume_root predicate that checks Path.anchor / Path.drive before exempting from the mount-rejection gate.

Fix this →

---

No other security issues found

• is_os_managed_target classification: Sound. The per-drive synthesized markers (one [drive]/Windows, [drive]/Program Files, etc. per detected drive root) provide a reliable backstop independent of env vars — an OS drive with a real Windows install is always caught even if SystemRoot/ProgramFiles env vars are absent or redirected. Env var manipulation can only over-classify (deny more drives, conservative), never under-classify an OS drive that holds the real markers.
• hard_protection exemption (0e813f4): The current != target → nested-mount-point; current == target and not is_volume_root(target) → target-is-mount-point logic is correct. The exemption fires only when the target itself is a volume root (admitted upstream); nested mounts remain blocked; a non-volume-root target that became a mount after snapshot still gets target-is-mount-point. No reachable path where a mount block is incorrectly skipped.
• is_within direction semantics: is_within(marker_abs, target_abs) with os.path.commonpath is correct for direction 2 (checking that a marker path is within the target). The existence gate (.exists()) correctly prevents synthesized non-existent marker paths from misclassifying non-OS volumes.
• Subprocess calls: All use list form. No user-controlled data reaches a shell.
• State write path safety: state_output_path (unchanged) guards against writes outside CLAUDE_PLUGIN_DATA.
• No authorization bypass: is_os_managed_target fires before the mount-exemption in both main() and preview(). No path where a real OS drive with existing markers passes through as non-OS.
• WINDOWS_OS_DRIVE_MARKERS / WINDOWS_PER_VOLUME_METADATA split: Correct — $Recycle.Bin and System Volume Information are excluded from OS-drive classification, preserving per-entry deletion protection while not misclassifying Dev Drive roots.
• No dangerous GitHub Actions patterns: No workflow files changed.

[kyle-sexton]

Superseded by #1063 — identical final reviewed tree re-applied as a single signed commit on current main; all review threads here were resolved before supersession.