Skip to content

feat(cli): add --composition flag to render specific compositions#631

Merged
miguel-heygen merged 2 commits intomainfrom
feat/cli-composition-render
May 6, 2026
Merged

feat(cli): add --composition flag to render specific compositions#631
miguel-heygen merged 2 commits intomainfrom
feat/cli-composition-render

Conversation

@miguel-heygen
Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen commented May 5, 2026

Summary

  • Add --composition / -c flag to hyperframes render to render a specific composition file instead of index.html
  • Threads through both local and Docker render paths
  • Validates the file exists before starting the render with a clear error message
  • Leverages the existing entryFile config in the producer — no engine changes needed

Usage

# Render a specific sub-composition
hyperframes render -c compositions/intro.html -o intro.mp4

# Still works without the flag (renders index.html as before)
hyperframes render -o output.mp4

Changes

File Change
packages/cli/src/commands/render.ts Add --composition / -c arg, validate file exists, pass entryFile to both local and Docker render paths, show composition name in render plan output
packages/cli/src/utils/dockerRunArgs.ts Add entryFile to DockerRenderOptions, forward as --composition flag to container
packages/cli/src/utils/dockerRunArgs.test.ts 2 new tests: forwards --composition when set, omits when not
docs/packages/cli.mdx Add composition render example to quickstart

Test results

Automated tests

  • All 15 dockerRunArgs tests pass (13 existing + 2 new)
  • bunx vitest run packages/cli/src/utils/dockerRunArgs.test.ts — 15/15 passed

Manual tests

Error handling — nonexistent file:

$ hyperframes render -c nonexistent.html -o /tmp/test.mp4 ./my-project

✗  Composition not found

   "nonexistent.html" does not exist in the project directory.
   Use 'hyperframes compositions' to list available compositions.

Happy path — render broadcast-kit composition:

$ hyperframes render -c compositions/broadcast-kit.html -o /tmp/broadcast-test.mp4 -f 30 -q draft ./html-in-canvas-showcase

◆  Rendering html-in-canvas-showcase/compositions/broadcast-kit.html → /tmp/broadcast-test.mp4
   30fps · draft · auto workers (14 cores detected)

   600 frames captured (20s × 30fps — matches data-duration="20")
   561.8 KB · 10.0s · completed

Backwards compatible — no flag renders index.html as before:

$ hyperframes render -o /tmp/default.mp4 ./my-project
# Renders index.html (unchanged behavior)

Expose the existing entryFile config in the producer through
a new --composition / -c CLI flag. This lets users render
individual composition files without restructuring their project:

  hyperframes render -c compositions/intro.html -o intro.mp4

The flag validates the file exists before starting the render,
threads through both local and Docker render paths, and is
documented in the CLI help, examples, and docs.
@mintlify
Copy link
Copy Markdown

mintlify Bot commented May 5, 2026

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
hyperframes 🟢 Ready View Preview May 5, 2026, 7:15 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean, minimal, well-designed. Plumbs an existing producer field through the CLI rather than inventing new mechanism.

What I checked

  1. The producer-side claim is real. packages/producer/src/server.ts:77 already declares entryFile?: string on RenderJobOptions, parses it at line 100-103 (trim + truthy check), validates existence at line 124-126, and falls back to "index.html" at line 122. The PR's claim of "leverages the existing entryFile config — no engine changes needed" matches the source. ✓

  2. Backward compat. args.composition?.trim() || undefined at line 273 — empty string and whitespace-only treated as not-set. Producer defaults to index.html when undefined. Existing renders without the flag are byte-identical. ✓

  3. Validation. statSync against resolve(project.dir, entryFile) at lines 277-285 with the bare catch {} → friendly errorBox + process.exit(1). Manual test in the PR body shows the actual error UX. The bare catch can't distinguish "file missing" from "permission denied" but for a local CLI both lead the user to the same investigation. ✓

  4. Docker passthrough. Conditional --composition <entryFile> append at dockerRunArgs.ts:71. Two new tests at dockerRunArgs.test.ts:214-228 cover both branches (forwards when set, omits when not). ✓

  5. Render plan output. nameLabel = entryFile ? project.name + "/" + entryFile : project.name makes it visible to the user which file is being rendered when -c is passed. Reads cleanly. ✓

  6. Naming. --composition / -c is consistent with the existing HF CLI conventions (singular, kebab-case, lowercase short alias matching the first letter of the long form — -c for --composition, just like -o for --output). ✓

  7. CI. All 42 checks green or in_progress (Render on windows-latest still running at review time). Lint, Typecheck, Build, Test, Test: runtime contract, CLI smoke, Tests on windows-latest, Smoke: global install — all pass.

Non-blocking observations

  • Test gap on the CLI-side validation branch. The PR adds the existence-check logic in render.ts but only tests the Docker passthrough. A render.test.ts test for "render command surfaces errorBox and exits with code 1 when --composition points to a missing file" would round out the coverage. Manual test in the PR body covers the user-visible behavior so this isn't blocking, but worth a small follow-up commit.

  • Path containment. --composition ../../../some/other/dir/file.html would pass both the CLI's existence check and the producer's existence check at server.ts:125. hyperframeLint.ts:55-57 has a directory-containment guard (if (!absoluteEntryPath.startsWith(absProjectDir))) but the render path in server.ts doesn't. For a local CLI run this is a UX point rather than a security boundary — the user has shell access to anything they could pass via -c. Worth mirroring the guard in server.ts for symmetry with the lint service, but that's a separate refactor that doesn't belong in this PR. Flagging only because it's a quiet inconsistency between two adjacent producer surfaces.

LGTM. Posted as COMMENT per the stamp policy.

— Rames Jusso

Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: comment — change is well-scoped and the docker plumbing is solid, but a few things should land before merge.

This is a clean exposure of an existing producer capability (entryFile) to the CLI surface. The diff is small, threads through both render paths, and adds the right kind of test for the docker arg wiring. Two real gaps below — neither is a hard blocker, but important #1 (path traversal) shouldn't ship without a fix or an explicit decision to defer.

Important

  • packages/cli/src/commands/render.ts:273-287 — no path-traversal guard on --composition.
    resolve(project.dir, entryFile) followed by statSync will happily accept --composition ../../../etc/hosts (or anything outside project.dir). The exact same codebase already enforces this for entryFile in packages/producer/src/services/hyperframeLint.ts:55-58:

    const absoluteEntryPath = resolve(absProjectDir, entryFile);
    if (!absoluteEntryPath.startsWith(absProjectDir)) {
      return { error: `Entry file must stay inside project directory: ${entryFile}` };
    }

    Render should mirror that. The producer ultimately reads the file via join(projectDir, entryFile) in renderOrchestrator.ts:1974, so the CLI is the right place to fail fast with a clear error. The Docker mount is :ro which limits blast radius for the container path, but local renders run in the user's own FS context — escaping project.dir is real. Add the prefix check next to the statSync and re-use the lint message style.

  • packages/cli/src/commands/render.ts — no test that entryFile is forwarded to createRenderJob.
    The existing test pattern is right there (render.test.ts:105-134 — "forwards parsed --variables payload to createRenderJob" / "omits variables ... when not provided"). The docker-side wiring is well-covered by the new dockerRunArgs.test.ts cases, but the local-render path has no equivalent assertion. A copy-paste of the variables test pattern with entryFile: "compositions/intro.html" would close this and match the file's existing convention. Without it, a future refactor of renderLocal could drop entryFile silently and CI would stay green.

  • packages/cli/src/utils/dockerRunArgs.test.ts:150-179 — "regression tripwire" test not updated.
    The comment in that test explicitly says: "If a future option is added but only wired through to renderLocal, this test forces the author to update buildDockerRunArgs (and add a check here) too." The PR added a new option and added dedicated tests for it (good), but didn't extend the tripwire. That weakens the convention the file is trying to establish. Add entryFile: "compositions/intro.html" to the options and expect(args).toContain("--composition") to the assertions block.

Nits

  • packages/cli/src/commands/render.ts:283 — error message points users at hyperframes compositions, but that command lists composition IDs (data-composition-id), not file paths.
    The source column does surface the file path for sub-compositions, which covers the main use case. Still, a user who has a top-level-only index.html will run hyperframes compositions, see an ID like intro, and try --composition intro — that'll fail. Consider either (a) updating the compositions output to always show a "render with: --composition " hint, or (b) softening the error to "Pass a path to a .html file relative to the project directory (e.g. compositions/intro.html)." Out of scope for this PR if you want, but worth a follow-up issue.

  • packages/cli/src/commands/render.ts:294project.name + "/" + entryFile doesn't normalize.
    If someone runs --composition ./compositions/intro.html, the render-plan line prints myproj/./compositions/intro.html. Cosmetic only. entryFile.replace(/^\.\//, "") or relative() would be cleaner.

  • packages/cli/src/commands/render.ts:66-71 — the description "Render a specific composition file instead of index.html" is good, but doesn't mention the engine's actual constraint: sub-compositions using <template> wrappers require the project's index.html to exist and reference the file via data-composition-src (see renderOrchestrator.ts:1984-2000). Worth a one-line "must be referenced from index.html via data-composition-src for sub-compositions" so users don't try to render an arbitrary HTML file and get a confusing error from the engine.

Praise

  • Threading the option through both renderLocal and renderDocker in one PR (rather than the historical pattern of forgetting one path — see the --hdr regression that the dockerRunArgs file is now structured to prevent) is exactly right.
  • The validation-then-pass-through approach (no engine changes, leverages existing producer config) is the minimum-blast-radius design. Good call.
  • Both forward and omit cases tested for the docker arg builder — that's the right shape.

— Vai

…ipwire

- Add path-containment check mirroring hyperframeLint.ts: reject
  --composition paths that escape the project directory
- Normalize leading ./ from composition paths for clean render plan output
- Improve error message: suggest .html file path instead of compositions command
- Add description note about <template> sub-composition constraint
- Add render.test.ts: entryFile forwarded to createRenderJob (forward + omit)
- Update dockerRunArgs tripwire test with entryFile coverage
Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE — all three important items from my prior review addressed cleanly in 90399718. No new blockers.

Addressed since prior review

  1. Path-traversal guard ✅ — packages/cli/src/commands/render.ts:280-286 (commit 90399718). Mirrors hyperframeLint.ts:55-58 exactly: resolves absProjectDir, checks entryPath.startsWith(absProjectDir), fails fast with errorBox + process.exit(1). Same pattern, same trade-offs as the existing producer-side guard — consistent.

  2. Forward-test for entryFilecreateRenderJob ✅ — packages/cli/src/commands/render.test.ts:136-151 (forward case) and :153-166 (omit case). Asserts producerState.createdJobs[0]?.entryFile === "compositions/intro.html" and the symmetric toBeUndefined() when the flag is absent. Covers the silent-drop failure mode I flagged.

  3. Tripwire test extended ✅ — packages/cli/src/utils/dockerRunArgs.test.ts:164 adds entryFile: "compositions/intro.html" to options; :180-181 asserts args contains both --composition and compositions/intro.html. Future option additions will now hit the tripwire as intended.

Bonus nits also addressed

  • render.ts:70-71 — description now calls out the <template> sub-composition / data-composition-src constraint. 👍
  • render.ts:276.replace(/^\.\//, "") normalizes the ./ prefix so the render-plan label is clean.
  • render.ts:291 — error message no longer points at hyperframes compositions; now suggests a .html path directly.

All three "skip-friendly" nits from the prior review picked up anyway — nice cleanup.

Still open

None.

New findings

None blocking. One observation, non-actionable: the startsWith(absProjectDir) check inherits the same sibling-prefix edge case as hyperframeLint.ts (e.g., a project at /foo/bar would technically not reject a resolved path at /foo/bar-evil/x). The producer-side guard has lived with this since it shipped, and consistency between the two call sites is more valuable than diverging here. If we ever want to harden it, do it in both places at once with path.relative + .. check.

— Vai

Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — re-review at 90399718. All concerns from my round-1 review and Vai's resolved cleanly in 90399718.

Prior-concern resolution

Vai's Importants:

  • Path-traversal guard (#1) — render.ts:273-279 adds entryPath.startsWith(absProjectDir) check, mirrors hyperframeLint.ts:55-58 style and message
  • entryFile forwarded to createRenderJob test (#2) — render.test.ts:136-166 adds two cases (forwards-when-set, omits-when-not), mirroring the existing variables test pattern at :105-134
  • Tripwire test update (#3) — dockerRunArgs.test.ts:163 + :180-181 adds entryFile to options and --composition / compositions/intro.html to assertions, restoring the file's "every option must reach the container exactly once" invariant

Vai's Nits — all addressed:

  • ✅ Description now mentions the data-composition-src constraint for sub-comps using <template> wrappers (render.ts:67-70)
  • ✅ Path-normalization on ./ prefix (render.ts:274: .replace(/^\.\//, "")) — render-plan output and validation use the normalized form
  • ✅ Error message replaced "Use hyperframes compositions to list available" → "Pass a path to a .html file relative to the project root (e.g. compositions/intro.html)." Avoids the IDs-vs-paths confusion Vai called out

My round-1 observations: both addressed (the test-gap was Vai's #2, the path-containment was Vai's #1 — my round-1 framing on both was looser, Vai's was specific. Coverage is identical.)

New finding from re-review (non-blocking, pre-existing pattern)

render.ts:276startsWith(absProjectDir) has a classic prefix pitfall.

const absProjectDir = resolve(project.dir);
const entryPath = resolve(absProjectDir, entryFile);
if (!entryPath.startsWith(absProjectDir)) { ... }

For a project at /home/user/coolproj/, passing --composition ../coolproj-evil/secret.html resolves to /home/user/coolproj-evil/secret.html, which startsWith("/home/user/coolproj") returns true (because coolproj-evil shares the prefix coolproj). The guard passes, and the producer happily renders a file from the sibling directory.

The fix is the standard one — require a path separator after the project dir, or use path.relative():

import { sep } from "node:path";
// either:
if (entryPath !== absProjectDir && !entryPath.startsWith(absProjectDir + sep)) { ... }
// or:
import { relative } from "node:path";
if (relative(absProjectDir, entryPath).startsWith("..")) { ... }

Why I'm calling this non-blocking:

  1. Same pattern lives at packages/producer/src/services/hyperframeLint.ts:55-58 — this PR is mirroring an existing convention, and the bug is pre-existing on that surface too. A proper fix is "tighten both surfaces in one follow-up" rather than diverging this one.
  2. For a local CLI, the user has shell access to anything they could pass via -c, so this is a UX/intent guard rather than a security boundary. A user could cp ../coolproj-evil/secret.html ./ and pass the local path with no guard at all.
  3. Realistic exploit requires a sibling directory whose name happens to share the project dir's name as a prefix, which is unusual in practice.

That said — happy to file this as a separate "tighten path-containment guards across CLI + lint service" issue, or address it in this PR if you'd prefer the symmetry from the start. Either is fine.

Summary

Three rounds of review (mine round-1 + Vai's round-1 + this re-review), zero echo, all importants resolved, all nits resolved. Solid iteration loop. The new prefix-pitfall finding is a follow-up at most — the change is strictly better than master and the bug it inherits is already shared with the lint service.

Ship it.

— Re-review by Rames Jusso (pr-review)

@miguel-heygen miguel-heygen merged commit 0e0a0e4 into main May 6, 2026
29 checks passed
@miguel-heygen miguel-heygen deleted the feat/cli-composition-render branch May 6, 2026 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants