fix(disk-hygiene): qualify unknown and non-reclaimable byte counts

[kyle-sexton]

Summary

Completes finding 2 of #1806 (byte accounting cannot express "unknown" / "not reclaimable local bytes"). Findings 1 and 5 already shipped in 0.13.0 via PR #1820.

• Truncated directories now record logical_size: null with a not-walked qualifier instead of looking empty (0).
• Every entry records nlink and cheap allocated_size (st_blocks * 512 on POSIX; null on Windows).
• Files with st_nlink > 1 carry hardlinked; sparse files carry sparse when the platform exposes the signal.
• Snapshot / preview / apply / scan-complete report reclaimable_local_bytes* as a figure distinct from the walked logical roll-up; qualified entries are excluded.

Closes #1806

Related

• Refs disk-hygiene: hint matching is misanchored and case-sensitive, byte accounting cannot express "unknown", and five smaller findings from a live audit #1806 — findings 3 (summarize surface), 4 (Stop-detector marker amortisation), 6 (probe path provenance vs trusted settings), and 7 (run-state retention / snapshot path containment) need design or coupled-grammar calls; left as follow-ups, not in this PR.
• Refs disk-hygiene: cloud-sync placeholders are unprotected, unhinted, and counted as reclaimable bytes — deleting one destroys the org's cloud copy #1804 / PR fix(disk-hygiene): hard-protect cloud-sync placeholders and tenant sync roots #1818 — landed the size_qualifiers / file_attributes mechanism this builds on.
• Refs PR fix(disk-hygiene): match globs case-insensitively and complete the denial allow-list #1820 — already closed findings 1 and 5 on this issue.

Test plan

• python -m unittest -v plugins/disk-hygiene/skills/clean/scripts/test_hygiene.py — 245 tests, OK (4 skipped)
• New coverage: truncated-vs-empty contrast, hardlink exclusion from reclaimable, nlink/allocated_size shape, protected sync-root not-walked
• Do not merge from the worker lane

[claude]

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

---

Reviewing PR #1870

• Gather context (PR diff, changed files)
• Read CLAUDE.md / REVIEW.md for review scope
• Review hygiene.py changes (hint matching, byte accounting)
• Review test coverage in test_hygiene.py
• Review CHANGELOG.md and SKILL.md updates
• Post final review feedback

This repo has a claude-security-review workflow, so per REVIEW.md this lane omits security findings and covers correctness/design/tests/docs only.

Findings

🔴 Important — plugin.json version wasn't bumped to match the new CHANGELOG entry
plugins/disk-hygiene/.claude-plugin/plugin.json#L4 still reads "version": "0.13.0", while CHANGELOG.md#L6 adds a new ## [0.14.0] section for these changes. CLAUDE.md requires an explicit semver bump per release so "consumers update on bumps," and scripts/check-changelog-parity.sh --check-bump only fires in the other direction (manifest version changed ⇒ changelog must gain an entry) — it won't catch a changelog-only bump with no manifest change, so this will merge green. Bump plugin.json's version to 0.14.0.

🔴 Important — permission-denied subtrees still collapse to logical_size: 0, the exact bug this PR fixes elsewhere
hygiene.py#L921-L930: when os.scandir raises OSError (e.g. a permission-denied directory), visit() records it in errors and returns 0 — it does not add the path to truncated or set walked=False. That directory's metadata() call therefore gets walked=True, logical_size=0 with no not-walked qualifier, i.e. indistinguishable from a genuinely empty directory — the identical symptom this PR eliminates for VCS/protected/max-depth truncation (docstring at hygiene.py#L662-L664, CHANGELOG's "byte accounting can say unknown" claim). SKILL.md tells the reporting agent to treat errors entries as coverage gaps, but the snapshot's own target_reclaimable_local_bytes/logical_size numbers still silently treat that subtree as zero bytes rather than unknown. Worth covering with the same walked=False / not-walked / truncated treatment, or at least noting the gap in the CHANGELOG's follow-up list alongside findings 3/4/6/7.

🟡 Suggestion — target's own not-walked qualifier doesn't mean what the docstring says it means
metadata()'s docstring states "A truncated directory's logical_size is null, never 0" (hygiene.py#L662-L664), and SKILL.md repeats this as a general reader rule. But hygiene.py#L999-L1006 mutates target_identity["size_qualifiers"] to add not-walked after the fact, while intentionally keeping logical_size as the non-null partial sum. That's a reasonable choice for the root (documented in the adjacent comment), but it means not-walked ⟹ logical_size is null holds for every entry except the one root entry, via a different code path than the walked parameter that enforces it everywhere else. A one-line callout in the metadata() docstring (or a distinct qualifier name for "partially walked" vs "not walked at all") would keep the invariant honest for anyone reading the function in isolation.

🟡 Suggestion — PR title/description scope doesn't match the diff
The title and summary describe "hint matching anchoring/case-sensitivity" fixes, but nothing in this diff touches matching_hints, glob_matches, or any hint-related code — confirmed via git show on the commit and git log on hygiene.py (case-insensitive glob matching already landed in #1820/b50498b). The diff is entirely the byte-accounting work (issue #1806 finding 2), which is solid and well-tested. Worth tightening the title/summary so the CHANGELOG and PR history don't imply a second fix that isn't actually present in this change.

Not reviewed here (by design)

Security-relevant aspects (e.g. the os.open(..., O_NOFOLLOW) / identity-check paths in apply_plan) are left to the security lane per REVIEW.md.

Test execution

Sandbox restrictions in this session blocked running python3/pytest directly, so I reviewed test_hygiene.py statically rather than executing it — the new tests (test_truncated_directory_logical_size_is_unknown_not_zero, test_hard_linked_names_are_qualified_and_excluded_from_reclaimable, test_metadata_records_nlink_and_allocated_size_shape) match the implementation's actual behavior and look correct on inspection.

[claude]

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

---

Security Review — PR #1870

• Gather context (PR diff, changed files)
• Review hygiene.py changes for logic/security issues
• Review test_hygiene.py, SKILL.md, CHANGELOG.md
• Post findings

Scope reviewed: git diff 6c97b3f...056dad9 -- plugins/disk-hygiene/ (the actual PR diff — 173 lines in hygiene.py, plus tests/docs). Note: despite the PR title mentioning "hint matching," the glob_matches/matching_hints case-insensitivity logic was already present on main prior to this PR; this PR's code changes are entirely the byte-accounting vertical slice (allocated_size, nlink, hardlinked/sparse/not-walked qualifiers, reclaimable_local_bytes* fields).

Findings: none.

This diff is confined to reporting/accounting arithmetic in the disk-hygiene engine:

• New fields (allocated_size, nlink, size_qualifiers additions, reclaimable_local_bytes*) are derived read-only from os.lstat() results and only affect what byte totals are reported in snapshot/preview/apply JSON output.
• I specifically checked whether the subtotal = 0 → subtotal = None change for truncated directories, or the target_identity["size_qualifiers"] mutation, could weaken the safety gates that actually govern deletion:
  • The truncation branches (protected name, VCS boundary, max_depth) still skip recursing into those directories exactly as before — only the recorded value changed (None/not-walked instead of 0), not the control flow that stops the walk.
  • same_object_identity() (used both for resolve_snapshot_target's "target wasn't replaced" check and apply_plan's pre-delete re-check) compares kind/device/inode/mode only — it does not look at size_qualifiers, so appending "not-walked" to target_identity["size_qualifiers"] has no effect on that identity/tamper check.
  • entry_reclaimable_local_bytes()/reclaimable_local_bytes() are purely additive report fields; they are not consulted anywhere in the blocker/approval logic (hard_protection, validate_plan, preview's blockers, or apply_plan's deletion loop), so nothing about what gets deleted changes — only the reported byte count for what already gets deleted.
• No new subprocess/command construction, file writes, path joins from untrusted input, or credential/token handling was introduced or touched by this diff.
• No GitHub Actions files are touched by this PR, so the workflow-trigger/permissions/supply-chain lane is not applicable here (left to zizmor per review scope).

I reviewed CHANGELOG.md and SKILL.md changes as well — documentation only, no security-relevant content.

No findings to report for this PR.

[claude]

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

---

Security Review — PR #1870 (HEAD 846a9b7)

• Gather context (PR diff, changed files)
• Review new commit (846a9b7) for security issues
• Review hygiene.py byte-accounting / hint-matching changes
• Review test_hygiene.py additions
• Post findings

Findings: none.

The commit added since the prior review (056dad9 → 846a9b7) only touches plugin.json (version bump 0.13.0 → 0.14.0) and test_hygiene.py (adjusts the truncated-vs-empty test's max_depth fixture from 1 to 2, so deep/sub — not deep itself — is the truncated case). Neither is a functional or security-relevant change; hygiene.py is unmodified since the prior review.

Re-confirming the prior review's core claim against the code directly: same_object_identity() (plugins/disk-hygiene/skills/clean/scripts/hygiene.py:1284-1293) is the pre-delete identity re-check in apply_plan, and it compares only kind/device/inode/mode — it does not inspect size_qualifiers, allocated_size, nlink, or any of the new byte-accounting fields. So the reclaimable_local_bytes*/allocated_size/nlink/not-walked additions in this PR remain purely additive reporting fields with no path into the blocker/approval/deletion logic (hard_protection, validate_plan, preview's blockers, apply_plan's deletion loop).

No new subprocess/command construction, path joins from untrusted input, or credential/token handling in this PR. No .github/workflows files touched.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 056dad90a6

ℹ️ 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".

[github-actions]

Claude has reviewed this PR 5 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[claude]

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

---

Reviewing PR #1870 (HEAD 846a9b7)

• Re-gather context (git diff 2d4e276..846a9b7, matches the PR's stated 251/-24 across 5 files)
• Confirm what changed since the last code-review pass (056dad9 → 846a9b7)
• Re-check the two "Important" findings from the prior pass against current code
• Weigh in on the two Codex review comments
• Post updated feedback

Per REVIEW.md, this lane covers correctness/design/tests/docs; security-relevant aspects are left to the dedicated security lane, which already cleared this diff.

What the latest commit fixed

846a9b7 bumps plugin.json to 0.14.0 (now matches the CHANGELOG.md ## [0.14.0] entry) and fixes the truncated-vs-empty test to use max_depth=2 so deep/sub — not deep — is the truncated case. This resolves the prior "Important" version-bump finding and independently fixes the bug Codex's P1 comment flagged on test_hygiene.py:414 (with max_depth=1, empty itself would have been truncated, not genuinely walked-and-empty). Good fix.

Still open

🔴 Important — permission-denied subtrees still collapse to logical_size: 0, indistinguishable from empty
hygiene.py#L921-L930: when os.scandir raises (e.g. permission denied), visit() logs to errors and returns 0. The caller (hygiene.py#L946-L963) already set walked = True before calling visit, so that subtotal of 0 is recorded as logical_size: 0, walked=True — no not-walked qualifier, no truncated entry. That's the exact symptom this PR eliminates for VCS/protected/max-depth truncation. target_reclaimable_local_bytes will silently under-report a directory that couldn't actually be inventoried. This still isn't covered by a test. Worth giving this path the same walked=False/truncated treatment (or explicitly noting the gap in the CHANGELOG's follow-up list next to findings 3/4/6/7) before merge.

🟡 Suggestion — hard-link qualification can go stale between snapshot and delete (Codex P2, hygiene.py:1666)
Confirmed this is real: same_stat_identity (hygiene.py#L1261-L1281) doesn't check st_nlink, and entry_reclaimable_local_bytes derives its verdict entirely from the snapshot-time size_qualifiers. If a link to a candidate file is added after the snapshot but before preview/apply, the file keeps counting as fully reclaimable even though deleting the one name it's tracking wouldn't free those bytes. This doesn't affect the security-relevant deletion/blocker logic (confirmed by the security lane) — it's purely a byte-accounting staleness gap, but it's directly in-scope for a PR whose entire purpose is fixing byte accounting. Not a blocker, but worth a follow-up (re-stat nlink at preview time, or note the known gap in SKILL.md/CHANGELOG).

🟡 Suggestion — docstring invariant is true almost everywhere but not universally
hygiene.py#L662 states "A truncated directory's logical_size is null, never 0," but hygiene.py#L998-L1006 deliberately keeps the target's own logical_size as the non-null partial sum while adding not-walked after the fact — a reasonable choice for the root, but it means not-walked ⟹ logical_size is null holds everywhere except this one path. A one-line callout in the docstring would keep this honest for anyone reading metadata() in isolation.

🟡 Suggestion — PR title/description still describes a fix not present in this diff
Title and summary mention "hint matching anchoring/case-sensitivity," but nothing in 2d4e276..846a9b7 touches matching_hints/glob_matches — that landed separately in #1820 (b50498b). The diff here is entirely the byte-accounting vertical slice for issue #1806 finding 2, which is solid and well-tested. Worth retitling before merge so history/CHANGELOG don't imply a second fix that isn't actually in this change.

Test coverage

The three new tests (test_truncated_directory_logical_size_is_unknown_not_zero, test_hard_linked_names_are_qualified_and_excluded_from_reclaimable, test_metadata_records_nlink_and_allocated_size_shape) correctly exercise the truncation/hardlink/shape behavior implemented, and the max_depth=2 fix makes the truncated-vs-empty contrast actually test what it claims to. No test exists yet for the permission-denied-subtree gap above.

[claude]

Claude encountered an error after 1s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 1s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 1s —— View job

---

I'll analyze this and get back to you.

[claude]

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

---

Security Review — PR #1870 (HEAD 07d6b57)

• Gather context (diff since last reviewed commit 846a9b7)
• Review new commit 07d6b57 for security issues
• Re-confirm no regressions in previously-cleared code
• Post findings

Findings: none.

Scope reviewed: the only change since the last security pass (846a9b7 → 07d6b57) is commit 07d6b57, which touches a single function in hygiene.py:

same_stat_identity(), hygiene.py#L1261-L1285 now additionally compares st_nlink for kind == "file" entries. This directly resolves Codex's prior P2 comment (hard-link staleness between snapshot and delete).

This is a strictly stricter identity check, and it fails closed:

• same_stat_identity() is the pre-delete re-verification used at hygiene.py#L1946 (apply_plan's anchored-unlink path) and the drift check in preview at hygiene.py#L1803. Before this fix, a hard link added to a candidate file after scan (which leaves size/mtime/dev/ino/mode unchanged) would pass the identity check silently. Now it fails the check — preview reports changed-since-scan and apply_plan raises HygieneError("anchored entry changed since the snapshot") and refuses to unlink, instead of proceeding.
• nlink is unconditionally populated by metadata() (hygiene.py#L668, #L694) for every entry, so entry.get("nlink") is never missing for a kind == "file" snapshot record — no None-vs-int comparison surprises.
• The value is read straight from os.lstat()/os.fstat() — no user-controlled input flows into the comparison, and no new subprocess, path-join, or symlink-following behavior was introduced.
• same_object_identity() (the directory/O_NOFOLLOW-open identity check used for anchored directory removal) is untouched by this commit and still doesn't consider size_qualifiers or nlink, which is correct — that check exists for directories, not hard-linkable regular files.

Net effect: this change makes the deletion-time safety check more conservative (more likely to abort a deletion when the on-disk file has changed in a way that matters), not less. No new attack surface, no weakening of hard_protection/validate_plan/apply_plan's blocker logic.

Out of scope confirmation: the wider diff between the two review points (846a9b7..07d6b57) also shows unrelated files (guardrails, knowledge, repo-fleet-hygiene, work-items) — these came from a main merge into this branch (commit b17ea56), not from this PR's own commits, and are excluded per this lane's scope (only files changed by this PR, i.e., the 5 files in the PR's file list). This PR's actual diff remains: plugin.json, CHANGELOG.md, SKILL.md, hygiene.py, test_hygiene.py — consistent with prior reviews.

No GitHub Actions files touched by this PR.

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[claude]

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

---

Reviewing PR #1870 (HEAD 7e5adf9)

• Gather context (diff since last reviewed commit 846a9b7 → 07d6b57 → 7e5adf9)
• Review new commits for correctness/test coverage
• Re-check previously open findings against current code
• Post inline findings and summary

Per REVIEW.md, this lane covers correctness/design/tests/docs; security-relevant aspects are left to the dedicated security lane (which has already cleared every commit through 07d6b57, including today's nlink identity change).

What's changed since the last code-review pass

• 07d6b57 adds st_nlink to same_stat_identity()'s comparison for kind == "file" entries — this resolves Codex's P2 comment (hard-link-added-after-scan staleness). The check is used at both apply_plan's pre-delete re-verification and preview's drift check, so a link added post-scan now correctly fails identity instead of silently staying "ready."
• 7e5adf9 adds test_link_created_after_scan_breaks_stat_identity, a focused regression test that confirms every other field (size, mtime, dev, ino, mode) is unchanged by os.link() and only the nlink comparison catches the drift. Good, targeted test.
• plugin.json is now 0.14.0, matching the CHANGELOG.md [0.14.0] entry — resolves the earlier version-bump finding.
• The PR title/summary now accurately describes the byte-accounting scope (earlier passes flagged a "hint matching" title that didn't match the diff — resolved).

Still open

🔴 Important — permission-denied subtrees still collapse to logical_size: 0, indistinguishable from empty
hygiene.py#L921-L930 is unchanged since the first review pass on this PR and isn't touched by either of the two newest commits, so it can't carry an inline comment on a diff line — flagging here instead. When os.scandir raises (permission denied, etc.), visit() appends to errors and return 0. The caller already set walked = True before calling visit() (L948), and none of the truncation branches (VCS name, protections, max_depth) fire for this case, so metadata(path, kind, subtotal, walked=True) records logical_size: 0 with no not-walked qualifier — byte-identical to a genuinely empty, fully-inventoried directory. That's the exact symptom this PR eliminates for VCS/protected/max-depth truncation, and target_reclaimable_local_bytes will silently under-report a directory that couldn't actually be inventoried. Still no test covers this path (confirmed via grep — the existing PermissionError/errors tests in test_hygiene.py all exercise handoff_verify's descendant-stat path, not scan_tree's own visit() walk). Worth either giving this the same walked=False/truncated/not-walked treatment, or explicitly adding it to the CHANGELOG's follow-up list next to findings 3/4/6/7 so it isn't silently out of scope.

🟡 Suggestion — docstring invariant not universally true (posted inline at hygiene.py#L662-664): the "logical_size is null, never 0" rule holds for every entry produced through walked=, but not for the target's own record, which keeps a non-null partial sum after truncation and only appends the not-walked qualifier. Not a bug, just worth a callout so the invariant isn't misread from the docstring alone.

Test coverage

The new test_link_created_after_scan_breaks_stat_identity test is well-targeted and correctly isolates the nlink-only signal. No test exists for the permission-denied-subtree gap above — that would be the natural next addition alongside it.

[claude]

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

---

Security Review — PR #1870 (HEAD 7e5adf9)

• Gather context (diff since last reviewed commit 07d6b57)
• Review new commit 7e5adf9 for security issues
• Re-confirm no regressions in previously-cleared code
• Post findings

Findings: none.

Scope reviewed: the only change since the last security pass (07d6b57 → 7e5adf9) is commit 7e5adf9, which touches a single file: test_hygiene.py (+40 lines, no other files changed).

The commit adds test_link_created_after_scan_breaks_stat_identity, a regression test pinning the nlink fix from 07d6b57 (already reviewed and cleared). It:

• Creates a file inside a tempfile.TemporaryDirectory(), hard-links it via os.link(), and asserts same_stat_identity() now returns False post-link where it previously returned True.
• Uses only local, process-owned temp paths — no untrusted input, no subprocess, no path construction from external data.
• Is test-only code; it does not change hygiene.py or any runtime/production code path. hygiene.py is unmodified since the last security review.

This is a pure test addition with no security-relevant surface. Net result across the full PR (056dad9…7e5adf9, files: plugin.json, CHANGELOG.md, SKILL.md, hygiene.py, test_hygiene.py) remains: no findings. The nlink addition to same_stat_identity() (confirmed in the prior pass at 07d6b57) is a strictly stricter, fail-closed identity check with no path into hard_protection/validate_plan/apply_plan's blocker logic, and this commit only adds test coverage proving that check can't silently regress.

No GitHub Actions files touched by this PR.

[claude]

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

---

Security Review — PR #1870 (HEAD cdb47fe)

• Gather context (diff since last-reviewed commit 7e5adf9)
• Review new commits for security issues
• Re-confirm no regressions in previously-cleared code
• Post findings

Findings: none.

Scope reviewed: git diff origin/main...HEAD — the PR's own commits only, isolated from the intervening main merges (b17ea56, and the merge at cdb47fe which pulled in 1218452 etc.). Confirmed this matches the PR's stated file list: plugin.json, CHANGELOG.md, skills/clean/SKILL.md, hygiene.py, test_hygiene.py — 309/-36 across 5 files, consistent with the PR description.

What changed since the last security pass (7e5adf9 → cdb47fe):

• hygiene.py and test_hygiene.py — unchanged. No new runtime or test code to review; the nlink-identity fix (07d6b57) and its regression test (7e5adf9) already cleared in the prior pass remain the full extent of this PR's code diff.
• plugin.json — version bump 0.15.0 → 0.16.0 (renumbered again after a main merge; no behavior change).
• CHANGELOG.md — prose renumbering/reordering of the 0.14.0→0.16.0 entry to stay collision-free against what landed on main in the meantime. Documentation only.
• skills/clean/SKILL.md#L192-L202 — expands the reader guidance on excluding hardlinked/sparse/not-walked entries from reclaimable-byte totals, and steers readers toward the engine's own reclaimable_local_bytes* fields over hand-summing logical_size. Prose only, describes behavior already implemented and reviewed in hygiene.py; doesn't change any code path.

Note: the skills/setup/SKILL.md and python3_alias_probe.py changes visible in a raw two-dot git diff 7e5adf9..cdb47fe are not part of this PR — they arrived via the main merge (unrelated PR) and are outside this PR's actual file list, confirmed via the three-dot origin/main...HEAD diff above.

Net result across the full PR (plugin.json, CHANGELOG.md, skills/clean/SKILL.md, hygiene.py, test_hygiene.py): no findings, consistent with every prior pass on this PR. The st_nlink addition to same_stat_identity() remains a strictly stricter, fail-closed identity check with no path into hard_protection/validate_plan/apply_plan's blocker logic, and nothing added since then touches code.

No GitHub Actions files touched by this PR.

[claude]

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

---

Reviewing PR #1870 (HEAD e34d954)

• Gather context (diff since last reviewed commit cdb47fe → e34d954)
• Review new commit for correctness/design/tests/docs
• Re-check previously open findings against current code
• Post updated feedback

Per REVIEW.md, this lane covers correctness/design/tests/docs; security-relevant aspects are left to the dedicated security lane (already cleared every commit through cdb47fe).

What changed since the last code-review pass

e34d954 is a single-line, docs-only fix: it removes a stray double blank line in CHANGELOG.md (introduced while hand-composing the 0.16.0 entry during the origin/main merge in cdb47fe) that would otherwise trip markdownlint's MD012. Confirmed no other consecutive-blank-line issues remain in the file. hygiene.py and test_hygiene.py are unchanged since the 7e5adf9 pass — no new runtime or test code to review.

Also re-confirmed plugin.json's version (0.16.0) matches the CHANGELOG.md ## [0.16.0] heading, and the changelog's own "never published 0.14.0" note is consistent with the actual heading sequence (0.16.0 → 0.15.0 → 0.13.0, no 0.14.0 entry) — this resolves the earlier version-bump finding for good, including through the renumbering forced by the main merge.

I was unable to execute python3 -m unittest or the repo's own check-changelog-parity.sh in this sandbox (both required approval that wasn't available), so I did not independently re-run the test suite or the parity script this pass — noting per instructions that the PR's own Test plan claims are author-reported, not independently verified by me.

Still open (unchanged from prior passes, not touched by e34d954)

🔴 Important — permission-denied subtrees still collapse to logical_size: 0, indistinguishable from empty
hygiene.py#L921-L930 is unchanged and not part of this PR's diff (no changed/context line to anchor an inline comment to), so flagging here again. When os.scandir raises (permission denied, etc.), visit() appends to errors and return 0. The caller already set walked = True before calling visit() (L948), and none of the truncation branches (VCS name, protections, max_depth) fire for this case, so metadata(path, kind, subtotal, walked=True) records logical_size: 0 with no not-walked qualifier — byte-identical to a genuinely empty, fully-inventoried directory. That's the exact symptom this PR eliminates for VCS/protected/max-depth truncation, and target_reclaimable_local_bytes will silently under-report a directory that couldn't actually be inventoried. Still no test covers this path, and it's still not mentioned in the CHANGELOG's follow-up list. Worth either giving this the same walked=False/truncated/not-walked treatment, or explicitly scoping it out in the CHANGELOG alongside findings 3/4/6/7 before merge.

🟡 Suggestion — docstring invariant not universally true
Already flagged inline on hygiene.py#L662-664 in the prior pass and still unresolved — not re-posting a duplicate inline comment. The "logical_size is null, never 0" rule holds for every entry produced through walked=, but not for the target's own record, which keeps a non-null partial sum after truncation and only appends the not-walked qualifier after the fact.

Test coverage

No change since the last pass — the permission-denied-subtree gap above remains the one untested path in this vertical slice.
· branch fix/disk-hygiene-hint-matching

[claude]

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

---

Security Review — PR #1870 (HEAD e34d954)

• Gather context (diff since last-reviewed commit cdb47fe)
• Review new commit for security issues
• Re-confirm no regressions in previously-cleared code
• Post findings

Findings: none.

Scope reviewed: the only change since the last security pass (cdb47fe → e34d954) is commit e34d954, which touches a single file: CHANGELOG.md (removes one stray blank line left over from the earlier merge-conflict resolution). hygiene.py and test_hygiene.py are byte-for-byte unchanged since the prior security pass at cdb47fe.

Confirmed via gh pr diff 1870 that the PR's full file scope is unchanged from prior reviews: plugin.json, CHANGELOG.md, skills/clean/SKILL.md, hygiene.py, test_hygiene.py. No new code paths, no runtime behavior change — this commit is a pure whitespace/formatting fix in documentation.

Net result across the full PR (plugin.json, CHANGELOG.md, skills/clean/SKILL.md, hygiene.py, test_hygiene.py): no findings, consistent with every prior security pass on this PR (056dad9 → 846a9b7 → 07d6b57 → 7e5adf9 → cdb47fe → e34d954). The st_nlink addition to same_stat_identity() remains a strictly stricter, fail-closed identity check with no path into hard_protection/validate_plan/apply_plan's blocker logic. All new byte-accounting fields (allocated_size, nlink, size_qualifiers additions, reclaimable_local_bytes*) remain purely additive, read-only reporting derived from os.lstat() — they do not influence what gets deleted.

No GitHub Actions files touched by this PR.

[claude]

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

---

Reviewing PR #1870 (HEAD 450cb13)

• Gather context (diff since last reviewed commit e34d954 → 450cb13)
• Review new commit for correctness/design/tests/docs
• Re-check previously open findings against current code
• Post updated feedback

Per REVIEW.md, this lane covers correctness/design/tests/docs; security-relevant aspects are left to the dedicated security lane (already cleared every commit through e34d954; this pass's only change is docs-only, so nothing new for that lane to review).

What changed since the last code-review pass

450cb13 is a docs-only fix that directly resolves the 🟡 "docstring invariant" suggestion raised repeatedly across earlier passes (most recently posted inline on hygiene.py:664 in the prior pass):

• hygiene.py#L666-671 adds a paragraph to metadata()'s docstring explicitly carving out the target's own record as the one exemption to "not-walked implies null logical_size," and explains why (scan_tree deliberately keeps the partial walked sum on the root because it's the number a caller reaches first).
• SKILL.md#L196-198 mirrors the same carve-out for the reporting-agent-facing rule, telling the reader to treat the target's own logical_size as a floor when not-walked is present rather than assuming it's null.

Both additions are accurate against the actual scan_tree behavior (the target record does keep its partial sum and only appends the qualifier) and consistent with each other. This closes out that suggestion; no further action needed on it.

hygiene.py's runtime code and test_hygiene.py are otherwise unchanged since 7e5adf9 — no new logic or tests to review this pass.

Still open (unchanged from every prior pass, not touched by 450cb13)

🔴 Important — permission-denied subtrees still collapse to logical_size: 0, indistinguishable from empty
hygiene.py#L928-937, re-verified against current HEAD: visit()'s os.scandir still raises straight into errors.append(...); return 0 with no truncated/not-walked treatment. The caller at L955 sets walked = True before any of the three truncation branches (VCS name, protections, max_depth) get a chance to flip it back to False, and none of those branches fire for a permission error — so metadata(path, "directory", 0, walked=True) records logical_size: 0 with no not-walked qualifier, byte-identical to a genuinely empty, fully-inventoried directory. That's the exact symptom this PR eliminates for VCS/protected/max-depth truncation, and target_reclaimable_local_bytes will silently under-report a directory that couldn't actually be inventoried.

Checked whether this pass's CHANGELOG.md update scoped it out alongside findings 3/4/6/7 — it does not; the "Follow-ups left open on #1806" section still only lists those four. Still no test exercises scan_tree's own visit() walk hitting a permission error (the existing PermissionError/errors tests all cover handoff_verify's descendant-stat path instead). Worth either giving this the same walked=False/truncated/not-walked treatment, or explicitly adding it to the follow-ups list so it's a documented, deliberate exclusion rather than a silent gap — six review passes in, it's the one correctness item still outstanding on an otherwise well-tested, cleanly-scoped PR.

Test coverage

No change since the last pass — the permission-denied-subtree gap above remains the one untested path in this vertical slice.
· branch fix/disk-hygiene-hint-matching

[claude]

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

---

Security Review — PR #1870 (HEAD 450cb13)

• Gather context (diff since last-reviewed commit e34d954)
• Review new commit for security issues
• Re-confirm no regressions in previously-cleared code
• Post findings

Findings: none.

Scope reviewed: the only change since the last security pass (e34d954 → 450cb13) is commit 450cb13, which touches two files:

• skills/clean/SKILL.md — one clause added to the reader guidance, noting the target record's own logical_size keeps its partial walked sum (read it as a floor) rather than going null like every other not-walked entry.
• skills/clean/scripts/hygiene.py#L662-L671 — a docstring addition to metadata() explaining the same target-record exemption.

Both changes are prose only — pure comment/documentation additions inside a docstring and a Markdown file. No executable code, control flow, byte-accounting logic, or identity-check logic was touched. hygiene.py's actual function bodies and test_hygiene.py are byte-for-byte unchanged since the prior security pass at e34d954.

This commit resolves the previously-open 🟡 suggestion (docstring invariant not universally documented) from the code-review lane — it has no bearing on this security lane's findings.

Net result across the full PR (plugin.json, CHANGELOG.md, skills/clean/SKILL.md, hygiene.py, test_hygiene.py): no findings, consistent with every prior security pass on this PR (056dad9 → 846a9b7 → 07d6b57 → 7e5adf9 → cdb47fe → e34d954 → 450cb13). The st_nlink addition to same_stat_identity() remains a strictly stricter, fail-closed identity check with no path into hard_protection/validate_plan/apply_plan's blocker logic. All byte-accounting fields (allocated_size, nlink, size_qualifiers additions, reclaimable_local_bytes*) remain purely additive, read-only reporting derived from os.lstat() — they do not influence what gets deleted.

No GitHub Actions files touched by this PR.