Skip to content

Commit 9791fd8

Browse files
committed
refactor(media): reuse shared execution limits and give each rule one 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.
1 parent 260abe8 commit 9791fd8

4 files changed

Lines changed: 249 additions & 242 deletions

File tree

apps/sim/lib/copilot/tools/server/base-tool.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import type { z } from 'zod'
22
import type { BillingAttributionSnapshot } from '@/lib/billing/core/billing-attribution'
3+
import { combineExecutionAbortSignals } from '@/lib/core/execution-limits'
34
import type { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry'
45

56
export interface ServerToolContext {
@@ -28,6 +29,18 @@ export interface ServerToolContext {
2829
resolvedSecretTraceRegistry?: ResolvedSecretTraceRegistry
2930
}
3031

32+
/**
33+
* One signal covering every way a tool call can be cancelled, for tools that
34+
* hold a killable resource (a child process, a long stream) rather than merely
35+
* checking between steps as {@link assertServerToolNotAborted} does.
36+
*/
37+
export function resolveServerToolAbortSignal(context?: ServerToolContext): AbortSignal | undefined {
38+
const signals = [context?.abortSignal, context?.userStopSignal].filter(
39+
(signal): signal is AbortSignal => Boolean(signal)
40+
)
41+
return signals.length > 0 ? combineExecutionAbortSignals(signals) : undefined
42+
}
43+
3144
export function assertServerToolNotAborted(
3245
context?: ServerToolContext,
3346
message = 'Request aborted before tool mutation could be applied.'

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

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import { Ffmpeg } from '@/lib/copilot/generated/tool-catalog-v1'
88
import {
99
assertServerToolNotAborted,
1010
type BaseServerTool,
11+
resolveServerToolAbortSignal,
1112
type ServerToolContext,
1213
} from '@/lib/copilot/tools/server/base-tool'
1314
import { writeCopilotWorkspaceFileByPath } from '@/lib/copilot/vfs/resource-writer'
@@ -76,19 +77,6 @@ interface FfmpegResult {
7677
probe?: unknown
7778
}
7879

79-
/**
80-
* A transcode outlives its request unless the child process is killed, so both
81-
* the transport abort and the explicit user stop must reach FFmpeg — checking
82-
* them only between steps leaves a cancelled turn burning cores.
83-
*/
84-
function resolveFfmpegAbortSignal(context: ServerToolContext): AbortSignal | undefined {
85-
const signals = [context.abortSignal, context.userStopSignal].filter(
86-
(signal): signal is AbortSignal => Boolean(signal)
87-
)
88-
if (signals.length === 0) return undefined
89-
return signals.length === 1 ? signals[0] : AbortSignal.any(signals)
90-
}
91-
9280
export const ffmpegServerTool: BaseServerTool<FfmpegArgs, FfmpegResult> = {
9381
name: Ffmpeg.id,
9482

@@ -180,7 +168,9 @@ export const ffmpegServerTool: BaseServerTool<FfmpegArgs, FfmpegResult> = {
180168
loopToVideo: params.loopToVideo,
181169
format: params.format,
182170
},
183-
{ signal: resolveFfmpegAbortSignal(context) }
171+
// A transcode outlives its request unless the child is killed, so the
172+
// cancellation signal must reach FFmpeg itself, not just the steps around it.
173+
{ signal: resolveServerToolAbortSignal(context) }
184174
)
185175

186176
// probe reports metadata only — no file written.

apps/sim/lib/media/ffmpeg.test.ts

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -49,14 +49,18 @@ describe('runFfmpegOperation output format validation', () => {
4949
).rejects.toThrow('Unsupported output format')
5050
})
5151

52-
it.each(['webp', 'weba', 'mp4', 'gif'])(
53-
'still accepts %s, which the input MIME map already supported',
54-
async (format) => {
55-
await expect(runFfmpegOperation('convert', [mediaFile()], { format })).rejects.not.toThrow(
56-
'Unsupported output format'
57-
)
52+
it('keeps the formats the input MIME map already supported', async () => {
53+
// Asserted through the rejection's own "Supported:" list rather than by
54+
// converting for real: a `not.toThrow` on a live transcode passes on any
55+
// rejection, including "FFmpeg not found".
56+
const error = await runFfmpegOperation('convert', [mediaFile()], { format: 'exe' }).catch(
57+
(e: Error) => e
58+
)
59+
60+
for (const format of ['mp4', 'mov', 'webm', 'mp3', 'wav', 'gif', 'webp', 'weba']) {
61+
expect(error.message).toContain(format)
5862
}
59-
)
63+
})
6064
})
6165

6266
describe('runFfmpegOperation scale bounds', () => {
@@ -75,12 +79,15 @@ describe('runFfmpegOperation scale bounds', () => {
7579
})
7680

7781
describe('runFfmpegOperation per-operation validation', () => {
78-
it('ignores options the operation never consumes', async () => {
79-
// overlay_audio does not read `volume`; an out-of-range surplus value from
80-
// the model must not fail the whole call.
82+
// Each rule is asserted at the operation that owns it. The complementary
83+
// property — that an operation ignores options it never reads — cannot be
84+
// asserted without running a real transcode, so it is left to review.
85+
it('rejects an out-of-range volume on mix_audio, which consumes it', async () => {
8186
await expect(
82-
runFfmpegOperation('overlay_audio', [mediaFile(), mediaFile('audio/mpeg')], { volume: 15 })
83-
).rejects.not.toThrow(/volume/)
87+
runFfmpegOperation('mix_audio', [mediaFile('audio/mpeg'), mediaFile('audio/mpeg')], {
88+
volume: 15,
89+
})
90+
).rejects.toThrow('volume must be a number between 0 and 10')
8491
})
8592

8693
it('rejects a trim whose end precedes its start', async () => {

0 commit comments

Comments
 (0)