fix(cli): reject blank default composition entries - #3392
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 02307067. First review on the PR — no prior reviews or comments, so nothing here is additive to another reviewer.
Audited: all 9 changed files end to end; packages/lint/src/project.ts and its test file in full at head; every other lintProject consumer (check, lint, preview, publish, validate); commandResult.ts; snapshot's arg surface; templates/blank/index.html + init.ts's substitution; and the reporter's own repro project from #3391.
The diagnosis is right, and I verified it against the reporter's project rather than the commit message
I reimplemented the new predicate exactly as written (linkedom 0.18.12, same selectors) and ran it over bonaAG/hyperframes-black-frame-repro:
root index.html -> blank: true (no element children)
compositions/index.html -> candidate: true (authored timed content)
=> rule fires
So the fix reaches the reported case. Better than that, the provenance closes: the reporter's root is verbatim packages/cli/src/templates/blank/index.html after init.ts:357-362 strips the <video>/<audio> placeholders for a no-video init, with __VIDEO_DURATION__ → the "10" default at init.ts:366. That single fact explains every anomaly in #3391 that looked like a runtime bug:
| #3391 observation | explanation |
|---|---|
10.0s render, getDuration() === 10 |
the scaffold's default data-duration="10" |
__timelines keys ["main", "__proxied"], not bona-brand-card |
the scaffold registers window.__timelines["main"] — it is the root |
"added an end-of-timeline frame at 9.70s" |
same 10s root |
black frames in check/snapshot/render and Studio preview |
all four open the root |
Playwright straight at compositions/index.html works |
that file was never the entry |
Nothing in the issue is left unexplained, which is what licenses the Fixes keyword. Worth noting the trap is manufactured by the tool's own defaults, not by user error: init writes the empty root, the scaffold's hyperframes.json names compositions as paths.blocks, and nothing wires the two together.
Three other things I checked that came back clean:
- The
!entryFilegate is the right seam.project.ts:233skips the rule for an explicit entry, andexecute.ts:179derivesexplicitEntry = plan.entryFile ? plan.renderTarget : undefined, sorender -c compositions/index.htmlreally does stay supported. checkgenuinely fails now.checkPipeline.ts:1126runsshouldBlockRender(true, false, …), so any lint error short-circuits the pipeline andcheckExitCodereturns 1 — the reporter's "check reported success" becomes a failure.preview.ts:420picks the finding up for free, which is the Studio half of the description.- The abort really aborts.
failCommand()isnever(throwsCliRuntimeError), sosnapshot.ts:664-672stops before any capture, and the pre-existingtotalErrors === 0 && totalWarnings === 0early return inrunRenderLintcan't swallow anerror-severity finding.
Important
1. The <template> skip is redundant where the description says it helps, and it creates a false negative. project.ts:272.
The real sub-composition contract is a top-level <template> with no <body> at all — e.g. packages/producer/tests/sub-comp-t0/src/compositions/hook.html. Executed against that shape: document.querySelector("body") is null and body [data-composition-id] matches nothing, because template content is a separate fragment. So the selector on line 274 already excludes template-wrapped sub-compositions; the continue adds nothing for them. Mutation confirms it — deleting the guard leaves all three new lint tests green.
What it does do is skip any candidate file that merely contains a <template> anywhere. A genuine standalone composition with a repeated row or card template hits body [data-composition-id] fine, and the exact bug this PR exists to catch goes unreported there. Fails toward silence rather than a false block, hence important and not a blocker. Either drop the line, or scope it to the composition itself (composition.closest("template")).
2. The two command call sites — the actual fix — have no test. All three added CLI tests assert an exported predicate against a hand-built finding object:
render.test.ts:264→renderLintShouldAbort(false, false, lintResult)snapshot.test.ts:93→snapshotLintShouldAbort(lintResult)lintProject.test.ts:279→hasDefinitiveEntryMismatch(result)
snapshotLintShouldAbort (snapshot.ts:119) is a pure alias for hasDefinitiveEntryMismatch (lintProject.ts:7), so that is three tests of one function under three names, and zero tests of the wiring. Revert runRenderLint to the old bare shouldBlockRender(...) call and delete the snapshot.ts:664 preflight block, and all three still pass — the suite cannot tell the fix from its absence.
Cheap to close, because the abort precedes any browser launch: build a fixture dir the way project.test.ts's makeProject does and drive the command object, the way catalog.test.ts:195 already does with command.run({ args }). One assertion per command that it rejects.
3. publish is the third default-entry consumer and it is ungated. publish.ts:89-95 prints the findings and then walks straight into the confirm prompt; --yes (publish.ts:97) skips the prompt entirely. So a project this PR now blocks locally can still be uploaded, and the hosted player opens the same blank root — which is not hypothetical, the repro in #3391 is a hyperframes.dev/p/... link. hasDefinitiveEntryMismatch is already exported; gating publish on it is the same two lines as snapshot.
4. snapshot has no explicit-entry escape, but it inherits render's fix hint. The finding's fixHint ends with "render it explicitly with --composition compositions/index.html", and snapshot's args (snapshot.ts:597) have no composition flag — so the advice names a flag the command the user just ran does not have. After this PR the reporter can render their composition and cannot snapshot it, other than via hyperframes snapshot compositions, which happens to work (resolveProject only needs an index.html, project.ts:46-57) and which nothing tells them about. Not a regression — snapshot previously produced black frames for these projects — so this is incomplete coverage of the new gate rather than something that broke. One extra line in the snapshot.ts:670 message covers it.
Nits
5. The blankness predicate's :not(...) chain is unpinned. validHtml() (project.test.ts:19-25) uses a completely empty composition div with its scripts as siblings outside it, and so does the reporter's project — so *:not(script):not(style):not(link):not(meta):not(template) (project.ts:264) is inert in every test and in the real repro. Dropping the whole chain keeps the suite green. It matters for a root that keeps a scoped <style> or an inline <script> inside the composition div, which is the case a future simplification to root.children.length === 0 would silently change. One fixture pins it.
Mutation results, for the record — three of the four operators in the new rule are unconstrained by the three tests:
| mutant | suite |
|---|---|
| unmutated control | green |
drop :not(script|style|link|meta|template) |
green |
drop !hasAttribute("data-composition-src") |
green |
drop the <template> skip |
green |
6. Scope worth stating out loud: the rule catches the no-media scaffold and, by design, not the A-roll one. init with a video keeps <video class="clip"> + <audio> inside the root (templates/blank/index.html:40-57), so that root is not blank and the same "authored composition under compositions/ never renders" outcome is not flagged. The output is not black there, so it is a much less silent failure — but a sentence in the rule's comment saves the next reader the trip.
7. snapshot now pays a full project lint on every invocation. Root plus every composition parsed, asset existence checks, and up to N ffprobe spawns when local videos exist (hevcPreviewLint.ts, PROBE_CONCURRENCY = 8, PROBE_TIMEOUT_MS = 4000). Bounded and skipped entirely without ffprobe, so not a problem — just new cost on the command agents loop on fastest, and it is not in the description.
On the closing keyword
Fixes #3391 is defensible: the diagnosis covers every symptom the reporter documented, and their project genuinely has two entries. The thing to be careful about is that the reporter did real diagnostic work and landed on "renderSeek updates internal time but does not repaint" — and an unattended auto-close tells them that hypothesis was what got fixed. A short comment naming the actual mechanism (no-video init scaffold + paths.blocks: compositions, so every default command opened the empty 10s root) before this merges would be worth more to them than the close notification.
CI
All 8 required contexts are green at 02307067 — Semantic PR title, Typecheck, Build, Test, Test: runtime contract, regression, Render on windows-latest, Tests on windows-latest. The three that were still pending when I started drafting (Test and both Windows contexts) have since reported success at this same head, so the green covers the code as written; everything else on the rollup is green or path-skipped. The description's per-package counts (498 lint / 2,805 CLI) are the author's local run rather than CI's, but CI's own Test context agrees.
Verdict: COMMENT
Reasoning: The root-cause analysis is correct and I confirmed it independently against the reporter's own project, the gate is scoped correctly for explicit entries, check now fails where it previously passed, and all 8 required contexts are green — so nothing here is CI-driven. Holding short of approve only on the three items I would like in the same push: the <template> skip that is redundant for the shape it names and over-broad for everything else, the untested command wiring (revert either call site and the suite stays green), and the ungated publish path. None is a blocker; say the word if you would rather land this and do them as a follow-up, and I will stamp it as-is. I have not merged anything.
— Rames Jusso
Addressed at
Mutation-checked all three command gates: reverting render, snapshot, or publish independently turns its command-level regression red. Full validation is green: lint 500/500, CLI 2,806 passed / 3 skipped, both typechecks, Studio + CLI builds, formatting/lint, and pre-commit hooks. I also posted the actual two-entry root cause on #3391 before merge. |
jrusso1020
left a comment
There was a problem hiding this comment.
Post-merge advisory review — this squash-merged into main as a9ea07ed while I was writing it up. Findings below are follow-ups, not gating. I had finished the verification against e8331681, which is exactly the tree that got squashed, so all of the evidence here applies to what landed.
Additive to my earlier review — all four items from it are addressed, and I re-ran the checks rather than reading the diff, because the whole point of item 2 was that a test can exist without pinning anything.
Ran locally at this head: @hyperframes/lint 500/500, @hyperframes/cli 2,806 passed / 3 skipped — matching the test-plan numbers exactly. (My first pass at the CLI suite showed 13 red in init.test.ts + cloud/render.test.ts; that was my environment, not this branch — those tests spawnSync("bun", …) and I had bun off PATH, so they returned status -1. With bun on PATH, clean.)
Mutation table — executed, one revert at a time, suite re-run each time
| Reverted | Test file | Result |
|---|---|---|
hasDefinitiveEntryMismatch(lintResult) || dropped from renderLintShouldAbort (render/execute.ts:54) |
render.test.ts |
RED — 1 failed / 73, and it is "aborts the real render lint preflight on a default-entry mismatch without --strict" |
snapshot preflight neutered (snapshot.ts:656) |
snapshot.test.ts |
RED — 1 / 32, "rejects the real fixture before invoking browser capture" |
publish gate neutered (publish.ts:95) |
publish.test.ts |
RED — 1 / 6, "rejects the real fixture before creating or uploading an archive" |
if (document.querySelector("template")) continue restored (project.ts:275) |
project.test.ts |
RED — 1 / 30, "still catches a standalone composition that contains an unrelated nested template" |
:not(...) chain replaced with bare * (project.ts:267) |
project.test.ts |
RED — 1 / 30, "treats non-rendering script, style, link, meta, and template children as blank" |
!element.hasAttribute("data-composition-src") dropped (project.ts:280) |
project.test.ts |
GREEN — see nit 4 |
Each revert killed exactly one test, and the right one. That is the difference from the first pass, where all three CLI tests survived deleting both call-site gates. Two things I like about how it was done: the snapshot and publish tests build a real temp fixture and run the real lintProject, so they cover the whole path rather than a hand-built finding object; and deleting snapshotLintShouldAbort outright removed the alias that made three tests look like three tests when they were one function under three names.
False-positive sweep, redone with the real linter. The template skip is gone, so the rule is strictly broader than what I swept last time. I ran the built lintProject over every in-repo project that has both an index.html and compositions/**/*.html — 43 of them — and the finding fires on 0. (Last pass I swept a reimplementation of the predicate; this one is the shipped code.) The reason it stays quiet is worth stating, since it is what bounds the blast radius: a root that mounts anything has a data-composition-src element as a rendered descendant, so it never reads as blank in the first place.
packages/lint/src/project.ts:234 still gates the rule on !entryFile, so render --composition … remains a real escape.
Worth a follow-up — one claim, not one bug
The Studio half of "check and Studio surface the exact entrypoint mismatch" does not hold. packages/studio-server/src/routes/lint.ts walks the project's HTML files and calls adapter.lint(content, { filePath }) once per file, and the CLI supplies that adapter as lintHyperframeHtml (packages/cli/src/server/studioServer.ts:429) — the single-file linter. Every project-level finding is therefore structurally unreachable from Studio's LintModal: not just blank_root_with_standalone_composition but its siblings multiple_root_compositions, missing_or_empty_sub_composition, and audio_file_without_element. Nothing in the diff changes that, and I could not find any other path from lintProject into a Studio surface.
Two consequences, and I'd separate them: the sentence in the description should lose "and Studio", and a follow-up that routes project lint through the Studio lint route is worth having on its own — #3391's title leads with the Studio symptom, so Studio is precisely where the next person hits this and gets no diagnosis. Not a blocker for this PR; the CLI surfaces are the ones that were silently shipping black output.
Small follow-ups
publishaborts with no escape, and the hint above it names a flagpublishdoes not accept.publish.ts:95-99prints the one-line abort and nothing else; the finding'sfixHintprinted just above ends with "render it explicitly with--composition compositions/index.html", and publish's args aredir/yes/public/update/space/proxy. Snapshot got exactly the parallel one-liner in this push — the same treatment for publish (hyperframes publish <project>/compositions) closes it.- Snapshot's new hint only resolves when the candidate happens to be
compositions/index.html. The string is fixed, andresolveProjectrequires anindex.htmlin the target directory (packages/cli/src/utils/project.ts:51), so for a candidate likecompositions/card.htmlthe suggested command exits with "No composition found in …". The rule already computesstandaloneCandidates[0]; deriving the directory from it — or only printing the hint when the candidate's basename isindex.html— makes the advice true in every case it is printed. - The finding's command list is now one short. "Default check, snapshot, preview, and render commands open index.html" —
publishblocks on this too as of this push. - One operator still unpinned (last row of the table). If
!element.hasAttribute("data-composition-src")ever regresses, acompositions/*.htmlthat only mounts other sub-compositions counts as a standalone candidate. One fixture — a compositions file whose only timed child carriesdata-composition-src— closes the last hole in an otherwise well-pinned rule.
CI at the merge point
main requires 8 contexts. At e8331681, six were green from the fresh runs (Semantic PR title, Typecheck, Build, Test: runtime contract, regression, Render on windows-latest). The admin merge landed with the other two — Test and Tests on windows-latest — still in progress, so neither ever reported at that head. Stating what that does and does not leave open:
Testis the job that runs these unit suites, and I ran both of them locally on that exact tree:@hyperframes/lint500/500 and@hyperframes/cli2,806 passed / 3 skipped. So the content of that gate is independently covered.Tests on windows-latestis the one I cannot stand in for.Render on windows-latestwas green, so the Windows toolchain itself was fine at that head; the unit/aggregate job is the residual unverified gate.- Two of this push's three new tests write real temp fixtures via
mkdtempSyncand clean up in afinally— that is the shape most likely to behave differently on Windows, which is the specific reason I would want that job's result rather than infer it.
main's own post-merge run on a9ea07ed is the place that closes this out: as of writing, regression is green and Build, Typecheck, Render on windows-latest, and Tests on windows-latest are still running, with Test not yet reported. Worth a glance when it settles.
The red Test that was visible on the PR beforehand was not signal, and the read of it in the thread was right: a 4-second job from run 32494234208, which was cancelled wholesale when the description edit re-triggered CI. In a cancelled run the producer gate trips before checkout and manufactures that red, and the check-run name stays on the commit afterward, so it never clears itself.
Verdict: COMMENT (post-merge: Ready, with follow-ups)
Reasoning: All four items from my first pass are fixed, and the fixes are now held down by tests that die when the fix is reverted — verified by running the reverts, not by reading the diff. Nothing here would have blocked the merge. The follow-ups worth carrying forward are the Studio project-lint gap (the surface #3391 leads with still gives no diagnosis), the two hint-accuracy items on publish and snapshot, and one unpinned operator. I did not merge this and cast no approval on it.
— Rames Jusso
What
Default project surfaces now reject projects whose top-level
index.htmlis an empty scaffold while an authored standalone composition lives undercompositions/.checksurfaces the exact entrypoint mismatch;snapshot, defaultrender, andpublishstop before producing or uploading background-only output. Explicitrender --composition compositions/...remains supported, and snapshot names the equivalent directory-target command.Fixes #3391.
Why
The #3391 reproduction looked like a GSAP seek failure, but the downloaded project contained two entries: an empty 10-second
index.htmland the real 5-second BONA animation incompositions/index.html. Every default command correctly selected the empty master, then reported success because that file was structurally valid. The result was silent black snapshots, a black 10-second render, a blank Studio canvas with the wrongmaintimeline, and a published link that opened the same blank root.How
The project linter reports
blank_root_with_standalone_compositiononly when the default root has no renderable descendants and an unwrapped, timed standalone composition exists undercompositions/. Authored masters and template-wrapped sub-compositions are excluded; a standalone composition may still contain unrelated nested templates without escaping the gate. A no-media blank scaffold is covered deliberately, while an A-roll scaffold remains visibly non-blank and is left alone.The definitive mismatch is always blocking for default render, snapshot, and publish, independent of
--strictor--yes. Snapshot runs the full project lint before capture so it shares the same diagnosis as the other surfaces; that includes bounded local-video codec probes when applicable. The finding explains how to move/mount the authored composition or render the separate entry explicitly, and snapshot adds its ownhyperframes snapshot <project>/compositionsguidance.Test plan
@hyperframes/lint: 500 passed@hyperframes/cli: 2,806 passed, 3 skippedcheck,snapshot, defaultrender, andpublish --yesstop with the new findingrender -c compositions/index.htmlstill produces the visible 5.0-second BONA animation