From fe99c8391d54f250982ec1064658a1e3aaefbe69 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 14 Aug 2026 15:19:55 -0700 Subject: [PATCH] fix(pi): bind the titlebar spinner to its session incarnation (STA-4246) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 80ms spinner frame called pi.getSessionName() on the module-captured handle. After newSession/fork/switchSession/reload that handle is invalid, so the next frame threw pi's assertActive guard out of a setInterval callback — a top-level frame with no caller to catch it — and the uncaughtException killed the agent process. Fix the lifetime, not the throw: read the session name and cwd once per turn so the frame never touches a replaceable handle, unbind on session_start (the runtime's new/fork/switch/resume/reload signal) so no frame can run against a replaced ctx, and re-arm on the next agent_start with the incoming ctx. A failed title write unbinds rather than escalating, since a timer callback can never let a throw reach a catch. --- .../pi/titlebar-extension-service.test.ts | 22 +++ src/main/pi/titlebar-extension-source.test.ts | 142 ++++++++++++++++++ src/main/pi/titlebar-extension-source.ts | 82 ++++++++-- .../pi/titlebar-extension-test-harness.ts | 142 ++++++++++++++++++ 4 files changed, 376 insertions(+), 12 deletions(-) create mode 100644 src/main/pi/titlebar-extension-source.test.ts create mode 100644 src/main/pi/titlebar-extension-test-harness.ts diff --git a/src/main/pi/titlebar-extension-service.test.ts b/src/main/pi/titlebar-extension-service.test.ts index e197a4a0c3a..232dee87c8d 100644 --- a/src/main/pi/titlebar-extension-service.test.ts +++ b/src/main/pi/titlebar-extension-service.test.ts @@ -181,6 +181,28 @@ describe('PiTitlebarExtensionService', () => { expectPiHomeIntact() }) + // Why: the spinner ships to both Pi and OMP, which share the session-replacement + // lifecycle that crashed the agent — so the fix has to reach both launch paths, + // not just Pi's (STA-4246). Prime's exclusion is asserted in the Prime spec above. + it('installs the session-replacement-safe titlebar spinner for both Pi and OMP', () => { + const svc = new PiTitlebarExtensionService() + + for (const kind of ['pi', 'omp'] as const) { + svc.buildPtyEnv(`pty-spinner-${kind}`, piHome, kind) + const source = readFileSync(join(piHome, 'extensions', 'orca-titlebar-spinner.ts'), 'utf-8') + expect(source).toContain("pi.on('session_start'") + + // The crashing frame read the session handle on every tick; it must not anymore. + const bodyStart = source.indexOf('timer = setInterval(') + const bodyEnd = source.indexOf('}, FRAME_INTERVAL_MS)') + expect(bodyStart).toBeGreaterThan(-1) + expect(bodyEnd).toBeGreaterThan(bodyStart) + expect(source.slice(bodyStart, bodyEnd)).not.toContain('pi.getSessionName()') + } + + expectPiHomeIntact() + }) + it('uses the same source dir for multiple PTYs with the same Pi dir', () => { const svc = new PiTitlebarExtensionService() const firstEnv = svc.buildPtyEnv('pty-shared-1', piHome, 'pi') diff --git a/src/main/pi/titlebar-extension-source.test.ts b/src/main/pi/titlebar-extension-source.test.ts new file mode 100644 index 00000000000..715636fd105 --- /dev/null +++ b/src/main/pi/titlebar-extension-source.test.ts @@ -0,0 +1,142 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +import { createHarness } from './titlebar-extension-test-harness' + +describe('generated titlebar spinner extension', () => { + beforeEach(() => { + vi.useFakeTimers() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('registers no hooks outside an Orca pane', () => { + expect(createHarness({ paneKey: undefined }).handlers).toEqual({}) + }) + + it('animates the title through a turn and restores the base title on agent_end', async () => { + const harness = createHarness() + + await harness.callHook('agent_start') + await vi.advanceTimersByTimeAsync(240) + + expect(harness.titles).toEqual([ + '⠋ π - work - project', + '⠙ π - work - project', + '⠹ π - work - project' + ]) + + await harness.callHook('agent_end') + expect(vi.getTimerCount()).toBe(0) + expect(harness.titles.at(-1)).toBe('π - work - project') + }) + + // The reported crash: getSessionName() ran on every frame, so a session + // replacement mid-turn threw out of a timer callback and killed the agent. + it('never reads the session handle from inside the animation frame', async () => { + const harness = createHarness() + + await harness.callHook('agent_start') + const readsAfterStart = harness.getSessionName.mock.calls.length + expect(readsAfterStart).toBe(1) + + await vi.advanceTimersByTimeAsync(4000) + expect(harness.titles.length).toBeGreaterThan(40) + expect(harness.getSessionName).toHaveBeenCalledTimes(readsAfterStart) + }) + + it('unbinds on session_start before any frame can run against the replaced ctx', async () => { + const harness = createHarness() + + await harness.callHook('agent_start') + await vi.advanceTimersByTimeAsync(80) + expect(vi.getTimerCount()).toBe(1) + + await harness.callHook('session_start') + expect(vi.getTimerCount()).toBe(0) + + const titlesAtReplacement = harness.titles.length + await vi.advanceTimersByTimeAsync(800) + expect(harness.titles.length).toBe(titlesAtReplacement) + }) + + it('re-arms the spinner on the next turn after a session replacement', async () => { + const harness = createHarness() + + await harness.callHook('agent_start') + await vi.advanceTimersByTimeAsync(80) + await harness.callHook('session_start') + harness.reviveHandle('forked') + + const replacementCtx = { ui: { setTitle: vi.fn() } } + await harness.callHook('agent_start', replacementCtx) + await vi.advanceTimersByTimeAsync(160) + + expect(vi.getTimerCount()).toBe(1) + expect(replacementCtx.ui.setTitle.mock.calls.map(([title]) => title)).toEqual([ + '⠋ π - forked - project', + '⠙ π - forked - project' + ]) + }) + + it('unbinds instead of crashing when a frame writes through a replaced ctx', async () => { + const harness = createHarness() + const staleCtx = harness.createStaleContext() + + await harness.callHook('agent_start', staleCtx) + // A throw from the frame surfaces here as a rejection — and in production as + // the uncaughtException that took the whole agent process down. + await vi.advanceTimersByTimeAsync(80) + + expect(vi.getTimerCount()).toBe(0) + await vi.advanceTimersByTimeAsync(800) + expect(vi.getTimerCount()).toBe(0) + }) + + it('degrades to a cwd-only title when the captured handle goes stale, without re-probing it', async () => { + const harness = createHarness({ sessionName: undefined }) + harness.staleHandle() + + await harness.callHook('agent_start') + await vi.advanceTimersByTimeAsync(160) + + expect(harness.titles).toEqual(['⠋ π - project', '⠙ π - project']) + // Latched after the first throw: a dead handle is not retried every turn boundary. + expect(harness.getSessionName).toHaveBeenCalledTimes(1) + + await harness.callHook('agent_end') + expect(harness.titles.at(-1)).toBe('π - project') + expect(harness.getSessionName).toHaveBeenCalledTimes(1) + }) + + it('keeps the last known session name when the handle goes stale mid-turn', async () => { + const harness = createHarness() + + await harness.callHook('agent_start') + harness.staleHandle() + await vi.advanceTimersByTimeAsync(80) + + await harness.callHook('agent_end') + expect(harness.titles.at(-1)).toBe('π - work - project') + }) + + it('settles session_shutdown when both the handle and the ctx are already stale', async () => { + const harness = createHarness() + harness.staleHandle() + + await expect( + harness.callHook('session_shutdown', harness.createStaleContext()) + ).resolves.toBeUndefined() + expect(vi.getTimerCount()).toBe(0) + }) + + it('unrefs the animation interval so it cannot hold a shutting-down agent alive', async () => { + const harness = createHarness() + + await harness.callHook('agent_start') + + expect(harness.intervalUnrefs).toHaveLength(1) + expect(harness.intervalUnrefs[0]).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/main/pi/titlebar-extension-source.ts b/src/main/pi/titlebar-extension-source.ts index 6070d35a54a..601fee66265 100644 --- a/src/main/pi/titlebar-extension-source.ts +++ b/src/main/pi/titlebar-extension-source.ts @@ -15,36 +15,81 @@ export function getPiTitlebarExtensionSource(): string { " '\\u280f'", ']', '', - 'function getBaseTitle(pi) {', - ' const cwd = process.cwd().split(/[\\\\/]/).filter(Boolean).at(-1) || process.cwd()', - ' const session = pi.getSessionName()', - ' return session ? `\\u03c0 - ${session} - ${cwd}` : `\\u03c0 - ${cwd}`', + 'const FRAME_INTERVAL_MS = 80', + '', + 'function getCwdLabel() {', + ' return process.cwd().split(/[\\\\/]/).filter(Boolean).at(-1) || process.cwd()', + '}', + '', + 'function formatTitle(frame, session, cwd) {', + ' const prefix = frame ? `${frame} \\u03c0` : `\\u03c0`', + ' return session ? `${prefix} - ${session} - ${cwd}` : `${prefix} - ${cwd}`', '}', '', 'export default function (pi) {', ' if (!process.env.ORCA_PANE_KEY) return', ' let timer = null', ' let frameIndex = 0', + ' let sessionLabel = null', + ' let cwdLabel = getCwdLabel()', + ' let handleStale = false', '', - ' function stopAnimation(ctx) {', + ' // Why: the captured pi handle is invalidated by newSession/fork/switchSession/', + ' // reload. Read the name once per turn instead of on every frame, and latch the', + ' // failure so a dead handle is not re-probed 12x a second. The last good label', + ' // is kept so the title degrades to a stale name rather than losing it.', + ' function refreshSessionLabel() {', + ' if (handleStale) return', + ' try {', + ' sessionLabel = pi.getSessionName()', + ' } catch {', + ' handleStale = true', + ' }', + ' }', + '', + ' // Why: setTitle throws on a replaced ctx too. Report the failure instead of', + ' // propagating so each caller can decide whether to unbind.', + ' function writeTitle(ctx, title) {', + ' try {', + ' ctx.ui.setTitle(title)', + ' return true', + ' } catch {', + ' return false', + ' }', + ' }', + '', + ' function clearTimer() {', ' if (timer) {', ' clearInterval(timer)', ' timer = null', ' }', ' frameIndex = 0', - ' ctx.ui.setTitle(getBaseTitle(pi))', + ' }', + '', + ' function stopAnimation(ctx) {', + ' clearTimer()', + ' refreshSessionLabel()', + " writeTitle(ctx, formatTitle('', sessionLabel, cwdLabel))", ' }', '', ' function startAnimation(ctx) {', - ' stopAnimation(ctx)', + ' clearTimer()', + ' cwdLabel = getCwdLabel()', + ' refreshSessionLabel()', ' timer = setInterval(() => {', ' const frame = BRAILLE_FRAMES[frameIndex % BRAILLE_FRAMES.length]', - ' const cwd = process.cwd().split(/[\\\\/]/).filter(Boolean).at(-1) || process.cwd()', - ' const session = pi.getSessionName()', - ' const title = session ? `${frame} \\u03c0 - ${session} - ${cwd}` : `${frame} \\u03c0 - ${cwd}`', - ' ctx.ui.setTitle(title)', + ' // Why: a timer callback is a top-level frame with no caller to catch it,', + ' // so any throw here is an uncaughtException that takes the agent process', + ' // down. Unbind on the first failed write instead of escalating.', + ' if (!writeTitle(ctx, formatTitle(frame, sessionLabel, cwdLabel))) {', + ' clearTimer()', + ' return', + ' }', ' frameIndex++', - ' }, 80)', + ' }, FRAME_INTERVAL_MS)', + ' // Why: a cosmetic title animation must never hold the event loop open and', + ' // keep ticking against a session that is already shutting down.', + " if (typeof timer.unref === 'function') timer.unref()", ' }', '', " pi.on('agent_start', async (_event, ctx) => {", @@ -58,6 +103,19 @@ export function getPiTitlebarExtensionSource(): string { " pi.on('session_shutdown', async (_event, ctx) => {", ' stopAnimation(ctx)', ' })', + '', + ' // Why: session_start is the runtime signal for new/fork/switch/resume/reload —', + ' // exactly the replacements that invalidate the ctx an in-flight animation is', + ' // bound to. Unbind here so no frame ever runs against a replaced session, and', + ' // re-probe the handle in case the replacement revived it. The next agent_start', + ' // re-arms the spinner with the incoming ctx.', + " pi.on('session_start', async (_event, ctx) => {", + ' clearTimer()', + ' handleStale = false', + ' cwdLabel = getCwdLabel()', + ' refreshSessionLabel()', + " writeTitle(ctx, formatTitle('', sessionLabel, cwdLabel))", + ' })', '}', '' ].join('\n') diff --git a/src/main/pi/titlebar-extension-test-harness.ts b/src/main/pi/titlebar-extension-test-harness.ts new file mode 100644 index 00000000000..0b825fe85e7 --- /dev/null +++ b/src/main/pi/titlebar-extension-test-harness.ts @@ -0,0 +1,142 @@ +// Runs the generated titlebar spinner extension inside a VM sandbox with a +// stubbed pi handle and hook ctx, so specs assert what a real Pi/OMP process +// would render — and what it does when the runtime invalidates either one. +// Mirrors agent-status-extension-test-harness.ts for the sibling extension. +import { runInNewContext } from 'node:vm' +// TypeScript 7 is a native CLI; transpile tests still need the legacy JavaScript API. +import ts from 'typescript-api' +import { vi } from 'vitest' + +import { getPiTitlebarExtensionSource } from './titlebar-extension-source' + +export type HookContext = { + ui: { setTitle: (title: string) => void } +} + +export type HookHandler = (event?: unknown, context?: HookContext) => Promise | void + +export type Harness = { + handlers: Record + /** Titles written through the live ctx, in order. */ + titles: string[] + setTitle: ReturnType + getSessionName: ReturnType + /** Invoke a hook with the live ctx (or an explicit replacement ctx). */ + callHook: (name: string, context?: HookContext) => Promise + /** Build a ctx whose setTitle throws, as pi does once its session is replaced. */ + createStaleContext: () => HookContext + /** Make the captured `pi` handle throw, as pi's assertActive guard does. */ + staleHandle: () => void + reviveHandle: (sessionName?: string) => void + /** `unref` spies for every interval the extension has armed, in order. */ + intervalUnrefs: ReturnType[] +} + +const STALE_CTX_ERROR = + 'This extension ctx is stale after session replacement or reload. Do not use a captured pi or command ctx after ctx.newSession(), ctx.fork(), ctx.switchSession(), or ctx.reload().' + +export function createHarness(args?: { + paneKey?: string + cwd?: string + sessionName?: string +}): Harness { + const cwd = args?.cwd ?? '/Users/me/project' + let sessionName: string | undefined = args?.sessionName ?? 'work' + let handleIsStale = false + + const getSessionName = vi.fn(() => { + if (handleIsStale) { + throw new Error(STALE_CTX_ERROR) + } + return sessionName + }) + + const titles: string[] = [] + const setTitle = vi.fn((title: string) => { + titles.push(title) + }) + + const module = { + exports: {} as { + default?: (pi: { + getSessionName: () => string | undefined + on: (name: string, handler: HookHandler) => void + }) => void + } + } + + // Why: the extension unrefs its interval so a cosmetic animation cannot hold a + // shutting-down agent alive. Spy on the real handle so that stays assertable. + const intervalUnrefs: ReturnType[] = [] + const trackedSetInterval = (...params: Parameters): unknown => { + const handle = setInterval(...params) as unknown as { unref?: () => unknown } + if (handle && typeof handle === 'object') { + const original = typeof handle.unref === 'function' ? handle.unref.bind(handle) : () => handle + const spy = vi.fn(original) + handle.unref = spy + intervalUnrefs.push(spy) + } + return handle + } + + const context = { + module, + exports: module.exports, + process: { + env: + 'paneKey' in (args ?? {}) ? { ORCA_PANE_KEY: args?.paneKey } : { ORCA_PANE_KEY: 'pane-1' }, + cwd: () => cwd + }, + setInterval: trackedSetInterval, + clearInterval, + console: { warn: vi.fn(), error: vi.fn(), log: vi.fn() } + } as Record + context.globalThis = context + + const output = ts.transpileModule(getPiTitlebarExtensionSource(), { + compilerOptions: { module: ts.ModuleKind.CommonJS, target: ts.ScriptTarget.ES2020 } + }).outputText + runInNewContext(output, context) + + const register = module.exports.default + if (!register) { + throw new Error('expected default export from generated source') + } + + const handlers: Record = {} + register({ + getSessionName, + on(name: string, handler: HookHandler) { + handlers[name] = handler + } + }) + + const liveContext: HookContext = { ui: { setTitle } } + + return { + handlers, + titles, + setTitle, + getSessionName, + callHook: async (name, hookContext) => { + await handlers[name]?.(undefined, hookContext ?? liveContext) + }, + createStaleContext: () => ({ + ui: { + setTitle: () => { + throw new Error(STALE_CTX_ERROR) + } + } + }), + staleHandle: () => { + handleIsStale = true + }, + reviveHandle: (nextSessionName) => { + handleIsStale = false + if (nextSessionName !== undefined) { + sessionName = nextSessionName + } + }, + intervalUnrefs + } +}