fix(producer): honor variables + outputResolution in HTTP render server#1152
Conversation
The producer HTTP server's parseRenderOptions read only fps/quality/workers/gpu/debug/entryFile/format from the request body. `variables` and `outputResolution` were silently dropped, so any caller of the server render path (the cloud-render sidecar that experiment-framework POSTs to) got the composition's declared variable defaults and its intrinsic dimensions regardless of what was requested. RenderConfig already supports both fields (the local CLI `render` command passes them); the server just never forwarded them. Wire them through RenderInput, parseRenderOptions, and a shared buildRenderJobConfig used by the sync + streaming handlers. outputResolution now drives the same resolveDeviceScaleFactor supersampling path the local CLI uses, so a 4k render against a matching-aspect composition produces true 4k. Validation: a non-object `variables` or an unknown `outputResolution` returns a clean 400 instead of being silently ignored. Also extracts resolvePreparedRenderOutput + parseRenderOverrides helpers to keep both handlers DRY. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This stack of pull requests is managed by Graphite. Learn more about stacking. |
vanceingalls
left a comment
There was a problem hiding this comment.
Fixes a real silent-drop bug that had been live in the cloud render path. Both fields were plumbed correctly through the backend — they were dropped only at this surface. The buildRenderJobConfig extraction is clean DRY work.
Strengths
buildRenderJobConfigextraction: path-resolution +createRenderJobspread were copy-pasted across both handlers; single source of truth now. ✓- Strict
isPlainObjectguard onvariables: rejecting arrays, primitives, null with a named 400 is correct. ✓ normalizeResolutionFlagreuse: server and CLI stay in sync automatically when new presets land. ✓- CI:
baseRefName: main— fully green, earlier CANCELLED run was superseded by a newer push. ✓
Important — validateRenderOverrides silently drops non-string outputResolution, contradicting the stated guarantee
The PR says "invalid input is rejected with a clean 400 instead of being silently ignored." True for variables, false for outputResolution when the caller sends a non-string.
validateRenderOverrides({ outputResolution: 123 }): typeof 123 === "string" is false → branch skipped → parseRenderOverrides coerces to undefined → render proceeds at intrinsic dimensions. Same silent-drop the PR exists to eliminate.
Not a live break (the backend always sends a string), but a plausible client mistake and the stated guarantee is falsified. Fix — add before the existing string-path check:
if (body.outputResolution !== undefined && typeof body.outputResolution !== "string") {
return "outputResolution must be a string preset";
}And flip the existing toBeUndefined() test to assert the 400 via prepareRenderBody.
Nits
normalizeResolutionFlagis called twice per request (validate + parse). Not a correctness issue.- Aspect-ratio mismatch surfaces as 500 from
resolveDeviceScaleFactorthrowing inside the pipeline. A 400 would be better UX but requires compiled composition dimensions — follow-up, not a blocker.
The core fix is correct and well-structured. The outputResolution non-string gap is important for the stated contract but not a production break.
— Vai
Addresses review on #1152. - A non-string `outputResolution` (e.g. a JSON number) was coerced to `undefined` by parseRenderOverrides and silently ignored — the same silent-drop this validation exists to prevent. Now rejected with a 400. - `outputResolution` + an alpha format (webm/mov) is rejected up front: supersampling runs through a deviceScaleFactor the alpha capture path can't apply, so resolveDeviceScaleFactor throws mid-render. Guarding it here makes the producer self-defending for every caller (not just the CLI / external API), and closes the 1080p-webm regression window during the producer-honors-outputResolution rollout. Extracted validateOutputResolutionOverride to keep validateRenderOverrides under the complexity gate. +2 prepareRenderBody tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@vanceingalls thanks — addressed in
Nits: the double |
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed commit 00375f67. All three gaps from round 1 are closed:
- Non-string
outputResolution→ 400:validateOutputResolutionOverridecorrectly rejects non-string inputs (typeof !== 'string') before any coercion. New test with{ outputResolution: 123 }confirms the rejection fires. - outputResolution + alpha (webm/mov) → 400: Defense-in-depth guard added in the producer layer, covering the full combination. Test covers both webm and mov.
- Extraction of
validateOutputResolutionOverride: Clean standalone export; separation from parse-level coercion is correct.
One minor design note (not a blocker): empty-string "" coerces to undefined silently (length === 0 short-circuits). Equivalent to omission, so it's fine — just noting the asymmetry with the numeric-input rejection for future readers.

Summary
The producer HTTP render server (
@hyperframes/producer— the surface the cloud-render sidecar inhyperframes-internalmounts viacreateRenderHandlers) silently dropped two request fields:variablesandoutputResolution.parseRenderOptionsonly readfps / quality / workers / gpu / debug / entryFile / format. So any caller of the server render path got:--variablesoverrides ignored), andThis is the root cause of two black-box bugs found testing
hyperframes cloud render:--variableshad no effect, and--resolution 4kproduced a 1080p file. The backend (experiment-framework controller → workflow → streaming activity) plumbs both fields correctly all the way to the sidecar POST body — they were dropped only here.Fix
RenderConfigalready supportsvariablesandoutputResolution(the localhyperframes renderCLI passes them). This wires them through the server:RenderInputgains both fields.parseRenderOptionsparses them (variablesmust be a plain object;outputResolutionnormalized vianormalizeResolutionFlag, accepting aliases like4k).buildRenderJobConfigfeeds both the sync and streaming handlers, so the field set lives in one place.outputResolutionnow drives the sameresolveDeviceScaleFactorsupersampling path the local CLI uses — a 4k render against a matching-aspect composition produces true 4k (Chrome renders at higher DPR).Invalid input is rejected with a clean 400 (
validateRenderOverrides) instead of being silently ignored.Testing
packages/producer/src/server.test.ts(7 cases): variables object/non-object parsing, outputResolution alias normalization + rejection, and end-to-end threading into the prepared render input.bun test packages/producer/src/server.test.ts→ 7 pass.