Skip to content

Commit d41a1dd

Browse files
committed
test(copilot): pin the read caps, and correct what the limits actually claim
Audit follow-up. No behaviour change; the code was right and the comments explaining it were not. Measured the bomb through this exact pipeline rather than reasoning about it (100MP/256MP/576MP/1024MP): libvips decodes sequentially, so peak RSS stays flat in the tens of MB no matter what the header declares. What scales is CPU, roughly linearly — ~240ms at 100MP, ~1.35s at 1024MP, once per resize rung. The pixel budget is a CPU bound, not the memory bound the comment described, and the "~400MB raster" arithmetic was wrong by an order of magnitude. - say that, with the measurements, so the next reader tunes against the real cost - stop claiming the source byte cap covers everything a user can upload: presigned and multipart accept gigabytes, so an image above it is stored fine and simply cannot be read inline. Deliberate trade, now stated as one - correct the resize-ladder comment, which asserted the failure is always a decode when the try also wraps the encoder - correct two claims in read-placeholders: the handler does pull the VFS, and the oversized set excludes an image size refusal it said it included Tests: cover the document and compiled-artifact size refusals (the document one was the newest behaviour change and had no coverage at all), the text and document download caps, and that the cap is handed to the download rather than merely producing the right message. Placeholder cases are built from the producers instead of hand-copied — a literal only proves the matcher agrees with the test, which is the drift this module exists to prevent.
1 parent 6d2bf05 commit d41a1dd

5 files changed

Lines changed: 128 additions & 50 deletions

File tree

apps/sim/lib/copilot/tools/handlers/vfs.test.ts

Lines changed: 23 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ vi.mock('./upload-file-reader', () => ({
5151
}))
5252

5353
import { WorkspaceFileGrepError } from '@/lib/copilot/vfs/operations'
54+
import { readPlaceholder } from '@/lib/copilot/vfs/read-placeholders'
5455
import { executeVfsGlob, executeVfsGrep, executeVfsRead } from './vfs'
5556

5657
const OVERSIZED_INLINE_CONTENT = 'x'.repeat(TOOL_RESULT_MAX_INLINE_CHARS + 1)
@@ -179,31 +180,41 @@ describe('vfs handlers oversize policy', () => {
179180
expect((result.output as { attachment?: { type: string } })?.attachment?.type).toBe('file')
180181
})
181182

182-
it('fails oversized image placeholder when image exceeds size limit', async () => {
183+
/**
184+
* Every size refusal is a failed read, whichever path produced it. Built from the
185+
* producers so a prefix leaving `OVERSIZED_PREFIXES` fails here rather than
186+
* silently downgrading a refusal to a one-line "successful" read.
187+
*/
188+
it.each([
189+
['image', readPlaceholder.imageTooLarge('huge.png', 99, 5)],
190+
['file', readPlaceholder.fileTooLarge('huge.txt', 99, 5)],
191+
['document', readPlaceholder.documentTooLarge('huge.pdf', 99, 5)],
192+
['compiled artifact', readPlaceholder.compiledArtifactTooLarge('app.js', 99, 5)],
193+
])('fails the read when a %s exceeds its size limit', async (_kind, content) => {
183194
const vfs = makeVfs()
184-
vfs.readFileContent.mockResolvedValue({
185-
content: '[Image too large to read inline: huge.png (26214401 bytes, limit 26214400)]',
186-
totalLines: 1,
187-
})
195+
vfs.readFileContent.mockResolvedValue({ content, totalLines: 1 })
188196
getOrMaterializeVFS.mockResolvedValue(vfs)
189197

190198
const result = await executeVfsRead(
191-
{ path: 'files/huge.png/content' },
199+
{ path: 'files/huge/content' },
192200
{ userId: 'user-1', workflowId: 'wf-1', workspaceId: 'ws-1' }
193201
)
194202

195203
expect(result.success).toBe(false)
196-
expect(result.error).toContain('too large')
204+
// The placeholder verbatim, not the generic "grep this instead" fallback.
205+
expect(result.error).toBe(content)
197206
})
198207

199208
it('returns an undecodable image placeholder as content, not as a size failure', async () => {
200209
const vfs = makeVfs()
201210
// Not a size problem — the bytes were read fine and the reason is already in the
202211
// message, so the model should see it rather than a "too large, use grep" error.
203-
vfs.readFileContent.mockResolvedValue({
204-
content: '[Image unavailable: bomb.png (90 Bytes). It is too large to decode safely.]',
205-
totalLines: 1,
206-
})
212+
const content = readPlaceholder.imageUnavailable(
213+
'bomb.png',
214+
90,
215+
'It is too large to decode safely.'
216+
)
217+
vfs.readFileContent.mockResolvedValue({ content, totalLines: 1 })
207218
getOrMaterializeVFS.mockResolvedValue(vfs)
208219

209220
const result = await executeVfsRead(
@@ -212,6 +223,7 @@ describe('vfs handlers oversize policy', () => {
212223
)
213224

214225
expect(result.success).toBe(true)
226+
expect((result.output as { content?: string })?.content).toBe(content)
215227
})
216228

217229
it('reads canonical file leaf metadata without fetching dynamic content', async () => {

apps/sim/lib/copilot/vfs/file-reader.test.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ vi.mock('@/lib/uploads/contexts/workspace/workspace-file-manager', () => ({
1717
import {
1818
MAX_IMAGE_READ_BYTES,
1919
MAX_IMAGE_SOURCE_BYTES,
20+
MAX_PARSEABLE_READ_BYTES,
21+
MAX_TEXT_READ_BYTES,
2022
readFileRecord,
2123
} from '@/lib/copilot/vfs/file-reader'
2224
import { PayloadSizeLimitError } from '@/lib/core/utils/stream-limits'
@@ -108,8 +110,41 @@ describe('readFileRecord', () => {
108110
expect(result?.content).toContain('Image too large to read inline')
109111
// The observed size, not the understated 1024 the cap exists to distrust.
110112
expect(result?.content).toContain(`${MAX_IMAGE_SOURCE_BYTES + 5_000} bytes`)
113+
// And the cap was actually handed to the download — the placeholder alone would
114+
// still appear if the argument were dropped, since the mock rejects regardless.
115+
expect(fetchWorkspaceFileBuffer).toHaveBeenCalledWith(expect.anything(), {
116+
maxBytes: MAX_IMAGE_SOURCE_BYTES,
117+
})
111118
})
112119

120+
it.each([
121+
['text', 'notes.txt', 'text/plain', MAX_TEXT_READ_BYTES, 'File too large to display inline'],
122+
[
123+
'document',
124+
'report.pdf',
125+
'application/pdf',
126+
MAX_PARSEABLE_READ_BYTES,
127+
'Document too large to parse inline',
128+
],
129+
])(
130+
'caps the %s download and reports the observed size when it breaches',
131+
async (_kind, name, type, cap, expected) => {
132+
fetchWorkspaceFileBuffer.mockRejectedValue(
133+
new PayloadSizeLimitError({
134+
label: 'workspace file',
135+
maxBytes: cap,
136+
observedBytes: cap + 7_000,
137+
})
138+
)
139+
140+
const result = await readFileRecord(imageRecord(name, 1024, type))
141+
142+
expect(result?.content).toContain(expected)
143+
expect(result?.content).toContain(`${cap + 7_000} bytes`)
144+
expect(fetchWorkspaceFileBuffer).toHaveBeenCalledWith(expect.anything(), { maxBytes: cap })
145+
}
146+
)
147+
113148
it('reports an oversized HEIF as a size refusal, not as a corrupt file', async () => {
114149
// `ftyp`+`heic` brand, past the WebAssembly transcoder's own tighter ceiling.
115150
const heif = Buffer.alloc(MAX_TRANSCODE_INPUT_BYTES + 1)

apps/sim/lib/copilot/vfs/file-reader.ts

Lines changed: 29 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -47,25 +47,33 @@ export const MAX_IMAGE_READ_BYTES = 5 * 1024 * 1024 // 5 MB
4747
// Parseable-document byte cap. Large office/PDF files can still
4848
// produce huge extracted text; reject up front to avoid wasting a
4949
// download + parse only to blow past the tool-result budget.
50-
const MAX_PARSEABLE_READ_BYTES = 5 * 1024 * 1024 // 5 MB
50+
export const MAX_PARSEABLE_READ_BYTES = 5 * 1024 * 1024 // 5 MB
5151
/**
52-
* Source-image byte ceiling, checked before the download and enforced by it. A
53-
* workspace file may be up to {@link MAX_WORKSPACE_FILE_SIZE}, and buffering one of
54-
* those whole would exhaust memory on its own — the pixel budget below bounds the
55-
* decode, not the transfer.
52+
* Source-image byte ceiling, checked before the download and enforced by it. This
53+
* route holds the whole file in worker memory, so it reuses the ceiling the FormData
54+
* upload route set for that same failure mode rather than inventing a number.
5655
*
57-
* Deliberately the FormData upload ceiling rather than a number of its own: that cap
58-
* exists for exactly this failure mode (a route holding an entire file in worker
59-
* memory), so anything a user could upload through it stays readable here. Picking
60-
* something tighter would refuse images that read fine today for no security gain —
61-
* a decompression bomb is small, and it is the pixel budget that stops it.
56+
* It does NOT cover everything a user can store: presigned and multipart uploads
57+
* accept up to `MAX_WORKSPACE_FILE_SIZE` (gigabytes), so an image above this ceiling
58+
* is stored fine and simply cannot be read inline. That is a deliberate trade — the
59+
* alternative is buffering a multi-gigabyte file to answer one read — and it is a
60+
* memory budget, not part of the decompression-bomb defence, which is the pixel
61+
* budget below. A bomb is small; no byte cap would catch it.
6262
*/
6363
export const MAX_IMAGE_SOURCE_BYTES = MAX_WORKSPACE_FORMDATA_FILE_SIZE
6464
/**
65-
* Pixel ceiling on the decoded raster. libvips materialises the whole raster, and
66-
* an allocation this large OOM-kills the process rather than throwing, so it has to
67-
* be refused up front. 100MP caps the decode near 400MB while clearing every real
68-
* camera — a 48MP iPhone still is 8064x6048.
65+
* Pixel ceiling on the decoded image, and the actual decompression-bomb defence: a
66+
* few hundred KB of PNG can declare an arbitrarily large raster.
67+
*
68+
* The cost it bounds is CPU, not memory. libvips decodes this pipeline sequentially,
69+
* so peak RSS stays flat (tens of MB) no matter what the header declares — measured
70+
* on this exact pipeline, 100MP..1024MP all sat under ~120MB. What scales is time,
71+
* roughly linearly: ~240ms at 100MP, ~1.35s at 1024MP, once per resize rung. So the
72+
* budget caps what one read can burn, and the `break` below caps how many rungs a
73+
* failing image gets.
74+
*
75+
* 100MP clears every single-shot camera (a 48MP iPhone still is 8064x6048) but will
76+
* refuse a stitched gigapixel panorama, which is the known cost of the ceiling.
6977
*/
7078
const MAX_IMAGE_INPUT_PIXELS = 100_000_000
7179
const MAX_IMAGE_DIMENSION = 1568
@@ -381,9 +389,13 @@ async function prepareImageForVision(
381389
}
382390
}
383391
} catch (err) {
384-
// Next dimension, not next quality: the quality rungs re-decode the
385-
// identical source, so repeating a failed decode there is pure waste.
386-
// A smaller dimension is worth trying — libvips shrinks JPEG on load.
392+
// Next dimension, not next quality: every quality rung re-decodes the
393+
// same source and only varies the encoder, so a failure here almost
394+
// always repeats. Dropping a dimension is the one thing that can change
395+
// the outcome (JPEG shrinks on load), and it bounds a bomb at 4 decodes
396+
// instead of 16. A genuinely encoder-only failure would lose its lower
397+
// quality rungs at that dimension — no such failure mode is known, and
398+
// 4 attempts is the deliberate ceiling.
387399
logger.warn('Failed image resize attempt for VFS read', {
388400
mediaType,
389401
dimension,

apps/sim/lib/copilot/vfs/operations.test.ts

Lines changed: 34 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
*/
44
import { describe, expect, it } from 'vitest'
55
import { glob, grep, grepReadResult, WorkspaceFileGrepError } from '@/lib/copilot/vfs/operations'
6+
import { readPlaceholder } from '@/lib/copilot/vfs/read-placeholders'
67

78
function vfsFromEntries(entries: [string, string][]): Map<string, string> {
89
return new Map(entries)
@@ -200,21 +201,42 @@ describe('grepReadResult placeholders', () => {
200201
grepReadResult('files/x.png/content', { content, totalLines: 1 }, 'x', 'files/x.png/content')
201202

202203
/**
203-
* Every `readFileRecord` placeholder carries no searchable text, so grep must
204-
* report the placeholder rather than matching against its own prose.
204+
* Built from the producers rather than hand-copied: a literal here would only
205+
* prove the matcher agrees with this file, which is exactly the drift that let a
206+
* gate test for a prefix no producer emitted. Covers every builder, so dropping
207+
* one from the shared table fails here.
205208
*/
206-
it.each([
207-
'[Image unavailable: bomb.png (90 Bytes). It is too large to decode safely.]',
208-
'[Image too large to read inline: huge.png (26214401 bytes, limit 26214400)]',
209-
'[File too large to display inline: big.txt (99 bytes, limit 5)]',
210-
'[Document too large to parse inline: big.pdf (99 bytes, limit 5)]',
211-
'[Binary file: app.bin (application/octet-stream, 10 bytes). Cannot display as text.]',
212-
])('reports %s instead of grepping it', (content) => {
213-
expect(() => grepPlaceholder(content)).toThrow(WorkspaceFileGrepError)
214-
expect(() => grepPlaceholder(content)).toThrow(content)
215-
})
209+
const everyPlaceholder = Object.entries({
210+
fileTooLarge: readPlaceholder.fileTooLarge('big.txt', 99, 5),
211+
imageTooLarge: readPlaceholder.imageTooLarge('huge.png', 99, 5),
212+
imageUnavailable: readPlaceholder.imageUnavailable('bomb.png', 90, 'It could not be decoded.'),
213+
documentTooLarge: readPlaceholder.documentTooLarge('big.pdf', 99, 5),
214+
compiledArtifactTooLarge: readPlaceholder.compiledArtifactTooLarge('app.js', 99, 5),
215+
couldNotParse: readPlaceholder.couldNotParse('x.pdf', 'application/pdf', 10),
216+
binaryFile: readPlaceholder.binaryFile('app.bin', 'application/octet-stream', 10),
217+
})
218+
219+
it.each(everyPlaceholder)(
220+
'reports the %s placeholder instead of grepping it',
221+
(_name, content) => {
222+
expect(() => grepPlaceholder(content)).toThrow(WorkspaceFileGrepError)
223+
expect(() => grepPlaceholder(content)).toThrow(content)
224+
}
225+
)
216226

217227
it('still greps ordinary single-line content', () => {
218228
expect(grepPlaceholder('x marks the spot')).toHaveLength(1)
219229
})
230+
231+
it('greps a real multi-line file that merely opens like a placeholder', () => {
232+
// The single-line guard is what keeps this file searchable rather than swallowed.
233+
const content = `${readPlaceholder.binaryFile('app.bin', 'text/plain', 10)}\nx marks the spot`
234+
const matches = grepReadResult(
235+
'files/notes.txt/content',
236+
{ content, totalLines: 2 },
237+
'x',
238+
'files/notes.txt/content'
239+
)
240+
expect(matches.length).toBeGreaterThan(0)
241+
})
220242
})

apps/sim/lib/copilot/vfs/read-placeholders.ts

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,6 @@
33
* predicates that classify them. Producers and matchers live in different modules;
44
* hand-written copies of the same prefix are how an oversized image once slipped past
55
* the read-size gate, which tested for a prefix no producer emitted.
6-
*
7-
* Keep free of heavy imports — the tool handlers pull it in without wanting the VFS.
86
*/
97

108
import { formatFileSize } from '@/lib/uploads/utils/file-utils'
@@ -39,16 +37,15 @@ export const readPlaceholder = {
3937
} as const
4038

4139
/**
42-
* Placeholders standing in for content that exists but exceeded a read cap; the read
43-
* handler turns these into a tool error.
40+
* Placeholders meaning "the file is there, but reading it was refused on size"; the
41+
* read handler turns these into a tool error rather than a one-line success.
4442
*
45-
* Every size refusal belongs here — a document that breaches its cap is the same
46-
* kind of answer as a file or an image that does, and reporting one of the three as
47-
* a successful read was an inconsistency, not a distinction.
43+
* File, image, document and compiled artifact all belong here — reporting one of
44+
* the four as a successful read was an inconsistency, not a distinction.
4845
*
49-
* Deliberately narrower than {@link isNonGreppablePlaceholder}: a parse failure or
50-
* a binary file is not a size problem, and `[Image unavailable:` covers undecodable
51-
* images as well as oversized ones, so neither belongs on the size path.
46+
* `[Image unavailable:` is excluded even though one of its reasons is a size, because
47+
* its other reasons are not: it also covers an undecodable or unsupported image, and
48+
* those are answers rather than refusals. Callers get it as content.
5249
*/
5350
const OVERSIZED_PREFIXES = [
5451
PREFIX.fileTooLarge,

0 commit comments

Comments
 (0)