From 0a2af963bc4339d4dd803be3de6aa3cb9856e182 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Sun, 20 Sep 2026 19:30:05 -0400 Subject: [PATCH] fix(terminal): let the renderer decide which workspace key a revealed pane is filed under The reveal identity assert compared the renderer's reply against the worktree key the caller passed in, so a reveal that surfaced exactly the right pane under a different key was rejected and, in legacy worker recovery, rolled back. tabId, leafId and ptyId are still asserted on both paths; only the workspace key now follows the renderer. The legacy-worker comparison moves into its own module so it is typechecked and testable on its own. --- .../runtime/legacy-worker-reveal-identity.ts | 20 ++ ...act-persisted-terminal-surface-identity.ts | 10 +- src/main/window/runtime-window-lifecycle.ts | 11 +- .../runtime-window-reveal-identity.test.ts | 212 ++++++++++++++++++ 4 files changed, 241 insertions(+), 12 deletions(-) create mode 100644 src/main/runtime/legacy-worker-reveal-identity.ts create mode 100644 src/main/window/runtime-window-reveal-identity.test.ts diff --git a/src/main/runtime/legacy-worker-reveal-identity.ts b/src/main/runtime/legacy-worker-reveal-identity.ts new file mode 100644 index 00000000000..0f4e99c6de3 --- /dev/null +++ b/src/main/runtime/legacy-worker-reveal-identity.ts @@ -0,0 +1,20 @@ +import type { TerminalRevealIdentity } from '../../shared/terminal-reveal-identity' + +/** + * Whether the renderer materialized the exact pane a legacy worker recovery asked for. + * + * Why no worktreeId: ownership is tab-keyed, so the renderer decides which workspace key the row + * is filed under, and re-asserting the caller's key rolled back a reveal that had in fact + * surfaced the right pane under a different one (STA-7961). The pane identity is still asserted. + */ +export function revealedLegacyWorkerIdentityMatches( + identity: TerminalRevealIdentity | undefined, + candidate: { tabId: string; leafId: string; ptyId: string } +): boolean { + return Boolean( + identity && + identity.tabId === candidate.tabId && + identity.leafId === candidate.leafId && + identity.ptyId === candidate.ptyId + ) +} diff --git a/src/main/runtime/orca-runtime-has-exact-persisted-terminal-surface-identity.ts b/src/main/runtime/orca-runtime-has-exact-persisted-terminal-surface-identity.ts index d0425cdc1f9..8d561d8cfcb 100644 --- a/src/main/runtime/orca-runtime-has-exact-persisted-terminal-surface-identity.ts +++ b/src/main/runtime/orca-runtime-has-exact-persisted-terminal-surface-identity.ts @@ -5,6 +5,7 @@ import { runtimeWorktreeIdsEqual } from './runtime-worktree-path-identity' import { makePaneKey } from '../../shared/stable-pane-id' +import { revealedLegacyWorkerIdentityMatches } from './legacy-worker-reveal-identity' import type { LegacyWorkerTerminalRecoveryPlan } from './orchestration/orchestration-legacy-worker-terminal-recovery' import { retireTerminalSurfacesFromSnapshot } from './mobile-session-terminal-retirement' import type { @@ -169,14 +170,7 @@ export class OrcaRuntimeWithHasExactPersistedTerminalSurfaceIdentity extends Orc incarnationId: candidate.incarnationId } }) - const identity = reveal?.identity - return Boolean( - identity && - runtimeWorktreeIdsEqual(identity.worktreeId, candidate.worktreeId) && - identity.tabId === candidate.tabId && - identity.leafId === candidate.leafId && - identity.ptyId === candidate.ptyId - ) + return revealedLegacyWorkerIdentityMatches(reveal?.identity, candidate) } setAutomationService(service: AutomationService): void { diff --git a/src/main/window/runtime-window-lifecycle.ts b/src/main/window/runtime-window-lifecycle.ts index 78c5f2ec426..f37787b67b0 100644 --- a/src/main/window/runtime-window-lifecycle.ts +++ b/src/main/window/runtime-window-lifecycle.ts @@ -69,12 +69,14 @@ export function registerRuntimeWindowLifecycle( title: opts.title, ...(opts.presentation ? { presentation: opts.presentation } : {}) }), - revealTerminalSession: (worktreeId, opts) => + // Why worktreeHint: ownership is tab-keyed, so the renderer decides which workspace key the + // surface is filed under. This argument only says where to look first. + revealTerminalSession: (worktreeHint, opts) => new Promise((resolve, reject) => { const requestId = randomUUID() const expectedIdentity = opts.expectedProcessIdentity ? opts.tabId && opts.leafId - ? { worktreeId, tabId: opts.tabId, leafId: opts.leafId, ptyId: opts.ptyId } + ? { tabId: opts.tabId, leafId: opts.leafId, ptyId: opts.ptyId } : null : undefined if (expectedIdentity === null) { @@ -96,10 +98,11 @@ export function registerRuntimeWindowLifecycle( reject(new Error(reply.error)) return } + // Why no worktreeId here: the renderer's answer is authoritative for the workspace key, + // and rejecting it stranded a reveal whose owner row is filed elsewhere (STA-7961). if ( expectedIdentity && (!reply.identity || - reply.identity.worktreeId !== expectedIdentity.worktreeId || reply.identity.tabId !== expectedIdentity.tabId || reply.identity.leafId !== expectedIdentity.leafId || reply.identity.ptyId !== expectedIdentity.ptyId) @@ -116,7 +119,7 @@ export function registerRuntimeWindowLifecycle( ipcMain.on('terminal:tabCreateReply', handler) const sent = send('ui:createTerminal', { requestId, - worktreeId, + worktreeId: worktreeHint, ptyId: opts.ptyId, title: opts.title ?? undefined, ...(opts.cwd ? { cwd: opts.cwd } : {}), diff --git a/src/main/window/runtime-window-reveal-identity.test.ts b/src/main/window/runtime-window-reveal-identity.test.ts new file mode 100644 index 00000000000..4a8da26735a --- /dev/null +++ b/src/main/window/runtime-window-reveal-identity.test.ts @@ -0,0 +1,212 @@ +// A reveal reply attests which pane the renderer actually bound. Ownership is tab-keyed, so the +// renderer decides which workspace key holds the row; re-asserting the caller's key here rejected +// a reveal that had surfaced exactly the right pane under a different one (STA-7961). +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { TerminalTabCreateReply } from '../../shared/terminal-reveal-identity' +import { revealedLegacyWorkerIdentityMatches } from '../runtime/legacy-worker-reveal-identity' + +let lastWebContents: unknown = null +const sentByChannel: [string, ...unknown[]][] = [] + +const { ipcMainOnMock, ipcMainRemoveListenerMock } = vi.hoisted(() => ({ + ipcMainOnMock: vi.fn(), + ipcMainRemoveListenerMock: vi.fn() +})) + +vi.mock('electron', () => ({ + ipcMain: { on: ipcMainOnMock, removeListener: ipcMainRemoveListenerMock } +})) +vi.mock('../ipc/worktree-change-invalidators', () => ({ runWorktreeChangeInvalidators: vi.fn() })) +vi.mock('./mobile-markdown-request-relay', () => ({ + requestMobileMarkdownFromRenderer: vi.fn() +})) +vi.mock('./renderer-document-navigation', () => ({ + registerRendererDocumentNavigation: vi.fn() +})) +vi.mock('./session-tab-close-request-relay', () => ({ + requestSessionTabCloseFromRenderer: vi.fn() +})) +vi.mock('./terminal-tab-close-request-relay', () => ({ + requestTerminalTabCloseFromRenderer: vi.fn() +})) +// Captures the stub webContents the most recent registration created. +vi.mock('./runtime-renderer-notification-sender', () => ({ + createRuntimeRendererNotificationSender: (args: { webContents: unknown }) => { + lastWebContents = args.webContents + return { + send: (channel: string, ...values: unknown[]) => { + sentByChannel.push([channel, ...values]) + return true + }, + onMainFrameReloadStarted: vi.fn(), + onMainFrameReloadCancelled: vi.fn(), + onMainFrameLoadFinished: vi.fn(), + onRendererProcessGone: vi.fn(), + close: vi.fn() + } + } +})) + +import { registerRuntimeWindowLifecycle } from './runtime-window-lifecycle' + +const CALLER_WORKTREE_ID = 'repo::/worktree' +const OWNER_WORKTREE_ID = 'repo::/other-worktree' + +type RevealNotifier = { + revealTerminalSession: ( + worktreeId: string, + opts: Record + ) => Promise<{ tabId: string }> +} + +/** Registers the lifecycle against stub window/runtime objects and hands back its notifier. */ +function attachNotifier(): RevealNotifier { + let notifier: RevealNotifier | null = null + const webContents = { isDestroyed: () => false, send: vi.fn(() => true), on: vi.fn() } + const mainWindow = { id: 1, isDestroyed: () => false, webContents, on: vi.fn() } + const runtime = { + attachWindow: vi.fn(), + setNotifier: vi.fn((next: RevealNotifier | null) => { + notifier = next ?? notifier + }), + markGraphReloadFailed: vi.fn(), + markGraphUnavailable: vi.fn(), + markRendererReloading: vi.fn(), + markRendererReloadCancelled: vi.fn() + } + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the stubs above cover every member registerRuntimeWindowLifecycle touches; the rest of BrowserWindow/OrcaRuntimeService is unreachable from this call. + registerRuntimeWindowLifecycle(mainWindow as never, runtime as never) + if (!notifier) { + throw new Error('Expected registerRuntimeWindowLifecycle to install a notifier') + } + return notifier +} + +/** Fires the reply the renderer would send, using the requestId the reveal generated. */ +function replyToReveal(reply: Omit): void { + const registered = ipcMainOnMock.mock.calls.at(-1) + expect(registered?.[0]).toBe('terminal:tabCreateReply') + const handler = registered?.[1] + if (typeof handler !== 'function') { + throw new Error('Expected a terminal:tabCreateReply listener') + } + const listener: (event: { sender: unknown }, reply: TerminalTabCreateReply) => void = handler + listener( + { sender: lastWebContents }, + { requestId: sentCreateTerminalPayload().requestId, ...reply } + ) +} + +function sentCreateTerminalPayload(): { requestId: string; worktreeId: string } { + const payload = sentByChannel.findLast(([channel]) => channel === 'ui:createTerminal')?.[1] + if (!isCreateTerminalPayload(payload)) { + throw new Error('Expected the reveal to send a ui:createTerminal payload') + } + return payload +} + +function isCreateTerminalPayload( + value: unknown +): value is { requestId: string; worktreeId: string } { + if (typeof value !== 'object' || value === null) { + return false + } + return ( + 'requestId' in value && + typeof value.requestId === 'string' && + 'worktreeId' in value && + typeof value.worktreeId === 'string' + ) +} + +function startReveal(): Promise<{ tabId: string }> { + const notifier = attachNotifier() + return notifier.revealTerminalSession(CALLER_WORKTREE_ID, { + ptyId: 'pty-a', + tabId: 'tab-a', + leafId: 'leaf-a', + expectedProcessIdentity: { terminalHandle: 'handle-1', incarnationId: 'inc-1' } + }) +} + +beforeEach(() => { + ipcMainOnMock.mockClear() + ipcMainRemoveListenerMock.mockClear() + sentByChannel.length = 0 + vi.spyOn(console, 'warn').mockImplementation(() => {}) +}) + +describe('revealTerminalSession identity assertion', () => { + it('resolves when the renderer files the pane under a different worktree key', async () => { + const reveal = startReveal() + + replyToReveal({ + tabId: 'tab-a', + identity: { + worktreeId: OWNER_WORKTREE_ID, + tabId: 'tab-a', + leafId: 'leaf-a', + ptyId: 'pty-a' + } + }) + + await expect(reveal).resolves.toMatchObject({ + tabId: 'tab-a', + identity: { worktreeId: OWNER_WORKTREE_ID } + }) + }) + + it.each([ + ['tabId', { tabId: 'tab-other' }], + ['leafId', { leafId: 'leaf-other' }], + ['ptyId', { ptyId: 'pty-other' }] + ])('still rejects when the reply disagrees on %s', async (_field, override) => { + const reveal = startReveal() + + replyToReveal({ + tabId: 'tab-a', + identity: { + worktreeId: CALLER_WORKTREE_ID, + tabId: 'tab-a', + leafId: 'leaf-a', + ptyId: 'pty-a', + ...override + } + }) + + await expect(reveal).rejects.toThrow('terminal_reveal_identity_mismatch') + }) + + it('still passes the caller worktree to the renderer as the hint to look under first', () => { + void startReveal().catch(() => {}) + + expect(sentCreateTerminalPayload().worktreeId).toBe(CALLER_WORKTREE_ID) + }) +}) + +describe('revealedLegacyWorkerIdentityMatches', () => { + const candidate = { tabId: 'tab-a', leafId: 'leaf-a', ptyId: 'pty-a' } + + it('accepts a reveal filed under another worktree key', () => { + // Without this the recovery rolled the surface back and the worker never materialized. + expect( + revealedLegacyWorkerIdentityMatches( + { worktreeId: OWNER_WORKTREE_ID, ...candidate }, + candidate + ) + ).toBe(true) + }) + + it('refuses a reply that names a different pane', () => { + expect( + revealedLegacyWorkerIdentityMatches( + { worktreeId: CALLER_WORKTREE_ID, ...candidate, leafId: 'leaf-other' }, + candidate + ) + ).toBe(false) + }) + + it('refuses a reply with no identity at all', () => { + expect(revealedLegacyWorkerIdentityMatches(undefined, candidate)).toBe(false) + }) +})