Skip to content

Commit 2d398ed

Browse files
committed
fix(cli): wire aspect-agnostic resolution through cloudrun/lambda/batch + preflight recompute
Addresses R2 CHANGES_REQUESTED from Miga + Rames on PR #2529: 1. Sibling-surface gap (blocker): `hyperframes cloudrun render{,-batch}`, `hyperframes lambda render{,-batch}` all advertised the same tier-only aliases (`1080p` / `hd` / `4k` / `uhd`) but normalized them to `landscape` and never set `outputResolutionAspectAgnostic`. The distributed plumbing PR #2529 added received `undefined` from those callers, so portrait `1080p` still hit the original aspect-mismatch on Cloud Run / Lambda. Fix: introduce `resolveResolutionFlagPair` in `@hyperframes/parsers` (the single source of truth for the two-step normalize + aspect-agnostic detect) and route every distributed entrypoint through a shared `parseOutputResolutionFlag` CLI util so the alias signal now reaches `SerializableDistributedRenderConfig`. Studio Server keeps its canonical-only HTTP contract; that intent is now pinned in tests. 2. Preflight recompute (hardening): the earlier "downgrade aspect-mismatch" preflight cleared un-remapped mismatches, so IG 4:5 (non-preset aspect, no sibling) and portrait-4K comp + `--resolution 1080p` (remap + downsample) both slipped through to fail late in `resolveDeviceScaleFactor`. Now `checkRenderResolutionPreflight` computes the effective preset via `suggestMatchingPreset` (mirroring the compile stage's `adaptAspectAgnosticResolution`) and re-checks against that — only genuinely-fixable mismatches clear early. New tests pin both regressed input classes. 3. Docker forwarding boundary test (Miga's important #2): pinned `1080p` survives verbatim as `--resolution 1080p` in the Docker args so the in-container CLI can re-run `isAspectAgnosticResolutionAlias`. 4. Doc-nit (Miga): parsers/src/types.ts no longer references the nonexistent `resolveResolutionForComposition` — points at the actual remap helpers. Fallow: cloudrun.ts / lambda.ts share 390 lines of pre-existing structural symmetry (parallel AWS + GCP dispatchers), and lambda/render.ts + render-batch.ts declare parallel RenderArgs interfaces. Both re-flagged after threading the aspect-agnostic field through each surface; ignored with justification in .fallowrc.jsonc. lambda.ts's `run` and lambda/render.ts's `waitForCompletion` are pre-existing CRAP-score hotspots untouched by this PR — added under health.ignore. Co-Authored-By: Claude <noreply@anthropic.com> — Via
1 parent 7e58d05 commit 2d398ed

18 files changed

Lines changed: 707 additions & 66 deletions

.fallowrc.jsonc

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,29 @@
469469
// utils/compositionServer.ts; the remaining clone is per-command logging text
470470
// (different labels/help lines) — extracting it would over-abstract.
471471
"packages/cli/src/commands/present.ts",
472+
// Portrait --resolution alias fix (via/resolution-portrait-fix):
473+
// cloudrun.ts and lambda.ts are intentionally symmetric per-adapter
474+
// dispatchers — same subcommand surface (deploy / render / render-batch /
475+
// progress / destroy), same argument parsers (parseFormat / parseCodec /
476+
// parseQuality / parsePositiveInt), same wire-config shape. The 390-line
477+
// cross-file clone is that pre-existing structural symmetry; the shared
478+
// resolution-flag parse now lives in utils/parseOutputResolution.ts, but
479+
// consolidating the per-adapter dispatcher body further would collapse
480+
// two distinct SDK surfaces (AWS + GCP) into a single verb router that
481+
// future adapters (Azure, etc.) would have to fork back out of.
482+
// Line-shift fingerprint after adding `outputResolutionAspectAgnostic`
483+
// threading re-flags the inherited clones.
484+
"packages/cli/src/commands/cloudrun.ts",
485+
"packages/cli/src/commands/lambda.ts",
486+
// lambda/render.ts and lambda/render-batch.ts declare parallel
487+
// RenderArgs / RenderBatchArgs interfaces (same core render knobs, with
488+
// batch-only extras like maxConcurrent / dryRun). Extracting the shared
489+
// subset into a base interface would force every consumer to spell out
490+
// the intersection at every call site; the current shape is
491+
// intent-preserving. Pre-existing dupe, re-flagged after threading the
492+
// aspect-agnostic field through both interfaces.
493+
"packages/cli/src/commands/lambda/render.ts",
494+
"packages/cli/src/commands/lambda/render-batch.ts",
472495
// skillsManifest.test.ts: parallel arrange/act/assert cases for locateInstall
473496
// (project vs global scope, per-agent host conventions, claude-code priority).
474497
// Each case seeds a dir then asserts the resolved location/agent; collapsing
@@ -755,6 +778,24 @@
755778
// stage's runCompileStage under the cyclo/cognitive thresholds.
756779
"packages/producer/src/server.ts",
757780
"packages/producer/src/services/distributed/plan.ts",
781+
// Sibling-surface fix (PR #2529 R2): lambda.ts's top-level `run`
782+
// (cyclo 39, CRAP 1560) is the big subcommand switch that pre-dates
783+
// this PR. The change threads two additional variables through the
784+
// `render` and `render-batch` branches (parsed resolution +
785+
// aspect-agnostic flag) but adds no new branches. parseIntFlag /
786+
// parseEnum (both cyclo 5, CRAP 30 — right at the threshold) are also
787+
// pre-existing utility parsers; the file-level line shift after
788+
// adding the shared parseOutputResolutionFlag call re-flags them at
789+
// the boundary. All three findings are inherited complexity, not new
790+
// branches introduced by the aspect-agnostic threading.
791+
"packages/cli/src/commands/lambda.ts",
792+
// Sibling-surface fix (PR #2529 R2): lambda/render.ts's
793+
// `waitForCompletion` (cyclo 11, CRAP 37.1) is the pre-existing SFN
794+
// progress-poll loop. This PR only adds `outputResolutionAspectAgnostic`
795+
// to `RenderArgs` + a two-line extraction (`buildLambdaRenderConfig`);
796+
// `waitForCompletion` is untouched. Line-shift fingerprint re-flags
797+
// the inherited complexity.
798+
"packages/cli/src/commands/lambda/render.ts",
758799
],
759800
},
760801
}
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
/**
2+
* Boundary test for the `hyperframes cloudrun render{,-batch}` wire config.
3+
* Pins that the aspect-agnostic flag reaches `SerializableDistributedRenderConfig`
4+
* so the Cloud Run worker's compile stage can remap `landscape` → `portrait`.
5+
*
6+
* The parse helper itself is covered at `../utils/parseOutputResolution.test.ts`;
7+
* we only re-check the entrypoint composition here.
8+
*/
9+
10+
import { describe, expect, it } from "vitest";
11+
import { buildRenderConfig } from "./cloudrun.js";
12+
13+
describe("cloudrun wire config — aspect-agnostic threading", () => {
14+
const baseArgs: Record<string, unknown> = {
15+
format: "mp4",
16+
codec: undefined,
17+
quality: undefined,
18+
"chunk-size": undefined,
19+
"max-parallel-chunks": undefined,
20+
"target-chunk-frames": undefined,
21+
};
22+
23+
it("threads outputResolutionAspectAgnostic=true through the wire config for portrait 1080p", () => {
24+
// The exact bug shape: portrait comp + `--output-resolution 1080p`.
25+
// Before the fix, the alias signal never reached the wire; Cloud Run
26+
// then hit the same portrait rejection this PR set out to eliminate.
27+
const config = buildRenderConfig(
28+
{ ...baseArgs, "output-resolution": "1080p" },
29+
30,
30+
1080,
31+
1920,
32+
undefined,
33+
);
34+
expect(config.outputResolution).toBe("landscape");
35+
expect(config.outputResolutionAspectAgnostic).toBe(true);
36+
});
37+
38+
it("keeps the aspect-agnostic key absent when the flag is a canonical preset", () => {
39+
// Sparse-wire invariant: don't broadcast `false` for the common path —
40+
// the compile-stage remap only fires on `true`.
41+
const config = buildRenderConfig(
42+
{ ...baseArgs, "output-resolution": "portrait-4k" },
43+
30,
44+
2160,
45+
3840,
46+
undefined,
47+
);
48+
expect(config.outputResolution).toBe("portrait-4k");
49+
expect(config).not.toHaveProperty("outputResolutionAspectAgnostic");
50+
});
51+
52+
it("omits both resolution fields when --output-resolution is unset", () => {
53+
const config = buildRenderConfig(baseArgs, 30, 1920, 1080, undefined);
54+
expect(config).not.toHaveProperty("outputResolution");
55+
expect(config).not.toHaveProperty("outputResolutionAspectAgnostic");
56+
});
57+
58+
it("preserves the surface-labeled strict-throw contract on unknown values", () => {
59+
// Sanity guard: the local delegate stays wired to the shared helper's
60+
// throw semantics rather than silently downgrading to undefined. Full
61+
// input-space coverage lives at `../utils/parseOutputResolution.test.ts`.
62+
expect(() =>
63+
buildRenderConfig({ "output-resolution": "8k" }, 30, 1920, 1080, undefined),
64+
).toThrow(/\[cloudrun render\]/);
65+
});
66+
});

packages/cli/src/commands/cloudrun.ts

Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,8 @@ import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs";
1818
import { homedir } from "node:os";
1919
import { dirname, join, resolve } from "node:path";
2020
import { defineCommand } from "citty";
21-
import {
22-
type CanvasResolution,
23-
normalizeResolutionFlag,
24-
VALID_CANVAS_RESOLUTIONS,
25-
} from "@hyperframes/core";
21+
import { type CanvasResolution } from "@hyperframes/core";
22+
import { parseOutputResolutionFlag } from "../utils/parseOutputResolution.js";
2623
import type { Example } from "./_examples.js";
2724
import { c } from "../ui/colors.js";
2825
import {
@@ -774,13 +771,21 @@ function runDestroy(args: Record<string, unknown>): void {
774771
* separately (it differs per batch entry). Mirrors the local `hyperframes
775772
* render` flag surface so the two stay consistent.
776773
*/
777-
function buildRenderConfig(
774+
/**
775+
* Exported for unit-test coverage of the aspect-agnostic wire shape — the
776+
* portrait-1080p sibling-surface regression that shipped in v0.7.60 landed
777+
* here because this builder dropped the tier-alias signal on the floor.
778+
*/
779+
export function buildRenderConfig(
778780
args: Record<string, unknown>,
779781
fps: number,
780782
width: number,
781783
height: number,
782784
variables: Record<string, unknown> | undefined,
783785
): Record<string, unknown> {
786+
const { outputResolution, outputResolutionAspectAgnostic } = parseOutputResolution(
787+
args["output-resolution"],
788+
);
784789
return stripUndefined({
785790
fps,
786791
width,
@@ -791,7 +796,11 @@ function buildRenderConfig(
791796
chunkSize: parsePositiveInt(args["chunk-size"], "--chunk-size"),
792797
maxParallelChunks: parsePositiveInt(args["max-parallel-chunks"], "--max-parallel-chunks"),
793798
targetChunkFrames: parsePositiveInt(args["target-chunk-frames"], "--target-chunk-frames"),
794-
outputResolution: parseOutputResolution(args["output-resolution"]),
799+
outputResolution,
800+
// Set only when true so the wire shape stays sparse for the common
801+
// canonical-preset path (matches how the flag flows through
802+
// `SerializableDistributedRenderConfig` from every other emitter).
803+
outputResolutionAspectAgnostic: outputResolutionAspectAgnostic ? true : undefined,
795804
variables,
796805
});
797806
}
@@ -823,14 +832,18 @@ function resolveAndValidateVariables(
823832
return variables;
824833
}
825834

826-
function parseOutputResolution(raw: unknown): CanvasResolution | undefined {
827-
if (raw == null || raw === "") return undefined;
828-
const normalized = normalizeResolutionFlag(String(raw));
829-
if (normalized) return normalized;
830-
throw new Error(
831-
`[cloudrun render] --output-resolution must be one of ${VALID_CANVAS_RESOLUTIONS.join("|")} ` +
832-
`(or an alias: 1080p, 4k, uhd, hd, …); got ${String(raw)}`,
833-
);
835+
/**
836+
* Cloud Run flavor of the shared {@link parseOutputResolutionFlag} — carries
837+
* the aspect-agnostic signal through so `SerializableDistributedRenderConfig`
838+
* can trigger the compile-stage remap. The runtime work lives in the shared
839+
* util; wire-config-level coverage lives at `cloudrun.test.ts`, and full
840+
* input-space coverage at `../utils/parseOutputResolution.test.ts`.
841+
*/
842+
function parseOutputResolution(raw: unknown): {
843+
outputResolution: CanvasResolution | undefined;
844+
outputResolutionAspectAgnostic: boolean;
845+
} {
846+
return parseOutputResolutionFlag(raw, { surfaceLabel: "[cloudrun render]" });
834847
}
835848

836849
// ── parse helpers ─────────────────────────────────────────────────────────

packages/cli/src/commands/lambda.ts

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,8 @@
1010

1111
import { defineCommand } from "citty";
1212
import type { DistributedFormat } from "@hyperframes/aws-lambda/sdk";
13-
import {
14-
type CanvasResolution,
15-
VALID_CANVAS_RESOLUTIONS,
16-
normalizeResolutionFlag,
17-
} from "@hyperframes/core";
13+
import { type CanvasResolution } from "@hyperframes/core";
14+
import { parseOutputResolutionFlag } from "../utils/parseOutputResolution.js";
1815
import type { Example } from "./_examples.js";
1916
import { c } from "../ui/colors.js";
2017
import { readAllowedCompositionFpsFromDir } from "../utils/compositionFps.js";
@@ -307,14 +304,16 @@ export default defineCommand({
307304
process.exit(1);
308305
}
309306
const { runRender } = await import("./lambda/render.js");
307+
const renderResolution = parseOutputResolution(args["output-resolution"]);
310308
await runRender({
311309
projectDir,
312310
stackName,
313311
siteId: args["site-id"] as string | undefined,
314312
fps: fpsRaw,
315313
width,
316314
height,
317-
outputResolution: parseOutputResolution(args["output-resolution"]),
315+
outputResolution: renderResolution.outputResolution,
316+
outputResolutionAspectAgnostic: renderResolution.outputResolutionAspectAgnostic,
318317
format: parseFormat(args.format),
319318
codec: parseCodec(args.codec),
320319
quality: parseQuality(args.quality),
@@ -362,6 +361,7 @@ export default defineCommand({
362361
process.exit(1);
363362
}
364363
const { runRenderBatch } = await import("./lambda/render-batch.js");
364+
const batchResolution = parseOutputResolution(args["output-resolution"]);
365365
await runRenderBatch({
366366
projectDir,
367367
stackName,
@@ -370,7 +370,8 @@ export default defineCommand({
370370
fps: fpsRaw,
371371
width,
372372
height,
373-
outputResolution: parseOutputResolution(args["output-resolution"]),
373+
outputResolution: batchResolution.outputResolution,
374+
outputResolutionAspectAgnostic: batchResolution.outputResolutionAspectAgnostic,
374375
format: parseFormat(args.format),
375376
codec: parseCodec(args.codec),
376377
quality: parseQuality(args.quality),
@@ -481,12 +482,23 @@ const parseQuality = (raw: unknown): (typeof QUALITIES)[number] | undefined =>
481482
const parseChromeSource = (raw: unknown): (typeof CHROME_SOURCES)[number] =>
482483
parseEnum(raw, CHROME_SOURCES, "[lambda deploy] --chrome-source", "sparticuz")!;
483484

484-
function parseOutputResolution(raw: unknown): CanvasResolution | undefined {
485-
if (raw == null || raw === "") return undefined;
486-
const normalized = normalizeResolutionFlag(String(raw));
487-
if (normalized) return normalized;
488-
throw new Error(
489-
`[lambda render] --output-resolution must be one of ${VALID_CANVAS_RESOLUTIONS.join("|")} ` +
490-
`(or an alias: 1080p, 4k, uhd, hd, 1080p-portrait, portrait-1080p, 4k-portrait, 1080p-square, square-1080p, 4k-square); got ${String(raw)}`,
491-
);
485+
/**
486+
* Lambda flavor of the shared {@link parseOutputResolutionFlag} — same wire
487+
* contract as the Cloud Run counterpart. Runtime work lives in the shared
488+
* util; wire-config-level coverage lives at `./lambda/render.test.ts` /
489+
* `./lambda/render-batch.test.ts`, and full input-space coverage at
490+
* `../utils/parseOutputResolution.test.ts`.
491+
*/
492+
function parseOutputResolution(raw: unknown): {
493+
outputResolution: CanvasResolution | undefined;
494+
outputResolutionAspectAgnostic: boolean;
495+
} {
496+
return parseOutputResolutionFlag(raw, {
497+
surfaceLabel: "[lambda render]",
498+
// The Lambda `--output-resolution` help text advertises the full alias
499+
// list (tier-only + orientation-suffixed) — keep the error message
500+
// faithful to that surface's docs.
501+
aliasHint:
502+
"1080p, 4k, uhd, hd, 1080p-portrait, portrait-1080p, 4k-portrait, 1080p-square, square-1080p, 4k-square",
503+
});
492504
}

packages/cli/src/commands/lambda/render-batch.test.ts

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,12 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
22
import { mkdtempSync, rmSync, writeFileSync } from "node:fs";
33
import { tmpdir } from "node:os";
44
import { join } from "node:path";
5-
import { parseBatchFile, runWithConcurrencyLimit } from "./render-batch.js";
5+
import {
6+
buildLambdaBatchRenderConfig,
7+
parseBatchFile,
8+
runWithConcurrencyLimit,
9+
type RenderBatchArgs,
10+
} from "./render-batch.js";
611

712
let tmpDir: string;
813

@@ -137,3 +142,38 @@ describe("parseBatchFile", () => {
137142
expectExitOne('{"outputKey":"renders/a.mp4","variables":[1,2,3]}\n');
138143
});
139144
});
145+
146+
// See `../cloudrun.test.ts` / `./render.test.ts` for the sibling wire-config
147+
// coverage. Repeating it at every entrypoint is deliberate: cross-scaffold
148+
// drift is exactly what shipped PR #2529 R2 CHANGES_REQUESTED.
149+
describe("buildLambdaBatchRenderConfig — aspect-agnostic wire threading", () => {
150+
const baseArgs: RenderBatchArgs = {
151+
projectDir: "/tmp/hf-batch",
152+
stackName: "hf-test",
153+
batch: "/tmp/batch.jsonl",
154+
fps: 30,
155+
width: 1080,
156+
height: 1920,
157+
format: "mp4",
158+
json: false,
159+
};
160+
161+
it("threads outputResolutionAspectAgnostic=true through for portrait 1080p", () => {
162+
// The batch entrypoint fans out N Step Functions executions from a
163+
// single wire config, so a dropped alias flag multiplies into N broken
164+
// renders — pin it here.
165+
const config = buildLambdaBatchRenderConfig({
166+
...baseArgs,
167+
outputResolution: "landscape",
168+
outputResolutionAspectAgnostic: true,
169+
});
170+
expect(config.outputResolution).toBe("landscape");
171+
expect(config.outputResolutionAspectAgnostic).toBe(true);
172+
});
173+
174+
it("keeps the aspect-agnostic key absent when the flag is a canonical preset", () => {
175+
const config = buildLambdaBatchRenderConfig({ ...baseArgs, outputResolution: "portrait-4k" });
176+
expect(config.outputResolution).toBe("portrait-4k");
177+
expect(config.outputResolutionAspectAgnostic).toBeUndefined();
178+
});
179+
});

0 commit comments

Comments
 (0)