Merge pull request #2529 from heygen-com/via/resolution-portrait-fix

fix(cli): accept portrait aspects for --resolution alias flag

packages/cli/src/utils/dockerRunArgs.test.ts

@@ -377,6 +377,26 @@ describe("buildDockerRunArgs", () => {
     expect(args[idx + 1]).toBe("landscape-4k");
   });
 
+  it("forwards the RAW aspect-agnostic alias `1080p` verbatim (does not pre-normalize to `landscape`)", () => {
+    // Miga R2 important note on PR #2529: Docker correctness now depends on
+    // forwarding the raw alias string, not the canonical preset — the
+    // in-container CLI re-runs `normalizeResolutionFlag` +
+    // `isAspectAgnosticResolutionAlias` so aspect-agnostic aliases keep
+    // their orientation-adaptive behavior. A future refactor that
+    // silently substitutes the normalized preset here would restore the
+    // portrait-only Docker failure this test pins against.
+    const args = buildDockerRunArgs({
+      ...FIXED_INPUT,
+      options: { ...BASE, outputResolution: "1080p" },
+    });
+    const idx = args.indexOf("--resolution");
+    expect(idx).toBeGreaterThan(-1);
+    expect(args[idx + 1]).toBe("1080p");
+    // Belt-and-braces: the normalized preset name must NOT slip in as a
+    // second value that would confuse citty parsing on the container side.
+    expect(args).not.toContain("landscape");
+  });
+
   it("omits --resolution when outputResolution is not set", () => {
     const args = buildDockerRunArgs({ ...FIXED_INPUT, options: BASE });
     expect(args).not.toContain("--resolution");

packages/cli/src/utils/parseOutputResolution.test.ts

@@ -0,0 +1,105 @@
+/**
+ * Boundary tests for the shared `parseOutputResolutionFlag` helper. Every
+ * distributed entrypoint (`hyperframes cloudrun render{,-batch}`,
+ * `hyperframes lambda render{,-batch}`) delegates to this one function —
+ * covering it here (rather than at each surface) makes the sibling-surface
+ * regression this PR fixes unreachable by construction: any future surface
+ * that calls `parseOutputResolutionFlag` inherits the correct alias
+ * threading. The wire-config-level tests at
+ * `../commands/cloudrun.test.ts` / `../commands/lambda/render.test.ts` /
+ * `../commands/lambda/render-batch.test.ts` still exercise the
+ * per-entrypoint composition so the plumbing stays end-to-end covered.
+ */
+
+import { describe, expect, it } from "vitest";
+import { parseOutputResolutionFlag } from "./parseOutputResolution.js";
+
+const CLOUDRUN = { surfaceLabel: "[cloudrun render]" } as const;
+const LAMBDA = {
+  surfaceLabel: "[lambda render]",
+  aliasHint:
+    "1080p, 4k, uhd, hd, 1080p-portrait, portrait-1080p, 4k-portrait, 1080p-square, square-1080p, 4k-square",
+} as const;
+
+describe("parseOutputResolutionFlag", () => {
+  it.each([undefined, "", null])(
+    "returns undefined + false when the flag is omitted (raw=%s)",
+    (raw) => {
+      expect(parseOutputResolutionFlag(raw, CLOUDRUN)).toEqual({
+        outputResolution: undefined,
+        outputResolutionAspectAgnostic: false,
+      });
+    },
+  );
+
+  it.each(["landscape", "portrait-4k", "square", "square-4k"])(
+    "normalizes canonical preset %s with aspect-agnostic=false",
+    (preset) => {
+      const { outputResolution, outputResolutionAspectAgnostic } = parseOutputResolutionFlag(
+        preset,
+        CLOUDRUN,
+      );
+      expect(outputResolution).toBe(preset);
+      expect(outputResolutionAspectAgnostic).toBe(false);
+    },
+  );
+
+  // The blocker path from Miga's R2 review: without this pair, a portrait
+  // composition with `--output-resolution 1080p` reaches the compile stage
+  // as the explicit `landscape` preset and rejects with the original
+  // aspect-mismatch instead of remapping to `portrait`.
+  it.each(["1080p", "hd", "4k", "uhd"])(
+    "flags aspect-agnostic tier alias %s so the compile stage can remap orientation",
+    (alias) => {
+      const { outputResolution, outputResolutionAspectAgnostic } = parseOutputResolutionFlag(
+        alias,
+        CLOUDRUN,
+      );
+      expect(outputResolutionAspectAgnostic).toBe(true);
+      expect(outputResolution).toBeDefined();
+    },
+  );
+
+  it.each(["1080p-portrait", "portrait-1080p", "1080p-square", "4k-portrait", "4k-square"])(
+    "does NOT flag orientation-suffixed alias %s as aspect-agnostic",
+    (alias) => {
+      // The user picked an orientation — respect it, don't silently swap.
+      const { outputResolutionAspectAgnostic } = parseOutputResolutionFlag(alias, CLOUDRUN);
+      expect(outputResolutionAspectAgnostic).toBe(false);
+    },
+  );
+
+  it("treats input case-insensitively (1080P, UHD, HD, 4K all pass)", () => {
+    for (const alias of ["1080P", "UHD", "HD", "4K"]) {
+      expect(parseOutputResolutionFlag(alias, CLOUDRUN).outputResolutionAspectAgnostic).toBe(true);
+    }
+  });
+
+  it("throws with the caller-supplied surface label on unknown values", () => {
+    // The two surfaces MUST use the same underlying helper (see PR #2529 —
+    // divergent copies is exactly the cross-scaffold drift class this
+    // consolidation prevents), but each stakes its own label so debugging
+    // still points at the right verb.
+    expect(() => parseOutputResolutionFlag("8k", CLOUDRUN)).toThrow(/\[cloudrun render\]/);
+    expect(() => parseOutputResolutionFlag("8k", LAMBDA)).toThrow(/\[lambda render\]/);
+  });
+
+  it("appends the caller-supplied aliasHint to the error text (so the message stays surface-accurate)", () => {
+    // Lambda advertises the full orientation-suffixed alias list in help
+    // text; the error message must match that surface for the user's
+    // "did you mean?" search to land on real docs.
+    const err = getThrown(() => parseOutputResolutionFlag("8k", LAMBDA));
+    expect(err.message).toContain("1080p-portrait");
+    expect(err.message).toContain("4k-portrait");
+  });
+});
+
+function getThrown(fn: () => void): Error {
+  try {
+    fn();
+  } catch (e) {
+    if (e instanceof Error) return e;
+    throw new Error(`Non-Error thrown: ${String(e)}`);
+  }
+  throw new Error("Expected fn to throw, but it did not");
+}

packages/cli/src/utils/parseOutputResolution.ts

@@ -0,0 +1,67 @@
+/**
+ * Shared `--output-resolution` / `--resolution` normalizer for the distributed
+ * render entrypoints (`hyperframes cloudrun render{,-batch}`, `hyperframes
+ * lambda render{,-batch}`) plus the local `hyperframes render` command.
+ *
+ * The one field this helper carries that the previous per-surface copies
+ * were dropping is `outputResolutionAspectAgnostic`: `true` when the raw
+ * flag was a tier-only alias (`1080p` / `hd` / `4k` / `uhd`). Passing it
+ * through into `SerializableDistributedRenderConfig` is what lets the
+ * remote worker's compile stage remap `landscape` → `portrait` when the
+ * composition demands it. Dropping the flag at any single entrypoint
+ * reproduces the portrait-1080p regression this helper prevents (see
+ * PR #2529 R2 CHANGES_REQUESTED and the sibling-surface enumeration in
+ * Miga + Rames's reviews).
+ *
+ * The strict-throw contract (unknown values raise instead of silently
+ * degrading to `outputResolution: undefined`) is preserved so a typo like
+ * `--output-resolution 8k` fails fast rather than falling back to
+ * composition dimensions.
+ */
+
+import { type CanvasResolution, resolveResolutionFlagPair } from "@hyperframes/core";
+import { VALID_CANVAS_RESOLUTIONS } from "@hyperframes/core";
+
+/**
+ * Free-text prefix the thrown error is scoped to (e.g. `"[cloudrun render]"`,
+ * `"[lambda render]"`). Kept as a caller-supplied string rather than a
+ * fixed enum so future surfaces (Studio Server, an SDK wrapper, …) can
+ * opt in without editing this file.
+ */
+export interface OutputResolutionParseOptions {
+  surfaceLabel: string;
+  /**
+   * Optional per-surface hint appended to the error message. Defaults to a
+   * generic tier-alias hint; the Lambda surface exposes additional
+   * orientation-suffixed aliases the CLI accepts (`1080p-portrait`, `4k-portrait`,
+   * …) — pass a custom hint to keep the error text faithful.
+   */
+  aliasHint?: string;
+}
+
+/**
+ * Parse the user-supplied resolution flag into the pair the distributed
+ * wire config needs. Returns `{ outputResolution: undefined,
+ * outputResolutionAspectAgnostic: false }` when the flag is absent so the
+ * caller can spread the result unconditionally.
+ *
+ * Throws (not exits) on an unknown value — CLI callers wrap that in their
+ * own errorBox / process.exit; SDK callers surface the error to their own
+ * user.
+ */
+export function parseOutputResolutionFlag(
+  raw: unknown,
+  options: OutputResolutionParseOptions,
+): { outputResolution: CanvasResolution | undefined; outputResolutionAspectAgnostic: boolean } {
+  if (raw == null || raw === "") {
+    return { outputResolution: undefined, outputResolutionAspectAgnostic: false };
+  }
+  const asString = String(raw);
+  const { outputResolution, outputResolutionAspectAgnostic } = resolveResolutionFlagPair(asString);
+  if (outputResolution) return { outputResolution, outputResolutionAspectAgnostic };
+  const aliasHint = options.aliasHint ?? "1080p, 4k, uhd, hd, …";
+  throw new Error(
+    `${options.surfaceLabel} --output-resolution must be one of ${VALID_CANVAS_RESOLUTIONS.join("|")} ` +
+      `(or an alias: ${aliasHint}); got ${asString}`,
+  );
+}