diff --git a/MonitorLizard/MonitorLizard.xcodeproj/project.pbxproj b/MonitorLizard/MonitorLizard.xcodeproj/project.pbxproj index 5c56d2c..ff1150b 100644 --- a/MonitorLizard/MonitorLizard.xcodeproj/project.pbxproj +++ b/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 = ""; }; 32TESTS032F4A000000000000 /* WindowOcclusionObserverTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = WindowOcclusionObserverTests.swift; sourceTree = ""; }; 32TESTS042F4A000000000000 /* GitHubServiceTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = GitHubServiceTests.swift; sourceTree = ""; }; + 32TESTS052F4A000000000000 /* ShellExecutorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ShellExecutorTests.swift; sourceTree = ""; }; 7BF1FEEFD8924EFA85A1FE55 /* Assets.xcassets */ = {isa = PBXFileReference; lastKnownFileType = folder.assetcatalog; path = Assets.xcassets; sourceTree = ""; }; AA0002222F103E3200F0ABCD /* OtherPRsService.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = OtherPRsService.swift; sourceTree = ""; }; AA0004442F103E3200F0ABCD /* AddPRView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AddPRView.swift; sourceTree = ""; }; @@ -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; }; diff --git a/MonitorLizard/Services/ShellExecutor.swift b/MonitorLizard/Services/ShellExecutor.swift index 2ff98e7..856d4ad 100644 --- a/MonitorLizard/Services/ShellExecutor.swift +++ b/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 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) in + let resumedLock = OSAllocatedUnfairLock(initialState: false) + let timeoutTaskBox = OSAllocatedUnfairLock?>(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,30 +181,23 @@ 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) } @@ -163,7 +205,7 @@ actor ShellExecutor: ShellExecuting { 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 diff --git a/MonitorLizardTests/PRMonitorViewModelTests.swift b/MonitorLizardTests/PRMonitorViewModelTests.swift index aa85374..c36a6c8 100644 --- a/MonitorLizardTests/PRMonitorViewModelTests.swift +++ b/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 { diff --git a/MonitorLizardTests/ShellExecutorTests.swift b/MonitorLizardTests/ShellExecutorTests.swift new file mode 100644 index 0000000..9d0c10a --- /dev/null +++ b/MonitorLizardTests/ShellExecutorTests.swift @@ -0,0 +1,64 @@ +import Testing +import Foundation +@testable import MonitorLizard + +// MARK: - parseHosts Tests + +struct ShellExecutorParseHostsTests { + + @Test func singleHostIsExtracted() { + let output = """ + github.com + ✓ Logged in to github.com account alice (keyring) + """ + #expect(ShellExecutor.parseHosts(from: output) == ["github.com"]) + } + + @Test func multipleHostsAreExtracted() { + let output = """ + github.com + ✓ Logged in to github.com account alice (keyring) + enterprise.example.com + ✓ Logged in to enterprise.example.com account alice (keyring) + """ + #expect(ShellExecutor.parseHosts(from: output) == ["github.com", "enterprise.example.com"]) + } + + @Test func emptyOutputFallsBackToGitHubCom() { + #expect(ShellExecutor.parseHosts(from: "") == ["github.com"]) + } + + @Test func linesWithLeadingWhitespaceAreIgnored() { + // A hostname-like string with leading whitespace is a status line, not a host declaration. + let output = " enterprise.example.com" + #expect(ShellExecutor.parseHosts(from: output) == ["github.com"]) + } + + @Test func statusLinesContainingSpacesAreIgnored() { + // Status lines have no leading whitespace but contain spaces between words. + let output = "✓ Logged in to github.com account alice" + #expect(ShellExecutor.parseHosts(from: output) == ["github.com"]) + } + + @Test func linesWithoutDotAreIgnored() { + let output = "notahost\ngithub.com" + #expect(ShellExecutor.parseHosts(from: output) == ["github.com"]) + } +} + +// MARK: - Timeout Tests + +struct ShellExecutorTimeoutTests { + + @Test func commandTimesOut() async { + let executor = ShellExecutor() + do { + _ = try await executor.execute(command: "sleep", arguments: ["10"], timeout: 0.1) + Issue.record("Expected an error to be thrown") + } catch ShellError.executionFailed(let message) { + #expect(message.contains("timed out")) + } catch { + Issue.record("Unexpected error type: \(error)") + } + } +}