refactor(codex): cut the sign-in-link plumbing to its smallest form

Review found the change correct but larger than it needs to be:

- The pending-link store was a class with one permanent subscriber, a
  never-called unsubscribe and a try/catch that could not fire. It is a
  field and a listener set on the service, beside the cancel handle it
  already owned — and the service now clears both in one place.
- The optional login-session dependencies were always supplied.
- The parser's https check could not fail; the pattern already fixed the
  scheme. The renderer's unmount guard inside a synchronous IPC listener
  could not fire either.
- The broadcast channel and the cancellation message are single sources
  of truth in src/shared now, rather than exported next to a hardcoded
  copy of themselves.
- The duplicated seven-line rationale in both services says the same
  thing in three, including why only add and reauthenticate supersede.
- The codex suite reuses its own factory, and unmocks once.

Also reverts four reformat hunks the formatter pulled in around edits.
This commit is contained in:
Neil
2026-09-17 22:51:37 -07:00
parent 6d64028a2f
commit e5daad4431
13 changed files with 114 additions and 187 deletions
+3 -6
View File
@@ -111,12 +111,9 @@ export class ClaudeAccountService {
return this.cancelPendingClaudeLogin?.() ?? false
}
/**
* Why: an abandoned login holds the mutation queue for its whole deadline, so
* the next add would sit behind it with a spinner and no browser, then inherit
* the abandoned login's timeout failure. Cancelling before enqueueing — never
* inside the queue, which the abandoned login owns — frees it immediately.
*/
// Why before the queue, not inside it: the abandoned login owns the queue slot
// the next add is waiting for. Only add/reauthenticate open a browser, so only
// they supersede — never serializeMutation, which background work also uses.
private supersedePendingLogin(): void {
if (this.cancelPendingLogin()) {
console.info(
@@ -24,8 +24,8 @@ export function parseCodexLoginAuthUrl(output: string): string | null {
return null
}
try {
const url = new URL(match[1])
return url.protocol === 'https:' ? url.toString() : null
// Only the shape is trusted here; the pattern already fixed the scheme.
return new URL(match[1]).toString()
} catch {
return null
}
+13 -22
View File
@@ -55,10 +55,10 @@ type CodexLoginSessionDependencies = {
child: CodexLoginChild,
interactiveLogin?: WindowsHostInteractiveLoginSpawn | null
) => void
/** Registers the handle that abandons this login; called with null once it settles. */
setCancel?: (cancel: (() => boolean) | 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
onAuthUrl: (url: string) => void
}
type LoginCancellation = {
@@ -72,7 +72,7 @@ export async function runCodexLoginSession(
): Promise<void> {
let cancelSpawnedLogin: (() => boolean) | null = null
let cancelled = false
dependencies.setCancel?.(() => {
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.
@@ -84,16 +84,12 @@ export async function runCodexLoginSession(
cancelled = true
return true
})
try {
await runCodexLoginProcess(managedHomePath, dependencies, {
isCancelled: () => cancelled,
setSpawnedCancel: (cancel) => {
cancelSpawnedLogin = cancel
}
})
} finally {
dependencies.setCancel?.(null)
}
await runCodexLoginProcess(managedHomePath, dependencies, {
isCancelled: () => cancelled,
setSpawnedCancel: (cancel) => {
cancelSpawnedLogin = cancel
}
})
}
async function runCodexLoginProcess(
@@ -158,7 +154,7 @@ async function runCodexLoginProcess(
const authUrl = parseCodexLoginAuthUrl(stdoutText)
if (authUrl) {
publishedAuthUrl = true
dependencies.onAuthUrl?.(authUrl)
dependencies.onAuthUrl(authUrl)
}
}
@@ -309,10 +305,7 @@ function createHostLoginSpawn(managedHomePath: string): {
return {
command: spawnCmd,
args: spawnArgs,
env: withCliRuntimeOnPath(codexCommand, {
...process.env,
CODEX_HOME: managedHomePath
}),
env: withCliRuntimeOnPath(codexCommand, { ...process.env, CODEX_HOME: managedHomePath }),
codexCommand,
interactiveLogin
}
@@ -340,9 +333,7 @@ async function assertWslCodexCliAvailable(wslInfo: {
if (result.code !== 0 || result.timedOut) {
throw new Error(
`Codex CLI is not available in WSL ${wslInfo.distro}. Install Codex in that distro or switch Account location to Windows.`,
{
cause: new Error(result.stderr.trim() || `codex lookup exited with ${result.code}`)
}
{ cause: new Error(result.stderr.trim() || `codex lookup exited with ${result.code}`) }
)
}
}
@@ -1,33 +0,0 @@
type PendingLoginUrlListener = (url: string | null) => void
/**
* The sign-in link of the Codex login that is waiting on a browser right now.
*
* Kept outside the login session so a renderer that opens Settings midway
* through a login can still ask for the link it never saw published.
*/
export class CodexPendingLoginUrl {
private url: string | null = null
private readonly listeners = new Set<PendingLoginUrlListener>()
get(): string | null {
return this.url
}
set(url: string | null): void {
if (this.url === url) {
return
}
this.url = url
for (const listener of this.listeners) {
listener(url)
}
}
subscribe(listener: PendingLoginUrlListener): () => void {
this.listeners.add(listener)
return () => {
this.listeners.delete(listener)
}
}
}
@@ -1,4 +1,4 @@
import { describe, expect, it, vi } from 'vitest'
import { afterEach, describe, expect, it, vi } from 'vitest'
import { EventEmitter } from 'node:events'
import { existsSync, writeFileSync } from 'node:fs'
import { join } from 'node:path'
@@ -59,6 +59,8 @@ async function createServiceWithHangingLogin(): Promise<{
service: {
addAccount: () => Promise<{ accounts: { email: string }[] }>
cancelPendingLogin: () => boolean
getPendingLoginUrl: () => string | null
subscribePendingLoginUrl: (listener: (url: string | null) => void) => void
}
children: StubLoginChild[]
/** The `CODEX_HOME` each login was spawned against. */
@@ -91,119 +93,86 @@ async function createServiceWithHangingLogin(): Promise<{
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()
try {
const abandoned = service.addAccount()
const abandonedRejection = expect(abandoned).rejects.toThrow('Codex sign-in was cancelled.')
await vi.waitUntil(() => children.length === 1)
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()
// 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()
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
} finally {
vi.doUnmock('node:child_process')
vi.doUnmock('../codex-cli/command')
}
service.cancelPendingLogin()
await retryRejection
})
it('refuses to cancel a sign-in that already wrote credentials, and keeps the account', async () => {
const { service, children, loginHomes } = await createServiceWithHangingLogin()
try {
const pending = service.addAccount()
await vi.waitUntil(() => children.length === 1)
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'
)
// 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()
// 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)
} finally {
vi.doUnmock('node:child_process')
vi.doUnmock('../codex-cli/command')
}
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()
try {
const pending = service.addAccount()
const rejection = expect(pending).rejects.toThrow('Codex sign-in was cancelled.')
await vi.waitUntil(() => children.length === 1)
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)
} finally {
vi.doUnmock('node:child_process')
vi.doUnmock('../codex-cli/command')
}
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 () => {
vi.resetModules()
const children: StubLoginChild[] = []
vi.doMock('node:child_process', () => ({
execFileSync: vi.fn(),
spawn: vi.fn(() => {
const child = createStubLoginChild()
children.push(child)
return child
})
}))
vi.doMock('../codex-cli/command', () => ({
resolveCodexCommand: () => 'codex'
}))
try {
const { CodexAccountService } = await import('./service')
const service = new CodexAccountService(
asServiceDouble(createStore(createSettings())),
asServiceDouble(createRateLimits()),
asServiceDouble(createRuntimeHome())
)
const published: (string | null)[] = []
service.subscribePendingLoginUrl((url) => published.push(url))
const { service, children } = await createServiceWithHangingLogin()
const published: (string | null)[] = []
service.subscribePendingLoginUrl((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 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])
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])
} finally {
vi.doUnmock('node:child_process')
vi.doUnmock('../codex-cli/command')
}
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])
})
})
+25 -14
View File
@@ -17,7 +17,6 @@ import type { CodexAccountSelectionTarget } from './runtime-selection'
import { CodexAccountIdentity, type ResolvedCodexIdentity } from './codex-account-identity'
import { CodexConfigMirror } from './codex-config-mirror'
import { runCodexLoginSession, type CodexLoginChild } from './codex-login-session'
import { CodexPendingLoginUrl } from './codex-pending-login-url'
import { CodexManagedHomePath } from './codex-managed-home-path'
import { CodexManagedHomeLifecycle } from './codex-managed-home-lifecycle'
import { CodexResetCreditCoordinator } from './codex-reset-credit-coordinator'
@@ -82,7 +81,8 @@ export class CodexAccountService {
// Why: serialize the read-modify-write of settings; overlapping calls (e.g. double-click Add) would lose updates.
private mutationQueue: Promise<unknown> = Promise.resolve()
private cancelPendingCodexLogin: (() => boolean) | null = null
private readonly pendingLoginUrl = new CodexPendingLoginUrl()
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
@@ -175,19 +175,28 @@ export class CodexAccountService {
/** The sign-in link of the login waiting on a browser, for a late-joining renderer. */
getPendingLoginUrl(): string | null {
return this.pendingLoginUrl.get()
return this.pendingLoginUrl
}
subscribePendingLoginUrl(listener: (url: string | null) => void): () => void {
return this.pendingLoginUrl.subscribe(listener)
subscribePendingLoginUrl(listener: (url: string | null) => void): void {
this.pendingLoginUrlListeners.add(listener)
}
/**
* Why: an abandoned login holds the mutation queue for its whole deadline, so
* the next add would sit behind it with a spinner and no browser, then inherit
* the abandoned login's timeout failure. Cancelling before enqueueing — never
* inside the queue, which the abandoned login owns — frees it immediately.
*/
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
// the next add is waiting for. Only add/reauthenticate open a browser, so only
// they supersede — never serializeMutation, which background work also uses.
private supersedePendingLogin(): void {
if (this.cancelPendingLogin()) {
console.info('[codex-accounts] Cancelled a pending Codex login superseded by a new request.')
@@ -279,11 +288,13 @@ export class CodexAccountService {
setCancel: (cancel) => {
this.cancelPendingCodexLogin = cancel
},
onAuthUrl: (url) => this.pendingLoginUrl.set(url)
onAuthUrl: (url) => this.setPendingLoginUrl(url)
})
} finally {
// Why: the link dies with the login server, so no surface may keep offering it.
this.pendingLoginUrl.set(null)
// 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)
}
}
}
@@ -1,6 +1,5 @@
import { BrowserWindow } from 'electron'
export const CODEX_PENDING_LOGIN_URL_CHANGED_CHANNEL = 'codexAccounts:pendingLoginUrlChanged'
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()) {
+3 -2
View File
@@ -1,4 +1,5 @@
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 = {
@@ -11,8 +12,8 @@ export const codexAccountsApi = {
ipcRenderer.invoke('codexAccounts:pendingLoginUrl'),
onPendingLoginUrlChanged: (callback: (url: string | null) => void): (() => void) => {
const listener = (_event: Electron.IpcRendererEvent, url: string | null): void => callback(url)
ipcRenderer.on('codexAccounts:pendingLoginUrlChanged', listener)
return () => ipcRenderer.removeListener('codexAccounts:pendingLoginUrlChanged', listener)
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),
@@ -2,7 +2,7 @@ import { Check, Copy, ExternalLink } from 'lucide-react'
import { translate } from '@/i18n/i18n'
import { useClipboardTextCopyFeedback } from '@/hooks/use-clipboard-text-copy-feedback'
import { Button } from '../ui/button'
import { useCodexPendingLoginUrl } from './codex-pending-login-url'
import { useCodexPendingLoginUrl } from './use-codex-pending-login-url'
/**
* The sign-in link of an in-flight `codex login`, so the user can finish the
@@ -15,9 +15,6 @@ export function useCodexPendingLoginUrl(): string | null {
let mounted = true
publishedRef.current = false
const unsubscribe = window.api.codexAccounts.onPendingLoginUrlChanged((next) => {
if (!mounted) {
return
}
publishedRef.current = true
setUrl(next)
})
@@ -3,11 +3,7 @@ import type { PreloadApi } from '../../../../preload/api-types'
export function createMiniMaxCredentialsApi(): NonNullable<
Partial<PreloadApi>['minimaxCredentials']
> {
const notConfigured = {
configured: false,
cookieConfigured: false,
apiKeyConfigured: false
}
const notConfigured = { configured: false, cookieConfigured: false, apiKeyConfigured: false }
const unsupportedError = new Error('MiniMax cookie storage is only available in the desktop app.')
return {
getStatus: () => Promise.resolve(notConfigured),
+1 -5
View File
@@ -113,11 +113,7 @@ function createWebPreloadApi(): Partial<PreloadApi> {
macosTccPrompts: createMacosTccPromptsApi(),
codexConfigSync: {
status: () =>
Promise.resolve({
state: 'synced',
reason: null,
systemConfigPath: ''
} as const)
Promise.resolve({ state: 'synced', reason: null, systemConfigPath: '' } as const)
},
developerPermissions: createDeveloperPermissionsApi(),
computerUsePermissions: createComputerUsePermissionsApi(),
+3
View File
@@ -1,3 +1,6 @@
/** Carries the sign-in link of an in-flight `codex login` from main to every window. */
export const CODEX_PENDING_LOGIN_URL_CHANGED_CHANNEL = 'codexAccounts:pendingLoginUrlChanged'
/** The rejection a cancelled `codex login` produces; the Accounts pane reads it to keep a cancellation out of the error toast. */
export const CODEX_LOGIN_CANCELLED_MESSAGE = 'Codex sign-in was cancelled.'