From 1fd36e42cd7a5c682fd579a2307076cbd533858e Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 20 May 2026 16:48:14 -0700 Subject: [PATCH] Avoid blocking repo/workspace setup on GitHub CLI login (#2456) Co-authored-by: Orca --- src/main/git/repo-username.test.ts | 88 ++++++++++++++++++++++++++++++ src/main/git/repo.ts | 36 ++++++++++-- 2 files changed, 119 insertions(+), 5 deletions(-) create mode 100644 src/main/git/repo-username.test.ts diff --git a/src/main/git/repo-username.test.ts b/src/main/git/repo-username.test.ts new file mode 100644 index 000000000..856daf7c1 --- /dev/null +++ b/src/main/git/repo-username.test.ts @@ -0,0 +1,88 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as ChildProcess from 'child_process' +import type * as RepoModule from './repo' + +const execSyncMock = vi.hoisted(() => vi.fn()) +const execFileSyncMock = vi.hoisted(() => vi.fn()) + +vi.mock('child_process', async () => { + const actual = await vi.importActual('child_process') + return { + ...actual, + execSync: execSyncMock, + execFileSync: execFileSyncMock + } +}) + +describe('getGitUsername', () => { + let gitConfig: Record + let getGitUsername: typeof RepoModule.getGitUsername + + beforeEach(async () => { + vi.resetModules() + execSyncMock.mockReset() + execFileSyncMock.mockReset() + gitConfig = {} + + execFileSyncMock.mockImplementation((_binary: string, args: string[]) => { + if (args[0] === 'config' && args[1] === '--get') { + const value = gitConfig[args[2]] + if (value !== undefined) { + return `${value}\n` + } + throw new Error(`missing config ${args[2]}`) + } + throw new Error(`unexpected git args: ${args.join(' ')}`) + }) + + ;({ getGitUsername } = await import('./repo')) + }) + + it('uses repo-local email before checking GitHub CLI login', () => { + gitConfig['user.email'] = 'demo@example.com' + gitConfig['user.name'] = 'Demo User' + + expect(getGitUsername('/repo')).toBe('demo') + expect(execSyncMock).not.toHaveBeenCalled() + }) + + it('bounds and caches failed GitHub CLI lookup', () => { + execSyncMock.mockImplementation(() => { + throw new Error('gh unavailable') + }) + + expect(getGitUsername('/repo')).toBe('') + expect(getGitUsername('/repo')).toBe('') + + expect(execSyncMock).toHaveBeenCalledTimes(2) + for (const [, options] of execSyncMock.mock.calls) { + expect(options).toMatchObject({ timeout: 2500 }) + } + }) + + it('skips auth status fallback when GitHub CLI API lookup times out', () => { + execSyncMock.mockImplementationOnce(() => { + throw Object.assign(new Error('spawnSync /bin/sh ETIMEDOUT'), { code: 'ETIMEDOUT' }) + }) + + expect(getGitUsername('/repo')).toBe('') + expect(getGitUsername('/repo')).toBe('') + + expect(execSyncMock).toHaveBeenCalledTimes(1) + expect(execSyncMock.mock.calls[0][1]).toMatchObject({ timeout: 2500 }) + }) + + it('uses auth status fallback after fast GitHub CLI API failure', () => { + execSyncMock + .mockImplementationOnce(() => { + throw new Error('gh api unavailable') + }) + .mockImplementationOnce( + () => + 'github.com\n ✓ Logged in to github.com account demo-user\n - Active account: true\n' + ) + + expect(getGitUsername('/repo')).toBe('demo-user') + expect(execSyncMock).toHaveBeenCalledTimes(2) + }) +}) diff --git a/src/main/git/repo.ts b/src/main/git/repo.ts index 9b593f178..dc4c29fdd 100644 --- a/src/main/git/repo.ts +++ b/src/main/git/repo.ts @@ -6,6 +6,8 @@ import hostedGitInfo from 'hosted-git-info' import { gitExecFileSync, gitExecFileAsync } from './runner' import type { BaseRefSearchResult } from '../../shared/types' +const GH_LOGIN_TIMEOUT_MS = 2500 + /** * Ordered probe list used to resolve a repo's default base ref when no * explicit origin/HEAD symbolic-ref is set. `returnAs` is the short-name @@ -116,6 +118,18 @@ function normalizeUsername(value: string): string { let cachedGhLogin: string | undefined +function isGhProbeTimeout(error: unknown): boolean { + if (!error || typeof error !== 'object') { + return false + } + + const err = error as { code?: unknown; message?: unknown } + return ( + err.code === 'ETIMEDOUT' || + (typeof err.message === 'string' && /\bETIMEDOUT\b|timed out/i.test(err.message)) + ) +} + function getGhLogin(): string { if (cachedGhLogin !== undefined) { return cachedGhLogin @@ -124,13 +138,20 @@ function getGhLogin(): string { try { const apiLogin = execSync('gh api user -q .login', { encoding: 'utf-8', - stdio: ['pipe', 'pipe', 'pipe'] + stdio: ['pipe', 'pipe', 'pipe'], + timeout: GH_LOGIN_TIMEOUT_MS }).trim() if (apiLogin) { cachedGhLogin = normalizeUsername(apiLogin) return cachedGhLogin } - } catch { + } catch (err) { + if (isGhProbeTimeout(err)) { + // Why: if `gh api user` timed out, `gh auth status` is likely to hit the + // same stuck keychain/network path. Keep repo creation bounded to one probe. + cachedGhLogin = '' + return '' + } // Fall through to auth status parsing } @@ -140,7 +161,8 @@ function getGhLogin(): string { const output = execSync('gh auth status 2>&1', { encoding: 'utf-8', shell: process.platform === 'win32' ? process.env.ComSpec || 'cmd.exe' : '/bin/bash', - stdio: ['pipe', 'pipe', 'pipe'] + stdio: ['pipe', 'pipe', 'pipe'], + timeout: GH_LOGIN_TIMEOUT_MS }) const activeAccountMatch = output.match( @@ -158,7 +180,9 @@ function getGhLogin(): string { } return login } catch { - // Don't cache empty results on failure — allow retry on next call + // Why: broken tokens/keychains can block the Electron main process. + // Keep the fallback best-effort for this app session. + cachedGhLogin = '' return '' } } @@ -170,8 +194,10 @@ export function getGitUsername(path: string): string { return normalizeUsername( getGitConfigValue(path, 'github.user') || getGitConfigValue(path, 'user.username') || - getGhLogin() || + // Why: GitHub CLI login can touch network/keychain state. A repo-local + // email is already enough for the branch prefix and keeps repo add fast. getGitConfigValue(path, 'user.email').split('@')[0] || + getGhLogin() || getGitConfigValue(path, 'user.name') ) }