From 048547d72f32f366967a00e6ad5330816b81e0f7 Mon Sep 17 00:00:00 2001 From: Heznpc Date: Thu, 13 Aug 2026 20:46:43 +0900 Subject: [PATCH] Fix false-success, a crash, and a dead storage watch found in review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of #60-#73 (six parallel reviewers, findings then reproduced by hand before fixing). Four defects that made the app either crash or state something untrue: login_items.sh reported removal SUCCESS when the check itself failed. login_item_exists returned the same status for "not a login item" and "the System Events query failed", so a failed read at execute time emitted already_gone/ok with exit 0. Reproduced end to end: preview issues a token normally, System Events then fails, execute reports "ok" -- while the login item is still installed. The app maps that to success and announces removal, so a security tool affirmatively told the owner a persistence mechanism was gone while it was still there. Now tri-state: only a successful read may assert absence; an unreadable list is blocked, and a failed post-delete recheck is failed, never ok. SpaceGoalView hard-crashed the 목표 tab. Slider(in: 1...max(x, 1)) collapses to 1...1 whenever the cleanable total is above zero but at or below 1GB -- one small npm cache -- and SwiftUI fatals with "max stride must be positive". Reproduced as a real crash in a hosted view, not inferred. The range is now provably wider than its lower bound, and below 1GB the slider is replaced by the achievable total. The storage watch could never be turned on, from two independent causes, both predating this range but only diagnosable together: PCH_STORAGE_WATCH_APP_BUNDLE was never in allowedEnvironmentOverrides, so sanitizedEnvironment threw and the spawn was refused; and once that was fixed, StorageWatchService's expectedArguments still omitted the entry schedule.sh actually writes, so the installed plist failed the exact-array comparison and every install was judged stale. The Swift unit test could not catch that because its fixture was built from the same wrong list the implementation used. Verified on a live machine: the toggle now turns on and the app's reported state matches launchd for the first time. A cross-language guard now compares schedule.sh's and Swift's argument lists so they cannot drift apart again. network_watch.sh reported a false all-clear when the closing lsof sample failed: `|| true` cannot distinguish failure from zero matches, so an unread sample looked like a quiet window. The closing LISTEN set is the liveness probe (every real Mac has listening sockets); an empty one now reports an error instead of "0 new connections". Every new test was proven against the pre-fix code first. --- .../Modore/Services/LocalProcessRunner.swift | 1 + .../Modore/Services/StorageWatchService.swift | 12 ++- .../Sources/Modore/Views/SpaceGoalView.swift | 28 +++++- .../ModoreTests/CleanupSafetyTests.swift | 38 +++++--- .../ModoreTests/SpaceGoalViewRangeTests.swift | 71 +++++++++++++++ scripts/login_items.sh | 44 ++++++++-- scripts/network_watch.sh | 18 ++++ tests/test_macos_login_items.py | 86 +++++++++++++++++++ tests/test_macos_network_watch.py | 39 +++++++-- tests/test_service_contracts.py | 45 ++++++++++ 10 files changed, 357 insertions(+), 25 deletions(-) create mode 100644 macos/Modore/Tests/ModoreTests/SpaceGoalViewRangeTests.swift diff --git a/macos/Modore/Sources/Modore/Services/LocalProcessRunner.swift b/macos/Modore/Sources/Modore/Services/LocalProcessRunner.swift index c928572..23250ab 100644 --- a/macos/Modore/Sources/Modore/Services/LocalProcessRunner.swift +++ b/macos/Modore/Sources/Modore/Services/LocalProcessRunner.swift @@ -51,6 +51,7 @@ enum LocalProcessRunner { "PCH_PINNED_WHITELIST", "PCH_STORAGE_DU_TIMEOUT", "PCH_STORAGE_TOTAL_DU_BUDGET", + "PCH_STORAGE_WATCH_APP_BUNDLE", "PCH_STORAGE_WATCH_SCRIPT", "PCH_STORAGE_WATCH_SHA256", "VT_API_KEY", diff --git a/macos/Modore/Sources/Modore/Services/StorageWatchService.swift b/macos/Modore/Sources/Modore/Services/StorageWatchService.swift index 1620357..be046b0 100644 --- a/macos/Modore/Sources/Modore/Services/StorageWatchService.swift +++ b/macos/Modore/Sources/Modore/Services/StorageWatchService.swift @@ -171,11 +171,20 @@ enum StorageWatchService { return values } + /// `expectedAppBundlePath` must match what `schedule.sh` actually writes + /// into ProgramArguments. That script has emitted a + /// `PCH_STORAGE_WATCH_APP_BUNDLE=` entry unconditionally since the watch + /// notification moved under the app's own identity, but this expectation + /// was never updated to match, so the exact-array comparison below could + /// never succeed: every freshly installed plist was judged `.stale`, the + /// toggle reported failure, and the UI showed the watch as off while + /// launchd had in fact loaded the job. static func runtimeState( protocolValues: [String: String], expectedWatcherURL: URL, expectedWatcherSHA256: String? = nil, - expectedHomeURL: URL = FileManager.default.homeDirectoryForCurrentUser + expectedHomeURL: URL = FileManager.default.homeDirectoryForCurrentUser, + expectedAppBundlePath: String = Bundle.main.bundleURL.path ) -> StorageWatchRuntimeState { guard let plistPath = protocolValues["plist"], plistPath.hasPrefix("/") else { return .stale @@ -201,6 +210,7 @@ enum StorageWatchService { "PATH=\(LocalProcessRunner.safeSystemPath)", "LANG=en_US.UTF-8", "LC_ALL=en_US.UTF-8", + "PCH_STORAGE_WATCH_APP_BUNDLE=\(expectedAppBundlePath)", "/bin/bash", "-p", "-c", diff --git a/macos/Modore/Sources/Modore/Views/SpaceGoalView.swift b/macos/Modore/Sources/Modore/Views/SpaceGoalView.swift index 4d2094d..df7162d 100644 --- a/macos/Modore/Sources/Modore/Views/SpaceGoalView.swift +++ b/macos/Modore/Sources/Modore/Views/SpaceGoalView.swift @@ -42,6 +42,19 @@ struct SpaceGoalWorkspaceList: View { private var achievableGB: Double { Self.achievableGB(storage) } + /// Upper bound for the goal slider. SwiftUI's Slider divides the range by + /// `step` and fatals with "max stride must be positive" on a zero-width + /// range, so `1...max(achievableGB, 1)` hard-crashed the whole page + /// whenever the cleanable total was greater than zero but at or below + /// 1GB (one small npm cache is enough). Rounding up and flooring at 2 + /// keeps the range provably wider than its lower bound. + private var goalUpperBoundGB: Double { max(achievableGB.rounded(.up), 2) } + + /// A whole-GB goal picker is meaningless below 1GB, and that is exactly + /// the range where a degenerate slider used to crash -- show the real + /// achievable total instead of a control the user cannot move. + private var supportsGoalSlider: Bool { achievableGB >= 1 } + private var selection: [StorageItem] { SpaceGoalSelection.select(from: storage.cleanupCandidates, targetGB: targetGB) } @@ -92,9 +105,13 @@ struct SpaceGoalWorkspaceList: View { @ViewBuilder private var goalPicker: some View { VStack(alignment: .leading, spacing: 8) { - Slider(value: $targetGB, in: 1...max(achievableGB, 1), step: 1) + if supportsGoalSlider { + Slider(value: $targetGB, in: 1...goalUpperBoundGB, step: 1) + } HStack { - Text("목표: \(String(format: "%.0f", targetGB))GB") + Text(supportsGoalSlider + ? "목표: \(String(format: "%.0f", targetGB))GB" + : "정리 가능한 용량이 1GB 미만이라 목표를 나눌 수 없습니다.") .font(.callout.weight(.medium)) Spacer() Text("정리 가능 총합 \(String(format: "%.1f", achievableGB))GB") @@ -103,6 +120,13 @@ struct SpaceGoalWorkspaceList: View { } } .padding(.vertical, 4) + // A rescan can shrink what is cleanable while this tab stays on + // screen; @State survives that, so an old goal could sit outside the + // new range (slider pinned at its end, header quoting a goal the + // track cannot reach). + .onChange(of: goalUpperBoundGB) { newUpperBound in + targetGB = min(max(targetGB, 1), newUpperBound) + } } private static func achievableGB(_ storage: StorageSnapshot) -> Double { diff --git a/macos/Modore/Tests/ModoreTests/CleanupSafetyTests.swift b/macos/Modore/Tests/ModoreTests/CleanupSafetyTests.swift index eaacaeb..20ef24b 100644 --- a/macos/Modore/Tests/ModoreTests/CleanupSafetyTests.swift +++ b/macos/Modore/Tests/ModoreTests/CleanupSafetyTests.swift @@ -252,6 +252,7 @@ final class CleanupSafetyTests: XCTestCase { withIntermediateDirectories: true ) try "#!/bin/bash\nexit 0\n".write(to: expectedWatcher, atomically: true, encoding: .utf8) + let appBundlePath = "/Applications/Modore.app" func writePlist(watcher: URL, extraEnvironment: Bool = false) throws { let watcherData = (try? Data(contentsOf: watcher)) ?? Data(watcher.path.utf8) @@ -265,6 +266,9 @@ final class CleanupSafetyTests: XCTestCase { "PATH=\(LocalProcessRunner.safeSystemPath)", "LANG=en_US.UTF-8", "LC_ALL=en_US.UTF-8", + // schedule.sh writes this entry unconditionally; a fixture + // without it is not a plist this product can actually produce. + "PCH_STORAGE_WATCH_APP_BUNDLE=\(appBundlePath)", "/bin/bash", "-p", "-c", @@ -299,7 +303,8 @@ final class CleanupSafetyTests: XCTestCase { XCTAssertEqual(StorageWatchService.runtimeState( protocolValues: protocolValues, expectedWatcherURL: expectedWatcher, - expectedHomeURL: root + expectedHomeURL: root, + expectedAppBundlePath: appBundlePath ), .stale) // Installing replaces the stale definition with the current signed @@ -308,7 +313,8 @@ final class CleanupSafetyTests: XCTestCase { XCTAssertEqual(StorageWatchService.runtimeState( protocolValues: protocolValues, expectedWatcherURL: expectedWatcher, - expectedHomeURL: root + expectedHomeURL: root, + expectedAppBundlePath: appBundlePath ), .current) try "#!/bin/bash\nexit 99\n".write( @@ -319,7 +325,8 @@ final class CleanupSafetyTests: XCTestCase { XCTAssertEqual(StorageWatchService.runtimeState( protocolValues: protocolValues, expectedWatcherURL: expectedWatcher, - expectedHomeURL: root + expectedHomeURL: root, + expectedAppBundlePath: appBundlePath ), .stale) try "#!/bin/bash\nexit 0\n".write( to: expectedWatcher, @@ -333,7 +340,8 @@ final class CleanupSafetyTests: XCTestCase { XCTAssertEqual(StorageWatchService.runtimeState( protocolValues: mismatchedLoadedValues, expectedWatcherURL: expectedWatcher, - expectedHomeURL: root + expectedHomeURL: root, + expectedAppBundlePath: appBundlePath ), .stale) try FileManager.default.setAttributes( @@ -343,7 +351,8 @@ final class CleanupSafetyTests: XCTestCase { XCTAssertEqual(StorageWatchService.runtimeState( protocolValues: protocolValues, expectedWatcherURL: expectedWatcher, - expectedHomeURL: root + expectedHomeURL: root, + expectedAppBundlePath: appBundlePath ), .stale) try FileManager.default.setAttributes( [.posixPermissions: 0o600], @@ -354,7 +363,8 @@ final class CleanupSafetyTests: XCTestCase { XCTAssertEqual(StorageWatchService.runtimeState( protocolValues: protocolValues, expectedWatcherURL: expectedWatcher, - expectedHomeURL: root + expectedHomeURL: root, + expectedAppBundlePath: appBundlePath ), .stale) let mutableWatcher = root.appendingPathComponent("Application Support/Modore/runtime/scripts/storage_watch.sh") @@ -362,7 +372,8 @@ final class CleanupSafetyTests: XCTestCase { XCTAssertEqual(StorageWatchService.runtimeState( protocolValues: protocolValues, expectedWatcherURL: expectedWatcher, - expectedHomeURL: root + expectedHomeURL: root, + expectedAppBundlePath: appBundlePath ), .stale) let outsidePlist = root.appendingPathComponent("outside.plist") @@ -371,7 +382,8 @@ final class CleanupSafetyTests: XCTestCase { XCTAssertEqual(StorageWatchService.runtimeState( protocolValues: protocolValues, expectedWatcherURL: expectedWatcher, - expectedHomeURL: root + expectedHomeURL: root, + expectedAppBundlePath: appBundlePath ), .stale) // Uninstall must remove the entry rather than merely unload it. @@ -379,11 +391,13 @@ final class CleanupSafetyTests: XCTestCase { XCTAssertEqual(StorageWatchService.runtimeState( protocolValues: protocolValues, expectedWatcherURL: expectedWatcher, - expectedHomeURL: root + expectedHomeURL: root, + expectedAppBundlePath: appBundlePath ), .absent) } func testStorageWatchRejectsOversizedPlistAndSymlinkedParent() throws { + let appBundlePath = "/Applications/Modore.app" let root = FileManager.default.temporaryDirectory .appendingPathComponent("pch-watch-bounds-\(UUID().uuidString)") defer { try? FileManager.default.removeItem(at: root) } @@ -403,7 +417,8 @@ final class CleanupSafetyTests: XCTestCase { XCTAssertEqual(StorageWatchService.runtimeState( protocolValues: values, expectedWatcherURL: expectedWatcher, - expectedHomeURL: root + expectedHomeURL: root, + expectedAppBundlePath: appBundlePath ), .stale) try FileManager.default.removeItem(at: launchAgents) @@ -418,7 +433,8 @@ final class CleanupSafetyTests: XCTestCase { XCTAssertEqual(StorageWatchService.runtimeState( protocolValues: values, expectedWatcherURL: expectedWatcher, - expectedHomeURL: root + expectedHomeURL: root, + expectedAppBundlePath: appBundlePath ), .stale) } diff --git a/macos/Modore/Tests/ModoreTests/SpaceGoalViewRangeTests.swift b/macos/Modore/Tests/ModoreTests/SpaceGoalViewRangeTests.swift new file mode 100644 index 0000000..e5a0350 --- /dev/null +++ b/macos/Modore/Tests/ModoreTests/SpaceGoalViewRangeTests.swift @@ -0,0 +1,71 @@ +import SwiftUI +import XCTest +@testable import Modore + +/// The goal slider's range is the one place in this view that can take the +/// whole page down: SwiftUI's Slider divides the range by `step` and calls +/// `fatalError("max stride must be positive")` on a zero-width range, which +/// is not catchable. `1...max(achievableGB, 1)` collapsed to `1...1` for any +/// cleanable total in (0, 1]GB -- a single small npm cache -- so the 목표 tab +/// hard-crashed the app. These assert the bound is always strictly above the +/// lower bound, and that the sub-1GB case doesn't render a slider at all. +final class SpaceGoalViewRangeTests: XCTestCase { + private func snapshot(cleanableGB: [Double]) -> StorageSnapshot { + let candidates = cleanableGB.enumerated().map { index, size in + [ + "risk": "warning", + "kind": "cache", + "label": "cache-\(index)", + "sizeGB": size, + "path": "/tmp/cache-\(index)", + "action": "정리", + "note": "", + "measureStatus": "ok", + "cleanupId": "npm_cache", + ] as [String: Any] + } + return StorageSnapshot(json: [ + "volume": [ + "mount": "/", "freeGB": 30, "usedGB": 70, + "totalGB": 100, "usePercent": 70, "risk": "safe", + ], + "cleanupCandidates": candidates, + ])! + } + + @MainActor + private func renderGoalTab(cleanableGB: [Double]) { + let view = SpaceGoalWorkspaceList(storage: snapshot(cleanableGB: cleanableGB)) + .environmentObject(ScanModel()) + let host = NSHostingView(rootView: view) + host.frame = NSRect(x: 0, y: 0, width: 640, height: 480) + host.layoutSubtreeIfNeeded() + _ = host.fittingSize + } + + /// The exact crash: cleanable total greater than zero but at or below 1GB. + @MainActor + func testRendersWithASubOneGigabyteCleanableTotal() { + renderGoalTab(cleanableGB: [0.5]) + } + + @MainActor + func testRendersWhenEveryCandidateMeasuresZero() { + renderGoalTab(cleanableGB: [0, 0]) + } + + @MainActor + func testRendersAtExactlyOneGigabyte() { + renderGoalTab(cleanableGB: [1.0]) + } + + @MainActor + func testRendersWithAnOrdinaryMultiGigabyteTotal() { + renderGoalTab(cleanableGB: [1.5, 1.5, 1.4]) + } + + @MainActor + func testRendersWithNoCandidatesAtAll() { + renderGoalTab(cleanableGB: []) + } +} diff --git a/scripts/login_items.sh b/scripts/login_items.sh index 00a23d5..7b30184 100755 --- a/scripts/login_items.sh +++ b/scripts/login_items.sh @@ -79,9 +79,18 @@ current_login_item_names() { "$OSASCRIPT_BIN" -e 'tell application "System Events" to get the name of every login item' 2>/dev/null } +# 0 = present, 1 = confirmed absent, 2 = could not determine. +# +# "Could not determine" must never collapse into "absent". A failed System +# Events query and an item that is genuinely gone are indistinguishable from +# the exit status alone, and treating the first as the second made this +# script report a persistence item as removed while it was still installed -- +# the exact class of silent false success the post-delete recheck exists to +# prevent. An empty list from a *successful* query is still a real answer +# (this Mac has zero login items), so only the query failing yields 2. login_item_exists() { local target="$1" names entry - names="$(current_login_item_names)" || return 1 + names="$(current_login_item_names)" || return 2 IFS=',' read -ra parts <<< "$names" # A Mac with zero login items yields an empty array, and macOS's bash 3.2 # treats "${parts[@]}" on an empty array as unbound under set -u. @@ -94,8 +103,16 @@ login_item_exists() { } cmd_preview() { - local target="$1" - if ! login_item_exists "$target"; then + local target="$1" presence + login_item_exists "$target" + presence=$? + if [[ "$presence" -eq 2 ]]; then + # No token may be issued off a reading we could not actually take. + emit "status" "blocked" + emit "name" "$target" + return 1 + fi + if [[ "$presence" -ne 0 ]]; then emit "status" "not_found" emit "name" "$target" return 1 @@ -209,7 +226,17 @@ cmd_execute() { return 1 fi - if ! login_item_exists "$target"; then + local presence + login_item_exists "$target" + presence=$? + if [[ "$presence" -eq 2 ]]; then + # Cannot read the current list, so we can neither confirm the item is + # there nor claim it is gone. Refuse rather than delete blind. + emit "status" "blocked" + emit "name" "$target" + return 1 + fi + if [[ "$presence" -ne 0 ]]; then # Removed some other way (System Settings, the app itself) between # preview and execute. The desired end state already holds. emit "status" "already_gone" @@ -222,8 +249,13 @@ cmd_execute() { "$OSASCRIPT_BIN" -e "tell application \"System Events\" to delete login item \"$escaped\"" >/dev/null 2>&1 # A clean osascript exit only means the command was accepted, not that - # the item is actually gone. Re-read the real list before reporting ok. - if login_item_exists "$target"; then + # the item is actually gone. Re-read the real list before reporting ok -- + # and only a successful read proving absence counts. A failed re-read + # leaves the outcome unknown, which is a failure to report removal, not + # a removal. + login_item_exists "$target" + presence=$? + if [[ "$presence" -ne 1 ]]; then emit "status" "failed" emit "name" "$target" return 1 diff --git a/scripts/network_watch.sh b/scripts/network_watch.sh index 9446c58..9823f74 100755 --- a/scripts/network_watch.sh +++ b/scripts/network_watch.sh @@ -80,6 +80,19 @@ sample_listen() { "$LSOF_BIN" -nP -iTCP -sTCP:LISTEN 2>/dev/null || true } +# lsof exits non-zero with no output both when it fails and when nothing +# matches, and `|| true` cannot tell those apart -- so an empty closing +# sample is indistinguishable from "the window was quiet". Reporting 0 new +# connections off a sample we may never have taken is a false all-clear on a +# security surface, so the closing LISTEN set is used as the liveness probe: +# a real Mac always has listening sockets (launchd/rapportd/mDNSResponder), +# making an empty closing list overwhelmingly a failed read rather than a +# genuine state. The opening sample needs no such probe -- an empty opening +# baseline just makes everything look new, which errs toward over-reporting. +closing_sample_looks_unreadable() { + [[ ! -s "$WORKSPACE/second_listen" ]] +} + # 테스트는 네 표본 파일을 직접 주입한다. 실제 lsof 표는 재현할 수 없으므로, # 델타 계산을 고정된 입력으로 검증한다. inject_or_empty() { @@ -110,6 +123,11 @@ emit "version" "$PROTOCOL_VERSION" emit "operation" "network-watch" emit "windowSeconds" "$WINDOW_SECONDS" +if closing_sample_looks_unreadable; then + emit "error" "관찰 종료 시점의 네트워크 목록을 읽지 못했습니다." + exit 0 +fi + # lsof 열: COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME (ESTABLISHED 행은 # NAME 뒤에 "(ESTABLISHED)"가 하나 더 붙어 총 10 필드). NAME(9번째 필드)이 # established는 "LOCAL->REMOTE", listen은 "ADDR:PORT" 형태다. diff --git a/tests/test_macos_login_items.py b/tests/test_macos_login_items.py index 6f44a71..bd5a327 100644 --- a/tests/test_macos_login_items.py +++ b/tests/test_macos_login_items.py @@ -25,6 +25,38 @@ def parse_protocol(text: str) -> dict[str, str]: return values +def _failing_query_osascript(tmp_path, *, initial_items, healthy_queries): + """System Events answers `healthy_queries` list reads, then fails every + later one (no output, non-zero exit) -- the shape of Automation being + revoked or System Events going unscriptable between preview and execute. + Deletes always fail, so the item genuinely survives.""" + state_file = tmp_path / "login-items-state.txt" + state_file.write_text(initial_items, encoding="utf-8") + counter = tmp_path / "query-count.txt" + counter.write_text("0", encoding="utf-8") + stub = tmp_path / "osascript-failing-stub" + stub.write_text( + f"""#!/bin/bash + script="$2" + case "$script" in + *"get the name of every login item"*) + n=$(( $(cat "{counter}") + 1 )) + printf '%s' "$n" > "{counter}" + [[ "$n" -gt {healthy_queries} ]] && exit 1 + cat "{state_file}" + ;; + *"delete login item"*) + exit 1 + ;; + esac + exit 0 + """, + encoding="utf-8", + ) + stub.chmod(0o755) + return stub, state_file + + def _fake_osascript(tmp_path, *, initial_items, delete_is_noop=False): """A stateful System Events stand-in. `state_file` holds the current comma-separated login item list; `get` reads it, `delete` mutates it @@ -242,6 +274,60 @@ def test_execute_reports_already_gone_when_item_vanished_before_execute(project_ assert "delete login item" not in calls_after[len(calls_before):] +def test_execute_does_not_call_a_failed_query_already_gone(project_root, tmp_path): + """A failed System Events read and a genuinely removed item are the same + exit status, and treating the first as the second told the owner a + persistence mechanism was gone while it was still installed. Only a + successful read may assert absence.""" + home = tmp_path / "home" + # Query 1 is the preview; every query from the execute onward fails. + stub, state_file = _failing_query_osascript(tmp_path, initial_items="Foo, Bar", healthy_queries=1) + + _, preview_payload = _preview(project_root, tmp_path, "Bar", osascript_stub=stub, home=home) + assert preview_payload["status"] == "ready" + + result, payload = _execute( + project_root, tmp_path, "Bar", preview_payload["approvalToken"], osascript_stub=stub, home=home + ) + + assert payload["status"] == "blocked", payload + assert result.returncode == 1 + assert "Bar" in state_file.read_text(encoding="utf-8") + + +def test_execute_does_not_report_ok_when_the_post_delete_recheck_fails(project_root, tmp_path): + """The recheck exists to distinguish "osascript accepted the command" + from "the item is actually gone". If the recheck itself cannot run, the + outcome is unknown -- which is a failure to confirm removal, not a + removal.""" + home = tmp_path / "home" + # Queries 1 (preview) and 2 (pre-delete check) succeed; the post-delete + # recheck fails, and the delete itself failed too, so the item survives. + stub, state_file = _failing_query_osascript(tmp_path, initial_items="Foo, Bar", healthy_queries=2) + + _, preview_payload = _preview(project_root, tmp_path, "Bar", osascript_stub=stub, home=home) + result, payload = _execute( + project_root, tmp_path, "Bar", preview_payload["approvalToken"], osascript_stub=stub, home=home + ) + + assert payload["status"] == "failed", payload + assert result.returncode == 1 + assert "Bar" in state_file.read_text(encoding="utf-8") + + +def test_preview_does_not_call_a_failed_query_not_found(project_root, tmp_path): + """not_found means "this is not a login item". A failed read means we do + not know, and must not issue an approval token off it either way.""" + home = tmp_path / "home" + stub, _ = _failing_query_osascript(tmp_path, initial_items="Foo, Bar", healthy_queries=0) + + result, payload = _preview(project_root, tmp_path, "Bar", osascript_stub=stub, home=home) + + assert payload["status"] == "blocked", payload + assert result.returncode == 1 + assert "approvalToken" not in payload + + def test_execute_requires_owner_approved_flag(project_root, tmp_path): home = tmp_path / "home" stub, _, _ = _fake_osascript(tmp_path, initial_items="Foo") diff --git a/tests/test_macos_network_watch.py b/tests/test_macos_network_watch.py index 7cd2171..4ad344d 100644 --- a/tests/test_macos_network_watch.py +++ b/tests/test_macos_network_watch.py @@ -16,6 +16,11 @@ import pytest LSOF_HEADER = "COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME" +# Every real Mac has listening sockets (launchd, rapportd, mDNSResponder), so +# a genuinely empty closing LISTEN read means the read failed. Tests that +# aren't about the listen delta still need a plausible baseline in both +# samples, or they'd be exercising the can't-read path by accident. +BASELINE_LISTEN = 'rapportd 658 ren 11u IPv4 0x475 0t0 TCP *:49152 (LISTEN)\n' def run_watcher( @@ -23,8 +28,8 @@ def run_watcher( tmp_path: Path, first_established: str, second_established: str, - first_listen: str = "", - second_listen: str = "", + first_listen: str = BASELINE_LISTEN, + second_listen: str = BASELINE_LISTEN, *args: str, ): files = { @@ -149,8 +154,8 @@ def test_lsof_escaped_spaces_in_process_names_are_unescaped(project_root, tmp_pa 'Chrome 1000 ren 23u IPv4 0xaaa 0t0 TCP 192.168.0.156:51000->1.1.1.1:443 (ESTABLISHED)\n' 'Codex\\x20 1142 ren 24u IPv4 0xbbb 0t0 TCP 192.168.0.156:52000->2.2.2.2:8080 (ESTABLISHED)\n' ) - first_listen = "" - second_listen = 'Manus\\x20 2200 ren 11u IPv4 0xccc 0t0 TCP *:9999 (LISTEN)\n' + first_listen = BASELINE_LISTEN + second_listen = BASELINE_LISTEN + 'Manus\\x20 2200 ren 11u IPv4 0xccc 0t0 TCP *:9999 (LISTEN)\n' result = run_watcher( project_root, tmp_path, first, second, first_listen, second_listen, "--window", "5" @@ -173,12 +178,36 @@ def test_a_failed_first_sample_does_not_suppress_new_reports(project_root, tmp_p # genuinely new one, and over-reporting is the safe direction. second = 'Codex 1142 ren 24u IPv4 0xbbb 0t0 TCP 192.168.0.156:52000->2.2.2.2:8080 (ESTABLISHED)\n' - result = run_watcher(project_root, tmp_path, "", second, "", "", "--window", "5") + result = run_watcher( + project_root, tmp_path, "", second, BASELINE_LISTEN, BASELINE_LISTEN, "--window", "5" + ) assert result.returncode == 0, result.stderr assert parse_rows(result.stdout, "established") == [["Codex", "1142", "2.2.2.2:8080"]] +@pytest.mark.skipif(sys.platform != "darwin", reason="the network observer is macOS-only") +def test_an_unreadable_closing_sample_is_reported_not_read_as_a_quiet_window(project_root, tmp_path): + # lsof exits non-zero with no output both on failure and on zero matches, + # and `|| true` erases the difference. Emitting newEstablished/newListen 0 + # off a closing sample we may never have taken is a false all-clear on a + # security surface: the window looks quiet precisely because nothing was + # read. Every real Mac has listening sockets, so an empty closing LISTEN + # list is the usable liveness signal. + established = 'Chrome 1000 ren 23u IPv4 0xaaa 0t0 TCP 192.168.0.156:51000->1.1.1.1:443 (ESTABLISHED)\n' + + result = run_watcher( + project_root, tmp_path, established, established, BASELINE_LISTEN, "", "--window", "5" + ) + + assert result.returncode == 0, result.stderr + values = parse_values(result.stdout) + assert "error" in values, values + # A caller must not be able to read this as "0 new connections". + assert "newEstablished" not in values + assert "newListen" not in values + + def test_watcher_refuses_an_unbounded_window(project_root, tmp_path): result = run_watcher(project_root, tmp_path, "", "", "", "", "--window", "9000") diff --git a/tests/test_service_contracts.py b/tests/test_service_contracts.py index c65f02a..3fd67a5 100644 --- a/tests/test_service_contracts.py +++ b/tests/test_service_contracts.py @@ -522,6 +522,51 @@ def test_release_artifacts_exclude_runtime_python(project_root): ) +def test_storage_watch_plist_arguments_agree_across_schedule_sh_and_swift(project_root): + """schedule.sh writes the LaunchAgent's ProgramArguments; Swift's + StorageWatchService.runtimeState re-derives the same array and compares it + element-by-element to decide whether an installed watch is current or + stale. Nothing tied the two lists together, and they drifted: schedule.sh + started emitting a PCH_STORAGE_WATCH_APP_BUNDLE= entry while the Swift + expectation kept the older shape, so every freshly installed plist failed + the comparison. The toggle reported failure and the UI showed the watch as + off while launchd had actually loaded the job -- verified against a real + installed plist on a live machine, not hypothesised. + + The Swift-side unit test could not catch this because its fixture built + the plist from the same wrong list the implementation used. + """ + schedule = (project_root / "scripts" / "schedule.sh").read_text(encoding="utf-8") + service = ( + project_root / "macos" / "Modore" / "Sources" / "Modore" / "Services" + / "StorageWatchService.swift" + ).read_text(encoding="utf-8") + + shell_block = re.search( + r"expected_arguments=\"\$\(/usr/bin/printf '%s\\n' \\\n(.*?)\n\s*\[\[", + schedule, + re.DOTALL, + ) + assert shell_block, "could not find schedule.sh's expected_arguments block" + swift_block = re.search(r"let expectedArguments = \[(.*?)\n\s*\]", service, re.DOTALL) + assert swift_block, "could not find StorageWatchService's expectedArguments array" + + # Compare only the environment assignments -- the surrounding literals + # (paths, the wrapper body, the hash) are spelled differently per language + # by necessity, but an env entry appearing on one side only is exactly the + # drift that broke this. + def env_names(text: str) -> list[str]: + return re.findall(r"([A-Z][A-Z0-9_]*)=", text) + + shell_env = env_names(shell_block.group(1)) + swift_env = env_names(swift_block.group(1)) + assert shell_env == swift_env, ( + f"storage-watch plist argument drift:\n" + f" schedule.sh: {shell_env}\n" + f" Swift: {swift_env}" + ) + + def test_bundled_app_runtime_includes_every_macos_script(project_root): """release_smoke.py's MACOS_FILES (checked above) is a manifest- completeness gate, not what actually ships -- build_macos_swift_app.sh