mirror of
https://github.com/stablyai/orca.git
synced 2026-10-22 08:03:37 +00:00
fix(pi): bind the titlebar spinner to its session incarnation (STA-4246)
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.
This commit is contained in:
Brennan Benson
@@ -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')
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
})
|
||||
@@ -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')
|
||||
|
||||
@@ -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> | void
|
||||
|
||||
export type Harness = {
|
||||
handlers: Record<string, HookHandler>
|
||||
/** Titles written through the live ctx, in order. */
|
||||
titles: string[]
|
||||
setTitle: ReturnType<typeof vi.fn>
|
||||
getSessionName: ReturnType<typeof vi.fn>
|
||||
/** Invoke a hook with the live ctx (or an explicit replacement ctx). */
|
||||
callHook: (name: string, context?: HookContext) => Promise<void>
|
||||
/** 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<typeof vi.fn>[]
|
||||
}
|
||||
|
||||
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<typeof vi.fn>[] = []
|
||||
const trackedSetInterval = (...params: Parameters<typeof setInterval>): 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<string, unknown>
|
||||
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<string, HookHandler> = {}
|
||||
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
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user