diff --git a/docs/computer-use-runtime-hardening.md b/docs/computer-use-runtime-hardening.md new file mode 100644 index 0000000000..b882569404 --- /dev/null +++ b/docs/computer-use-runtime-hardening.md @@ -0,0 +1,33 @@ +# Computer Use Runtime Hardening + +This follow-up addresses lifecycle gaps found during review of PR #892. + +## Problems + +- `clearSession()` did not create a stop tombstone when no session-state record + existed yet, so a first queued invocation could activate after cleanup. +- Read-only host actions did not acquire a session lease and could continue + after `user_stopped`. +- Later lifecycle events could overwrite `blocked_url` or `user_stopped`. + +## Root Cause + +The Runtime treated observation and mutation leases as the only operations that +needed lifecycle fencing. Cleanup also mutated only an already-created state +record, while terminal transitions shared the same unrestricted transition +helper as recoverable states. + +## Fix + +- Create the same-turn stop tombstone unconditionally during `clearSession()`. +- Require an observation lease for every host-reading or waiting action. +- Make `blocked_url` and `user_stopped` absorb later lifecycle events. + +A new turn still creates a fresh Computer Use session state, preserving the +existing explicit recovery boundary. + +## Verification + +- `npm --workspace @maka/runtime run typecheck` +- focused Computer Use and session-state tests: 52 passed +- `git diff --check` diff --git a/packages/runtime/src/__tests__/computer-use-tools.test.ts b/packages/runtime/src/__tests__/computer-use-tools.test.ts index a1b4482fa9..057c4d7150 100644 --- a/packages/runtime/src/__tests__/computer-use-tools.test.ts +++ b/packages/runtime/src/__tests__/computer-use-tools.test.ts @@ -1292,6 +1292,99 @@ describe('buildComputerUseTools — the `maka_computer` MakaTool', () => { assert.doesNotMatch(nextTurn.text, /user_stopped/); }); + test('clearSession fences a first invocation that is already queued', async () => { + let observeAppCalls = 0; + const backend = fakeBackend() as CuDispatchBackend & { + observeApp: NonNullable; + }; + backend.observeApp = async () => { + observeAppCalls += 1; + return observation(); + }; + const tools = buildComputerUseTools({ backend }); + const tool = tools[0]; + + const pending = tool.impl({ + action: 'observe', + app: 'Fixture', + } as never, ctx()); + tools.clearSession('s1'); + + const result = await pending as { text: string }; + assert.match(result.text, /user_stopped/); + assert.equal(observeAppCalls, 0); + }); + + test('clearSession after a non-CU turn does not block the next turn observe', async () => { + let observeAppCalls = 0; + const backend = fakeBackend() as CuDispatchBackend & { + observeApp: NonNullable; + }; + backend.observeApp = async () => { + observeAppCalls += 1; + return observation(); + }; + const tools = buildComputerUseTools({ backend }); + const tool = tools[0]; + + tools.clearSession('s1'); + const result = await tool.impl( + { action: 'observe', app: 'Fixture' } as never, + ctx(undefined, { turnId: 'next-turn', toolCallId: 'observe-next' }), + ) as { text: string }; + + assert.doesNotMatch(result.text, /user_stopped/); + assert.equal(observeAppCalls, 1); + }); + + test('clearSession fences host-reading results that complete after stop', async () => { + for (const input of [ + { action: 'list_apps' }, + { action: 'screenshot', app: 'Fixture' }, + { action: 'cursor_position' }, + { action: 'wait', duration: 0.001 }, + ] as const) { + let release!: () => void; + let started!: () => void; + const gate = new Promise((resolve) => { release = resolve; }); + const entered = new Promise((resolve) => { started = resolve; }); + const backend = fakeBackend() as CuDispatchBackend & { + listApps: NonNullable; + observeApp: NonNullable; + }; + backend.listApps = async () => { + started(); + await gate; + return []; + }; + backend.observeApp = async () => { + started(); + await gate; + return observation(); + }; + backend.run = async (action) => { + started(); + await gate; + return action.type === 'cursor_position' + ? { + outcome: { ok: true, tier: 'coordinate-background' }, + resolvedScreenPoint: { x: 10, y: 20 }, + } + : { outcome: { ok: true, tier: 'coordinate-background' } }; + }; + const tools = buildComputerUseTools({ backend }); + const tool = tools[0]; + const pending = tool.impl(input as never, ctx()); + await entered; + tools.clearSession('s1'); + release(); + + const result = await pending as { text: string; screenshot?: unknown }; + assert.match(result.text, /user_stopped/, input.action); + assert.equal(result.screenshot, undefined, input.action); + } + }); + test('S17: surfaces the typed backend failure code without leaking raw driver text', async () => { const backend = fakeBackend({ result: { outcome: { ok: false, error: 'capture_failed', message: 'AXPress err -25202', completedSubSteps: 0 } } }); const r = await callComputer(backend, { action: 'wait' }); diff --git a/packages/runtime/src/__tests__/cua-session-state.test.ts b/packages/runtime/src/__tests__/cua-session-state.test.ts index 75cd976d42..776c2b3f8c 100644 --- a/packages/runtime/src/__tests__/cua-session-state.test.ts +++ b/packages/runtime/src/__tests__/cua-session-state.test.ts @@ -129,6 +129,30 @@ describe('CuaSessionState', () => { }); }); + test('terminal states absorb later lifecycle events', () => { + const blocked = new CuaSessionState('blocked'); + blocked.blockedUrlDetected(); + blocked.screenLocked(); + blocked.reobserveRequired(); + blocked.physicalUserIntervened(); + blocked.userStopped(); + assert.deepEqual(blocked.snapshot(), { + status: 'blocked_url', + generation: 1, + }); + + const stopped = new CuaSessionState('stopped'); + stopped.userStopped(); + stopped.blockedUrlDetected(); + stopped.screenLocked(); + stopped.reobserveRequired(); + stopped.physicalUserIntervened(); + assert.deepEqual(stopped.snapshot(), { + status: 'user_stopped', + generation: 1, + }); + }); + test('dynamic content changes neither synthesize intervention nor fence a lease', () => { const state = new CuaSessionState('session-1'); state.freshObservationSucceeded(); diff --git a/packages/runtime/src/computer-use-tools.ts b/packages/runtime/src/computer-use-tools.ts index a89c025ff4..6bc5821eb9 100644 --- a/packages/runtime/src/computer-use-tools.ts +++ b/packages/runtime/src/computer-use-tools.ts @@ -567,6 +567,7 @@ export function buildComputerUseTools(deps: { const presentationWaiters = new Map void>>(); const presentationQueueWaiters = new Map void>>(); const presentationGenerations = new Map(); + const pendingInvocationTurns = new Map>(); let presentationQueue = Promise.resolve(); interface SessionObservationRecord { turnId: string; @@ -611,6 +612,16 @@ export function buildComputerUseTools(deps: { return next; } + function trackPendingInvocation(sessionId: string, turnId: string): () => void { + const turns = pendingInvocationTurns.get(sessionId) ?? new Set(); + turns.add(turnId); + pendingInvocationTurns.set(sessionId, turns); + return () => { + turns.delete(turnId); + if (turns.size === 0) pendingInvocationTurns.delete(sessionId); + }; + } + function invalidateObservation(sessionId: string): void { const record = observations.get(sessionId); if (!record) return; @@ -1079,9 +1090,18 @@ export function buildComputerUseTools(deps: { if (abortSignal.aborted) return { text: 'computer aborted before start' }; const input = snapshotComputerParams(computerParams.parse(args)); const invocationGeneration = presentationGenerations.get(sessionId) ?? 0; - return withInvocationQueue(sessionId, abortSignal, async () => { + const releasePendingInvocation = trackPendingInvocation(sessionId, turnId); + try { + return await withInvocationQueue(sessionId, abortSignal, async () => { const state = sessionState(sessionId, turnId); - const observationLease = input.action === 'observe' + const requiresObservationLease = ( + input.action === 'observe' + || input.action === 'screenshot' + || input.action === 'list_apps' + || input.action === 'cursor_position' + || input.action === 'wait' + ); + const observationLease = requiresObservationLease ? state.beforeObservation() : undefined; if (observationLease && !observationLease.ok) { @@ -1125,6 +1145,15 @@ export function buildComputerUseTools(deps: { return { text: 'maka_computer.list_apps failed: unsupported_action' }; } const apps = await deps.backend.listApps(abortSignal); + if ( + !observationLease?.ok + || !state.validateObservationLease(observationLease.lease).ok + ) { + const blocked = state.beforeAction(); + return sessionFailure( + blocked.ok ? 'reobserve_required' : blocked.reason, + ); + } return { text: JSON.stringify({ app_count: apps.length, @@ -1208,6 +1237,15 @@ export function buildComputerUseTools(deps: { windowId: input.window_id, includeScreenshot: true, }, abortSignal, runCtx); + if ( + !observationLease?.ok + || !state.validateObservationLease(observationLease.lease).ok + ) { + const blocked = state.beforeAction(); + return sessionFailure( + blocked.ok ? 'reobserve_required' : blocked.reason, + ); + } if (!screenshotObservation.screenshot) { return { text: 'maka_computer.screenshot failed: capture_failed' }; } @@ -1443,6 +1481,15 @@ export function buildComputerUseTools(deps: { if (presentation.blocked) return presentation.blocked; result = presentation.result; if (result) applyTypedOutcomeState(state, result.outcome); + if (observationLease?.ok) { + const validated = state.validateObservationLease( + observationLease.lease, + ); + if (!validated.ok) { + presentation.finish(); + return sessionFailure(validated.reason); + } + } if (actionLease) { const leaseFailure = validateActionLease(state, actionLease); if (leaseFailure) { @@ -1511,7 +1558,10 @@ export function buildComputerUseTools(deps: { } : { text, modelText }; } - }); + }); + } finally { + releasePendingInvocation(); + } }, // Map the raw result into model-visible content: the summary as text, plus the // screenshot as a native image block when present. `image-data` becomes the @@ -1546,7 +1596,13 @@ export function buildComputerUseTools(deps: { ); for (const wake of presentationQueueWaiters.get(sessionId) ?? []) wake(); for (const wake of presentationWaiters.get(sessionId) ?? []) wake(); - sessionStates.get(sessionId)?.state.userStopped(); + const current = sessionStates.get(sessionId); + if (current) { + current.state.userStopped(); + } else { + const pendingTurn = pendingInvocationTurns.get(sessionId)?.values().next().value; + if (pendingTurn) sessionState(sessionId, pendingTurn).userStopped(); + } invalidateObservation(sessionId); observations.delete(sessionId); deps.backend.clearSession?.(sessionId); diff --git a/packages/runtime/src/cua-session-state.ts b/packages/runtime/src/cua-session-state.ts index 53769c7ee3..324ea5fbf3 100644 --- a/packages/runtime/src/cua-session-state.ts +++ b/packages/runtime/src/cua-session-state.ts @@ -82,6 +82,7 @@ export class CuaSessionState { } physicalUserIntervened(): CuaSessionSnapshot { + if (this.isTerminal()) return this.snapshot(); return this.transition('intervention_debounce'); } @@ -92,10 +93,12 @@ export class CuaSessionState { } reobserveRequired(): CuaSessionSnapshot { + if (this.isTerminal()) return this.snapshot(); return this.transition('reobserve_required'); } screenLocked(): CuaSessionSnapshot { + if (this.isTerminal()) return this.snapshot(); return this.transition('screen_locked'); } @@ -106,10 +109,12 @@ export class CuaSessionState { } blockedUrlDetected(): CuaSessionSnapshot { + if (this.isTerminal()) return this.snapshot(); return this.transition('blocked_url'); } userStopped(): CuaSessionSnapshot { + if (this.isTerminal()) return this.snapshot(); return this.transition('user_stopped'); } @@ -128,6 +133,10 @@ export class CuaSessionState { || this.status === 'reobserve_required'; } + private isTerminal(): boolean { + return this.status === 'blocked_url' || this.status === 'user_stopped'; + } + private transition(status: CuaSessionStatus): CuaSessionSnapshot { this.generation += 1; this.status = status;