FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(browser): reconcile native unload claims · simstudioai/sim@6029ab6 · GitHub

/ sim Public

Commit 6029ab6

Browse files
waleedlatif1
committed
fix(browser): reconcile native unload claims
1 parent de4464a commit 6029ab6

12 files changed

Lines changed: 316 additions & 34 deletions

File tree

‎apps/desktop/src/main/browser-agent/cdp.test.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -597,6 +597,29 @@ describe('browser-agent screenshot capture', () => {
597597
expect(shot.imageSize).toEqual({ width: 1024, height: 512 })
598598
})
599599

600+
it('accepts stable finite scroll offsets around the capture', async () => {
601+
const { contents } = captureFixture({ width: 1024, height: 512 })
602+
vi.mocked(contents.debugger.sendCommand).mockImplementation((method: string) => {
603+
if (method === 'Page.getLayoutMetrics') {
604+
return Promise.resolve({
605+
cssLayoutViewport: {
606+
clientWidth: 2048,
607+
clientHeight: 1024,
608+
pageX: 12,
609+
pageY: 34,
610+
},
611+
})
612+
}
613+
if (method === 'Page.captureScreenshot') return Promise.resolve({ data: 'c2lt' })
614+
return Promise.resolve(undefined)
615+
})
616+
617+
await expect(captureScreenshot(contents)).resolves.toMatchObject({
618+
viewport: { width: 2048, height: 1024 },
619+
imageSize: { width: 1024, height: 512 },
620+
})
621+
})
622+
600623
it.each([
601624
[
602625
'dimensions',

‎apps/desktop/src/main/browser-agent/cdp.ts‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -501,13 +501,6 @@ export async function captureScreenshot(contents: WebContents): Promise<Screensh
501501
if (size.width === 0 || size.height === 0) {
502502
return { dataUrl: captured, scale, viewport: cssViewport, imageSize: null }
503503
}
504-
// Without layout metrics there is no CSS frame of reference to resize
505-
// against, so the raw capture is the honest answer — the same fallback the
506-
// clipped path took. Its decoded size still lets the driver establish the
507-
// coordinate scale after obtaining the CSS viewport in-page.
508-
if (targetWidth <= 0 || targetHeight <= 0) {
509-
return { dataUrl: captured, scale, viewport: cssViewport, imageSize: size }
510-
}
511504
if (size.width === targetWidth && size.height === targetHeight) {
512505
return { dataUrl: captured, scale, viewport: cssViewport, imageSize: size }
513506
}

‎apps/desktop/src/main/browser-agent/driver.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2151,8 +2151,11 @@ async function executeToolInner(
21512151
'The page changed while its screenshot was being captured. Retry browser_screenshot before using image coordinates.'
21522152
)
21532153
}
2154-
const shot = await cdp.captureScreenshot(contents).catch(() => null)
2155-
if (shot === null) {
2154+
const shot = await cdp.captureScreenshot(contents).catch((error) => {
2155+
logger.warn('Browser screenshot capture failed', { error: getErrorMessage(error) })
2156+
return null
2157+
})
2158+
if (!shot) {
21562159
throw new ToolError(
21572160
'Could not capture the page. Use browser_snapshot or browser_read_text instead.'
21582161
)

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

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ const {
99
getAsyncToolCall,
1010
getRunSegment,
1111
completeAsyncToolCall,
12+
completeClaimedAsyncToolCall,
1213
completePendingAsyncToolCall,
1314
detachAsyncToolCall,
1415
publishToolConfirmation,
@@ -18,6 +19,7 @@ const {
1819
getAsyncToolCall: vi.fn(),
1920
getRunSegment: vi.fn(),
2021
completeAsyncToolCall: vi.fn(),
22+
completeClaimedAsyncToolCall: vi.fn(),
2123
completePendingAsyncToolCall: vi.fn(),
2224
detachAsyncToolCall: vi.fn(),
2325
publishToolConfirmation: vi.fn(),
@@ -31,6 +33,7 @@ vi.mock('@/lib/copilot/async-runs/repository', () => ({
3133
getAsyncToolCall,
3234
getRunSegment,
3335
completeAsyncToolCall,
36+
completeClaimedAsyncToolCall,
3437
completePendingAsyncToolCall,
3538
detachAsyncToolCall,
3639
getClaimedWorkflowExecutionId: (claimedBy?: string | null) =>
@@ -75,6 +78,7 @@ describe('Copilot Confirm API Route', () => {
7578
workflowId: 'workflow-from-run',
7679
})
7780
completeAsyncToolCall.mockResolvedValue(existingRow)
81+
completeClaimedAsyncToolCall.mockResolvedValue(existingRow)
7882
completePendingAsyncToolCall.mockResolvedValue(existingRow)
7983
detachAsyncToolCall.mockResolvedValue(existingRow)
8084
encryptSecret.mockResolvedValue({ encrypted: 'sealed-client-result', iv: 'iv' })
@@ -328,12 +332,95 @@ describe('Copilot Confirm API Route', () => {
328332
expect(response.status).toBe(404)
329333
expect(await response.json()).toEqual({ error: 'Pending client tool call not found' })
330334
expect(completePendingAsyncToolCall).toHaveBeenCalledOnce()
335+
expect(completeClaimedAsyncToolCall).not.toHaveBeenCalled()
331336
expect(completeAsyncToolCall).not.toHaveBeenCalled()
332337
expect(detachAsyncToolCall).not.toHaveBeenCalled()
333338
expect(publishToolConfirmation).not.toHaveBeenCalled()
334339
}
335340
)
336341

342+
it.each([
343+
['browser_snapshot', 'desktop-browser'],
344+
['terminal', 'desktop-terminal'],
345+
] as const)(
346+
'settles an indeterminate pending %s result when the exact %s claim wins the race',
347+
async (toolName, claimOwner) => {
348+
getAsyncToolCall.mockResolvedValue({
349+
...existingRow,
350+
toolName,
351+
status: 'pending',
352+
})
353+
completePendingAsyncToolCall.mockResolvedValueOnce(null)
354+
355+
const response = await POST(
356+
createMockPostRequest({
357+
toolCallId: 'tool-call-123',
358+
status: 'error',
359+
message: 'untrusted page-exit message',
360+
data: { outcomeUnknown: true, doNotRetry: true, untrusted: 'discard me' },
361+
})
362+
)
363+
364+
expect(response.status).toBe(200)
365+
expect(completePendingAsyncToolCall).toHaveBeenCalledOnce()
366+
expect(completeClaimedAsyncToolCall).toHaveBeenCalledWith(
367+
{
368+
toolCallId: 'tool-call-123',
369+
status: 'failed',
370+
result: { __sealedClientToolCompletionV1: 'sealed-client-result' },
371+
error: 'Tool failed',
372+
},
373+
claimOwner
374+
)
375+
expect(encryptSecret).toHaveBeenCalledWith(expect.stringContaining('"outcomeUnknown":true'))
376+
expect(encryptSecret).toHaveBeenCalledWith(expect.not.stringContaining('discard me'))
377+
expect(publishToolConfirmation).toHaveBeenCalledOnce()
378+
}
379+
)
380+
381+
it('does not publish when another terminal transition wins indeterminate claim reconciliation', async () => {
382+
getAsyncToolCall.mockResolvedValue({
383+
...existingRow,
384+
toolName: 'browser_snapshot',
385+
status: 'pending',
386+
})
387+
completePendingAsyncToolCall.mockResolvedValueOnce(null)
388+
completeClaimedAsyncToolCall.mockResolvedValueOnce(null)
389+
390+
const response = await POST(
391+
createMockPostRequest({
392+
toolCallId: 'tool-call-123',
393+
status: 'error',
394+
data: { outcomeUnknown: true, doNotRetry: true },
395+
})
396+
)
397+
398+
expect(response.status).toBe(404)
399+
expect(completeClaimedAsyncToolCall).toHaveBeenCalledWith(expect.any(Object), 'desktop-browser')
400+
expect(publishToolConfirmation).not.toHaveBeenCalled()
401+
})
402+
403+
it('returns 500 without publishing when exact claim reconciliation fails', async () => {
404+
getAsyncToolCall.mockResolvedValue({
405+
...existingRow,
406+
toolName: 'browser_snapshot',
407+
status: 'pending',
408+
})
409+
completePendingAsyncToolCall.mockResolvedValueOnce(null)
410+
completeClaimedAsyncToolCall.mockRejectedValueOnce(new Error('database unavailable'))
411+
412+
const response = await POST(
413+
createMockPostRequest({
414+
toolCallId: 'tool-call-123',
415+
status: 'error',
416+
data: { outcomeUnknown: true, doNotRetry: true },
417+
})
418+
)
419+
420+
expect(response.status).toBe(500)
421+
expect(publishToolConfirmation).not.toHaveBeenCalled()
422+
})
423+
337424
it.each(['error', 'cancelled'] as const)(
338425
'rejects a pending retired browser tool %s before a native claim',
339426
async (status) => {

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

Lines changed: 58 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,14 @@ import {
1111
ASYNC_TOOL_STATUS,
1212
type AsyncCompletionData,
1313
type AsyncConfirmationStatus,
14+
DESKTOP_TOOL_CLAIM_OWNER,
1415
isDeliveredAsyncStatus,
1516
isTerminalAsyncStatus,
1617
isWorkflowToolExecutionClaimable,
1718
} from '@/lib/copilot/async-runs/lifecycle'
1819
import {
1920
completeAsyncToolCall,
21+
completeClaimedAsyncToolCall,
2022
completePendingAsyncToolCall,
2123
detachAsyncToolCall,
2224
getAsyncToolCall,
@@ -53,12 +55,16 @@ import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
5355
import { getTrustedWorkflowToolExecution } from '@/lib/workflows/executor/execution-state'
5456

5557
const logger = createLogger('CopilotConfirmAPI')
58+
const NATIVE_HANDOFF_INTERRUPTED_MESSAGE =
59+
'The desktop action was interrupted during handoff. Its outcome is unknown; do not retry it automatically.'
5660

5761
type ToolCallStatusUpdateOutcome = 'updated' | 'conflict' | 'failed'
5862

5963
interface UpdateToolCallStatusOptions {
6064
executionId?: string
61-
requirePendingStatus?: boolean
65+
completionGuard?:
66+
| { status: typeof ASYNC_TOOL_STATUS.pending }
67+
| { status: typeof ASYNC_TOOL_STATUS.running; claimedBy: string }
6268
}
6369

6470
function getClientToolCompletionMessage(status: AsyncConfirmationStatus): string {
@@ -113,9 +119,12 @@ async function updateToolCallStatus(
113119
result: data ?? null,
114120
error: status === 'success' ? null : message || status,
115121
}
116-
const completed = options.requirePendingStatus
117-
? await completePendingAsyncToolCall(completionInput)
118-
: await completeAsyncToolCall(completionInput)
122+
const completed =
123+
options.completionGuard?.status === ASYNC_TOOL_STATUS.pending
124+
? await completePendingAsyncToolCall(completionInput)
125+
: options.completionGuard?.status === ASYNC_TOOL_STATUS.running
126+
? await completeClaimedAsyncToolCall(completionInput, options.completionGuard.claimedBy)
127+
: await completeAsyncToolCall(completionInput)
119128
if (!completed) return 'conflict'
120129
publishToolConfirmation({
121130
toolCallId,
@@ -283,6 +292,17 @@ export const POST = withRouteHandler((req: NextRequest) => {
283292
(isCurrentBrowserToolName(existing.toolName) || isTerminalToolName(existing.toolName)) &&
284293
existing.status === ASYNC_TOOL_STATUS.pending &&
285294
isErrorOrCancelledOutcome
295+
const nativeClaimOwner = isCurrentBrowserToolName(existing.toolName)
296+
? DESKTOP_TOOL_CLAIM_OWNER.browser
297+
: isTerminalToolName(existing.toolName)
298+
? DESKTOP_TOOL_CLAIM_OWNER.terminal
299+
: undefined
300+
const isIndeterminateNativeExit =
301+
isPreclaimNativeTerminalOutcome &&
302+
status === ASYNC_TOOL_CONFIRMATION_STATUS.error &&
303+
isPlainRecord(data) &&
304+
data.outcomeUnknown === true &&
305+
data.doNotRetry === true
286306
const isMutableClientToolCall = isWorkflowTool
287307
? isWorkflowToolExecutionClaimable(existing.status, existing.permissionDecision)
288308
: existing.status === ASYNC_TOOL_STATUS.running || isPreclaimNativeTerminalOutcome
@@ -377,8 +397,19 @@ export const POST = withRouteHandler((req: NextRequest) => {
377397
toolCallId,
378398
runId: existing.runId,
379399
userId: authenticatedUserId,
380-
...(message !== undefined ? { message } : {}),
381-
...(data !== undefined ? { data } : {}),
400+
...(isIndeterminateNativeExit
401+
? {
402+
message: NATIVE_HANDOFF_INTERRUPTED_MESSAGE,
403+
data: {
404+
error: NATIVE_HANDOFF_INTERRUPTED_MESSAGE,
405+
outcomeUnknown: true,
406+
doNotRetry: true,
407+
},
408+
}
409+
: {
410+
...(message !== undefined ? { message } : {}),
411+
...(data !== undefined ? { data } : {}),
412+
}),
382413
})),
383414
},
384415
}
@@ -390,16 +421,34 @@ export const POST = withRouteHandler((req: NextRequest) => {
390421
projected.data,
391422
{
392423
...(isWorkflowTool && executionId ? { executionId } : {}),
393-
requirePendingStatus: isPreclaimNativeTerminalOutcome,
424+
...(isPreclaimNativeTerminalOutcome
425+
? { completionGuard: { status: ASYNC_TOOL_STATUS.pending } as const }
426+
: {}),
394427
}
395428
)
396429

397-
if (updateOutcome === 'conflict' && isPreclaimNativeTerminalOutcome) {
430+
const reconciledOutcome =
431+
updateOutcome === 'conflict' && isIndeterminateNativeExit && nativeClaimOwner
432+
? await updateToolCallStatus(
433+
existing,
434+
ASYNC_TOOL_CONFIRMATION_STATUS.error,
435+
projected.message,
436+
projected.data,
437+
{
438+
completionGuard: {
439+
status: ASYNC_TOOL_STATUS.running,
440+
claimedBy: nativeClaimOwner,
441+
},
442+
}
443+
)
444+
: updateOutcome
445+
446+
if (reconciledOutcome === 'conflict' && isPreclaimNativeTerminalOutcome) {
398447
span.setAttribute(TraceAttr.CopilotConfirmOutcome, CopilotConfirmOutcome.ToolCallNotFound)
399448
return createNotFoundResponse('Pending client tool call not found')
400449
}
401450

402-
if (updateOutcome !== 'updated') {
451+
if (reconciledOutcome !== 'updated') {
403452
logger.error(`[${tracker.requestId}] Failed to update tool call status`, {
404453
userId: authenticatedUserId,
405454
toolCallId,

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { isRecordLike } from '@sim/utils/object'
44
import { type NextRequest, NextResponse } from 'next/server'
55
import { authorizeDesktopToolContract } from '@/lib/api/contracts/desktop-tool-authorization'
66
import { parseRequest } from '@/lib/api/server'
7+
import { DESKTOP_TOOL_CLAIM_OWNER } from '@/lib/copilot/async-runs/lifecycle'
78
import {
89
claimPendingAsyncToolCall,
910
getAsyncToolCall,
@@ -66,7 +67,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
6667
}
6768
const claimed = await claimPendingAsyncToolCall(
6869
toolCall.toolCallId,
69-
isBrowserTool ? 'desktop-browser' : 'desktop-terminal'
70+
isBrowserTool ? DESKTOP_TOOL_CLAIM_OWNER.browser : DESKTOP_TOOL_CLAIM_OWNER.terminal
7071
)
7172
if (!claimed) {
7273
return createNotFoundResponse('Pending client tool call not found')

‎apps/sim/lib/copilot/async-runs/lifecycle.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,11 @@ export const EXECUTABLE_TOOL_PERMISSION_DECISIONS = [
1212
'always_allow',
1313
] as const satisfies readonly CopilotToolPermissionDecision[]
1414

15+
export const DESKTOP_TOOL_CLAIM_OWNER = {
16+
browser: 'desktop-browser',
17+
terminal: 'desktop-terminal',
18+
} as const
19+
1520
export type AsyncLifecycleStatus =
1621
| typeof ASYNC_TOOL_STATUS.pending
1722
| typeof ASYNC_TOOL_STATUS.running

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL