diff --git a/supacode/Clients/Github/GithubCLIClient.swift b/supacode/Clients/Github/GithubCLIClient.swift index b32878b92..e761b68bd 100644 --- a/supacode/Clients/Github/GithubCLIClient.swift +++ b/supacode/Clients/Github/GithubCLIClient.swift @@ -8,7 +8,7 @@ struct GithubAuthStatus: Equatable, Sendable { let host: String } -private struct GithubAuthStatusResponse: Sendable { +struct GithubAuthStatusResponse: Sendable { let hosts: [String: [GithubAuthAccount]] struct GithubAuthAccount: Sendable { @@ -41,6 +41,148 @@ extension GithubAuthStatusResponse.GithubAuthAccount: Decodable { } } +// A login shell sources `.zprofile` / `.zlogin` before `gh` runs, so a banner or version-manager +// line can prepend to captured stdout and corrupt the JSON. +enum GithubCLIOutput { + // No JSON found at all: the likely cause is shell startup output polluting stdout. + nonisolated static let noPayloadMessage = + "Could not read GitHub CLI output. Your shell startup files may be printing extra output to stdout." + + // Valid JSON was found but failed to decode: the likely cause is an incompatible gh version. + nonisolated static let undecodableMessage = + "Could not parse GitHub CLI output. The installed GitHub CLI version may be incompatible." + + nonisolated private static let logger = SupaLogger("GithubCLI") + + // Every balanced top-level JSON value ({...} or [...]) in `output`, in source order. An opener that + // never balances (a stray brace from shell noise) is skipped, so leading noise cannot swallow a real + // payload that follows. + nonisolated static func balancedJSONSpans(in output: String) -> [Substring] { + var spans: [Substring] = [] + var searchStart = output.startIndex + while let start = output[searchStart...].firstIndex(where: { $0 == "{" || $0 == "[" }) { + let opener = output[start] + let closer: Character = opener == "{" ? "}" : "]" + var depth = 0 + var inString = false + var escaped = false + var matchedEnd: String.Index? + var index = start + scan: while index < output.endIndex { + let character = output[index] + switch character { + case _ where inString && escaped: + escaped = false + case "\\" where inString: + escaped = true + case "\"" where inString: + inString = false + case _ where inString: + break + case "\"": + inString = true + case opener: + depth += 1 + case closer: + depth -= 1 + guard depth == 0 else { break } + matchedEnd = output.index(after: index) + break scan + default: + break + } + index = output.index(after: index) + } + guard let end = matchedEnd else { + // Unbalanced opener (stray bracket in noise): skip it and keep scanning. + searchStart = output.index(after: start) + continue + } + spans.append(output[start..( + _ type: T.Type, + from output: String, + decoder: JSONDecoder = JSONDecoder() + ) throws -> T { + guard let value = try decodeIfPresent(T.self, from: output, decoder: decoder) else { + logDecodeFailure(output) + throw GithubCLIError.commandFailed(noPayloadMessage) + } + return value + } + + // Returns nil when `output` carries no JSON payload at all (e.g. `gh run list` with no runs), and + // throws only when a payload is present but cannot be decoded. gh prints its JSON after any leading + // shell noise, so the last decodable span wins. + nonisolated static func decodeIfPresent( + _ type: T.Type, + from output: String, + decoder: JSONDecoder = JSONDecoder() + ) throws -> T? { + let spans = balancedJSONSpans(in: output) + guard !spans.isEmpty else { + return nil + } + var lastDecodingError: Error? + var sawValidJSON = false + for span in spans.reversed() { + let data = Data(span.utf8) + do { + return try decoder.decode(T.self, from: data) + } catch { + lastDecodingError = error + if (try? JSONSerialization.jsonObject(with: data)) != nil { + sawValidJSON = true + } + } + } + let detail = lastDecodingError.map { "\($0)" } ?? "unknown" + logger.error("Failed to decode GitHub CLI JSON output: \(snapshot(of: output)) error=\(detail)") + // Valid JSON that failed to decode points at gh schema drift; otherwise the brackets were noise. + throw GithubCLIError.commandFailed(sawValidJSON ? undecodableMessage : noPayloadMessage) + } + + // Logs a length-capped snapshot of the offending output so the user can retrieve it from the log + // stream without flooding it with a large banner. + nonisolated static func logDecodeFailure(_ output: String) { + logger.error("Failed to read GitHub CLI JSON output: \(snapshot(of: output))") + } + + // Caps the logged output so a large banner does not flood the log stream. + nonisolated private static func snapshot(of output: String) -> String { + let limit = 500 + let prefix = output.prefix(limit) + return prefix.endIndex == output.endIndex ? String(prefix) : "\(prefix)… (truncated)" + } +} + +enum GithubAuthStatusParsing { + // `hosts` is an unordered dictionary, so prefer github.com and otherwise sort the keys: the result + // is deterministic even when more than one host has an active account. + nonisolated static func activeAccount( + in response: GithubAuthStatusResponse + ) -> (host: String, login: String)? { + let orderedHosts = response.hosts.keys.sorted { lhs, rhs in + if lhs == "github.com" { return true } + if rhs == "github.com" { return false } + return lhs < rhs + } + for host in orderedHosts { + if let active = response.hosts[host]?.first(where: { $0.active }) { + return (host, active.login) + } + } + return nil + } +} + struct GithubCLIClient: Sendable { var defaultBranch: @Sendable (URL) async throws -> String var latestRun: @Sendable (URL, String) async throws -> GithubWorkflowRun? @@ -193,10 +335,9 @@ nonisolated private func defaultBranchFetcher( arguments: ["repo", "view", "--json", "defaultBranchRef"], repoRoot: repoRoot ) - let data = Data(output.utf8) let decoder = JSONDecoder() decoder.dateDecodingStrategy = .iso8601 - let response = try decoder.decode(GithubRepoViewResponse.self, from: data) + let response = try GithubCLIOutput.decode(GithubRepoViewResponse.self, from: output, decoder: decoder) return response.defaultBranchRef.name } } @@ -221,14 +362,11 @@ nonisolated private func latestRunFetcher( ], repoRoot: repoRoot ) - if output.isEmpty { - return nil - } - let data = Data(output.utf8) let decoder = JSONDecoder() decoder.dateDecodingStrategy = .iso8601 - let runs = try decoder.decode([GithubWorkflowRun].self, from: data) - return runs.first + // nil payload means no runs; a present-but-undecodable payload still throws. + let runs = try GithubCLIOutput.decodeIfPresent([GithubWorkflowRun].self, from: output, decoder: decoder) + return runs?.first } } @@ -247,33 +385,36 @@ nonisolated private func resolveRemoteInfoFetcher( resolver: GithubCLIExecutableResolver ) -> @Sendable (URL) async -> GithubRemoteInfo? { { repoRoot in + let output: String do { - let output = try await runGh( + output = try await runGh( shell: shell, resolver: resolver, arguments: ["repo", "view", "--json", "owner,name,url"], repoRoot: repoRoot ) - let trimmed = output.trimmingCharacters(in: .whitespacesAndNewlines) - guard !trimmed.isEmpty else { - return nil - } - let response = try JSONDecoder().decode( + } catch { + return nil + } + // nil contract: decodeIfPresent tolerates leading shell noise and logs a present-but-undecodable + // payload before throwing, which `try?` collapses back to nil. + guard + let response = try? GithubCLIOutput.decodeIfPresent( GithubRepoViewRemoteInfoResponse.self, - from: Data(trimmed.utf8) - ) - let host = hostFromRepoViewURL(response.url) ?? "github.com" - guard !response.owner.login.isEmpty, !response.name.isEmpty else { - return nil - } - return GithubRemoteInfo( - host: host, - owner: response.owner.login, - repo: response.name + from: output ) - } catch { + else { + return nil + } + let host = hostFromRepoViewURL(response.url) ?? "github.com" + guard !response.owner.login.isEmpty, !response.name.isEmpty else { return nil } + return GithubRemoteInfo( + host: host, + owner: response.owner.login, + repo: response.name + ) } } @@ -460,14 +601,11 @@ nonisolated private func authStatusFetcher( arguments: ["auth", "status", "--json", "hosts"], repoRoot: nil ) - let data = Data(output.utf8) - let response = try decodeAuthStatusResponse(from: data) - guard let (host, accounts) = response.hosts.first, - let activeAccount = accounts.first(where: { $0.active }) - else { + let response = try GithubCLIOutput.decode(GithubAuthStatusResponse.self, from: output) + guard let active = GithubAuthStatusParsing.activeAccount(in: response) else { return nil } - return GithubAuthStatus(username: activeAccount.login, host: host) + return GithubAuthStatus(username: active.login, host: active.host) } } @@ -617,10 +755,13 @@ nonisolated private func fetchPullRequestsChunk( return (chunkIndex, [:]) } - let data = Data(result.output.utf8) let decoder = JSONDecoder() decoder.dateDecodingStrategy = .iso8601 - let response = try decoder.decode(GithubGraphQLPullRequestResponse.self, from: data) + let response = try GithubCLIOutput.decode( + GithubGraphQLPullRequestResponse.self, + from: result.output, + decoder: decoder + ) let prsByBranch = response.pullRequestsByBranch( aliasMap: result.aliasMap, owner: request.owner, @@ -792,7 +933,3 @@ nonisolated private func shouldRetryGhExecution(after error: Error) -> Bool { } return false } - -nonisolated private func decodeAuthStatusResponse(from data: Data) throws -> GithubAuthStatusResponse { - try JSONDecoder().decode(GithubAuthStatusResponse.self, from: data) -} diff --git a/supacodeTests/GithubCLIClientTests.swift b/supacodeTests/GithubCLIClientTests.swift index de5973792..051448fa2 100644 --- a/supacodeTests/GithubCLIClientTests.swift +++ b/supacodeTests/GithubCLIClientTests.swift @@ -543,6 +543,297 @@ struct GithubCLIClientTests { #expect(snapshot.ghCallCount == 2) #expect(snapshot.loginCallCount == 2) } + + // MARK: - Noisy login-shell output (#377) + + // A ShellClient that resolves `gh` via `which` and returns `stdout` for every gh invocation. + static func ghShell(stdout: String) -> ShellClient { + ShellClient( + run: { executableURL, _, _ in + executableURL.lastPathComponent == "which" + ? ShellOutput(stdout: "/usr/bin/gh", stderr: "", exitCode: 0) + : ShellOutput(stdout: "", stderr: "", exitCode: 0) + }, + runLoginImpl: { executableURL, _, _, _ in + guard executableURL.lastPathComponent == "gh" else { + return ShellOutput(stdout: "", stderr: "", exitCode: 0) + } + return ShellOutput(stdout: stdout, stderr: "", exitCode: 0) + } + ) + } + + @Test func balancedSpansReturnsCleanObjectUnchanged() { + #expect(GithubCLIOutput.balancedJSONSpans(in: #"{"a":1}"#).map(String.init) == [#"{"a":1}"#]) + } + + @Test func balancedSpansStripBannerBeforeObject() { + let noisy = "fastfetch banner line\nanother line\n" + #"{"hosts":{}}"# + #expect(GithubCLIOutput.balancedJSONSpans(in: noisy).map(String.init) == [#"{"hosts":{}}"#]) + } + + @Test func balancedSpansStripBannerBeforeArray() { + let noisy = "nvm: now using node v20\n" + #"[{"x":1},{"y":2}]"# + #expect(GithubCLIOutput.balancedJSONSpans(in: noisy).map(String.init) == [#"[{"x":1},{"y":2}]"#]) + } + + @Test func balancedSpansDropTrailingNoise() { + #expect(GithubCLIOutput.balancedJSONSpans(in: "{\"a\":1}\nshell goodbye").map(String.init) == [#"{"a":1}"#]) + } + + @Test func balancedSpansIgnoreBracesInsideStrings() { + #expect(GithubCLIOutput.balancedJSONSpans(in: #"{"a":"}]"}"#).map(String.init) == [#"{"a":"}]"}"#]) + } + + @Test func balancedSpansIgnoreEscapedQuoteInsideString() { + // The escaped quote keeps the scanner in-string, so the brace before it is not a premature close. + #expect(GithubCLIOutput.balancedJSONSpans(in: #"{"a":"}\""}"#).map(String.init) == [#"{"a":"}\""}"#]) + } + + @Test func balancedSpansReturnBothTopLevelObjectsInOrder() { + let input = #"{"a":1}"# + "\n" + #"{"b":2}"# + #expect(GithubCLIOutput.balancedJSONSpans(in: input).map(String.init) == [#"{"a":1}"#, #"{"b":2}"#]) + } + + @Test func balancedSpansSkipUnbalancedOpener() { + // A stray brace from a verbose login shell must not swallow the real trailing payload. + let noisy = "chpwd () {\n" + #"[{"databaseId":7}]"# + #expect(GithubCLIOutput.balancedJSONSpans(in: noisy).map(String.init) == [#"[{"databaseId":7}]"#]) + } + + @Test func balancedSpansReturnEmptyForPureNoise() { + #expect(GithubCLIOutput.balancedJSONSpans(in: "no json here at all").isEmpty) + } + + @Test func balancedSpansReturnEmptyForEmptyInput() { + #expect(GithubCLIOutput.balancedJSONSpans(in: " \n ").isEmpty) + } + + @Test func balancedSpansReturnEmptyForUnterminatedPayload() { + #expect(GithubCLIOutput.balancedJSONSpans(in: #"{"a":1"#).isEmpty) + } + + private struct SingleKeyPayload: Decodable, Equatable { + let value: Int + } + + @Test func decodeSkipsBracketBearingBannerBeforeObject() throws { + let noisy = "warn [deprecated]\n" + #"{"value":1}"# + let decoded = try GithubCLIOutput.decode(SingleKeyPayload.self, from: noisy) + #expect(decoded == SingleKeyPayload(value: 1)) + } + + @Test func decodeSkipsVersionManagerBannerBeforeObject() throws { + let noisy = "[nvm] using node v20\n" + #"{"hosts":{}}"# + let decoded = try GithubCLIOutput.decode(GithubAuthStatusResponse.self, from: noisy) + #expect(decoded.hosts.isEmpty) + } + + @Test func activeAccountScansAllHostsForActiveAccount() { + let response = GithubAuthStatusResponse(hosts: [ + "ghe.acme.com": [.init(active: false, login: "work")], + "github.com": [.init(active: true, login: "sbertix")], + ]) + + let active = GithubAuthStatusParsing.activeAccount(in: response) + + #expect(active?.host == "github.com") + #expect(active?.login == "sbertix") + } + + @Test func activeAccountReturnsNilWhenNoAccountActive() { + let response = GithubAuthStatusResponse(hosts: [ + "github.com": [.init(active: false, login: "sbertix")] + ]) + + #expect(GithubAuthStatusParsing.activeAccount(in: response) == nil) + } + + @Test func activeAccountPrefersGithubComWhenMultipleHostsActive() { + let response = GithubAuthStatusResponse(hosts: [ + "ghe.acme.com": [.init(active: true, login: "work")], + "github.com": [.init(active: true, login: "sbertix")], + ]) + + let active = GithubAuthStatusParsing.activeAccount(in: response) + + #expect(active?.host == "github.com") + #expect(active?.login == "sbertix") + } + + @Test func activeAccountSortsHostsWhenGithubComAbsent() { + let response = GithubAuthStatusResponse(hosts: [ + "z.example.com": [.init(active: true, login: "zed")], + "a.example.com": [.init(active: true, login: "ann")], + ]) + + let active = GithubAuthStatusParsing.activeAccount(in: response) + + #expect(active?.host == "a.example.com") + #expect(active?.login == "ann") + } + + @Test func authStatusSucceedsDespiteBannerPollutedStdout() async throws { + let stdout = + "╭─ fastfetch ─╮\n│ os macOS │\n╰─────────────╯\n" + + #"{"hosts":{"github.com":[{"state":"success","active":true,"host":"github.com","login":"sbertix"}]}}"# + let client = GithubCLIClient.live(shell: Self.ghShell(stdout: stdout)) + + let status = try await client.authStatus() + + #expect(status == GithubAuthStatus(username: "sbertix", host: "github.com")) + } + + @Test func authStatusReportsActiveAccountOnNonFirstHost() async throws { + let stdout = #""" + {"hosts":{"ghe.acme.com":[{"active":false,"login":"work"}],"github.com":[{"active":true,"login":"sbertix"}]}} + """# + let client = GithubCLIClient.live(shell: Self.ghShell(stdout: stdout)) + + let status = try await client.authStatus() + + #expect(status == GithubAuthStatus(username: "sbertix", host: "github.com")) + } + + @Test func authStatusThrowsCommandFailedOnNonJsonOutput() async { + let client = GithubCLIClient.live(shell: Self.ghShell(stdout: "command not found: gh")) + + do { + _ = try await client.authStatus() + Issue.record("Expected authStatus to throw") + } catch GithubCLIError.commandFailed(let message) { + // No JSON payload -> the shell-pollution hypothesis, not a raw error. + #expect(message == GithubCLIOutput.noPayloadMessage) + } catch { + Issue.record("Unexpected error type: \(error.localizedDescription)") + } + } + + @Test func authStatusThrowsUndecodableMessageWhenPayloadParsesButSchemaDiffers() async { + let client = GithubCLIClient.live(shell: Self.ghShell(stdout: #"{"unexpected":true}"#)) + + do { + _ = try await client.authStatus() + Issue.record("Expected authStatus to throw") + } catch GithubCLIError.commandFailed(let message) { + // Found JSON but it failed to decode -> the version-incompatibility hypothesis. + #expect(message == GithubCLIOutput.undecodableMessage) + } catch { + Issue.record("Unexpected error type: \(error.localizedDescription)") + } + } + + @Test func resolveRemoteInfoSucceedsDespiteBannerPollutedStdout() async { + let stdout = + "loading shell profile…\n" + + #"{"name":"upstream-repo","owner":{"login":"upstream-org"},"# + + #""url":"https://github.com/upstream-org/upstream-repo"}"# + let client = GithubCLIClient.live(shell: Self.ghShell(stdout: stdout)) + + let info = await client.resolveRemoteInfo(URL(fileURLWithPath: "/tmp/repo")) + + #expect(info == GithubRemoteInfo(host: "github.com", owner: "upstream-org", repo: "upstream-repo")) + } + + @Test func defaultBranchSucceedsDespiteBannerPollutedStdout() async throws { + let stdout = "conda activate base\n" + #"{"defaultBranchRef":{"name":"main"}}"# + let client = GithubCLIClient.live(shell: Self.ghShell(stdout: stdout)) + + let branch = try await client.defaultBranch(URL(fileURLWithPath: "/tmp/repo")) + + #expect(branch == "main") + } + + @Test func defaultBranchThrowsCommandFailedOnNonJsonOutput() async { + let client = GithubCLIClient.live(shell: Self.ghShell(stdout: "not a repo")) + + do { + _ = try await client.defaultBranch(URL(fileURLWithPath: "/tmp/repo")) + Issue.record("Expected defaultBranch to throw") + } catch GithubCLIError.commandFailed { + // Expected. + } catch { + Issue.record("Unexpected error type: \(error.localizedDescription)") + } + } + + @Test func latestRunSucceedsDespiteBannerPollutedArray() async throws { + let stdout = + "pyenv: version 3.12\n" + + #"[{"databaseId":7,"workflowName":"CI","name":"CI","displayTitle":"Fix","# + + #""status":"completed","conclusion":"success","# + + #""createdAt":"2026-05-01T00:00:00Z","updatedAt":"2026-05-01T00:00:00Z"}]"# + let client = GithubCLIClient.live(shell: Self.ghShell(stdout: stdout)) + + let run = try await client.latestRun(URL(fileURLWithPath: "/tmp/repo"), "main") + + #expect(run?.databaseId == 7) + #expect(run?.conclusion == "success") + } + + @Test func latestRunReturnsNilForEmptyOutput() async throws { + let client = GithubCLIClient.live(shell: Self.ghShell(stdout: "")) + + let run = try await client.latestRun(URL(fileURLWithPath: "/tmp/repo"), "main") + + #expect(run == nil) + } + + @Test func latestRunReturnsNilForBannerWithNoRuns() async throws { + // Banner-only stdout with no JSON payload means no runs, not a parse failure. + let client = GithubCLIClient.live(shell: Self.ghShell(stdout: "shell banner with no json")) + + let run = try await client.latestRun(URL(fileURLWithPath: "/tmp/repo"), "main") + + #expect(run == nil) + } + + @Test func latestRunThrowsCommandFailedOnPresentButUndecodablePayload() async { + let client = GithubCLIClient.live(shell: Self.ghShell(stdout: #"[{"databaseId":"not-an-int"}]"#)) + + do { + _ = try await client.latestRun(URL(fileURLWithPath: "/tmp/repo"), "main") + Issue.record("Expected latestRun to throw on a present-but-undecodable payload") + } catch GithubCLIError.commandFailed { + // Expected: a JSON payload that fails to decode is a real parse failure. + } catch { + Issue.record("Unexpected error type: \(error.localizedDescription)") + } + } + + @Test func latestRunSkipsStrayBraceBannerBeforeRunArray() async throws { + // A stray unbalanced brace (e.g. a `set -x` trace) must not swallow the real run array. + let stdout = "+ chpwd () {\n" + #"[{"databaseId":11,"status":"completed"}]"# + let client = GithubCLIClient.live(shell: Self.ghShell(stdout: stdout)) + + let run = try await client.latestRun(URL(fileURLWithPath: "/tmp/repo"), "main") + + #expect(run?.databaseId == 11) + } + + @Test func latestRunPrefersRealArrayOverLeadingEmptyArrayBanner() async throws { + // A leading `[]` from shell noise must not shadow the real run list that follows. + let stdout = "[]\n" + #"[{"databaseId":9,"status":"completed","conclusion":"success"}]"# + let client = GithubCLIClient.live(shell: Self.ghShell(stdout: stdout)) + + let run = try await client.latestRun(URL(fileURLWithPath: "/tmp/repo"), "main") + + #expect(run?.databaseId == 9) + } + + @Test func authStatusReportsPollutionForBracketNoiseWithoutValidJson() async { + // Brackets that are not valid JSON (e.g. `[INFO]` log prefixes) are shell noise, not schema drift. + let client = GithubCLIClient.live(shell: Self.ghShell(stdout: "[INFO] starting up\n[WARN] no config")) + + do { + _ = try await client.authStatus() + Issue.record("Expected authStatus to throw") + } catch GithubCLIError.commandFailed(let message) { + #expect(message == GithubCLIOutput.noPayloadMessage) + } catch { + Issue.record("Unexpected error type: \(error.localizedDescription)") + } + } } nonisolated private func graphQLResponse(for arguments: [String]) -> String {