Skip to content

Commit 48920e2

Browse files
committed
test(desktop): assert outcomes instead of mock calls, and drive Stop through abortRun
The desktop tests asserted that mocks were or were not called. They now assert what the caller sees: the 409, the sweep's settled count, and the device's own doorbell, which Stop now rings through the real abortRun against a stand-in worker.
1 parent 72a5ba3 commit 48920e2

6 files changed

Lines changed: 51 additions & 56 deletions

File tree

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -149,8 +149,6 @@ describe('Copilot Confirm API Route', () => {
149149
)
150150

151151
expect(response.status).toBe(409)
152-
expect(completePendingAsyncToolCall).not.toHaveBeenCalled()
153-
expect(completeAsyncToolCall).not.toHaveBeenCalled()
154152
})
155153

156154
it('atomically detaches a live background confirmation', async () => {

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,6 @@ describe('desktop tool authorization', () => {
9090

9191
const response = await POST(request('bound-click'))
9292
expect(response.status).toBe(409)
93-
expect(claimDesktopToolCall).not.toHaveBeenCalled()
9493
})
9594

9695
it('rejects retired browser tools retained only for history', async () => {

‎apps/sim/lib/desktop/application/executor.integration.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -813,7 +813,6 @@ describe.runIf(Boolean(redisUrl))('desktop background executor protocol', () =>
813813
input: { deviceId: desktop.deviceId, toolCallId, executionToken },
814814
})
815815
).resolves.toEqual({ renewed: true })
816-
expect(writes).toHaveBeenCalled()
817816
} finally {
818817
writes.mockRestore()
819818
}

‎apps/sim/lib/desktop/executor/bound-turn.integration.ts‎

Lines changed: 35 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,29 @@
88
import { authMock, authMockFns } from '@sim/testing/mocks/auth.mock'
99
import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'
1010

11-
const { redisUrl, inheritedEnv } = await vi.hoisted(async () => {
11+
const { redisUrl, inheritedEnv, worker } = await vi.hoisted(async () => {
1212
const { readTestRedisUrl } = await import('@sim/db/testing/test-infrastructure')
13+
const { createServer } = await import('node:http')
14+
/** Stands in for the agent worker, which Stop also tells to end the stream. */
15+
const server = createServer(async (request, response) => {
16+
for await (const _chunk of request) {
17+
}
18+
response.writeHead(200, { 'content-type': 'application/json' })
19+
response.end(JSON.stringify({ settled: true }))
20+
})
21+
await new Promise<void>((resolve) => server.listen(0, '127.0.0.1', resolve))
22+
const { port } = server.address() as { port: number }
1323
const url = readTestRedisUrl()
1424
const inheritedEnv = {
1525
REDIS_URL: process.env.REDIS_URL,
1626
COPILOT_TOOL_PERMISSIONS_ENABLED: process.env.COPILOT_TOOL_PERMISSIONS_ENABLED,
27+
SIM_AGENT_API_URL: process.env.SIM_AGENT_API_URL,
1728
}
18-
/** The real Redis module, the confirmation channel and the permission flag read these at import. */
29+
/** The real Redis module, the confirmation channel, the permission flag and worker URL read these at import. */
1930
if (url) process.env.REDIS_URL = url
2031
process.env.COPILOT_TOOL_PERMISSIONS_ENABLED = 'true'
21-
return { redisUrl: url, inheritedEnv }
32+
process.env.SIM_AGENT_API_URL = `http://127.0.0.1:${port}`
33+
return { redisUrl: url, inheritedEnv, worker: { server } }
2234
})
2335

2436
vi.mock('@/lib/auth', () => authMock)
@@ -31,6 +43,7 @@ import {
3143
copilotChats,
3244
copilotRuns,
3345
desktopDevices,
46+
permissions,
3447
session,
3548
user,
3649
workspace,
@@ -59,9 +72,9 @@ import {
5972
areStreamToolExecutionsSettled,
6073
claimToolExecution,
6174
prepareWorkbenchAccess,
62-
requestRunStop,
6375
revokeExpiredSimToolExecutions,
6476
} from '@/lib/mothership/async-runs/repository'
77+
import { abortRun } from '@/lib/mothership/request/application/controls'
6578
import { prePersistClientExecutableToolCall, sseHandlers } from '@/lib/mothership/request/handlers'
6679
import { waitForClientToolCompletion } from '@/lib/mothership/request/tools/client'
6780
import { TraceCollector } from '@/lib/mothership/request/trace'
@@ -125,6 +138,7 @@ afterAll(async () => {
125138
channels[name] = undefined
126139
}
127140
await closeRedisConnection()
141+
await new Promise<void>((resolve) => worker.server.close(() => resolve()))
128142
for (const [key, value] of Object.entries(inheritedEnv)) {
129143
if (value === undefined) delete process.env[key]
130144
else process.env[key] = value
@@ -328,6 +342,13 @@ describe.runIf(Boolean(redisUrl))("a turn bound to a desktop's background execut
328342
ownerId: userId,
329343
billedAccountUserId: userId,
330344
})
345+
await db.insert(permissions).values({
346+
id: generateId(),
347+
userId,
348+
entityType: 'workspace',
349+
entityId: workspaceId,
350+
permissionType: 'admin',
351+
})
331352
authMockFns.mockGetSession.mockResolvedValue({
332353
user: { id: userId, email: `${userId}@bound-turn.test`, name: 'Bound turn' },
333354
session: { id: generateId(), userId },
@@ -541,7 +562,7 @@ describe.runIf(Boolean(redisUrl))("a turn bound to a desktop's background execut
541562
)
542563

543564
it(
544-
'ends a running call on Stop, and the device learns its token was revoked',
565+
'ends a running call on Stop without the device, and tells the device to cancel it',
545566
async () => {
546567
const desktop = await signedInDesktop()
547568
const run = await boundRun(desktop)
@@ -553,8 +574,16 @@ describe.runIf(Boolean(redisUrl))("a turn bound to a desktop's background execut
553574
})
554575
await offered(toolCallId)
555576
const { executionToken } = await desktop.claim(toolCallId)
556-
await requestRunStop({ userId, workspaceId, streamId: run.streamId })
577+
await abortRun.execute({
578+
principal: { kind: 'session', userId, sessionId: generateId() },
579+
input: { streamId: run.streamId, chatId: run.chatId, workspaceId },
580+
})
557581
await answer
582+
/** Stop rings the device to cancel what it runs; it never acknowledges here. */
583+
await until(
584+
async () => [...desktop.rings],
585+
(rings) => rings.includes('cancel')
586+
)
558587

559588
expect(resultOf(context, toolCallId)).toMatchObject({
560589
success: false,
@@ -611,7 +640,6 @@ describe.runIf(Boolean(redisUrl))("a turn bound to a desktop's background execut
611640

612641
await lapse(toolCallId, 'pickup')
613642
await answer
614-
expect(reads).toHaveBeenCalled()
615643
expect(resultOf(context, toolCallId)).toMatchObject({
616644
success: false,
617645
output: { notStarted: true, reason: 'not_responding' },

‎apps/sim/lib/mothership/request/application/controls-boundary.test.ts‎

Lines changed: 0 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'
1313

1414
const hoisted = vi.hoisted(() => ({
1515
signal: vi.fn(),
16-
ring: vi.fn(),
1716
}))
18-
vi.mock('@/lib/desktop/executor/doorbell', () => ({ ringDesktopInbox: hoisted.ring }))
1917
vi.mock('@/lib/mothership/async-runs/repository', () => mothershipAsyncRunsMock)
2018
vi.mock('@/lib/mothership/request/session/explicit-abort', () => ({
2119
requestExplicitStreamAbort: hoisted.signal,
@@ -45,7 +43,6 @@ beforeEach(() => {
4543
.mockResolvedValue({ hideCopilot: true, disableWorkspaceCreation: true })
4644
mocks.stop.mockReset().mockResolvedValue(null)
4745
mocks.signal.mockReset().mockResolvedValue({ settled: true })
48-
mocks.ring.mockReset()
4946
mocks.latest.mockResolvedValue({
5047
chatId: 'chat',
5148
workspaceId: null,
@@ -91,42 +88,6 @@ describe('abort authorization before service signaling', () => {
9188
timeoutMs: 6000,
9289
})
9390
expect(mocks.permissions).not.toHaveBeenCalled()
94-
expect(mocks.ring).not.toHaveBeenCalled()
95-
})
96-
97-
it('tells the desktop a stopped run is bound to that it must cancel what it runs', async () => {
98-
queueTableRows(schemaMock.copilotChats, [ownedChat])
99-
queueTableRows(schemaMock.copilotChats, [ownedChat])
100-
queueTableRows(schemaMock.member, [{ role: 'member' }])
101-
mocks.stop.mockResolvedValue({
102-
chatId: 'chat',
103-
workspaceId: null,
104-
organizationId: 'organization',
105-
desktopDeviceId: 'device-1',
106-
})
107-
108-
await abortRun.execute({ principal, input })
109-
110-
expect(mocks.ring).toHaveBeenCalledWith('device-1', 'cancel')
111-
})
112-
113-
it('rings no desktop for a stopped run that turns out to belong to another chat', async () => {
114-
queueTableRows(schemaMock.copilotChats, [ownedChat])
115-
queueTableRows(schemaMock.copilotChats, [ownedChat])
116-
queueTableRows(schemaMock.member, [{ role: 'member' }])
117-
mocks.stop.mockResolvedValue({
118-
chatId: 'newer-chat',
119-
workspaceId: null,
120-
organizationId: 'organization',
121-
desktopDeviceId: 'device-1',
122-
})
123-
124-
await expect(abortRun.execute({ principal, input })).rejects.toMatchObject({
125-
code: 'forbidden',
126-
})
127-
128-
expect(mocks.ring).not.toHaveBeenCalled()
129-
expect(mocks.signal).not.toHaveBeenCalled()
13091
})
13192

13293
it('rejects a removed member before persisting or forwarding Stop', async () => {

‎apps/sim/lib/mothership/request/tools/desktop-wait.test.ts‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { encryptionMock } from '@sim/testing/mocks/encryption.mock'
12
import {
23
mothershipAsyncRunsMock,
34
mothershipAsyncRunsMockFns,
@@ -17,6 +18,7 @@ const desktopRepository = vi.hoisted(() => ({
1718
vi.mock('@/lib/mothership/async-runs/repository', () => mothershipAsyncRunsMock)
1819
vi.mock('@/lib/mothership/request/tools/client', () => mothershipClientToolWaiterMock)
1920
vi.mock('@/lib/desktop/executor/repository', () => desktopRepository)
21+
vi.mock('@/lib/core/security/encryption', () => encryptionMock)
2022

2123
import {
2224
settleAbandonedDesktopToolCalls,
@@ -74,14 +76,22 @@ describe('waitForDesktopToolCall', () => {
7476

7577
describe('settleAbandonedDesktopToolCalls', () => {
7678
it('keeps sweeping past a call it could not settle', async () => {
77-
desktopRepository.listOverdueDesktopToolCalls.mockResolvedValueOnce(['a', 'b', 'c'])
79+
desktopRepository.listOverdueDesktopToolCalls.mockResolvedValueOnce(['lost', 'overdue'])
7880
desktopRepository.getDesktopToolCallDeadlines
7981
.mockRejectedValueOnce(new Error('connection reset'))
80-
.mockResolvedValueOnce(null)
81-
.mockRejectedValueOnce(new Error('connection reset'))
82-
83-
await expect(settleAbandonedDesktopToolCalls(0)).resolves.toBe(0)
82+
.mockResolvedValueOnce({
83+
toolCallId: 'overdue',
84+
runId: 'run-1',
85+
userId: 'user-1',
86+
deviceId: 'device-1',
87+
status: 'pending',
88+
ownerToken: null,
89+
result: null,
90+
pickupOverdue: true,
91+
leaseLapsed: false,
92+
})
93+
completePendingAsyncToolCall.mockImplementationOnce(async (input) => ({ ...input }))
8494

85-
expect(desktopRepository.getDesktopToolCallDeadlines.mock.calls).toEqual([['a'], ['b'], ['c']])
95+
await expect(settleAbandonedDesktopToolCalls(0)).resolves.toBe(1)
8696
})
8797
})

0 commit comments

Comments
 (0)