Skip to content

Commit 2361d23

Browse files
icecrasher321claude
andcommitted
fix(media): bound the ffmpeg tool's child processes, inputs and scale targets
The copilot ffmpeg tool shells out to FFmpeg in the Sim app process — not in a sandbox — and nothing bounded the run. runCommand had no timer, no kill and no signal, so a transcode ran until it finished and survived the request that asked for it: a stopped copilot turn left the encode pinning both cores of a shared instance. Every operation now shares one 10-minute wall-clock deadline, and both the deadline and the caller's cancellation SIGKILL the child. The budget is per-operation rather than per-command because concat runs a full re-encode per input, so a per-command timeout would let N inputs multiply into N timeouts. Cancellation is wired to context.abortSignal, whose every abort reason is an explicit user stop — the copilot lifecycle tracks a passive client disconnect separately and does not abort on it — so an encode dies when the user asks and not before. (userStopSignal, which assertServerToolNotAborted reads, has no producer on this path.) ffprobe had the same shape in miniature: fluent-ffmpeg's static ffprobe hands back no process handle, so a timeout could only race the callback and leave a wedged prober alive, and concat probes once per input. It now runs through execFile, which takes timeout, killSignal and signal natively. Scale targets reached the filter graph unvalidated, where libavfilter sizes its per-frame buffers from them — scale=30000:30000 is ~2.7 GB a frame, allocated in a child that shares the instance's memory. Dimensions are now rejected outside 16-4096 with a message the model can act on, plus an area cap. concat's targets come from the source container rather than a caller assertion, so those are clamped instead. Two further holes: readOut buffered the output with no ceiling, and CRF-18 re-encodes routinely exceed their input, so the input budget did not bound it; and the handler accepted unlimited input files. Both are capped, and the byte check now runs against the recorded size before the download rather than after. FFMPEG_LIMITS is the single source for the four numbers the Go tool catalog mirrors into its schema, pinned by ffmpeg-schema-parity.test.ts so the model is never told a ceiling the executor does not enforce. Companion: simstudioai/copilot#PENDING Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 7ea8692 commit 2361d23

8 files changed

Lines changed: 760 additions & 604 deletions

File tree

apps/sim/lib/copilot/generated/tool-catalog-v1.ts

Lines changed: 35 additions & 234 deletions
Large diffs are not rendered by default.

apps/sim/lib/copilot/generated/tool-schemas-v1.ts

Lines changed: 29 additions & 258 deletions
Large diffs are not rendered by default.

apps/sim/lib/copilot/tools/server/media/ffmpeg.test.ts

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -309,3 +309,91 @@ describe('ffmpeg server tool secret provenance', () => {
309309
})
310310
})
311311
})
312+
313+
describe('ffmpeg server tool input admission', () => {
314+
const context = {
315+
userId: 'user-1',
316+
workspaceId: 'workspace-1',
317+
toolCallId: 'tool-1',
318+
copilotToolExecution: true as const,
319+
resolvedSecretTraceRegistry: new ResolvedSecretTraceRegistry([], {
320+
userId: 'user-1',
321+
workspaceId: 'workspace-1',
322+
}),
323+
}
324+
325+
beforeEach(() => {
326+
vi.clearAllMocks()
327+
resolveWorkspaceFileReferenceMock.mockResolvedValue(file)
328+
fetchWorkspaceFileBufferMock.mockResolvedValue(Buffer.from('media'))
329+
getBoundWorkspaceFileSecretProvenanceMock.mockResolvedValue(EXACT_EMPTY)
330+
mergeWorkspaceFileSecretProvenanceMock.mockImplementation(mergeProvenance)
331+
runFfmpegOperationMock.mockResolvedValue({
332+
buffer: Buffer.from('output'),
333+
ext: 'mp4',
334+
contentType: 'video/mp4',
335+
})
336+
writeWorkspaceFileByPathMock.mockResolvedValue({
337+
id: 'output-1',
338+
name: 'converted.mp4',
339+
vfsPath: 'files/converted.mp4',
340+
downloadUrl: '/api/files/serve/converted.mp4',
341+
mode: 'create',
342+
})
343+
})
344+
345+
it('refuses more inputs than one call may transcode, before reading any of them', async () => {
346+
const files = Array.from({ length: 21 }, () => ({ path: 'files/input.mp4' }))
347+
348+
const result = await ffmpegServerTool.execute(
349+
{ operation: 'concat', inputs: { files } },
350+
context
351+
)
352+
353+
expect(result.success).toBe(false)
354+
expect(result.message).toContain('at most 20')
355+
expect(resolveWorkspaceFileReferenceMock).not.toHaveBeenCalled()
356+
expect(runFfmpegOperationMock).not.toHaveBeenCalled()
357+
})
358+
359+
it('admits a batch at the input ceiling', async () => {
360+
const files = Array.from({ length: 20 }, () => ({ path: 'files/input.mp4' }))
361+
362+
const result = await ffmpegServerTool.execute(
363+
{ operation: 'concat', inputs: { files } },
364+
context
365+
)
366+
367+
expect(result.success).toBe(true)
368+
expect(runFfmpegOperationMock).toHaveBeenCalledTimes(1)
369+
})
370+
371+
it('rejects on the recorded size before spending the download', async () => {
372+
resolveWorkspaceFileReferenceMock.mockResolvedValue({ ...file, size: 300 * 1024 * 1024 })
373+
374+
const result = await ffmpegServerTool.execute(
375+
{ operation: 'convert', inputs: { files: [{ path: 'files/huge.mp4' }] } },
376+
context
377+
)
378+
379+
expect(result.success).toBe(false)
380+
expect(result.message).toContain('byte limit')
381+
expect(fetchWorkspaceFileBufferMock).not.toHaveBeenCalled()
382+
})
383+
384+
it('hands the caller cancellation signal to the transcode', async () => {
385+
const controller = new AbortController()
386+
387+
await ffmpegServerTool.execute(
388+
{ operation: 'convert', inputs: { files: [{ path: 'files/input.mp4' }] } },
389+
{ ...context, abortSignal: controller.signal }
390+
)
391+
392+
expect(runFfmpegOperationMock).toHaveBeenCalledWith(
393+
'convert',
394+
expect.anything(),
395+
expect.anything(),
396+
{ signal: controller.signal }
397+
)
398+
})
399+
})

apps/sim/lib/copilot/tools/server/media/ffmpeg.ts

Lines changed: 40 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
import { writeCopilotWorkspaceFileByPath } from '@/lib/copilot/vfs/resource-writer'
1414
import { MAX_MEDIA_BYTES } from '@/lib/media/falai'
1515
import { type FfmpegOperation, type MediaFile, runFfmpegOperation } from '@/lib/media/ffmpeg'
16+
import { FFMPEG_LIMITS } from '@/lib/media/ffmpeg-limits'
1617
import {
1718
createWorkspaceFileSecretProvenanceFromRegistry,
1819
getBoundWorkspaceFileSecretProvenance,
@@ -26,6 +27,13 @@ import { projectResolvedSecretModelContent } from '@/executor/utils/resolved-sec
2627
const logger = createLogger('FfmpegTool')
2728
const MEDIA_OPERATION_FAILED_SAFELY = 'The media operation failed safely'
2829

30+
/**
31+
* Backstops the `maxItems` the generated tool schema declares: the byte budget
32+
* below does not bound a call that lists many small clips, and a caller that
33+
* reaches this handler without passing Ajv still must not get an unbounded run.
34+
*/
35+
const { maxInputFiles: MAX_INPUT_FILES } = FFMPEG_LIMITS
36+
2937
const VALID_OPERATIONS: FfmpegOperation[] = [
3038
'overlay_audio',
3139
'mux',
@@ -93,6 +101,12 @@ export const ffmpegServerTool: BaseServerTool<FfmpegArgs, FfmpegResult> = {
93101
if (inputPaths.length === 0) {
94102
return { success: false, message: 'At least one input file is required in inputs.files' }
95103
}
104+
if (inputPaths.length > MAX_INPUT_FILES) {
105+
return {
106+
success: false,
107+
message: `${inputPaths.length} input files were requested; at most ${MAX_INPUT_FILES} are allowed per ffmpeg call. Combine them in batches.`,
108+
}
109+
}
96110

97111
let inputRequiresOpaqueError = false
98112
try {
@@ -108,6 +122,11 @@ export const ffmpegServerTool: BaseServerTool<FfmpegArgs, FfmpegResult> = {
108122
reference: filePath,
109123
}
110124
)
125+
// Reject on the recorded size before spending the download. The
126+
// accumulated check below still backstops a stale size row.
127+
if (totalInputBytes + fileRecord.size > MAX_MEDIA_BYTES) {
128+
throw new Error(`Input files exceed the ${MAX_MEDIA_BYTES} byte limit`)
129+
}
111130
const fileProvenance = await getBoundWorkspaceFileSecretProvenance(workspaceId, {
112131
fileId: fileRecord.id,
113132
key: fileRecord.key,
@@ -141,19 +160,27 @@ export const ffmpegServerTool: BaseServerTool<FfmpegArgs, FfmpegResult> = {
141160
inputRequiresOpaqueError ||=
142161
inputProvenance.status !== 'exact' || inputProvenance.entries.length > 0
143162
assertServerToolNotAborted(context)
144-
const result = await runFfmpegOperation(params.operation, mediaFiles, {
145-
text: params.text,
146-
position: params.position,
147-
start: params.start,
148-
end: params.end,
149-
width: params.width,
150-
height: params.height,
151-
aspectRatio: params.aspectRatio,
152-
volume: params.volume,
153-
musicVolume: params.musicVolume,
154-
loopToVideo: params.loopToVideo,
155-
format: params.format,
156-
})
163+
const result = await runFfmpegOperation(
164+
params.operation,
165+
mediaFiles,
166+
{
167+
text: params.text,
168+
position: params.position,
169+
start: params.start,
170+
end: params.end,
171+
width: params.width,
172+
height: params.height,
173+
aspectRatio: params.aspectRatio,
174+
volume: params.volume,
175+
musicVolume: params.musicVolume,
176+
loopToVideo: params.loopToVideo,
177+
format: params.format,
178+
},
179+
// Every abort of this signal is an explicit user stop — the copilot
180+
// lifecycle tracks a passive client disconnect separately and does not
181+
// abort on it — so a transcode dies when the user says stop, and only then.
182+
{ signal: context.abortSignal }
183+
)
157184

158185
// probe reports metadata only — no file written.
159186
if (params.operation === 'probe') {
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
/**
2+
* Execution bounds the ffmpeg tool enforces.
3+
*
4+
* These are mirrored into the Go tool catalog
5+
* (`copilot/internal/tools/catalog/other/ffmpeg.go`) so the model reads the
6+
* limits off its own schema instead of discovering them as a failed tool call.
7+
* `ffmpeg-schema-parity.test.ts` fails when the two copies drift.
8+
*
9+
* `maxScalePixels` has no JSON Schema equivalent, so it lives in the parameter
10+
* description on the Go side and is enforced here only.
11+
*/
12+
export const FFMPEG_LIMITS = {
13+
/** Every input costs a full re-encode pass in `concat`, the only multi-input operation. */
14+
maxInputFiles: 20,
15+
minScaleDimension: 16,
16+
maxScaleDimension: 4096,
17+
/** DCI 4K in either orientation — bounds the square frames the per-axis cap alone allows. */
18+
maxScalePixels: 4096 * 2304,
19+
} as const
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { describe, expect, it } from 'vitest'
5+
import { TOOL_RUNTIME_SCHEMAS } from '@/lib/copilot/generated/tool-schemas-v1'
6+
import { FFMPEG_LIMITS } from '@/lib/media/ffmpeg-limits'
7+
8+
/**
9+
* The ffmpeg bounds live twice: here, where the executor enforces them, and in
10+
* the Go tool catalog, where they become the JSON Schema the model reads and
11+
* Ajv checks at the router. Drift between the two is silent and user-visible —
12+
* the model is told one ceiling and the transcode refuses at another — so pin
13+
* the generated schema against the executor's own numbers.
14+
*
15+
* When this fails, change `ffmpeg.go` in the copilot repo and regenerate; do
16+
* not edit the generated schema.
17+
*/
18+
interface SchemaNode {
19+
properties?: Record<string, SchemaNode>
20+
items?: SchemaNode
21+
maxItems?: number
22+
minimum?: number
23+
maximum?: number
24+
}
25+
26+
const ffmpegParameters = TOOL_RUNTIME_SCHEMAS.ffmpeg?.parameters as SchemaNode | undefined
27+
28+
describe('ffmpeg tool schema parity', () => {
29+
it('declares the tool in the generated catalog', () => {
30+
expect(ffmpegParameters?.properties).toBeDefined()
31+
})
32+
33+
it('caps inputs.files at the executor limit', () => {
34+
expect(ffmpegParameters?.properties?.inputs?.properties?.files?.maxItems).toBe(
35+
FFMPEG_LIMITS.maxInputFiles
36+
)
37+
})
38+
39+
it('bounds the scale dimensions at the executor limits', () => {
40+
for (const axis of ['width', 'height'] as const) {
41+
expect(ffmpegParameters?.properties?.[axis]?.minimum).toBe(FFMPEG_LIMITS.minScaleDimension)
42+
expect(ffmpegParameters?.properties?.[axis]?.maximum).toBe(FFMPEG_LIMITS.maxScaleDimension)
43+
}
44+
})
45+
46+
it('does not offer sandbox-only fields on a tool that runs in this process', () => {
47+
const inputs = ffmpegParameters?.properties?.inputs?.properties
48+
expect(Object.keys(inputs ?? {})).toEqual(['files'])
49+
expect(inputs?.files?.items?.properties).toBeDefined()
50+
expect(Object.keys(inputs?.files?.items?.properties ?? {})).toEqual(['path'])
51+
52+
const outputItem = ffmpegParameters?.properties?.outputs?.properties?.files?.items?.properties
53+
expect(Object.keys(outputItem ?? {}).sort()).toEqual(['mimeType', 'mode', 'path'])
54+
})
55+
})

0 commit comments

Comments
 (0)