Skip to content

Commit 262a715

Browse files
committed
fix(desktop): imports arrive whole, survive the rate limit, and record their folders
- The proxy no longer runs for the desktop's raw upload routes (import, and the browser file transfer that has the same shape). Running it made Next buffer the body and cut it off at its 10 MB proxy limit. The import route also requires a declared length and refuses a body that does not match it, so a cut-off file is never stored as complete. - A file entry with no body is refused instead of stored as an empty file. - A name Sim cannot store (one with a backslash) is refused for the whole import before anything lands, and at the contract. - A chunk that runs past the size the manifest listed is refused, like one that ends early. - A rate-limited entry is sent again after the limit clears. A 429 means nothing was stored, so a large tree no longer fails part way. - Folders an import creates are audited as folder creations and announced to the workspace's file views, as folders made by hand are. - Tests record what the fake Sim stored instead of asserting mock calls. The integration suite removes its audit rows.
1 parent 7419def commit 262a715

11 files changed

Lines changed: 227 additions & 39 deletions

File tree

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

Lines changed: 44 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { tmpdir } from 'node:os'
33
import { join } from 'node:path'
44
import type { TerminalToolResponse } from '@sim/terminal-protocol'
55
import { afterEach, describe, expect, it, vi } from 'vitest'
6+
import { DeviceRequestError } from '@/main/desktop-executor/client'
67
import type {
78
ClaimedDesktopCall,
89
DesktopImportEntryRequest,
@@ -140,19 +141,27 @@ describe('background imports', () => {
140141
}
141142
}
142143

143-
/** Records what reached Sim, with each file's bytes as text. */
144-
function recordingSim(fail?: (request: DesktopImportEntryRequest) => boolean) {
144+
/**
145+
* A fake Sim import route: records every entry it stores, with each file's bytes as text, and
146+
* the token each request presented. `answer` can refuse a request instead.
147+
*/
148+
function recordingSim(answer?: (request: DesktopImportEntryRequest) => Error | null) {
145149
const stored: Array<{ kind: string; relativePath: string; text?: string }> = []
146-
const importEntry = vi.fn(async (request: DesktopImportEntryRequest) => {
147-
if (fail?.(request)) throw new Error('Sim refused the entry')
150+
const tokens: string[] = []
151+
let requests = 0
152+
const importEntry = async (request: DesktopImportEntryRequest) => {
153+
requests += 1
154+
tokens.push(request.call.executionToken)
155+
const refusal = answer?.(request)
156+
if (refusal) throw refusal
148157
stored.push({
149158
kind: request.kind,
150159
relativePath: request.relativePath,
151160
...(request.content ? { text: await request.content.text() } : {}),
152161
})
153162
return { id: `id-${stored.length}`, name: request.relativePath || request.sourceName }
154-
})
155-
return { stored, importEntry }
163+
}
164+
return { stored, tokens, importEntry, requests: () => requests }
156165
}
157166

158167
it("stores a folder's tree in Sim, each file with the bytes on disk", async () => {
@@ -169,10 +178,7 @@ describe('background imports', () => {
169178
{ kind: 'directory', relativePath: 'q3' },
170179
{ kind: 'file', relativePath: 'q3/summary.txt', text: 'quarterly numbers' },
171180
])
172-
expect(sim.importEntry.mock.calls[0]?.[0]).toMatchObject({
173-
sourceName: 'Reports',
174-
call: { executionToken: 'token-import-1' },
175-
})
181+
expect(new Set(sim.tokens)).toEqual(new Set(['token-import-1']))
176182
expect(completion.data).toMatchObject({
177183
success: true,
178184
workspaceId: 'ws-1',
@@ -188,7 +194,9 @@ describe('background imports', () => {
188194
})
189195

190196
it('reports what landed when an import stops part way, and not to retry it', async () => {
191-
const sim = recordingSim((request) => request.relativePath === 'q3/summary.txt')
197+
const sim = recordingSim((request) =>
198+
request.relativePath === 'q3/summary.txt' ? new Error('Sim refused the entry') : null
199+
)
192200
const completion = await runner({ imports: { importEntry: sim.importEntry } }).run(
193201
importCall(await reportsFolder()),
194202
new AbortController().signal
@@ -207,18 +215,38 @@ describe('background imports', () => {
207215

208216
it('stores nothing more once the call is stopped', async () => {
209217
const controller = new AbortController()
210-
const sim = recordingSim()
211-
sim.importEntry.mockImplementationOnce(async (request) => {
218+
const sim = recordingSim(() => {
212219
controller.abort()
213-
return { id: 'id-root', name: request.sourceName }
220+
return null
214221
})
215222
const completion = await runner({ imports: { importEntry: sim.importEntry } }).run(
216223
importCall(await reportsFolder()),
217224
controller.signal
218225
)
219226

220227
expect(completion.status).toBe('error')
221-
expect(sim.importEntry).toHaveBeenCalledTimes(1)
228+
expect(sim.stored).toEqual([{ kind: 'directory', relativePath: '' }])
229+
})
230+
231+
it("waits out Sim's rate limit instead of failing the import part way", async () => {
232+
let limited = false
233+
const sim = recordingSim((request) => {
234+
if (request.relativePath !== 'notes.txt' || limited) return null
235+
limited = true
236+
return new DeviceRequestError(429, 'Too many requests', 10)
237+
})
238+
const completion = await runner({ imports: { importEntry: sim.importEntry } }).run(
239+
importCall(await reportsFolder()),
240+
new AbortController().signal
241+
)
242+
243+
expect(completion.status).toBe('success')
244+
expect(sim.stored.map((entry) => entry.relativePath)).toEqual([
245+
'',
246+
'notes.txt',
247+
'q3',
248+
'q3/summary.txt',
249+
])
222250
})
223251

224252
it('fails without storing anything when the source cannot be read', async () => {
@@ -229,7 +257,7 @@ describe('background imports', () => {
229257
)
230258

231259
expect(completion.status).toBe('error')
232-
expect(sim.importEntry).not.toHaveBeenCalled()
260+
expect(sim.requests()).toBe(0)
233261
expect(completion.data).toMatchObject({ workspaceId: 'ws-1', partial: false })
234262
})
235263
})

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

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,8 @@ import {
4444
import { getErrorMessage } from '@sim/utils/errors'
4545
import { interruptibleSleep } from '@sim/utils/helpers'
4646
import { isRecordLike } from '@sim/utils/object'
47+
import { backoffWithJitter } from '@sim/utils/retry'
48+
import { DeviceRequestError } from '@/main/desktop-executor/client'
4749
import type { DesktopToolRunner } from '@/main/desktop-executor/executor'
4850
import type {
4951
ClaimedDesktopCall,
@@ -54,6 +56,9 @@ import type {
5456
const logger = createLogger('DesktopExecutorRunner')
5557

5658
const USER_LOCAL_TOOLS: ReadonlySet<string> = new Set(['read', 'grep', 'glob'])
59+
/** A rate-limited import entry is retried this many times; Sim never stored a refused one. */
60+
const IMPORT_RATE_LIMIT_ATTEMPTS = 8
61+
const IMPORT_RATE_LIMIT_MAX_WAIT_MS = 30_000
5762

5863
/** The model learns a call never ran because a surface is switched off on this machine. */
5964
function surfaceOff(surface: string): DesktopToolCompletion {
@@ -231,6 +236,34 @@ export function createDesktopToolRunner(deps: DesktopToolRunnerDeps): DesktopToo
231236
* Imports the call's source into its workspace one entry at a time: each directory as a folder,
232237
* each file read in chunks and checked against the manifest that listed it.
233238
*/
239+
/**
240+
* Sends one entry, waiting out Sim's rate limit: a large tree can outrun it, and a 429 means
241+
* nothing was stored, so sending the entry again cannot duplicate it.
242+
*/
243+
async function importEntry(
244+
request: DesktopImportEntryRequest,
245+
signal: AbortSignal
246+
): Promise<DesktopImportedEntry> {
247+
for (let attempt = 1; ; attempt++) {
248+
try {
249+
return await deps.imports.importEntry(request, signal)
250+
} catch (error) {
251+
if (
252+
!(error instanceof DeviceRequestError) ||
253+
error.status !== 429 ||
254+
attempt >= IMPORT_RATE_LIMIT_ATTEMPTS
255+
) {
256+
throw error
257+
}
258+
await interruptibleSleep(
259+
backoffWithJitter(attempt, error.retryAfterMs, { maxMs: IMPORT_RATE_LIMIT_MAX_WAIT_MS }),
260+
signal
261+
)
262+
signal.throwIfAborted()
263+
}
264+
}
265+
}
266+
234267
async function runImport(
235268
call: ClaimedDesktopCall,
236269
signal: AbortSignal
@@ -250,12 +283,12 @@ export function createDesktopToolRunner(deps: DesktopToolRunnerDeps): DesktopToo
250283
signal.throwIfAborted()
251284
const target = { call, sourceName: manifest.name, relativePath: entry.relativePath }
252285
if (entry.kind === 'directory') {
253-
const folder = await deps.imports.importEntry({ ...target, kind: 'directory' }, signal)
286+
const folder = await importEntry({ ...target, kind: 'directory' }, signal)
254287
folders.push({ id: folder.id, relativePath: entry.relativePath })
255288
continue
256289
}
257290
const parts = await readImportEntry(call.toolCallId, entry, read, signal)
258-
const file = await deps.imports.importEntry(
291+
const file = await importEntry(
259292
{ ...target, kind: 'file', content: new Blob(parts) },
260293
signal
261294
)

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

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,16 @@ export const PUT = withRouteHandler(async (request: NextRequest) => {
4040
const parsed = await parseRequest(importDesktopEntryContract, request, {})
4141
if (!parsed.success) return parsed.response
4242
const query = parsed.data.query
43-
const declaredLength = Number(request.headers.get('content-length') ?? 0)
43+
const lengthHeader = request.headers.get('content-length')
44+
const declaredLength = Number(lengthHeader ?? 0)
4445
if (!Number.isFinite(declaredLength) || declaredLength > MAX_DESKTOP_IMPORT_FILE_BYTES) {
4546
return NextResponse.json(withRequestId({ error: TOO_LARGE }), { status: 413 })
4647
}
48+
if (query.kind === 'file' && lengthHeader === null) {
49+
return NextResponse.json(withRequestId({ error: 'A file import must declare its length' }), {
50+
status: 411,
51+
})
52+
}
4753

4854
try {
4955
await admitDesktopImportEntry(principal, query)
@@ -65,6 +71,13 @@ export const PUT = withRouteHandler(async (request: NextRequest) => {
6571
}
6672
throw error
6773
}
74+
// Anything between the device and here that cut the body short must not become a stored file.
75+
if (content.length !== declaredLength) {
76+
return NextResponse.json(
77+
withRequestId({ error: 'The file did not arrive whole; nothing was stored' }),
78+
{ status: 400 }
79+
)
80+
}
6881
}
6982

7083
try {

‎apps/sim/lib/api/contracts/desktop-executor.ts‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,9 @@ export const completeDesktopToolContract = defineRouteContract({
183183
error: z.object({ error: z.string() }),
184184
})
185185

186+
/** Workspace file names cannot hold a backslash, which a macOS or Linux file name can. */
187+
const BACKSLASH_NAME = 'Sim cannot store a file or folder whose name contains a backslash'
188+
186189
/** One relative path inside an import source, as the device's manifest lists it. */
187190
const desktopImportRelativePathSchema = z
188191
.string()
@@ -193,14 +196,21 @@ const desktopImportRelativePathSchema = z
193196
path.split('/').every((segment) => segment !== '' && segment !== '.' && segment !== '..'),
194197
'Relative path must stay inside the import source'
195198
)
199+
.refine((path) => !path.includes('\\'), BACKSLASH_NAME)
196200

197201
const importDesktopEntryQuerySchema = z.object({
198202
deviceId: desktopDeviceIdSchema,
199203
toolCallId: desktopToolCallIdSchema,
200204
executionToken: z.string().min(1).max(128),
201205
kind: z.enum(['file', 'directory']),
202206
/** The import source's own name: the folder a directory import lands in, or the file. */
203-
sourceName: z.string().trim().min(1, 'Source name is required').max(255),
207+
sourceName: z
208+
.string()
209+
.trim()
210+
.min(1, 'Source name is required')
211+
.max(255)
212+
.refine((name) => !name.includes('/'), 'Source name must be a single name')
213+
.refine((name) => !name.includes('\\'), BACKSLASH_NAME),
204214
relativePath: desktopImportRelativePathSchema,
205215
})
206216

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ vi.mock('@/lib/uploads/core/setup.server', () => ({
2020
import type { SessionPrincipal } from '@sim/auth/principal'
2121
import { db } from '@sim/db'
2222
import {
23+
auditLog,
2324
copilotAsyncToolCalls,
2425
copilotChats,
2526
copilotRuns,
@@ -49,6 +50,8 @@ describe('desktop imports', () => {
4950

5051
afterAll(async () => {
5152
if (userIds.length) {
53+
// Audit rows outlive their actor (the foreign key sets null), so they go first.
54+
await db.delete(auditLog).where(inArray(auditLog.actorId, userIds))
5255
await db.delete(workspace).where(inArray(workspace.ownerId, userIds))
5356
await db.delete(user).where(inArray(user.id, userIds))
5457
}
@@ -185,6 +188,30 @@ describe('desktop imports', () => {
185188
expect(stored).toBeDefined()
186189
})
187190

191+
it('records the folders an import creates, and only those', async () => {
192+
const claimed = await claimedImport()
193+
194+
const root = await entry(claimed, 'directory', '')
195+
await entry(claimed, 'directory', '')
196+
197+
const auditedRoot = async () =>
198+
(
199+
await db
200+
.select({ resourceId: auditLog.resourceId, action: auditLog.action })
201+
.from(auditLog)
202+
.where(eq(auditLog.workspaceId, claimed.workspaceId))
203+
).filter((row) => row.resourceId === root.id)
204+
await expect.poll(auditedRoot).toEqual([{ resourceId: root.id, action: 'folder.created' }])
205+
})
206+
207+
it('refuses a file entry with no bytes instead of storing an empty file', async () => {
208+
const claimed = await claimedImport()
209+
210+
await expect(entry(claimed, 'file', 'empty.txt')).rejects.toMatchObject({
211+
code: 'validation',
212+
})
213+
})
214+
188215
it('merges into folders that already exist and never overwrites a file', async () => {
189216
const claimed = await claimedImport()
190217
const first = await entry(claimed, 'directory', '')

‎apps/sim/lib/desktop/application/import.ts‎

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
1+
import { AuditAction, AuditResourceType } from '@sim/audit'
12
import type { Principal } from '@sim/auth/principal'
23
import { toRecord } from '@sim/utils/object'
34
import { OrchestrationError } from '@/lib/core/orchestration/types'
45
import { DesktopDeviceUnrecognizedError } from '@/lib/desktop/executor/errors'
56
import { getBoundDesktopCall, getBoundDesktopDevice } from '@/lib/desktop/executor/repository'
67
import { ASYNC_TOOL_STATUS } from '@/lib/mothership/async-runs/lifecycle'
78
import { getAsyncToolCall } from '@/lib/mothership/async-runs/repository'
9+
import { notifyWorkspaceFilesChanged } from '@/lib/realtime/notify'
810
import {
911
ensureWorkspaceFileChildFolder,
1012
loadActiveWorkspaceContext,
@@ -105,21 +107,25 @@ export const importDesktopEntry = defineAuthorizedWorkspaceFileUseCase({
105107
const segments = input.relativePath
106108
? [input.sourceName, ...input.relativePath.split('/')]
107109
: [input.sourceName]
108-
const folders = input.kind === 'directory' ? segments : segments.slice(0, -1)
110+
const folderSegments = input.kind === 'directory' ? segments : segments.slice(0, -1)
109111
let folderId = context.rootFolderId
110-
for (const name of folders) {
111-
folderId = await ensureWorkspaceFileChildFolder({
112+
const createdFolders: Array<{ id: string; name: string }> = []
113+
for (const name of folderSegments) {
114+
const folder = await ensureWorkspaceFileChildFolder({
112115
workspaceId: context.workspaceId,
113116
userId: context.importingUserId,
114117
parentId: folderId,
115118
name,
116119
})
120+
if (folder.created) createdFolders.push({ id: folder.id, name: folder.name })
121+
folderId = folder.id
117122
}
118123
const name = segments[segments.length - 1] ?? input.sourceName
119124
if (input.kind === 'directory') {
120125
if (!folderId) throw new OrchestrationError('validation', 'A directory needs a name')
121-
return { kind: 'directory' as const, id: folderId, name }
126+
return { kind: 'directory' as const, id: folderId, name, createdFolders }
122127
}
128+
if (!input.content) throw new OrchestrationError('validation', 'A file import needs its bytes')
123129
const created = await createAuthorizedWorkspaceFile({
124130
principal,
125131
input: {
@@ -129,11 +135,30 @@ export const importDesktopEntry = defineAuthorizedWorkspaceFileUseCase({
129135
folderId,
130136
exactName: false,
131137
},
132-
content: input.content ?? Buffer.alloc(0),
138+
content: input.content,
133139
workspace: context,
134140
})
135-
return { kind: 'file' as const, id: created.file.id, name: created.file.name, created }
141+
return {
142+
kind: 'file' as const,
143+
id: created.file.id,
144+
name: created.file.name,
145+
createdFolders,
146+
created,
147+
}
148+
},
149+
// Folders an import creates are recorded like folders made by hand; the file records itself.
150+
projectAudit: ({ result }) => [
151+
...result.createdFolders.map((folder) => ({
152+
action: AuditAction.FOLDER_CREATED,
153+
resourceType: AuditResourceType.FOLDER,
154+
resourceId: folder.id,
155+
resourceName: folder.name,
156+
description: `Created file folder "${folder.name}"`,
157+
})),
158+
...(result.kind === 'file' ? [projectCreateWorkspaceFileAudit(result.created)] : []),
159+
],
160+
async afterSuccess({ context, result }) {
161+
// A new file announces itself; a new folder is announced here, as folder creation does.
162+
if (result.createdFolders.length > 0) await notifyWorkspaceFilesChanged(context.workspaceId)
136163
},
137-
projectAudit: ({ result }) =>
138-
result.kind === 'file' ? projectCreateWorkspaceFileAudit(result.created) : [],
139164
})

0 commit comments

Comments
 (0)