From 5f3db15ac761b2911f211e8272b3c5c3eabb9bf2 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 28 Jul 2026 15:29:57 -0700 Subject: [PATCH] fix(codex): keep the stale-pane prompt when two accounts share a label (#11228) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The startup sweep asks main which panes are stale, and main answers by account id. The renderer then threw that away: it resolved both ids to labels and let the store's A -> B -> A collapse compare the strings. Two accounts can share a label — doAddAccount has no duplicate-email check, so one OpenAI login used in two ChatGPT workspaces gives both the same email, and a failed roster read collapses every account to 'Codex account'. Either way the notice was deleted for a pane that really is running under the account the user switched away from. The sweep then made it permanent: it marked every stale pane notified, including the ones whose notice had just been dropped, and a notified pane is suppressed for the rest of the session. Relaunching cleared the set but the deletion recurred, so the prompt never came back and the pane kept running on the other account's auth.json and quota, silently. Carry the account ids into the notice and decide on them, falling back to labels only for callers that have none; report which panes were left holding a notice so a dropped one cannot claim suppression. The prompt also names the ChatGPT workspace when that is what tells two same-email accounts apart, which is what the two duplicated getCodexAccountLabel copies now share. --- .../src/components/settings/AccountsPane.tsx | 29 ++-- .../src/components/status-bar/StatusBar.tsx | 29 ++-- .../src/lib/codex-session-restart.test.ts | 4 +- src/renderer/src/lib/codex-session-restart.ts | 60 +++++-- .../codex-stale-pane-account-identity.test.ts | 162 ++++++++++++++++++ .../src/lib/codex-stale-pane-sweep.test.ts | 60 ++++++- .../codex-restart-notice-account-identity.ts | 26 +++ .../codex-restart-notice-lifecycle.test.ts | 76 ++++++++ src/renderer/src/store/slices/terminals.ts | 49 +++++- 9 files changed, 438 insertions(+), 57 deletions(-) create mode 100644 src/renderer/src/lib/codex-stale-pane-account-identity.test.ts create mode 100644 src/renderer/src/store/slices/codex-restart-notice-account-identity.ts diff --git a/src/renderer/src/components/settings/AccountsPane.tsx b/src/renderer/src/components/settings/AccountsPane.tsx index 4188db5c831..7578112895a 100644 --- a/src/renderer/src/components/settings/AccountsPane.tsx +++ b/src/renderer/src/components/settings/AccountsPane.tsx @@ -57,7 +57,10 @@ import { ProviderHostScopeControl } from './ProviderHostScopeControl' import { SearchableSetting } from './SearchableSetting' import { SettingsRow, SettingsSegmentedControl } from './SettingsFormControls' import { matchesSettingsSearch } from './settings-search' -import { markLiveCodexSessionsForRestart } from '@/lib/codex-session-restart' +import { + markLiveCodexSessionsForRestart, + resolveCodexRestartPromptAccountLabel +} from '@/lib/codex-session-restart' import { Dialog, DialogContent, @@ -173,16 +176,6 @@ function getHostRuntimeLabel(): string { : translate('auto.components.settings.AccountsPane.9baf45d071', 'This device') } -function getCodexAccountLabel( - state: CodexRateLimitAccountsState, - accountId: string | null | undefined -): string { - if (accountId == null) { - return 'System default' - } - return state.accounts.find((account) => account.id === accountId)?.email ?? 'Codex account' -} - // Why: the system-default row has no stored identity, so surface the real // ~/.codex login live — the OAuth email when signed in, a clear custom-provider // note for env-key logins, and the generic fallback when signed out. @@ -761,8 +754,18 @@ export function AccountsPane({ // Falling back to the row is the pre-existing behaviour, not a new risk. const addedAccount = newAccounts.length === 1 ? newAccounts[0] : undefined void markLiveCodexSessionsForRestart({ - previousAccountLabel: getCodexAccountLabel(codexAccounts, previousActiveAccountId), - nextAccountLabel: getCodexAccountLabel(next, nextActiveAccountId), + previousAccountLabel: resolveCodexRestartPromptAccountLabel( + codexAccounts.accounts, + previousActiveAccountId + ), + nextAccountLabel: resolveCodexRestartPromptAccountLabel( + next.accounts, + nextActiveAccountId + ), + // Why: two accounts can share an email, so the labels alone cannot + // tell the store whether this switch lands back on the launch account. + previousAccountId: previousActiveAccountId ?? null, + nextAccountId: nextActiveAccountId ?? null, // Why: the mutation wrote this row's slot only, so panes on any other // lane still launch under the account they already had. target: addedAccount ? getProviderAccountRuntime(addedAccount) : actionRuntime, diff --git a/src/renderer/src/components/status-bar/StatusBar.tsx b/src/renderer/src/components/status-bar/StatusBar.tsx index 2fb006b34d9..2c321685ff6 100644 --- a/src/renderer/src/components/status-bar/StatusBar.tsx +++ b/src/renderer/src/components/status-bar/StatusBar.tsx @@ -65,7 +65,10 @@ import { UsageRosterPanel, getTightestUsageSection } from './UsageRosterPanel' import { getUsageProviderAccountsSectionId } from './usage-provider-settings-target' import { formatRateLimitWindowChipLabel } from '@/lib/window-label-formatter' import { useResetCountdownClock } from '@/hooks/useResetCountdownClock' -import { markLiveCodexSessionsForRestart } from '@/lib/codex-session-restart' +import { + markLiveCodexSessionsForRestart, + resolveCodexRestartPromptAccountLabel +} from '@/lib/codex-session-restart' import { UpdateStatusSegment } from './UpdateStatusSegment' import { SkillUpdateStatusSegment } from './SkillUpdateStatusSegment' import { RemoteServerUpdateStatusSegment } from './RemoteServerUpdateStatusSegment' @@ -168,16 +171,6 @@ function getHostRuntimeLabel(): string { return navigator.userAgent.includes('Windows') ? 'Windows' : 'This device' } -function getCodexAccountLabel( - state: CodexRateLimitAccountsState, - accountId: string | null | undefined -): string { - if (accountId == null) { - return 'System default' - } - return state.accounts.find((account) => account.id === accountId)?.email ?? 'Codex account' -} - function getCodexAccountDisplayLabel(account: CodexStatusAccount): string { return account.workspaceLabel ? `${account.email} (${account.workspaceLabel})` : account.email } @@ -1466,8 +1459,18 @@ export function CodexSwitcherMenu({ const nextActiveAccountId = getCodexStatusActiveId(next, target) if (previousActiveAccountId !== nextActiveAccountId) { await markLiveCodexSessionsForRestart({ - previousAccountLabel: getCodexAccountLabel(accountState, previousActiveAccountId), - nextAccountLabel: getCodexAccountLabel(next, nextActiveAccountId), + previousAccountLabel: resolveCodexRestartPromptAccountLabel( + accountState.accounts, + previousActiveAccountId + ), + nextAccountLabel: resolveCodexRestartPromptAccountLabel( + next.accounts, + nextActiveAccountId + ), + // Why: two accounts can share an email, so the labels alone cannot + // tell the store whether this switch lands back on the launch account. + previousAccountId: previousActiveAccountId ?? null, + nextAccountId: nextActiveAccountId ?? null, // Why: the mutation wrote this row's slot only, so panes on any other // lane still launch under the account they already had. target, diff --git a/src/renderer/src/lib/codex-session-restart.test.ts b/src/renderer/src/lib/codex-session-restart.test.ts index 3c9c2c816b0..7263edcd9ab 100644 --- a/src/renderer/src/lib/codex-session-restart.test.ts +++ b/src/renderer/src/lib/codex-session-restart.test.ts @@ -795,7 +795,9 @@ describe('markRestoredStaleCodexSessionsForRestart', () => { expect(window.api.codexAccounts.listStalePanes).toHaveBeenCalledWith({ ptyIds: ['pty-1'] }) expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ previousAccountLabel: ACCOUNT_A, - nextAccountLabel: ACCOUNT_B + nextAccountLabel: ACCOUNT_B, + previousAccountId: 'account-a', + nextAccountId: 'account-b' }) }) diff --git a/src/renderer/src/lib/codex-session-restart.ts b/src/renderer/src/lib/codex-session-restart.ts index cf5a6f5ac2c..791fdc6ba4a 100644 --- a/src/renderer/src/lib/codex-session-restart.ts +++ b/src/renderer/src/lib/codex-session-restart.ts @@ -140,6 +140,11 @@ async function scanCodexPanes( export async function markLiveCodexSessionsForRestart(args: { previousAccountLabel: string nextAccountLabel: string + /** Ids behind the labels. Two accounts can share a label, so without these the + * store cannot tell a switch back to the launch account from a switch to a + * different account that merely reads the same. */ + previousAccountId?: string | null + nextAccountId?: string | null target?: CodexAccountSelectionTarget | null /** Set when the change cleared the selection rather than pointing it somewhere. */ clearsEveryWslDistro?: boolean @@ -162,7 +167,11 @@ export async function markLiveCodexSessionsForRestart(args: { liveCodexSessionPtyIds.map((ptyId) => ({ ptyId, previousAccountLabel: args.previousAccountLabel, - nextAccountLabel: args.nextAccountLabel + nextAccountLabel: args.nextAccountLabel, + ...(args.previousAccountId === undefined + ? {} + : { previousAccountId: args.previousAccountId }), + ...(args.nextAccountId === undefined ? {} : { nextAccountId: args.nextAccountId }) })) ) } @@ -203,28 +212,53 @@ export async function markRestoredStaleCodexSessionsForRestart(args?: { } const resolveAccountLabel = await createCodexAccountLabelResolver() - useAppStore.getState().markCodexRestartNotices( + const noticedPtyIds = useAppStore.getState().markCodexRestartNotices( stalePanes.map((pane) => ({ ptyId: pane.ptyId, previousAccountLabel: resolveAccountLabel(pane.launchAccountId), - nextAccountLabel: resolveAccountLabel(pane.activeAccountId) + nextAccountLabel: resolveAccountLabel(pane.activeAccountId), + // Why the ids: main decided staleness by id, and the labels can collide. + // Passing only labels hands the store a question it cannot answer. + previousAccountId: pane.launchAccountId, + nextAccountId: pane.activeAccountId })) ) - const notifiedPtyIds = new Set(stalePanes.map((pane) => pane.ptyId)) + // Why not every stale pane: the bind sweep suppresses a "notified" pane for the + // rest of the session, so a pane whose notice the store dropped must not claim + // one — that trades a missing prompt for a permanently missing prompt. + const notifiedPtyIds = new Set(noticedPtyIds) return scans.map((scan) => (notifiedPtyIds.has(scan.ptyId) ? { ...scan, notified: true } : scan)) } +/** + * Names an account for the restart prompt. + * + * Why the collision check: one OpenAI login added under two ChatGPT workspaces + * gives both accounts the same email, and "switch from x@y to x@y" names + * neither. The workspace is appended only when it is what tells them apart. + */ +export function resolveCodexRestartPromptAccountLabel( + accounts: readonly { id: string; email: string; workspaceLabel?: string | null }[], + accountId: string | null | undefined +): string { + if (accountId == null) { + return translate('auto.lib.codex.session.restart.4bd4a3a9c7', 'System default') + } + const account = accounts.find((entry) => entry.id === accountId) + if (!account) { + return translate('auto.lib.codex.session.restart.9f0b1c2d3e', 'Codex account') + } + const sharesEmail = accounts.some( + (entry) => entry.id !== account.id && entry.email === account.email + ) + return sharesEmail && account.workspaceLabel + ? `${account.email} (${account.workspaceLabel})` + : account.email +} + async function createCodexAccountLabelResolver(): Promise<(accountId: string | null) => string> { // Why: a failed roster read still yields usable prompts — the account ids are // already known, only their friendly emails are missing. const accounts = await window.api.codexAccounts.list().catch(() => null) - return (accountId) => { - if (accountId == null) { - return translate('auto.lib.codex.session.restart.4bd4a3a9c7', 'System default') - } - return ( - accounts?.accounts.find((account) => account.id === accountId)?.email ?? - translate('auto.lib.codex.session.restart.9f0b1c2d3e', 'Codex account') - ) - } + return (accountId) => resolveCodexRestartPromptAccountLabel(accounts?.accounts ?? [], accountId) } diff --git a/src/renderer/src/lib/codex-stale-pane-account-identity.test.ts b/src/renderer/src/lib/codex-stale-pane-account-identity.test.ts new file mode 100644 index 00000000000..aeeb1c674c9 --- /dev/null +++ b/src/renderer/src/lib/codex-stale-pane-account-identity.test.ts @@ -0,0 +1,162 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { useAppStore } from '@/store' +import { clearRuntimeCompatibilityCacheForTests } from '@/runtime/runtime-rpc-client' +import { markRestoredStaleCodexSessionsForRestart } from './codex-session-restart' + +// Why one shared email: doAddAccount has no duplicate-email check, so one OpenAI +// login used in two ChatGPT workspaces produces two accounts with equal labels. +const SHARED_EMAIL = 'shared@example.com' +const FALLBACK_LABEL = 'Codex account' + +function seedRoster( + accounts: { id: string; email: string; workspaceLabel?: string | null }[] +): void { + vi.mocked(window.api.codexAccounts.list).mockResolvedValue({ + accounts, + activeAccountId: 'account-b' + } as never) +} + +function noticeFor(ptyId: string): unknown { + return useAppStore.getState().codexRestartNoticeByPtyId[ptyId] +} + +describe('stale Codex panes are decided by account id, not label', () => { + const originalWindow = (globalThis as { window?: typeof window }).window + + beforeEach(() => { + clearRuntimeCompatibilityCacheForTests() + useAppStore.setState({ + settings: null as never, + tabsByWorktree: { + wt1: [ + { + id: 'tab-1', + ptyId: 'pty-1', + worktreeId: 'wt1', + title: 'orca-1', + customTitle: null, + color: null, + sortOrder: 0, + createdAt: 1 + } + ] + }, + ptyIdsByTabId: { 'tab-1': ['pty-1'] }, + pendingCodexPaneRestartIds: {}, + codexRestartNoticeByPtyId: {} + }) + ;(globalThis as { window: typeof window }).window = { + ...originalWindow, + api: { + ...originalWindow?.api, + pty: { + ...originalWindow?.api?.pty, + getForegroundProcess: vi.fn(), + hasChildProcesses: vi.fn().mockResolvedValue(false), + inspectProcess: vi + .fn() + .mockResolvedValue({ foregroundProcess: 'codex', hasChildProcesses: false }), + confirmForegroundProcess: vi.fn().mockResolvedValue(null) + }, + codexAccounts: { + ...originalWindow?.api?.codexAccounts, + list: vi.fn(), + listStalePanes: vi.fn().mockResolvedValue([]) + } + } + } as unknown as typeof window + seedRoster([ + { id: 'account-a', email: SHARED_EMAIL }, + { id: 'account-b', email: SHARED_EMAIL } + ]) + }) + + afterEach(() => { + if (originalWindow) { + ;(globalThis as { window: typeof window }).window = originalWindow + } else { + delete (globalThis as { window?: typeof window }).window + } + }) + + it('keeps the prompt when the two accounts resolve to the same label', async () => { + vi.mocked(window.api.codexAccounts.listStalePanes).mockResolvedValue([ + { ptyId: 'pty-1', launchAccountId: 'account-a', activeAccountId: 'account-b' } + ]) + + const scans = await markRestoredStaleCodexSessionsForRestart() + + expect(noticeFor('pty-1')).toMatchObject({ + previousAccountId: 'account-a', + nextAccountId: 'account-b' + }) + expect(scans.find((scan) => scan.ptyId === 'pty-1')?.notified).toBe(true) + }) + + it('keeps the prompt when the roster read fails and every label collapses', async () => { + vi.mocked(window.api.codexAccounts.list).mockRejectedValue(new Error('roster unavailable')) + vi.mocked(window.api.codexAccounts.listStalePanes).mockResolvedValue([ + { ptyId: 'pty-1', launchAccountId: 'account-a', activeAccountId: 'account-b' } + ]) + + const scans = await markRestoredStaleCodexSessionsForRestart() + + expect(noticeFor('pty-1')).toMatchObject({ + previousAccountLabel: FALLBACK_LABEL, + nextAccountLabel: FALLBACK_LABEL, + previousAccountId: 'account-a', + nextAccountId: 'account-b' + }) + expect(scans.find((scan) => scan.ptyId === 'pty-1')?.notified).toBe(true) + }) + + it('disambiguates shared emails by ChatGPT workspace', async () => { + seedRoster([ + { id: 'account-a', email: SHARED_EMAIL, workspaceLabel: 'Personal' }, + { id: 'account-b', email: SHARED_EMAIL, workspaceLabel: 'Acme' } + ]) + vi.mocked(window.api.codexAccounts.listStalePanes).mockResolvedValue([ + { ptyId: 'pty-1', launchAccountId: 'account-a', activeAccountId: 'account-b' } + ]) + + await markRestoredStaleCodexSessionsForRestart() + + expect(noticeFor('pty-1')).toMatchObject({ + previousAccountLabel: `${SHARED_EMAIL} (Personal)`, + nextAccountLabel: `${SHARED_EMAIL} (Acme)` + }) + }) + + it('raises no notice and reports nothing notified for a pane on the selected account', async () => { + const scans = await markRestoredStaleCodexSessionsForRestart() + + expect(useAppStore.getState().codexRestartNoticeByPtyId).toEqual({}) + expect(scans.every((scan) => scan.notified === false)).toBe(true) + }) + + it('does not report notified for a pane whose notice the store dropped', async () => { + // The pane already carries a notice remembering account-b as its launch + // account, so re-marking it against active account-b collapses the notice. + useAppStore.setState({ + codexRestartNoticeByPtyId: { + 'pty-1': { + previousAccountLabel: SHARED_EMAIL, + nextAccountLabel: SHARED_EMAIL, + previousAccountId: 'account-b', + nextAccountId: 'account-a' + } + } + }) + vi.mocked(window.api.codexAccounts.listStalePanes).mockResolvedValue([ + { ptyId: 'pty-1', launchAccountId: 'account-a', activeAccountId: 'account-b' } + ]) + + const scans = await markRestoredStaleCodexSessionsForRestart() + + expect(noticeFor('pty-1')).toBeUndefined() + // Why this matters: the sweep permanently suppresses every pane it is told + // was notified, so a dropped notice must never claim one. + expect(scans.find((scan) => scan.ptyId === 'pty-1')?.notified).toBe(false) + }) +}) diff --git a/src/renderer/src/lib/codex-stale-pane-sweep.test.ts b/src/renderer/src/lib/codex-stale-pane-sweep.test.ts index 251881a57de..390021b6bd1 100644 --- a/src/renderer/src/lib/codex-stale-pane-sweep.test.ts +++ b/src/renderer/src/lib/codex-stale-pane-sweep.test.ts @@ -91,7 +91,9 @@ describe('notifyCodexPaneBoundForStaleSweep', () => { expect(window.api.codexAccounts.listStalePanes).toHaveBeenCalledWith({ ptyIds: ['pty-1'] }) expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ previousAccountLabel: ACCOUNT_A, - nextAccountLabel: ACCOUNT_B + nextAccountLabel: ACCOUNT_B, + previousAccountId: 'account-a', + nextAccountId: 'account-b' }) }) @@ -136,7 +138,9 @@ describe('notifyCodexPaneBoundForStaleSweep', () => { expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ previousAccountLabel: ACCOUNT_A, - nextAccountLabel: ACCOUNT_B + nextAccountLabel: ACCOUNT_B, + previousAccountId: 'account-a', + nextAccountId: 'account-b' }) }) @@ -176,7 +180,9 @@ describe('notifyCodexPaneBoundForStaleSweep', () => { expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ previousAccountLabel: ACCOUNT_A, - nextAccountLabel: ACCOUNT_B + nextAccountLabel: ACCOUNT_B, + previousAccountId: 'account-a', + nextAccountId: 'account-b' }) expect(inspectCallCountFor('pty-1')).toBe(4) }) @@ -230,7 +236,9 @@ describe('notifyCodexPaneBoundForStaleSweep', () => { expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ previousAccountLabel: ACCOUNT_A, - nextAccountLabel: ACCOUNT_B + nextAccountLabel: ACCOUNT_B, + previousAccountId: 'account-a', + nextAccountId: 'account-b' }) }) @@ -266,7 +274,9 @@ describe('notifyCodexPaneBoundForStaleSweep', () => { expect(window.api.codexAccounts.listStalePanes).toHaveBeenCalledWith({ ptyIds: ['pty-1'] }) expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ previousAccountLabel: ACCOUNT_A, - nextAccountLabel: ACCOUNT_B + nextAccountLabel: ACCOUNT_B, + previousAccountId: 'account-a', + nextAccountId: 'account-b' }) }) @@ -288,6 +298,34 @@ describe('notifyCodexPaneBoundForStaleSweep', () => { expect(window.api.codexAccounts.listStalePanes).toHaveBeenCalledTimes(1) }) + it('does not suppress a pane whose notice the store dropped', async () => { + // Why: suppression is permanent for the session, so claiming a pane was + // notified when no notice survived silently strands it on the old account. + // The pane already remembers account-b as its launch account, which + // collapses a fresh notice pointing back at account-b. + useAppStore.setState({ + codexRestartNoticeByPtyId: { + 'pty-1': { + previousAccountLabel: ACCOUNT_B, + nextAccountLabel: ACCOUNT_A, + previousAccountId: 'account-b', + nextAccountId: 'account-a' + } + } + }) + vi.mocked(window.api.codexAccounts.listStalePanes).mockResolvedValue([STALE_PANE]) + + notifyCodexPaneBoundForStaleSweep('pty-1') + await vi.advanceTimersByTimeAsync(300) + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toBeUndefined() + expect(inspectCallCountFor('pty-1')).toBe(1) + + notifyCodexPaneBoundForStaleSweep('pty-1') + await vi.advanceTimersByTimeAsync(300) + + expect(inspectCallCountFor('pty-1')).toBe(2) + }) + it('never marks a plain shell pane, so its input is never blocked', async () => { useAppStore.setState({ tabsByWorktree: { @@ -333,7 +371,9 @@ describe('notifyCodexPaneBoundForStaleSweep', () => { expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ previousAccountLabel: ACCOUNT_A, - nextAccountLabel: ACCOUNT_B + nextAccountLabel: ACCOUNT_B, + previousAccountId: 'account-a', + nextAccountId: 'account-b' }) }) @@ -350,7 +390,9 @@ describe('notifyCodexPaneBoundForStaleSweep', () => { expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ previousAccountLabel: ACCOUNT_A, - nextAccountLabel: ACCOUNT_B + nextAccountLabel: ACCOUNT_B, + previousAccountId: 'account-a', + nextAccountId: 'account-b' }) }) @@ -396,7 +438,9 @@ describe('notifyCodexPaneBoundForStaleSweep', () => { expect(inspectCallCountFor('remote:env-1@@term-1')).toBe(0) expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ previousAccountLabel: ACCOUNT_A, - nextAccountLabel: ACCOUNT_B + nextAccountLabel: ACCOUNT_B, + previousAccountId: 'account-a', + nextAccountId: 'account-b' }) }) }) diff --git a/src/renderer/src/store/slices/codex-restart-notice-account-identity.ts b/src/renderer/src/store/slices/codex-restart-notice-account-identity.ts new file mode 100644 index 00000000000..e7a95ae8db0 --- /dev/null +++ b/src/renderer/src/store/slices/codex-restart-notice-account-identity.ts @@ -0,0 +1,26 @@ +/** One side of a Codex restart notice: the account's id when the caller knows + * it (`null` is the system default), plus the label shown in the prompt. */ +export type CodexRestartNoticeAccount = { + id?: string | null + label: string +} + +/** + * Decides whether two restart-notice accounts are the same account. + * + * Why not the labels: an account label is an email, and nothing stops two + * accounts from sharing one — the same OpenAI login added under two ChatGPT + * workspaces, or a failed roster read collapsing every account to the same + * fallback string. Deciding on labels there erases a prompt for a pane that is + * genuinely running on the account the user switched away from. Labels remain + * the fallback only for callers that carry no ids at all. + */ +export function isSameCodexRestartNoticeAccount( + a: CodexRestartNoticeAccount, + b: CodexRestartNoticeAccount +): boolean { + if (a.id !== undefined && b.id !== undefined) { + return a.id === b.id + } + return a.label === b.label +} diff --git a/src/renderer/src/store/slices/codex-restart-notice-lifecycle.test.ts b/src/renderer/src/store/slices/codex-restart-notice-lifecycle.test.ts index 3d78a0353c1..6e8f0ede98f 100644 --- a/src/renderer/src/store/slices/codex-restart-notice-lifecycle.test.ts +++ b/src/renderer/src/store/slices/codex-restart-notice-lifecycle.test.ts @@ -111,6 +111,22 @@ describe('codex restart notice lifecycle', () => { expect(useAppStore.getState().codexRestartNoticeByPtyId).toBe(before) }) + it('reports which panes are left holding a notice', () => { + expect( + useAppStore + .getState() + .markCodexRestartNotices([{ ptyId: 'pty-1', previousAccountLabel: A, nextAccountLabel: B }]) + ).toEqual(['pty-1']) + + // Why the caller needs this: the startup sweep suppresses a pane for the + // rest of the session once it is told a prompt went up. + expect( + useAppStore + .getState() + .markCodexRestartNotices([{ ptyId: 'pty-1', previousAccountLabel: B, nextAccountLabel: A }]) + ).toEqual([]) + }) + it('still deletes the record when the pane actually restarts', () => { switchAccount('pty-1', A, B) @@ -126,3 +142,63 @@ describe('codex restart notice lifecycle', () => { }) }) }) + +// Why: doAddAccount has no duplicate-email check, so one OpenAI login used in +// two ChatGPT workspaces gives two accounts the same label — as does a failed +// roster read, which collapses every account to 'Codex account'. +describe('codex restart notices with colliding account labels', () => { + const SHARED = 'shared@example.com' + + function switchAccountById(ptyId: string, from: string | null, to: string | null): void { + useAppStore.getState().markCodexRestartNotices([ + { + ptyId, + previousAccountLabel: SHARED, + nextAccountLabel: SHARED, + previousAccountId: from, + nextAccountId: to + } + ]) + } + + it('raises the prompt for two different accounts that read the same', () => { + switchAccountById('pty-1', 'account-a', 'account-b') + + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ + previousAccountLabel: SHARED, + nextAccountLabel: SHARED, + previousAccountId: 'account-a', + nextAccountId: 'account-b' + }) + }) + + it('still collapses A -> B -> A when the ids match', () => { + switchAccountById('pty-1', 'account-a', 'account-b') + switchAccountById('pty-1', 'account-b', 'account-a') + + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toBeUndefined() + }) + + it('collapses back to the system default the pane launched under', () => { + switchAccountById('pty-1', null, 'account-b') + switchAccountById('pty-1', 'account-b', null) + + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toBeUndefined() + }) + + it('re-asks a dismissed pane when a same-labelled third account is selected', () => { + switchAccountById('pty-1', 'account-a', 'account-b') + useAppStore.getState().dismissCodexRestartNotices(['pty-1']) + + switchAccountById('pty-1', 'account-b', 'account-c') + + // Why: carrying the dismissal on the shared label would strand the pane on + // account-a with its prompt answered for an account it never moved to. + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ + previousAccountLabel: SHARED, + nextAccountLabel: SHARED, + previousAccountId: 'account-a', + nextAccountId: 'account-c' + }) + }) +}) diff --git a/src/renderer/src/store/slices/terminals.ts b/src/renderer/src/store/slices/terminals.ts index 2720ce9a520..b24610cbab9 100644 --- a/src/renderer/src/store/slices/terminals.ts +++ b/src/renderer/src/store/slices/terminals.ts @@ -35,6 +35,7 @@ import { } from '../../../../shared/stable-pane-id' import { isValidHostTerminalTabId, isValidTerminalTabId } from '../../../../shared/terminal-tab-id' import { buildByIdIndex, buildWorktreeByIdIndex } from './worktree-by-id-index' +import { isSameCodexRestartNoticeAccount } from './codex-restart-notice-account-identity' import { getRepoIdFromWorktreeId, splitWorktreeIdForFilesystem @@ -465,9 +466,14 @@ export type AutomaticAgentResumeClaim = { export type CodexRestartNotice = { previousAccountLabel: string nextAccountLabel: string + /** Ids behind the two labels, when the caller knows them (`null` is the system + * default). Two accounts can share a label, so only these can decide whether + * a re-mark actually points back at the account the pane launched under. */ + previousAccountId?: string | null + nextAccountId?: string | null /** Set once the user asks for the restart. The record outlives the prompt - * because `previousAccountLabel` is the only memory of the account this pane - * actually launched under, which drives the A -> B -> A collapse. */ + * because the previous-account fields are the only memory of the account this + * pane actually launched under, which drives the A -> B -> A collapse. */ restartRequested?: true /** Set when the user answers "Keep old account". Same reason the record has to * survive: deleting it erased the launch account, so re-selecting it looked @@ -650,9 +656,14 @@ export type TerminalSlice = { isPtyShutdownPending: (ptyId: string) => boolean queueCodexPaneRestarts: (ptyIds: string[]) => void consumePendingCodexPaneRestart: (ptyId: string) => boolean + /** Returns the ptyIds left holding a notice, so callers can tell a raised + * prompt from one the A -> B -> A collapse dropped. */ markCodexRestartNotices: ( - notices: { ptyId: string; previousAccountLabel: string; nextAccountLabel: string }[] - ) => void + notices: (Pick & + Partial> & { + ptyId: string + })[] + ) => string[] clearCodexRestartNotice: (ptyId: string) => void dismissCodexRestartNotices: (ptyIds: string[]) => void setTabPaneExpanded: (tabId: string, expanded: boolean) => void @@ -2862,25 +2873,39 @@ export const createTerminalSlice: StateCreator markCodexRestartNotices: (notices) => { if (notices.length === 0) { - return + return [] } + const noticedPtyIds: string[] = [] set((s) => { const next = { ...s.codexRestartNoticeByPtyId } const nextPendingCodexPaneRestartIds = { ...s.pendingCodexPaneRestartIds } for (const notice of notices) { const existing = next[notice.ptyId] - const previousAccountLabel = existing?.previousAccountLabel ?? notice.previousAccountLabel + // Why one record rather than two lookups: the label and the id of the + // launch account have to come from the same source, or they can end up + // describing two different accounts. + const launch = existing ?? notice + const target = { id: notice.nextAccountId, label: notice.nextAccountLabel } // Why: a live Codex pane keeps its original launch account until it actually restarts, so A -> B -> A must not leave a stale restart notice. - if (previousAccountLabel === notice.nextAccountLabel) { + if ( + isSameCodexRestartNoticeAccount( + { id: launch.previousAccountId, label: launch.previousAccountLabel }, + target + ) + ) { delete next[notice.ptyId] delete nextPendingCodexPaneRestartIds[notice.ptyId] continue } next[notice.ptyId] = { - previousAccountLabel, + previousAccountLabel: launch.previousAccountLabel, nextAccountLabel: notice.nextAccountLabel, + ...(launch.previousAccountId === undefined + ? {} + : { previousAccountId: launch.previousAccountId }), + ...(notice.nextAccountId === undefined ? {} : { nextAccountId: notice.nextAccountId }), // Why: a queued restart relaunches under whatever account is selected // when it runs, so a later switch does not reopen an answered prompt. ...(existing?.restartRequested ? { restartRequested: true as const } : {}), @@ -2889,16 +2914,22 @@ export const createTerminalSlice: StateCreator // so a later C reopens the prompt while adding an account or // reauthenticating the active one (both re-mark live panes with the // selection unchanged) must not resurrect it and re-mute the pane. - ...(existing?.dismissed && existing.nextAccountLabel === notice.nextAccountLabel + ...(existing?.dismissed && + isSameCodexRestartNoticeAccount( + { id: existing.nextAccountId, label: existing.nextAccountLabel }, + target + ) ? { dismissed: true as const } : {}) } + noticedPtyIds.push(notice.ptyId) } return { codexRestartNoticeByPtyId: next, pendingCodexPaneRestartIds: nextPendingCodexPaneRestartIds } }) + return noticedPtyIds }, clearCodexRestartNotice: (ptyId) => {