Skip to content

Commit 9af89fc

Browse files
authored
fix(mothership): fail unclaimed desktop calls fast and stop dropping them silently (#8652)
* fix(mothership): fail unclaimed desktop calls fast and stop dropping them silently A desktop call reaches the user's machine only through the chat view showing that chat, so one issued while the user is elsewhere was never claimed and the turn waited out a 90-160 s watchdog that called it hung. One desktop wait now fails a call still unclaimed after a 15 s pickup grace as never started, with the inverse of the desktop's claim (pending -> failed) and a result sealed like any client completion. A call claimed in time keeps waiting, and a result that lands as the grace runs out is returned, not discarded. Local reads (read_local_file, user-local VFS reads) join them for a desktop that advertises localReadClaims: the desktop claims each read through authorize, as it already does for imports. Older desktops keep today's behaviour. The single "hung and was abandoned" result becomes two: notStarted for an unclaimed call, and outcomeUnknown/doNotRetry for one that started and lost its result. Both are settled through one sealed failure helper. A terminal run's server budget now follows the wait the desktop holds it for. A not-started report can settle only an unclaimed call. In the renderer, a running browser action is cancelled only by Stop: recovering the stream or leaving the chat view lets it finish and report. A terminal call delivered too late is reported as not started instead of being skipped. * refactor(mothership): one desktop-tool classifier and a typed claim for each claimant The authorize and confirm routes classify desktop tools through lib/mothership/tools/desktop-tools.ts instead of inline checks. The Sim execution claim and the desktop claim share one run-admission lock but return their own outcome types, so a Sim caller can no longer receive the desktop-only awaiting_permission outcome. The terminal-status mapping moves to lifecycle as getTerminalConfirmationStatus, since confirm uses it for every client tool. * test(mothership): drive the authorize unit test through the desktop claim * fix(mothership): settle an unclaimed desktop call whose wait ends before the grace A wait shorter than the pickup grace ended with no result and left the call claimable; it now settles it as never started like any unclaimed call. The desktop E2E fixture models claims per call, the way the server accepts a local read's repeat claim and refuses a second import claim. The admission probe follows the claim rename.
1 parent ef59524 commit 9af89fc

38 files changed

Lines changed: 1269 additions & 285 deletions

‎apps/desktop/e2e/local-files.spec.ts‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,8 @@ test('native file tools read and import through the installed preload without Si
1616
const png =
1717
'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAusB9Y9Zl1sAAAAASUVORK5CYII='
1818
writeFileSync(join(source, 'image.png'), Buffer.from(png, 'base64'))
19-
let claimed = false
19+
/** Calls the server saw claimed; like the server, only an import refuses a second claim. */
20+
const claimed = new Set<string>()
2021
const calls: Record<string, { toolName: string; args: Record<string, unknown> }> = {
2122
text: { toolName: 'read_local_file', args: { path: join(source, 'report.txt') } },
2223
image: { toolName: 'read_local_file', args: { path: join(source, 'image.png') } },
@@ -44,11 +45,14 @@ test('native file tools read and import through the installed preload without Si
4445
for await (const chunk of request) body += chunk.toString()
4546
const input = JSON.parse(body)
4647
const call = calls[input.toolCallId]
47-
if (!call || (input.claim && claimed)) {
48+
if (
49+
!call ||
50+
(input.claim && call.toolName === 'import_local_files' && claimed.has(input.toolCallId))
51+
) {
4852
response.writeHead(call ? 409 : 403, { 'Content-Type': 'application/json' }).end('{}')
4953
return
5054
}
51-
if (input.claim) claimed = true
55+
if (input.claim) claimed.add(input.toolCallId)
5256
response
5357
.writeHead(200, { 'Content-Type': 'application/json' })
5458
.end(JSON.stringify({ ...call, chatId: 'org-chat' }))
@@ -92,6 +96,7 @@ test('native file tools read and import through the installed preload without Si
9296
ok: true,
9397
data: { observations: [{ mediaType: 'image/png', data: png }] },
9498
})
99+
expect([...claimed]).toEqual(['text', 'image'])
95100
const result = await invoke({ operation: 'manifest', toolCallId: 'import' })
96101
if (!result.ok || result.data.kind !== 'manifest') throw new Error(JSON.stringify(result))
97102
expect(result.data.targetWorkspaceId).toBe('target-workspace')

‎apps/desktop/src/main/ipc.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -485,7 +485,7 @@ describe('registerIpcHandlers', () => {
485485
expect(mounts).not.toHaveBeenCalled()
486486
expect(fetchAuthorization).toHaveBeenCalledWith(
487487
`${APP}/api/desktop/tool/authorize`,
488-
expect.objectContaining({ body: JSON.stringify({ toolCallId: 'tool-native' }) })
488+
expect.objectContaining({ body: JSON.stringify({ toolCallId: 'tool-native', claim: true }) })
489489
)
490490
expect(
491491
await handler?.(evilEvent, { operation: 'read', toolCallId: 'tool-native' })
@@ -569,7 +569,7 @@ describe('registerIpcHandlers', () => {
569569
expect.objectContaining({
570570
method: 'POST',
571571
credentials: 'include',
572-
body: JSON.stringify({ toolCallId: 'tool-1' }),
572+
body: JSON.stringify({ toolCallId: 'tool-1', claim: true }),
573573
})
574574
)
575575
})

‎apps/desktop/src/main/ipc.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -578,10 +578,13 @@ async function authorizeLocalFilesystemTool(
578578
request: unknown
579579
): Promise<boolean> {
580580
if (typeof request !== 'object' || request === null) return false
581+
// Claimed like an import: the server sees the read picked up, and refuses one it already
582+
// failed as never started.
581583
const authorization = await fetchDesktopToolAuthorization(
582584
event,
583585
deps,
584-
(request as { requestId?: unknown }).requestId
586+
(request as { requestId?: unknown }).requestId,
587+
true
585588
)
586589
return authorization
587590
? deps.localFilesystem.isAuthorizedClientToolRequest(request, authorization)
@@ -2113,7 +2116,7 @@ export function registerIpcHandlers(deps: IpcDeps): void {
21132116
event,
21142117
deps,
21152118
request.toolCallId,
2116-
request.operation === 'manifest',
2119+
request.operation === 'manifest' || request.operation === 'read',
21172120
(status) => {
21182121
failureStatus = status
21192122
}

‎apps/desktop/src/main/terminal/index.ts‎

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,10 @@ import { homedir } from 'node:os'
1414
import type { TerminalShortcutCommand } from '@sim/desktop-bridge'
1515
import { createLogger } from '@sim/logger'
1616
import {
17-
DEFAULT_RUN_WAIT_MS,
1817
isTerminalControlKey,
1918
MAX_INPUT_KEYS,
20-
MAX_RUN_WAIT_MS,
2119
MAX_TOOL_OUTPUT_CHARS,
20+
resolveRunWaitMs,
2221
type TerminalCommandEvent,
2322
type TerminalControlKey,
2423
type TerminalCwdResult,
@@ -109,14 +108,6 @@ const HANDOFF_MAX_MS = 12 * 60 * 60 * 1000
109108
*/
110109
const HANDOFF_SETTLE_MS = 5_000
111110

112-
/** How long to hold the turn before handing a still-running command back. */
113-
function resolveWaitMs(waitSeconds: number | undefined): number {
114-
const requested = Number(waitSeconds)
115-
return Number.isFinite(requested) && requested > 0
116-
? Math.min(requested * 1000, MAX_RUN_WAIT_MS)
117-
: DEFAULT_RUN_WAIT_MS
118-
}
119-
120111
function elideOutput(value: string): { text: string; truncated: boolean } {
121112
return elide(value, MAX_TOOL_OUTPUT_CHARS)
122113
}
@@ -1097,7 +1088,7 @@ export class TerminalService {
10971088
const handle = await startRun(session, command, terminal.currentCwd, terminal.env)
10981089
if ('error' in handle) throw new TerminalError('SPAWN_FAILED', handle.error)
10991090

1100-
const waitMs = resolveWaitMs(args.waitSeconds)
1091+
const waitMs = resolveRunWaitMs(args.waitSeconds)
11011092
const outcome = await awaitRun(handle, waitMs)
11021093
if (outcome.done) {
11031094
await closeRunWindow(handle, terminal.env)
@@ -1149,7 +1140,7 @@ export class TerminalService {
11491140
)
11501141
}
11511142

1152-
return session.runCommand(command, toolCallId, resolveWaitMs(args.waitSeconds))
1143+
return session.runCommand(command, toolCallId, resolveRunWaitMs(args.waitSeconds))
11531144
}
11541145

11551146
private spawn(

‎apps/desktop/src/preload/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,7 @@ const api: SimDesktopApi = {
156156
ipcRenderer.invoke('desktop:local-filesystem', request),
157157
localFiles: (request: DesktopLocalFileRequest): Promise<DesktopLocalFileResponse> =>
158158
ipcRenderer.invoke('desktop:local-files', request),
159+
localReadClaims: true,
159160
onCommand: (callback: (command: DesktopCommand) => void): (() => void) => {
160161
const listener = (_event: unknown, command: DesktopCommand) => callback(command)
161162
ipcRenderer.on('desktop:command', listener)

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

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
import type { Span } from '@opentelemetry/api'
2-
import { isBrowserToolName } from '@sim/browser-protocol'
32
import { createLogger } from '@sim/logger'
4-
import { isTerminalToolName } from '@sim/terminal-protocol'
53
import { getErrorMessage, toError } from '@sim/utils/errors'
64
import { isPlainRecord } from '@sim/utils/object'
75
import { type NextRequest, NextResponse } from 'next/server'
@@ -14,6 +12,7 @@ import {
1412
type AsyncCompletionData,
1513
type AsyncConfirmationStatus,
1614
type AsyncTerminalStatus,
15+
getTerminalConfirmationStatus,
1716
isDeliveredAsyncStatus,
1817
isTerminalAsyncStatus,
1918
isWorkflowToolExecutionClaimable,
@@ -41,12 +40,11 @@ import {
4140
import { withIncomingGoSpan } from '@/lib/mothership/request/otel'
4241
import { sealClientToolSettlement } from '@/lib/mothership/request/tools/client-completion-seal.server'
4342
import { isWorkflowToolName } from '@/lib/mothership/tools/client-executed-tools'
44-
import { getDesktopToolClaimOwner } from '@/lib/mothership/tools/desktop-tools'
43+
import { getDesktopToolClaimOwner, isNativeDesktopTool } from '@/lib/mothership/tools/desktop-tools'
4544
import {
4645
createStructuralWorkflowToolCompletionData,
4746
getWorkflowToolCompletionExecutionId,
4847
getWorkflowToolCompletionMessage,
49-
getWorkflowToolConfirmationStatus,
5048
getWorkflowToolLaunchError,
5149
resolveWorkflowToolTargetId,
5250
WORKFLOW_EXECUTION_BUSY,
@@ -92,7 +90,7 @@ function acknowledgeSettledToolCall(
9290
toolCallId: string,
9391
storedStatus: AsyncTerminalStatus
9492
): NextResponse {
95-
const settledStatus = getWorkflowToolConfirmationStatus(storedStatus)
93+
const settledStatus = getTerminalConfirmationStatus(storedStatus)
9694
span.setAttributes({
9795
[TraceAttr.ToolConfirmationStatus]: settledStatus,
9896
[TraceAttr.CopilotConfirmOutcome]: CopilotConfirmOutcome.Delivered,
@@ -264,7 +262,7 @@ export const POST = withRouteHandler((req: NextRequest) => {
264262
return createNotFoundResponse('Completed workflow execution not found')
265263
}
266264

267-
const terminalStatus = getWorkflowToolConfirmationStatus(existing.status)
265+
const terminalStatus = getTerminalConfirmationStatus(existing.status)
268266
span.setAttributes({
269267
[TraceAttr.ToolConfirmationStatus]: terminalStatus,
270268
[TraceAttr.CopilotConfirmOutcome]: CopilotConfirmOutcome.Delivered,
@@ -308,10 +306,7 @@ export const POST = withRouteHandler((req: NextRequest) => {
308306
const isErrorOrCancelledOutcome =
309307
status === ASYNC_TOOL_CONFIRMATION_STATUS.error ||
310308
status === ASYNC_TOOL_CONFIRMATION_STATUS.cancelled
311-
const isNativeClientTool =
312-
isBrowserToolName(existing.toolName) ||
313-
isTerminalToolName(existing.toolName) ||
314-
existing.toolName === 'import_local_files'
309+
const isNativeClientTool = isNativeDesktopTool(existing.toolName)
315310
const nativeClaimOwner = getDesktopToolClaimOwner(existing.toolName)
316311
const isPreclaimNativeTerminalOutcome =
317312
nativeClaimOwner !== undefined &&
@@ -330,6 +325,17 @@ export const POST = withRouteHandler((req: NextRequest) => {
330325
span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound)
331326
return createNotFoundResponse('Running client tool call not found')
332327
}
328+
// A reporter that says the call never started (a stale replay, a closed view) cannot speak
329+
// for a call the desktop claimed: only the claim's own result may settle it.
330+
if (
331+
isNativeClientTool &&
332+
isPlainRecord(data) &&
333+
data.notStarted === true &&
334+
existing.status !== ASYNC_TOOL_STATUS.pending
335+
) {
336+
span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound)
337+
return createNotFoundResponse('Pending client tool call not found')
338+
}
333339

334340
let effectiveStatus = status
335341
let executionId = submittedExecutionId
@@ -365,7 +371,7 @@ export const POST = withRouteHandler((req: NextRequest) => {
365371
executionId = claimedExecutionId
366372
if (status !== ASYNC_TOOL_CONFIRMATION_STATUS.background) {
367373
if (trustedExecution) {
368-
effectiveStatus = getWorkflowToolConfirmationStatus(trustedExecution.status)
374+
effectiveStatus = getTerminalConfirmationStatus(trustedExecution.status)
369375
} else if (!isErrorOrCancelledOutcome) {
370376
span.setAttribute(
371377
TraceAttr.CopilotConfirmOutcome,
@@ -378,7 +384,7 @@ export const POST = withRouteHandler((req: NextRequest) => {
378384
executionId = submittedExecutionId
379385
} else if (trustedExecution) {
380386
executionId = trustedExecution.executionId
381-
effectiveStatus = getWorkflowToolConfirmationStatus(trustedExecution.status)
387+
effectiveStatus = getTerminalConfirmationStatus(trustedExecution.status)
382388
} else if (!isErrorOrCancelledOutcome) {
383389
effectiveStatus = ASYNC_TOOL_CONFIRMATION_STATUS.error
384390
executionId = undefined

‎apps/sim/app/api/desktop/tool/authorize/route.test.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ vi.mock('@/lib/mothership/async-runs/repository', () => mothershipAsyncRunsMock)
1717

1818
import { POST } from './route'
1919

20-
const claimToolExecution = mothershipAsyncRunsMockFns.mockClaimToolExecution
20+
const claimDesktopToolCall = mothershipAsyncRunsMockFns.mockClaimDesktopToolCall
2121
const getAsyncToolCall = mothershipAsyncRunsMockFns.mockGetAsyncToolCall
2222
const getRunSegment = mothershipAsyncRunsMockFns.mockGetRunSegment
2323
const resolveInvocationWorkspace = mothershipWorkspaceTargetMockFns.mockResolveInvocationWorkspace
@@ -50,7 +50,7 @@ describe('desktop tool authorization', () => {
5050
userId: 'user-1',
5151
status: 'active',
5252
})
53-
claimToolExecution.mockResolvedValue({ outcome: 'claimed' })
53+
claimDesktopToolCall.mockResolvedValue({ outcome: 'claimed' })
5454
})
5555

5656
it('never returns presentation activity as an executable browser argument', async () => {
@@ -193,7 +193,7 @@ describe('desktop tool authorization', () => {
193193
new OrchestrationError('not_found', 'Workspace not found')
194194
)
195195
expect((await POST(request('import-1', true))).status).toBe(404)
196-
claimToolExecution.mockResolvedValueOnce({ outcome: 'existing' })
196+
claimDesktopToolCall.mockResolvedValueOnce({ outcome: 'existing' })
197197
expect((await POST(request('import-1', true))).status).toBe(409)
198198
})
199199

‎apps/sim/app/api/desktop/tool/authorize/route.ts‎

Lines changed: 34 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
import { isCurrentBrowserToolName } from '@sim/browser-protocol'
2-
import { isTerminalToolName } from '@sim/terminal-protocol'
31
import { isRecordLike, omit } from '@sim/utils/object'
42
import { type NextRequest, NextResponse } from 'next/server'
53
import { authorizeDesktopToolContract } from '@/lib/api/contracts/desktop-tool-authorization'
@@ -9,17 +7,21 @@ import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
97
import { resolveInvocationWorkspace } from '@/lib/mothership/application/workspace-target'
108
import { DESKTOP_TOOL_CLAIM_OWNER } from '@/lib/mothership/async-runs/lifecycle'
119
import {
12-
claimToolExecution,
10+
claimDesktopToolCall,
11+
type DesktopToolCallClaim,
1312
getAsyncToolCall,
1413
getRunSegment,
15-
type ToolExecutionClaim,
1614
} from '@/lib/mothership/async-runs/repository'
1715
import {
1816
authenticateCopilotRequestSessionOnly,
1917
createNotFoundResponse,
2018
createUnauthorizedResponse,
2119
} from '@/lib/mothership/request/http'
22-
import { isUserLocalVfsToolCall } from '@/lib/mothership/tools/local-filesystem'
20+
import {
21+
getDesktopToolClaimOwner,
22+
isDesktopToolCall,
23+
isLocalReadToolCall,
24+
} from '@/lib/mothership/tools/desktop-tools'
2325

2426
const admissionClosedResponse = () =>
2527
NextResponse.json(
@@ -29,7 +31,7 @@ const admissionClosedResponse = () =>
2931

3032
/** A refused claim answers the same way for every tool, except how each reports a lost race. */
3133
function refusedClaimResponse(
32-
claim: Exclude<ToolExecutionClaim['outcome'], 'claimed'>,
34+
claim: Exclude<DesktopToolCallClaim['outcome'], 'claimed'>,
3335
notPending: () => NextResponse
3436
): NextResponse {
3537
if (claim === 'closed') return admissionClosedResponse()
@@ -74,16 +76,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
7476
}
7577

7678
const args = isRecordLike(toolCall.args) ? (toolCall.args as Record<string, unknown>) : {}
77-
const isBrowserTool = isCurrentBrowserToolName(toolCall.toolName)
78-
const isTerminalTool = isTerminalToolName(toolCall.toolName)
79-
const isLocalFileTool =
80-
toolCall.toolName === 'read_local_file' || toolCall.toolName === 'import_local_files'
81-
const authorized =
82-
isBrowserTool ||
83-
isTerminalTool ||
84-
isLocalFileTool ||
85-
isUserLocalVfsToolCall(toolCall.toolName, args)
86-
if (!authorized) {
79+
if (!isDesktopToolCall(toolCall.toolName, args)) {
8780
return NextResponse.json(
8881
{ error: 'Tool call is not authorized for desktop execution' },
8982
{ status: 403 }
@@ -115,7 +108,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
115108
{ status: 409 }
116109
)
117110
if (toolCall.status !== 'pending') return alreadyStarted()
118-
const { outcome } = await claimToolExecution({
111+
const { outcome } = await claimDesktopToolCall({
119112
toolCallId: toolCall.toolCallId,
120113
runId: toolCall.runId,
121114
userId,
@@ -129,20 +122,39 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
129122
return createNotFoundResponse('The import must be started before reading file bytes')
130123
}
131124

125+
// A desktop that claims local reads claims each one before its first read; its later reads of
126+
// the same call ride on that claim. A call persisted running (an older desktop's turn) is read
127+
// as before.
128+
if (parsed.data.body.claim && isLocalReadToolCall(toolCall.toolName, args)) {
129+
const notPending = () => createNotFoundResponse('Pending client tool call not found')
130+
if (toolCall.status === 'pending') {
131+
const { outcome } = await claimDesktopToolCall({
132+
toolCallId: toolCall.toolCallId,
133+
runId: toolCall.runId,
134+
userId,
135+
claimedBy: DESKTOP_TOOL_CLAIM_OWNER.files,
136+
})
137+
if (outcome !== 'claimed') return refusedClaimResponse(outcome, notPending)
138+
} else if (toolCall.claimedBy !== null && toolCall.claimedBy !== DESKTOP_TOOL_CLAIM_OWNER.files)
139+
return notPending()
140+
}
141+
132142
// Browser and terminal actions are one-shot side effects on the user's own
133143
// machine, so the pending call is claimed here, atomically, before crossing
134144
// the Electron boundary — a replayed renderer event must not run a command
135145
// or click a button twice.
136-
if (isBrowserTool || isTerminalTool) {
146+
const actionClaimOwner = getDesktopToolClaimOwner(toolCall.toolName)
147+
if (
148+
actionClaimOwner === DESKTOP_TOOL_CLAIM_OWNER.browser ||
149+
actionClaimOwner === DESKTOP_TOOL_CLAIM_OWNER.terminal
150+
) {
137151
const notPending = () => createNotFoundResponse('Pending client tool call not found')
138152
if (toolCall.status !== 'pending') return notPending()
139-
const { outcome } = await claimToolExecution({
153+
const { outcome } = await claimDesktopToolCall({
140154
toolCallId: toolCall.toolCallId,
141155
runId: toolCall.runId,
142156
userId,
143-
claimedBy: isBrowserTool
144-
? DESKTOP_TOOL_CLAIM_OWNER.browser
145-
: DESKTOP_TOOL_CLAIM_OWNER.terminal,
157+
claimedBy: actionClaimOwner,
146158
})
147159
if (outcome !== 'claimed') return refusedClaimResponse(outcome, notPending)
148160
}

0 commit comments

Comments
 (0)