fix(disk-hygiene): unblock guarded engine lane and bounded large-root audits

[kyle-sexton]

No related issue: this PR emerged from a live disk-hygiene audit session rather than a pre-filed tracker item, and its defects fixed span several undocumented gaps together (see below).

Related

• Issue disk-hygiene:clean misses underscore-prefixed temp dirs and unowned agent-session files at a home-directory root #287 (disk-hygiene:clean misses underscore-prefixed temp dirs and unowned agent-session files at a home-directory root) — this PR's tmp_*/scratch* baseline-hint addition (defect 7 below) covers only that report's hint-glob gap; its other two gaps (an agent-session status-file heuristic, and SKILL.md's undefined "suspicious" triage criteria) remain open and untouched here.

Why

A live audit of a Windows user profile (/disk-hygiene:clean on the profile root) hit every one of these in sequence; full defect log in the session run notes. The engine was unusable through its own sanctioned lane, and the guard did not cover the platform's primary shell.

Defects fixed

1. Engine unreachable via guarded lane. CLAUDE_PLUGIN_DATA is exported to hook processes and interpolated into skill content, but not into Bash/PowerShell tool subprocesses (plugins reference), so every guard-approved scan died with CLAUDE_PLUGIN_DATA is required for generated state, and the guard's literal-word tokenizer offered no way to supply it. New --data-root flag on scan/preview/apply; the guard validates the value against the CLAUDE_PLUGIN_DATA its own hook process received, discloses the authorized root in denial guidance (alongside the interpreter path), and fails closed without hook authority. Env var remains a fallback.
2. Guard covered Bash only. On Windows the engine (and any deletion) could run unguarded through PowerShell. Matcher is now Bash|PowerShell (hooks guide: matchers accept any tool name): engine invocations from PowerShell are hard-denied, deletion spellings / .NET Delete calls become a final human permission prompt (same bar as engine apply), read-only support work passes. Documented as a raised bar, not fail-closed — Bash stays the deny-unknown engine lane.
3. Profile-root audit impossible. The all-or-nothing walk exceeded the 250k entry cap (30+ min, then abort) before inventorying one loose root file. New --max-depth bounded scan; truncated directories land in truncated_paths, are reported as coverage gaps, and are mechanically blocked from plans by a new truncated-not-inventoried preview blocker (defense in depth on top of the live descendant-set comparison).
4. Cap abort was silent and lossy. Progress heartbeat to stderr every 25k entries; cap error now suggests --max-depth.
5. os_autoclean advisory unreachable for its primary target. Advisory (Storage Sense / systemd-tmpfiles) is computed pre-walk and included in scan failure payloads, so a capped profile scan still surfaces it.
6. No sanctioned Windows removal path. SKILL.md documents the unsupported-platform manual handoff: same exact-list AskUserQuestion bar, per-path revalidation, reversible (Recycle Bin/Trash) deletion preferred.
7. Hint gaps. tmp_* (medium ceiling) and scratch* (low ceiling) baseline hints; the audit's 12 confirmed leftovers carried zero hints.
8. 8.3 short-name gotcha. Guard rejects ~ as an expansion character, which includes Windows short paths; documented (long-form paths required; guard disclosures are long-form).

Verification

• python -m unittest test_hygiene: 64 tests, OK (4 pre-existing platform skips), on Windows / Python 3.14.
• New tests: guard data-root authority (allow/deny/no-authority), --max-depth literal validation, PowerShell lane (engine deny, deletion-spelling ask, read-only defer), engine truncation + preview blocker, --data-root without env, capped-scan advisory + guidance, apply-command --data-root authorization, preview's live-descendant-walk short-circuit for a truncated candidate, and annotate_tracked's git-query path scoping.
• Facts about env-var export, skill interpolation, and hook matchers verified this session against the plugins reference and hooks guide (URLs above).

Delivery

disk-hygiene 0.2.0 -> 0.3.0 with a new plugin CHANGELOG (version bump is the delivery vehicle per the migration playbook).

🤖 Generated with Claude Code

[claude]

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

---

PR Review — fix(disk-hygiene): unblock guarded engine lane and bounded large-root audits

Tasks

• Gather context (PR diff, changed files)
• Review destructive_guard.py — guard logic, new --data-root flag, PowerShell lane
• Review hygiene.py — --max-depth, truncation, advisory surfacing
• Review test_hygiene.py — coverage completeness
• Review SKILL.md, safety-model.md, baseline-policy.json — doc/policy alignment
• Review plugin.json — version bump, manifest correctness
• Review CHANGELOG.md — accuracy and format
• Synthesize findings and post

---

Summary

The PR addresses a real-world audit failure cleanly. The --data-root flag, PowerShell lane, --max-depth truncation, and advisory surfacing are all mechanically sound. Guard token-length arithmetic is correct across all three command shapes. No security regressions found. Four findings below — one correctness nit, one per-candidate vs. whole-preview error propagation difference, one test coverage gap, and one test isolation fragility. None are blockers.

---

Findings

1. Entry-cap check is off-by-one — entry #250,001 is added before the error fires

hygiene.py:609

if len(entries) > MAX_SNAPSHOT_ENTRIES:
    raise HygieneError(...)

> fires only after the 250,001st entry is already appended. If the intent is "at most 250,000 entries", this should be >=. The error message says "exceeds 250,000" which is technically correct, but snapshot["entries"] will contain 250,001 items at the moment the error is raised. The test patches MAX_SNAPSHOT_ENTRIES = 1 and confirms the error fires after the second entry, which is consistent with the current > check — but also confirms the extra entry exists. Minor, but the comment in the error message implies the cap is the limit, not the limit + 1.

Fix this →

---

2. HygieneError from current_descendants propagates through preview() rather than blocking per-candidate

hygiene.py:1147

try:
    current_paths = current_descendants(target, path)
except PermissionError:
    current_paths = set()
    blockers.append("needs-elevation")
except OSError:
    current_paths = set()
    blockers.append("filesystem-state-unverified")

current_descendants raises HygieneError when linux_mount_points() fails inside it. That exception is not caught here — it escapes preview() and causes the entire preview to fail (status invalid-or-blocked, exit 2) rather than blocking just that candidate with "filesystem-state-unverified". This is safe behavior (mount state unverified → preview fails conservatively), but it diverges from how PermissionError/OSError are handled per-candidate one line above. If this divergence is intentional (mount failure = total abort), a comment would clarify.

---

3. Missing guard test for apply --data-root

test_hygiene.py:1228

The guard tests cover --data-root for scan (line 1160) and preview (line 1228) but not for apply. The apply path in classify_exact_engine_command accepts --data-root via _consume_optional_pairs(tokens[14:], frozenset({"--data-root"})), changing valid lengths from {14} to {14, 16}. An explicit apply + --data-root test would confirm the token count and authorization path are wired correctly end-to-end.

Fix this →

---

4. DATA_ROOT_OVERRIDE global creates latent test isolation fragility

hygiene.py:70, hygiene.py:1492

main() resets the module-level global with DATA_ROOT_OVERRIDE = args.data_root at entry, so any call to main() with --data-root followed by a call to main() without it is safe today. However, state_output_path reads the global directly — if a test or caller ever invokes state_output_path outside of main() after a previous main(["...", "--data-root", some_path]) call, it could see a stale temporary directory that no longer exists. The current test suite is safe, but the pattern is fragile. A context-variable or thread-local approach, or resetting the global at function exit, would make this more robust.

---

5. PowerShell pass-through for non-enumerated mutation verbs (documented, but surface area not stated)

destructive_guard.py:217

The PR and SKILL.md correctly document this as a raised bar, not fail-closed. Worth noting for auditors: PowerShell commands that don't contain hygiene.py, don't match _POWERSHELL_MUTATION_WORDS, and don't match _POWERSHELL_DOTNET_DELETE pass silently. Third-party deletion cmdlets (e.g., Move-ToRecycleBin from a module, or [System.IO.Directory]::Move) would not be caught. The engine's own containment and the Linux-only execution gate remain the deletion authority as documented — just confirm this scope statement is acceptable to the security reviewer.

---

What's working well

• Approval token anti-replay — binding to snapshot nonce + plan digest + tier is solid; altering any element of the plan changes the token.
• truncated-not-inventoried blocker — correctly handles candidate == truncated path, candidate beneath a truncated directory, and candidate that is an ancestor of a truncated directory (lines 1135-1141). Defense in depth on top of the live descendant-set comparison.
• os_autoclean_advisory pre-walk placement — computing the advisory before scan_tree and including it in failure payloads is a clean fix that requires no structural refactoring.
• --max-depth guard validation — the regex [1-9][0-9]{0,3} rejects leading zeros (octal-looking literals) and negative values at the guard layer before argparse, with the test confirming "007" and "-1" are both denied.
• Bootstrap flow — the deliberate first-deny pattern in SKILL.md (submit bare-python scan, read the denial guidance for the absolute interpreter + authorized --data-root, retry) is well-designed for the env-var-not-exported constraint and is clearly documented.
• _data_root_key uses strict=False — correctly handles a not-yet-created data directory for path comparison.
• Standing-policy isolation in tests — mock.patch.object(hygiene, "standing_policy_paths", return_value=[]) in setUp prevents developer-machine policy files from affecting scan results in CI.
• Guard scan token-length set {7, 9, 11, 13, 15} — correctly accounts for 4 optional flag pairs (--policy, --project-dir, --data-root, --max-depth); arithmetic checks out.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 57dd369840

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

---

Tasks

• Gather context (PR diff, changed files)
• Review destructive_guard.py — guard logic, --data-root flag, PowerShell lane
• Review hygiene.py — --max-depth, truncation, advisory surfacing, cap check
• Review test_hygiene.py — coverage completeness
• Review SKILL.md, safety-model.md, baseline-policy.json — doc/policy alignment
• Review plugin.json — version bump, manifest correctness
• Review CHANGELOG.md — accuracy and format
• Synthesize findings and post

---

PR Review — fix(disk-hygiene): unblock guarded engine lane and bounded large-root audits

Note: The triggering sync is a merge of origin/main into the PR branch (1675a9d); no PR-authored code changed. The findings below reflect the PR's code at 57dd369840, which is identical to what was reviewed previously. Prior findings remain open.

---

Summary

The core design (guarded engine lane, --data-root authority binding, --max-depth truncation, PowerShell lane, advisory surfacing) is sound. One previously flagged finding rises to blocking concern when combined with the Codex P2 observation: the truncated-not-inventoried blocker correctly prevents deletion but does not prevent current_descendants from executing an unbounded live walk on the candidate — recreating the very traversal --max-depth was meant to avoid. Four additional findings remain from the prior review.

---

Findings

1. Truncated candidate does not short-circuit before current_descendants — defeat of --max-depth protection in preview

hygiene.py:1135–1148

The truncated-not-inventoried blocker is appended at line 1141 but execution falls through immediately to current_descendants(target, path) at line 1148, which recursively walks the live filesystem with no depth bound:

if any(
    name == relative or name.startswith(relative + "/") or relative.startswith(name + "/")
    for name in truncated_paths
):
    blockers.append("truncated-not-inventoried")
# ↓ still executes on truncated directories
expected_paths = {name for name in entries if ...}
try:
    current_paths = current_descendants(target, path)  # unbounded live walk

A --max-depth 1 scan of a home directory records nearly every top-level directory in truncated_paths. Those directories are valid snapshot entries — a model can legally propose them as candidates. On preview, current_descendants walks their full live subtrees. On a real Windows user-profile (the audit that drove this PR), that is the same 30+ minute unbounded traversal --max-depth was added to prevent.

Suggested fix — skip the live-walk block when the candidate is already known-truncated:

if "truncated-not-inventoried" not in blockers:
    expected_paths = {
        name for name in entries
        if name == relative or name.startswith(relative + "/")
    }
    try:
        current_paths = current_descendants(target, path)
    except PermissionError:
        current_paths = set()
        blockers.append("needs-elevation")
    except OSError:
        current_paths = set()
        blockers.append("filesystem-state-unverified")
    if current_paths != expected_paths:
        blockers.append("changed-since-scan")
    # individual entry identity loop can also be skipped — expected_paths is empty for truncated dirs

The per-entry same_identity loop below line 1157 reads from expected_paths; for a truncated directory that set is empty (no children were scanned), so no live I/O happens there — only the current_descendants call needs guarding.

Fix this →

---

2. Entry-cap check off-by-one — snapshot contains 250,001 entries when error fires

hygiene.py:609

if len(entries) > MAX_SNAPSHOT_ENTRIES:
    raise HygieneError(...)

> fires only after the 250,001st entry is already appended; >= would cap at exactly MAX_SNAPSHOT_ENTRIES. The error message says "exceeds 250,000" which is technically accurate, but snapshot["entries"] will contain 250,001 items at the throw site. The existing test patches MAX_SNAPSHOT_ENTRIES = 1 and observes the error fires after the second entry — consistent with >, but also confirms the extra entry is present.

Fix this →

---

3. Missing guard test for apply --data-root

test_hygiene.py:1228

test_guard_preview_accepts_optional_authorized_data_root exists (line 1228) but the apply equivalent does not. The apply path in classify_exact_engine_command accepts --data-root via _consume_optional_pairs(tokens[14:], frozenset({"--data-root"})), making the valid token-count set {14, 16}. A test covering apply --data-root <authorized> would pin that the guard decision is "ask" (not "deny") and that token count 16 is correctly accepted.

Fix this →

---

4. HygieneError from current_descendants escapes preview() without per-candidate handling

hygiene.py:1147–1154

linux_mount_points() inside current_descendants raises HygieneError on read failure. That exception is not caught in the per-candidate block — it escapes the preview() loop and aborts the entire preview with status: invalid-or-blocked. PermissionError and OSError are handled per-candidate one line above, so the divergence (mount failure = total abort vs. I/O error = per-candidate block) is visible but undocumented. If intentional, a comment stating "mount state unverifiable → abort entire preview" would make the design explicit. (Note: finding 1's fix would also make this moot for the truncated-candidate code path.)

---

5. DATA_ROOT_OVERRIDE module global — latent test isolation fragility

hygiene.py:70, hygiene.py:1492

main() sets the module-level DATA_ROOT_OVERRIDE on entry but never resets it on exit. The current test suite is safe, but any future test or caller that invokes state_output_path after main(["...", "--data-root", tmp_path]) could pick up a stale temp path that no longer exists. A reset to None at the end of main() (or at the top of main() before parsing, which already happens for the args.data_root assignment) would close the window. The current assignment at line 1492 is the reset — but only if main() is called again; the global persists across calls within a single process lifecycle.

---

What's working well

• Guard token-length arithmetic for scan {7, 9, 11, 13, 15} correctly accounts for four optional flag pairs; the _consume_optional_pairs implementation handles duplicates and unknown flags correctly.
• Approval token anti-replay — binding to session_nonce + snapshot_digest + tier + candidates is solid.
• truncated-not-inventoried blocker logic — correctly handles the three overlap cases (candidate == truncated path, candidate beneath a truncated directory, candidate is ancestor of a truncated directory) at lines 1135–1141. The defense-in-depth framing is accurate.
• --max-depth guard validation — regex [1-9][0-9]{0,3} correctly rejects leading zeros (octal-looking literals), zero, and negative values at the guard layer; "007" and "-1" denial confirmed by tests.
• os_autoclean_advisory pre-walk placement — computing the advisory before scan_tree and including it in failure payloads is clean; no structural refactoring required.
• _data_root_key uses strict=False — correctly handles a not-yet-created data directory for path comparison in _is_authorized_data_root.
• Standing-policy isolation in tests — mock.patch.object(hygiene, "standing_policy_paths", return_value=[]) in setUp prevents developer-machine policy files from affecting CI results.
• plugin.json version bump to 0.3.0 — matches CHANGELOG and is the correct delivery vehicle per the migration playbook.
• CHANGELOG format — follows Keep a Changelog 1.1.0; Added/Changed sections accurately reflect the diff; BREAKING change for HOOK_DISK_HYGIENE_ENABLED is clearly called out.

---

Bottom line: Finding 1 (truncated candidate triggers unbounded live walk in preview) should be fixed before merge — it directly negates the primary --max-depth use case for preview operations. Finding 2 (off-by-one) is a low-risk correctness nit. Findings 3–5 are non-blocking but would tighten the coverage.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1675a9d502

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

---

Tasks

• Gather context — inspect new commit(s) since last review
• Review hygiene.py — verify bounded live walk and git-tracking fix
• Check all prior findings are addressed
• Synthesize and post updated review

---

PR Review — fix(disk-hygiene): unblock guarded engine lane and bounded large-root audits

Reviewed commit: e933fc3 — "fix(disk-hygiene): bound the live descendant walk and git-tracking query to the scan's --max-depth"

This is a follow-up to the two prior review rounds. The new commit addresses every finding that was flagged across both reviews plus both Codex P2 items. Detailed verification below.

---

All prior findings resolved

Finding 1 (was blocking): Truncated candidate no longer calls current_descendants

hygiene.py:1168

The if "truncated-not-inventoried" in blockers guard is inserted before the current_descendants call. For a truncated candidate, current_paths is set to expected_paths (the statically-known snapshot entries — just the directory itself, with no children) rather than triggering a live recursive walk. The changed-since-scan comparison current_paths != expected_paths evaluates to False, so no spurious blocker is added. The per-entry same_identity loop over expected_paths runs only for the truncated directory node itself (a single stat, not a recursive walk).

The new test test_preview_skips_live_descendant_walk_for_truncated_candidate patches current_descendants to raise AssertionError if called, then asserts "truncated-not-inventoried" is present and "changed-since-scan" is absent — a clean, targeted regression guard.

Codex P2 / Finding (from second review): annotate_tracked no longer walks the whole repo on a bounded scan

hygiene.py:650

annotate_tracked now accepts truncated: list[str] and builds a scoped pathspec list before invoking git ls-files:

• base = target.relative_to(repo) when target is inside the repo (e.g., "profile"), or "." for nested repos and the same-path case
• Each truncated directory contributes :(exclude)<repo-relative-path> so a bounded profile scan does not pull the entire repository's tracked-file index

The ValueError-guarded relative_to calls handle the nested-repo case (repo is a child of target) correctly by falling back to "." as base. The call site in scan_tree passes truncated as the new fourth positional argument; no other callers exist.

The new test test_annotate_tracked_scopes_git_query_to_inventoried_paths captures the subprocess command and asserts pathspecs[0] == "profile" and ":(exclude)profile/huge-vendored-dep" is present — verifying both the base restriction and the exclude entry.

Finding 2: Entry-cap check is now inclusive

hygiene.py:609

Changed from > to >=. The snapshot now contains at most MAX_SNAPSHOT_ENTRIES items when the error fires. The existing test_capped_scan_reports_advisory_and_bounded_rerun_guidance test (which patches MAX_SNAPSHOT_ENTRIES = 1) remains compatible — it checks exit code, status field, --max-depth in the error message, and os_autoclean in the payload, none of which depend on the exact count.

Finding 3: Guard test for apply --data-root added

test_hygiene.py:1298

test_guard_apply_accepts_optional_authorized_data_root mirrors the preview test: constructs a 16-token apply command with --data-root pointing to the authorized CLAUDE_PLUGIN_DATA value, asserts the guard decision is "ask". Confirms both the token-count set {14, 16} and the authorization path are wired correctly.

Finding 4: HygieneError from current_descendants now caught per-candidate

hygiene.py:1180

The except clause is widened to except (OSError, HygieneError), so a mount-read failure inside current_descendants now adds "filesystem-state-unverified" to that candidate's blockers rather than aborting the entire preview. Behavior is now consistent with how OSError is handled.

Finding 5: DATA_ROOT_OVERRIDE global always reset

hygiene.py:1607

The global DATA_ROOT_OVERRIDE declaration is moved to the top of main(), the assignment (DATA_ROOT_OVERRIDE = args.data_root) is placed before the try block, and a finally: DATA_ROOT_OVERRIDE = None resets the global on every exit path — normal return, exception, or early return. The state_output_path() function can never see a stale override from a prior main() invocation in the same process.

---

What's working well (unchanged from prior reviews)

• Guard token-length arithmetic for scan {7, 9, 11, 13, 15} and apply {14, 16} — all correct
• truncated-not-inventoried blocker logic — ancestor, descendant, and exact-match cases all handled correctly at lines 1155–1161; now defense-in-depth is truly independent of the live-walk short-circuit
• --max-depth guard regex [1-9][0-9]{0,3} — rejects "007" and "-1" at the guard layer
• Approval token anti-replay — binding to nonce + snapshot digest + tier + candidates is solid
• os_autoclean_advisory pre-walk placement — included in failure payloads; no structural refactoring needed
• _data_root_key uses strict=False — correct for not-yet-created data directories
• Standing-policy isolation in tests via mock.patch.object(hygiene, "standing_policy_paths", ...)
• plugin.json version bump to 0.3.0 and CHANGELOG format — accurate

---

Bottom line

All five prior findings and both Codex P2 items are resolved. The implementation is mechanically correct, the new tests are well-targeted, and no regressions are introduced. This is ready to merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e933fc3795

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

---

Tasks

• Gather context — inspect new commit(s) since last review
• Review hygiene.py — verify remaining truncated-candidate short-circuits (tracked_blocker, candidate_handle_state)
• Check all prior findings remain resolved
• Synthesize and post updated review

---

PR Review — fix(disk-hygiene): unblock guarded engine lane and bounded large-root audits

Reviewed commit: 4ecf1e2 — "fix(disk-hygiene): skip the remaining unbounded live checks for a truncated preview candidate"

This commit addresses the remaining Codex P2 item from the third review round. All prior findings remain resolved. Full verification below.

---

Remaining Codex P2 finding resolved

tracked_blocker and candidate_handle_state no longer called on truncated candidates

hygiene.py:1200

The second "truncated-not-inventoried" in blockers guard wraps both remaining live calls:

• tracked_blocker → discover_current_repositories → unbounded recursive scandir of the candidate subtree
• candidate_handle_state → lsof +D on POSIX → unbounded process-handle enumeration over the candidate subtree

Both are now skipped when the candidate is truncated, with state, detail = "unverified", "truncated-not-inventoried" set instead. The if state != "clear": blockers.append(...) call is inside the else branch, so no spurious handle-state blocker is added for the truncated path. The "unverified" state surfaces correctly in the candidate result at lines 1230–1231 (handle_state, handle_detail).

The comment is accurate: a truncated candidate is already hard-blocked and can never become approvable, so there is no value in running these probes.

Test updated correctly

test_hygiene.py:732

test_preview_skips_live_recursive_checks_for_truncated_candidate now patches all three functions (current_descendants, tracked_blocker, candidate_handle_state) to raise AssertionError if called — a clean regression guard covering the full surface. The prior handle_state mock returning ("clear", None) is correctly removed; that workaround is no longer needed since candidate_handle_state never runs. New assertions on handle_state == "unverified" and handle_detail == "truncated-not-inventoried" verify the truncated-path output contract.

---

All prior findings — status unchanged (all resolved)

Finding	Commit	Status
Truncated candidate calls current_descendants (was blocking)	e933fc3	✅ resolved
Truncated candidate calls tracked_blocker / candidate_handle_state (Codex P2)	4ecf1e2	✅ resolved
Entry-cap off-by-one (> → >=)	e933fc3	✅ resolved
Missing guard test for apply --data-root	e933fc3	✅ resolved
HygieneError from current_descendants escapes preview()	e933fc3	✅ resolved
DATA_ROOT_OVERRIDE global not reset on exit	e933fc3	✅ resolved

---

Bottom line

All flagged findings across three review rounds are resolved. The truncated-candidate preview path is now fully short-circuited at every live-I/O point (current_descendants, tracked_blocker/discover_current_repositories, candidate_handle_state/lsof +D). The implementation is correct and the test coverage is well-targeted. Ready to merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4ecf1e2b26

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

---

Tasks

• Gather context — inspect new commit(s) since last review
• Review hygiene.py — verify VCS-boundary and protected-directory truncation fix
• Check all prior findings remain resolved
• Synthesize and post updated review

---

PR Review — fix(disk-hygiene): unblock guarded engine lane and bounded large-root audits

Reviewed commit: fc6f46c — "fix(disk-hygiene): mark VCS-boundary and protected directories as truncated too"

This commit addresses the final Codex P2 item from the fourth review round. All prior findings remain resolved. Detailed verification below.

---

Final Codex P2 finding resolved

VCS-boundary and protected-directory entries now land in truncated_paths

hygiene.py:580–588

The fix is minimal and surgical — two truncated.append(relative) calls added at the correct positions in the existing if/elif/elif/else chain:

if path.name.casefold() in VCS_NAMES:
    subtotal = 0
    truncated.append(relative)          # ← added
elif protections:
    subtotal = 0
    truncated.append(relative)          # ← added
elif max_depth is not None and depth >= max_depth:
    subtotal = 0
    truncated.append(relative)          # pre-existing
else:
    subtotal = visit(path, depth + 1)

All three stop-recursion branches share the same underlying fact — descendants were not inventoried — and now produce identical observable behaviour: the directory lands in truncated_paths and is hard-blocked by "truncated-not-inventoried" in preview(), which then skips current_descendants, tracked_blocker, and candidate_handle_state.

The annotate_tracked interaction is automatically correct: VCS-boundary and protected directories now appear in the truncated list passed into annotate_tracked, so they contribute :(exclude) pathspecs to git ls-files just as --max-depth cutoffs already did.

Tests added and updated

test_hygiene.py:240 — the existing protected-folder scan test gains self.assertEqual(["Documents"], snapshot["truncated_paths"]), pinning that the protected folder is recorded in truncated_paths without --max-depth.

test_hygiene.py:764 — test_preview_skips_live_recursive_checks_for_unvisited_protected_candidate mirrors the existing truncated-candidate test: creates a Documents subtree, scans without --max-depth, asserts truncated_paths == ["Documents"], then runs preview() with Documents as a candidate while patching all three live-check functions to raise AssertionError if called. Asserts "truncated-not-inventoried" is present and "changed-since-scan" is absent.

The refuse_call() helper is extracted to module level (test_hygiene.py:42), eliminating the duplicate inline definition that would have appeared with two tests using the same pattern.

SKILL.md updated

SKILL.md:65 — truncated_paths description widened from "cut off by --max-depth" to "cut off by --max-depth, a protected root, or a VCS boundary", matching the now-unified behaviour. Also adds that preview skips live re-verification checks for truncated candidates, which was documented in code comments but not in the skill-facing doc.

---

All findings across all review rounds — final status

Finding	Commit	Status
VCS-boundary / protected dirs not in truncated_paths (Codex P2, round 4)	fc6f46c	✅ resolved
Truncated candidate calls current_descendants (was blocking, round 2)	e933fc3	✅ resolved
Truncated candidate calls tracked_blocker / candidate_handle_state (Codex P2, round 3)	4ecf1e2	✅ resolved
annotate_tracked unbounded on bounded scan (Codex P2, round 2)	e933fc3	✅ resolved
Entry-cap off-by-one (> → >=)	e933fc3	✅ resolved
Missing guard test for apply --data-root	e933fc3	✅ resolved
HygieneError from current_descendants escapes preview()	e933fc3	✅ resolved
DATA_ROOT_OVERRIDE global not reset on exit	e933fc3	✅ resolved

---

What's working well (unchanged from prior reviews)

• Guard token-length arithmetic for scan {7, 9, 11, 13, 15} and apply {14, 16} — all correct
• truncated-not-inventoried blocker logic — ancestor, descendant, and exact-match cases all handled correctly; now covers all three truncation sources uniformly
• --max-depth guard regex [1-9][0-9]{0,3} — rejects "007" and "-1" at the guard layer
• Approval token anti-replay — binding to nonce + snapshot digest + tier + candidates is solid
• os_autoclean_advisory pre-walk placement — included in failure payloads; no structural refactoring needed
• _data_root_key uses strict=False — correct for not-yet-created data directories
• Standing-policy isolation in tests via mock.patch.object(hygiene, "standing_policy_paths", ...)
• plugin.json version bump to 0.3.0 and CHANGELOG format — accurate

---

Bottom line: All findings across four review rounds and all Codex P2 items are fully resolved. The implementation is mechanically correct, the test coverage is comprehensive and well-targeted, and no regressions are introduced. Ready to merge.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fc6f46c8fc

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

---

Tasks

• Gather context — inspect new commit(s) since last review
• Review hygiene.py — verify cap-check revert and correctness
• Check all prior findings remain resolved
• Synthesize and post updated review

---

PR Review — fix(disk-hygiene): unblock guarded engine lane and bounded large-root audits

Reviewed commit: 47aa4bb — "fix(disk-hygiene): allow a snapshot at exactly the configured entry cap"

This commit addresses the final Codex P2 item raised after the > → >= fix in e933fc3. All prior findings remain resolved. Detailed verification below.

---

Final Codex P2 finding resolved

Cap check moved before entries.append() — exactly-at-cap scans now succeed

hygiene.py:603

The fix is a single-location reorder — the >= MAX_SNAPSHOT_ENTRIES guard moves from after entries.append(...) to before it:

# before: check AFTER append — exactly-at-cap was rejected
entries.append({...})
if len(entries) >= MAX_SNAPSHOT_ENTRIES:   # fired when len == cap → rejected cap-exact scans
    raise HygieneError(...)

# after: check BEFORE append — exactly-at-cap is allowed
if len(entries) >= MAX_SNAPSHOT_ENTRIES:   # fires when len == cap, about to add one more
    raise HygieneError(...)
entries.append({...})

With the new ordering and MAX_SNAPSHOT_ENTRIES = N: the error fires when the list already holds exactly N items and we are about to append a further entry. The snapshot at the throw site contains N items — the configured maximum. A scan that touches exactly N entries never triggers the guard at all. The error message "snapshot exceeds N entries" is accurate: the rejected traversal step would have pushed the count above N.

The existing test_capped_scan_reports_advisory_and_bounded_rerun_guidance (patching MAX_SNAPSHOT_ENTRIES = 1, 3 files) is unaffected: the guard still fires on the second file, the snapshot exits with status invalid-or-blocked, and all three assertions (exit 2, --max-depth in error, os_autoclean in payload) remain valid.

Two new boundary tests added

test_hygiene.py:789

• test_entry_cap_allows_exactly_the_configured_maximum — patches MAX_SNAPSHOT_ENTRIES = 2, creates exactly 2 files, asserts the snapshot succeeds with len(entries) == 2. Pins the boundary precisely.
• test_entry_cap_rejects_one_more_than_the_configured_maximum — same cap, 3 files, asserts HygieneError with message matching "exceeds 2 entries". Confirms the guard fires one step past the limit.

These two tests replace the implicit boundary testing the old off-by-one inherited from the > check, and are more readable than the previous indirect coverage via the capped-scan integration test.

---

All prior findings — final status

Finding	Commit	Status
Cap check off-by-one — now checks before append (Codex P2, round 5)	47aa4bb	✅ resolved
VCS-boundary / protected dirs not in truncated_paths (Codex P2, round 4)	fc6f46c	✅ resolved
Truncated candidate calls tracked_blocker / candidate_handle_state (Codex P2, round 3)	4ecf1e2	✅ resolved
Truncated candidate calls current_descendants (was blocking, round 2)	e933fc3	✅ resolved
annotate_tracked unbounded on bounded scan (Codex P2, round 2)	e933fc3	✅ resolved
Missing guard test for apply --data-root	e933fc3	✅ resolved
HygieneError from current_descendants escapes preview()	e933fc3	✅ resolved
DATA_ROOT_OVERRIDE global not reset on exit	e933fc3	✅ resolved

---

Bottom line

All findings across five review rounds and all Codex P2 items are fully resolved. The cap-check reorder is correct: exactly-at-cap scans succeed, one-over-cap scans raise, and the error message accurately describes the rejected state. The two new boundary tests are precise and well-named. Ready to merge.

[kyle-sexton]

Summary reply to the five claude[bot] review passes on this PR (5006549021, 5013037094, 5013159166, 5013181336, 5013204466, 5013222563) and the two Codex P2 threads not already covered inline. Per-finding classification:

#	Finding	Classification	Evidence	Reacted
1	Entry-cap check off-by-one (> should be >=, snapshot held 250,001 items when the error fired)	VALID — fixed, then corrected	Fixed in e933fc3; a follow-up Codex P2 (replied on its own thread) found the naive >= rejected an exactly-at-cap scan, corrected in 47aa4bb (check moved before entries.append).	👍
2	HygieneError from current_descendants (raised on a mount-state read failure) escaped preview() uncaught, aborting the whole preview instead of blocking one candidate	VALID — fixed	except (OSError, HygieneError): in commit e933fc3, consistent with the existing OSError handling one line above.	👍
3	Missing guard test for apply --data-root (only scan/preview were covered)	VALID — fixed	Added test_guard_apply_accepts_optional_authorized_data_root in e933fc3, mirroring the existing preview test; asserts the 16-token apply command decides "ask".	👍
4	DATA_ROOT_OVERRIDE module global left set after main() returns — latent test-isolation fragility	VALID — fixed	main() now resets it to None in a finally block in commit e933fc3, so it can never leak past one invocation regardless of exit path.	👍
5	PowerShell pass-through for non-enumerated mutation verbs (e.g. a third-party deletion cmdlet) is possible; asked to confirm the scope statement is acceptable	VALID (defer) — accepted, documented tradeoff, no code change	This is the intentional "raised bar, not fail-closed" design already stated in this PR's own SKILL.md (Windows/PowerShell notes) and PR body (defect 2): the engine's own containment plus the Linux-only Bash execution gate remain the actual deletion authority; the PowerShell lane only adds a permission prompt on top for support-work convenience. Enumerating every possible third-party PowerShell deletion cmdlet is an open-ended list that would need to be maintained forever and would still never be complete — not a narrow, scoped fix this PR should chase. Flagging as accepted residual risk rather than a defect to close here; a future PR can revisit if a concrete bypass is demonstrated.	👍
6	Truncated candidate does not short-circuit before current_descendants in preview() (originally raised independently by both a claude[bot] pass and a Codex P2 thread on the same code)	VALID — fixed	See the reply on the Codex inline thread (hygiene.py, current_descendants short-circuit) — fixed in e933fc3.	👍

All six items are now resolved (five fixed in code, one accepted-and-documented as an intentional tradeoff). The full commit sequence: e933fc3 (current_descendants short-circuit, git-query scoping, entry-cap >=, missing guard test, HygieneError catch, DATA_ROOT_OVERRIDE reset) → 4ecf1e2 (tracked_blocker/candidate_handle_state short-circuit) → fc6f46c (VCS-boundary/protected-dir truncation) → 47aa4bb (entry-cap exact-boundary correction). 64 → 67 tests, all green.