fix(media): bound the copilot ffmpeg tool's inputs, runtime, and output paths by waleedlatif1 · Pull Request #6544 · simstudioai/sim

waleedlatif1 · 2026-08-11T17:40:14Z

Summary

Bound the copilot ffmpeg tool, which runs FFmpeg in-process on the app server. Caps inputs at 10 per operation, rejected before any file is downloaded
Give each operation one wall-clock budget (5 min) shared across every process it spawns, and SIGKILL on expiry — concat runs an encode per clip, so a per-command timeout would still multiply out
Wire the tool's cancellation signal through to that kill, so a stopped turn stops the transcode instead of leaving it running
Validate scale_pad dimensions (16–4096) before they reach the filter graph, and clamp the dimensions concat derives from the first clip's container metadata
Restrict convert/extract_audio output formats to an explicit allowlist. The format previously reached a temp-file name unsanitized, so it could resolve outside the working directory
Resolve every temp path through a helper that verifies containment, making that invariant structural rather than dependent on sanitizing each filename source
Spawn ffprobe directly instead of via fluent-ffmpeg, which exposes no handle on the child and so could not be killed at all

Type of Change

Bug fix

Testing

Unit tests for every bound (input cap, format allowlist, scale bounds, trim ordering, abort). Hermetic — verified passing with no ffmpeg on PATH, as in CI
Verified out-of-band against real ffmpeg that probe metadata, convert, scale_pad, budget expiry, and external abort all behave correctly, since the change touches every execution path
Confirmed each new guard fails its test when reverted
bun run type-check, bun run lint, and bun run check:api-validation all pass; 328 copilot server-tool tests pass

Notes

lib/audio/extractor.ts has the same unbounded-runtime shape and is reachable from the STT route. Left out of scope, worth a follow-up
The generated tool schema still advertises these params as unbounded; the limits originate in the sibling copilot repo's catalog, so surfacing them there would stop the model retrying rejected calls
There is no rate limit or concurrency cap on copilot tool calls, so per-call bounds do not bound call volume

Checklist

Code follows project style guidelines
Self-reviewed my changes
Tests added/updated and passing
No new warnings introduced
I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

vercel · 2026-08-11T17:40:23Z

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
docs	Skipped		Aug 11, 2026 6:06pm

cursor · 2026-08-11T17:40:26Z

PR Summary

^{Cursor Bugbot is generating a summary for commit 41a7329. Configure here.}

greptile-apps · 2026-08-11T17:45:39Z

Greptile Summary

The PR hardens the server-side FFmpeg tool with bounded inputs, dimensions, runtime, output formats, and temp paths while propagating cancellation to child processes. It also replaces fluent-ffmpeg probing with directly controlled ffprobe execution and completes the previously requested standalone-resolution and lookup-precedence fixes.

Separates optional FFmpeg discovery from the requirement imposed by transcoding.
Resolves ffprobe through configured path, PATH, then the FFmpeg sibling.
Adds shared operation deadlines, abort handling, and forced child-process termination.
Validates output extensions, media parameters, and temp-directory containment.
Adds focused tests for bounds, cancellation, standalone probing, and resolver precedence.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
apps/sim/lib/media/ffmpeg.ts	Adds bounded execution and direct ffprobe management; the two previously reported resolver defects are fixed at current HEAD.
apps/sim/lib/copilot/tools/server/media/ffmpeg.ts	Rejects excessive inputs before download and forwards combined cancellation to media execution.
apps/sim/lib/copilot/tools/server/base-tool.ts	Adds a helper combining the two server-tool cancellation sources.
apps/sim/lib/media/ffmpeg-probe-resolution.test.ts	Verifies repeated standalone probing remains independent of FFmpeg availability.
apps/sim/lib/media/ffmpeg-probe-precedence.test.ts	Pins PATH precedence over an existing FFmpeg-directory sibling.
apps/sim/lib/media/ffmpeg.test.ts	Covers input, output-format, dimension, parameter, and pre-abort bounds.
apps/sim/lib/copilot/tools/server/media/ffmpeg.test.ts	Covers pre-download input rejection and cancellation propagation at the tool boundary.

_{Reviews (4): Last reviewed commit: "fix(media): floor the probe timeout and ..." | Re-trigger Greptile}

…ut paths FFmpeg runs in-process on the request-serving app server, so every attacker-influenced dimension of a tool call is an instance-wide resource concern rather than a single failed request. - Cap input count at 10 in both the tool handler (before any download) and runFfmpegOperation. The existing MAX_MEDIA_BYTES budget bounded RAM but still permitted hundreds of small clips, and concat re-encodes each one serially with libx264. - Give the whole operation a single 5-minute wall-clock budget shared across every spawned process, and SIGKILL on expiry. A per-command timeout would still multiply out across concat's per-clip encodes. - Wire abortSignal and userStopSignal through to that kill, so a cancelled copilot turn stops the transcode instead of leaving it running. - Validate scale_pad width/height as integers in 16..4096 before they reach the filter graph, and clamp the probed dimensions concat derives from the first clip's container metadata. - Restrict the convert/extract_audio `format` to known muxers with safe file names. It was interpolated into path.join(dir, `out.${ext}`) unsanitized, so a format of "../../x.mp4" escaped the temp dir and wrote there. - Spawn ffprobe directly rather than through fluent-ffmpeg, which exposes no handle on the child and so cannot be killed. Validation runs before any temp dir or binary resolution, so a rejected request costs nothing and reports the real reason.

Follow-up to the input/runtime/output-path bounds, from a multi-agent review of that change. Regressions the first pass introduced: - webp and weba were in MIME_TO_EXT but not EXT_TO_MIME, so convert to either hard-failed where it previously worked. Both are now valid outputs. - Bounds were asserted for every operation, so a surplus out-of-range value an operation never reads (overlay_audio + volume) failed the whole call. Each operation now validates only what it consumes. - width/height of 0 bypassed the scale check via a truthy guard. - clampProbedDimension raised a probed 0 to 16 instead of falling back to the default, yielding a 16x16 concat. Bugs found in the new code: - fluent-ffmpeg's kill() is a no-op until the child spawns, and .save() spawns asynchronously. A kill landing in that window rejected the promise while the encode spawned orphaned and unkillable. Re-issue the kill on 'start'. - ffprobe ran with -v quiet, which left a timeout, a corrupt file, and a missing file byte-identical and uninformative. Use -v error and report the distinct cause, with the server's paths stripped from the diagnostic. - resolveFfprobePath narrowed fluent-ffmpeg's lookup; restore FFPROBE_PATH and PATH fallback with existence checks. Also: reject a trim whose end precedes its start rather than silently writing an empty file, restrict extract_audio to audio containers, name the actionable cause in the timeout message, and drop a redundant second probe budget. Tests: assert no temp dir is created on an already-aborted signal (the previous assertion passed with the guard reverted), cover the 0-dimension and end-before-start cases, and track MAX_FFMPEG_INPUTS instead of a literal. Reviewers also flagged that LLM-supplied numerics arrive as strings; verified against the router that Ajv rejects those upstream, so no coercion was added.

… home Quality pass over the ffmpeg hardening, from a four-angle review. Reuse: - Replace the hand-rolled OperationBudget with createTimeoutAbortController from @/lib/core/execution-limits, which already models "one deadline plus a parent signal, and tell me which fired". This also removes the per-command setTimeout: the controller's single deadline covers the whole operation, so probeFile now takes its cap from getRemainingExecutionMs. - Combine the tool's cancellation signals with combineExecutionAbortSignals, and move that helper to base-tool.ts next to assertServerToolNotAborted, where the shared "how does a tool consume cancellation" concern lives. Altitude: - Delete assertOptionsWithinBounds. Every rule in it also lived in the operation that consumes it, and the two copies had already diverged: extract_audio's allowlist existed only in the preflight, and the end >= start check only there. Each rule now has exactly one home, at its point of use. - Stop resolving the ffmpeg binary in withTempDir, so a validation failure no longer surfaces as "FFmpeg not found" on a host without it. runCommand and probeFile resolve it, being the only things that need it. - Add tempPath(), which resolves a name inside the temp dir and refuses anything that escapes. All 14 path sites go through it, so the containment invariant is structural rather than dependent on remembering to sanitize every filename source. - Declare OUTPUT_EXTS explicitly instead of deriving the allowlist from EXT_TO_MIME, so widening a content-type map cannot widen what may be written. Also: a supplied width/height of 0 now reaches the bounds check rather than being treated as absent, memoize the resolved ffprobe path, and settle() on a synchronous throw from .save() so no listener outlives the command. Tests: replace two assertions that could not fail (`rejects.not.toThrow` passes on any rejection, including "FFmpeg not found") with deterministic ones, which also removes every real ffmpeg spawn from the suite — 293ms of test time to 21ms. Verified hermetic with ffmpeg off PATH, and verified against real ffmpeg out-of-band that probe, convert, scale_pad, budget expiry, and external abort all still behave.

Two narrated the adjacent literal or ternary. The third was orphaned by the deleted validation chain, so TSDoc bound it to ASPECT_TARGETS and documented the wrong declaration; its rationale is in the commit that removed the chain.

ensureFfmpeg() conflated 'resolve the binary' with 'require the binary', and resolveFfprobePath called it first — so on a host with FFPROBE_PATH set but no ffmpeg, the first probe succeeded and every later one threw, because the failed lookup is memoized. Split the non-throwing init from the ffmpeg-required assertion; transcoding still demands ffmpeg, probing no longer does. Covered by a test in its own file, since the binary lookup memoizes at module scope and a shared file would already have consumed that state.

waleedlatif1 · 2026-08-11T17:49:27Z

@greptile

waleedlatif1 · 2026-08-11T17:49:31Z

@cursor review

FFmpeg's muxer is named webm and refuses a .weba output ('Error initializing the muxer'), so allowlisting the extension only converted a clear 'unsupported format' rejection into a confusing encode-time failure. weba was added in this PR because it appears in the input MIME map, but naming an input file and naming an output muxer are different questions. extract_audio takes webm.

waleedlatif1 · 2026-08-11T17:55:09Z

@greptile

waleedlatif1 · 2026-08-11T17:55:14Z

@cursor review

The TSDoc claimed fluent-ffmpeg's order (FFPROBE_PATH, then PATH, then ffmpeg's directory) while the code checked the sibling second, so a stray or unusable file next to the ffmpeg binary would be cached and mask a working PATH install for every probe. Match the documented order. Pinned by a test in its own file: the resolved path memoizes at module scope, so precedence is only observable in a module no other test has resolved in. It mocks existsSync as well as execSync, without which the sibling never exists on the test host and the two orderings are indistinguishable.

Two findings from review: - Node reads execFile's `timeout: 0` as 'no timeout', so once the shared budget was spent the 15s probe cap disappeared entirely — the opposite of what an exhausted budget should do. Reachable between the deadline passing and the abort timer firing, where assertOperationLive still sees a live signal. Floor the computed cap at 1ms. - extract_audio accepts webm, but mimeFromExt resolves that container to video/webm, so an audio-only extract was stored with a video content type. Resolve audio-only outputs through a small override map.

waleedlatif1 · 2026-08-11T18:07:28Z

@greptile

waleedlatif1 · 2026-08-11T18:07:32Z

@cursor review

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit dc6cc8b. Configure here.}

waleedlatif1 · 2026-08-11T18:14:25Z

Note on the red Lint and Test check — both failures are pre-existing on staging, not from this PR:

bun audit fails on transitive advisories (DOMPurify, undici, fast-uri). This PR adds no dependencies. PR feat(calendly): extend tools with booking, availability, no-shows, and routing forms #6545, unrelated, fails on the identical step.
search-modal.test.tsx — two assertions expecting 'New Chat: …' against actual 'New chat: …'. I verified this by checking out clean origin/staging with none of my commits and reproducing both failures there. This branch does not touch that file.

Everything this PR owns is green: 334 tests across lib/media/ and lib/copilot/tools/server/ pass, and I ran the suite six times to confirm no flakiness after removing a racy test of my own.

The chat-title casing failure looks like it wants a real fix on staging — the component renders sentence case while the test expects title case. Flagging rather than fixing it here, since it is outside this PR's scope.

greptile-apps Bot reviewed Aug 11, 2026

View reviewed changes