From 1dfcca9e3be3418b19d873b8492716b89c111e40 Mon Sep 17 00:00:00 2001 From: onevcat Date: Tue, 23 Jun 2026 00:38:49 +0900 Subject: [PATCH 1/2] Invert login-shell fallback logic for robust git detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of maintaining an allowlist of errors that should trigger the login-shell fallback (exit 127, "command not found", Xcode license, …), invert the check: fallback for ALL shell errors EXCEPT the one case where it's pointless — git ran successfully but confirmed this isn't a git repository. This handles Xcode shim failures (unaccepted license, broken developer path) and any future unknown environment errors without needing to track Apple's error messages. The worst case for an unnecessary fallback is one extra shell invocation that also fails. Closes #486 --- .../Clients/Git/GitClientShellHelpers.swift | 10 +-- .../GitClientShellFallbackTests.swift | 68 +++++++++++++++++++ 2 files changed, 74 insertions(+), 4 deletions(-) create mode 100644 supacodeTests/GitClientShellFallbackTests.swift diff --git a/supacode/Clients/Git/GitClientShellHelpers.swift b/supacode/Clients/Git/GitClientShellHelpers.swift index 8b7ae55e..5bd77939 100644 --- a/supacode/Clients/Git/GitClientShellHelpers.swift +++ b/supacode/Clients/Git/GitClientShellHelpers.swift @@ -7,11 +7,13 @@ nonisolated func shouldFallbackToLoginShell(_ error: Error) -> Bool { guard let shellError = error as? ShellClientError else { return false } - if shellError.exitCode == 127 { - return true - } let output = "\(shellError.stderr)\n\(shellError.stdout)".lowercased() - return output.contains("command not found") + // When git itself ran fine but confirmed this isn't a repo, retrying + // under a login shell won't change the answer. + if output.contains("not a git repository") { + return false + } + return true } nonisolated func wrapShellError( diff --git a/supacodeTests/GitClientShellFallbackTests.swift b/supacodeTests/GitClientShellFallbackTests.swift new file mode 100644 index 00000000..3e838e35 --- /dev/null +++ b/supacodeTests/GitClientShellFallbackTests.swift @@ -0,0 +1,68 @@ +import Foundation +import Testing + +@testable import supacode + +struct GitClientShellFallbackTests { + private func shellError( + stderr: String = "", + stdout: String = "", + exitCode: Int32 = 1 + ) -> ShellClientError { + ShellClientError(command: "wt root", stdout: stdout, stderr: stderr, exitCode: exitCode) + } + + // MARK: - Should fallback + + @Test func fallsBackWhenExecutableNotFound() { + #expect(shouldFallbackToLoginShell(shellError(exitCode: 127))) + } + + @Test func fallsBackOnCommandNotFoundMessage() { + #expect(shouldFallbackToLoginShell(shellError(stderr: "env: git: command not found"))) + } + + @Test func fallsBackWhenXcodeLicenseUnaccepted() { + let stderr = """ + You have not agreed to the Xcode license agreements. Please run 'sudo xcodebuild -license' \ + from within a Terminal window to review and agree to the Xcode and Apple SDKs license. + """ + #expect(shouldFallbackToLoginShell(shellError(stderr: stderr, exitCode: 69))) + } + + @Test func fallsBackOnInvalidActiveDeveloperPath() { + let stderr = "xcode-select: error: invalid active developer path (/Library/Developer/CommandLineTools)" + #expect(shouldFallbackToLoginShell(shellError(stderr: stderr, exitCode: 1))) + } + + @Test func fallsBackOnUnknownShellError() { + #expect(shouldFallbackToLoginShell(shellError(stderr: "something unexpected", exitCode: 42))) + } + + @Test func fallsBackOnEmptyErrorOutput() { + #expect(shouldFallbackToLoginShell(shellError(exitCode: 1))) + } + + // MARK: - Should NOT fallback + + @Test func doesNotFallBackForGenuineNonGitDirectory() { + #expect( + shouldFallbackToLoginShell( + shellError(stderr: "fatal: not a git repository (or any of the parent directories)", exitCode: 128) + ) == false + ) + } + + @Test func doesNotFallBackWhenWtReportsNotGitRepo() { + #expect( + shouldFallbackToLoginShell( + shellError(stderr: "wt: not a git repository", exitCode: 1) + ) == false + ) + } + + @Test func doesNotFallBackForNonShellError() { + struct OtherError: Error {} + #expect(shouldFallbackToLoginShell(OtherError()) == false) + } +} -- 2.51.2 From bb871a6ef9f742f5aa05beaf1d9f10179eb3f9a0 Mon Sep 17 00:00:00 2001 From: onevcat Date: Tue, 23 Jun 2026 00:48:55 +0900 Subject: [PATCH 2/2] Update worktree discovery tests for inverted fallback logic Replace the "no fallback for regular failures" test with two tests that match the new behavior: non-git directories skip the fallback (pointless to retry), while environment errors (e.g. permission denied) do trigger it and recover via the login shell. --- .../GitClientWorktreeDiscoveryTests.swift | 50 +++++++++++++++++-- 1 file changed, 45 insertions(+), 5 deletions(-) diff --git a/supacodeTests/GitClientWorktreeDiscoveryTests.swift b/supacodeTests/GitClientWorktreeDiscoveryTests.swift index a7202de5..dffea2bc 100644 --- a/supacodeTests/GitClientWorktreeDiscoveryTests.swift +++ b/supacodeTests/GitClientWorktreeDiscoveryTests.swift @@ -209,7 +209,7 @@ struct GitClientWorktreeDiscoveryTests { } } - @Test func worktreesDoNotFallbackToLoginShellForRegularFailures() async { + @Test func worktreesDoNotFallbackToLoginShellForNonGitDirectory() async { let recorder = GitWorktreeDiscoveryRecorder() let shell = ShellClient( run: { executableURL, arguments, currentDirectoryURL in @@ -221,8 +221,8 @@ struct GitClientWorktreeDiscoveryTests { throw ShellClientError( command: "wt ls --json", stdout: "", - stderr: "permission denied", - exitCode: 1 + stderr: "fatal: not a git repository (or any of the parent directories): .git", + exitCode: 128 ) }, runLoginImpl: { executableURL, arguments, currentDirectoryURL, _ in @@ -231,17 +231,57 @@ struct GitClientWorktreeDiscoveryTests { arguments: arguments, currentDirectoryURL: currentDirectoryURL ) - Issue.record("worktrees should not fallback to runLogin for regular command failures") + Issue.record("worktrees should not fallback to runLogin for non-git directories") return ShellOutput(stdout: "", stderr: "", exitCode: 0) } ) let client = GitClient(shell: shell) await #expect(throws: GitClientError.self) { - _ = try await client.worktrees(for: URL(fileURLWithPath: "/tmp/repo")) + _ = try await client.worktrees(for: URL(fileURLWithPath: "/tmp/not-a-repo")) } #expect(recorder.runInvocations().count == 1) #expect(recorder.loginInvocations().isEmpty) } + + @Test func worktreesFallbackToLoginShellForEnvironmentErrors() async throws { + let recorder = GitWorktreeDiscoveryRecorder() + let shell = ShellClient( + run: { executableURL, arguments, currentDirectoryURL in + recorder.recordRun( + executableURL: executableURL, + arguments: arguments, + currentDirectoryURL: currentDirectoryURL + ) + throw ShellClientError( + command: "wt ls --json", + stdout: "", + stderr: "permission denied", + exitCode: 1 + ) + }, + runLoginImpl: { executableURL, arguments, currentDirectoryURL, _ in + recorder.recordLogin( + executableURL: executableURL, + arguments: arguments, + currentDirectoryURL: currentDirectoryURL + ) + return ShellOutput( + stdout: """ + [{"branch":"main","path":"/tmp/repo","head":"abc","is_bare":false}] + """, + stderr: "", + exitCode: 0 + ) + } + ) + let client = GitClient(shell: shell) + + let worktrees = try await client.worktrees(for: URL(fileURLWithPath: "/tmp/repo")) + + #expect(worktrees.count == 1) + #expect(recorder.runInvocations().count == 1) + #expect(recorder.loginInvocations().count == 1) + } } -- 2.51.2