Fix false-success, a crash, and a dead storage watch found in review

[heznpc]

Adversarial review of this session's work (#60–#73) using six parallel reviewers with different scopes. Every finding below was reproduced by hand before being fixed — none is taken on the reviewers' word.

1. login_items.sh reported removal SUCCESS when the verification itself failed

login_item_exists returned the same exit status for "this is not a login item" and "the System Events query failed", so a failed read was silently read as absence. Two consumers turned that into a success report:

• pre-delete check fails → already_gone, exit 0
• post-delete recheck fails → ok, exit 0 (the delete's own exit code is discarded)

Reproduced end to end with a stateful osascript stand-in: preview issues a token normally, System Events then fails, execute reports status ok / exit 0 — while the login item is still installed. LoginItemService maps both to success and the app announces removal. A security tool affirmatively telling the owner a persistence mechanism is gone while it is still there is the worst version of this bug class.

login_item_exists is now tri-state (present / confirmed absent / could not determine). Only a successful read may assert absence; an unreadable list is blocked, and a failed post-delete recheck is failed, never ok.

2. SpaceGoalView hard-crashed the 목표 tab

Slider(value:in: 1...max(achievableGB, 1), step: 1) collapses to 1...1 whenever the cleanable total is above zero but at or below 1GB — a single small npm cache is enough. SwiftUI divides the range by step and calls fatalError:

SwiftUI/Slider.swift:635: Fatal error: max stride must be positive

Reproduced as a genuine crash (signal 5) in a hosted view, not inferred from reading. The eligibleCandidates.isEmpty guard doesn't cover it, because candidates do exist. The bound is now provably wider than its lower bound, and below 1GB the slider is replaced by the achievable total (a whole-GB goal picker is meaningless there anyway). Five rendering regression tests cover 0 / sub-1GB / exactly-1GB / multi-GB / no-candidates; they crash against the old view.

3. The storage watch could never be turned on — two independent causes

Both predate this review range (introduced with the watch-notification work), but neither was diagnosable without the other:

1. PCH_STORAGE_WATCH_APP_BUNDLE was passed by setStorageWatchEnabled but never added to allowedEnvironmentOverrides. sanitizedEnvironment throws on any non-allowlisted key, so the spawn was refused before schedule.sh ever ran.
2. With that fixed, schedule.sh runs and installs a real plist — but StorageWatchService.runtimeState's expectedArguments still omitted the PCH_STORAGE_WATCH_APP_BUNDLE= entry that schedule.sh writes unconditionally. The exact-array comparison therefore always failed, every fresh install was judged .stale, and the UI reported failure and showed the watch as off while launchd had actually loaded the job.

The Swift unit test could not catch (2) because its fixture built the plist from the same wrong list the implementation used — the test encoded the bug.

Verified on a live machine: the toggle now turns on, and the app's reported state matches launchctl for the first time. (The LaunchAgent installed during verification was removed afterward.) A new cross-language contract test compares schedule.sh's and Swift's argument lists so they cannot drift apart again — it fails against the pre-fix Swift.

4. network_watch.sh reported a false all-clear on an unread closing sample

lsof exits non-zero with no output both on failure and on zero matches, and || true erases the difference — so a closing sample that was never read emitted newEstablished 0 / newListen 0, indistinguishable from a genuinely quiet window. The closing LISTEN set is the usable liveness probe (every real Mac has listening sockets); an empty one now reports an error instead of a count. Existing tests were passing unrealistically empty listen samples and are corrected to a real baseline.

Test plan

• pytest tests/ — 366 passed
• swift test — 162 passed
• release_smoke.py --check-only — ok
• Every new test proven against the pre-fix code (4 pytest failures, the SwiftUI crash, and the drift guard all confirmed first)
• Live verification in the real signed build for the storage-watch fix

Not included

Lower-severity findings from the same review (pinned Homebrew packages dropped by the brew outdated regex; comma-in-name login items; non-\x20 lsof escapes; goal-selection tie-break determinism and float-epsilon; missing isBusy guards) are real but none produces a crash or a false success. They're deliberately left for a follow-up rather than mixed into this one.

🤖 Generated with Claude Code