Drop un-cleanable scan items before they reach the UI (1.5.1)#24
Merged
Conversation
Pure access(W_OK)-based probe in MacCleanKit. An item is cleanable iff:
1. Its parent directory is writable by us (unlink/trash is a
directory-modification syscall; permission lives on the parent).
2. If the item is a directory, the directory itself is also
writable (recursive descent needs to modify contents).
3. The path still exists.
This single probe catches two distinct macOS realities that both
surface as 'you don't have permission' errors today:
- Root-owned children of writable system dirs (/Library/Caches/*,
/Library/Logs/*, /private/var/log/*) — POSIX denies via EACCES.
- Data-vaulted Apple caches in the user's own home
(~/Library/Caches/com.apple.containermanagerd/, …ap.adprivacyd/,
…Safari.SafeBrowsing/) — kernel denies via EPERM (UF_DATAVAULT)
regardless of FDA, ownership, or root.
6 spec tests cover the contract using POSIX permission stripping
to simulate the data-vault case (the actual UF_DATAVAULT flag can't
be set from userland).
Single insertion point in scanModules(): every module's results flow through here, so every result list gets filtered exactly once. Items the current process couldn't trash never reach the ViewModel, never render in the file list, never count toward selectable bytes, never get queued for clean. Users no longer see 'X errors' on the completion screen for items nothing on this machine could clean. This intentionally affects every module — including Optimization / Maintenance / Updater whose items aren't trashed via FileManager. Items in those modules don't typically live under root-owned dirs in practice, but if they do, surfacing them in the UI without an actionable path was already misleading. Two ScanCoordinator/SmartScan tests previously used non-existent fake URLs (/tmp/f0, /tmp/a.cache); the filter correctly dropped them, breaking aggregation assertions. Updated to materialize real files in per-test temp dirs so the aggregation contract is tested against actual cleanable inputs.
Patch bump. UX fix: the completion screen no longer lists 'X errors' for items the current process was never going to be able to trash in the first place — root-owned children of /Library/Caches/, /Library/Logs/, /private/var/log/, and macOS data-vaulted Apple caches under ~/Library/Caches/com.apple.*. Items disappear from the scan results upfront via a single access(W_OK) probe in ScanCoordinator.
…tion CI macos-15 runner hit: error: main actor-isolated property 'tmpRoot' can not be mutated from a nonisolated context XCTestCase declares setUp(WithError) as nonisolated; an @mainactor subclass can't mutate its own @mainactor stored property from those inherited entry points without an explicit hop. Sidestep by creating the per-test sandbox inside each test (which IS @MainActor-isolated in this class) via a static factory + defer for cleanup. No class- level mutable state means no actor mismatch to resolve. Local Swift 6.3.2 happened to let this through; the CI toolchain catches it. Now both pass.
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
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
Stops showing items the current process can't trash. After 1.5.0 the completion screen still listed 10 stubborn errors on a real-world clean — all of them paths nothing on this machine could ever clean:
/Library/Caches/com.apple.InferenceProviderService/,/Library/Logs/PaloAltoNetworks/GlobalProtect/*.log,/private/var/log/com.apple.xpc.launchd/launchd.log. Parent dirs are0755 root:wheel, sounlinkfrom inside returnsEACCES.~/Library/Caches/com.apple.containermanagerd/,…ap.adprivacyd/,…Safari.SafeBrowsing/. macOS marks theseUF_DATAVAULT; the kernel rejects writes regardless of FDA, ownership, or root.EPERMat the syscall.Both categories surface as
access(W_OK)denials at the syscall layer, so a single probe inMacCleanKit/CleanFiltercovers both.ScanCoordinator.scanModulesapplies it once to every module's results — the only choke point that touches all 13 modules.What changed
Sources/MacCleanKit/CleanFilter.swift(new, 35 LOC) —isCleanableByCurrentProcess(URL) -> Bool. Checks parent dir writability; if the item is a directory, also checks self writability.Sources/MacClean/Core/Scanner/ScanCoordinator.swift— single insertion point, filters every module's results before they becomeModuleScanResult.Tests/MacCleanKitTests/CleanFilterTests.swift(new) — 6 spec tests: cleanable file in writable dir, cleanable dir, non-existent path, unwritable parent, unwritable self, files-inside-unwritable-parent.Tests/MacCleanTests/ScanCoordinatorTests.swift+SmartScanE2ETests.swift— two pre-existing tests used non-existent fake URLs (/tmp/f0,/tmp/a.cache); the filter correctly dropped them, breaking aggregation assertions. Updated to materialize real files in per-test temp dirs.VERSION+MCConstants.appVersion→ 1.5.1.Why a blanket filter (not module-scoped)
Optimization / Maintenance / Updater items aren't trashed via
FileManager, so in theory the filter could over-reach. In practice their items don't live under root-owned dirs (launch agents the user can disable live in~/Library/LaunchAgents/, login items in~/Library/Application Support/). And if some did, surfacing them in the UI without an actionable cleanup path was already misleading UX. Re-evaluate if we ever build the XPC helper for elevated cleanup.Real-system probe (pre-implementation)
Verified
access(W_OK)semantics on the actual user-reported error paths before writing code:Both errno=13 (EACCES, POSIX) and errno=1 (EPERM, data-vault) surface as
access != 0. One probe, both cases.Test plan
swift test— 387 tests, 4 skipped (existing XPC-skipped), 0 failuresswift build— cleanbash scripts/check-version-sync.sh— VERSION ↔ MCConstants synced at 1.5.1