Skip to content

Commit b6861b3

Browse files
authored
fix(desktop): poll desktop activity only for users with a desktop, keep XDG zsh configs integrated (#8785)
* fix(desktop): poll desktop activity only for users with a desktop, keep XDG zsh configs integrated * fix(desktop): give user zsh files their own ZDOTDIR, drop stale activity, require a live session * test(desktop): check delivered events and real requests, not mock calls
1 parent c93fe67 commit b6861b3

25 files changed

Lines changed: 462 additions & 67 deletions

‎apps/desktop/e2e/background-executor.spec.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -384,7 +384,10 @@ test.describe('background executor', () => {
384384
const deviceId = await registeredDevice(sim)
385385
const pulls = () =>
386386
sim.requests.filter((request) => request.startsWith('GET /api/desktop/inbox')).length
387+
const claimAttempts = () =>
388+
sim.requests.filter((request) => request === 'POST /api/desktop/tool/claim').length
387389
const pullsBefore = pulls()
390+
const claimAttemptsBefore = claimAttempts()
388391

389392
const gated = sim.issue(
390393
deviceId,
@@ -399,7 +402,8 @@ test.describe('background executor', () => {
399402

400403
await check('F: the declined call is never claimed, and its command never runs', async () => {
401404
await sleep(RECONCILE_MS * 2)
402-
expect(sim.requireCall(gated).claims).toBe(0)
405+
// Attempts, not successful claims: the fixture refuses a declined call's claim itself.
406+
expect(claimAttempts() - claimAttemptsBefore).toBe(0)
403407
expect(sim.requireCall(gated).completions).toEqual([])
404408
expect(readFileSafe(marker)).toBe('')
405409
})

‎apps/desktop/e2e/desktop-tools-live-sim.spec.ts‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ const ARRIVAL_MS = 60_000
4848
/** First requests to a route compile it, which takes minutes on a cold dev app. */
4949
const COMPILE_MS = 300_000
5050
const REGISTRATION_PATH = '/api/desktop/devices'
51+
const DESKTOP_COMPLETION_PATH = '/api/desktop/tool/complete'
5152
/** Sim's answer to registration on an install that cannot run the background executor. */
5253
const executorUnavailable = (answer: Record<string, unknown>) => {
5354
answer.enabled = false
@@ -120,6 +121,7 @@ test.describe('desktop tools against a live Sim', () => {
120121
await app?.close().catch(() => {})
121122
app = undefined
122123
proxy.rewriteAnswer(REGISTRATION_PATH, executorUnavailable)
124+
proxy.rewriteAnswer(DESKTOP_COMPLETION_PATH, undefined)
123125
rmSync(scratch, { recursive: true, force: true })
124126
})
125127

@@ -377,7 +379,7 @@ test.describe('desktop tools against a live Sim', () => {
377379
entry.method === 'POST' && entry.path === '/api/mothership/chat'
378380
/** The background executor's report of a call's result. */
379381
const isDesktopCompletion = (method: string, path: string) =>
380-
method === 'POST' && path === '/api/desktop/tool/complete'
382+
method === 'POST' && path === DESKTOP_COMPLETION_PATH
381383
/** A client tool's report of its own result. */
382384
const isToolReport = (method: string, path: string) =>
383385
method === 'POST' && path === '/api/copilot/confirm'
@@ -830,14 +832,16 @@ test.describe('desktop tools against a live Sim', () => {
830832
// Reaches Sim on release although the cut made the app give up on it, as a report already on
831833
// the wire would: the app cannot know it landed, so it reports again once back online.
832834
const completion = proxy.hold(isDesktopCompletion, { deliverIfAbandoned: true })
835+
const answers = proxy.recordAnswers(DESKTOP_COMPLETION_PATH)
833836
await send(page, '[network-cut] read my notes')
834837
await completion.arrival(ARRIVAL_MS, 'The result report')
835838

836839
proxy.cutNetwork()
837840
await expect.poll(() => completion.isAbandoned, { timeout: 15_000 }).toBe(true)
838841
completion.release()
839842
await expect.poll(() => callState(chatId), { timeout: 30_000 }).toMatch(/^completed/)
840-
await sleep(5_000)
843+
// The late report alone resumed the agent while the app was still offline.
844+
await agent.waitForResume(() => Boolean(agent.resultFor(callId)), 60_000)
841845
const restoredAt = Date.now()
842846
proxy.restoreNetwork()
843847

@@ -852,8 +856,10 @@ test.describe('desktop tools against a live Sim', () => {
852856
await expect.poll(() => retried().length, { timeout: 60_000 }).toBeGreaterThan(0)
853857
for (const entry of retried()) expect(entry.status).toBeLessThan(300)
854858

855-
await agent.waitForResume(() => Boolean(agent.resultFor(callId)), 60_000)
856859
await proxy.settled(30_000)
860+
// Sim had already recorded the late report, so every retry the app heard back from is a no-op.
861+
expect(answers.length).toBeGreaterThan(0)
862+
expect(answers).toEqual(answers.map(() => expect.objectContaining({ outcome: 'duplicate' })))
857863
const delivered = agent.resumes.filter((resume) =>
858864
resume.results.some((entry) => entry.callId === callId)
859865
)

‎apps/desktop/e2e/fixtures/live-sim.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,18 @@ export class SimProxy {
259259
else this.answerRewrites.delete(path)
260260
}
261261

262+
/**
263+
* Sim's JSON answers to `path` that reach the app from now on, recorded as they pass and left
264+
* unchanged. An answer to a request whose client gave up reaches no one, so it is not recorded.
265+
*/
266+
recordAnswers(path: string): Record<string, unknown>[] {
267+
const answers: Record<string, unknown>[] = []
268+
this.rewriteAnswer(path, (body) => {
269+
answers.push(structuredClone(body))
270+
})
271+
return answers
272+
}
273+
262274
/**
263275
* Cuts the network between the app and Sim: every open connection drops mid-flight, the
264276
* doorbell stream included, and new ones are reset until `restoreNetwork`.

‎apps/desktop/src/main/terminal/index.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1420,7 +1420,7 @@ export class TerminalService {
14201420
const screen = (await session.readScrollback(STARTUP_SCREEN_LINES)).output.trim()
14211421
throw new TerminalError(
14221422
'NO_SHELL_INTEGRATION',
1423-
`The shell has been running its startup files for over ${SHELL_STARTUP_BOUNDS.startingMs / 1000} s without reaching a prompt, so nothing was run. Its screen:\n${screen || '(empty)'}\nIf it is waiting for an answer, ask the user to answer it in that terminal (terminalId ${session.terminalId}), then run the command again.`
1423+
`The shell began its startup files over ${SHELL_STARTUP_BOUNDS.startingMs / 1000} s ago but never reached a prompt Sim can track, so nothing was run. Its screen:\n${screen || '(empty)'}\nIf a startup file is waiting for an answer, ask the user to answer it in that terminal (terminalId ${session.terminalId}), then run the command again. If the screen shows a prompt, a startup file replaced the shell (such as exec tmux or exec fish), so ask the user to run the command themselves.`
14241424
)
14251425
}
14261426
case 'not-instrumented':

‎apps/desktop/src/main/terminal/session.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -328,7 +328,6 @@ export class TerminalSession {
328328
private columns: number
329329
private lines: number
330330
private shellIntegration = false
331-
/** The shell has begun the startup files we generated, so its integration is on the way. */
332331
/** When the shell began the startup files we generated, or null while it has not. */
333332
private startupBegunAt: number | null = null
334333
private readonly spawnedAt = Date.now()

‎apps/desktop/src/main/terminal/shell-integration.test.ts‎

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { spawnSync } from 'node:child_process'
2-
import { existsSync, mkdtempSync, writeFileSync } from 'node:fs'
2+
import { existsSync, mkdirSync, mkdtempSync, writeFileSync } from 'node:fs'
33
import { tmpdir } from 'node:os'
44
import { join } from 'node:path'
55
import { describe, expect, it } from 'vitest'
@@ -140,6 +140,37 @@ describe('startup marker', () => {
140140
}
141141
)
142142

143+
it.skipIf(!existsSync('/bin/zsh'))(
144+
"zsh keeps integrating when the user's .zshenv moves ZDOTDIR, and runs all their files there",
145+
() => {
146+
const home = mkdtempSync(join(tmpdir(), 'sim-shell-home-'))
147+
const userDir = join(home, '.config', 'zsh')
148+
mkdirSync(userDir, { recursive: true })
149+
writeFileSync(join(home, '.zshenv'), 'export ZDOTDIR="$HOME/.config/zsh"\n')
150+
writeFileSync(join(userDir, '.zprofile'), 'USER_PROFILE_RAN=1\n')
151+
writeFileSync(join(userDir, 'plugins.zsh'), 'USER_PLUGIN_RAN=1\n')
152+
writeFileSync(join(userDir, '.zshrc'), 'USER_RC_RAN=1\nsource "$ZDOTDIR/plugins.zsh"\n')
153+
writeFileSync(join(userDir, '.zlogin'), 'echo "login=$USER_RC_RAN"\n')
154+
const env = { PATH: process.env.PATH ?? '/usr/bin:/bin', HOME: home }
155+
const launch = buildShellLaunch('zsh', mkdtempSync(join(tmpdir(), 'sim-zsh-')), NONCE, env)
156+
157+
const output = spawnSync(
158+
'/bin/zsh',
159+
[
160+
...launch.args,
161+
'-i',
162+
'-c',
163+
'echo "profile=$USER_PROFILE_RAN rc=$USER_RC_RAN plugin=$USER_PLUGIN_RAN zdotdir=$ZDOTDIR"; whence -w __sim_precmd',
164+
],
165+
{ env: { ...env, ...launch.env }, encoding: 'utf8' }
166+
).stdout
167+
168+
expect(output).toContain(`profile=1 rc=1 plugin=1 zdotdir=${userDir}`)
169+
expect(output).toContain('__sim_precmd: function')
170+
expect(output).toContain('login=1')
171+
}
172+
)
173+
143174
it.skipIf(!existsSync('/bin/bash'))("bash sends it before the user's files", () => {
144175
const home = userHome('.bashrc')
145176
const env = { PATH: process.env.PATH ?? '/usr/bin:/bin', HOME: home }

‎apps/desktop/src/main/terminal/shell-integration.ts‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -166,8 +166,17 @@ function findTerminator(buffer: string, from: number): { index: number; length:
166166
* prompt, aliases, and PATH win over ours.
167167
*/
168168
function writeZshFiles(dir: string, nonce: string, originalZdotdir: string): void {
169-
const sourceOriginal = (file: string) =>
170-
`[ -f "$SIM_ZDOTDIR_ORIG/${file}" ] && builtin source "$SIM_ZDOTDIR_ORIG/${file}"`
169+
// The user's file runs with ZDOTDIR at their own directory, so it finds its siblings and
170+
// plugins there. A file that moves ZDOTDIR (an XDG `.zshenv`) moves where the rest of theirs are
171+
// read from; ZDOTDIR then points back here, so zsh reads our next file.
172+
const sourceOriginal = (file: string) => `if [ -f "$SIM_ZDOTDIR_ORIG/${file}" ]; then
173+
__sim_zdotdir="$ZDOTDIR"
174+
ZDOTDIR="$SIM_ZDOTDIR_ORIG"
175+
builtin source "$SIM_ZDOTDIR_ORIG/${file}"
176+
SIM_ZDOTDIR_ORIG="\${ZDOTDIR:-$HOME}"
177+
ZDOTDIR="$__sim_zdotdir"
178+
builtin unset __sim_zdotdir
179+
fi`
171180

172181
// `.zshenv` is the first file zsh reads, so the startup marker goes out before any of the user's
173182
// files run. Only from an interactive shell: a script's output must not carry it.

‎apps/desktop/src/main/terminal/shell-startup.test.ts‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { TerminalService } from '@/main/terminal'
99
*/
1010
const pty = vi.hoisted(() => ({
1111
emit: null as ((data: string) => void) | null,
12+
exit: null as (() => void) | null,
1213
writes: [] as string[],
1314
}))
1415

@@ -18,7 +19,9 @@ vi.mock('@lydell/node-pty', () => ({
1819
onData: (handler: (data: string) => void) => {
1920
pty.emit = handler
2021
},
21-
onExit: () => {},
22+
onExit: (handler: () => void) => {
23+
pty.exit = handler
24+
},
2225
write: (data: string) => pty.writes.push(data),
2326
resize: vi.fn(),
2427
kill: vi.fn(),
@@ -71,6 +74,7 @@ beforeEach(() => {
7174
afterEach(() => {
7275
vi.useRealTimers()
7376
pty.emit = null
77+
pty.exit = null
7478
pty.writes.length = 0
7579
vi.unstubAllEnvs()
7680
})
@@ -122,6 +126,20 @@ describe('a shell that is still starting', () => {
122126
expect(response).toMatchObject({ ok: false, code: 'NO_SHELL_INTEGRATION' })
123127
expect(response.error).toContain('[oh-my-zsh] Would you like to update? [Y/n]')
124128
expect(response.error).toContain('ask the user to answer it')
129+
expect(response.error).toContain('a startup file replaced the shell')
130+
expect(pty.writes).toEqual([])
131+
terminal.dispose()
132+
})
133+
134+
it('reports the session closed when the shell exits while its startup files run', async () => {
135+
const terminal = new TerminalService({ loadCwd: () => '/tmp' })
136+
const running = terminal.executeTool('call-exit', 'run', { command: 'echo hi' })
137+
shell(STARTUP)
138+
await vi.advanceTimersByTimeAsync(2_000)
139+
pty.exit?.()
140+
const response = await running
141+
142+
expect(response).toMatchObject({ ok: false, code: 'SESSION_CLOSED' })
125143
expect(pty.writes).toEqual([])
126144
terminal.dispose()
127145
})

‎apps/sim/app/api/mothership/events/route.test.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -220,4 +220,25 @@ describe('Mothership owner-scoped event stream', () => {
220220
expect(authorize).not.toHaveBeenCalled()
221221
expect(authMockFns.mockGetSession).toHaveBeenCalledTimes(1)
222222
})
223+
224+
it('tells each member whether a workspace chat is their own, never whose it is', async () => {
225+
const abort = new AbortController()
226+
const response = await GET(request('workspaceId=ws-1', abort.signal))
227+
if (!response.body) throw new Error('The event stream has no body')
228+
const chunks: string[] = []
229+
const collected = collect(response.body, chunks)
230+
await vi.advanceTimersByTimeAsync(0)
231+
emit({ workspaceId: 'ws-1', userId: 'user-1', chatId: 'own-chat', type: 'started' })
232+
emit({ workspaceId: 'ws-1', userId: 'teammate-1', chatId: 'teammate-chat', type: 'started' })
233+
emit({ workspaceId: 'ws-1', chatId: 'unknown-owner-chat', type: 'started' })
234+
abort.abort()
235+
await collected
236+
const payloads = chunks.map((chunk) => JSON.parse(chunk.split('data: ')[1]))
237+
expect(payloads).toEqual([
238+
expect.objectContaining({ chatId: 'own-chat', ownChat: true }),
239+
expect.objectContaining({ chatId: 'teammate-chat', ownChat: false }),
240+
expect.not.objectContaining({ ownChat: expect.anything() }),
241+
])
242+
expect(chunks.join('')).not.toMatch(/userId|teammate-1|user-1/)
243+
})
223244
})

‎apps/sim/app/api/mothership/events/route.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,14 +30,16 @@ const mothershipEventsHandler = createWorkspaceSSE({
3030
label: 'mothership-events',
3131
subscriptions: [
3232
{
33-
subscribe: (workspaceId, send) => {
33+
subscribe: (workspaceId, send, viewerUserId) => {
3434
if (!chatPubSub) return () => {}
3535
return chatPubSub.onStatusChanged((event) => {
3636
if (event.workspaceId !== workspaceId) return
3737
send('task_status', {
3838
chatId: event.chatId,
3939
type: event.type,
4040
...(event.streamId ? { streamId: event.streamId } : {}),
41+
// Whether the chat is the viewer's own, never whose it is; absent when unknown.
42+
...(event.userId ? { ownChat: event.userId === viewerUserId } : {}),
4143
timestamp: Date.now(),
4244
})
4345
})

0 commit comments

Comments
 (0)