mirror of
https://github.com/stablyai/orca.git
synced 2026-09-21 16:02:20 +00:00
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.
This commit is contained in:
@@ -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
|
||||
)
|
||||
}
|
||||
@@ -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 {
|
||||
|
||||
@@ -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 } : {}),
|
||||
|
||||
@@ -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<string, unknown>
|
||||
) => 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<TerminalTabCreateReply, 'requestId'>): 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)
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user