fix: non-blocking ShellExecutor — deadlock-safe, with tests

MonitorLizard/MonitorLizard.xcodeproj/project.pbxproj

@@ -28,6 +28,7 @@
 		32TESTS022F4A000000000001 /* UpdateServiceTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 32TESTS022F4A000000000000 /* UpdateServiceTests.swift */; };
 		32TESTS032F4A000000000001 /* WindowOcclusionObserverTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 32TESTS032F4A000000000000 /* WindowOcclusionObserverTests.swift */; };
 		32TESTS042F4A000000000001 /* GitHubServiceTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 32TESTS042F4A000000000000 /* GitHubServiceTests.swift */; };
+		32TESTS052F4A000000000001 /* ShellExecutorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 32TESTS052F4A000000000000 /* ShellExecutorTests.swift */; };
 		A8C2D4E19F7B4A2D9E3B7C41 /* Assets.xcassets in Resources */ = {isa = PBXBuildFile; fileRef = 7BF1FEEFD8924EFA85A1FE55 /* Assets.xcassets */; };
 		AA0001112F103E3200F0ABCD /* OtherPRsService.swift in Sources */ = {isa = PBXBuildFile; fileRef = AA0002222F103E3200F0ABCD /* OtherPRsService.swift */; };
 		AA0003332F103E3200F0ABCD /* AddPRView.swift in Sources */ = {isa = PBXBuildFile; fileRef = AA0004442F103E3200F0ABCD /* AddPRView.swift */; };
@@ -68,6 +69,7 @@
 		32TESTS022F4A000000000000 /* UpdateServiceTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = UpdateServiceTests.swift; sourceTree = "<group>"; };
 		32TESTS032F4A000000000000 /* WindowOcclusionObserverTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = WindowOcclusionObserverTests.swift; sourceTree = "<group>"; };
 		32TESTS042F4A000000000000 /* GitHubServiceTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = GitHubServiceTests.swift; sourceTree = "<group>"; };
+		32TESTS052F4A000000000000 /* ShellExecutorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ShellExecutorTests.swift; sourceTree = "<group>"; };
 		7BF1FEEFD8924EFA85A1FE55 /* Assets.xcassets */ = {isa = PBXFileReference; lastKnownFileType = folder.assetcatalog; path = Assets.xcassets; sourceTree = "<group>"; };
 		AA0002222F103E3200F0ABCD /* OtherPRsService.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = OtherPRsService.swift; sourceTree = "<group>"; };
 		AA0004442F103E3200F0ABCD /* AddPRView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AddPRView.swift; sourceTree = "<group>"; };
@@ -180,6 +182,7 @@
 				32TESTS022F4A000000000000 /* UpdateServiceTests.swift */,
 				32TESTS032F4A000000000000 /* WindowOcclusionObserverTests.swift */,
 				32TESTS042F4A000000000000 /* GitHubServiceTests.swift */,
+				32TESTS052F4A000000000000 /* ShellExecutorTests.swift */,
 			);
 			name = MonitorLizardTests;
 			path = ../MonitorLizardTests;
@@ -323,6 +326,7 @@
 				32TESTS022F4A000000000001 /* UpdateServiceTests.swift in Sources */,
 				32TESTS032F4A000000000001 /* WindowOcclusionObserverTests.swift in Sources */,
 				32TESTS042F4A000000000001 /* GitHubServiceTests.swift in Sources */,
+				32TESTS052F4A000000000001 /* ShellExecutorTests.swift in Sources */,
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 		};

MonitorLizard/Services/ShellExecutor.swift

@@ -1,4 +1,5 @@
 import Foundation
+import os
 
 /// Abstraction over shell command execution. Enables test doubles to be injected into services.
 protocol ShellExecuting: Sendable {
@@ -35,86 +36,135 @@ enum ShellError: Error {
     }
 }
 
-actor ShellExecutor: ShellExecuting {
-    func execute(command: String, arguments: [String] = [], timeout: TimeInterval = 30, host: String? = nil) async throws -> String {
+/// ShellExecutor runs shell commands. Each invocation creates its own
+/// `Process`, so concurrent calls are safe. Marked as `final class`
+/// (not `actor`) to allow true parallelism in task groups.
+final class ShellExecutor: ShellExecuting {
+    nonisolated(unsafe) static let networkErrorPatterns = [
+        "error connecting",
+        "check your internet connection",
+        "could not resolve host",
+        "network is unreachable",
+        "dial tcp",
+        "no such host",
+        "connection refused",
+        "i/o timeout",
+        "unable to connect",
+        "failed to connect"
+    ]
+
+    nonisolated func execute(command: String, arguments: [String] = [], timeout: TimeInterval = 30, host: String? = nil) async throws -> String {
         let process = Process()
 
-        // Set up the process
         process.executableURL = URL(fileURLWithPath: "/usr/bin/env")
         process.arguments = [command] + arguments
 
-        // Set PATH to include Homebrew and common installation locations
         var environment = ProcessInfo.processInfo.environment
         let homebrewPaths = [
-            "/opt/homebrew/bin",      // Apple Silicon Homebrew
-            "/usr/local/bin",          // Intel Homebrew
+            "/opt/homebrew/bin",
+            "/usr/local/bin",
             "/opt/homebrew/sbin",
             "/usr/local/sbin"
         ]
         let existingPath = environment["PATH"] ?? "/usr/bin:/bin:/usr/sbin:/sbin"
-        let newPath = (homebrewPaths + [existingPath]).joined(separator: ":")
-        environment["PATH"] = newPath
+        environment["PATH"] = (homebrewPaths + [existingPath]).joined(separator: ":")
 
-        // Set GH_HOST to target a specific GitHub host (e.g. enterprise)
-        if let host = host {
+        if let host {
             environment["GH_HOST"] = host
         }
 
         process.environment = environment
 
-        // Set up pipes for output and error
+        // Set up pipes for output and error.
+        // Collect data as it arrives via readabilityHandler to avoid
+        // deadlock when output exceeds the pipe buffer (~64KB).
         let outputPipe = Pipe()
         let errorPipe = Pipe()
         process.standardOutput = outputPipe
         process.standardError = errorPipe
 
-        // Launch the process
-        do {
-            try process.run()
-        } catch {
-            throw ShellError.commandNotFound
+        // OSAllocatedUnfairLock<Data> is safe to use from readabilityHandler
+        // callbacks (background threads) and from async contexts alike.
+        let outputBuffer = OSAllocatedUnfairLock(initialState: Data())
+        let errorBuffer = OSAllocatedUnfairLock(initialState: Data())
+
+        outputPipe.fileHandleForReading.readabilityHandler = { handle in
+            let data = handle.availableData
+            if !data.isEmpty { outputBuffer.withLock { $0.append(data) } }
+        }
+        errorPipe.fileHandleForReading.readabilityHandler = { handle in
+            let data = handle.availableData
+            if !data.isEmpty { errorBuffer.withLock { $0.append(data) } }
         }
 
-        // Wait for completion with timeout
-        let timeoutTask = Task {
-            try await Task.sleep(nanoseconds: UInt64(timeout * 1_000_000_000))
-            if process.isRunning {
-                process.terminate()
-                throw ShellError.executionFailed("Command timed out after \(timeout) seconds")
+        // Wait for completion without blocking the cooperative thread pool.
+        // terminationHandler must be set BEFORE run() to avoid a race
+        // where the process finishes before the handler is installed.
+        //
+        // resumedLock holds "already resumed" state. Both the terminationHandler
+        // and the timeout Task race to flip it; only the first caller wins.
+        // timeoutTaskBox lets terminationHandler cancel the timeout Task so it
+        // doesn't linger for the full timeout duration after normal completion.
+        let timedOut = try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<Bool, Error>) in
+            let resumedLock = OSAllocatedUnfairLock(initialState: false)
+            let timeoutTaskBox = OSAllocatedUnfairLock<Task<Void, Never>?>(initialState: nil)
+
+            process.terminationHandler = { _ in
+                timeoutTaskBox.withLock { $0 }?.cancel()
+                let alreadyResumed = resumedLock.withLock { state -> Bool in
+                    defer { state = true }
+                    return state
+                }
+                if !alreadyResumed { continuation.resume(returning: false) }
+            }
+
+            do {
+                try process.run()
+            } catch {
+                process.terminationHandler = nil
+                let alreadyResumed = resumedLock.withLock { state -> Bool in
+                    defer { state = true }
+                    return state
+                }
+                if !alreadyResumed { continuation.resume(throwing: ShellError.commandNotFound) }
+                return
+            }
+
+            // Timeout: terminate the process if it hasn't finished in time.
+            timeoutTaskBox.withLock {
+                $0 = Task {
+                    try? await Task.sleep(nanoseconds: UInt64(timeout * 1_000_000_000))
+                    guard !Task.isCancelled else { return }
+                    let alreadyResumed = resumedLock.withLock { state -> Bool in
+                        defer { state = true }
+                        return state
+                    }
+                    if !alreadyResumed {
+                        process.terminate()
+                        continuation.resume(returning: true)
+                    }
+                }
             }
         }
 
-        process.waitUntilExit()
-        timeoutTask.cancel()
+        // Stop reading handlers and drain remaining data.
+        outputPipe.fileHandleForReading.readabilityHandler = nil
+        errorPipe.fileHandleForReading.readabilityHandler = nil
+        outputBuffer.withLock { $0.append(outputPipe.fileHandleForReading.readDataToEndOfFile()) }
+        errorBuffer.withLock { $0.append(errorPipe.fileHandleForReading.readDataToEndOfFile()) }
 
-        // Read output
-        let outputData = outputPipe.fileHandleForReading.readDataToEndOfFile()
-        let errorData = errorPipe.fileHandleForReading.readDataToEndOfFile()
+        if timedOut {
+            throw ShellError.executionFailed("Command timed out after \(Int(timeout)) seconds")
+        }
+
+        let outputData = outputBuffer.withLock { $0 }
+        let errorData = errorBuffer.withLock { $0 }
 
-        // Check exit status
         guard process.terminationStatus == 0 else {
             let errorMessage = String(data: errorData, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "Unknown error"
 
-            // Detect network-related errors from gh CLI output
-            // When network is unavailable, gh returns messages like:
-            // - "error connecting to api.github.com"
-            // - "check your internet connection"
-            // These patterns help distinguish network issues from auth/permission errors
-            let networkErrorPatterns = [
-                "error connecting",
-                "check your internet connection",
-                "could not resolve host",
-                "network is unreachable",
-                "dial tcp",
-                "no such host",
-                "connection refused",
-                "i/o timeout",
-                "unable to connect",
-                "failed to connect"
-            ]
-
             let lowercaseMessage = errorMessage.lowercased()
-            for pattern in networkErrorPatterns {
+            for pattern in Self.networkErrorPatterns {
                 if lowercaseMessage.contains(pattern) {
                     throw ShellError.networkError(errorMessage)
                 }
@@ -123,7 +173,6 @@ actor ShellExecutor: ShellExecuting {
             throw ShellError.executionFailed(errorMessage)
         }
 
-        // Return output
         guard let output = String(data: outputData, encoding: .utf8) else {
             throw ShellError.invalidOutput
         }
@@ -132,38 +181,31 @@ actor ShellExecutor: ShellExecuting {
     }
 
     /// Returns all GitHub hosts the user is authenticated with (e.g. ["github.com", "github.enterprise.com"])
-    func getAuthenticatedHosts() async throws -> [String] {
+    nonisolated func getAuthenticatedHosts() async throws -> [String] {
         do {
             let output = try await execute(command: "gh", arguments: ["auth", "status"])
-            return parseHosts(from: output)
+            return Self.parseHosts(from: output)
         } catch let ShellError.executionFailed(message) {
             // gh auth status exits with non-zero if any host has issues,
             // but still prints host info to stderr/stdout
-            return parseHosts(from: message)
+            return Self.parseHosts(from: message)
         } catch {
-            return ["github.com"]  // Fallback to default
+            return ["github.com"]
         }
     }
 
-    private func parseHosts(from output: String) -> [String] {
-        // gh auth status output has hostnames at the start of lines (no leading whitespace)
-        // e.g.:
-        // github.com
-        //   ✓ Logged in to github.com account user (keyring)
-        // enterprise.example.com
-        //   ✓ Logged in to enterprise.example.com account user (keyring)
+    nonisolated static func parseHosts(from output: String) -> [String] {
         var hosts: [String] = []
         for line in output.components(separatedBy: .newlines) {
             let trimmed = line.trimmingCharacters(in: .whitespaces)
-            // Host lines start at column 0 (no leading whitespace) and contain a dot
             if !trimmed.isEmpty && !line.hasPrefix(" ") && !line.hasPrefix("\t") && trimmed.contains(".") && !trimmed.contains(" ") {
                 hosts.append(trimmed)
             }
         }
         return hosts.isEmpty ? ["github.com"] : hosts
     }
 
-    func checkGHInstalled() async throws -> Bool {
+    nonisolated func checkGHInstalled() async throws -> Bool {
         do {
             _ = try await execute(command: "which", arguments: ["gh"])
             return true
@@ -172,38 +214,20 @@ actor ShellExecutor: ShellExecuting {
         }
     }
 
-    func checkGHAuthenticated() async throws -> Bool {
+    nonisolated func checkGHAuthenticated() async throws -> Bool {
         do {
             let output = try await execute(command: "gh", arguments: ["auth", "status"])
             return output.contains("Logged in")
         } catch let ShellError.networkError(message) {
-            // Re-throw network errors so they can be handled properly upstream
-            // Important: Don't confuse network errors with authentication failures
             throw ShellError.networkError(message)
         } catch let ShellError.executionFailed(message) {
-            // Backup check for network errors that weren't caught by execute()
-            // Note: When offline, `gh auth status` may report "token is invalid"
-            // but actual API calls like `gh search prs` give proper "error connecting" messages
-            let networkErrorPatterns = [
-                "error connecting",
-                "check your internet connection",
-                "could not resolve host",
-                "network is unreachable",
-                "dial tcp",
-                "no such host",
-                "connection refused",
-                "i/o timeout",
-                "unable to connect"
-            ]
-
             let lowercaseMessage = message.lowercased()
-            for pattern in networkErrorPatterns {
+            for pattern in Self.networkErrorPatterns {
                 if lowercaseMessage.contains(pattern) {
                     throw ShellError.networkError(message)
                 }
             }
 
-            // If not a network error, this is likely an auth issue
             return false
         } catch {
             return false

MonitorLizardTests/PRMonitorViewModelTests.swift

@@ -163,6 +163,7 @@ struct ResolveReviewDecisionTests {
     }
 }
 
+@MainActor
 struct ParsePRURLTests {
 
     @Test func validGitHubURL() {
@@ -213,6 +214,7 @@ struct ParsePRURLTests {
     }
 }
 
+@MainActor
 struct OtherPRsServiceTests {
 
     private func makeService() -> OtherPRsService {
@@ -277,6 +279,7 @@ struct OtherPRsServiceTests {
     }
 }
 
+@MainActor
 struct PRTypeDisplayTitleTests {
 
     @Test(arguments: [0, 1, 5])
@@ -295,6 +298,7 @@ struct PRTypeDisplayTitleTests {
     }
 }
 
+@MainActor
 struct CustomNamesServiceTests {
 
     private func makeService() -> CustomNamesService {