Skip to content

Commit 46cd710

Browse files
authored
fix(mothership): refuse desktop claims for unapproved or stopped calls (#8643)
* fix(mothership): refuse desktop claims for unapproved or stopped calls The desktop authorize route claimed any pending call, so a gated terminal run that was still awaiting approval, or a call on a run the user had stopped, could be claimed and executed. The claim now locks the run row and refuses once tool admission has closed (Stop, a newer turn, or the run's end), and refuses a call held for the user's decision until they allow it. Whether a call is gated depends on the turn, so pre-persist records it on the row (permission_requested_at). Stop now settles the stopped runs' open desktop calls in the transaction that closes admission: unclaimed calls as never started, claimed calls as outcome unknown. A result for an already-settled call (a retry, or one that lost to Stop) is acknowledged with the stored outcome instead of 404/500. * fix(mothership): settle every open desktop call on Stop and answer 410 after it Stop also settles delivered desktop calls and reads of granted local folders. Authorize checks admission before the call's status, so a call Stop already settled answers 410 rather than 404. * test(mothership): assert the acknowledged outcome, not the publish mock * test(mothership): give the hand-built tool call table the new permission column * refactor(mothership): one claim primitive, one desktop-tool classifier, sealed Stop results The desktop claim is now an option of the run-locked tool execution claim (claimSimToolExecution becomes claimToolExecution) instead of a second copy of the admission check. Stop picks the open desktop calls with the shared TS classifier, which moves to lib/mothership/tools/desktop-tools.ts, instead of a SQL restatement of it, and seals each result the way the confirm route does, so a waiter restores what Stop did rather than failing to unseal it. * fix(mothership): a declined call stays unclaimable without its gate marker Calls gated before permission_requested_at existed carry no marker, so a recorded decision that does not allow the call now disqualifies it too. * test(mothership): assert authorize outcomes, not claim mock calls
1 parent d4f26a7 commit 46cd710

25 files changed

Lines changed: 30765 additions & 178 deletions

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -624,6 +624,25 @@ describe('Copilot Confirm API Route', () => {
624624
)
625625
})
626626

627+
it.each([
628+
['client_tool', 'running', 'success', completeAsyncToolCall],
629+
['browser_snapshot', 'pending', 'error', completePendingAsyncToolCall],
630+
] as const)(
631+
'acknowledges a %s result whose write lost to a settlement that landed first',
632+
async (toolName, storedStatus, status, completion) => {
633+
const row = { ...existingRow, toolName, claimedBy: null }
634+
getAsyncToolCall
635+
.mockResolvedValueOnce({ ...row, status: storedStatus })
636+
.mockResolvedValueOnce({ ...row, status: 'failed' })
637+
completion.mockResolvedValueOnce(null)
638+
639+
const response = await POST(createMockPostRequest({ toolCallId: 'tool-call-123', status }))
640+
641+
expect(response.status).toBe(200)
642+
expect(await response.json()).toMatchObject({ toolCallId: 'tool-call-123', status: 'error' })
643+
}
644+
)
645+
627646
it('does not publish when another terminal confirmation already won', async () => {
628647
completeAsyncToolCall.mockResolvedValueOnce(null)
629648

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

Lines changed: 54 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
import { isBrowserToolName, isCurrentBrowserToolName } from '@sim/browser-protocol'
1+
import type { Span } from '@opentelemetry/api'
2+
import { isBrowserToolName } from '@sim/browser-protocol'
23
import { createLogger } from '@sim/logger'
34
import { isTerminalToolName } from '@sim/terminal-protocol'
45
import { getErrorMessage, toError } from '@sim/utils/errors'
@@ -12,7 +13,7 @@ import {
1213
ASYNC_TOOL_STATUS,
1314
type AsyncCompletionData,
1415
type AsyncConfirmationStatus,
15-
DESKTOP_TOOL_CLAIM_OWNER,
16+
type AsyncTerminalStatus,
1617
isDeliveredAsyncStatus,
1718
isTerminalAsyncStatus,
1819
isWorkflowToolExecutionClaimable,
@@ -38,11 +39,9 @@ import {
3839
createUnauthorizedResponse,
3940
} from '@/lib/mothership/request/http'
4041
import { withIncomingGoSpan } from '@/lib/mothership/request/otel'
41-
import {
42-
retainSealedClientToolContext,
43-
sealClientToolCompletion,
44-
} from '@/lib/mothership/request/tools/client-completion-seal.server'
42+
import { sealClientToolSettlement } from '@/lib/mothership/request/tools/client-completion-seal.server'
4543
import { isWorkflowToolName } from '@/lib/mothership/tools/client-executed-tools'
44+
import { getDesktopToolClaimOwner } from '@/lib/mothership/tools/desktop-tools'
4645
import {
4746
createStructuralWorkflowToolCompletionData,
4847
getWorkflowToolCompletionExecutionId,
@@ -83,6 +82,24 @@ function createConfirmationResponse(
8382
return NextResponse.json({ success: true, message, toolCallId, status })
8483
}
8584

85+
/**
86+
* A result for a call that is already settled — a retried delivery, or one that lost to the server
87+
* settling the call first (Stop, a fast "not started" failure) — answers with the stored outcome.
88+
* Nothing is written or published again, and the reporter stops retrying.
89+
*/
90+
function acknowledgeSettledToolCall(
91+
span: Span,
92+
toolCallId: string,
93+
storedStatus: AsyncTerminalStatus
94+
): NextResponse {
95+
const settledStatus = getWorkflowToolConfirmationStatus(storedStatus)
96+
span.setAttributes({
97+
[TraceAttr.ToolConfirmationStatus]: settledStatus,
98+
[TraceAttr.CopilotConfirmOutcome]: CopilotConfirmOutcome.Delivered,
99+
})
100+
return createConfirmationResponse(toolCallId, settledStatus, 'Tool call was already settled')
101+
}
102+
86103
/** Atomically finalize or detach a client tool before publishing its wakeup event. */
87104
async function updateToolCallStatus(
88105
existing: NonNullable<Awaited<ReturnType<typeof getAsyncToolCall>>>,
@@ -284,26 +301,22 @@ export const POST = withRouteHandler((req: NextRequest) => {
284301
)
285302
}
286303

304+
if (!isWorkflowTool && isTerminalAsyncStatus(existing.status)) {
305+
return acknowledgeSettledToolCall(span, toolCallId, existing.status)
306+
}
307+
287308
const isErrorOrCancelledOutcome =
288309
status === ASYNC_TOOL_CONFIRMATION_STATUS.error ||
289310
status === ASYNC_TOOL_CONFIRMATION_STATUS.cancelled
290311
const isNativeClientTool =
291312
isBrowserToolName(existing.toolName) ||
292313
isTerminalToolName(existing.toolName) ||
293314
existing.toolName === 'import_local_files'
315+
const nativeClaimOwner = getDesktopToolClaimOwner(existing.toolName)
294316
const isPreclaimNativeTerminalOutcome =
295-
(isCurrentBrowserToolName(existing.toolName) ||
296-
isTerminalToolName(existing.toolName) ||
297-
existing.toolName === 'import_local_files') &&
317+
nativeClaimOwner !== undefined &&
298318
existing.status === ASYNC_TOOL_STATUS.pending &&
299319
isErrorOrCancelledOutcome
300-
const nativeClaimOwner = isCurrentBrowserToolName(existing.toolName)
301-
? DESKTOP_TOOL_CLAIM_OWNER.browser
302-
: isTerminalToolName(existing.toolName)
303-
? DESKTOP_TOOL_CLAIM_OWNER.terminal
304-
: existing.toolName === 'import_local_files'
305-
? DESKTOP_TOOL_CLAIM_OWNER.files
306-
: undefined
307320
const isIndeterminateNativeExit =
308321
isPreclaimNativeTerminalOutcome &&
309322
status === ASYNC_TOOL_CONFIRMATION_STATUS.error &&
@@ -401,27 +414,24 @@ export const POST = withRouteHandler((req: NextRequest) => {
401414
}
402415
: {
403416
message: getClientToolCompletionMessage(status),
404-
data: {
405-
...retainSealedClientToolContext(existing.result),
406-
...(await sealClientToolCompletion({
407-
toolCallId,
408-
runId: existing.runId,
409-
userId: authenticatedUserId,
410-
...(isIndeterminateNativeExit
411-
? {
412-
message: NATIVE_HANDOFF_INTERRUPTED_MESSAGE,
413-
data: {
414-
error: NATIVE_HANDOFF_INTERRUPTED_MESSAGE,
415-
outcomeUnknown: true,
416-
doNotRetry: true,
417-
},
418-
}
419-
: {
420-
...(message !== undefined ? { message } : {}),
421-
...(data !== undefined ? { data } : {}),
422-
}),
423-
})),
424-
},
417+
data: await sealClientToolSettlement(existing.result, {
418+
toolCallId,
419+
runId: existing.runId,
420+
userId: authenticatedUserId,
421+
...(isIndeterminateNativeExit
422+
? {
423+
message: NATIVE_HANDOFF_INTERRUPTED_MESSAGE,
424+
data: {
425+
error: NATIVE_HANDOFF_INTERRUPTED_MESSAGE,
426+
outcomeUnknown: true,
427+
doNotRetry: true,
428+
},
429+
}
430+
: {
431+
...(message !== undefined ? { message } : {}),
432+
...(data !== undefined ? { data } : {}),
433+
}),
434+
}),
425435
}
426436

427437
const updateOutcome = await updateToolCallStatus(
@@ -453,6 +463,13 @@ export const POST = withRouteHandler((req: NextRequest) => {
453463
)
454464
: updateOutcome
455465

466+
if (reconciledOutcome === 'conflict' && !isWorkflowTool) {
467+
const settled = await getAsyncToolCall(toolCallId).catch(() => null)
468+
if (settled && isTerminalAsyncStatus(settled.status)) {
469+
return acknowledgeSettledToolCall(span, toolCallId, settled.status)
470+
}
471+
}
472+
456473
if (reconciledOutcome === 'conflict' && isPreclaimNativeTerminalOutcome) {
457474
span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound)
458475
return createNotFoundResponse('Pending client tool call not found')

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

Lines changed: 3 additions & 8 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 claimPendingAsyncToolCall = mothershipAsyncRunsMockFns.mockClaimPendingAsyncToolCall
20+
const claimToolExecution = mothershipAsyncRunsMockFns.mockClaimToolExecution
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-
claimPendingAsyncToolCall.mockResolvedValue({ toolCallId: 'browser-tool', status: 'running' })
53+
claimToolExecution.mockResolvedValue({ outcome: 'claimed' })
5454
})
5555

5656
it('never returns presentation activity as an executable browser argument', async () => {
@@ -84,7 +84,6 @@ describe('desktop tool authorization', () => {
8484
const response = await POST(request('retired-browser-tool'))
8585

8686
expect(response.status).toBe(403)
87-
expect(claimPendingAsyncToolCall).not.toHaveBeenCalled()
8887
})
8988

9089
it('rejects a replayed browser action after its pending row was claimed', async () => {
@@ -98,7 +97,6 @@ describe('desktop tool authorization', () => {
9897

9998
const response = await POST(request('browser-tool'))
10099
expect(response.status).toBe(404)
101-
expect(claimPendingAsyncToolCall).not.toHaveBeenCalled()
102100
})
103101

104102
it('rejects workspace VFS calls and mutating legacy local tools', async () => {
@@ -176,13 +174,11 @@ describe('desktop tool authorization', () => {
176174
{ userId: 'user-1', chatId: 'chat-1', organizationId: 'org-1', workspaceId: undefined },
177175
'target'
178176
)
179-
expect(claimPendingAsyncToolCall).toHaveBeenCalledExactlyOnceWith('import-1', 'desktop-files')
180177
getAsyncToolCall.mockResolvedValue({ ...tool, status: 'running', claimedBy: 'desktop-files' })
181178
expect((await POST(request('import-1', true))).status).toBe(409)
182179
expect((await POST(request('import-1'))).status).toBe(200)
183180
getAsyncToolCall.mockResolvedValue({ ...tool, status: 'running', claimedBy: 'sim-stream' })
184181
expect((await POST(request('import-1'))).status).toBe(404)
185-
expect(claimPendingAsyncToolCall).toHaveBeenCalledOnce()
186182
})
187183

188184
it('rejects inaccessible destinations and lost import claims before exposing files', async () => {
@@ -197,8 +193,7 @@ describe('desktop tool authorization', () => {
197193
new OrchestrationError('not_found', 'Workspace not found')
198194
)
199195
expect((await POST(request('import-1', true))).status).toBe(404)
200-
expect(claimPendingAsyncToolCall).not.toHaveBeenCalled()
201-
claimPendingAsyncToolCall.mockResolvedValueOnce(null)
196+
claimToolExecution.mockResolvedValueOnce({ outcome: 'existing' })
202197
expect((await POST(request('import-1', true))).status).toBe(409)
203198
})
204199

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

Lines changed: 50 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,10 @@ import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
99
import { resolveInvocationWorkspace } from '@/lib/mothership/application/workspace-target'
1010
import { DESKTOP_TOOL_CLAIM_OWNER } from '@/lib/mothership/async-runs/lifecycle'
1111
import {
12-
claimPendingAsyncToolCall,
12+
claimToolExecution,
1313
getAsyncToolCall,
1414
getRunSegment,
15+
type ToolExecutionClaim,
1516
} from '@/lib/mothership/async-runs/repository'
1617
import {
1718
authenticateCopilotRequestSessionOnly,
@@ -20,12 +21,33 @@ import {
2021
} from '@/lib/mothership/request/http'
2122
import { isUserLocalVfsToolCall } from '@/lib/mothership/tools/local-filesystem'
2223

24+
const admissionClosedResponse = () =>
25+
NextResponse.json(
26+
{ error: 'This chat turn ended or was stopped, so the tool call can no longer run' },
27+
{ status: 410 }
28+
)
29+
30+
/** A refused claim answers the same way for every tool, except how each reports a lost race. */
31+
function refusedClaimResponse(
32+
claim: Exclude<ToolExecutionClaim['outcome'], 'claimed'>,
33+
notPending: () => NextResponse
34+
): NextResponse {
35+
if (claim === 'closed') return admissionClosedResponse()
36+
if (claim === 'awaiting_permission')
37+
return NextResponse.json({ error: 'The user has not approved this tool call' }, { status: 403 })
38+
return notPending()
39+
}
40+
2341
/**
2442
* Electron calls this endpoint from the main process before every privileged
2543
* native model action. It returns only server-persisted canonical tool args;
2644
* Electron validates local-file requests against them and uses them directly
2745
* for browser and terminal tools. The presentation-only `activity` field is
2846
* dropped: desktop actions reject arguments they do not declare.
47+
*
48+
* Nothing is handed over once the run's tool admission has closed (Stop, a
49+
* newer turn, or the run's end), nor for a call held for the user's decision
50+
* that they have not allowed.
2951
*/
3052
export const POST = withRouteHandler(async (request: NextRequest) => {
3153
const { userId, isAuthenticated } = await authenticateCopilotRequestSessionOnly()
@@ -37,13 +59,16 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
3759
if (!parsed.success) return parsed.response
3860

3961
const toolCall = await getAsyncToolCall(parsed.data.body.toolCallId)
40-
if (!toolCall || (toolCall.status !== 'pending' && toolCall.status !== 'running')) {
41-
return createNotFoundResponse('Pending client tool call not found')
42-
}
62+
if (!toolCall) return createNotFoundResponse('Pending client tool call not found')
4363
const run = await getRunSegment(toolCall.runId)
4464
if (!run || run.userId !== userId) {
4565
return NextResponse.json({ error: 'Forbidden' }, { status: 403 })
4666
}
67+
// Ahead of the status checks: Stop settles the run's open calls in the same commit.
68+
if (run.toolAdmissionClosedAt) return admissionClosedResponse()
69+
if (toolCall.status !== 'pending' && toolCall.status !== 'running') {
70+
return createNotFoundResponse('Pending client tool call not found')
71+
}
4772
if (run.status === 'complete' || run.status === 'error' || run.status === 'cancelled') {
4873
return createNotFoundResponse('Pending client tool call not found')
4974
}
@@ -84,14 +109,19 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
84109
return NextResponse.json(projected.body, { status: projected.status })
85110
}
86111
if (parsed.data.body.claim) {
87-
if (
88-
toolCall.status !== 'pending' ||
89-
!(await claimPendingAsyncToolCall(toolCall.toolCallId, DESKTOP_TOOL_CLAIM_OWNER.files))
90-
)
91-
return NextResponse.json(
112+
const alreadyStarted = () =>
113+
NextResponse.json(
92114
{ error: 'This import was already started; inspect its result before retrying' },
93115
{ status: 409 }
94116
)
117+
if (toolCall.status !== 'pending') return alreadyStarted()
118+
const { outcome } = await claimToolExecution({
119+
toolCallId: toolCall.toolCallId,
120+
runId: toolCall.runId,
121+
userId,
122+
claimedBy: DESKTOP_TOOL_CLAIM_OWNER.files,
123+
})
124+
if (outcome !== 'claimed') return refusedClaimResponse(outcome, alreadyStarted)
95125
} else if (
96126
toolCall.status !== 'running' ||
97127
toolCall.claimedBy !== DESKTOP_TOOL_CLAIM_OWNER.files
@@ -104,16 +134,17 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
104134
// the Electron boundary — a replayed renderer event must not run a command
105135
// or click a button twice.
106136
if (isBrowserTool || isTerminalTool) {
107-
if (toolCall.status !== 'pending') {
108-
return createNotFoundResponse('Pending client tool call not found')
109-
}
110-
const claimed = await claimPendingAsyncToolCall(
111-
toolCall.toolCallId,
112-
isBrowserTool ? DESKTOP_TOOL_CLAIM_OWNER.browser : DESKTOP_TOOL_CLAIM_OWNER.terminal
113-
)
114-
if (!claimed) {
115-
return createNotFoundResponse('Pending client tool call not found')
116-
}
137+
const notPending = () => createNotFoundResponse('Pending client tool call not found')
138+
if (toolCall.status !== 'pending') return notPending()
139+
const { outcome } = await claimToolExecution({
140+
toolCallId: toolCall.toolCallId,
141+
runId: toolCall.runId,
142+
userId,
143+
claimedBy: isBrowserTool
144+
? DESKTOP_TOOL_CLAIM_OWNER.browser
145+
: DESKTOP_TOOL_CLAIM_OWNER.terminal,
146+
})
147+
if (outcome !== 'claimed') return refusedClaimResponse(outcome, notPending)
117148
}
118149

119150
return NextResponse.json({

0 commit comments

Comments
 (0)