feat(cli): hyperframes lambda render --variables / --variables-file / --strict-variables#966
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
cbbb7bc to
24c52b0
Compare
0445b42 to
0b1732d
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Clean refactor + feature addition. The variables logic is hoisted faithfully from render.ts into packages/cli/src/utils/variables.ts with byte-identical implementations, then wired into the lambda CLI surface with identical flag names and validation behavior.
reportVariableIssues extraction with the { strict, quiet } options bag is a nice touch. The graceful skip when index.html is missing (for --site-id workflows) is correct. Tests moved 1:1 with updated imports. Docs updated with usage examples.
Backwards compatibility is solid — absent --variables and --variables-file produce undefined which is omitted from config.
CI green. Ship it.
vanceingalls
left a comment
There was a problem hiding this comment.
Phase 9.3 of the variables stack — bringing the local --variables / --variables-file / --strict-variables UX to hyperframes lambda render. The CLI surface is symmetric with the local flow, the shared helpers are well-factored, and the threading from CLI args through runRender → renderToLambda → validateDistributedRenderConfig (which now calls validateVariablesPayload) is correct.
Strengths
- Real deduplication, not a cosmetic move. The hoist of
parseVariablesArg/resolveVariablesArg/validateVariablesAgainstProjectfrompackages/cli/src/commands/render.tstopackages/cli/src/utils/variables.tscollapses two copies of the warning-block printing into a singlereportVariableIssueshelper. The diff inrender.ts(lines 513-535 pre vs post) drops ~20 lines of inline UI that's now centrally owned. That's how this kind of refactor should look. - Layered validation that fails at the right boundary. Structural validation (
validateVariablesPayload) lives in the SDK and runs unconditionally insidevalidateDistributedRenderConfig; semantic schema validation (validateVariablesAgainstProject) lives in the CLI and only runs whenindex.htmlis present on disk. That split is correct — schema lives next to the project, structural shape is wire-relevant for any caller. The unit tests on the parser cover conflict / parse-error / shape-error / read-error / type-mismatch / undeclared paths. - Symmetric flag spelling and error messages.
--variables,--variables-file,--strict-variablesare byte-identical to the local CLI flags, including theerrorBoxtitles. Users who learned the local flow won't trip over a renamed flag at the lambda surface.
Findings
important — --strict-variables silently degrades when projectDir isn't on disk
packages/cli/src/commands/lambda/render.ts skips the entire validation block when existsSync(indexPath) is false (the typical --site-id-only flow). The comment frames this as intentional ("the project dir isn't on disk — --site-id pointing at a pre-uploaded site that was packaged elsewhere"), and the reasoning is sound: with no index.html, there's no schema to validate against.
But the consequence is that --strict-variables becomes a no-op in exactly the case where users explicitly asked for strict checking. Failure mode:
hyperframes lambda render --site-id sha-abc \
--variables '{"tytle":"Hello"}' \ # typo, undeclared
--strict-variablesUser expects: command aborts with "undeclared variable tytle". Actual: command silently proceeds — the typo flows to the chunk workers as window.__hfVariables.tytle, the composition reads data-composition-variables → "title", gets undefined, and the render produces a broken video. The user paid for the run, got nothing actionable, and never saw a warning.
Fix options (any of these — listed easiest → most invasive):
- When
strict && !existsSync(indexPath), log a single warning line:--strict-variables specified but no index.html at <path>; schema validation skipped. Cheap, preserves current behavior, kills the surprise. - Treat
--strict-variableswith no schema as an error: require the user to either drop--strict-variablesor run from a directory that hasindex.html. Hardest stance, most defensible — strict means strict. - (Future-shaped, out of scope here) Resolve the schema from the uploaded site: download
index.htmlfroms3://.../sites/<siteId>/project.tar.gzfor the validation pass. Real fix, but lives in PR 9.4+ once the upload artifact contract is firm.
Recommend (1) for this PR — one console.warn call, no schema-resolution work. Document the limitation in the docs section you added.
important — no dispatcher test for the variables threading
runRender in packages/cli/src/commands/lambda/render.ts is the load-bearing wire that builds config: { ..., variables } and hands it to renderToLambda. The change is small and visually correct, but the codebase has no test that exercises runRender itself — packages/cli/src/commands/lambda/policies.test.ts and state.test.ts cover the utilities, never the dispatcher.
That's exactly the shape of hf#910 (the --memory drop). The fix there was a one-line dispatcher edit; the bug was that no test asserted "if the flag is present, the SDK call sees it." A future refactor of runRender could re-introduce the same class of drop on variables and ship green.
Concrete ask: add a runRender.test.ts that stubs loadSDK to capture the renderToLambda opts argument, then asserts:
args.variables = '{"a":1}'→ capturedopts.config.variablesequals{ a: 1 }.args.variablesFile = "vars.json"(with a fake reader injection or fixture) → capturedopts.config.variablesequals the file contents.- Neither flag set → captured
opts.config.variablesisundefined. args.variables = '{"a":1}'andargs.variablesFile = "vars.json"→ command exits non-zero (conflict).
Three of those four are already covered indirectly by variables.test.ts for the parser, but none of them assert the dispatcher actually carries the parsed value into the config. A test that fakes loadSDK + captures the argument is the cheapest reproduction of the hf#910 class of bug.
nit — empty-object --variables '{}' flows through as {} instead of undefined
parseVariablesArg('{}', undefined) returns { ok: true, value: {} }. runRender then sets config.variables = {}. The schema-validation block is skipped (Object.keys(variables).length > 0 is false), but the empty object still rides the wire into Step Functions and reaches the chunk workers as window.__hfVariables = {} instead of undefined. Cheap to fix — collapse empty-object to undefined in parseVariablesArg, or in the wrapper. Mostly a tidiness issue; no failure mode I can construct.
nit — lambda.ts examples use sha-abc for the site-id
packages/cli/src/commands/lambda.ts:32,40 use sha-abc as the site-id placeholder; the docs example uses abc1234deadbeef0. Pick one and stay consistent — sha-abc is shorter and clearer that it's a placeholder.
nit — // fallow-ignore-next-line complexity on runRender
Adding the suppression at the top of runRender (line 60) is fine, but runRender is at the boundary where the variables threading lives — exactly the place where a future reader will want clean cyclomatic complexity to spot a missed forward. If the complexity rule is genuinely too tight here, a structured pre-extracted helper (buildLambdaRenderConfig(args)) gets the line count down without the suppression and gives you a natural test seam for the dispatcher coverage above. Optional refactor; the suppression is acceptable for this PR.
Verdict
Verdict: APPROVE
Reasoning: Threading is correct, validation is layered at the right boundary, helper extraction is real (not cosmetic), and unit tests cover the parsers. The strict-mode-when-no-index case and the absent dispatcher test are real but landable as follow-ups in 9.4+ since neither breaks the happy path.
— Vai
0b1732d to
a4b1d1a
Compare
24c52b0 to
078cf71
Compare
|
Addressed Vai's feedback in the latest force-push:
Skipping for follow-up:
|
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed after force-push. Both items addressed:
-
--strict-variables warns when no schema — Fixed. When
--strict-variablesis set but noindex.htmlexists on disk, a visible warning is now emitted ("schema validation skipped. Variables flow through unchecked"). Suppressed in--jsonmode, consistent with output suppression logic. -
site-id placeholder consistent — Fixed. Both CLI examples and docs now use
abc1234deadbeef0consistently.
No regressions from previously-approved code. CI green. Ship it.
vanceingalls
left a comment
There was a problem hiding this comment.
Re-stamp post-force-push. Per James's per-PR response, both prior importants addressed:
--strict-variableswarns when no schema is resolvable (covers the--site-idsilent-no-op gap).- site-id placeholder consistency between
lambda.tsexamples and docs.
Re-stamp by Vai
… --strict-variables Mirror the local hyperframes render variables UX on the Lambda CLI: - --variables '<json>' inline JSON object of variable values - --variables-file <path> path to a JSON file with variable values - --strict-variables fail on type/declared-mismatch (warn by default) Resolution + validation logic is hoisted to packages/cli/src/utils/variables.ts so both surfaces share one parser. The new reportVariableIssues helper formats the warning block + handles --strict-variables exit, deduping the per-CLI issue-handling block. Variables flow into SerializableDistributedRenderConfig.variables and reach every chunk worker via the path PR 9.1 + 9.2 wired up (plan() → meta/encoder.json → renderChunk() → window.__hfVariables). Pre-validation against the composition's data-composition-variables declaration runs only when the project's index.html is on disk — --site-id pointing at a pre-uploaded site that was packaged elsewhere skips the check, matching how the local CLI treats unreadable index files. The render.ts re-exports of parseVariablesArg / resolveVariablesArg / validateVariablesAgainstProject are dropped; the matching tests move to packages/cli/src/utils/variables.test.ts where the implementations now live. Docs: docs/packages/cli.mdx adds a section on --variables / --variables-file / --strict-variables for lambda render, including the 256 KiB Step Functions execution-input cap and a pointer to the upcoming templates-on-lambda guide (PR 9.5). Phase 9 PR 9.3 of the distributed rendering plan.
078cf71 to
0310fff
Compare
a4b1d1a to
39891fe
Compare
The base branch was changed.
Fallow audit reportFound 25 findings. Dead code (4)
Duplication (10)
Health (11)
Generated by fallow. |
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-approved — same diff, rebased onto main.
vanceingalls
left a comment
There was a problem hiding this comment.
Re-stamp at the new HEAD after rebase onto main. Same content as my prior approve (4323426149); both importants addressed (--strict-variables warns on no-schema, site-id placeholder consistent). Base is now main so main CI workflow actually runs.
Re-stamp by Vai (post-rebase)

What
Phase 9 PR 9.3 of the distributed rendering plan — bring the local
hyperframes rendervariables UX tohyperframes lambda render:--variables '{"title":"Hello"}'--variables-file ./vars.json--strict-variablesSame flag names, same parse-error messages, same strict-mode behavior — users who learned the local flow can drive Lambda renders without re-learning the surface.
Why
PR 9.1 added
variablestoDistributedRenderConfig. PR 9.2 added SDK-level validation + the 256 KiB Step Functions cap. With both plumbing layers in place, the CLI surface is the last piece needed before users can run personalised renders end-to-end. This PR adds the flags + wires them throughrunRender.How
parseVariablesArg,resolveVariablesArg,validateVariablesAgainstProjectfrompackages/cli/src/commands/render.tstopackages/cli/src/utils/variables.tsso both CLIs import the same parser. The local CLI used to re-export these for tests; the tests move topackages/cli/src/utils/variables.test.tsalongside the implementations.reportVariableIssues(issues, { strict, quiet })helper formats the warning block + handles--strict-variablesexit. Replaces the inline duplication that existed between the two CLIs.lambda/render.tsresolves variables, runs pre-validation against<projectDir>/index.htmlwhen present (skipped when the project dir isn't on disk —--site-idpointing at a pre-uploaded site packaged elsewhere is the typical case), and threadsvariablesintoSerializableDistributedRenderConfig.lambda.tsregisters--variables/--variables-file/--strict-variablesflags and forwards them torunRender.docs/packages/cli.mdxadds a section on the new flags, including the 256 KiB Step Functions execution-input cap and a pointer to the upcomingtemplates-on-lambdaguide (PR 9.5).Test plan
bun run --cwd packages/cli test— 335 tests pass (12 variables tests moved fromrender.test.tstovariables.test.ts, still passing).bun run --cwd packages/cli typecheckclean.bunx oxlint+bunx oxfmt --checkclean on changed files.bunx fallow audit --base origin/main—✓ No issues in 19 changed files.npx tsx packages/cli/src/cli.ts lambda --helpshows--variables,--variables-file,--strict-variablesflags.This is part of a stack of 5 (+1 optional) PRs (9.1–9.6); this is PR 9.3.
🤖 Generated with Claude Code