From 110253040a6f23ceec52af9085391261ca5126cf Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 6 Oct 2026 08:29:10 -0700 Subject: [PATCH 1/3] 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. --- .../sim/app/api/copilot/confirm/route.test.ts | 55 ++++++++++++++++++- apps/sim/app/api/copilot/confirm/route.ts | 12 ++-- .../home/hooks/desktop-tool-lifetimes.test.ts | 16 ++++++ .../home/hooks/desktop-tool-lifetimes.ts | 8 +++ .../[workspaceId]/home/hooks/use-chat.ts | 12 +++- .../generated/trace-attribute-values-v1.ts | 1 + .../client/browser-tool-execution.test.ts | 5 +- .../tools/client/browser-tool-execution.ts | 4 +- .../tools/client/completion.test.ts | 9 +++ .../lib/mothership/tools/client/completion.ts | 2 +- apps/sim/stores/index.test.ts | 16 ++++++ apps/sim/stores/index.ts | 6 ++ 12 files changed, 132 insertions(+), 14 deletions(-) diff --git a/apps/sim/app/api/copilot/confirm/route.test.ts b/apps/sim/app/api/copilot/confirm/route.test.ts index 6c0f3414f2e..f2d8ff8dce2 100644 --- a/apps/sim/app/api/copilot/confirm/route.test.ts +++ b/apps/sim/app/api/copilot/confirm/route.test.ts @@ -1,3 +1,9 @@ +import { trace } from '@opentelemetry/api' +import { + BasicTracerProvider, + InMemorySpanExporter, + SimpleSpanProcessor, +} from '@opentelemetry/sdk-trace-base' import { copilotHttpMock, copilotHttpMockFns } from '@sim/testing' import { encryptionMock, encryptionMockFns } from '@sim/testing/mocks/encryption.mock' import { @@ -6,7 +12,7 @@ import { } from '@sim/testing/mocks/mothership-async-runs.mock' import { createMockRequest } from '@sim/testing/mocks/request.mock' import type { NextRequest } from 'next/server' -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const { publishToolConfirmation, getTrustedWorkflowToolExecution } = vi.hoisted(() => ({ publishToolConfirmation: vi.fn(), @@ -27,6 +33,9 @@ vi.mock('@/lib/workflows/executor/execution-state', () => ({ getTrustedWorkflowToolExecution, })) +import { CopilotConfirmOutcome } from '@/lib/mothership/generated/trace-attribute-values-v1' +import { TraceAttr } from '@/lib/mothership/generated/trace-attributes-v1' +import { TraceSpan } from '@/lib/mothership/generated/trace-spans-v1' import { POST } from './route' const { @@ -40,6 +49,17 @@ const { const encryptSecret = encryptionMockFns.mockEncryptSecret +/** Records the confirm spans and returns a reader for the outcome the route recorded. */ +function recordConfirmOutcome(): () => unknown { + const exporter = new InMemorySpanExporter() + trace.setGlobalTracerProvider( + new BasicTracerProvider({ spanProcessors: [new SimpleSpanProcessor(exporter)] }) + ) + return () => + exporter.getFinishedSpans().find((span) => span.name === TraceSpan.CopilotConfirmToolResult) + ?.attributes[TraceAttr.CopilotConfirmOutcome] +} + describe('Copilot Confirm API Route', () => { const existingRow = { toolCallId: 'tool-call-123', @@ -51,6 +71,10 @@ describe('Copilot Confirm API Route', () => { claimedBy: 'workflow:execution-1', } + afterEach(() => { + trace.disable() + }) + beforeEach(() => { copilotHttpMockFns.mockAuthenticateCopilotRequestSessionOnly.mockResolvedValue({ userId: 'user-1', @@ -151,6 +175,7 @@ describe('Copilot Confirm API Route', () => { }) it('rejects a native success before the desktop authorization claim', async () => { + const recordedOutcome = recordConfirmOutcome() getAsyncToolCall.mockResolvedValue({ ...existingRow, toolName: 'browser_snapshot', @@ -166,6 +191,7 @@ describe('Copilot Confirm API Route', () => { ) expect(response.status).toBe(409) + expect(recordedOutcome()).toBe(CopilotConfirmOutcome.HeldByDesktop) expect(completeAsyncToolCall).not.toHaveBeenCalled() expect(detachAsyncToolCall).not.toHaveBeenCalled() expect(encryptSecret).not.toHaveBeenCalled() @@ -218,6 +244,7 @@ describe('Copilot Confirm API Route', () => { ] as const)( 'rejects a pending %s %s when the native authorization claim wins the race', async (toolName, status) => { + const recordedOutcome = recordConfirmOutcome() getAsyncToolCall.mockResolvedValue({ ...existingRow, toolName, @@ -237,6 +264,7 @@ describe('Copilot Confirm API Route', () => { expect(await response.json()).toEqual({ error: 'The desktop app holds this tool call; only its own result settles it', }) + expect(recordedOutcome()).toBe(CopilotConfirmOutcome.HeldByDesktop) expect(completePendingAsyncToolCall).toHaveBeenCalledOnce() expect(completeClaimedAsyncToolCall).not.toHaveBeenCalled() expect(completeAsyncToolCall).not.toHaveBeenCalled() @@ -285,6 +313,31 @@ describe('Copilot Confirm API Route', () => { } ) + it('refuses a not-started report for a call the desktop already claimed', async () => { + const recordedOutcome = recordConfirmOutcome() + getAsyncToolCall.mockResolvedValue({ + ...existingRow, + toolName: 'browser_snapshot', + status: 'running', + claimedBy: 'desktop-browser', + }) + + const response = await POST( + createMockPostRequest({ + toolCallId: 'tool-call-123', + status: 'error', + message: 'The desktop action did not start.', + data: { notStarted: true }, + }) + ) + + expect(response.status).toBe(409) + expect(recordedOutcome()).toBe(CopilotConfirmOutcome.HeldByDesktop) + expect(completeAsyncToolCall).not.toHaveBeenCalled() + expect(completeClaimedAsyncToolCall).not.toHaveBeenCalled() + expect(publishToolConfirmation).not.toHaveBeenCalled() + }) + it('does not publish when another terminal transition wins indeterminate claim reconciliation', async () => { getAsyncToolCall.mockResolvedValue({ ...existingRow, diff --git a/apps/sim/app/api/copilot/confirm/route.ts b/apps/sim/app/api/copilot/confirm/route.ts index 392f90a284e..d9ea0e60b8f 100644 --- a/apps/sim/app/api/copilot/confirm/route.ts +++ b/apps/sim/app/api/copilot/confirm/route.ts @@ -89,7 +89,8 @@ function acknowledgeSettledToolCall( * report raced that claim and lost), so only the claim's own result settles it. Final, not * retryable: the reporter stops. */ -function heldByAnotherReporterResponse(): NextResponse { +function heldByAnotherReporterResponse(span: Span): NextResponse { + span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.HeldByDesktop) return NextResponse.json( { error: 'The desktop app holds this tool call; only its own result settles it' }, { status: 409 } @@ -285,8 +286,7 @@ export const POST = withRouteHandler((req: NextRequest) => { ? isWorkflowToolExecutionClaimable(existing.status, existing.permissionDecision) : existing.status === ASYNC_TOOL_STATUS.running || isPreclaimNativeTerminalOutcome if (isNativeClientTool && !isMutableClientToolCall) { - span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound) - return heldByAnotherReporterResponse() + return heldByAnotherReporterResponse(span) } if (isWorkflowTool && !isMutableClientToolCall) { span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound) @@ -300,8 +300,7 @@ export const POST = withRouteHandler((req: NextRequest) => { data.notStarted === true && existing.status !== ASYNC_TOOL_STATUS.pending ) { - span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound) - return heldByAnotherReporterResponse() + return heldByAnotherReporterResponse(span) } let effectiveStatus = status @@ -437,8 +436,7 @@ export const POST = withRouteHandler((req: NextRequest) => { } if (reconciledOutcome === 'conflict' && isPreclaimNativeTerminalOutcome) { - span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound) - return heldByAnotherReporterResponse() + return heldByAnotherReporterResponse(span) } if (reconciledOutcome !== 'updated') { diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.test.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.test.ts index 07e4f2358c0..9cf43b508c9 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.test.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from 'vitest' import { leaseDesktopTool, + stopAllDesktopTools, stopDesktopTools, } from '@/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes' @@ -54,4 +55,19 @@ describe('desktop tool leases', () => { expect(next.signal.aborted).toBe(true) }) + + it('cancels the running tools of every turn when the session ends', () => { + const first = leaseDesktopTool('turn-f') + const second = leaseDesktopTool('turn-g') + + stopAllDesktopTools('signed_out') + const next = leaseDesktopTool('turn-f') + + expect(first.signal.aborted).toBe(true) + expect(second.signal.aborted).toBe(true) + expect(first.signal.reason).toBe('signed_out') + expect(next.signal.aborted).toBe(false) + first.release() + next.release() + }) }) diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.ts index 8d0d2559de9..ed201ec2538 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.ts @@ -8,6 +8,8 @@ interface RunningTurnTools { * Turns with a desktop tool still running in this tab, keyed by the turn's stream id. A tool * outlives the chat view that started it (and any stream reader), so the turn, not the view, owns * its Stop. A turn is held only while one of its tools runs: each tool releases it as it settles. + * Terminal commands take no lease: Stop ends them through the turn's resource activity, which + * clears the agent's commands in each scope it touched. */ const runningTurns = new Map() @@ -49,3 +51,9 @@ export function stopDesktopTools(streamId: string, reason: string): void { runningTurns.get(streamId)?.stop.abort(reason) runningTurns.delete(streamId) } + +/** Cancels every running desktop tool in this tab, so none outlives the session that started it. */ +export function stopAllDesktopTools(reason: string): void { + for (const turn of runningTurns.values()) turn.stop.abort(reason) + runningTurns.clear() +} diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts index feb4fde0e4a..41fe7b51c47 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts @@ -1577,7 +1577,12 @@ export function useChat( ) const startClientLocalFilesystemTool = useCallback( - (toolCallId: string, toolName: string, toolArgs: Record) => { + ( + toolCallId: string, + toolName: string, + toolArgs: Record, + turnStreamId: string | undefined + ) => { if ( !isNativeFileTool(toolName) && (!workspaceId || !isUserLocalVfsToolCall(toolName, toolArgs)) @@ -1588,7 +1593,7 @@ export function useChat( return } handledClientLocalFilesystemToolIds.add(toolCallId) - const lease = streamIdRef.current ? leaseDesktopTool(streamIdRef.current) : undefined + const lease = turnStreamId ? leaseDesktopTool(turnStreamId) : undefined const options = { workspaceId, chatId: chatIdRef.current ?? selectedChatIdRef.current, @@ -2226,7 +2231,8 @@ export function useChat( addResource, removeResource, startClientWorkflowTool, - startClientLocalFilesystemTool, + startClientLocalFilesystemTool: (toolCallId, toolName, toolArgs) => + startClientLocalFilesystemTool(toolCallId, toolName, toolArgs, turnStreamId), startClientBrowserTool: startClientBrowserToolForStream, startClientTerminalTool: startClientTerminalToolForStream, startBrowserAgentRun: startBrowserAgentRunForStream, diff --git a/apps/sim/lib/mothership/generated/trace-attribute-values-v1.ts b/apps/sim/lib/mothership/generated/trace-attribute-values-v1.ts index c7312321dc3..553519db8f2 100644 --- a/apps/sim/lib/mothership/generated/trace-attribute-values-v1.ts +++ b/apps/sim/lib/mothership/generated/trace-attribute-values-v1.ts @@ -115,6 +115,7 @@ export type CopilotChatPersistOutcomeValue = export const CopilotConfirmOutcome = { Delivered: 'delivered', Forbidden: 'forbidden', + HeldByDesktop: 'held_by_desktop', InternalError: 'internal_error', RunNotFound: 'run_not_found', ToolCallNotFound: 'tool_call_not_found', diff --git a/apps/sim/lib/mothership/tools/client/browser-tool-execution.test.ts b/apps/sim/lib/mothership/tools/client/browser-tool-execution.test.ts index 06fd00b369d..fca9b753518 100644 --- a/apps/sim/lib/mothership/tools/client/browser-tool-execution.test.ts +++ b/apps/sim/lib/mothership/tools/client/browser-tool-execution.test.ts @@ -1470,9 +1470,12 @@ describe('pre-dispatch drops still resolve the waiter', () => { expect(mockReportCompletion).toHaveBeenCalledWith( 'stale-call-1', 'error', - expect.stringContaining('too late'), + expect.stringContaining('never started'), expect.objectContaining({ staleEvent: true }) ) + const [, , message] = mockReportCompletion.mock.calls[0] ?? [] + expect(message).toContain('Do not retry it in this turn') + expect(message).toContain('keep this chat open in the Sim desktop app') }) it('marks a stale stateful event outcome unknown and unsafe to retry', async () => { diff --git a/apps/sim/lib/mothership/tools/client/browser-tool-execution.ts b/apps/sim/lib/mothership/tools/client/browser-tool-execution.ts index e4e1739a571..404e30ee812 100644 --- a/apps/sim/lib/mothership/tools/client/browser-tool-execution.ts +++ b/apps/sim/lib/mothership/tools/client/browser-tool-execution.ts @@ -109,6 +109,8 @@ const OUTCOME_UNKNOWN_MESSAGE = '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.' const REPLAY_OUTCOME_UNKNOWN_MESSAGE = '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.' +const STALE_OBSERVATION_NOT_STARTED_MESSAGE = + '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.' const STALE_STATEFUL_OUTCOME_UNKNOWN_MESSAGE = '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.' const REPLAY_GUARD_CAPACITY_MESSAGE = @@ -593,7 +595,7 @@ export async function executeBrowserToolOnClient( logger.info('Skipping stale browser tool event', { toolCallId, toolName, age }) const observationOnly = OBSERVATION_ONLY_BROWSER_TOOLS[toolName] const message = observationOnly - ? 'This browser observation was delivered too late to run safely. Ask again to retry it.' + ? STALE_OBSERVATION_NOT_STARTED_MESSAGE : STALE_STATEFUL_OUTCOME_UNKNOWN_MESSAGE retainAndReportTerminalCompletion( toolCallId, diff --git a/apps/sim/lib/mothership/tools/client/completion.test.ts b/apps/sim/lib/mothership/tools/client/completion.test.ts index 48f1be02cd6..222e58cb300 100644 --- a/apps/sim/lib/mothership/tools/client/completion.test.ts +++ b/apps/sim/lib/mothership/tools/client/completion.test.ts @@ -115,6 +115,15 @@ describe('client tool completion reporting', () => { expect(signal?.aborted).toBe(true) }) + it('treats a 409 as final: the desktop app holds the call', async () => { + fetchMock.mockResolvedValue(new Response(null, { status: 409 })) + + await expect( + reportClientToolCompletionOnPageExit('tool-1', 'error', 'Browser failed') + ).resolves.toBeUndefined() + expect(fetchMock).toHaveBeenCalledOnce() + }) + it('rejects a non-success response', async () => { fetchMock.mockResolvedValue(new Response(null, { status: 503 })) diff --git a/apps/sim/lib/mothership/tools/client/completion.ts b/apps/sim/lib/mothership/tools/client/completion.ts index 5c97abcad23..9ea7ad5ee7e 100644 --- a/apps/sim/lib/mothership/tools/client/completion.ts +++ b/apps/sim/lib/mothership/tools/client/completion.ts @@ -145,7 +145,7 @@ export async function reportClientToolCompletionOnPageExit( }), keepalive: true, }) - if (!response.ok) { + if (!isSettledDelivery(response, toolCallId)) { throw new CompletionReportError(`Page-exit completion failed with status ${response.status}`) } } diff --git a/apps/sim/stores/index.test.ts b/apps/sim/stores/index.test.ts index caf42b86e29..3adca1b708b 100644 --- a/apps/sim/stores/index.test.ts +++ b/apps/sim/stores/index.test.ts @@ -13,6 +13,7 @@ vi.mock('@/stores/reset-all-stores', () => { return { resetAllStores: mockResetAllStores } }) +import { leaseDesktopTool } from '@/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes' import { clearUserData, RECENT_IMPERSONATIONS_STORAGE_KEY } from '@/stores' expect(mockModuleLoaded).not.toHaveBeenCalled() @@ -95,4 +96,19 @@ describe('clearUserData', () => { expect(inMemoryResetSucceeded).toBe(false) expect(localStorage.getItem('private-cache')).toBeNull() }) + + it('cancels desktop tools still running for the signed-out identity', async () => { + const localRead = leaseDesktopTool('turn-before-sign-out') + const browserAction = leaseDesktopTool('other-turn-before-sign-out') + mockResetAllStores.mockImplementationOnce(() => { + throw new Error('Chunk unavailable') + }) + + await clearUserData() + + expect(localRead.signal.aborted).toBe(true) + expect(browserAction.signal.aborted).toBe(true) + localRead.release() + browserAction.release() + }) }) diff --git a/apps/sim/stores/index.ts b/apps/sim/stores/index.ts index ffecaba5aec..a65e02a8846 100644 --- a/apps/sim/stores/index.ts +++ b/apps/sim/stores/index.ts @@ -1,11 +1,15 @@ 'use client' import { createLogger } from '@sim/logger' +import { stopAllDesktopTools } from '@/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes' const logger = createLogger('Stores') export const RECENT_IMPERSONATIONS_STORAGE_KEY = 'recent-impersonations' +/** Why desktop tools still running for the previous identity were cancelled. */ +const SIGNED_OUT_ABORT_REASON = 'identity_boundary:clearUserData' + interface ClearUserDataOptions { preserveRecentImpersonations?: boolean } @@ -21,6 +25,8 @@ export async function clearUserData(options: ClearUserDataOptions = {}): Promise let cleanupFailed = false let inMemoryResetSucceeded = true + stopAllDesktopTools(SIGNED_OUT_ABORT_REASON) + try { const keysToKeep = [ 'next-favicon', From b9c18978e080b3ae0bc70f43fb7d3340dc5956eb Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 6 Oct 2026 09:05:15 -0700 Subject: [PATCH 2/3] fix(mothership): accurate stale-observation wording and lease scope notes - A stale browser observation says it was not run this time, which stays true when an earlier delivery already ran it. - The lifetimes module says what a terminal call leaves running, and that sign-out ends leased tools only. - Tests check responses and recorded outcomes instead of mock calls. --- apps/sim/app/api/copilot/confirm/route.test.ts | 6 +++--- .../home/hooks/desktop-tool-lifetimes.ts | 11 ++++++++--- .../tools/client/browser-tool-execution.test.ts | 2 +- .../mothership/tools/client/browser-tool-execution.ts | 6 +++--- .../lib/mothership/tools/client/completion.test.ts | 1 - 5 files changed, 15 insertions(+), 11 deletions(-) diff --git a/apps/sim/app/api/copilot/confirm/route.test.ts b/apps/sim/app/api/copilot/confirm/route.test.ts index f2d8ff8dce2..183eb757989 100644 --- a/apps/sim/app/api/copilot/confirm/route.test.ts +++ b/apps/sim/app/api/copilot/confirm/route.test.ts @@ -332,10 +332,10 @@ describe('Copilot Confirm API Route', () => { ) expect(response.status).toBe(409) + expect(await response.json()).toEqual({ + error: 'The desktop app holds this tool call; only its own result settles it', + }) expect(recordedOutcome()).toBe(CopilotConfirmOutcome.HeldByDesktop) - expect(completeAsyncToolCall).not.toHaveBeenCalled() - expect(completeClaimedAsyncToolCall).not.toHaveBeenCalled() - expect(publishToolConfirmation).not.toHaveBeenCalled() }) it('does not publish when another terminal transition wins indeterminate claim reconciliation', async () => { diff --git a/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.ts b/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.ts index ed201ec2538..5189f62177f 100644 --- a/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.ts +++ b/apps/sim/app/workspace/[workspaceId]/home/hooks/desktop-tool-lifetimes.ts @@ -8,8 +8,10 @@ interface RunningTurnTools { * Turns with a desktop tool still running in this tab, keyed by the turn's stream id. A tool * outlives the chat view that started it (and any stream reader), so the turn, not the view, owns * its Stop. A turn is held only while one of its tools runs: each tool releases it as it settles. - * Terminal commands take no lease: Stop ends them through the turn's resource activity, which - * clears the agent's commands in each scope it touched. + * Terminal calls take no lease. A terminal call returns once its operation does (a `run` after + * its wait window), and the command it started keeps running in a terminal tab the user can see + * and control. Stop settles the agent's marks on that tab through the turn's resource activity; + * the process itself is the user's to end. */ const runningTurns = new Map() @@ -52,7 +54,10 @@ export function stopDesktopTools(streamId: string, reason: string): void { runningTurns.delete(streamId) } -/** Cancels every running desktop tool in this tab, so none outlives the session that started it. */ +/** + * Cancels every leased desktop tool running in this tab (browser actions, local file reads and + * imports), so none outlives the session that started it. + */ export function stopAllDesktopTools(reason: string): void { for (const turn of runningTurns.values()) turn.stop.abort(reason) runningTurns.clear() diff --git a/apps/sim/lib/mothership/tools/client/browser-tool-execution.test.ts b/apps/sim/lib/mothership/tools/client/browser-tool-execution.test.ts index fca9b753518..7c45358c7c8 100644 --- a/apps/sim/lib/mothership/tools/client/browser-tool-execution.test.ts +++ b/apps/sim/lib/mothership/tools/client/browser-tool-execution.test.ts @@ -1470,7 +1470,7 @@ describe('pre-dispatch drops still resolve the waiter', () => { expect(mockReportCompletion).toHaveBeenCalledWith( 'stale-call-1', 'error', - expect.stringContaining('never started'), + expect.stringContaining('not run this time'), expect.objectContaining({ staleEvent: true }) ) const [, , message] = mockReportCompletion.mock.calls[0] ?? [] diff --git a/apps/sim/lib/mothership/tools/client/browser-tool-execution.ts b/apps/sim/lib/mothership/tools/client/browser-tool-execution.ts index 404e30ee812..95c908ec0af 100644 --- a/apps/sim/lib/mothership/tools/client/browser-tool-execution.ts +++ b/apps/sim/lib/mothership/tools/client/browser-tool-execution.ts @@ -109,8 +109,8 @@ const OUTCOME_UNKNOWN_MESSAGE = '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.' const REPLAY_OUTCOME_UNKNOWN_MESSAGE = '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.' -const STALE_OBSERVATION_NOT_STARTED_MESSAGE = - '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.' +const STALE_OBSERVATION_NOT_RUN_MESSAGE = + 'Not run: this browser observation reached the Sim desktop app too late to run safely, so it was not run this time and has no result. An observation changes nothing 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.' const STALE_STATEFUL_OUTCOME_UNKNOWN_MESSAGE = '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.' const REPLAY_GUARD_CAPACITY_MESSAGE = @@ -595,7 +595,7 @@ export async function executeBrowserToolOnClient( logger.info('Skipping stale browser tool event', { toolCallId, toolName, age }) const observationOnly = OBSERVATION_ONLY_BROWSER_TOOLS[toolName] const message = observationOnly - ? STALE_OBSERVATION_NOT_STARTED_MESSAGE + ? STALE_OBSERVATION_NOT_RUN_MESSAGE : STALE_STATEFUL_OUTCOME_UNKNOWN_MESSAGE retainAndReportTerminalCompletion( toolCallId, diff --git a/apps/sim/lib/mothership/tools/client/completion.test.ts b/apps/sim/lib/mothership/tools/client/completion.test.ts index 222e58cb300..dcf3aefa4fd 100644 --- a/apps/sim/lib/mothership/tools/client/completion.test.ts +++ b/apps/sim/lib/mothership/tools/client/completion.test.ts @@ -121,7 +121,6 @@ describe('client tool completion reporting', () => { await expect( reportClientToolCompletionOnPageExit('tool-1', 'error', 'Browser failed') ).resolves.toBeUndefined() - expect(fetchMock).toHaveBeenCalledOnce() }) it('rejects a non-success response', async () => { From a37e9ea47b770447e464d3e8f01c0cc4d11c2f27 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 6 Oct 2026 09:40:01 -0700 Subject: [PATCH 3/3] revert(mothership): keep the confirm trace outcome vocabulary unchanged Drop the held_by_desktop outcome so this change needs no trace contract update: the 409 paths record tool_call_not_found as before. Keep a route test for a not-started report on a claimed call. --- .../sim/app/api/copilot/confirm/route.test.ts | 32 +------------------ apps/sim/app/api/copilot/confirm/route.ts | 12 ++++--- .../generated/trace-attribute-values-v1.ts | 1 - 3 files changed, 8 insertions(+), 37 deletions(-) diff --git a/apps/sim/app/api/copilot/confirm/route.test.ts b/apps/sim/app/api/copilot/confirm/route.test.ts index 183eb757989..8ca4757735b 100644 --- a/apps/sim/app/api/copilot/confirm/route.test.ts +++ b/apps/sim/app/api/copilot/confirm/route.test.ts @@ -1,9 +1,3 @@ -import { trace } from '@opentelemetry/api' -import { - BasicTracerProvider, - InMemorySpanExporter, - SimpleSpanProcessor, -} from '@opentelemetry/sdk-trace-base' import { copilotHttpMock, copilotHttpMockFns } from '@sim/testing' import { encryptionMock, encryptionMockFns } from '@sim/testing/mocks/encryption.mock' import { @@ -12,7 +6,7 @@ import { } from '@sim/testing/mocks/mothership-async-runs.mock' import { createMockRequest } from '@sim/testing/mocks/request.mock' import type { NextRequest } from 'next/server' -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' const { publishToolConfirmation, getTrustedWorkflowToolExecution } = vi.hoisted(() => ({ publishToolConfirmation: vi.fn(), @@ -33,9 +27,6 @@ vi.mock('@/lib/workflows/executor/execution-state', () => ({ getTrustedWorkflowToolExecution, })) -import { CopilotConfirmOutcome } from '@/lib/mothership/generated/trace-attribute-values-v1' -import { TraceAttr } from '@/lib/mothership/generated/trace-attributes-v1' -import { TraceSpan } from '@/lib/mothership/generated/trace-spans-v1' import { POST } from './route' const { @@ -49,17 +40,6 @@ const { const encryptSecret = encryptionMockFns.mockEncryptSecret -/** Records the confirm spans and returns a reader for the outcome the route recorded. */ -function recordConfirmOutcome(): () => unknown { - const exporter = new InMemorySpanExporter() - trace.setGlobalTracerProvider( - new BasicTracerProvider({ spanProcessors: [new SimpleSpanProcessor(exporter)] }) - ) - return () => - exporter.getFinishedSpans().find((span) => span.name === TraceSpan.CopilotConfirmToolResult) - ?.attributes[TraceAttr.CopilotConfirmOutcome] -} - describe('Copilot Confirm API Route', () => { const existingRow = { toolCallId: 'tool-call-123', @@ -71,10 +51,6 @@ describe('Copilot Confirm API Route', () => { claimedBy: 'workflow:execution-1', } - afterEach(() => { - trace.disable() - }) - beforeEach(() => { copilotHttpMockFns.mockAuthenticateCopilotRequestSessionOnly.mockResolvedValue({ userId: 'user-1', @@ -175,7 +151,6 @@ describe('Copilot Confirm API Route', () => { }) it('rejects a native success before the desktop authorization claim', async () => { - const recordedOutcome = recordConfirmOutcome() getAsyncToolCall.mockResolvedValue({ ...existingRow, toolName: 'browser_snapshot', @@ -191,7 +166,6 @@ describe('Copilot Confirm API Route', () => { ) expect(response.status).toBe(409) - expect(recordedOutcome()).toBe(CopilotConfirmOutcome.HeldByDesktop) expect(completeAsyncToolCall).not.toHaveBeenCalled() expect(detachAsyncToolCall).not.toHaveBeenCalled() expect(encryptSecret).not.toHaveBeenCalled() @@ -244,7 +218,6 @@ describe('Copilot Confirm API Route', () => { ] as const)( 'rejects a pending %s %s when the native authorization claim wins the race', async (toolName, status) => { - const recordedOutcome = recordConfirmOutcome() getAsyncToolCall.mockResolvedValue({ ...existingRow, toolName, @@ -264,7 +237,6 @@ describe('Copilot Confirm API Route', () => { expect(await response.json()).toEqual({ error: 'The desktop app holds this tool call; only its own result settles it', }) - expect(recordedOutcome()).toBe(CopilotConfirmOutcome.HeldByDesktop) expect(completePendingAsyncToolCall).toHaveBeenCalledOnce() expect(completeClaimedAsyncToolCall).not.toHaveBeenCalled() expect(completeAsyncToolCall).not.toHaveBeenCalled() @@ -314,7 +286,6 @@ describe('Copilot Confirm API Route', () => { ) it('refuses a not-started report for a call the desktop already claimed', async () => { - const recordedOutcome = recordConfirmOutcome() getAsyncToolCall.mockResolvedValue({ ...existingRow, toolName: 'browser_snapshot', @@ -335,7 +306,6 @@ describe('Copilot Confirm API Route', () => { expect(await response.json()).toEqual({ error: 'The desktop app holds this tool call; only its own result settles it', }) - expect(recordedOutcome()).toBe(CopilotConfirmOutcome.HeldByDesktop) }) it('does not publish when another terminal transition wins indeterminate claim reconciliation', async () => { diff --git a/apps/sim/app/api/copilot/confirm/route.ts b/apps/sim/app/api/copilot/confirm/route.ts index d9ea0e60b8f..392f90a284e 100644 --- a/apps/sim/app/api/copilot/confirm/route.ts +++ b/apps/sim/app/api/copilot/confirm/route.ts @@ -89,8 +89,7 @@ function acknowledgeSettledToolCall( * report raced that claim and lost), so only the claim's own result settles it. Final, not * retryable: the reporter stops. */ -function heldByAnotherReporterResponse(span: Span): NextResponse { - span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.HeldByDesktop) +function heldByAnotherReporterResponse(): NextResponse { return NextResponse.json( { error: 'The desktop app holds this tool call; only its own result settles it' }, { status: 409 } @@ -286,7 +285,8 @@ export const POST = withRouteHandler((req: NextRequest) => { ? isWorkflowToolExecutionClaimable(existing.status, existing.permissionDecision) : existing.status === ASYNC_TOOL_STATUS.running || isPreclaimNativeTerminalOutcome if (isNativeClientTool && !isMutableClientToolCall) { - return heldByAnotherReporterResponse(span) + span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound) + return heldByAnotherReporterResponse() } if (isWorkflowTool && !isMutableClientToolCall) { span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound) @@ -300,7 +300,8 @@ export const POST = withRouteHandler((req: NextRequest) => { data.notStarted === true && existing.status !== ASYNC_TOOL_STATUS.pending ) { - return heldByAnotherReporterResponse(span) + span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound) + return heldByAnotherReporterResponse() } let effectiveStatus = status @@ -436,7 +437,8 @@ export const POST = withRouteHandler((req: NextRequest) => { } if (reconciledOutcome === 'conflict' && isPreclaimNativeTerminalOutcome) { - return heldByAnotherReporterResponse(span) + span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound) + return heldByAnotherReporterResponse() } if (reconciledOutcome !== 'updated') { diff --git a/apps/sim/lib/mothership/generated/trace-attribute-values-v1.ts b/apps/sim/lib/mothership/generated/trace-attribute-values-v1.ts index 553519db8f2..c7312321dc3 100644 --- a/apps/sim/lib/mothership/generated/trace-attribute-values-v1.ts +++ b/apps/sim/lib/mothership/generated/trace-attribute-values-v1.ts @@ -115,7 +115,6 @@ export type CopilotChatPersistOutcomeValue = export const CopilotConfirmOutcome = { Delivered: 'delivered', Forbidden: 'forbidden', - HeldByDesktop: 'held_by_desktop', InternalError: 'internal_error', RunNotFound: 'run_not_found', ToolCallNotFound: 'tool_call_not_found',