Skip to content

Commit 78e9300

Browse files
committed
fix(desktop): stop only the run's own tmux pane, stay dormant while the executor is off
- A tmux run's pane is tagged with a per-run @sim-run-id option. Stops and closes check that tag first and act on the run's own pane only, never the active pane or the whole window. A pane the user split beside it, or an id a restarted tmux server handed to one of the user's panes, is never touched. A run whose pane is gone is released. - With the executor off for this user, the app no longer opens the inbox or the doorbell. It rechecks registration on the existing triggers and every 15 minutes. Against a Sim without the executor routes (404 or 405) it stays dormant until the next launch, session change or toggle. - An inbox read that Sim answers after sign-out raises no approval notification and claims nothing. - A result this device cannot encode is reported once as outcome unknown, instead of being retried forever as if the network had failed. - Only a missing install-id file counts as no id; any other read failure is retried rather than minting a new id over the old one. - A generic terminal failure reports its code to the model again. - The doorbell, notifier and service tests observe state through typed fakes.
1 parent 5a9db49 commit 78e9300

15 files changed

Lines changed: 593 additions & 105 deletions

‎apps/desktop/src/main/desktop-executor/approval-notifier.test.ts‎

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { describe, expect, it, vi } from 'vitest'
1+
import { describe, expect, it } from 'vitest'
22
import { createApprovalNotifier } from '@/main/desktop-executor/approval-notifier'
33
import type { DesktopApprovalItem } from '@/main/desktop-executor/executor'
44

@@ -15,35 +15,41 @@ function approval(toolCallId: string, chatId = 'chat-b'): DesktopApprovalItem {
1515
}
1616

1717
function harness(options: { enabled?: boolean; focusedChatId?: string | null } = {}) {
18+
/** Each OS notification the notifier made, and what the user would see of it now. */
1819
const notifications: Array<{
1920
options: { title: string; body: string; silent: boolean }
20-
show: ReturnType<typeof vi.fn>
21-
close: ReturnType<typeof vi.fn>
21+
state: 'created' | 'shown' | 'closed'
2222
click: () => void
2323
}> = []
24-
const openRoute = vi.fn()
24+
/** Routes the app was asked to open. */
25+
const opened: Array<string | undefined> = []
2526
let focusedChatId = options.focusedChatId ?? null
2627
const notifier = createApprovalNotifier({
2728
preferences: () => ({
2829
notificationsEnabled: options.enabled ?? true,
2930
notificationSounds: true,
3031
}),
3132
focusedChatId: () => focusedChatId,
32-
openRoute,
33+
openRoute: (route) => {
34+
opened.push(route)
35+
},
3336
createNotification: (notificationOptions) => {
3437
let click = () => {}
3538
const notification = {
3639
options: notificationOptions,
37-
show: vi.fn(),
38-
close: vi.fn(),
40+
state: 'created' as 'created' | 'shown' | 'closed',
3941
get click() {
4042
return click
4143
},
4244
}
4345
notifications.push(notification)
4446
return {
45-
show: notification.show,
46-
close: notification.close,
47+
show: () => {
48+
notification.state = 'shown'
49+
},
50+
close: () => {
51+
notification.state = 'closed'
52+
},
4753
on: (_event, listener) => {
4854
click = listener
4955
},
@@ -53,7 +59,7 @@ function harness(options: { enabled?: boolean; focusedChatId?: string | null } =
5359
return {
5460
notifier,
5561
notifications,
56-
openRoute,
62+
opened,
5763
focus: (chatId: string | null) => {
5864
focusedChatId = chatId
5965
},
@@ -62,17 +68,17 @@ function harness(options: { enabled?: boolean; focusedChatId?: string | null } =
6268

6369
describe('approval notifications', () => {
6470
it('notifies once per waiting call and opens its chat, naming neither chat nor command', () => {
65-
const { notifier, notifications, openRoute } = harness()
71+
const { notifier, notifications, opened } = harness()
6672

6773
notifier.update([approval('call-1')])
6874
notifier.update([approval('call-1')])
6975

7076
expect(notifications).toHaveLength(1)
71-
expect(notifications[0]?.show).toHaveBeenCalledOnce()
77+
expect(notifications[0]?.state).toBe('shown')
7278
expect(notifications[0]?.options.body).not.toContain('rm -rf')
7379
expect(notifications[0]?.options.body).not.toContain('Fix CI')
7480
notifications[0]?.click()
75-
expect(openRoute).toHaveBeenCalledWith('/workspace/ws-1/chat/chat-b')
81+
expect(opened).toEqual(['/workspace/ws-1/chat/chat-b'])
7682
})
7783

7884
it('closes the notification once the call is decided', () => {
@@ -81,7 +87,7 @@ describe('approval notifications', () => {
8187

8288
notifier.update([])
8389

84-
expect(notifications[0]?.close).toHaveBeenCalledOnce()
90+
expect(notifications[0]?.state).toBe('closed')
8591
})
8692

8793
it('stays quiet for the chat the user is looking at, even after they leave it', () => {
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
import { describe, expect, it } from 'vitest'
2+
import { createDesktopExecutorClient, UnsendableRequestError } from '@/main/desktop-executor/client'
3+
4+
describe('desktop executor client', () => {
5+
it('refuses a request it cannot encode without sending anything', async () => {
6+
let sent = 0
7+
const client = createDesktopExecutorClient({
8+
origin: () => 'https://sim.test',
9+
fetch: async () => {
10+
sent += 1
11+
return Response.json({ outcome: 'recorded', status: 'completed' })
12+
},
13+
deviceId: '00000000-0000-4000-8000-000000000000',
14+
})
15+
const cyclic: Record<string, unknown> = {}
16+
cyclic.self = cyclic
17+
18+
await expect(
19+
client.complete({
20+
toolCallId: 'call-1',
21+
executionToken: 'token-1',
22+
completion: { status: 'success', message: 'done', data: cyclic },
23+
})
24+
).rejects.toBeInstanceOf(UnsendableRequestError)
25+
expect(sent).toBe(0)
26+
})
27+
})

‎apps/desktop/src/main/desktop-executor/client.ts‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,25 @@ export class DeviceRequestError extends Error {
4444
}
4545
}
4646

47+
/**
48+
* A request this device could not even encode (a result holding a cycle or a BigInt). Nothing was
49+
* sent, and sending it again cannot help.
50+
*/
51+
export class UnsendableRequestError extends Error {
52+
constructor(message: string) {
53+
super(message)
54+
this.name = 'UnsendableRequestError'
55+
}
56+
}
57+
58+
function encode(body: Record<string, unknown>): string {
59+
try {
60+
return JSON.stringify(body)
61+
} catch (error) {
62+
throw new UnsendableRequestError(getErrorMessage(error))
63+
}
64+
}
65+
4766
type DeviceFetch = (url: string, init: RequestInit) => Promise<Response>
4867

4968
export interface DesktopExecutorClient {
@@ -83,6 +102,7 @@ export function createDesktopExecutorClient(
83102
body?: Record<string, unknown>,
84103
signal?: AbortSignal
85104
): Promise<Response> {
105+
const encoded = body ? encode(body) : undefined
86106
const timeout = AbortSignal.timeout(REQUEST_TIMEOUT_MS)
87107
let response: Response
88108
try {
@@ -93,7 +113,7 @@ export function createDesktopExecutorClient(
93113
Accept: 'application/json',
94114
...(body ? { 'Content-Type': 'application/json' } : {}),
95115
},
96-
...(body ? { body: JSON.stringify(body) } : {}),
116+
...(encoded !== undefined ? { body: encoded } : {}),
97117
signal: signal ? AbortSignal.any([signal, timeout]) : timeout,
98118
})
99119
} catch (error) {

‎apps/desktop/src/main/desktop-executor/doorbell.test.ts‎

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,14 @@ function controllableStream(signal: AbortSignal) {
2727
function harness(options: { staleAfterMs?: number; retryBaseMs?: number } = {}) {
2828
const connections: ReturnType<typeof controllableStream>[] = []
2929
const failures: DeviceRequestError[] = []
30-
const onRing = vi.fn()
31-
const onUnregistered = vi.fn()
30+
/** How many times the doorbell rang, and asked to register again. */
31+
const counts = { rings: 0, unregistered: 0 }
32+
const onRing = () => {
33+
counts.rings += 1
34+
}
35+
const onUnregistered = () => {
36+
counts.unregistered += 1
37+
}
3238
const doorbell = new InboxDoorbell({
3339
client: {
3440
openInboxStream: async (signal) => {
@@ -44,7 +50,7 @@ function harness(options: { staleAfterMs?: number; retryBaseMs?: number } = {})
4450
retryBaseMs: 5,
4551
...options,
4652
})
47-
return { doorbell, connections, failures, onRing, onUnregistered }
53+
return { doorbell, connections, failures, counts }
4854
}
4955

5056
describe('parseServerSentEvents', () => {
@@ -59,12 +65,12 @@ describe('parseServerSentEvents', () => {
5965

6066
describe('InboxDoorbell', () => {
6167
it('rings on connect and on every inbox change', async () => {
62-
const { doorbell, connections, onRing } = harness()
68+
const { doorbell, connections, counts } = harness()
6369
doorbell.start()
64-
await vi.waitFor(() => expect(onRing).toHaveBeenCalledTimes(1))
70+
await vi.waitFor(() => expect(counts.rings).toBe(1))
6571

6672
connections[0]?.write('event: inbox_changed\ndata: {"reason":"call"}\n\n')
67-
await vi.waitFor(() => expect(onRing).toHaveBeenCalledTimes(2))
73+
await vi.waitFor(() => expect(counts.rings).toBe(2))
6874
doorbell.stop()
6975
})
7076

@@ -79,14 +85,14 @@ describe('InboxDoorbell', () => {
7985
})
8086

8187
it('reconnects after the server drops the stream or refuses a connection', async () => {
82-
const { doorbell, connections, failures, onRing } = harness()
88+
const { doorbell, connections, failures, counts } = harness()
8389
failures.push(new DeviceRequestError(0, 'offline'), new DeviceRequestError(502, 'deploy'))
8490
doorbell.start()
8591
await vi.waitFor(() => expect(connections).toHaveLength(1))
8692

8793
connections[0]?.end()
8894
await vi.waitFor(() => expect(connections).toHaveLength(2))
89-
expect(onRing).toHaveBeenCalledTimes(2)
95+
expect(counts.rings).toBe(2)
9096
doorbell.stop()
9197
})
9298

@@ -100,8 +106,8 @@ describe('InboxDoorbell', () => {
100106
return new ReadableStream<Uint8Array>({ start: (controller) => controller.close() })
101107
},
102108
},
103-
onRing: vi.fn(),
104-
onUnregistered: vi.fn(),
109+
onRing: () => {},
110+
onUnregistered: () => {},
105111
retryBaseMs: 5,
106112
})
107113
try {
@@ -126,23 +132,23 @@ describe('InboxDoorbell', () => {
126132
})
127133

128134
it('reports a device Sim no longer recognizes', async () => {
129-
const { doorbell, failures, onUnregistered } = harness()
135+
const { doorbell, failures, counts } = harness()
130136
failures.push(new DeviceRequestError(401, 'unregistered'))
131137
doorbell.start()
132138

133-
await vi.waitFor(() => expect(onUnregistered).toHaveBeenCalled())
139+
await vi.waitFor(() => expect(counts.unregistered).toBeGreaterThan(0))
134140
doorbell.stop()
135141
})
136142

137143
it('reconnects at once on wake, without waiting out a backoff', async () => {
138-
const { doorbell, connections, onRing } = harness({ retryBaseMs: 60_000 })
144+
const { doorbell, connections, counts } = harness({ retryBaseMs: 60_000 })
139145
doorbell.start()
140146
await vi.waitFor(() => expect(connections).toHaveLength(1))
141147

142148
doorbell.wake()
143149

144150
await vi.waitFor(() => expect(connections).toHaveLength(2))
145-
await vi.waitFor(() => expect(onRing).toHaveBeenCalledTimes(2))
151+
await vi.waitFor(() => expect(counts.rings).toBe(2))
146152
doorbell.stop()
147153
})
148154

‎apps/desktop/src/main/desktop-executor/executor.test.ts‎

Lines changed: 70 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,16 @@
11
import type { DesktopToolCompletion } from '@sim/desktop-bridge/tool-results'
22
import { sleep } from '@sim/utils/helpers'
33
import { describe, expect, it, vi } from 'vitest'
4-
import { type DesktopExecutorClient, DeviceRequestError } from '@/main/desktop-executor/client'
5-
import { DesktopExecutor, type DesktopToolRunner } from '@/main/desktop-executor/executor'
4+
import {
5+
type DesktopExecutorClient,
6+
DeviceRequestError,
7+
UnsendableRequestError,
8+
} from '@/main/desktop-executor/client'
9+
import {
10+
type DesktopApprovalItem,
11+
DesktopExecutor,
12+
type DesktopToolRunner,
13+
} from '@/main/desktop-executor/executor'
614
import type { ExecutorJournal, JournalEntry } from '@/main/desktop-executor/journal'
715
import type {
816
ClaimedDesktopCall,
@@ -148,6 +156,8 @@ class FakeRunner implements DesktopToolRunner {
148156
}
149157

150158
function setup(options: { leaseRenewMs?: number; maxHeldCalls?: number } = {}) {
159+
/** Approval items the executor handed to the notifier, one array per inbox read. */
160+
const approvals: DesktopApprovalItem[][] = []
151161
const sim = new FakeSim()
152162
const journal = new MemoryJournal()
153163
const runner = new FakeRunner()
@@ -161,9 +171,10 @@ function setup(options: { leaseRenewMs?: number; maxHeldCalls?: number } = {}) {
161171
retryBaseMs: 5,
162172
onUnregistered,
163173
onBusyChange: (value) => busy.push(value),
174+
onApprovals: (items) => approvals.push(items),
164175
...(options.maxHeldCalls ? { maxHeldCalls: options.maxHeldCalls } : {}),
165176
})
166-
return { sim, journal, runner, executor, onUnregistered, busy }
177+
return { sim, journal, runner, executor, onUnregistered, busy, approvals }
167178
}
168179

169180
describe('claiming', () => {
@@ -449,6 +460,34 @@ describe('delivery', () => {
449460
await vi.waitFor(() => expect(sim.completions).toHaveLength(1))
450461
})
451462

463+
it('reports a result it cannot encode as outcome unknown, once, instead of retrying it', async () => {
464+
const { sim, journal, executor } = setup()
465+
const complete = sim.client.complete
466+
let attempts = 0
467+
sim.client.complete = async (request) => {
468+
attempts += 1
469+
if (request.completion.data && 'cyclic' in request.completion.data) {
470+
throw new UnsendableRequestError('Converting circular structure to JSON')
471+
}
472+
return complete(request)
473+
}
474+
await journal.put({
475+
toolCallId: 'r-1',
476+
state: 'result',
477+
executionToken: 't-1',
478+
completion: { status: 'success', message: 'done', data: { cyclic: true } },
479+
})
480+
481+
await executor.recover()
482+
483+
await vi.waitFor(() => expect(sim.completions).toHaveLength(1))
484+
expect(attempts).toBe(2)
485+
expect(sim.completions[0]?.completion).toMatchObject({
486+
status: 'error',
487+
data: { outcomeUnknown: true, doNotRetry: true },
488+
})
489+
})
490+
452491
it('holds a result Sim refuses as unregistered until the device registers again', async () => {
453492
const { sim, journal, executor, onUnregistered } = setup()
454493
sim.completeErrors = [new DeviceRequestError(401, 'unregistered')]
@@ -609,6 +648,34 @@ describe('registration', () => {
609648
expect(journal.entries.size).toBe(0)
610649
})
611650

651+
it('raises no approval from an inbox read that Sim answers after sign-out', async () => {
652+
const { sim, executor, approvals } = setup()
653+
const answer = deferred<void>()
654+
const listInbox = sim.client.listInbox
655+
sim.client.listInbox = async () => {
656+
await answer.promise
657+
return listInbox()
658+
}
659+
sim.inbox = [
660+
{
661+
kind: 'approval_needed',
662+
toolCallId: 'gated-1',
663+
toolName: 'terminal',
664+
chatId: 'chat-a',
665+
chatTitle: 'Fix CI',
666+
workspaceId: 'ws-1',
667+
summary: 'npm publish',
668+
},
669+
]
670+
const reading = executor.reconcile()
671+
672+
const signingOut = executor.dispose()
673+
answer.resolve()
674+
await Promise.all([reading, signingOut])
675+
676+
expect(approvals).toEqual([])
677+
})
678+
612679
it('does not keep a claim that Sim answers after sign-out', async () => {
613680
const { sim, journal, runner, executor } = setup({ leaseRenewMs: 10 })
614681
const answer = deferred<void>()

0 commit comments

Comments
 (0)