From 34e179eca92ce39208e67d0ecd5a5e1d7660b607 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 26 Jul 2026 20:58:24 -0700 Subject: [PATCH] fix(codex): keep a dismissed restart notice from killing the pane's keyboard (#10802) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Codex restart notice makes the pane's onData handler return early, so while one exists EVERY KEYSTROKE IS DROPPED. Dismissing ("Keep old account") deleted the notice outright, destroying the only memory of the account the pane launched under — so re-selecting that ORIGINAL account produced a fresh, INVERTED notice and a dead keyboard. Adds a `dismissed` marker plus two shared predicates (blocksCodexPaneInput / awaitsCodexRestartAnswer) honoured by every consumer, so a dismissal frees the keyboard immediately while keeping the launch-account memory that drives the A -> B -> A collapse. Reproduced on main before fixing, so this is pre-existing; #10770's startup sweep only widened its reach. Four defects found across four review rounds, all dead-keyboard: a dismissed pane still blocked by a sibling's notice in a split tab; queueCodexPaneRestarts not stripping the marker; a bound pane with no record inheriting tab.ptyId's block (a plain shell going silently mute); and — introduced by this PR — markCodexRestartNotices dropping the marker on every re-mark, so a routine token reauth resurrected an answered prompt and re-muted the pane. Live-validated in a real Orca dev build: 7/7 desktop cases, input liveness proven with filesystem probes rather than inferred from state, including a before/after reproduction of the bug on main's dismissal path in the same build. Known and out of scope: switching the HOST account marks and mutes live SSH remote panes. Verified pre-existing and byte-identical on main by two independent QA runs; needs a follow-up gating the marking on the selection lane. --- .../src/components/CodexRestartChip.tsx | 24 +- .../codex-restart-chip-inputs.test.ts | 29 ++- .../components/codex-restart-chip-inputs.ts | 6 +- .../src/components/codex-restart-chip.test.ts | 149 ++++++++++- .../components/codex-restart-notice-state.ts | 18 ++ .../codex-restart-status-summary.test.ts | 19 ++ .../codex-restart-status-summary.ts | 6 +- .../terminal-pane/pty-connection.test.ts | 232 +++++++++++++++++- .../terminal-pane/pty-connection.ts | 15 +- .../codex-restart-notice-lifecycle.test.ts | 128 ++++++++++ src/renderer/src/store/slices/terminals.ts | 49 +++- 11 files changed, 646 insertions(+), 29 deletions(-) create mode 100644 src/renderer/src/components/codex-restart-notice-state.ts create mode 100644 src/renderer/src/store/slices/codex-restart-notice-lifecycle.test.ts diff --git a/src/renderer/src/components/CodexRestartChip.tsx b/src/renderer/src/components/CodexRestartChip.tsx index 531e6a5cda4..a7537343494 100644 --- a/src/renderer/src/components/CodexRestartChip.tsx +++ b/src/renderer/src/components/CodexRestartChip.tsx @@ -7,6 +7,7 @@ import { selectCodexRestartInputs } from './codex-restart-chip-inputs' import { translate } from '@/i18n/i18n' import { shouldFocusMobileDriverAction } from './terminal-pane/mobile-driver-overlay-focus' import { buildCodexRestartNoticeKey } from './codex-restart-notice-key' +import { awaitsCodexRestartAnswer } from './codex-restart-notice-state' import type { CodexRestartNotice } from '../store/slices/terminals' const EMPTY_TABS: { id: string }[] = [] @@ -22,12 +23,13 @@ export function collectStalePtyIdsForTabs({ }): string[] { // Why: an already-requested restart runs when its pane next mounts. Keeping it // out of the prompt is what stops the panel from sticking on a worktree whose - // stale pane is parked or deferred and cannot answer the request yet. + // stale pane is parked or deferred and cannot answer the request yet. A + // dismissed notice is likewise answered — it only survives as launch-account + // memory. return tabs.flatMap((tab) => - (ptyIdsByTabId[tab.id] ?? []).filter((ptyId) => { - const notice = codexRestartNoticeByPtyId[ptyId] - return Boolean(notice) && !notice?.restartRequested - }) + (ptyIdsByTabId[tab.id] ?? []).filter((ptyId) => + awaitsCodexRestartAnswer(codexRestartNoticeByPtyId[ptyId]) + ) ) } @@ -51,15 +53,13 @@ export function collectStaleWorktreePtyIds({ export function dismissStaleWorktreePtyIds( staleWorktreePtyIds: string[], - clearCodexRestartNotice: (ptyId: string) => void, + dismissCodexRestartNotices: (ptyIds: string[]) => void, forgetLaunchAccounts: (ptyIds: string[]) => void ): void { // Why: restart notices are stored per PTY, but the workspace host presents - // one shared prompt. Clearing all matching PTY notices keeps every pane in + // one shared prompt. Dismissing all matching PTY notices keeps every pane in // that worktree consistent with the dismissal. - for (const ptyId of staleWorktreePtyIds) { - clearCodexRestartNotice(ptyId) - } + dismissCodexRestartNotices(staleWorktreePtyIds) // Why: notices are renderer-only, so without dropping the on-disk launch // record the startup sweep re-raises this exact prompt — and re-blocks the // pane's input — after every app restart the user already answered. @@ -104,7 +104,7 @@ export default function CodexRestartChip({ ? codexRestartNoticeByPtyId[staleWorktreePtyIds[0]] : undefined const queueCodexPaneRestarts = useAppStore((s) => s.queueCodexPaneRestarts) - const clearCodexRestartNotice = useAppStore((s) => s.clearCodexRestartNotice) + const dismissCodexRestartNotices = useAppStore((s) => s.dismissCodexRestartNotices) const noticeKey = restartNotice ? buildCodexRestartNoticeKey(restartNotice) : null @@ -117,7 +117,7 @@ export default function CodexRestartChip({ } const handleDismiss = (): void => { - dismissStaleWorktreePtyIds(staleWorktreePtyIds, clearCodexRestartNotice, (ptyIds) => { + dismissStaleWorktreePtyIds(staleWorktreePtyIds, dismissCodexRestartNotices, (ptyIds) => { void window.api.codexAccounts.forgetStalePanes({ ptyIds }).catch((err: unknown) => { console.warn('Failed to forget dismissed Codex pane accounts:', err) }) diff --git a/src/renderer/src/components/codex-restart-chip-inputs.test.ts b/src/renderer/src/components/codex-restart-chip-inputs.test.ts index ad6412efd16..9d54d5c1889 100644 --- a/src/renderer/src/components/codex-restart-chip-inputs.test.ts +++ b/src/renderer/src/components/codex-restart-chip-inputs.test.ts @@ -32,9 +32,32 @@ describe('selectCodexRestartInputs', () => { ) }) + it('re-idles once every surviving notice is answered', () => { + // Why: answered notices linger as launch-account memory for the pty's whole + // life, so existence alone would keep every chip subscribed to pty churn. + const dismissed: CodexRestartInputsState = { + ptyIdsByTabId: { 'tab-1': ['pty-1'] }, + codexRestartNoticeByPtyId: { + 'pty-1': { previousAccountLabel: 'a', nextAccountLabel: 'b', dismissed: true } + } + } + expect(selectCodexRestartInputs(dismissed)).toBe(EMPTY_CODEX_RESTART_INPUTS) + + const alsoUnanswered: CodexRestartInputsState = { + ...dismissed, + codexRestartNoticeByPtyId: { + ...dismissed.codexRestartNoticeByPtyId, + 'pty-2': { previousAccountLabel: 'a', nextAccountLabel: 'b' } + } + } + expect(selectCodexRestartInputs(alsoUnanswered)).not.toBe(EMPTY_CODEX_RESTART_INPUTS) + }) + it('exposes both live maps the instant a restart notice exists', () => { const ptyIdsByTabId = { 'tab-1': ['pty-1'] } - const codexRestartNoticeByPtyId = { 'pty-1': {} as never } + const codexRestartNoticeByPtyId = { + 'pty-1': { previousAccountLabel: 'a', nextAccountLabel: 'b' } + } const state: CodexRestartInputsState = { ptyIdsByTabId, codexRestartNoticeByPtyId } const selected = selectCodexRestartInputs(state) // Live references pass straight through so the stale-pty memo + notice lookup derive fully. @@ -47,7 +70,9 @@ describe('selectCodexRestartInputs', () => { const ptyIdsByTabId = { 'tab-1': ['pty-1'] } const s1: CodexRestartInputsState = { ptyIdsByTabId, - codexRestartNoticeByPtyId: { 'pty-1': {} as never } + codexRestartNoticeByPtyId: { + 'pty-1': { previousAccountLabel: 'a', nextAccountLabel: 'b' } + } } const r1 = selectCodexRestartInputs(s1) expect(shallow(r1, selectCodexRestartInputs(s1))).toBe(true) diff --git a/src/renderer/src/components/codex-restart-chip-inputs.ts b/src/renderer/src/components/codex-restart-chip-inputs.ts index eec46f6a8e7..52adc9b9e26 100644 --- a/src/renderer/src/components/codex-restart-chip-inputs.ts +++ b/src/renderer/src/components/codex-restart-chip-inputs.ts @@ -1,3 +1,4 @@ +import { awaitsCodexRestartAnswer } from './codex-restart-notice-state' import type { AppState } from '@/store/types' export type CodexRestartInputsState = Pick @@ -27,7 +28,10 @@ export const EMPTY_CODEX_RESTART_INPUTS: CodexRestartInputs = Object.freeze({ * `useShallow` subscription skips re-renders on unrelated pty-lifecycle churn. */ export function selectCodexRestartInputs(s: CodexRestartInputsState): CodexRestartInputs { - if (Object.keys(s.codexRestartNoticeByPtyId).length === 0) { + // Why: answered notices linger as launch-account memory for the life of the + // pty, so gating on mere existence would leave every chip permanently + // subscribed to that churn after a single dismissal. + if (!Object.values(s.codexRestartNoticeByPtyId).some(awaitsCodexRestartAnswer)) { return EMPTY_CODEX_RESTART_INPUTS } return { diff --git a/src/renderer/src/components/codex-restart-chip.test.ts b/src/renderer/src/components/codex-restart-chip.test.ts index 8d62bcccf47..5f3a8f5b14f 100644 --- a/src/renderer/src/components/codex-restart-chip.test.ts +++ b/src/renderer/src/components/codex-restart-chip.test.ts @@ -98,17 +98,30 @@ describe('CodexRestartChip helpers', () => { }) it('dismisses every stale PTY notice in the worktree prompt', () => { - const clearCodexRestartNotice = vi.fn() + const dismissCodexRestartNotices = vi.fn() const forgetLaunchAccounts = vi.fn() - dismissStaleWorktreePtyIds(['pty-1', 'pty-3'], clearCodexRestartNotice, forgetLaunchAccounts) + dismissStaleWorktreePtyIds(['pty-1', 'pty-3'], dismissCodexRestartNotices, forgetLaunchAccounts) - expect(clearCodexRestartNotice).toHaveBeenNthCalledWith(1, 'pty-1') - expect(clearCodexRestartNotice).toHaveBeenNthCalledWith(2, 'pty-3') - expect(clearCodexRestartNotice).toHaveBeenCalledTimes(2) + expect(dismissCodexRestartNotices).toHaveBeenCalledWith(['pty-1', 'pty-3']) expect(forgetLaunchAccounts).toHaveBeenCalledWith(['pty-1', 'pty-3']) }) + it('drops dismissed PTYs from the prompt while keeping their launch account', () => { + expect( + collectStalePtyIdsForTabs({ + tabs: [{ id: 'tab-1' }], + ptyIdsByTabId: { + 'tab-1': ['pty-1', 'pty-2'] + }, + codexRestartNoticeByPtyId: { + 'pty-1': { previousAccountLabel: 'a', nextAccountLabel: 'b', dismissed: true }, + 'pty-2': { previousAccountLabel: 'a', nextAccountLabel: 'b' } + } + }) + ).toEqual(['pty-2']) + }) + it('renders only account-resolution actions without an external-store update loop', async () => { useAppStore.setState({ tabsByWorktree: { @@ -188,7 +201,131 @@ describe('CodexRestartChip helpers', () => { // Why: without this the startup sweep re-raises the prompt the user just answered. expect(forgetStalePanes).toHaveBeenCalledWith({ ptyIds: ['pty-1'] }) - expect(useAppStore.getState().codexRestartNoticeByPtyId).toEqual({}) + expect(container.textContent).toBe('') + // Why: the record survives as the pane's launch-account memory, but marked + // answered so it neither prompts again nor blocks the pane's keyboard. + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ + previousAccountLabel: 'old@example.com', + nextAccountLabel: 'new@example.com', + dismissed: true + }) + }) + + it('stays closed when the user re-selects the account the pane launched under', async () => { + const forgetStalePanes = vi.fn(() => Promise.resolve()) + Object.defineProperty(window, 'api', { + configurable: true, + value: { codexAccounts: { forgetStalePanes } } + }) + useAppStore.setState({ + tabsByWorktree: { + 'worktree-1': [ + { + id: 'tab-1', + worktreeId: 'worktree-1', + title: 'Terminal', + customTitle: null, + color: null, + sortOrder: 0, + createdAt: 1, + ptyId: null + } + ] + }, + ptyIdsByTabId: { 'tab-1': ['pty-1'] } + }) + useAppStore + .getState() + .markCodexRestartNotices([ + { + ptyId: 'pty-1', + previousAccountLabel: 'old@example.com', + nextAccountLabel: 'new@example.com' + } + ]) + await act(async () => { + root.render(React.createElement(CodexRestartChip, { worktreeId: 'worktree-1' })) + }) + const dismissButton = Array.from(container.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Keep old account' + ) + await act(async () => { + dismissButton?.click() + }) + + // Re-select the pane's original account: it never left it, so there is + // nothing to restart and nothing to prompt about. + await act(async () => { + useAppStore.getState().markCodexRestartNotices([ + { + ptyId: 'pty-1', + previousAccountLabel: 'new@example.com', + nextAccountLabel: 'old@example.com' + } + ]) + }) + + expect(container.textContent).toBe('') + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toBeUndefined() + }) + + it('reopens the prompt when a dismissed pane is out of date against a third account', async () => { + const forgetStalePanes = vi.fn(() => Promise.resolve()) + Object.defineProperty(window, 'api', { + configurable: true, + value: { codexAccounts: { forgetStalePanes } } + }) + useAppStore.setState({ + tabsByWorktree: { + 'worktree-1': [ + { + id: 'tab-1', + worktreeId: 'worktree-1', + title: 'Terminal', + customTitle: null, + color: null, + sortOrder: 0, + createdAt: 1, + ptyId: null + } + ] + }, + ptyIdsByTabId: { 'tab-1': ['pty-1'] } + }) + useAppStore + .getState() + .markCodexRestartNotices([ + { + ptyId: 'pty-1', + previousAccountLabel: 'old@example.com', + nextAccountLabel: 'new@example.com' + } + ]) + await act(async () => { + root.render(React.createElement(CodexRestartChip, { worktreeId: 'worktree-1' })) + }) + const dismissButton = Array.from(container.querySelectorAll('button')).find( + (button) => button.textContent?.trim() === 'Keep old account' + ) + await act(async () => { + dismissButton?.click() + }) + expect(container.textContent).toBe('') + + await act(async () => { + useAppStore.getState().markCodexRestartNotices([ + { + ptyId: 'pty-1', + previousAccountLabel: 'new@example.com', + nextAccountLabel: 'third@example.com' + } + ]) + }) + + // Why: the dismissal answered "keep old instead of new", not "never prompt + // again"; the pane really is out of date against third@example.com. + expect(container.textContent).toContain('Codex is still signed in as old@example.com') + expect(container.textContent).toContain('Restart this session to use third@example.com') }) it('closes the prompt after Restart even when no mounted pane can run it yet', async () => { diff --git a/src/renderer/src/components/codex-restart-notice-state.ts b/src/renderer/src/components/codex-restart-notice-state.ts new file mode 100644 index 00000000000..a3f0cd6962c --- /dev/null +++ b/src/renderer/src/components/codex-restart-notice-state.ts @@ -0,0 +1,18 @@ +type CodexRestartNoticeState = { restartRequested?: true; dismissed?: true } | null | undefined + +/** + * True while the pane must not accept keystrokes. + * + * Why the two answered states differ: a requested restart still leaves the pane + * running under the old account until it actually relaunches, so its input stays + * blocked. A dismissal is the user choosing to keep that account, so blocking + * their keyboard would punish them for answering. + */ +export function blocksCodexPaneInput(notice: CodexRestartNoticeState): boolean { + return Boolean(notice) && !notice?.dismissed +} + +/** True while the restart prompt still has an unanswered question for the user. */ +export function awaitsCodexRestartAnswer(notice: CodexRestartNoticeState): boolean { + return Boolean(notice) && !notice?.dismissed && !notice?.restartRequested +} diff --git a/src/renderer/src/components/status-bar/codex-restart-status-summary.test.ts b/src/renderer/src/components/status-bar/codex-restart-status-summary.test.ts index 5a72e7670c7..e0dc25bc197 100644 --- a/src/renderer/src/components/status-bar/codex-restart-status-summary.test.ts +++ b/src/renderer/src/components/status-bar/codex-restart-status-summary.test.ts @@ -78,6 +78,25 @@ describe('summarizeCodexRestartStatus', () => { }) }) + it('stops offering sessions the user chose to keep on the old account', () => { + expect( + summarizeCodexRestartStatus({ + tabsByWorktree: { wt1: [{ id: 'tab-1' }] }, + ptyIdsByTabId: { 'tab-1': ['pty-1', 'pty-2'] }, + codexRestartNoticeByPtyId: { + // Why: the dismissed record survives only as launch-account memory. + 'pty-1': { previousAccountLabel: 'a', nextAccountLabel: 'b', dismissed: true }, + 'pty-2': { previousAccountLabel: 'a', nextAccountLabel: 'b' } + } + }) + ).toEqual({ + stalePtyIds: ['pty-2'], + staleSessionCount: 1, + staleTabCount: 1, + staleWorktreeCount: 1 + }) + }) + it('hides the prompt once every stale session has a requested restart', () => { expect( summarizeCodexRestartStatus({ diff --git a/src/renderer/src/components/status-bar/codex-restart-status-summary.ts b/src/renderer/src/components/status-bar/codex-restart-status-summary.ts index 6d3f0d7202e..8d7c44d458f 100644 --- a/src/renderer/src/components/status-bar/codex-restart-status-summary.ts +++ b/src/renderer/src/components/status-bar/codex-restart-status-summary.ts @@ -1,3 +1,4 @@ +import { awaitsCodexRestartAnswer } from '@/components/codex-restart-notice-state' import type { CodexRestartNotice } from '@/store/slices/terminals' type TabLookup = Record @@ -28,9 +29,10 @@ export function summarizeCodexRestartStatus({ codexRestartNoticeByPtyId }: CodexRestartStatusSummaryInput): CodexRestartStatusSummary { // Why: a requested restart is already scheduled for its pane's next mount, so - // re-offering it here would leave a prompt whose button does nothing. + // re-offering it here would leave a prompt whose button does nothing; a + // dismissed notice survives only as launch-account memory. const stalePtyIds = Object.entries(codexRestartNoticeByPtyId) - .filter(([, notice]) => notice && !notice.restartRequested) + .filter(([, notice]) => awaitsCodexRestartAnswer(notice)) .map(([ptyId]) => ptyId) if (stalePtyIds.length === 0) { return EMPTY_CODEX_RESTART_STATUS_SUMMARY diff --git a/src/renderer/src/components/terminal-pane/pty-connection.test.ts b/src/renderer/src/components/terminal-pane/pty-connection.test.ts index 422c25307ca..6a95ae76840 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.test.ts @@ -176,7 +176,12 @@ type StoreState = { } | null codexRestartNoticeByPtyId: Record< string, - { previousAccountLabel: string; nextAccountLabel: string } + { + previousAccountLabel: string + nextAccountLabel: string + restartRequested?: true + dismissed?: true + } > deferredSshReconnectTargets: string[] deferredSshSessionIdsByTabId: Record @@ -4496,6 +4501,231 @@ describe('connectPanePty', () => { expect(transport.sendInput).not.toHaveBeenCalled() }) + it('restores input to a Codex pane whose restart notice the user dismissed', async () => { + const { connectPanePty } = await import('./pty-connection') + + const transport = createMockTransport('pty-live') + transportFactoryQueue.push(transport) + mockStoreState = { + ...mockStoreState, + tabsByWorktree: { 'wt-1': [{ id: 'tab-1', ptyId: 'pty-live' }] }, + ptyIdsByTabId: { 'tab-1': ['pty-live'] }, + // Why: the record survives a dismissal as launch-account memory, so the + // input gate has to read the marker rather than the record's existence. + codexRestartNoticeByPtyId: { + 'pty-live': { previousAccountLabel: 'A', nextAccountLabel: 'B', dismissed: true } + } + } + + const pane = createPane(1) + let onDataHandler: ((data: string) => void) | null = null + pane.terminal.onData = vi.fn(((handler: (data: string) => void) => { + onDataHandler = handler + return { dispose: vi.fn() } + }) as typeof pane.terminal.onData) + const manager = createManager(1) + const deps = createDeps() + + connectPanePty(pane as never, manager as never, deps as never) + + expect(onDataHandler).toBeDefined() + if (!onDataHandler) { + throw new Error('expected onData handler to be registered') + } + ;(onDataHandler as (data: string) => void)('a') + + expect(transport.sendInput).toHaveBeenCalledWith('a') + }) + + it('restores input through the tab fallback when the dismissed pane has no live PTY binding', async () => { + const { connectPanePty } = await import('./pty-connection') + + const transport = createMockTransport(null) + transportFactoryQueue.push(transport) + mockStoreState = { + ...mockStoreState, + tabsByWorktree: { 'wt-1': [{ id: 'tab-1', ptyId: 'pty-live' }] }, + ptyIdsByTabId: { 'tab-1': ['pty-live'] }, + codexRestartNoticeByPtyId: { + 'pty-live': { previousAccountLabel: 'A', nextAccountLabel: 'B', dismissed: true } + } + } + + const pane = createPane(1) + let onDataHandler: ((data: string) => void) | null = null + pane.terminal.onData = vi.fn(((handler: (data: string) => void) => { + onDataHandler = handler + return { dispose: vi.fn() } + }) as typeof pane.terminal.onData) + const manager = createManager(1) + const deps = createDeps() + + connectPanePty(pane as never, manager as never, deps as never) + + expect(onDataHandler).toBeDefined() + if (!onDataHandler) { + throw new Error('expected onData handler to be registered') + } + ;(onDataHandler as (data: string) => void)('a') + + expect(transport.sendInput).toHaveBeenCalledWith('a') + }) + + it('keeps a dismissed split pane typing while a sibling still holds the prompt', async () => { + const { connectPanePty } = await import('./pty-connection') + + const transport = createMockTransport('pty-dismissed') + transportFactoryQueue.push(transport) + mockStoreState = { + ...mockStoreState, + // Why: `tab.ptyId` is whichever pane bound last, so a sibling's unanswered + // notice would otherwise kill this pane's keyboard with no prompt of its own. + tabsByWorktree: { 'wt-1': [{ id: 'tab-1', ptyId: 'pty-sibling' }] }, + ptyIdsByTabId: { 'tab-1': ['pty-dismissed', 'pty-sibling'] }, + codexRestartNoticeByPtyId: { + 'pty-dismissed': { previousAccountLabel: 'A', nextAccountLabel: 'B', dismissed: true }, + 'pty-sibling': { previousAccountLabel: 'A', nextAccountLabel: 'C' } + } + } + + const pane = createPane(1) + let onDataHandler: ((data: string) => void) | null = null + pane.terminal.onData = vi.fn(((handler: (data: string) => void) => { + onDataHandler = handler + return { dispose: vi.fn() } + }) as typeof pane.terminal.onData) + const manager = createManager(1) + const deps = createDeps({ + // Why: the sibling already holds `tab.ptyId`, which is what makes this a + // split rather than this pane adopting the tab's binding. + paneTransportsRef: { current: new Map([[2, createMockTransport('pty-sibling')]]) } + }) + + connectPanePty(pane as never, manager as never, deps as never) + + expect(onDataHandler).toBeDefined() + if (!onDataHandler) { + throw new Error('expected onData handler to be registered') + } + ;(onDataHandler as (data: string) => void)('a') + + expect((transport.getPtyId as unknown as () => string | null)()).toBe('pty-dismissed') + expect(transport.sendInput).toHaveBeenCalledWith('a') + }) + + it('keeps blocking a pane with its own unanswered notice next to a dismissed sibling', async () => { + const { connectPanePty } = await import('./pty-connection') + + const transport = createMockTransport('pty-stale') + transportFactoryQueue.push(transport) + mockStoreState = { + ...mockStoreState, + tabsByWorktree: { 'wt-1': [{ id: 'tab-1', ptyId: 'pty-sibling' }] }, + ptyIdsByTabId: { 'tab-1': ['pty-stale', 'pty-sibling'] }, + codexRestartNoticeByPtyId: { + 'pty-stale': { previousAccountLabel: 'A', nextAccountLabel: 'B' }, + 'pty-sibling': { previousAccountLabel: 'A', nextAccountLabel: 'B', dismissed: true } + } + } + + const pane = createPane(1) + let onDataHandler: ((data: string) => void) | null = null + pane.terminal.onData = vi.fn(((handler: (data: string) => void) => { + onDataHandler = handler + return { dispose: vi.fn() } + }) as typeof pane.terminal.onData) + const manager = createManager(1) + const deps = createDeps({ + paneTransportsRef: { current: new Map([[2, createMockTransport('pty-sibling')]]) } + }) + + connectPanePty(pane as never, manager as never, deps as never) + + expect(onDataHandler).toBeDefined() + if (!onDataHandler) { + throw new Error('expected onData handler to be registered') + } + ;(onDataHandler as (data: string) => void)('a') + + expect((transport.getPtyId as unknown as () => string | null)()).toBe('pty-stale') + expect(transport.sendInput).not.toHaveBeenCalled() + }) + + it('keeps a bound pane with no notice typing while a sibling holds a queued restart', async () => { + const { connectPanePty } = await import('./pty-connection') + + const transport = createMockTransport('pty-plain-shell') + transportFactoryQueue.push(transport) + mockStoreState = { + ...mockStoreState, + // Why: a requested restart hides the prompt, so blocking this record-less + // pane through `tab.ptyId` would leave a dead keyboard and nothing to answer. + tabsByWorktree: { 'wt-1': [{ id: 'tab-1', ptyId: 'pty-sibling' }] }, + ptyIdsByTabId: { 'tab-1': ['pty-plain-shell', 'pty-sibling'] }, + codexRestartNoticeByPtyId: { + 'pty-sibling': { previousAccountLabel: 'A', nextAccountLabel: 'B', restartRequested: true } + } + } + + const pane = createPane(1) + let onDataHandler: ((data: string) => void) | null = null + pane.terminal.onData = vi.fn(((handler: (data: string) => void) => { + onDataHandler = handler + return { dispose: vi.fn() } + }) as typeof pane.terminal.onData) + const manager = createManager(1) + const deps = createDeps({ + paneTransportsRef: { current: new Map([[2, createMockTransport('pty-sibling')]]) } + }) + + connectPanePty(pane as never, manager as never, deps as never) + + expect(onDataHandler).toBeDefined() + if (!onDataHandler) { + throw new Error('expected onData handler to be registered') + } + ;(onDataHandler as (data: string) => void)('a') + + expect((transport.getPtyId as unknown as () => string | null)()).toBe('pty-plain-shell') + expect(transport.sendInput).toHaveBeenCalledWith('a') + }) + + it('keeps blocking input on a pane whose restart is requested but not yet run', async () => { + const { connectPanePty } = await import('./pty-connection') + + const transport = createMockTransport('pty-live') + transportFactoryQueue.push(transport) + mockStoreState = { + ...mockStoreState, + tabsByWorktree: { 'wt-1': [{ id: 'tab-1', ptyId: 'pty-live' }] }, + ptyIdsByTabId: { 'tab-1': ['pty-live'] }, + // Why: unlike a dismissal, a queued restart leaves the pane running under + // the old account until it actually relaunches. + codexRestartNoticeByPtyId: { + 'pty-live': { previousAccountLabel: 'A', nextAccountLabel: 'B', restartRequested: true } + } + } + + const pane = createPane(1) + let onDataHandler: ((data: string) => void) | null = null + pane.terminal.onData = vi.fn(((handler: (data: string) => void) => { + onDataHandler = handler + return { dispose: vi.fn() } + }) as typeof pane.terminal.onData) + const manager = createManager(1) + const deps = createDeps() + + connectPanePty(pane as never, manager as never, deps as never) + + expect(onDataHandler).toBeDefined() + if (!onDataHandler) { + throw new Error('expected onData handler to be registered') + } + ;(onDataHandler as (data: string) => void)('a') + + expect(transport.sendInput).not.toHaveBeenCalled() + }) + it('blocks input to stale Codex panes until they restart', async () => { const { connectPanePty } = await import('./pty-connection') diff --git a/src/renderer/src/components/terminal-pane/pty-connection.ts b/src/renderer/src/components/terminal-pane/pty-connection.ts index df1e39e6ebb..b2e56485fc1 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.ts @@ -5,6 +5,7 @@ import type { IBuffer, IDisposable } from '@xterm/xterm' import { resolveCursorAgentImeAnchor } from '@/lib/pane-manager/terminal-ime-anchor' import { detectAgentStatusFromTitle, agentTypeToIconAgent, isClaudeAgent } from '@/lib/agent-status' import { resolvePaneTitleDecision } from './terminal-title-evidence' +import { blocksCodexPaneInput } from '../codex-restart-notice-state' import { resolveLiveAgentStatusConnectionRouting } from '@/lib/agent-status-connection-ownership' import { scheduleRuntimeGraphSync } from '@/runtime/sync-runtime-graph' import { useAppStore } from '@/store' @@ -826,12 +827,20 @@ function isCodexPaneStale(args: { if (!hasCodexRestartNotices(codexRestartNoticeByPtyId)) { return false } - if (args.panePtyId && codexRestartNoticeByPtyId[args.panePtyId]) { - return true + // Why: a bound pane's own record is the last word — its ptyId is exactly the + // shell its keystrokes reach. `tab.ptyId` holds one sibling's id, not this + // pane's, so in a split tab consulting it would kill this pane's keyboard over + // that sibling's notice, with no prompt on screen once the sibling is answered. + if (args.panePtyId) { + return blocksCodexPaneInput(codexRestartNoticeByPtyId[args.panePtyId]) } + // Why: only an unbound pane needs the tab's persisted id. Both transports + // refuse writes while their pty binding is null, so a keystroke here arms + // recovery — a coarser signal than a bound pane's own record, but the tab's id + // is the only evidence available of which shell this pane is about to own. const tab = (state.tabsByWorktree[args.worktreeId] ?? []).find((entry) => entry.id === args.tabId) - if (tab?.ptyId && codexRestartNoticeByPtyId[tab.ptyId]) { + if (tab?.ptyId && blocksCodexPaneInput(codexRestartNoticeByPtyId[tab.ptyId])) { return true } 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 new file mode 100644 index 00000000000..3d78a0353c1 --- /dev/null +++ b/src/renderer/src/store/slices/codex-restart-notice-lifecycle.test.ts @@ -0,0 +1,128 @@ +import { beforeEach, describe, expect, it } from 'vitest' +import { useAppStore } from '../index' + +const A = 'a@example.com' +const B = 'b@example.com' +const C = 'c@example.com' + +function switchAccount(ptyId: string, from: string, to: string): void { + useAppStore + .getState() + .markCodexRestartNotices([{ ptyId, previousAccountLabel: from, nextAccountLabel: to }]) +} + +beforeEach(() => { + useAppStore.setState(useAppStore.getInitialState(), true) +}) + +describe('codex restart notice lifecycle', () => { + it('raises no notice when the pane returns to the account it launched under', () => { + switchAccount('pty-1', A, B) + switchAccount('pty-1', B, A) + + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toBeUndefined() + }) + + it('keeps the launch account after a dismissal so re-selecting it raises no notice', () => { + switchAccount('pty-1', A, B) + useAppStore.getState().dismissCodexRestartNotices(['pty-1']) + + switchAccount('pty-1', B, A) + + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toBeUndefined() + }) + + it('marks a dismissal instead of erasing the pane record', () => { + switchAccount('pty-1', A, B) + useAppStore.getState().dismissCodexRestartNotices(['pty-1']) + + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ + previousAccountLabel: A, + nextAccountLabel: B, + dismissed: true + }) + }) + + it('re-raises the prompt against the pane launch account when a third account is selected', () => { + switchAccount('pty-1', A, B) + useAppStore.getState().dismissCodexRestartNotices(['pty-1']) + + switchAccount('pty-1', B, C) + + // Why: over-suppressing here would silently strand the pane on A while the + // user works under C; the labels must name the launch account, not B. + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ + previousAccountLabel: A, + nextAccountLabel: C + }) + }) + + it('leaves a dismissal answered when the active account is re-marked unchanged', () => { + switchAccount('pty-1', A, B) + useAppStore.getState().dismissCodexRestartNotices(['pty-1']) + + // Why: adding an account and reauthenticating the active one both re-mark + // live panes with the selection unchanged. Nothing about the pane moved, so + // resurrecting the prompt would also re-block a keyboard the user freed. + switchAccount('pty-1', B, B) + + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ + previousAccountLabel: A, + nextAccountLabel: B, + dismissed: true + }) + }) + + it('drops a queued restart when the user dismisses the same pane', () => { + switchAccount('pty-1', A, B) + useAppStore.getState().queueCodexPaneRestarts(['pty-1']) + + useAppStore.getState().dismissCodexRestartNotices(['pty-1']) + + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ + previousAccountLabel: A, + nextAccountLabel: B, + dismissed: true + }) + expect(useAppStore.getState().pendingCodexPaneRestartIds).toEqual({}) + }) + + it('re-blocks a dismissed pane once a restart is queued for it', () => { + switchAccount('pty-1', A, B) + useAppStore.getState().dismissCodexRestartNotices(['pty-1']) + + useAppStore.getState().queueCodexPaneRestarts(['pty-1']) + + // Why: the pane still runs under A until it relaunches, so the dismissal + // that freed its keyboard must not outlive the restart request. + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ + previousAccountLabel: A, + nextAccountLabel: B, + restartRequested: true + }) + }) + + it('leaves the notice map identity alone when nothing is dismissable', () => { + switchAccount('pty-1', A, B) + const before = useAppStore.getState().codexRestartNoticeByPtyId + + useAppStore.getState().dismissCodexRestartNotices(['pty-unknown']) + + expect(useAppStore.getState().codexRestartNoticeByPtyId).toBe(before) + }) + + it('still deletes the record when the pane actually restarts', () => { + switchAccount('pty-1', A, B) + + // Why: the relaunched pane genuinely runs under B, so its launch-account + // memory must reset rather than linger and suppress a later B -> A prompt. + useAppStore.getState().clearCodexRestartNotice('pty-1') + + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toBeUndefined() + switchAccount('pty-1', B, A) + expect(useAppStore.getState().codexRestartNoticeByPtyId['pty-1']).toEqual({ + previousAccountLabel: B, + nextAccountLabel: A + }) + }) +}) diff --git a/src/renderer/src/store/slices/terminals.ts b/src/renderer/src/store/slices/terminals.ts index 71c7c3c70a9..0d5d6930986 100644 --- a/src/renderer/src/store/slices/terminals.ts +++ b/src/renderer/src/store/slices/terminals.ts @@ -466,6 +466,10 @@ export type CodexRestartNotice = { * because `previousAccountLabel` is 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 + * like a fresh switch and raised an inverted prompt that killed pane input. */ + dismissed?: true } export type TerminalSlice = { @@ -647,6 +651,7 @@ export type TerminalSlice = { notices: { ptyId: string; previousAccountLabel: string; nextAccountLabel: string }[] ) => void clearCodexRestartNotice: (ptyId: string) => void + dismissCodexRestartNotices: (ptyIds: string[]) => void setTabPaneExpanded: (tabId: string, expanded: boolean) => void setTabCanExpandPane: (tabId: string, canExpand: boolean) => void setTabLayout: (tabId: string, layout: TerminalLayoutSnapshot | null) => void @@ -2797,7 +2802,11 @@ export const createTerminalSlice: StateCreator for (const ptyId of ptyIds) { const notice = nextCodexRestartNoticeByPtyId[ptyId] if (notice) { - nextCodexRestartNoticeByPtyId[ptyId] = { ...notice, restartRequested: true } + // Why: mirror of the strip in dismissCodexRestartNotices. A queued + // restart leaves the pane on the old account until it relaunches, so + // it must re-block input that an earlier dismissal had freed. + const { dismissed: _dismissed, ...kept } = notice + nextCodexRestartNoticeByPtyId[ptyId] = { ...kept, restartRequested: true } } } return { @@ -2847,7 +2856,15 @@ export const createTerminalSlice: StateCreator nextAccountLabel: notice.nextAccountLabel, // 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 } : {}) + ...(existing?.restartRequested ? { restartRequested: true as const } : {}), + // Why: a dismissal answered one question — "keep the launch account + // instead of this one". Only a genuinely different target re-asks it, + // 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 + ? { dismissed: true as const } + : {}) } } return { @@ -2873,6 +2890,34 @@ export const createTerminalSlice: StateCreator }) }, + dismissCodexRestartNotices: (ptyIds) => { + set((s) => { + // Why: keeping the old account is an answer, not a restart — the record + // stays so `previousAccountLabel` still names the pane's launch account, + // but every consumer treats it as answered (prompt hidden, input freed). + const next = { ...s.codexRestartNoticeByPtyId } + const nextPendingCodexPaneRestartIds = { ...s.pendingCodexPaneRestartIds } + let changed = false + for (const ptyId of ptyIds) { + const notice = next[ptyId] + if (!notice || notice.dismissed) { + continue + } + const { restartRequested: _restartRequested, ...kept } = notice + next[ptyId] = { ...kept, dismissed: true } + delete nextPendingCodexPaneRestartIds[ptyId] + changed = true + } + if (!changed) { + return {} + } + return { + codexRestartNoticeByPtyId: next, + pendingCodexPaneRestartIds: nextPendingCodexPaneRestartIds + } + }) + }, + setTabPaneExpanded: (tabId, expanded) => { set((s) => ({ expandedPaneByTabId: { ...s.expandedPaneByTabId, [tabId]: expanded }