feat(disk-hygiene): gate unbounded scans of known-large targets at the engine

[kyle-sexton]

Root cause

The scan lane had no engine-level backstop for known-large targets. SKILL.md
only asked the invoking agent to add --max-depth 1 on a large root — a
prompt-level convention. If the agent forgets, skips it, or a differently-tuned
future agent weights that instruction differently, an unbounded recursive walk of
$HOME/%USERPROFILE% runs unconfirmed. Only the destructive apply lane was
gated; the scan's own time/resource cost was not. The home directory is a valid
target today (its basename — the username — is not a protected shell-folder name),
so nothing stopped the full walk.

Fix

Detect the user home directory deterministically in the engine (large_scan_reasons,
exact resolved-path equality — descendants like ~/projects stay ungated). A scan
of that target carrying neither --max-depth nor the new --confirmed-large-scan
flag now does a cheap top-level os.scandir probe and returns
large-target-confirmation-required (exit 5) instead of walking, mirroring the
apply lane's "ask before it is expensive" posture — moved earlier because the cost
here is time, not data loss. --max-depth is the preferred bounded response;
--confirmed-large-scan is the explicit, human-confirmed override (SKILL.md directs
the agent to AskUserQuestion first).

The change is an engine backstop reached through the Bash guard, so the guard's
strict scan grammar had to accept it: destructive_guard.py now strips at most one
valueless --confirmed-large-scan from the scan optionals and validates the
remainder as the existing flag/value-pair grammar, keeping the shape exact
(duplicate flag or a trailing value is denied).

Docs updated: SKILL.md section 1 (engine gate + the two flags, command block),
safety-model.md (scan-cost gate paragraph + the new guard flag). The prompt-level
--max-depth 1 guidance stays, now backed by the engine.

Tests

test_hygiene.py (86 tests pass, 4 platform-skips):

• test_home_target_without_bound_requires_confirmation_and_never_walks — asserts
  large-target-confirmation-required, exit 5, refuse_call("scan_tree") proves no
  walk, and no snapshot file is written.
• test_home_target_with_max_depth_proceeds / test_home_target_with_confirmed_flag_proceeds
  — both reach scan-complete and write the snapshot.
• test_ordinary_subdirectory_is_not_gated — a non-home target is unaffected.
• test_guard_scan_accepts_single_confirmed_large_scan_flag — guard allows the flag
  (alone, with --max-depth, with --policy) and denies a doubled flag or a
  trailing value.

Verification

• bash plugins/disk-hygiene/skills/clean/scripts/hygiene.test.sh → 86 pass / 4 skip.
• ruff check (0.15.20) → all checks pass on the three scripts.
• bash scripts/validate-plugins.sh → all manifests + catalog validate.
• markdownlint-cli2 on the three changed docs → 0 errors.
• Pre-existing ruff format drift on lines this PR does not touch (a 0.15.20-vs-pinned-0.15.21
  quirk; disk-hygiene's .test.sh runs unittest only) is intentionally left alone; my added
  lines are ruff-canonical.

Scope / residual risks

• Companion disk-hygiene: blanket 'any filesystem root' rejection has no reasoning; blocks legitimate non-OS volumes (e.g. Windows Dev Drive) #984 (drive-root reasoning) is out of scope. hard_protection root-rejection
  is untouched. large_scan_reasons covers only user-home; drive roots are rejected upstream
  today, so coverage is complete now. Deferred trigger: if disk-hygiene: blanket 'any filesystem root' rejection has no reasoning; blocks legitimate non-OS volumes (e.g. Windows Dev Drive) #984 makes a reasoned drive root
  a valid scan target, extend large_scan_reasons to cover it, or an unbounded drive-root walk
  would bypass this gate.
• Version/CHANGELOG conflicts with sibling lanes are expected; entries kept minimal and mergeable.

Fresh-docs mandate

No official docs fetched: the change is internal Python engine/guard logic plus skill/reference
prose, touching no documented plugin/hook/manifest schema (the semver version bump is not a
schema change). The fresh-docs mandate targets schema/behavior changes against upstream docs, which
this PR does not make.

Related

• disk-hygiene: blanket 'any filesystem root' rejection has no reasoning; blocks legitimate non-OS volumes (e.g. Windows Dev Drive) #984 - companion drive-root-reasoning fix, concurrent sibling lane; this PR deliberately does not touch hard_protection root-rejection (see Scope above).
• 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 handoff pointer to source-control:worktree for VCS worktree directories #986 - sibling issues surfaced by the same disk-hygiene audit session, addressed in parallel PRs; not closed by this PR.

Fixes #985

🤖 Generated with Claude Code

https://claude.ai/code/session_01SFq1q99cNeZjKhDzHv2BQw

[claude]

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

---

PR Review: feat(disk-hygiene): gate unbounded scans of known-large targets

• Gather context and understand the request
• Read changed files
• Review engine logic (hygiene.py)
• Review guard logic (destructive_guard.py)
• Review tests (test_hygiene.py)
• Review documentation updates (SKILL.md, safety-model.md)
• Review plugin manifest and CHANGELOG
• Post final review

---

This is a well-designed safety backstop. The code is correct, the guard grammar is tight, and the test coverage is solid. A few observations below — none are blockers, but a couple are worth considering.

---

Architecture

The gate placement in main() is correct: it fires after all precondition checks (mount state, system root, os_autoclean probe, hard_protection checks) but before scan_tree(). No filesystem walk can begin by omission. The refuse_call("scan_tree") pattern in the test proves absence of the walk beyond just checking the exit code — that's a good testing idiom.

---

Engine logic (hygiene.py)

Path comparison is exact and symmetric. large_scan_reasons compares os.path.normcase(os.fspath(target)) against os.path.normcase(os.fspath(resolved_home)) where both sides resolve symlinks — target via resolve(strict=True) at line 1583, resolved_home via resolve(strict=False) inside large_scan_reasons. This correctly ensures ~/projects is not gated — descendants are not matched.

One subtle asymmetry to note: resolve(strict=False) on Path.home() will not error if home is a dangling symlink, but resolve(strict=True) on target would have succeeded (since the target exists). In the exotic case where home is a live symlink today but broken at check time, the comparison might produce different canonical paths. This is genuinely exotic and the fail-open behavior (gate doesn't fire) is safe, so this is informational only.

state_output_path() is called before the gate fires (line 1610 vs lines 1612–1636). This means --data-root (or CLAUDE_PLUGIN_DATA) must be present even for the gate-only path. This is a pre-existing design constraint, not a regression — state_output_path() validates but doesn't write here — but consumers need to know the argument is required even when the gate fires. The test correctly exercises this by supplying --data-root.

Exit code 5 is new and undocumented. SKILL.md doesn't enumerate exit codes, and agents read the JSON status field rather than the exit code, so this is fine. But it's worth keeping in mind if any future tooling around this engine script keys on exit codes.

probe_error in the gate output. When top_level_entry_count succeeds, probe_error is None → serializes as JSON null. The test only asserts self.assertIn("os_autoclean", payload) and doesn't verify probe_error or note are present. These fields are present in the implementation, just not explicitly asserted in tests. Very minor.

---

Guard logic (destructive_guard.py)

The changes to classify_exact_engine_command are correct and tight:

• Uses .count() then .remove() — strips at most one occurrence, position-independent; duplicate flag detected before stripping.
• The even-length check {0, 2, 4, 6, 8} is re-applied to the stripped optionals: with 4 optional pair-flags remaining, max stripped length is 8, which fits. ✓
• Trailing-value case (--confirmed-large-scan v) is correctly denied: after stripping --confirmed-large-scan, ['v'] has length 1, not in {0, 2, 4, 6, 8} → None. ✓

The guard test test_guard_scan_accepts_single_confirmed_large_scan_flag covers: flag alone, with --max-depth, with --policy, doubled (denied), trailing value (denied). Complete.

---

Tests (test_hygiene.py)

The four new engine tests cover all meaningful paths cleanly:

Test	What it proves
test_home_target_without_bound_requires_confirmation_and_never_walks	Gate fires, exit 5, no walk, no snapshot written
test_home_target_with_max_depth_proceeds	--max-depth bypasses gate, scan completes
test_home_target_with_confirmed_flag_proceeds	--confirmed-large-scan bypasses gate, scan completes
test_ordinary_subdirectory_is_not_gated	Non-home target unaffected

The _home_target_fixture fixture count (loose.tmp + nested = 2 entries) matches the assertion self.assertEqual(2, payload["immediate_entries"]). ✓

One missing case: the tests don't cover what happens when top_level_entry_count fails (i.e., os.scandir raises OSError). In that case probe_error would be non-null and immediate_entries would be None. The gate still fires correctly (this is just a probe for the informational output), but a test demonstrating the graceful degradation would add confidence.

---

Documentation

SKILL.md section 1 accurately describes the engine gate and both bypass paths. The instruction to use AskUserQuestion before --confirmed-large-scan mirrors the apply lane's human-confirmation bar. The deferred drive-root concern noted in the PR body (#984) is not in the code inline — a brief comment near large_scan_reasons noting the extension point would make the deferred trigger discoverable for future contributors, though this is a minor style preference.

safety-model.md paragraph is accurate and clearly distinguishes scan-cost gating from hard-rejection of invalid targets.

---

Summary

No correctness bugs. The design is sound and the implementation is faithful to the PR description. The three items most worth acting on:

1. Missing probe_error failure test — low priority, but would complete coverage of the top_level_entry_count failure path. Fix this →
2. --data-root required even for gate-only path — consider noting this in SKILL.md so the engine-call block is unambiguous.
3. Deferred trigger for drive roots — a one-line comment near large_scan_reasons pointing to the #984 extension point would help future contributors.

Ready to merge as-is from a safety and correctness standpoint.

[claude]

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

---

Security Review — PR #1010

No critical or important security vulnerabilities found. The changes are security-positive overall. One suggestion-level finding follows.

---

Reviewed files

• destructive_guard.py
• hygiene.py
• test_hygiene.py
• SKILL.md
• safety-model.md

---

Guard flag-stripping analysis

The new --confirmed-large-scan stripping logic in destructive_guard.py L239–250:

optionals = list(tokens[7:])
confirmed = optionals.count("--confirmed-large-scan")
if confirmed > 1:
    return None
if confirmed:
    optionals.remove("--confirmed-large-scan")
if len(optionals) not in {0, 2, 4, 6, 8} or not _consume_optional_pairs(...):
    return None

• Duplicate flag (count > 1): correctly rejected.
• --confirmed-large-scan=value form: _literal_shell_words would produce a single token --confirmed-large-scan=value; count returns 0, len becomes odd, rejected by the set check.
• --confirmed-large-scan used as the value of another flag (e.g. --policy --confirmed-large-scan): after removal, the orphaned --policy leaves len=1, rejected.
• list.remove() removes only the first occurrence; since count == 1 is the only path reaching remove(), this is correct.
• The flag is constrained to tokens[7:] by the strict positional validation of --target/--output at tokens[3–6]; it cannot appear as a positional argument value.

No bypass found.

---

Home-directory comparison analysis

hygiene.py L548–569:

• target arrives already resolved via strict=True in main().
• resolved_home = home.resolve(strict=False): for an existing home directory, strict=False follows all symlinks identically to strict=True. The difference only matters for non-existent path tails.
• A symlinked home directory path passed as --target is caught earlier by has_linkish_component(target_input) before reaching large_scan_reasons.
• If the user passes the real resolved absolute path of a symlinked home (/actual/home/user where HOME=/home/user → /actual/home/user), resolved_home still resolves to /actual/home/user via strict=False, and the comparison correctly matches. Gate triggers.
• os.path.normcase handles Windows case-insensitivity correctly.

No bypass found through path manipulation.

---

SUGGESTION — Gate silently passes when HOME / USERPROFILE is absent from the environment

Severity: SUGGESTION  |  Confidence: LOW

hygiene.py L541–545

def user_home() -> Path | None:
    try:
        return Path.home()
    except RuntimeError:
        return None

Path.home() raises RuntimeError on POSIX when HOME is unset and the password database entry is unavailable. user_home() swallows this and returns None; large_scan_reasons() then returns []; the gate never fires. An agent running in an environment with HOME absent could pass the home directory's absolute path directly and receive an unbounded scan.

Mitigating factors: (a) The agent cannot unset HOME through the Bash guard — a command like unset HOME && python ... contains &, which _SHELL_EXPANSION_OR_OPERATOR_CHARS catches, causing _literal_shell_words to return None and the guard to deny the call. (b) HOME being unset is uncommon in real Claude Code sessions. (c) This is an inherent limitation of env-var-based home detection, not a flaw in the PR's logic.

A hardened approach would fall back to pwd / getpwuid(os.getuid()) when HOME is absent (Python's Path.home() already does this on some builds; behavior is platform-dependent). If this gate is ever expected to defend against a motivated agent rather than against accidental omission, the fallback is worth adding. For its stated purpose — guarding against a forgetful agent — the current behavior is acceptable.

---

Other areas checked — no findings

• No injection surfaces introduced. large_scan_reasons and top_level_entry_count operate on Path objects and os.scandir; no subprocess, shell, or template expansion is involved.
• No new subprocess paths. The existing git subprocess calls (discover_enclosing_git, annotate_tracked) are unchanged.
• top_level_entry_count bounded. Uses os.scandir for a single directory level only — does not recurse.
• --confirmed-large-scan prompt-bypass is by design. SKILL.md requires AskUserQuestion before emitting this flag; the engine cannot cryptographically prove human intent, and this is the same pattern as --execute. Not a new weakness.
• Test fixture safety. New tests mock user_home to a temp directory; no real home directory is touched.
• Docs changes. SKILL.md and safety-model.md additions accurately describe the new gate behavior; no misleading guidance introduced.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b54f81b97d

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

---

PR Review: feat(disk-hygiene): gate unbounded scans of known-large targets

• Gather context and understand the request
• Read changed files via git diff origin/main...HEAD
• Review engine logic (hygiene.py)
• Review guard logic (destructive_guard.py)
• Review tests (test_hygiene.py)
• Review documentation updates (SKILL.md, safety-model.md)
• Review manifest and CHANGELOG
• Post final review

---

The design is sound and the implementation is faithful to the PR description. One correctness issue worth addressing before merge, plus two informational observations.

---

Architecture

Gate placement in main() is correct: it fires after all precondition checks (mount state, system root, hard_protection / has_protected_path_component — lines 1575–1609) and before scan_tree() (line 1638). There is no path through which scan_tree can be reached on a home-directory target without either --max-depth or --confirmed-large-scan. The refuse_call("scan_tree") idiom in the tests proves this at the call level rather than just by checking the exit code — that's a good pattern.

---

Issue: path comparison is string-based, not filesystem-identity-based

hygiene.py L565–568

if os.path.normcase(os.fspath(target)) == os.path.normcase(
    os.fspath(resolved_home)
):

os.path.normcase is a no-op on every POSIX platform including macOS — it only lowercases on Windows. On a case-insensitive macOS filesystem (HFS+/APFS), Path('/users/alice').resolve(strict=True) returns /users/alice (input casing preserved), while Path.home().resolve(strict=False) returns /Users/alice (from the password database). The string comparison returns False, the gate does not fire, and an unbounded whole-home scan proceeds.

The correct comparison for filesystem identity is os.path.samefile, which compares device/inode numbers and is immune to casing:

try:
    if os.path.samefile(target, resolved_home):
        reasons.append("user-home")
except OSError:
    pass  # resolved_home unreachable — fail open, gate does not fire

The OSError catch is needed for the exotic case where resolved_home came from the strict=False fallback at line 564 and points to a dangling path. The existing normcase/fspath code can be replaced entirely with this block.

This gate exists to prevent accidental expense rather than to enforce a security boundary (an agent can always bypass with --confirmed-large-scan), so this is a correctness issue rather than a critical vulnerability. But macOS is the dominant development platform, and the fix is small. Fix this →

---

Guard logic (destructive_guard.py)

The changes to classify_exact_engine_command are correct:

• list.count() then list.remove() — strips at most one occurrence, position-independent.
• Duplicate flag (count > 1) is caught before stripping. ✓
• --confirmed-large-scan=value form becomes a single token that doesn't match "--confirmed-large-scan", so count returns 0; the trailing value then lands in the pair grammar where length will be odd → not in {0, 2, 4, 6, 8} → denied. ✓
• --confirmed-large-scan used as the value of a pair flag (e.g., --policy --confirmed-large-scan) — after removal, the orphaned --policy leaves an odd-length list → denied. ✓
• The new length bound < 7 combined with the post-strip even-length check correctly supersedes the old {7, 9, 11, 13, 15} literal set. Max with the new flag: 7 positional + 1 confirmed-flag + 8 pair tokens = 16, within the implicit ceiling. ✓

Guard test coverage is complete: flag alone, with --max-depth, with --policy, doubled (denied), trailing value (denied).

---

Tests (test_hygiene.py)

The four new engine tests cover all meaningful gate paths:

Test	What it proves
test_home_target_without_bound_requires_confirmation_and_never_walks	Gate fires, exit 5, no walk, no snapshot written
test_home_target_with_max_depth_proceeds	--max-depth bypasses gate, scan completes
test_home_target_with_confirmed_flag_proceeds	--confirmed-large-scan bypasses gate, scan completes
test_ordinary_subdirectory_is_not_gated	Non-home target with a different mocked home is unaffected

The fixture count (loose.tmp + nested/ = 2) matches the immediate_entries == 2 assertion. ✓
mock.patch.object(hygiene.Path, "home", return_value=root.resolve()) correctly patches at the class level before user_home() calls it. ✓

One missing path: the probe_error failure case — when os.scandir raises OSError, immediate_entries is None and probe_error is a non-empty string. The gate still returns exit 5 correctly, but a test demonstrating graceful degradation would complete coverage. Low priority.

---

Documentation

SKILL.md section 1 accurately describes the engine gate, the two bypass flags, and the AskUserQuestion requirement before --confirmed-large-scan. The instruction is well-calibrated: --max-depth is presented as the preferred response, --confirmed-large-scan as the human-confirmed override.

safety-model.md correctly distinguishes scan-cost gating (time/resources) from hard rejection of invalid targets (system roots, mount points). The paragraph is clear and accurate.

CHANGELOG entry is minimal and mergeable. Version bump 0.4.7 → 0.5.0 is appropriate for a new engine gate.

---

Summary

One item to address before merge:

1. os.path.normcase is a POSIX no-op — use os.path.samefile instead (gate misses case-mismatched home paths on macOS). Small fix, high correctness value.

Two informational items (no action required):

2. Missing probe_error failure test — the OSError path through top_level_entry_count is untested but correct by inspection.
3. --data-root is required even in the gate-only path — state_output_path() is called at line 1610 before the gate fires. Pre-existing design; consumers passing only --target without --data-root get an early error rather than the gate response. Not a regression, but worth a note in SKILL.md's command block.

[claude]

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

---

Security Review — PR #1010

• Gather context and understand the request
• Read changed files via diff
• Review hygiene.py — path comparison, gate logic, probe
• Review destructive_guard.py — flag-stripping grammar
• Review test_hygiene.py — fixture isolation, mock surfaces
• Review docs (SKILL.md, safety-model.md)
• Confirm no GitHub Actions / workflow changes
• Post final security review

---

No critical or important security vulnerabilities found in the new code. One finding follows — it echoes the Codex P2 comment with additional analysis on exploitability.

---

SUGGESTION — Gate comparison misses case-insensitive macOS filesystems

Severity: SUGGESTION  |  Confidence: MEDIUM

hygiene.py L565–567

if os.path.normcase(os.fspath(target)) == os.path.normcase(
    os.fspath(resolved_home)
):

On POSIX (including macOS), os.path.normcase is defined as the identity function — it does not lowercase the path. On macOS's default case-insensitive APFS/HFS+ filesystem, Path.resolve() does not canonicalize case either; it follows symlinks via stat/readlink syscalls, which succeed for any casing but return the caller's casing unchanged. As a result, these two resolved paths compare unequal even though they denote the same directory:

target         = /users/alice   (agent-supplied, lowercase)
resolved_home  = /Users/alice   (from HOME env var or pwd database, canonical casing)
normcase result → '/users/alice' != '/Users/alice'  # gate does not fire

An agent that passes the home directory with any differing capitalization receives an unbounded scan instead of the gate response. Mitigation requires no user-visible change in normal operation because correctly-cased paths still compare equal.

Recommended fix: Replace the string comparison with os.path.samefile(), which delegates to the OS stat with the same inode across both paths and is case-insensitive on case-insensitive filesystems. target is guaranteed to exist at this point (it passed resolve(strict=True) at L1583); the home directory also typically exists.

try:
    if target.samefile(resolved_home):
        reasons.append("user-home")
except OSError:
    pass  # if home is unavailable, fail open (safe — this is cost gating, not data-loss gating)

Mitigating factors keeping this at SUGGESTION rather than IMPORTANT:

• The gate is scan-cost gating (time and resources), not data-loss prevention. The apply lane has its own separate guard. An unbounded scan does not itself delete anything.
• Agents reaching this gate through normal shell expansion (~ → $HOME) receive the canonical-cased path from the environment and will match correctly.
• Exploiting this requires the agent to intentionally craft a differently-cased absolute path to the home directory — a non-default, unusual invocation.
• has_linkish_component at L1578 checks for symlinks but does not prevent case variants, so there is no upstream filter catching this.

Fix this →

---

Areas checked with no findings

Guard flag-stripping — _literal_shell_words rejects every shell operator character (&, ;, |, $, etc.) before the token stream reaches the guard logic. --confirmed-large-scan cannot be injected as a positional value for another flag because _valid_optional_value rejects any value starting with -, and --max-depth requires a digit-only regex match. Duplicating the flag is caught by the count > 1 check before any removal. The even-length check on the stripped optionals is correctly re-applied and the maximum stripped length (8) fits within the {0, 2, 4, 6, 8} set. No bypass found.

Path manipulation via --target — target arrives at large_scan_reasons only after passing exists(), is_dir(), has_linkish_component(), mount-state validation, and system-root rejection. There is no injection surface: large_scan_reasons and top_level_entry_count operate on Path objects only, with no subprocess or template interpolation.

Home-absent environment — The prior Claude security comment covered this in detail (LOW confidence suggestion). The guard's shell-operator rejection (& in unset HOME && python ...) prevents the most obvious attempt at exploiting an absent HOME to bypass the gate. This is pre-existing design rather than a new weakness introduced by this PR.

No new subprocess paths — top_level_entry_count uses os.scandir only (one directory level, no recursion). No new subprocess calls were introduced.

No GitHub Actions / workflow changes — The seven changed files are all within the plugin skill; no .github/workflows/ files were touched.

Test fixture isolation — New tests mock user_home to a tempfile.mkdtemp() directory; no real home directory is touched at test time.

Docs — No misleading guidance. SKILL.md's requirement to use AskUserQuestion before --confirmed-large-scan matches the apply lane's human-confirmation bar; there is no stronger guarantee the engine could provide.

---

[claude]

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

---

PR Review: feat(disk-hygiene): gate unbounded scans of known-large targets

• Gather context and understand the request
• Review incremental change (synchronize commit 55caf1e)
• Verify samefile fix is correct and complete
• Check updated tests
• Post final review

---

The synchronize commit (55caf1e) addresses the os.path.normcase/macOS case-sensitivity issue flagged by both prior Claude reviews and the Codex P2 comment. This is the only open correctness item. Review below.

---

Fix: os.path.samefile in large_scan_reasons

hygiene.py L565–575

try:
    same_home = os.path.samefile(target, home)
except OSError:
    same_home = False
if same_home:
    reasons.append("user-home")

The implementation is correct on all axes:

• Platform correctness. os.path.samefile compares device + inode, which is immune to path casing and spelling. The prior os.path.normcase/string-equality approach was a no-op on POSIX and would let /users/alice bypass the gate on a case-insensitive APFS/HFS+ volume.
• Exception scope. OSError is the right catch — it's the base for FileNotFoundError, PermissionError, and every other stat-failure variant. target is guaranteed to exist upstream (resolve(strict=True)); the only realistic OSError here is an unstattable home.
• Fail-open semantics. Treating an unstattable home as no-match is correct: the gate is cost-gating (preventing an accidental expensive walk), not a security boundary. Fail-open means the edge case degrades to "scan proceeds" rather than "scan crashes", which is the right tradeoff.
• Docstring. The updated docstring in large_scan_reasons explains why samefile is used and names the exact failure mode of the prior approach. This is exactly the kind of non-obvious invariant worth documenting inline.

---

Updated test: test_ordinary_subdirectory_is_not_gated

The prior version patched Path.home to return (base / "home").resolve() without creating that directory on disk. With samefile, an unstattable home path raises OSError → caught → same_home = False → gate doesn't fire. The test would still pass, but for the wrong reason (fail-open error path, not a genuine identity mismatch).

The new version creates home.mkdir() first, so samefile can successfully stat both paths, confirm different inodes, and return False. This is the correct test path: the test now proves "different real directories → gate does not fire", not "missing home → gate fails open".

---

New regression tests

test_home_match_is_by_filesystem_identity_not_case_folded_string — the design is sound: mock both user_home and os.path.samefile (via "os.path.samefile" — correct patch target since hygiene accesses it through the os module reference at call time). The assertNotEqual on the raw path strings proves the old code would have missed this, and samefile.assert_called_once_with(target, home) confirms the right call shape.

test_home_match_treats_unstattable_home_as_no_match — covers the OSError fail-open path directly. FileNotFoundError is a correct side_effect choice (it's a subclass of OSError).

Both tests are tightly scoped and add exactly the coverage that was missing. Together with the four end-to-end gate tests from the first commit, the critical paths are all exercised.

---

Remaining informational items (no action required for merge)

The two low-priority items from prior reviews remain unaddressed — neither blocks merge:

1. probe_error failure test — when os.scandir raises OSError, immediate_entries is None and probe_error is a string. The gate still fires correctly; the graceful-degradation path is correct by inspection but untested.
2. --data-root required on gate-only path — state_output_path() is called before the gate check, so consumers omitting --data-root see an early error rather than the gate response. Pre-existing design, not a regression. The SKILL.md command block implies --data-root is always required, which is accurate — just not called out explicitly for this case.

---

Summary

The samefile fix is correct, complete, and well-tested. All correctness issues raised in prior reviews are resolved. No outstanding findings block merge.

[claude]

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

---

Security Review — PR #1010

• Gather context
• Fetch PR diff
• Read changed files (hygiene.py, destructive_guard.py, test_hygiene.py, docs)
• Analyze for security vulnerabilities
• Post findings

---

No critical or important security vulnerabilities found. The second commit (55caf1e) correctly resolves the macOS path-comparison bypass identified by prior reviews. One suggestion-level note follows, then a full areas-checked list.

---

Guard grammar — --confirmed-large-scan stripping

destructive_guard.py L239–250

The .count() → > 1 check → .remove() pattern is correct. I traced every injection angle:

• Flag used as a pair value (--max-depth --confirmed-large-scan): after stripping, the orphaned --max-depth leaves len=1, not in {0,2,4,6,8} → denied. ✓
• Flag used as --data-root value: _argument() rejects any value starting with -, so the guard would never reach the strip step with this arrangement — the pair grammar itself rejects it. ✓
• --confirmed-large-scan=value form: the =-joined token is a single literal word; .count("--confirmed-large-scan") returns 0; the odd-length remainder falls outside {0,2,4,6,8} → denied. ✓
• Duplicate flag: confirmed > 1 → return None before any removal. ✓
• Five pair-flags + flag: stripped optionals length = 10, not in {0,2,4,6,8} → denied. ✓

No bypass found in the guard grammar.

---

Home-directory comparison — os.path.samefile fix

hygiene.py L562–569

The fix correctly replaces the os.path.normcase string comparison with os.path.samefile. On POSIX, normcase is the identity function — a lowercase --target /users/alice on macOS (where HOME is /Users/alice) would have produced unequal strings under the old code, letting the gate silently pass. samefile delegates to (st_dev, st_ino) via stat(2), which is OS-level identity and handles case-insensitive APFS/HFS+ correctly.

target arrives at large_scan_reasons having already passed resolve(strict=True) (line 1589) and has_linkish_component (line 1583–1585), so it is the canonical inode-addressable path. home from Path.home() is unresolved, but samefile calls stat() on both sides independently — symlink indirection in either path is handled correctly.

The OSError catch (treating an unstattable home as no-match) is the right fail-open direction for a cost gate: if the home directory cannot be stat'd, no unbounded walk is in progress anyway.

---

SUGGESTION — _is_current_python guard still uses normcase for the string pre-filter

Severity: SUGGESTION  |  Confidence: LOW

destructive_guard.py L28–38

def _is_current_python(value: str) -> bool:
    ...
    return (
        os.path.normcase(os.fspath(candidate))
        == os.path.normcase(os.fspath(runtime))
        and os.path.samefile(candidate, runtime)
    )

This is pre-existing code, not introduced by this PR, but it is relevant in the context of the macOS normcase fix made elsewhere. The string pre-filter (normcase) is a no-op on POSIX, yet the subsequent samefile check is authoritative, so the comparison is already correct on macOS — the samefile call determines the outcome. The code is sound, just verbose: on POSIX, normcase(a) == normcase(b) is always true for strings that differ only in case, so the guard on the and side is always samefile. Functionally safe; no bypass possible because samefile is the binding check. Worth noting for future consistency if the hygiene.py pattern of using samefile-only is adopted here too, but not a security issue.

---

Areas checked with no findings

No injection surfaces. large_scan_reasons, top_level_entry_count, and the guard stripping logic all operate on Path objects and literal string tokens. No subprocess, shell expansion, or template interpolation is introduced.

No new subprocess paths. top_level_entry_count uses os.scandir for a single directory level only — it never recurses. Existing git subprocess calls (discover_enclosing_git, annotate_tracked) are unchanged.

HOME-absent environment. user_home() catches RuntimeError from Path.home() and returns None, causing the gate to fail open. As noted in the prior security review, the most obvious exploit path (unset HOME && python ...) requires a shell operator (&&) that _literal_shell_words rejects via _SHELL_EXPANSION_OR_OPERATOR_CHARS. This is a pre-existing limitation acknowledged in the PR scope.

--confirmed-large-scan bypass is by design. The gate is scan-cost gating (time and resources), not data-loss prevention; the apply lane has its own separate guard. SKILL.md's AskUserQuestion requirement mirrors the apply lane's human-confirmation bar. The engine cannot cryptographically prove human intent, and this is the same pattern as --execute.

Test fixture isolation. New tests mock user_home to a tempfile.mkdtemp() directory; no real home directory is touched at test time. The test_home_match_is_by_filesystem_identity_not_case_folded_string test patches os.path.samefile globally — acceptable for unit testing; no security implication.

No GitHub Actions / workflow changes. The seven changed files are all within the plugin skill directory; no .github/workflows/ files were touched.

Docs changes. SKILL.md and safety-model.md additions accurately describe the new gate behavior. The AskUserQuestion requirement before --confirmed-large-scan is clearly stated; no misleading guidance introduced.

[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.