Disk-path safety hardening (D1–D8): PathGuard, shared sizer, honest accounting, CLI --confirm gate, v2.2.0 - #454
Merged
Merged
Conversation
…Provider (D4) - FileSystemIdentityProvider: realpath(3) root resolution, ancestor-only target resolution (leaf never resolved), lstat (st_dev, st_ino) identity, st_nlink, statfs mount-point detection (unified APFS volume groups share one st_dev, so device-change alone misses the Data firmlink mount) - PathGuard: category-scoped admitDeletionRoot across all three path-bearing discovery kinds (.staticPath/.absolutePath/.probed fallbacks), constrained version-drift sibling rule (never a parent grant; .absolutePath exact-only), deny list regardless of policy (/, volume roots, $HOME in any spelling, protected first-level children), admitContainer split from deletion roots, validateContainedChild/validateRemovableItem containment via pathComponents arrays, cross-device refusal (R15) - Hermetic PathGuardTests: fixture home, symlink/case/NFD identity cases, injected-device cross-device + volume-root cases, registry smoke over all 23 real categories under their own policies, conditional read-only /System/Volumes/Data firmlink integration check Task: fn-1-disk-path-safety-hardening-d1-d8.1
…nResult state model (D2/D3/D7/D8) - New DirectorySizer (single sizing routine, D7): 4-arg enumerator with classified errorHandler (EPERM=TCC/EACCES=permission via the NSUnderlyingErrorKey chain, D6), no .skipsHiddenFiles (D3) or .skipsPackageDescendants (D2), symlinks 0/never descended, directory inode blocks uncounted, dual-signal mount boundaries (device-id change OR statfs isMountPoint — APFS firmlink-safe), within-walk hardlink dedupe into estimatedUpToBytes (D8 mitigation), and an InodeClaim per measured regular-file inode — including st_nlink==1 files and known inodes whose bytes are locally excluded (R8 claim mechanics). - Modes with explicit leaf dispatch: .scanRoot (root fully resolved) and .deletionTarget (lstat leaf first: symlink 0/never walked, regular file own size, directory enumerated, fifo recorded skip). - ScanResult gains the full state model (missing/empty/measured/ partiallyDenied/denied), classified scanError storage, and split exactBytes/estimatedUpToBytes with sizeBytes as compatibility sum; scanner-side derivation (denied vs empty vs partial, R6/R16). - CacheScanner: per-root PathGuard admission against the category's own policy BEFORE any walk — refusal is a .denied-family scan error and the tree is never enumerated (R19); injectable home/provider. - NodeModulesScanner: container admission per search root, lstat symlink rejection for the root, every manual descent AND every node_modules candidate (R19); hidden-dir traversal (R13); classified outcome errors (D6/R14); items carry origin-container provenance (R14). - Both private directorySize implementations deleted (R7 grep gate). - New hermetic suites: DirectorySizerTests (+CacheScannerDerivationTests) and NodeModulesScannerTests — independent lstat fixture math, chmod-000 with restore + euid-0 skip, injected-device/mount-point providers. Task: fn-1-disk-path-safety-hardening-d1-d8.2
…ing (review r1) - FileSystemIdentityProvider gains errno-aware probeKind(of:) (.kind/ .absent/.failed) as the override point; kind(of:) is now final and derives from it. - NodeModulesScanner: candidate probes, descent guards, and the search- root gate switch on probeKind — .absent stays silent (the normal 'no node_modules here' / raced-deletion case), .failed becomes a classified NodeModulesScanIssue (EPERM=TCC, EACCES=permission), never a silent 'not a directory' (D6/R14). - DirectorySizer leaf dispatch likewise records a classified denial for an unprobeable leaf instead of returning a silent zero. - New hermetic tests via a probe-failing provider subclass (canonical /private path spellings — Foundation lists children canonically); unreadable-subtree test updated for the additional visible capture. Task: fn-1-disk-path-safety-hardening-d1-d8.2
…ew r2) Associated-value-free enums conform automatically (the build and full test suite were already green), but the explicit declaration documents that KindProbe's synthesized Equatable and callers' == checks depend on it. Task: fn-1-disk-path-safety-hardening-d1-d8.2
…lures (review r3) The per-item walk loop now switches on probeKind(of:) directly: .failed routes through denial(forFailedProbe:errno:) so EPERM stays TCC and EACCES stays permission at the ScanError layer; .absent is a benign mid-walk deletion race and skips silently. New sizer test asserts both classifications for enumerated entries via an injected probe-failing provider. Task: fn-1-disk-path-safety-hardening-d1-d8.2
…bytes accounting (D1/D4) - Scan-state refusal (R18): .denied refused even force-selected, refusal surfaces as a per-category error and a classified cleanup.log line; .partiallyDenied proceeds explicit-only with measured deletions only - Mode-aware guard at all three primitives: category roots re-admitted via admitDeletionRoot + per-child validateContainedChild (contents mode); node_modules items via admitContainer + validateRemovableItem against origin-container provenance (item mode, cross-device refusal R15) - cleanCommands roots admitted BEFORE any argv executes (R17); refusal blocks the whole command set, reported + logged - Claim-based two-phase accounting (R8): new InodeAccountingRegistry actor (register claims pre-delete, transfer canonical bytes once post-success, sticky hardlink classification); freed bytes are per-child .deletionTarget measurements, never pre-scan totals (D1) - CleanupReport reworked: split-component entries (exactBytes / estimatedUpToBytes), pure-sum aggregates, disposal mode, honest mode-driven headline (trash never claims Freed, R11/R16); compatibility surface (cleaned/totalFreed/formattedTotal) retained for fn-1.4/fn-1.5 - Deletion uses the unresolved requestedURL, containment the resolvedURL; guard chain re-validated immediately pre-delete; ENOENT category child = skip (decided by provider probe, never inferred from an empty report); node_modules ghost semantics preserved; per-child error isolation (R10) - Injectable home (deny list + cleanup.log), container roots (shared with NodeModulesScanner.defaultSearchRoots), provider, and @mainactor trash seam so tests never touch the real Trash or real $HOME - Fix stale /bin/bash -c doc comment (commands run via /usr/bin/env argv) - Tests: 6 pre-existing cases preserved (node_modules fixtures gain provenance + container roots); 25 new cases cover R1/R4/R8 full ordering matrix (a-g)/R10/R11/R15/R16/R17/R18 Task: fn-1-disk-path-safety-hardening-d1-d8.3
…aveat + TCC gating (D6/D8) - ScanResult: statusLabel presentation (distinct denied/partiallyDenied/empty/missing), selection defaults reworked in the state-aware init (.denied unselectable, .partiallyDenied never auto-selected), ScanError.Kind wire mapping + Full Disk Access settings deep link, DiskSpaceCaveat D8 disclosure constant - CleanupReport: component-derived headline (Freed X / + up to Y more / up to Z) and Entry.componentSummary; report sheet migrated off cleaned/formattedTotal onto entries/headline/disposal — success claimed only when something succeeded - CacheoutViewModel: injectable scanners/categories, ScanTrigger plumbed to NodeModulesScanner (automatic scans skip TCC-protected roots), nodeModulesScanIssues published (denied search root visible, GUI-only until fn-2), toggleSelection refuses .denied, selectAllSafe auto-selects only .measured, overcountCaveat + hasPartiallyDeniedSelection for the sheets - Views: CategoryRow diverges denied from empty (lock, red status, Grant access link for TCC); MenuBarView appends denied rows visibly + caveat tooltip; confirmation sheet carries caveat + partiallyDenied warning; clean buttons disabled while scanning; node_modules section renders classified issues, never a false 'found none' - CLIHandler: scanItemJSON additive fields (state, exact_bytes, estimated_up_to_bytes, scan_error, grant_hint for TCC) Task: fn-1-disk-path-safety-hardening-d1-d8.4
…(review r1) - isAnyScanInProgress covers both phases: the cache phase clears isScanning while node_modules keeps running 10-30s longer - Scan/Clean/Quick Clean buttons, auto-rescan gate, and tab auto-scan use it - Model-level re-entrancy guards in scan() and clean() so correctness does not depend on button state - shouldAutoRescan returns false while any phase is in flight Task: fn-1-disk-path-safety-hardening-d1-d8.4
…p (review r2) - smartClean() clears ALL manual selections (categories + node_modules) before selectAllSafe, so a manually selected .partiallyDenied category never rides into Quick Clean (R18); filesystem-asserted test - scan() also refuses while isCleaning; Scan buttons disabled and auto-rescan held off during cleanup (closes the scan/clean race the reviewer flagged as pre-existing) Task: fn-1-disk-path-safety-hardening-d1-d8.4
… spotlight guard, schema v3 (D5) - Hoist --confirm parsing to run() (shared by clean/smart-clean/intervene); pure CleanGateDecision function covers the confirmed/dry-run/euid matrix - Unconfirmed clean/smart-clean: stdout empty, CONFIRMATION_REQUIRED on stderr with the per-category plan in details, exit 1; euid-0 refused outright with ROOT_REFUSED - Exit-code contract: total failure exits 1 CLEAN_FAILED (derived from the requested slug set vs report errors, never entries.isEmpty); partial failure exits 0 with per-item success flags, including vacuous-success rows for slugs with neither entry nor error - Clean/smart-clean JSON exposes exact_bytes/estimated_up_to_bytes; total_freed_bytes and target_met are exact-only (R16); dry-run and the refusal plan consume scan-time components, no re-walk - Named .denied slug surfaces the cleaner's refusal as a per-item error; .partiallyDenied proceeds with a warning field; smart-clean candidates skip both plus caution risk (R18) - handleSpotlight admits every root via PathGuard under its own category policy before any xattr/marker write and refuses .denied scan states; refusals reported in the additive refused array - schema_version 2 -> 3; retire the CleanupReport compatibility surface (cleaned/totalFreed/formattedTotal) now that the CLI derives from split entries - PROTOCOL.md: schema 3 contract (confirm gate, plan shape, exit codes, scan/clean split components, spotlight refusals, ROOT_REFUSED); update CLI-REFERENCE.md and CLIHandler/main.swift doc comments - Tests: CLIGateTests (gate matrix, plan/dry-run builders, smart-clean exact-only loop decision, spotlight admission) + two read-only subprocess framing tests; migrate CacheCleanerTests off the compat surface Task: fn-1-disk-path-safety-hardening-d1-d8.5
…lug guard (review r1) - smartCleanPlan entries carry state/action (derived via cleanPlanAction, never hardcoded) so the smart-clean CONFIRMATION_REQUIRED plan matches the documented shared details.plan shape - Validate smart-clean target before the gate: non-finite, negative, or >1e9 GB targets exit INVALID_ARGUMENTS instead of trapping in the Int64 conversion (nan/inf/overflow) - clean with an empty slug list exits MISSING_ARGUMENT instead of masquerading as a successful no-op - PROTOCOL.md + CLI-REFERENCE.md rows for all three; framing tests assert plan-entry shape, nan refusal (no --confirm — stays read-only by contract), and the empty-slug usage error Task: fn-1-disk-path-safety-hardening-d1-d8.5
…n, strict target parse, honest spotlight writes (review r2) - smartCleanPlan lists EVERY eligible candidate: entries past the projected target-met point carry action clean_if_needed (the real loop advances on delete-time bytes and may reach them), projected bytes_freed 0, would-free components intact; projected totals/target_met count unconditional entries only — the confirmation plan now describes everything the confirmed run may touch - Extract cleanRunIsTotalFailure (pure) and use it in both handlers; unit test the full matrix: no-op, all-success, vacuous success, partial (mixed flags / bytes-freed-despite-errors), total failure - smart-clean target: absent defaults to 5.0, but a present non-numeric value exits INVALID_ARGUMENTS instead of silently defaulting to a 5 GB deletion; extractFloat removed (dead) - spotlight captures both write outcomes: a root where neither the xattr nor the marker landed is reported in refused (metadata writes failed), never claimed as tagged; tagged entries carry xattr_written/marker_written - PROTOCOL.md + CLI-REFERENCE.md updated for all four; new tests: fallback disclosure, exit-decision matrix, malformed-target framing, read-only-root spotlight refusal (euid-0 skip) Task: fn-1-disk-path-safety-hardening-d1-d8.5
- A zero target was accepted but self-contradictory: the plan marked every candidate clean_if_needed with target_met true while the confirmed run touched nothing, and an empty candidate list reported target_met true against the documented semantics. Zero now exits INVALID_ARGUMENTS with the other malformed targets, before any scan or gate - PROTOCOL.md + CLI-REFERENCE.md updated; framing test for the zero target Task: fn-1-disk-path-safety-hardening-d1-d8.5
…ytes (review r4) - Validate the CONVERTED target: a positive sub-byte value (1e-20 GB) truncated to targetBytes == 0, recreating the zero-target contradiction (target_met true, nothing cleaned). Now INVALID_ARGUMENTS before any scan or gate; framing test + docs updated Task: fn-1-disk-path-safety-hardening-d1-d8.5
…gelog + docs, release metadata - NSDocuments/NSDesktop/NSDownloadsFolderUsageDescription in bundle.sh heredoc, Sources/Cacheout/Info.plist, and project.yml INFOPLIST_KEY entries (identical key sets; Downloads kept deliberately as a defensive string mirroring tccProtectedRootNames — no scan walks ~/Downloads today) - Release checklist in BUILD-AND-DISTRIBUTION.md with the manual TCC prompt verification procedure marked PENDING (human-owned release gate, R9) - CHANGELOG 2.2.0: breaking --confirm gate + schema 3, split-component totals, exit-code policy, truthful sizing, D1/D6 fixes, hidden-dir discovery, hardlink scope, overcount caveat - VERSION -> 2.2.0 and CLIHandler.fallbackVersion matched (CLI-REFERENCE already documents 2.2.0) - ARCHITECTURE: PathGuard/provider/sizer in file map, admission + dual canonicalization + two-signal mount detection + claim-based accounting design decisions, security-model bullets - API-REFERENCE: ScanState/ScanError/DiskSpaceCaveat/ScanResult as built, CleanupReport split entries (pre-split surface documented as retired) - CATEGORIES: size-reporting notes + 'up to <pre-scan size>' Simulator wording; README sparse-size bullet broadened beyond Docker - Cask caveats show --confirm/--dry-run; CONTRIBUTING CLI test guidance - Stale /bin/bash -c doc comment fixed in CacheCategory.swift Task: fn-1-disk-path-safety-hardening-d1-d8.6
…eanCommands naming in API reference (review r1) - CHANGELOG: absent smart-clean target still defaults to 5 GB; only a PRESENT malformed/non-positive/sub-byte/overflow target is INVALID_ARGUMENTS (matches CLIHandler.swift:132-144 and CLI-REFERENCE) - API-REFERENCE: CacheCategory documents cleanCommands: [[String]]? (argv via /usr/bin/env) instead of the retired cleanCommand: String?; init signature updated; ARCHITECTURE prose aligned Task: fn-1-disk-path-safety-hardening-d1-d8.6
…letion review r1) A .deletionTarget directory that is ITSELF a mount point (or on a foreign device vs its parent) was enumerated and deleted through — the boundary check only saw entries yielded inside the walk, and validateContainedChild is descendant-only by design. DirectorySizer now applies both boundary signals to the root before enumerating and flags rootMountBoundary; CacheCleaner refuses such children outright (logged, per-child error). Item mode was already covered by validateRemovableItem's deny-list re-check.
…(completion review r2) The r1 fix only refused targets that were THEMSELVES mount boundaries; a mount nested deeper inside a child was recorded-and-skipped for sizing but removeItem recursed straight through it. Both cleaner paths (category children and node_modules items) now refuse when the measured tree contains ANY mount boundary. Regression tests: nested foreign-device + same-device mount-point payloads survive while the sibling is still cleaned; item-mode nested mount refused.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76d9f10edf
ℹ️ 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".
- P1 pnpm store admission: probed path is a version child of the declared fallback; extended PathGuard drift rule with pure-version-child shape (+test, docs) - P2 scan trigger: timer-driven scan inherited .userInitiated via default; removed the default and classified all five call sites explicitly - P2 disposal honesty: command-erased bytes were reported as trashed; per-entry disposal tracking with honest headline/row/confirmation rendering (+3 tests) - P2 hermetic seam: resolvedPaths anchored to real home while admission used injected home; threaded home through discovery, probes, and all call sites (+3 seam tests) Validation: swift build + swift test — 305 tests, 0 failures. Claude-Session: https://claude.ai/code/session_01P6LVX7RkPM8NW7dHx16ewU
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ad9a9d5ec
ℹ️ 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".
- P1 vacuous admission: empty delete-time resolution let destructive argv run with no admitted root; added explicit refusal before command execution (+non-execution test) - P1 watchdog: stale pre-schema-3 syntax (--target ignored, no --confirm) made every emergency clean a swallowed refusal; updated invocation + exit-status-gated fallback - P2 hermetic seam (cluster): clean-command env leaked the real HOME; pinned to injected home, audited all Process() sites, annotated intentional real-home uses (+env test) Validation: swift build + swift test — 307 tests, 0 failures. Claude-Session: https://claude.ai/code/session_01P6LVX7RkPM8NW7dHx16ewU
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements flow-next epic
fn-1-disk-path-safety-hardening-d1-d8: closes defects D1–D7 and mitigates D8 (seeSCANNERS-ROADMAP.md) in the disk-cleaning path, before the scanner-expansion epics (fn-2…fn-6) build on it.InodeAccountingRegistry: register → delete → accept, transfer-once, sticky hardlink classification)DirectorySizer(no.skipsPackageDescendants/.skipsHiddenFiles), allocated-size totalsPathGuard+FileSystemIdentityProvider: category-scoped admission (sibling drift rule, no parent grants), deny list, inode identity, dual canonicalization (roots fully resolved; deletion leaves never), enforced at every deletion primitive and at scan time--confirmgate (CONFIRMATION_REQUIRED, plan in stderrdetails), exit-code policy (CLEAN_FAILED/ROOT_REFUSED), strict smart-clean target validation, spotlight admission guard,schema_version3ScanStatemodel (missing/empty/measured/partiallyDenied/denied) surfaced in GUI, menubar, and scan JSONDirectorySizer; both legacydirectorySizeimplementations deleted (grep-gated)exact/estimated_up_tobyte components end-to-end, user-facing caveat; cross-walk accounting deferred to fn-2Also: TCC usage strings in all three build paths (bundle.sh heredoc, Info.plist, project.yml), Full Disk Access UX with settings link,
VERSION/fallbackVersion→ 2.2.0, CHANGELOG, PROTOCOL.md + full docs pass.Notable findings baked in
st_devacross the Data firmlink, so mount detection uses two signals (device-id change +statfsf_mntonname).entries.isEmpty≠ total failure (zero-byte successes produce no entry); the exit-code contract keys off the requested-slug set.Testing
swift test: 298 tests, 0 failures (baseline was 168) — hermetic fixtures, injectable providers, full hardlink ordering matrix (a)–(g), CLI gate matrix, mount-boundary regressions.plutilverified TCC keys + 2.2.0).Breaking changes
clean/smart-cleannow require--confirm(schema_version2→3). Sequencing: cacheout-mcp must add--confirmbefore this reaches a release; cask caveats updated in the same release.Remaining human-owned gate
docs/v1/BUILD-AND-DISTRIBUTION.md(requires a real bundled-app run:tccutil reset, prompt-with-explanation, deny → "Access denied — not scanned" + FDA link, background rescan must not prompt).