diff --git a/mobile/app.json b/mobile/app.json index c1ab487c709..5ff4b7a51fe 100644 --- a/mobile/app.json +++ b/mobile/app.json @@ -2,7 +2,7 @@ "expo": { "name": "Orca", "slug": "orca-mobile", - "version": "0.0.50", + "version": "0.0.51", "orientation": "default", "icon": "./assets/icon.png", "userInterfaceStyle": "automatic", @@ -75,7 +75,7 @@ "allowBackup": false, "permissions": ["RECORD_AUDIO", "MODIFY_AUDIO_SETTINGS"], "package": "com.stably.orca.mobile", - "versionCode": 17, + "versionCode": 18, "googleServicesFile": "./google-services.json" }, "plugins": [ diff --git a/src/main/claude-accounts/claude-account-service-login-process.test.ts b/src/main/claude-accounts/claude-account-service-login-process.test.ts index ec5572f5b3e..8aee047e5fa 100644 --- a/src/main/claude-accounts/claude-account-service-login-process.test.ts +++ b/src/main/claude-accounts/claude-account-service-login-process.test.ts @@ -40,6 +40,9 @@ vi.mock('./keychain', () => ({ writeManagedClaudeKeychainCredentials: vi.fn(async () => {}) })) +// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the doubles below implement every member the add-account login path reaches; this case drives the service only through addAccount. +const asServiceDouble = (double: unknown): T => double as T + describe('ClaudeAccountService credential capture', () => { beforeEach(() => { setPlatform('darwin') @@ -498,6 +501,79 @@ describe('ClaudeAccountService credential capture', () => { } }) + it('supersedes the login a closed Settings pane abandoned instead of queueing behind it', async () => { + setPlatform('linux') + vi.resetModules() + vi.mocked(readActiveClaudeKeychainCredentials).mockResolvedValue(null) + const children: (EventEmitter & { stdin: PassThrough; kill: ReturnType })[] = [] + // No pid: the POSIX teardown signals -pid as a process group, which must + // never reach a real group from a test double. + const spawnMock = vi.fn(() => { + const child = Object.assign(new EventEmitter(), { + stdin: new PassThrough(), + stdout: new PassThrough(), + stderr: new PassThrough(), + kill: vi.fn() + }) + children.push(child) + return child + }) + vi.doMock('node:child_process', () => ({ spawn: spawnMock })) + + try { + const { ClaudeAccountService } = await import('./service') + let settings = { + claudeManagedAccounts: [], + activeClaudeManagedAccountId: null, + activeClaudeManagedAccountIdsByRuntime: { host: null, wsl: {} } + } + const store = { + getSettings: vi.fn(() => settings), + updateSettings: vi.fn((updates: Partial) => { + settings = { ...settings, ...updates } + return settings + }) + } + const runtimeAuth = { + clearLastWrittenCredentialsJson: vi.fn(), + syncForCurrentSelection: vi.fn(async () => {}), + forceMaterializeCurrentSelectionForRollback: vi.fn(async () => {}) + } + const rateLimits = { + evictInactiveClaudeCache: vi.fn(), + refreshForClaudeAccountChange: vi.fn() + } + const service = new ClaudeAccountService( + asServiceDouble(store), + asServiceDouble(rateLimits), + asServiceDouble(runtimeAuth) + ) + + const abandoned = service.addAccount({ runtime: 'host' }) + const abandonedRejection = expect(abandoned).rejects.toThrow('Claude sign-in was cancelled.') + await vi.waitFor(() => { + expect(children.length).toBe(1) + }) + + // The user reopens Settings and clicks Add Account again. + const retry = service.addAccount({ runtime: 'host' }) + const retryRejection = expect(retry).rejects.toThrow() + + await abandonedRejection + expect(children[0].kill).toHaveBeenCalled() + // Why: the point of the fix — the second login starts now, not after the + // abandoned one's whole sign-in deadline elapses. + await vi.waitFor(() => { + expect(children.length).toBe(2) + }) + + children[1].emit('close', 1) + await retryRejection + } finally { + vi.doUnmock('node:child_process') + } + }) + it('uses taskkill to cancel the Windows Claude login process tree', async () => { setPlatform('win32') vi.resetModules() diff --git a/src/main/claude-accounts/service.ts b/src/main/claude-accounts/service.ts index 8f1077ba1a5..d1c79eaf17d 100644 --- a/src/main/claude-accounts/service.ts +++ b/src/main/claude-accounts/service.ts @@ -76,6 +76,7 @@ export class ClaudeAccountService { } async addAccount(target?: ClaudeAccountAddTarget): Promise { + this.supersedePendingLogin() return this.serializeMutation(() => this.registration.add(target)) } @@ -87,14 +88,17 @@ export class ClaudeAccountService { } async reauthenticateAccount(accountId: string): Promise { + this.supersedePendingLogin() return this.serializeMutation(() => this.registration.reauthenticate(accountId)) } async removeAccount(accountId: string): Promise { + this.supersedePendingLogin() return this.serializeMutation(() => this.selection.remove(accountId)) } async selectAccount(accountId: string | null): Promise { + this.supersedePendingLogin() return this.serializeMutation(() => this.selection.select(accountId)) } @@ -102,6 +106,7 @@ export class ClaudeAccountService { accountId: string | null, target?: ClaudeAccountSelectionTarget ): Promise { + this.supersedePendingLogin() return this.serializeMutation(() => this.selection.select(accountId, target)) } @@ -109,6 +114,17 @@ export class ClaudeAccountService { return this.cancelPendingClaudeLogin?.() ?? false } + // Why before the queue, not inside it: the abandoned login owns the queue slot + // every later account action waits for. Called from the four the user drives, + // never from serializeMutation, which background work also uses. + private supersedePendingLogin(): void { + if (this.cancelPendingLogin()) { + console.info( + '[claude-accounts] Cancelled a pending Claude login superseded by a new request.' + ) + } + } + getRuntimeConfigDir(target?: ClaudeAccountSelectionTarget): string { return this.runtimeAuth.getRuntimeConfigDir(target) } diff --git a/src/main/codex-accounts/codex-login-auth-snapshot.ts b/src/main/codex-accounts/codex-login-auth-snapshot.ts new file mode 100644 index 00000000000..5347281f2e3 --- /dev/null +++ b/src/main/codex-accounts/codex-login-auth-snapshot.ts @@ -0,0 +1,27 @@ +import { readFileSync } from 'node:fs' + +/** The bytes of a managed home's auth.json, null when absent, undefined when unreadable. */ +export function readLoginAuthSnapshot(authJsonPath: string): string | null | undefined { + try { + return readFileSync(authJsonPath, 'utf-8') + } catch (error) { + const code = (error as NodeJS.ErrnoException).code + if (code === 'ENOENT' || code === 'ENOTDIR') { + return null + } + // Why: codex can atomically replace auth.json while the poll runs; a later + // poll will observe the stable credential. An unreadable initial file must + // disable the shortcut rather than look like a fresh login. + return undefined + } +} + +/** Whether a login has written credentials of its own since the snapshot was taken. */ +export function loginAuthChanged( + initial: string | null | undefined, + current: string | null | undefined +): boolean { + // Why: metadata-only touches can happen before OAuth finishes. Requiring new + // credential bytes prevents reauthentication from being killed prematurely. + return initial !== undefined && current !== undefined && current !== null && current !== initial +} diff --git a/src/main/codex-accounts/codex-login-auth-url.test.ts b/src/main/codex-accounts/codex-login-auth-url.test.ts new file mode 100644 index 00000000000..9dddaef048c --- /dev/null +++ b/src/main/codex-accounts/codex-login-auth-url.test.ts @@ -0,0 +1,51 @@ +import { describe, expect, it } from 'vitest' +import { parseCodexLoginAuthUrl } from './codex-login-auth-url' + +const AUTH_URL = 'https://auth.openai.com/oauth/authorize?client_id=orca&state=abc123' + +describe('parseCodexLoginAuthUrl', () => { + it('reads the link codex prints under its browser notice', () => { + expect( + parseCodexLoginAuthUrl( + `Starting local login server on http://localhost:1455.\nIf your browser did not open, navigate to this URL to authenticate:\n\n${AUTH_URL}\n` + ) + ).toBe(AUTH_URL) + }) + + it('ignores the local server line, which authenticates nothing', () => { + expect(parseCodexLoginAuthUrl('Starting local login server on http://localhost:1455.\n')).toBe( + null + ) + }) + + it('waits for a chunk boundary rather than publishing a truncated link', () => { + const truncated = `If your browser did not open, navigate to this URL to authenticate:\n\n${AUTH_URL.slice(0, 40)}` + expect(parseCodexLoginAuthUrl(truncated)).toBe(null) + expect(parseCodexLoginAuthUrl(`${truncated}${AUTH_URL.slice(40)}\n`)).toBe(AUTH_URL) + }) + + it('survives the escape sequences a coloured CLI writes around the link', () => { + expect( + parseCodexLoginAuthUrl( + `navigate to this URL to authenticate:\n\n${AUTH_URL}\n` + ) + ).toBe(AUTH_URL) + }) + + it('drops sentence punctuation that follows the link', () => { + expect( + parseCodexLoginAuthUrl(`navigate to this URL to authenticate: ${AUTH_URL}. Then return here.`) + ).toBe(AUTH_URL) + }) + + it('reports nothing for output without a link yet', () => { + expect(parseCodexLoginAuthUrl('')).toBe(null) + expect(parseCodexLoginAuthUrl('Codex login failed: network unreachable\n')).toBe(null) + }) + + it('offers no link at all rather than an unrelated one when the notice is missing', () => { + expect( + parseCodexLoginAuthUrl('A new version of codex is available: https://openai.com/codex\n') + ).toBe(null) + }) +}) diff --git a/src/main/codex-accounts/codex-login-auth-url.ts b/src/main/codex-accounts/codex-login-auth-url.ts new file mode 100644 index 00000000000..024f0b6e73e --- /dev/null +++ b/src/main/codex-accounts/codex-login-auth-url.ts @@ -0,0 +1,32 @@ +import { stripAnsiEscapeSequences } from '../../shared/ansi-escape-sequences' + +const AUTH_URL_MARKER = 'navigate to this url to authenticate:' + +/** + * The browser sign-in link `codex login` prints, or null while its output has + * not carried a complete one yet. + */ +export function parseCodexLoginAuthUrl(output: string): string | null { + const plain = stripAnsiEscapeSequences(output) + const markerIndex = plain.toLowerCase().indexOf(AUTH_URL_MARKER) + // Why the marker is required: without it the first https link codex happens to + // print — an update notice, a docs link — would be offered as the sign-in link. + // If codex rewords the line, no link beats the wrong one. + if (markerIndex === -1) { + return null + } + const searchable = plain.slice(markerIndex + AUTH_URL_MARKER.length) + // Why: the trailing whitespace is required, not incidental. Output arrives in + // chunks, and a flush that ends mid-token would otherwise publish a truncated + // link that authenticates nothing. + const match = /(https:\/\/\S+?)[.,;:)\]]*\s/.exec(searchable) + if (!match) { + return null + } + try { + // Only the shape is trusted here; the pattern already fixed the scheme. + return new URL(match[1]).toString() + } catch { + return null + } +} diff --git a/src/main/codex-accounts/codex-login-session.ts b/src/main/codex-accounts/codex-login-session.ts index 6df83c05e18..3595351ef37 100644 --- a/src/main/codex-accounts/codex-login-session.ts +++ b/src/main/codex-accounts/codex-login-session.ts @@ -1,19 +1,24 @@ -import { readFileSync } from 'node:fs' import { join } from 'node:path' import type { WindowsHostInteractiveLoginSpawn } from '../../shared/windows-interactive-login-spawn' import { buildWindowsHostInteractiveLoginSpawn } from '../../shared/windows-interactive-login-spawn' import { withCliRuntimeOnPath } from '../../shared/node-cli-command-resolution' +import { CODEX_LOGIN_CANCELLED_MESSAGE } from '../../shared/codex-auth-errors' import { parseWslUncPath } from '../../shared/wsl-paths' import { resolveCodexCommand } from '../codex-cli/command' import { getSpawnArgsForWindows } from '../win32-utils' import { runWslProcess } from '../wsl/wsl-runner' +import { parseCodexLoginAuthUrl } from './codex-login-auth-url' +import { loginAuthChanged, readLoginAuthSnapshot } from './codex-login-auth-snapshot' import { buildWslCodexAvailabilityScript, buildWslCodexLoginArgs, WSL_CODEX_AVAILABILITY_TIMEOUT_MS } from './wsl-codex-command' -const LOGIN_TIMEOUT_MS = 120_000 +// Why: matches Claude's window. Signing in through a copied link — a second +// browser, a password manager, an incognito window — routinely outlasts two +// minutes, and the old 120s deadline failed those users mid-flow. +const LOGIN_TIMEOUT_MS = 180_000 const MAX_LOGIN_OUTPUT_CHARS = 4_000 const WINDOWS_LOGIN_AUTH_POLL_INTERVAL_MS = 500 const WINDOWS_LOGIN_POST_AUTH_EXIT_GRACE_MS = 5_000 @@ -50,35 +55,47 @@ type CodexLoginSessionDependencies = { child: CodexLoginChild, interactiveLogin?: WindowsHostInteractiveLoginSpawn | null ) => void + /** Registers the handle that abandons this login; the caller clears it. */ + setCancel: (cancel: () => boolean) => void + /** The browser link codex printed, published as soon as it is complete. */ + onAuthUrl: (url: string) => void } -function readLoginAuthSnapshot(authJsonPath: string): string | null | undefined { - try { - return readFileSync(authJsonPath, 'utf-8') - } catch (error) { - const code = (error as NodeJS.ErrnoException).code - if (code === 'ENOENT' || code === 'ENOTDIR') { - return null - } - // Why: codex can atomically replace auth.json while the poll runs; a later - // poll will observe the stable credential. An unreadable initial file must - // disable the shortcut rather than look like a fresh login. - return undefined - } -} - -function loginAuthChanged( - initial: string | null | undefined, - current: string | null | undefined -): boolean { - // Why: metadata-only touches can happen before OAuth finishes. Requiring new - // credential bytes prevents reauthentication from being killed prematurely. - return initial !== undefined && current !== undefined && current !== null && current !== initial +type LoginCancellation = { + isCancelled: () => boolean + setSpawnedCancel: (cancel: () => boolean) => void } export async function runCodexLoginSession( managedHomePath: string, dependencies: CodexLoginSessionDependencies +): Promise { + let cancelSpawnedLogin: (() => boolean) | null = null + let cancelled = false + dependencies.setCancel(() => { + // Why: only an accepted cancel latches. A spawned login that refuses — + // because it already authenticated — must stay cancellable, or the Cancel + // button and the next add both go dead for the rest of the deadline. + if (cancelled || cancelSpawnedLogin?.() === false) { + return false + } + // A cancel before the spawn has no tree to kill; the pre-spawn probe reads + // this flag instead of opening a browser nobody is waiting for. + cancelled = true + return true + }) + await runCodexLoginProcess(managedHomePath, dependencies, { + isCancelled: () => cancelled, + setSpawnedCancel: (cancel) => { + cancelSpawnedLogin = cancel + } + }) +} + +async function runCodexLoginProcess( + managedHomePath: string, + dependencies: CodexLoginSessionDependencies, + cancellation: LoginCancellation ): Promise { const wslInfo = parseWslUncPath(managedHomePath) if (wslInfo) { @@ -87,9 +104,15 @@ export async function runCodexLoginSession( // Why: reauthentication starts with an existing auth.json. Only new auth // bytes prove this login completed; existence alone would kill the // Windows OAuth flow five seconds after it opened. + // WSL keeps its baseline unread — the UNC round trip belongs nowhere in the + // pre-spawn path — so there is nothing to compare a WSL home against. const initialAuthSnapshot = wslInfo ? null : readLoginAuthSnapshot(join(managedHomePath, 'auth.json')) + const hasAuthBaseline = !wslInfo + if (cancellation.isCancelled()) { + throw new Error(CODEX_LOGIN_CANCELLED_MESSAGE) + } await new Promise((resolvePromise, rejectPromise) => { const spawnConfig = wslInfo @@ -119,6 +142,22 @@ export async function runCodexLoginSession( } } + // Why its own buffer: a stderr chunk interleaved between two halves of the + // link would end the match early, and the published link never changes. + let stdoutText = '' + let publishedAuthUrl = false + const publishAuthUrl = (chunk: Buffer): void => { + if (publishedAuthUrl) { + return + } + stdoutText = `${stdoutText}${chunk.toString()}`.slice(-MAX_LOGIN_OUTPUT_CHARS) + const authUrl = parseCodexLoginAuthUrl(stdoutText) + if (authUrl) { + publishedAuthUrl = true + dependencies.onAuthUrl(authUrl) + } + } + let timeout: ReturnType | null = null let authWatchInterval: ReturnType | null = null let postAuthExitTimeout: ReturnType | null = null @@ -138,6 +177,7 @@ export async function runCodexLoginSession( postAuthExitTimeout = null } child.stdout?.off('data', appendOutput) + child.stdout?.off('data', publishAuthUrl) child.stderr?.off('data', appendOutput) child.off('error', onError) child.off('close', onClose) @@ -153,6 +193,23 @@ export async function runCodexLoginSession( callback() } + cancellation.setSpawnedCancel(() => { + // Why: once codex has written new credential bytes the sign-in already + // succeeded, and rejecting here would send the caller's rollback at the + // home it just authenticated. Nothing left to cancel — let it settle. + // Without a baseline (WSL) an existing auth.json says nothing about this + // login, so refusing on it would make a WSL reauth uncancellable. + const alreadyAuthenticated = + hasAuthBaseline && + loginAuthChanged(initialAuthSnapshot, readLoginAuthSnapshot(authJsonPath)) + if (settled || alreadyAuthenticated) { + return false + } + dependencies.killProcessTree(child, spawnConfig.interactiveLogin) + settle(() => rejectPromise(new Error(CODEX_LOGIN_CANCELLED_MESSAGE))) + return true + }) + const timeoutError = new Error('Codex sign-in took too long to finish. Please try again.') timeout = setTimeout(() => { dependencies.killProcessTree(child, spawnConfig.interactiveLogin) @@ -221,6 +278,7 @@ export async function runCodexLoginSession( } child.stdout?.on('data', appendOutput) + child.stdout?.on('data', publishAuthUrl) child.stderr?.on('data', appendOutput) child.on('error', onError) child.on('close', onClose) diff --git a/src/main/codex-accounts/service-abandoned-login-supersede.test.ts b/src/main/codex-accounts/service-abandoned-login-supersede.test.ts new file mode 100644 index 00000000000..11743258fe5 --- /dev/null +++ b/src/main/codex-accounts/service-abandoned-login-supersede.test.ts @@ -0,0 +1,192 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { EventEmitter } from 'node:events' +import { existsSync, writeFileSync } from 'node:fs' +import { join } from 'node:path' +import { PassThrough } from 'node:stream' +import { + createCodexAuthJson, + createRateLimits, + createRuntimeHome, + createSettings, + createStore, + registerCodexAccountsTestHomes, + testState +} from './service-test-harness' + +vi.mock('electron', () => ({ + app: { + getPath: () => testState.userDataDir + } +})) + +vi.mock('node:os', async () => { + const actual = await vi.importActual('node:os') // eslint-disable-line @typescript-eslint/consistent-type-imports -- vi.importActual requires inline import() + return { + ...actual, + homedir: () => testState.fakeHomeDir + } +}) + +// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the harness doubles implement every member the add-account login path reaches; this test drives the service only through addAccount, cancelPendingLogin and the login URL subscription. +const asServiceDouble = (double: unknown): T => double as T + +type StubLoginChild = EventEmitter & { + stdout: PassThrough + stderr: PassThrough + kill: () => boolean + exitCode: number | null + signalCode: string | null +} + +function createStubLoginChild(): StubLoginChild { + const child: StubLoginChild = Object.assign(new EventEmitter(), { + stdout: new PassThrough(), + stderr: new PassThrough(), + exitCode: null, + signalCode: null, + kill: () => true + }) + // The real codex CLI forwards SIGTERM to its native child and exits. + child.kill = vi.fn(() => { + child.exitCode = 143 + return true + }) + return child +} + +/** A service whose `codex login` never finishes on its own. */ +async function createServiceWithHangingLogin(): Promise<{ + service: { + addAccount: () => Promise<{ accounts: { email: string }[] }> + selectAccount: (accountId: string | null) => Promise + cancelPendingLogin: () => boolean + getPendingLoginUrl: () => string | null + onPendingLoginUrlChanged: (listener: (url: string | null) => void) => void + } + children: StubLoginChild[] + /** The `CODEX_HOME` each login was spawned against. */ + loginHomes: string[] +}> { + vi.resetModules() + const children: StubLoginChild[] = [] + const loginHomes: string[] = [] + vi.doMock('node:child_process', () => ({ + execFileSync: vi.fn(), + spawn: vi.fn((_command: string, _args: string[], options: { env: NodeJS.ProcessEnv }) => { + loginHomes.push(options.env.CODEX_HOME ?? '') + const child = createStubLoginChild() + children.push(child) + return child + }) + })) + vi.doMock('../codex-cli/command', () => ({ + resolveCodexCommand: () => 'codex' + })) + const { CodexAccountService } = await import('./service') + const service = new CodexAccountService( + asServiceDouble(createStore(createSettings())), + asServiceDouble(createRateLimits()), + asServiceDouble(createRuntimeHome()) + ) + return { service, children, loginHomes } +} + +describe('CodexAccountService abandoned login', () => { + registerCodexAccountsTestHomes() + + afterEach(() => { + vi.doUnmock('node:child_process') + vi.doUnmock('../codex-cli/command') + }) + + it('supersedes the login a closed Settings pane abandoned instead of queueing behind it', async () => { + const { service, children } = await createServiceWithHangingLogin() + const abandoned = service.addAccount() + const abandonedRejection = expect(abandoned).rejects.toThrow('Codex sign-in was cancelled.') + await vi.waitUntil(() => children.length === 1) + + // The user reopens Settings and clicks Add Account again. + const retry = service.addAccount() + const retryRejection = expect(retry).rejects.toThrow() + + await abandonedRejection + expect(children[0].kill).toHaveBeenCalledTimes(1) + // Why: the point of the fix — the second login starts now, not after the + // abandoned one's whole sign-in deadline elapses. + await vi.waitUntil(() => children.length === 2) + expect(children[1].kill).not.toHaveBeenCalled() + + service.cancelPendingLogin() + await retryRejection + }) + + it('frees the queue for a plain account switch too, not only for another add', async () => { + const { service, children } = await createServiceWithHangingLogin() + const abandoned = service.addAccount() + const abandonedRejection = expect(abandoned).rejects.toThrow('Codex sign-in was cancelled.') + await vi.waitUntil(() => children.length === 1) + + // Why: switching to the system default is the commonest thing a user does + // after giving up on a sign-in, and it shares the add's mutation queue. + await service.selectAccount(null) + await abandonedRejection + expect(children[0].kill).toHaveBeenCalled() + }) + + it('refuses to cancel a sign-in that already wrote credentials, and keeps the account', async () => { + const { service, children, loginHomes } = await createServiceWithHangingLogin() + const pending = service.addAccount() + await vi.waitUntil(() => children.length === 1) + + // The browser half of the OAuth flow finishes while the CLI lingers. + writeFileSync( + join(loginHomes[0], 'auth.json'), + createCodexAuthJson('user@example.com', 'provider-account-1', 'refresh-token'), + 'utf-8' + ) + + // Why: cancelling here would send the rollback at a home that just + // authenticated. There is nothing left to cancel. + expect(service.cancelPendingLogin()).toBe(false) + expect(children[0].kill).not.toHaveBeenCalled() + + children[0].emit('close', 0) + const accounts = await pending + expect(accounts.accounts.map((account) => account.email)).toEqual(['user@example.com']) + expect(existsSync(join(loginHomes[0], 'auth.json'))).toBe(true) + }) + + it('reports whether a pending login was there to cancel', async () => { + const { service, children } = await createServiceWithHangingLogin() + const pending = service.addAccount() + const rejection = expect(pending).rejects.toThrow('Codex sign-in was cancelled.') + await vi.waitUntil(() => children.length === 1) + + expect(service.cancelPendingLogin()).toBe(true) + await rejection + expect(service.cancelPendingLogin()).toBe(false) + }) + + it('publishes the sign-in link codex prints and drops it when the login ends', async () => { + const { service, children } = await createServiceWithHangingLogin() + const published: (string | null)[] = [] + service.onPendingLoginUrlChanged((url) => published.push(url)) + + const pending = service.addAccount() + const rejection = expect(pending).rejects.toThrow('Codex sign-in was cancelled.') + await vi.waitUntil(() => children.length === 1) + + const authUrl = 'https://auth.openai.com/oauth/authorize?client_id=orca&state=abc' + children[0].stdout.write( + `Starting local login server on http://localhost:1455.\nIf your browser did not open, navigate to this URL to authenticate:\n\n${authUrl}\n` + ) + await vi.waitUntil(() => service.getPendingLoginUrl() === authUrl) + expect(published).toEqual([authUrl]) + + service.cancelPendingLogin() + await rejection + // Why: the link dies with the login server it points back at. + expect(service.getPendingLoginUrl()).toBeNull() + expect(published).toEqual([authUrl, null]) + }) +}) diff --git a/src/main/codex-accounts/service-login-process-teardown.test.ts b/src/main/codex-accounts/service-login-process-teardown.test.ts index a49877a196d..42753d4d59b 100644 --- a/src/main/codex-accounts/service-login-process-teardown.test.ts +++ b/src/main/codex-accounts/service-login-process-teardown.test.ts @@ -70,7 +70,7 @@ describe('CodexAccountService config sync', () => { 'Codex sign-in took too long to finish.' ) - await vi.advanceTimersByTimeAsync(120_000) + await vi.advanceTimersByTimeAsync(180_000) await rejection expect(child.kill).toHaveBeenCalledTimes(1) @@ -89,7 +89,10 @@ describe('CodexAccountService config sync', () => { vi.resetModules() vi.useFakeTimers() const originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform')! - Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }) + Object.defineProperty(process, 'platform', { + value: 'win32', + configurable: true + }) const child = new EventEmitter() as EventEmitter & { stdout: PassThrough stderr: PassThrough @@ -162,7 +165,10 @@ describe('CodexAccountService config sync', () => { vi.resetModules() vi.useFakeTimers() const originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform')! - Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }) + Object.defineProperty(process, 'platform', { + value: 'win32', + configurable: true + }) const child = new EventEmitter() as EventEmitter & { stdout: PassThrough stderr: PassThrough @@ -182,7 +188,9 @@ describe('CodexAccountService config sync', () => { execFileSync: execFileSyncMock, spawn: vi.fn(() => child) })) - vi.doMock('../codex-cli/command', () => ({ resolveCodexCommand: () => 'codex' })) + vi.doMock('../codex-cli/command', () => ({ + resolveCodexCommand: () => 'codex' + })) const authPath = join(testState.fakeHomeDir, 'auth.json') writeFileSync( authPath, @@ -198,7 +206,9 @@ describe('CodexAccountService config sync', () => { createRuntimeHome() as never ) const loginPromise = ( - service as unknown as { runCodexLogin(managedHomePath: string): Promise } + service as unknown as { + runCodexLogin(managedHomePath: string): Promise + } ).runCodexLogin(testState.fakeHomeDir) await vi.advanceTimersByTimeAsync(6_000) diff --git a/src/main/codex-accounts/service.ts b/src/main/codex-accounts/service.ts index 6f5a4a9f0b7..67f8ebb218a 100644 --- a/src/main/codex-accounts/service.ts +++ b/src/main/codex-accounts/service.ts @@ -80,6 +80,9 @@ function killLoginProcessTree( export class CodexAccountService { // Why: serialize the read-modify-write of settings; overlapping calls (e.g. double-click Add) would lose updates. private mutationQueue: Promise = Promise.resolve() + private cancelPendingCodexLogin: (() => boolean) | null = null + private pendingLoginUrl: string | null = null + private readonly pendingLoginUrlListeners = new Set<(url: string | null) => void>() private readonly identity: CodexAccountIdentity private readonly configMirror: CodexConfigMirror private readonly managedHomePaths: CodexManagedHomePath @@ -161,9 +164,46 @@ export class CodexAccountService { } async addAccount(target?: CodexAccountAddTarget): Promise { + this.supersedePendingLogin() return this.serializeMutation(() => this.registration.add(target)) } + /** Abandons the login waiting on a browser, if any. True when one was stopped. */ + cancelPendingLogin(): boolean { + return this.cancelPendingCodexLogin?.() ?? false + } + + /** The sign-in link of the login waiting on a browser, for a late-joining renderer. */ + getPendingLoginUrl(): string | null { + return this.pendingLoginUrl + } + + /** Registration lasts the process's lifetime; there is no teardown to hand back. */ + onPendingLoginUrlChanged(listener: (url: string | null) => void): void { + this.pendingLoginUrlListeners.add(listener) + } + + private setPendingLoginUrl(url: string | null): void { + // Why the guard: every login that ends before printing a link clears an + // already-empty value, and each change reaches every window. + if (this.pendingLoginUrl === url) { + return + } + this.pendingLoginUrl = url + for (const listener of this.pendingLoginUrlListeners) { + listener(url) + } + } + + // Why before the queue, not inside it: the abandoned login owns the queue slot + // every later account action waits for. Called from the four the user drives, + // never from serializeMutation, which background reset-credit work also uses. + private supersedePendingLogin(): void { + if (this.cancelPendingLogin()) { + console.info('[codex-accounts] Cancelled a pending Codex login superseded by a new request.') + } + } + /** * Registers a managed Codex account from an already-authenticated `CODEX_HOME` * instead of driving `codex login` here. Lets the `orca account add --agent codex` @@ -181,14 +221,17 @@ export class CodexAccountService { accountId: string, options?: CodexAccountReauthenticateOptions ): Promise { + this.supersedePendingLogin() return this.serializeMutation(() => this.registration.reauthenticate(accountId, options)) } async removeAccount(accountId: string): Promise { + this.supersedePendingLogin() return this.serializeMutation(() => this.selection.remove(accountId)) } async selectAccount(accountId: string | null): Promise { + this.supersedePendingLogin() return this.serializeMutation(() => this.selection.select(accountId)) } @@ -196,6 +239,7 @@ export class CodexAccountService { accountId: string | null, target?: CodexAccountSelectionTarget ): Promise { + this.supersedePendingLogin() return this.serializeMutation(() => this.selection.select(accountId, target)) } @@ -234,16 +278,27 @@ export class CodexAccountService { } private async runCodexLogin(managedHomePath: string): Promise { - await runCodexLoginSession(managedHomePath, { - wslCommand: 'wsl.exe', - spawn: ({ command, args, env, stdio }) => - spawn(command, args, { - stdio, - // Why: hide the outer wrapper only. A dedicated login console stays visible. - windowsHide: true, - env - }), - killProcessTree: killLoginProcessTree - }) + try { + await runCodexLoginSession(managedHomePath, { + wslCommand: 'wsl.exe', + spawn: ({ command, args, env, stdio }) => + spawn(command, args, { + stdio, + // Why: hide the outer wrapper only. A dedicated login console stays visible. + windowsHide: true, + env + }), + killProcessTree: killLoginProcessTree, + setCancel: (cancel) => { + this.cancelPendingCodexLogin = cancel + }, + onAuthUrl: (url) => this.setPendingLoginUrl(url) + }) + } finally { + // Why: both die with the login server — no surface may keep offering a + // link nothing is listening on, or a cancel with nothing to cancel. + this.cancelPendingCodexLogin = null + this.setPendingLoginUrl(null) + } } } diff --git a/src/main/daemon/daemon-pty-adapter-protocol-compatibility.test.ts b/src/main/daemon/daemon-pty-adapter-protocol-compatibility.test.ts index 8132f2de4e2..3fc255b60ec 100644 --- a/src/main/daemon/daemon-pty-adapter-protocol-compatibility.test.ts +++ b/src/main/daemon/daemon-pty-adapter-protocol-compatibility.test.ts @@ -243,16 +243,29 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => { }) describe('background stream thinning compatibility', () => { - it('reports authoritative snapshot support only for the corrected serializer protocol', () => { + it('reports authoritative snapshot support only for the corrected serializer protocol', async () => { + const { id } = await adapter.spawn({ cols: 80, rows: 24 }) const legacy = new DaemonPtyAdapter({ socketPath, tokenPath, protocolVersion: 31 }) try { - expect(legacy.canProvideAuthoritativeBufferSnapshot('legacy-session')).toBe(false) - expect(adapter.canProvideAuthoritativeBufferSnapshot('current-session')).toBe(true) + expect(legacy.canProvideAuthoritativeBufferSnapshot(id)).toBe(false) + expect(adapter.canProvideAuthoritativeBufferSnapshot(id)).toBe(true) } finally { legacy.dispose() } }) + // Why this matters beyond tidiness: getProviderForPty falls back to the local provider for + // any id it cannot place, so these ids reach this adapter for real. Answered from the + // protocol flag alone they came back `true`, which the renderer caches as a definitive + // per-pty licence to unmount a pane whose bytes this daemon never held. + it('refuses an authoritative snapshot claim for a session it does not own', async () => { + const { id } = await adapter.spawn({ cols: 80, rows: 24 }) + + expect(adapter.canProvideAuthoritativeBufferSnapshot(id)).toBe(true) + expect(adapter.canProvideAuthoritativeBufferSnapshot('remote:env-1:pty-1')).toBe(false) + expect(adapter.canProvideAuthoritativeBufferSnapshot('never-spawned-session')).toBe(false) + }) + it('reports background state on the authoritative-snapshot protocol', () => { const notifySpy = vi.spyOn(DaemonClient.prototype, 'notify') try { diff --git a/src/main/daemon/daemon-pty-runtime-state.ts b/src/main/daemon/daemon-pty-runtime-state.ts index e471f698748..38480af1489 100644 --- a/src/main/daemon/daemon-pty-runtime-state.ts +++ b/src/main/daemon/daemon-pty-runtime-state.ts @@ -237,8 +237,12 @@ export abstract class DaemonPtyRuntimeState { return this.protocolVersion >= GIT_CREDENTIAL_GUARD_HOST_PROTOCOL_VERSION } - canProvideAuthoritativeBufferSnapshot(_id: string): boolean { - return this.supportsAuthoritativeBufferSnapshots + // Why the id is read rather than ignored: the contract promises a fact about THIS pty, and + // getProviderForPty falls back to the local provider for any id it cannot place. A + // remote-runtime id therefore reaches this adapter, and answering from the protocol flag + // alone returned `true` for a session this daemon has never owned. + canProvideAuthoritativeBufferSnapshot(id: string): boolean { + return this.supportsAuthoritativeBufferSnapshots && this.activeSessionIds.has(id) } protected get canDelegateBackgroundToDaemon(): boolean { diff --git a/src/main/ipc/codex-accounts.ts b/src/main/ipc/codex-accounts.ts index e89e8bca4be..4d828ecf82b 100644 --- a/src/main/ipc/codex-accounts.ts +++ b/src/main/ipc/codex-accounts.ts @@ -1,6 +1,7 @@ import { ipcMain } from 'electron' import type { CodexAccountAddTarget, CodexAccountService } from '../codex-accounts/service' import type { CodexAccountSelectionTarget } from '../codex-accounts/runtime-selection' +import { broadcastCodexPendingLoginUrl } from './codex-pending-login-url-broadcast' import { listRecordedCodexPaneLanes } from '../codex/codex-pane-account-registry' import { forgetStaleCodexPanes, listStaleCodexPanes } from '../codex/codex-stale-pane-accounts' import type { GlobalSettings } from '../../shared/global-settings-types' @@ -38,6 +39,11 @@ export function registerCodexAccountHandlers( ipcMain.handle('codexAccounts:add', (_event, args?: CodexAccountAddTarget) => codexAccounts.addAccount(args) ) + ipcMain.handle('codexAccounts:cancelPendingLogin', () => codexAccounts.cancelPendingLogin()) + ipcMain.handle('codexAccounts:pendingLoginUrl', () => codexAccounts.getPendingLoginUrl()) + // Why: Settings can open after the login already printed its link, so the + // renderer reads the current value on mount and this only carries changes. + codexAccounts.onPendingLoginUrlChanged(broadcastCodexPendingLoginUrl) ipcMain.handle( 'codexAccounts:reauthenticate', (_event, args: { accountId: string; activateIfSelectionWasEmpty?: boolean }) => diff --git a/src/main/ipc/codex-pending-login-url-broadcast.ts b/src/main/ipc/codex-pending-login-url-broadcast.ts new file mode 100644 index 00000000000..5599eb5f968 --- /dev/null +++ b/src/main/ipc/codex-pending-login-url-broadcast.ts @@ -0,0 +1,15 @@ +import { BrowserWindow } from 'electron' +import { CODEX_PENDING_LOGIN_URL_CHANGED_CHANNEL } from '../../shared/codex-auth-errors' + +export function broadcastCodexPendingLoginUrl(url: string | null): void { + for (const window of BrowserWindow.getAllWindows()) { + if (window.isDestroyed()) { + continue + } + try { + window.webContents.send(CODEX_PENDING_LOGIN_URL_CHANGED_CHANNEL, url) + } catch { + // A renderer can disappear between isDestroyed() and send(). + } + } +} diff --git a/src/preload/api/agent-account-api.ts b/src/preload/api/agent-account-api.ts index ff98244299d..f041e41f8b7 100644 --- a/src/preload/api/agent-account-api.ts +++ b/src/preload/api/agent-account-api.ts @@ -11,6 +11,10 @@ export type CodexAccountsApi = { runtime?: 'host' | 'wsl' wslDistro?: string | null }) => Promise + cancelPendingLogin: () => Promise + /** Sign-in link of the login waiting on a browser, or null when none is. */ + getPendingLoginUrl: () => Promise + onPendingLoginUrlChanged: (callback: (url: string | null) => void) => () => void reauthenticate: (args: { accountId: string /** Local-only: activate the re-authed account when its runtime lane had no selection. */ diff --git a/src/preload/api/codex-accounts-bridge.ts b/src/preload/api/codex-accounts-bridge.ts index d085de45855..d5ca98cdf76 100644 --- a/src/preload/api/codex-accounts-bridge.ts +++ b/src/preload/api/codex-accounts-bridge.ts @@ -1,10 +1,20 @@ import { ipcRenderer } from 'electron' +import { CODEX_PENDING_LOGIN_URL_CHANGED_CHANNEL } from '../../shared/codex-auth-errors' import type { PreloadApi } from '../api-types' export const codexAccountsApi = { list: () => ipcRenderer.invoke('codexAccounts:list'), add: (args?: { runtime?: 'host' | 'wsl'; wslDistro?: string | null }) => ipcRenderer.invoke('codexAccounts:add', args), + cancelPendingLogin: (): Promise => + ipcRenderer.invoke('codexAccounts:cancelPendingLogin'), + getPendingLoginUrl: (): Promise => + ipcRenderer.invoke('codexAccounts:pendingLoginUrl'), + onPendingLoginUrlChanged: (callback: (url: string | null) => void): (() => void) => { + const listener = (_event: Electron.IpcRendererEvent, url: string | null): void => callback(url) + ipcRenderer.on(CODEX_PENDING_LOGIN_URL_CHANGED_CHANNEL, listener) + return () => ipcRenderer.removeListener(CODEX_PENDING_LOGIN_URL_CHANGED_CHANNEL, listener) + }, reauthenticate: (args: { accountId: string; activateIfSelectionWasEmpty?: boolean }) => ipcRenderer.invoke('codexAccounts:reauthenticate', args), remove: (args: { accountId: string }) => ipcRenderer.invoke('codexAccounts:remove', args), diff --git a/src/renderer/src/components/editor/EditorConflictReviewSurface.tsx b/src/renderer/src/components/editor/EditorConflictReviewSurface.tsx index 85c53cfb41f..fbff9caf22e 100644 --- a/src/renderer/src/components/editor/EditorConflictReviewSurface.tsx +++ b/src/renderer/src/components/editor/EditorConflictReviewSurface.tsx @@ -131,6 +131,7 @@ export function EditorConflictReviewSurface({
reloadContent(contentFile)} />
diff --git a/src/renderer/src/components/editor/EditorContent.tsx b/src/renderer/src/components/editor/EditorContent.tsx index 9fab1cd5665..86527b25f29 100644 --- a/src/renderer/src/components/editor/EditorContent.tsx +++ b/src/renderer/src/components/editor/EditorContent.tsx @@ -188,6 +188,7 @@ export function EditorContent({ return ( reloadContent(activeFile)} /> ) diff --git a/src/renderer/src/components/editor/EditorEditFileSurface.tsx b/src/renderer/src/components/editor/EditorEditFileSurface.tsx index 06a7bb3f909..a9e3f8ff856 100644 --- a/src/renderer/src/components/editor/EditorEditFileSurface.tsx +++ b/src/renderer/src/components/editor/EditorEditFileSurface.tsx @@ -101,6 +101,7 @@ export function EditorEditFileSurface({ return ( reloadContent(activeFile)} /> ) diff --git a/src/renderer/src/components/editor/EditorFileLoadErrorView.test.tsx b/src/renderer/src/components/editor/EditorFileLoadErrorView.test.tsx new file mode 100644 index 00000000000..2d0d08fdfbe --- /dev/null +++ b/src/renderer/src/components/editor/EditorFileLoadErrorView.test.tsx @@ -0,0 +1,40 @@ +// @vitest-environment happy-dom +import { cleanup, fireEvent, render, screen } from '@testing-library/react' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { EditorFileLoadErrorView } from './EditorFileLoadErrorView' +import { + WORKTREE_HOST_UNRESOLVED_CODE, + WORKTREE_HOST_UNRESOLVED_ERROR +} from './editor-panel-content-types' + +describe('EditorFileLoadErrorView', () => { + afterEach(cleanup) + + it('offers Retry as its only action', () => { + // Why: closing must stay with the tab strip, whose path carries the pin, shared- + // reference, and unsaved-changes checks; a second close control here would not. + const onRetry = vi.fn() + + render() + + screen.getByText('selector_not_found') + expect(screen.getAllByRole('button')).toHaveLength(1) + fireEvent.click(screen.getByRole('button', { name: 'Retry' })) + expect(onRetry).toHaveBeenCalledOnce() + }) + + it('localizes the host-unresolved state by its sentinel code, not by the stored text', () => { + // Why: the stored `loadError` is an English fallback; the code is what selects the + // localized copy, so a translated catalog cannot desynchronize from the comparison. + render( + + ) + + screen.getByText(WORKTREE_HOST_UNRESOLVED_ERROR) + expect(screen.queryByText('stored fallback text')).toBeNull() + }) +}) diff --git a/src/renderer/src/components/editor/EditorFileLoadErrorView.tsx b/src/renderer/src/components/editor/EditorFileLoadErrorView.tsx index 41964fbd444..361e869d4ad 100644 --- a/src/renderer/src/components/editor/EditorFileLoadErrorView.tsx +++ b/src/renderer/src/components/editor/EditorFileLoadErrorView.tsx @@ -1,12 +1,32 @@ import { AlertCircle, RefreshCw } from 'lucide-react' import { Button } from '@/components/ui/button' import { translate } from '@/i18n/i18n' +import { WORKTREE_HOST_UNRESOLVED_CODE } from './editor-panel-content-types' +// Why: `loadError` is stored as English so logs and non-view consumers stay readable; the +// user-facing copy is keyed by the machine sentinel, never by the text, so localization +// cannot break the terminal-state comparison upstream (#21041). +function localizeFileLoadError(message: string, code: string | undefined): string { + if (code === WORKTREE_HOST_UNRESOLVED_CODE) { + return translate( + 'editor.fileLoad.hostUnresolved', + "The host couldn't find this file's workspace. It may have been removed, or the host may not know about it yet. Retry, or close this tab from the tab strip." + ) + } + return message +} + +// Why no Close action here: this view renders for real tabs and for synthesized inline +// conflict rows alike, and only the tab strip's own close path carries the pin, shared- +// reference, and unsaved-changes semantics. The copy points the user at that path instead +// of adding a second one that would have to reimplement it (#21041). export function EditorFileLoadErrorView({ message, + code, onRetry }: { message: string + code?: string onRetry: () => void }): React.JSX.Element { return ( @@ -17,7 +37,7 @@ export function EditorFileLoadErrorView({
{translate('auto.components.editor.EditorContent.39f018b052', 'Unable to load file')}
-
{message}
+
{localizeFileLoadError(message, code)}
+ + + {status === 'failed' ? ( +

+ {translate( + 'auto.components.settings.AccountsPane.codexLoginLinkCopyFailed', + 'Could not copy the link.' + )} +

+ ) : null} + + ) +} diff --git a/src/renderer/src/components/settings/accounts-pane-account-actions.ts b/src/renderer/src/components/settings/accounts-pane-account-actions.ts index 09be1ce001b..98ba3cac3f9 100644 --- a/src/renderer/src/components/settings/accounts-pane-account-actions.ts +++ b/src/renderer/src/components/settings/accounts-pane-account-actions.ts @@ -25,7 +25,8 @@ import type { import { getClaudeAccountErrorDescription, getCodexAccountErrorDescription, - isClaudeAccountCancellation + isClaudeAccountCancellation, + isCodexAccountCancellation } from './accounts-pane-action-errors' import { getClaudeAccountLabel } from './accounts-pane-runtime' @@ -117,6 +118,11 @@ export function createCodexAccountActionRunner( }) } } catch (error) { + // Why: cancelling — from the Cancel button, or by asking for a new login + // that supersedes this one — is the user's own doing, not a failure. + if (isCodexAccountCancellation(error)) { + return + } toast.error( translate( 'auto.components.settings.AccountsPane.5bf8764953', diff --git a/src/renderer/src/components/settings/accounts-pane-action-errors.ts b/src/renderer/src/components/settings/accounts-pane-action-errors.ts index c289e59d7df..c80a25b80bb 100644 --- a/src/renderer/src/components/settings/accounts-pane-action-errors.ts +++ b/src/renderer/src/components/settings/accounts-pane-action-errors.ts @@ -1,3 +1,5 @@ +import { CODEX_LOGIN_CANCELLED_MESSAGE } from '../../../../shared/codex-auth-errors' + export function getCodexAccountErrorDescription(error: unknown): string { const message = String((error as Error)?.message ?? error) .replace(/^Error occurred in handler for 'codexAccounts:[^']+':\s*/i, '') @@ -42,6 +44,13 @@ export function getClaudeAccountErrorDescription(error: unknown): string { ) } +export function isCodexAccountCancellation(error: unknown): boolean { + return ( + getCodexAccountErrorDescription(error).toLowerCase() === + CODEX_LOGIN_CANCELLED_MESSAGE.toLowerCase() + ) +} + export function isClaudeAccountCancellation(error: unknown): boolean { return getClaudeAccountErrorDescription(error).toLowerCase() === 'claude sign-in was cancelled.' } diff --git a/src/renderer/src/components/settings/accounts-pane-codex-section.tsx b/src/renderer/src/components/settings/accounts-pane-codex-section.tsx index 61d27ce49e0..8e30f563098 100644 --- a/src/renderer/src/components/settings/accounts-pane-codex-section.tsx +++ b/src/renderer/src/components/settings/accounts-pane-codex-section.tsx @@ -1,10 +1,11 @@ -import { AlertTriangle, Loader2, Plus } from 'lucide-react' +import { AlertTriangle, Loader2, Plus, X } from 'lucide-react' import { translate } from '@/i18n/i18n' import { selectCodexProviderAccount } from '@/runtime/runtime-provider-accounts-client' import { Badge } from '../ui/badge' import { Button } from '../ui/button' import { Label } from '../ui/label' import { OpenAIIcon } from '../status-bar/icons' +import { CodexLoginLinkNotice } from './CodexLoginLinkNotice' import { SearchableSetting } from './SearchableSetting' import { getAccountsCodexSearchEntries } from './accounts-search' import { getCodexSystemDefaultSubtitle } from './accounts-pane-runtime' @@ -152,36 +153,52 @@ export function renderCodexAccountsSection(model: AccountsPaneSectionModel): Rea )}

- {codexAction === 'adding' ? ( - - ) : ( - - )} - {translate('auto.components.settings.AccountsPane.b0e948a4f9', 'Add Account')} - + + ) : null} + {remoteAccountScopeNotice} + {/* Why not in a remote scope: the link belongs to a login running on + this desktop, which has nothing to do with the server named above. */} + {isRemoteAccountScope ? null : }