Review follow-up on the STUDIO-5433 defense.
Correctness
- The sniff ran above the documented video/audio failure split, so an
<audio> src that resolved to an HTML payload aborted the whole render
instead of degrading to duration 0. It now runs inside the same try, so
video surfaces the typed error while audio still drops out, with a
warning naming the element.
- Raw fs errors (EISDIR on a directory src, EACCES, the existsSync->open
ENOENT race, EMFILE) escaped and failed the compile with an unclassified
error carrying an unredacted temp path. The sniff is now a classifier that
never throws: an unreadable file reports "not markup" and the real probe
produces the real error.
- Elements whose duration the compiler never resolves (a data-end video, a
looping audio) skipped the sniff entirely, so the original ffprobe error
still escaped, and looping audio was reported as owner "system" after
every frame had been captured. Video is now caught in the asset preflight,
which sees every local src regardless of authored timing; audio is
classified per-element in audioMixer as source/invalid_media/owner "user",
keeping audio failures non-fatal as they already were.
- Detection is a byte-level check for a leading "<" (BOM-, whitespace- and
NUL-tolerant, looped read) instead of a <!doctype|<html|<?xml string
prefix, which missed a NUL-prefixed payload, >256B of leading whitespace,
UTF-16-encoded HTML, and a prolog-less <svg. No supported container starts
with "<", so the allowlist no longer grows per payload shape.
- finally { await fh.close() } could replace the in-flight typed error with
the close error.
Routing and privacy
- MARKUP_NOT_MEDIA is now in SAFE_RENDER_ERROR_CODES, the Lambda terminal
name map, the CDK and SAM non-retryable plan lists, and the Cloud Run
non-retryable set, and the class carries owner/retryable. Previously the
API emitted errorCode: undefined and a deterministic authoring bug burned
the full distributed retry budget.
- The message no longer carries 32 raw payload bytes or the src.
redactTelemetryString preserves host and path for HTTP srcs, so
per-tenant CDN paths reached a message the server forwards to clients.
Correlation is a sha256 element fingerprint, matching
AssetMediaTypeMismatchError.
- The message names both causes (unresolved nested-composition URL, or an
HTML/XML error page served as 200) rather than misdiagnosing an S3 403
body as an authoring bug.
Tests
- Byte-level detection is unit-tested in engine: markup shapes, BOMs,
UTF-16, nine container signatures, unreadable inputs.
- Replaced the tautological assertions. The old checks for "html" in and
"moov" absent from a fixed message template could not fail for any input.
- New coverage for audio degradation, the audioMixer classification, the
preflight video/image/audio split, and the API error metadata.
- The sibling htmlCompiler.mediaType failure was a vitest-under-bun runner
mismatch, not a missing ffmpeg binary. It passes, including the 4-wide
probe-semaphore invariant the sniff now runs inside.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
STUDIO-5433 defense: when the downloaded media file begins with
<!DOCTYPE, <html, or <?xml, throw a typed HtmlNotVideoError naming
the offending src instead of letting ffprobe emit an inscrutable
moov-atom-not-found on a plain HTML page.
Complements #3033 diagnosability layer. Root-cause EF fix ships
separately.
Signed-off-by: Via <vance@heygen.com>
A probe that could not run is no evidence about the GPU, so pointing the
operator at GPU passthrough hid broken Chrome installs behind a phantom
problem. Carry a cause off the probe and emit the matching remediation.
Also un-exports buildUnverifiedHardwareGpuWarning (Fallow: engine test
files are not audit entry points, so a test-only import would not have
counted as a consumer) and covers the non-linux branch via the spy.
Chrome's hardware GL args are advisory: with no usable GPU it silently
falls back to software WebGL and the capture runs at CPU speed. Run the
existing WebGL probe for explicit hardware mode too and warn loudly with
the platform's remediation. The requested mode is still honoured.
Fixes#2967
Three R3 findings.
The redactor's segment classes were ASCII `\w`, so `/数据/客户/秘密视频.mp4` and
`/data/客户/secret.mp4` went out verbatim — and the generic redactor also feeds
CLI telemetry and producer observation messages, where no known-path list
compensates. Segments are now defined by their delimiters instead of an
alphabet, which is correct for every script by construction rather than
requiring Unicode classes to be kept correct. The bare-relative lookbehind had
the same ASCII assumption and let a match start mid-token, redacting
`客户/秘密/视频.mp4` to `客户[path]`; it is now a token boundary, and
bare-relative runs before absolute so it claims the whole token.
sanitizeProbeFailure cast the rejection reason to Error and read `.message`.
An injected probe can reject with anything, so `Promise.reject("failed")` gave
`undefined` and threw inside the redactor — converting a returned failure
result into a rejected promise. Normalized at the boundary, and
redactKnownPaths no longer throws on a non-string.
The contract only admitted .ts/.js/.mjs/.cjs, so it missed shipped shell and
Python callers. frame_strip.sh passed a user-controlled path as ffprobe's last
positional with no terminator; render-and-composite.sh had four more. Both
fixed, and the sweep now covers .py/.sh. Python list argvs are bracket
literals so they get the same position check; shell command lines get a
separate presence check, because checking position there needs a shell parser
— stated as the weaker guarantee it is rather than implied to be equal.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CodeQL flagged js/redos on the all-literal argv matcher. It was right: the
`(?:"[^"]*"\s*,?\s*)+` form nests a quantifier inside a quantifier with an
optional separator, so whitespace can be matched two ways and a long
non-matching argv backtracks exponentially.
Replaced with a linear scan — find the spawn head, slice to the closing
bracket, and check the entries — plus small named helpers. Same behaviour: an
all-literal argv is treated as taking no input, an argv with a bare identifier
still has to be understood (verified by adding one and watching the guard fail).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI runs the PR merged with main, so it saw a caller my branch predated:
`spawnSync("ffprobe", ["-version"])` in engine/src/utils/ffprobe.test.ts. That
is a capability check with no runtime path, so there is nothing to terminate,
but the unclassified guard flagged it as a caller it could not parse.
An argv whose entries are all string literals carries no input by
construction. Those are dropped before the check; an argv with a bare
identifier still has to be understood, verified by adding one and watching the
guard fail.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The contract test only walked packages/*/src and only .ts, so it could not see
the shipped agent tools under skills/**, which are .mjs/.cjs. 19 call sites
there and in package tests were still missing `--` immediately before the
input while the suite reported the bug class closed — a dash-prefixed filename
is parsed as an option and fails the same way.
Sweeps packages/, skills/ and scripts/ now, including .mjs/.cjs and test
files (dither.test.mjs was one of the broken sites). Excludes only the
contract test itself, which documents the contract with example argvs
including a deliberately misordered one.
Two guards were fixed while widening: the terminator must never be inserted
after `-i`, which consumes the next token (a blind pass hit an ffmpeg input
and a base64 -i), and comment prose describing a spawn is not a spawn.
Also routes every audioPadTrim probe failure through one sanitizer at the
boundary. runFfprobeJson scrubbed its own stderr, but
defaultProbeVideoFrameInfo threw `no video stream in ${videoPath}` raw into
the public PadTrimAudioResult.error, and an injected probe can throw anything.
The redaction unit tests all passed with the caller wiring deleted; the new
public-path regressions fail without it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The generic scrub still missed a relative path with no `./` prefix:
`customer/acme-secret/video.mp4` and `assets/bgm.mp3` reached telemetry
completely unredacted, because the absolute rule needs a leading slash and
the `./` rule needs the dot. Adds a rule for them that still leaves `N/A`,
`24/1` and `48000/1001` alone.
Shape matching is a net with holes by construction, so audioPadTrim now
also redacts the exact path it put in the argv, plus its basename, before
the generic scrub runs. It built the argv, so it does not have to guess.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
redactTelemetryString enumerated roots — /Users, /home, /opt, /tmp and a
handful more — so a project on /data, /Volumes, an NFS mount or any root a
user invented reached telemetry verbatim. Relative paths and bare basenames
were never redacted at all, and audioPadTrim routes raw ffprobe stderr
through this on every probe failure.
Now redacts by shape: absolute paths under any root (two or more segments,
so N/A and a 24/1 frame rate are not mistaken for one), relative paths
including dash-prefixed ones, and bare basenames with an asset extension.
URLs still keep their host and drop only the query.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The prior contract test scanned a hardcoded file list for a format flag
followed by a bare identifier, so it only matched the shape it was written
against. Mutation testing showed removing `--` from engine/utils/ffprobe.ts
and cli/commands/init.ts did not fail it.
Now walks packages/*/src and finds callers itself, checks that `--` is
immediately BEFORE the input rather than merely present, and compares
discovery against a manifest so a regex regression cannot silently stop
checking a known caller. A separate guard fails on any file that spawns a
probe binary but builds an argv this test cannot parse.
All 11 seams mutation-tested for both removal and misordering.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit claimed "all nine now terminate their options". That
was false: `producer/src/utils/audioRegression.ts:307` still passed the
path bare, and it is production source used by the regression harness.
A repo-wide audit found two more in studio-server
(`mediaValidation.ts`, `mediaMetadata.ts`) — their current callers pass
absolute paths, so they were defence-in-depth rather than live bugs, but
the exhaustiveness claim should be true rather than narrowed.
Eleven sites total, all terminated.
Adds a SOURCE-level contract test, which is the gap that let this
happen twice. #2740 fixed one of ten sites and shipped a regression
asserting the argv of that single site, so CI reported the class closed
while nine invocations still parsed `-intro.mp4` as an option. A
per-site unit test has the same blind spot for site twelve; scanning the
tree does not. The test also asserts its own coverage list has not
shrunk.
Verification: engine 1300, lint 511, core 1431, studio-server 398, cli
init/webmAlphaCheck/whisper 146, producer utils 51, audioPadTrim 18.
Removing any single terminator fails the contract test by name.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#2740 added `--` to one of nine independent ffprobe invocations, so the
bug class it closed stayed open everywhere else while CI reported it
fixed — the regression test asserts the argv of that single site.
Reproduced on ffprobe 8.1.1: an asset named `-intro.mp4` probes fine
through extractMediaMetadata but fails with "Missing argument for option
'intro.mp4'" in audio pad/trim (mid-render), `hyperframes init`, whisper
duration probing and webmAlphaCheck. hevcPreviewLint catches and returns
false, so a dash-prefixed HEVC preview silently passes the lint rule.
Terminated at all of them:
producer/services/render/audioPadTrim.ts (x2)
producer/plan-parity-analysis.ts
cli/commands/init.ts
cli/utils/webmAlphaCheck.ts
cli/whisper/transcribe.ts (x2)
core/mediaGradeAnalyzer.ts
lint/hevcPreviewLint.ts
audioPadTrim's runFfprobeJson is a near-verbatim clone of the engine's
runFfprobe and structurally cannot add the terminator itself, because
callers bake the input path into `args`. It now asserts the terminator
is present rather than letting a dash-prefixed path through, takes the
same stdio ["ignore", ...] as the engine helper, and redacts its stderr
— it was throwing raw ffprobe output, which echoes the input path, into
logs and telemetry.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Extends the studio-preview fix to the render path and the asset-discovery
utilities, which share the same resolver and had the same defect.
`rewriteAssetPath` takes an optional `assetExists` probe. A plain relative ref
authored in a sub-composition (`_shared.css`, `clip.mp4`) is re-pointed at the
composition's own directory when that sibling exists on disk; project-root refs
with no sibling (the registry's `assets/logo.png` convention) stay as authored.
Callers that can see the filesystem supply the probe, so the module stays free
of node:fs.
Also fixes a second defect in the inliner: `<head>` <link> hrefs and external
script srcs are hoisted into the root document but never went through the
rewrite at all, so even the documented `../` form escaped the project and 404'd
at render time.
Wired into the preview bundler, the producer compiler, the studio preview
builder, the HEVC preview lint, the project lint's asset scans, publish proxy
baking, and media-treatment source resolution.
Fixes#2956
## What
A composition in a subdirectory that references a **sibling** file (`<link rel="stylesheet" href="_shared.css">`, not `../_shared.css`) is now resolved against the composition's own directory when building the standalone sub-composition preview page.
## Why
The preview page borrows the project-root `<base href="/api/projects/:id/preview/">`, but the path rewriter only rewrote `../`-prefixed paths. So `design/styleframes/frame-01.html` referencing `_shared.css` was served unrewritten and the browser requested `/preview/_shared.css` → **404**.
With its stylesheet missing, the frame renders unstyled: `body` has no background, and the thumbnail generator's transparent-body fallback paints it `#1c2028`. Result: dark navy thumbnail with unreadable dark text on every styleframe in the Board view.
The report attributed this to project scale (~21 sibling files). It is not scale-related: a 2-file project reproduces identically, and the same file moved to the project root renders correctly. The trigger is **composition-in-a-subdirectory + relative sibling asset ref**.
Reproduced before the fix (single `curl` against the thumbnail endpoint, plus a direct headless capture of the preview URL):
```
HTTP 404 http://localhost:5190/api/projects/big/preview/_shared.css
body bg: rgb(28, 32, 40)
```
## How
`resolvePreviewAssetPath` in `packages/studio-server/src/helpers/subComposition.ts`, applied through the single rewrite pass all three dispatch branches (template / full-doc / fragment) already share, so `src`, `href`, inline `style` urls, and `<style>` blocks all get the same rule:
1. `../` paths keep resolving against the composition dir (unchanged, shared with the producer's inliner so preview and render agree).
2. Any other relative path is re-pointed at the composition's directory **only when that sibling file exists on disk**.
The disk check is what keeps the two conventions apart: registry blocks are installed into a subdirectory but reference project-root assets (`assets/logo.png`), which are already correct under the root base and have no sibling on disk, so they are left untouched.
Not changed: the `#1c2028` transparent-body fallback in the thumbnail generator. It is correct for genuinely transparent compositions; the illegibility was a downstream symptom of the 404.
## Test plan
- [x] Unit tests added/updated — two tests in `subComposition.test.ts`: a red-first regression guard for the sibling `<link>` / `<img>` / `url()` case, and a guard that project-root-relative refs with no sibling on disk stay untouched.
- [x] Manual testing performed — reproduced the dark thumbnail on a generated project (21-file and 2-file variants both reproduce), then confirmed the same URL renders the white-to-lavender gradient with legible text after the fix, with no 404 in the network log.
- [x] `packages/studio-server` suite green: 29 files / 402 tests. Lint, format, typecheck clean.
- [ ] Documentation updated — n/a
citty parses string options greedily, so a bare --frame-check consumed
the following flag as its value (--caption-zone silently dropped,
--json disabled) and failed with an error blaming --caption-zone.
Normalize rawArgs at the check command boundary (bare --frame-check
followed by a flag or at end becomes --frame-check=), attribute
frame-check spec errors to --frame-check, and detect dash-prefixed
values with corrective guidance.
Fixes#2965
* fix(cli): stop dropping queued telemetry when process.exit races the final flush
Two exit-path defects introduced by the 0.7.65 process-lifecycle refactor:
1. The 'exit' handler returned early once finalizeCli had started, which
also skipped the flushSync() fallback. When an agent-pipe EPIPE killed
the process mid-flush (the NORMAL teardown under Claude Code / Codex),
the still-queued render_complete was silently dropped — fleet delivery
fell from ~90% (0.7.55-0.7.64) to ~35%. flushSync() is now
unconditional: empty queue is a no-op, event uuids dedupe re-sends.
2. The EPIPE handlers set commandFailed unconditionally, so every piped
successful render scored success:false in cli_command_result (fleet
success rate collapsed 89% -> 5-25%). EPIPE now only marks failure
when the pipe died before the render artifact was validated, matching
the existing isRenderSucceeded() exemption on the uncaughtException
path.
Regression tests cover both: flushSync-after-finalize, and EPIPE
before/after artifact validation.
* fix(cli): don't score a validated render as failed due to pre-artifact noise
Review follow-up: commandFailed can be set by noise that precedes artifact
validation — a stray unhandledRejection mid-render, or an EPIPE firing
before markRenderSucceeded on a run that still completes. Once the
artifact validates, that earlier noise must not flip the run's
cli_command_result to success:false. Genuine failures keep a non-zero
exit code and are still caught by the exitCode check.
Extracted commandSucceededForTelemetry() and applied it at both tracking
sites (finalizeCli and the exit handler), with a regression test.
* test(cli): pin the production-reachable producer of the stale-failure override
Review note: the pre-artifact-noise test drives the scenario with an
EPIPE, which only reaches 'render validates afterwards' because
process.exit is mocked — that sequence can't occur in production. Add a
test for the reachable producer: an unhandledRejection before validation
(the handler deliberately does not exit), followed by a validated render,
must score success:true at exit code 0. Verified red on the pre-override
cli.ts.
* fix: bound HDR and video extraction resources
* fix: trim negative video extraction preroll
* fix: skip invisible video extraction windows
* fix: preserve negative-start loop and held tails
* fix: cap finite video slots to source duration
* fix: bound held-tail frame extraction
* fix: plan from playable video duration
* fix: preserve open-ended held video tails
* fix: resolve held tails from decoded frames
* fix: normalize final-frame probe timestamps
* fix: handle unseekable final-frame sources
* fix: dedupe final-frame probes per render
* refactor: clarify output dynamic range contract
Miguel Ángel