Skip to content

Commit 1102530

Browse files
committed
fix(mothership): end desktop tools at sign-out and settle held reports as final
- Signing out cancels every desktop tool still running in the tab, so no local read or browser action outlives the session that started it. - Local file tools lease the turn captured at stream start, like browser actions. - The confirm route records a held_by_desktop outcome for its 409s, and the page-exit reporter treats a 409 as final. - A stale browser observation says it never started and not to retry it in this turn.
1 parent 65b247f commit 1102530

12 files changed

Lines changed: 132 additions & 14 deletions

File tree

‎apps/sim/app/api/copilot/confirm/route.test.ts‎

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,9 @@
1+
import { trace } from '@opentelemetry/api'
2+
import {
3+
BasicTracerProvider,
4+
InMemorySpanExporter,
5+
SimpleSpanProcessor,
6+
} from '@opentelemetry/sdk-trace-base'
17
import { copilotHttpMock, copilotHttpMockFns } from '@sim/testing'
28
import { encryptionMock, encryptionMockFns } from '@sim/testing/mocks/encryption.mock'
39
import {
@@ -6,7 +12,7 @@ import {
612
} from '@sim/testing/mocks/mothership-async-runs.mock'
713
import { createMockRequest } from '@sim/testing/mocks/request.mock'
814
import type { NextRequest } from 'next/server'
9-
import { beforeEach, describe, expect, it, vi } from 'vitest'
15+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
1016

1117
const { publishToolConfirmation, getTrustedWorkflowToolExecution } = vi.hoisted(() => ({
1218
publishToolConfirmation: vi.fn(),
@@ -27,6 +33,9 @@ vi.mock('@/lib/workflows/executor/execution-state', () => ({
2733
getTrustedWorkflowToolExecution,
2834
}))
2935

36+
import { CopilotConfirmOutcome } from '@/lib/mothership/generated/trace-attribute-values-v1'
37+
import { TraceAttr } from '@/lib/mothership/generated/trace-attributes-v1'
38+
import { TraceSpan } from '@/lib/mothership/generated/trace-spans-v1'
3039
import { POST } from './route'
3140

3241
const {
@@ -40,6 +49,17 @@ const {
4049

4150
const encryptSecret = encryptionMockFns.mockEncryptSecret
4251

52+
/** Records the confirm spans and returns a reader for the outcome the route recorded. */
53+
function recordConfirmOutcome(): () => unknown {
54+
const exporter = new InMemorySpanExporter()
55+
trace.setGlobalTracerProvider(
56+
new BasicTracerProvider({ spanProcessors: [new SimpleSpanProcessor(exporter)] })
57+
)
58+
return () =>
59+
exporter.getFinishedSpans().find((span) => span.name === TraceSpan.CopilotConfirmToolResult)
60+
?.attributes[TraceAttr.CopilotConfirmOutcome]
61+
}
62+
4363
describe('Copilot Confirm API Route', () => {
4464
const existingRow = {
4565
toolCallId: 'tool-call-123',
@@ -51,6 +71,10 @@ describe('Copilot Confirm API Route', () => {
5171
claimedBy: 'workflow:execution-1',
5272
}
5373

74+
afterEach(() => {
75+
trace.disable()
76+
})
77+
5478
beforeEach(() => {
5579
copilotHttpMockFns.mockAuthenticateCopilotRequestSessionOnly.mockResolvedValue({
5680
userId: 'user-1',
@@ -151,6 +175,7 @@ describe('Copilot Confirm API Route', () => {
151175
})
152176

153177
it('rejects a native success before the desktop authorization claim', async () => {
178+
const recordedOutcome = recordConfirmOutcome()
154179
getAsyncToolCall.mockResolvedValue({
155180
...existingRow,
156181
toolName: 'browser_snapshot',
@@ -166,6 +191,7 @@ describe('Copilot Confirm API Route', () => {
166191
)
167192

168193
expect(response.status).toBe(409)
194+
expect(recordedOutcome()).toBe(CopilotConfirmOutcome.HeldByDesktop)
169195
expect(completeAsyncToolCall).not.toHaveBeenCalled()
170196
expect(detachAsyncToolCall).not.toHaveBeenCalled()
171197
expect(encryptSecret).not.toHaveBeenCalled()
@@ -218,6 +244,7 @@ describe('Copilot Confirm API Route', () => {
218244
] as const)(
219245
'rejects a pending %s %s when the native authorization claim wins the race',
220246
async (toolName, status) => {
247+
const recordedOutcome = recordConfirmOutcome()
221248
getAsyncToolCall.mockResolvedValue({
222249
...existingRow,
223250
toolName,
@@ -237,6 +264,7 @@ describe('Copilot Confirm API Route', () => {
237264
expect(await response.json()).toEqual({
238265
error: 'The desktop app holds this tool call; only its own result settles it',
239266
})
267+
expect(recordedOutcome()).toBe(CopilotConfirmOutcome.HeldByDesktop)
240268
expect(completePendingAsyncToolCall).toHaveBeenCalledOnce()
241269
expect(completeClaimedAsyncToolCall).not.toHaveBeenCalled()
242270
expect(completeAsyncToolCall).not.toHaveBeenCalled()
@@ -285,6 +313,31 @@ describe('Copilot Confirm API Route', () => {
285313
}
286314
)
287315

316+
it('refuses a not-started report for a call the desktop already claimed', async () => {
317+
const recordedOutcome = recordConfirmOutcome()
318+
getAsyncToolCall.mockResolvedValue({
319+
...existingRow,
320+
toolName: 'browser_snapshot',
321+
status: 'running',
322+
claimedBy: 'desktop-browser',
323+
})
324+
325+
const response = await POST(
326+
createMockPostRequest({
327+
toolCallId: 'tool-call-123',
328+
status: 'error',
329+
message: 'The desktop action did not start.',
330+
data: { notStarted: true },
331+
})
332+
)
333+
334+
expect(response.status).toBe(409)
335+
expect(recordedOutcome()).toBe(CopilotConfirmOutcome.HeldByDesktop)
336+
expect(completeAsyncToolCall).not.toHaveBeenCalled()
337+
expect(completeClaimedAsyncToolCall).not.toHaveBeenCalled()
338+
expect(publishToolConfirmation).not.toHaveBeenCalled()
339+
})
340+
288341
it('does not publish when another terminal transition wins indeterminate claim reconciliation', async () => {
289342
getAsyncToolCall.mockResolvedValue({
290343
...existingRow,

‎apps/sim/app/api/copilot/confirm/route.ts‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,8 @@ function acknowledgeSettledToolCall(
8989
* report raced that claim and lost), so only the claim's own result settles it. Final, not
9090
* retryable: the reporter stops.
9191
*/
92-
function heldByAnotherReporterResponse(): NextResponse {
92+
function heldByAnotherReporterResponse(span: Span): NextResponse {
93+
span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.HeldByDesktop)
9394
return NextResponse.json(
9495
{ error: 'The desktop app holds this tool call; only its own result settles it' },
9596
{ status: 409 }
@@ -285,8 +286,7 @@ export const POST = withRouteHandler((req: NextRequest) => {
285286
? isWorkflowToolExecutionClaimable(existing.status, existing.permissionDecision)
286287
: existing.status === ASYNC_TOOL_STATUS.running || isPreclaimNativeTerminalOutcome
287288
if (isNativeClientTool && !isMutableClientToolCall) {
288-
span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound)
289-
return heldByAnotherReporterResponse()
289+
return heldByAnotherReporterResponse(span)
290290
}
291291
if (isWorkflowTool && !isMutableClientToolCall) {
292292
span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound)
@@ -300,8 +300,7 @@ export const POST = withRouteHandler((req: NextRequest) => {
300300
data.notStarted === true &&
301301
existing.status !== ASYNC_TOOL_STATUS.pending
302302
) {
303-
span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound)
304-
return heldByAnotherReporterResponse()
303+
return heldByAnotherReporterResponse(span)
305304
}
306305

307306
let effectiveStatus = status
@@ -437,8 +436,7 @@ export const POST = withRouteHandler((req: NextRequest) => {
437436
}
438437

439438
if (reconciledOutcome === 'conflict' && isPreclaimNativeTerminalOutcome) {
440-
span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound)
441-
return heldByAnotherReporterResponse()
439+
return heldByAnotherReporterResponse(span)
442440
}
443441

444442
if (reconciledOutcome !== 'updated') {

‎apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.test.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { describe, expect, it } from 'vitest'
22
import {
33
leaseDesktopTool,
4+
stopAllDesktopTools,
45
stopDesktopTools,
56
} from '@/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes'
67

@@ -54,4 +55,19 @@ describe('desktop tool leases', () => {
5455

5556
expect(next.signal.aborted).toBe(true)
5657
})
58+
59+
it('cancels the running tools of every turn when the session ends', () => {
60+
const first = leaseDesktopTool('turn-f')
61+
const second = leaseDesktopTool('turn-g')
62+
63+
stopAllDesktopTools('signed_out')
64+
const next = leaseDesktopTool('turn-f')
65+
66+
expect(first.signal.aborted).toBe(true)
67+
expect(second.signal.aborted).toBe(true)
68+
expect(first.signal.reason).toBe('signed_out')
69+
expect(next.signal.aborted).toBe(false)
70+
first.release()
71+
next.release()
72+
})
5773
})

‎apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ interface RunningTurnTools {
88
* Turns with a desktop tool still running in this tab, keyed by the turn's stream id. A tool
99
* outlives the chat view that started it (and any stream reader), so the turn, not the view, owns
1010
* its Stop. A turn is held only while one of its tools runs: each tool releases it as it settles.
11+
* Terminal commands take no lease: Stop ends them through the turn's resource activity, which
12+
* clears the agent's commands in each scope it touched.
1113
*/
1214
const runningTurns = new Map<string, RunningTurnTools>()
1315

@@ -49,3 +51,9 @@ export function stopDesktopTools(streamId: string, reason: string): void {
4951
runningTurns.get(streamId)?.stop.abort(reason)
5052
runningTurns.delete(streamId)
5153
}
54+
55+
/** Cancels every running desktop tool in this tab, so none outlives the session that started it. */
56+
export function stopAllDesktopTools(reason: string): void {
57+
for (const turn of runningTurns.values()) turn.stop.abort(reason)
58+
runningTurns.clear()
59+
}

‎apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1577,7 +1577,12 @@ export function useChat(
15771577
)
15781578

15791579
const startClientLocalFilesystemTool = useCallback(
1580-
(toolCallId: string, toolName: string, toolArgs: Record<string, unknown>) => {
1580+
(
1581+
toolCallId: string,
1582+
toolName: string,
1583+
toolArgs: Record<string, unknown>,
1584+
turnStreamId: string | undefined
1585+
) => {
15811586
if (
15821587
!isNativeFileTool(toolName) &&
15831588
(!workspaceId || !isUserLocalVfsToolCall(toolName, toolArgs))
@@ -1588,7 +1593,7 @@ export function useChat(
15881593
return
15891594
}
15901595
handledClientLocalFilesystemToolIds.add(toolCallId)
1591-
const lease = streamIdRef.current ? leaseDesktopTool(streamIdRef.current) : undefined
1596+
const lease = turnStreamId ? leaseDesktopTool(turnStreamId) : undefined
15921597
const options = {
15931598
workspaceId,
15941599
chatId: chatIdRef.current ?? selectedChatIdRef.current,
@@ -2226,7 +2231,8 @@ export function useChat(
22262231
addResource,
22272232
removeResource,
22282233
startClientWorkflowTool,
2229-
startClientLocalFilesystemTool,
2234+
startClientLocalFilesystemTool: (toolCallId, toolName, toolArgs) =>
2235+
startClientLocalFilesystemTool(toolCallId, toolName, toolArgs, turnStreamId),
22302236
startClientBrowserTool: startClientBrowserToolForStream,
22312237
startClientTerminalTool: startClientTerminalToolForStream,
22322238
startBrowserAgentRun: startBrowserAgentRunForStream,

‎apps/sim/lib/mothership/generated/trace-attribute-values-v1.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,7 @@ export type CopilotChatPersistOutcomeValue =
115115
export const CopilotConfirmOutcome = {
116116
Delivered: 'delivered',
117117
Forbidden: 'forbidden',
118+
HeldByDesktop: 'held_by_desktop',
118119
InternalError: 'internal_error',
119120
RunNotFound: 'run_not_found',
120121
ToolCallNotFound: 'tool_call_not_found',

‎apps/sim/lib/mothership/tools/client/browser-tool-execution.test.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1470,9 +1470,12 @@ describe('pre-dispatch drops still resolve the waiter', () => {
14701470
expect(mockReportCompletion).toHaveBeenCalledWith(
14711471
'stale-call-1',
14721472
'error',
1473-
expect.stringContaining('too late'),
1473+
expect.stringContaining('never started'),
14741474
expect.objectContaining({ staleEvent: true })
14751475
)
1476+
const [, , message] = mockReportCompletion.mock.calls[0] ?? []
1477+
expect(message).toContain('Do not retry it in this turn')
1478+
expect(message).toContain('keep this chat open in the Sim desktop app')
14761479
})
14771480

14781481
it('marks a stale stateful event outcome unknown and unsafe to retry', async () => {

‎apps/sim/lib/mothership/tools/client/browser-tool-execution.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,8 @@ const OUTCOME_UNKNOWN_MESSAGE =
109109
'The Sim window closed while this browser action was in flight. It may already have taken effect. Do not retry it automatically; take a fresh browser snapshot before deciding what to do.'
110110
const REPLAY_OUTCOME_UNKNOWN_MESSAGE =
111111
'This browser action was recorded before the Sim page reloaded, but its terminal result could not be recovered. It may already have taken effect. Do not retry it automatically; take a fresh browser snapshot before deciding what to do.'
112+
const STALE_OBSERVATION_NOT_STARTED_MESSAGE =
113+
'Not run: this browser observation never started, because it reached the Sim desktop app too late to run safely. Nothing happened in the browser. Do not retry it in this turn; tell the user to keep this chat open in the Sim desktop app, or to ask again later.'
112114
const STALE_STATEFUL_OUTCOME_UNKNOWN_MESSAGE =
113115
'This browser action was delivered too late to recover its exact result. It may already have taken effect. Do not retry it automatically; take a fresh browser snapshot before deciding what to do.'
114116
const REPLAY_GUARD_CAPACITY_MESSAGE =
@@ -593,7 +595,7 @@ export async function executeBrowserToolOnClient(
593595
logger.info('Skipping stale browser tool event', { toolCallId, toolName, age })
594596
const observationOnly = OBSERVATION_ONLY_BROWSER_TOOLS[toolName]
595597
const message = observationOnly
596-
? 'This browser observation was delivered too late to run safely. Ask again to retry it.'
598+
? STALE_OBSERVATION_NOT_STARTED_MESSAGE
597599
: STALE_STATEFUL_OUTCOME_UNKNOWN_MESSAGE
598600
retainAndReportTerminalCompletion(
599601
toolCallId,

‎apps/sim/lib/mothership/tools/client/completion.test.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,15 @@ describe('client tool completion reporting', () => {
115115
expect(signal?.aborted).toBe(true)
116116
})
117117

118+
it('treats a 409 as final: the desktop app holds the call', async () => {
119+
fetchMock.mockResolvedValue(new Response(null, { status: 409 }))
120+
121+
await expect(
122+
reportClientToolCompletionOnPageExit('tool-1', 'error', 'Browser failed')
123+
).resolves.toBeUndefined()
124+
expect(fetchMock).toHaveBeenCalledOnce()
125+
})
126+
118127
it('rejects a non-success response', async () => {
119128
fetchMock.mockResolvedValue(new Response(null, { status: 503 }))
120129

‎apps/sim/lib/mothership/tools/client/completion.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ export async function reportClientToolCompletionOnPageExit(
145145
}),
146146
keepalive: true,
147147
})
148-
if (!response.ok) {
148+
if (!isSettledDelivery(response, toolCallId)) {
149149
throw new CompletionReportError(`Page-exit completion failed with status ${response.status}`)
150150
}
151151
}

0 commit comments

Comments
 (0)