Skip to content

Commit 65b247f

Browse files
authored
fix(mothership): keep local file tools running when the chat view changes (#8666)
* fix(mothership): keep local file tools running when the chat view changes Local file reads and imports now take the same Stop-only lifetime as browser actions: a history reconnect, a stream recovery or leaving the chat view leaves them running so they finish and report, and only the user's Stop cancels them. The desktop-tool classifier in client-executed-tools.ts is removed in favour of the single one in tools/desktop-tools.ts. When the desktop app holds a call, confirm now answers 409 to any other reporter (a stale replay, a lost race against the claim), and the client treats 409 as final instead of retrying a 404 five times. The not-started results now tell the model what it can act on: the action never started; don't retry it in this turn; ask the user to keep the chat open in the Sim desktop app. A local read the server cannot see picked up is no longer described as started. The obsolete admission probe script is deleted. Nothing ran it, it no longer type-checked against the repository API, and the integration suites cover the same admission behaviour against real Postgres. * fix(mothership): Stop reaches every desktop tool of the view, and results say what is known Desktop tools now take one lifetime the view owns and only the user's Stop ends, so Stop still reaches a tool that a replaced stream reader started. A held-call 409 is final on the trimmed retry of an oversized report too. The not-started result no longer asserts why nothing picked the call up, and a local read the desktop claimed is described as started when its result is lost. * fix(mothership): scope a desktop tool's Stop to its turn A desktop tool outlives the chat view that started it, so its Stop belongs to the turn, not the view: tools are keyed by the turn's stream id, which survives reader replacement and remounts and differs between chats. Stop in another chat leaves them running, and Stop from a view reopened on the turn still reaches them. * fix(mothership): hold a turn's Stop only while its desktop tools run Replace the bounded LRU of turn controllers with leases: each running browser action or local file tool holds its turn's Stop until it settles, so a live turn can never be evicted and settled turns leave nothing behind. * test(mothership): import the lease registry by its absolute path
1 parent ba12945 commit 65b247f

23 files changed

Lines changed: 410 additions & 538 deletions

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

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -165,7 +165,7 @@ describe('Copilot Confirm API Route', () => {
165165
})
166166
)
167167

168-
expect(response.status).toBe(404)
168+
expect(response.status).toBe(409)
169169
expect(completeAsyncToolCall).not.toHaveBeenCalled()
170170
expect(detachAsyncToolCall).not.toHaveBeenCalled()
171171
expect(encryptSecret).not.toHaveBeenCalled()
@@ -233,8 +233,10 @@ describe('Copilot Confirm API Route', () => {
233233
})
234234
)
235235

236-
expect(response.status).toBe(404)
237-
expect(await response.json()).toEqual({ error: 'Pending client tool call not found' })
236+
expect(response.status).toBe(409)
237+
expect(await response.json()).toEqual({
238+
error: 'The desktop app holds this tool call; only its own result settles it',
239+
})
238240
expect(completePendingAsyncToolCall).toHaveBeenCalledOnce()
239241
expect(completeClaimedAsyncToolCall).not.toHaveBeenCalled()
240242
expect(completeAsyncToolCall).not.toHaveBeenCalled()
@@ -300,7 +302,7 @@ describe('Copilot Confirm API Route', () => {
300302
})
301303
)
302304

303-
expect(response.status).toBe(404)
305+
expect(response.status).toBe(409)
304306
expect(completeClaimedAsyncToolCall).toHaveBeenCalledWith(expect.any(Object), 'desktop-browser')
305307
expect(publishToolConfirmation).not.toHaveBeenCalled()
306308
})

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

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,18 @@ function acknowledgeSettledToolCall(
8484
return createConfirmationResponse(toolCallId, settledStatus, 'Tool call was already settled')
8585
}
8686

87+
/**
88+
* A desktop call this report may not settle: the desktop app holds it under its claim (or a
89+
* report raced that claim and lost), so only the claim's own result settles it. Final, not
90+
* retryable: the reporter stops.
91+
*/
92+
function heldByAnotherReporterResponse(): NextResponse {
93+
return NextResponse.json(
94+
{ error: 'The desktop app holds this tool call; only its own result settles it' },
95+
{ status: 409 }
96+
)
97+
}
98+
8799
/** Atomically finalize or detach a client tool before publishing its wakeup event. */
88100
async function updateToolCallStatus(
89101
existing: NonNullable<Awaited<ReturnType<typeof getAsyncToolCall>>>,
@@ -272,7 +284,11 @@ export const POST = withRouteHandler((req: NextRequest) => {
272284
const isMutableClientToolCall = isWorkflowTool
273285
? isWorkflowToolExecutionClaimable(existing.status, existing.permissionDecision)
274286
: existing.status === ASYNC_TOOL_STATUS.running || isPreclaimNativeTerminalOutcome
275-
if ((isNativeClientTool || isWorkflowTool) && !isMutableClientToolCall) {
287+
if (isNativeClientTool && !isMutableClientToolCall) {
288+
span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound)
289+
return heldByAnotherReporterResponse()
290+
}
291+
if (isWorkflowTool && !isMutableClientToolCall) {
276292
span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound)
277293
return createNotFoundResponse('Running client tool call not found')
278294
}
@@ -285,7 +301,7 @@ export const POST = withRouteHandler((req: NextRequest) => {
285301
existing.status !== ASYNC_TOOL_STATUS.pending
286302
) {
287303
span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound)
288-
return createNotFoundResponse('Pending client tool call not found')
304+
return heldByAnotherReporterResponse()
289305
}
290306

291307
let effectiveStatus = status
@@ -422,7 +438,7 @@ export const POST = withRouteHandler((req: NextRequest) => {
422438

423439
if (reconciledOutcome === 'conflict' && isPreclaimNativeTerminalOutcome) {
424440
span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound)
425-
return createNotFoundResponse('Pending client tool call not found')
441+
return heldByAnotherReporterResponse()
426442
}
427443

428444
if (reconciledOutcome !== 'updated') {
Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
import { describe, expect, it } from 'vitest'
2+
import {
3+
leaseDesktopTool,
4+
stopDesktopTools,
5+
} from '@/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes'
6+
7+
describe('desktop tool leases', () => {
8+
it('cancels every running tool of the stopped turn and no other turn', () => {
9+
const first = leaseDesktopTool('turn-a')
10+
const second = leaseDesktopTool('turn-a')
11+
const other = leaseDesktopTool('turn-b')
12+
13+
stopDesktopTools('turn-a', 'user_stop')
14+
15+
expect(first.signal.aborted).toBe(true)
16+
expect(second.signal.aborted).toBe(true)
17+
expect(first.signal.reason).toBe('user_stop')
18+
expect(other.signal.aborted).toBe(false)
19+
other.release()
20+
})
21+
22+
it('keeps a turn reachable by Stop while any of its tools still runs', () => {
23+
const settled = leaseDesktopTool('turn-c')
24+
const running = leaseDesktopTool('turn-c')
25+
for (let turn = 0; turn < 500; turn++) leaseDesktopTool(`busy-${turn}`).release()
26+
27+
settled.release()
28+
settled.release()
29+
stopDesktopTools('turn-c', 'user_stop')
30+
31+
expect(running.signal.aborted).toBe(true)
32+
})
33+
34+
it('gives a turn whose tools all settled a fresh lifetime for its next tool', () => {
35+
const settled = leaseDesktopTool('turn-d')
36+
settled.release()
37+
stopDesktopTools('turn-d', 'user_stop')
38+
39+
const next = leaseDesktopTool('turn-d')
40+
41+
expect(settled.signal.aborted).toBe(false)
42+
expect(next.signal).not.toBe(settled.signal)
43+
expect(next.signal.aborted).toBe(false)
44+
next.release()
45+
})
46+
47+
it('does not let a tool that settles after Stop release a newer lease on the turn', () => {
48+
const stopped = leaseDesktopTool('turn-e')
49+
stopDesktopTools('turn-e', 'user_stop')
50+
const next = leaseDesktopTool('turn-e')
51+
52+
stopped.release()
53+
stopDesktopTools('turn-e', 'user_stop')
54+
55+
expect(next.signal.aborted).toBe(true)
56+
})
57+
})
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
/** The desktop tools of one turn that are still running in this tab, and the Stop that ends them. */
2+
interface RunningTurnTools {
3+
stop: AbortController
4+
running: number
5+
}
6+
7+
/**
8+
* Turns with a desktop tool still running in this tab, keyed by the turn's stream id. A tool
9+
* outlives the chat view that started it (and any stream reader), so the turn, not the view, owns
10+
* its Stop. A turn is held only while one of its tools runs: each tool releases it as it settles.
11+
*/
12+
const runningTurns = new Map<string, RunningTurnTools>()
13+
14+
/** A running desktop tool's hold on its turn. */
15+
interface DesktopToolLease {
16+
/** Aborted only by the user's Stop of the turn. */
17+
signal: AbortSignal
18+
/** Called once when the tool settles. */
19+
release(): void
20+
}
21+
22+
/**
23+
* Starts a desktop tool (a browser action, a local file read or import) for a turn. Only the
24+
* user's Stop of that turn cancels it: replacing the stream reader, leaving the chat view, or
25+
* stopping another chat's turn leaves it running to finish and report its own result.
26+
*/
27+
export function leaseDesktopTool(streamId: string): DesktopToolLease {
28+
let turn = runningTurns.get(streamId)
29+
if (!turn) {
30+
turn = { stop: new AbortController(), running: 0 }
31+
runningTurns.set(streamId, turn)
32+
}
33+
turn.running += 1
34+
const held = turn
35+
let released = false
36+
return {
37+
signal: held.stop.signal,
38+
release() {
39+
if (released) return
40+
released = true
41+
held.running -= 1
42+
if (held.running === 0 && runningTurns.get(streamId) === held) runningTurns.delete(streamId)
43+
},
44+
}
45+
}
46+
47+
/** Cancels the running desktop tools of a turn the user stopped, from whichever view started them. */
48+
export function stopDesktopTools(streamId: string, reason: string): void {
49+
runningTurns.get(streamId)?.stop.abort(reason)
50+
runningTurns.delete(streamId)
51+
}

‎apps/sim/app/workspace/[workspaceId]/home/hooks/stream/handle-tool-event.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,9 @@ import {
1717
} from '@/lib/mothership/resources/extraction'
1818
import {
1919
isClientExecutedToolCall,
20-
isDesktopExecutedToolCall,
2120
isWorkflowToolName,
2221
} from '@/lib/mothership/tools/client-executed-tools'
22+
import { isDesktopToolCall } from '@/lib/mothership/tools/desktop-tools'
2323
import { invalidateResourceQueries } from '@/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-registry'
2424
import type { StreamLoopContext } from '@/app/workspace/[workspaceId]/home/hooks/stream/stream-context'
2525
import {
@@ -197,7 +197,7 @@ export function handleToolEvent(ctx: StreamLoopContext, parsed: ToolEvent): void
197197
// tools to it: its answer could only be an error, and that error would beat the real result.
198198
const shouldStartClientTool =
199199
isClientExecutedToolCall(name, args) &&
200-
(isDesktopApp() || !isDesktopExecutedToolCall(name, args)) &&
200+
(isDesktopApp() || !isDesktopToolCall(name, args)) &&
201201
!isPartial &&
202202
!deps.options.suppressedWorkflowToolStartIds?.has(rawId) &&
203203
node?.kind === 'tool' &&

‎apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.dom.test.tsx‎

Lines changed: 88 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,14 @@ import { QueryClient, QueryClientProvider } from '@tanstack/react-query'
2828
import { createRoot, type Root } from 'react-dom/client'
2929
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
3030

31-
const { mockRequestJson, mockExecuteWorkflow, mockExecuteBrowserToolOnClient } = vi.hoisted(() => ({
31+
const {
32+
mockRequestJson,
33+
mockExecuteWorkflow,
34+
mockExecuteBrowserToolOnClient,
35+
mockExecuteLocalFilesystemTool,
36+
} = vi.hoisted(() => ({
3237
mockExecuteBrowserToolOnClient: vi.fn(),
38+
mockExecuteLocalFilesystemTool: vi.fn(),
3339
mockRequestJson: vi.fn(),
3440
mockExecuteWorkflow:
3541
vi.fn<
@@ -47,6 +53,9 @@ vi.mock('@/app/workspace/[workspaceId]/providers/feature-flags-provider', () =>
4753

4854
vi.mock('next/navigation', () => nextNavigationMock)
4955
vi.mock('@/lib/desktop', () => libDesktopMock)
56+
vi.mock('@/lib/mothership/tools/client/local-filesystem', () => ({
57+
executeLocalFilesystemTool: mockExecuteLocalFilesystemTool,
58+
}))
5059
vi.mock('@/lib/mothership/tools/client/browser-tool-execution', () => ({
5160
executeBrowserToolOnClient: mockExecuteBrowserToolOnClient,
5261
}))
@@ -2129,8 +2138,21 @@ describe('useChat remount send recovery', () => {
21292138
?.messages.map((message) => message.id)
21302139
).toEqual(['saved-user', 'saved-assistant'])
21312140
})
2132-
describe('a desktop browser action in flight', () => {
2133-
const chatId = 'chat-browser-action'
2141+
describe.each([
2142+
{
2143+
kind: 'browser action',
2144+
toolName: 'browser_list_tabs',
2145+
arguments: {},
2146+
lifetimeOf: () => mockExecuteBrowserToolOnClient.mock.calls[0]?.[5],
2147+
},
2148+
{
2149+
kind: 'local file read',
2150+
toolName: 'read_local_file',
2151+
arguments: { path: '/Users/me/notes.txt' },
2152+
lifetimeOf: () => mockExecuteLocalFilesystemTool.mock.calls[0]?.[3]?.signal,
2153+
},
2154+
])('a desktop $kind in flight', ({ toolName, arguments: toolArguments, lifetimeOf }) => {
2155+
const chatId = 'chat-desktop-action'
21342156
const history: MothershipChatHistory = {
21352157
id: chatId,
21362158
mode: 'agent',
@@ -2140,8 +2162,8 @@ describe('useChat remount send recovery', () => {
21402162
resources: [],
21412163
}
21422164

2143-
/** Opens a turn whose stream delivers one desktop browser call and stays open. */
2144-
async function startBrowserAction() {
2165+
/** Opens a turn whose stream delivers one desktop tool call and stays open. */
2166+
async function startDesktopAction() {
21452167
let streamId: string | undefined
21462168
const replays: string[] = []
21472169
mockRequestJson.mockImplementation((contract: AnyApiRouteContract) =>
@@ -2168,9 +2190,9 @@ describe('useChat remount send recovery', () => {
21682190
phase: 'call',
21692191
executor: 'client',
21702192
mode: 'async',
2171-
toolName: 'browser_list_tabs',
2172-
toolCallId: 'browser-call',
2173-
arguments: {},
2193+
toolName,
2194+
toolCallId: 'desktop-call',
2195+
arguments: toolArguments,
21742196
},
21752197
}
21762198
return new Response(
@@ -2186,23 +2208,26 @@ describe('useChat remount send recovery', () => {
21862208
await act(async () => {
21872209
void chat.getResult().sendMessage('List my tabs')
21882210
})
2189-
await waitFor(() => mockExecuteBrowserToolOnClient.mock.calls.length === 1)
2190-
const toolSignal = mockExecuteBrowserToolOnClient.mock.calls[0]?.[5]
2211+
await waitFor(() => lifetimeOf() !== undefined)
2212+
const toolSignal = lifetimeOf()
21912213
if (!(toolSignal instanceof AbortSignal))
2192-
throw new Error('The browser action has no lifetime')
2193-
return { ...chat, toolSignal, replays }
2214+
throw new Error('The desktop action has no lifetime')
2215+
return { ...chat, toolSignal, replays, streamId: () => streamId }
21942216
}
21952217

21962218
beforeEach(() => {
21972219
libDesktopMockFns.mockIsDesktopApp.mockReturnValue(true)
2220+
const stillRunning = () => new Promise<void>(() => {})
2221+
mockExecuteBrowserToolOnClient.mockImplementation(stillRunning)
2222+
mockExecuteLocalFilesystemTool.mockImplementation(stillRunning)
21982223
})
21992224

22002225
afterEach(() => {
22012226
libDesktopMockFns.mockIsDesktopApp.mockReset()
22022227
})
22032228

22042229
it('keeps running when the window returns to view and the stream is recovered', async () => {
2205-
const { toolSignal, replays } = await startBrowserAction()
2230+
const { toolSignal, replays } = await startDesktopAction()
22062231

22072232
Object.defineProperty(document, 'visibilityState', {
22082233
configurable: true,
@@ -2217,15 +2242,63 @@ describe('useChat remount send recovery', () => {
22172242
})
22182243

22192244
it('keeps running when the chat view unmounts, so it finishes and reports its result', async () => {
2220-
const { toolSignal, unmount } = await startBrowserAction()
2245+
const { toolSignal, unmount } = await startDesktopAction()
22212246

22222247
unmount()
22232248

22242249
expect(toolSignal.aborted).toBe(false)
22252250
})
22262251

2252+
it('is still cancelled by Stop after the stream was recovered', async () => {
2253+
const { toolSignal, replays, getResult } = await startDesktopAction()
2254+
Object.defineProperty(document, 'visibilityState', {
2255+
configurable: true,
2256+
get: () => 'visible',
2257+
})
2258+
await act(async () => {
2259+
document.dispatchEvent(new Event('visibilitychange'))
2260+
})
2261+
await waitFor(() => replays.length > 0)
2262+
2263+
await act(async () => {
2264+
await getResult().stopGeneration()
2265+
})
2266+
2267+
expect(toolSignal.aborted).toBe(true)
2268+
})
2269+
2270+
it('keeps running when the user stops a turn in another chat', async () => {
2271+
const { toolSignal, navigate, getResult } = await startDesktopAction()
2272+
navigate('chat-other', { ...history, id: 'chat-other' })
2273+
await act(async () => {
2274+
void getResult().sendMessage('Something else')
2275+
})
2276+
2277+
await act(async () => {
2278+
await getResult().stopGeneration()
2279+
})
2280+
2281+
expect(toolSignal.aborted).toBe(false)
2282+
})
2283+
2284+
it('is still cancelled by Stop from the chat view reopened on its turn', async () => {
2285+
const { toolSignal, unmount, streamId } = await startDesktopAction()
2286+
unmount()
2287+
const reopened = renderUseChatInChat(chatId, {
2288+
...history,
2289+
activeStreamId: streamId() ?? null,
2290+
})
2291+
await waitFor(() => reopened.getResult().isSending)
2292+
2293+
await act(async () => {
2294+
await reopened.getResult().stopGeneration()
2295+
})
2296+
2297+
expect(toolSignal.aborted).toBe(true)
2298+
})
2299+
22272300
it('is cancelled when the user stops the chat', async () => {
2228-
const { toolSignal, getResult } = await startBrowserAction()
2301+
const { toolSignal, getResult } = await startDesktopAction()
22292302

22302303
await act(async () => {
22312304
await getResult().stopGeneration()

0 commit comments

Comments
 (0)