fix(lint): root_dimensions_mismatch sub-composition false positive + regex SSOT consolidation - #4012
Conversation
Sub-compositions mounted via data-composition-src are fetched with DOMParser and only their matched <template>/<body> subtree is imported into the live document. A sub-composition authored as a full standalone HTML document still carries its own <html>/<head>/<meta viewport>, but none of that ever reaches the rendering document, so a mismatch there was always a false positive. Guard the rule with options.isSubComposition, matching the sibling missing_timeline_registry rule's existing pattern. Split the warning wording: html/body CSS actually sizes the element that clips the root, but the CDP viewport is set from the root's own data-width/data-height (frameCapture.ts), never from <meta viewport>. A viewport-only mismatch is real drift but not a clipping risk. Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com>
…finition applyResolutionPreset kept its own prefix-capturing regex literals for the html/body CSS size and meta viewport content, duplicating the same three patterns already defined in @hyperframes/parsers for lint's read-only root_dimensions_mismatch comparison. Merge to one definition per pattern that captures both the prefix (for in-place replace) and the digits (for comparison), so the two callers can no longer drift on where a scaffold's resolution lives. Existing applyResolutionPreset tests stay green. Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com>
miga-heygen
left a comment
There was a problem hiding this comment.
Full pass at 85ceede (simplify + single-source-of-truth + adversarial). Both #4005 follow-ups land as described; no blockers.
Strengths
packages/lint/src/rules/core.ts:377— the guard readsoptions.isSubCompositionexactly likemissing_timeline_registry, andpackages/lint/src/project.ts:219is the one place that sets it, solint/check/publish/renderall inherit it throughlintProject.packages/parsers/src/canvasScaffoldPatterns.ts:8-16— one definition per location with prefix groups 1/3 and digit groups 2/4;canvasScaffoldPatterns.test.tspins the group layout for both callers, so lint andapplyResolutionPresetcan no longer renumber against each other.core.ts:275-311— the wording split keepscodeandseverityunchanged, so nothing keying onroot_dimensions_mismatchmoves.
Verified
- Exposure claim holds:
commands/lint.ts:91exits non-zero on errors only;publish.ts:93-99prints warnings and aborts only on a definitive entry mismatch;check.ts:191fails warnings only under--strict;render/execute.ts:59blocks warnings only under--strict-all. - Replacement semantics for every preset are unchanged: probed the old literals against the shared regexes on the width-first, height-first and viewport shapes, including a file with two
html, body {}blocks (still first match only, both before and after).init.test.ts27/27 on the head. - Top-level path still reports:
core.test.ts:199lints the sub-composition shape without the flag and asserts the finding. RED check — withcore.tsfrom main, 6/85 fail (the 3 pre-existing rule tests plus the sub-composition guard test, the top-level wording test and the viewport-only wording test); withcanvasScaffoldPatterns.tsfrom main, 6/6 fail. Head: lint 85/85, parsers 6/6, cli 27/27. - Runtime justification checked at
packages/core/src/runtime/compositionLoader.ts: the fetched document's<head>/<meta>and html/body CSS are discarded when the template or body subtree is mounted.
Nits
- Test plan says the guard is covered by "both a synthetic fixture and the real
hf2550flowchart fixture". No test reads that fixture;core.test.ts:180and:213only reference it in comments over synthetic HTML of the same shape. Either loadpackages/producer/tests/hf2550-video-subcomposition-ghost/src/compositions/flowchart-vertical-4973fa42.htmlin a test or drop the claim. - "Known follow-up" overstates the studio gap.
packages/studio-server/src/routes/lint.ts:9-24runsadapter.lintProjectfirst and marks its files covered, socompositions/*.htmlreach the studio endpoint with the guard;lintUncoveredHtml(:27) only lints HTML files outside project-lint coverage. The producer endpoint (producer/src/server.ts:698) lints the project entry only and returns JSON. Both are informational as the body says; the description of what is unguarded is narrower than written. VIEWPORT_META_SIZE_REreplace now preserves the author's separator (width=1920,height=1080becomeswidth=2160,height=3840), where the oldinit.tsliteral normalised it to,. Every shipped template and the init tests use,, so no preset changes; noting the behaviour so nobody reads the diff as byte-identical.
Verdict: APPROVE (posted as a comment: GitHub refuses an approval from the account that authored the PR, so the stamp is Miguel's)
Reasoning: Both #4005 findings are closed at the executing code with the guard carried through the only sub-composition lint path, the shared regexes are proven group-compatible for both callers, and the remaining items are PR-description accuracy.
— Miga
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
:large_green_circle: 85ceede8 APPROVED
Follow-up to #4005. Two independent fixes, both tightly-scoped and well-tested. Clean R1.
What the change is, verified end-to-end
1. Sub-composition guard for root_dimensions_mismatch (packages/lint/src/rules/core.ts)
The rule now bails when options.isSubComposition is true. Verified the mechanism the PR body claims — loadExternalCompositions (packages/core/src/runtime/compositionLoader.ts) mounts either the matched template.content or doc.body into the host (const sourceNode = template ? template.content : doc.body;), never the sub-comp's own <html> / <head> / <meta viewport>. So a sub-comp's own scaffold viewport is genuinely unobservable at capture time, and warning on it is a false positive. The guard is threaded from the only caller that has the concept: lintProject in packages/lint/src/project.ts passes isSubComposition: true when walking the compositions directory. The two other lint entry points (lintUncoveredHtml, runHyperframeLint) don't — but the PR body correctly scopes those out as informational surfaces, not --strict gates. Matches the same pattern the sibling missing_timeline_registry rule uses in the same file.
2. Message-wording split via describeRootDimensionsDrift
Body-CSS mismatch keeps the original "scaffolded body clips" wording (the real capture-time hazard); viewport-meta-only mismatch gets new "no effect on capture — stale metadata, not a clipping risk" wording. Correct: both page.setViewport({ width, height, ... }) sites in packages/engine/src/services/frameCapture.ts are driven from session.options.width/height, not any parse of the composition's own <meta viewport>, so the old "clips" copy was misleading for the viewport-only case. Severity stays warning in both — no strict-mode gate change.
3. Regex SSOT consolidation (packages/parsers/src/canvasScaffoldPatterns.ts + packages/cli/src/commands/init.ts)
The three patterns are now published with prefix-capturing shape (groups 1/3 = surrounding text, 2/4 = digits), and both consumers agree on the group contract:
applyResolutionPresetuses$1<new>$3<new>for in-place substitution.readHtmlBodyCssSize/readViewportMetaSizeincore.tsdestructure[, , width, , height].
The new canvasScaffoldPatterns.test.ts pins both the group layout (for lint's read) and the replace behavior (for CLI's write), so a future renumber breaks CI in one place.
4. Test coverage
Both sides of the invariant are pinned in packages/lint/src/rules/core.test.ts:
- suppression: two tests — synthetic fixture + the actual
hf2550flowchart shape — assert no finding underisSubComposition: true - non-suppression: the same shape at top-level still produces a finding (belt-and-suspenders against a future refactor turning the guard into an unconditional bail)
- wording split: viewport-only drift asserts
contains "no effect on capture"andnot "clips"; html/body drift assertscontains "clips"
Concerns — none.
Nits — none worth calling out.
What I didn't verify
- The
hf2550-video-subcomposition-ghostfixture wasn't executed locally under the new lint path — trusting the PR body's "verified against the real fixture" plus the passing CI matrix (all 7 required checks green, including preview-regression and Windows render).
— Review by Rames D Jusso
- Lint no longer reports root_dimensions_mismatch for sub-compositions, the false positive v0.8.43 shipped with; a top-level mismatch is still reported and a viewport-meta-only mismatch gets its own wording (#4012) - The streaming encoder accepts lockGopForChunkConcat and gopSize, emitting the same arguments as the disk encoder. Off by default, so an existing render produces an unchanged FFmpeg arg list (#3973) Co-authored-by: Miguel Ángel <miguel.sierra@heygen.com>
main merged #4012 while this branch was in flight, consolidating HTML_BODY_CSS_WIDTH_FIRST_RE/HEIGHT_FIRST_RE from 2 capture groups (digits only) to 4 (prefix text, digits, suffix text, digits) so cli's applyResolutionPreset could reuse the same regex for its in-place replace. detectRootBodySizeMismatch still read groups 1/2 as the digits, now the prefix/suffix text, so Number() on them was NaN and every render's rootBodyMismatch/rootBodyDeltaPxBucket silently came back undefined. Fixed to read groups 2/4, matching lint's core.ts readHtmlBodyCssSize exactly. Removes the temporary diagnostic test (ca4e739, e2e384c) now that its CI log gave the real answer: both a static and dynamic import of the regex resolved to the same file, and that file's own content had 4 groups because this branch's merge-base predates #4012, not because of any resolution or caching difference. A fresh CI run confirmed no CI-only failure remains after merging main and fixing the indices. Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com>
… properties (#4017) * feat(telemetry): add output-shape request facts to render events output_resolution_preset, output_format, hdr_mode, video_frame_format, and gif_fps_capped join render_complete/render_error, all threaded from already-resolved RenderOptions/RenderPlan fields rather than recomputed: createRenderPlan already computes gifFpsCapped (clamping a requested --fps above 30 to 30 for --format gif) but only used it for a console warning; this threads that same decision into RenderOptions and telemetry. All five are known before the pipeline starts, so both events carry them even when a render fails before a perfSummary exists. Also wires output_format/output_resolution_preset into Studio's separate emitStudioRenderComplete/emitStudioRenderError path, since Studio's request genuinely varies those two per-request. hdr_mode, video_frame_format, and gif_fps_capped stay CLI-only: Studio exposes no HDR-mode, video-frame-format, or GIF output control to users at all, so there is no per-request value to report for those three. Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com> * feat(telemetry): add adapters_used to render events adapters_used lists the runtime deterministic adapters (packages/core/src/runtime/adapters/) a composition exercises. Investigated the actual codebase rather than the ticket's guessed 7-adapter list and found 12 real registered adapters (gsap, css, waapi, animejs, lottie, three, typegpu, d3, leaflet, mapbox, maplibre, google-maps) — the map/data-source adapters share the exact same window.__hf<Name> registration mechanism and are equally real "adapters used" facts, so all 12 are included. Detection unions a live page.evaluate probe (piggybacked on the same probeSession resolveCompositionElementCount already uses) with a static regex fallback over the compiled HTML, since this is purely observational with no gating role (unlike compositionElementCount) — union strictly increases recall rather than needing one source of truth. Threaded through the same plumbing chain as the already-merged composition_element_tags/aroll_video_count/heygen_video_count. Studio's emitStudioRenderComplete/emitStudioRenderError get this field automatically, with no Studio-side code change, via the same shared capture-observability spread heygen_video_count already uses. Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com> * feat(telemetry): add audio/image/sub-composition/color-grading counts audio_count, image_count, sub_composition_count, audio_group_count, color_grading_count, and has_lut join composition_element_tags in the same single scanElementTags pass — audio/image/audio-group counts read from the scan's uncapped tag map so a 50+-distinct-tag composition can't push a named count into the "other" bucket. sub_composition_count counts data-composition-src mounts directly rather than reusing collectSubCompositionSrcs, which dedupes by src and drops placeholder/remote mounts (a different, smaller fact than the mount count this field reports). has_lut decodes the HTML-entity-escaped form of data-color-grading before parsing, since linkedom's serializer re-emits it "-escaped on every compile-pipeline round-trip — without the decode this silently reported false for every LUT-graded composition. The LUT-presence check now mirrors normalizeLut's own definition of "has a LUT" exactly, closing a gap where a blank or empty lut value would have reported true. Threaded through the same plumbing chain as the already-merged per-tag/provenance/adapter fields; Studio's emitter picks these up automatically via the shared capture-observability spread. Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com> * feat(telemetry): add browser_version_major and ffmpeg_version_major Both are already-run subprocess calls whose output was discarded: readToolVersion (checkFFmpeg's preflight) already runs `ffmpeg -version`, and chromeLaunchOutcome already runs `--version` as a Chrome launch sanity probe on every render. A shared extractMajorVersion helper pulls the first X.Y-shaped number from either banner, threaded onto EnvironmentCheckOutcome/ EnvironmentCheckResult, then RenderOptions right after runEnvironmentChecks resolves in the local render path only. Docker renders never populate these two RenderOptions fields (the containerized producer runs its own preflight the host CLI never sees), so both properties come back absent there rather than reporting the host's own toolchain versions for a render that didn't use them. Studio does not call runEnvironmentChecks or this ffmpeg/Chrome preflight at all before creating a render job, so its render_complete/ render_error events will not carry these two properties unless Studio's own startup path is separately instrumented — a genuine parity gap, not an oversight in this change. Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com> * feat(telemetry): add authoring_skill_source and authoring_skill_invalid Both derive from resolution logic createRenderPlan already runs: authoring_skill_source names whichever step actually won attribution (an explicit --skill flag, or the skill persisted in hyperframes.json) using the same flagSkill/projectConfigSkill values already computed for authoringSkill itself, not a second lookup. authoring_skill_invalid surfaces the raw --skill value when it fails slug-shape validation (normalizeSkillSlug rejects malformed input, not unregistered skill names) — this value already exists as RenderPlan.invalidAuthoringSkill and already drives a CLI warning in present.ts; this change threads the same field into telemetry rather than adding a parallel one. Threaded through both RenderOptions builders in execute.ts (single and batch render) and all three telemetry call sites in render.ts, including the Docker path — skill attribution is a request-level fact independent of which backend executes the render. Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com> * feat(telemetry): add root_body_mismatch and root_body_delta_px_bucket Measures the composition root's authored data-width/data-height against the scaffold's own html/body CSS size, straight from the compiled document string scanElementTags already scans — the source-level fact, not runtime DOM state. init.ts's applyCompositionSizing (the #4001 fix) only patches the live DOM after the browser loads this same HTML, so this measurement stays meaningful whether or not that fix ran for a given render. Reuses @hyperframes/parsers' canvasScaffoldPatterns regexes (the same ones packages/lint's root_dimensions_mismatch rule reads) for the CSS side. Root-tag detection here is a lighter first-data-composition-id-match heuristic, not lint's more thorough findRootTag (which also handles a leading decorative <svg> defs block) — that helper isn't part of @hyperframes/lint's public API, and expanding it is out of scope here. The delta is bucketed (0 / 1-10 / 11-50 / 51+ px) rather than reported raw, and both fields come back absent — not a false/"0" default — when the root or the CSS block can't be read at all. Threaded through the same plumbing as every other static-scan field this week, including the live-capture-observability fallback used on a crash that never reaches perfSummary. Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com> * feat(telemetry): add hf_env_overrides (names only, capped at 20) Reports the sorted names of HF_*/HYPERFRAMES_* env vars present when createRenderPlan resolves — never their values, since several of these carry filesystem paths. Snapshotting at plan-resolution time (the very first line of the render command, before any pipeline code runs) matters: this render's own preflight later injects HYPERFRAMES_FFMPEG_PATH/HYPERFRAMES_FFPROBE_PATH into its own process.env, which would otherwise make those two always read back as "operator overrides" whether or not the operator ever set them. Reports an empty array, never an absent key, when nothing is set, matching every other capped/bucketed field added this week. Capped at 20 names, documented alongside the cap constant. Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com> * fix(cli): stop clobbering studio render_complete fields, fix an env leak trackRenderComplete's explicit audio_count/image_count/root_body_mismatch/ adapters_used/etc. keys, sourced from perfSummary.drawElement and only ever populated on the CLI's own render.ts path, always won the object spread over the observability-capture fallback those same keys share with trackRenderError, even when the direct value was undefined. studioRenderTelemetry.ts's emitStudioRenderComplete never populates drawElement, only the capture fallback, so 24 fields silently came back missing from every studio-triggered render_complete event despite Studio having computed and sent the capture value. Each field now falls back to its captureXxx counterpart via a new directOrCapture() helper when the direct field is absent, with regression tests proving both the fallback and that the direct value still wins when both are present. Also excludes HF_SHADER_WORKER_ENTRY from hf_env_overrides: cli.ts sets this on every invocation to point the bundled shader worker pool at its own file, sharing the HF_ prefix by coincidence rather than because an operator set it. Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com> * style: remove em and en dashes from added code comments No behavior change. Rewrites 22 comment lines this PR's own diff introduced, using a comma or a period where the dash separated two clauses. Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com> * test(producer): add a temporary diagnostic for a CI-only rootBodyMismatch failure Not a real fix. The rootBodyMismatch test group fails deterministically in CI (confirmed on two runs of the same head) but will not reproduce locally by any method tried, including an isolated file run, the exact combined producer:test:unit command, a worker-count-constrained run, and a fresh clone with a clean install and build matching CI's own steps. This dumps the intermediate regex-match state for the simplest failing fixture to stderr so the next CI run's log says what actually differs. To be removed once that answer lands. Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com> * test(producer): widen the rootBodyMismatch diagnostic to compare import paths Still not a real fix. The previous diagnostic (ca4e739) proved the imported HTML_BODY_CSS_WIDTH_FIRST_RE has 4 capture groups in the failing CI run instead of the 2 the source and production code expect, but it used a dynamic import(), which can resolve through a different codepath than renderOrchestrator.ts's static import. This adds a static import of the same binding, a require.resolve() of the resolved file path, and a direct read of that file's own regex line, so the next run can rule in or out a static/dynamic resolution split versus a genuinely different file on disk. Locally both imports agree (2 groups, same object, correct file). To be removed once the CI answer lands. Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com> * fix(producer): read the merged 4-group scaffold-size regex correctly main merged #4012 while this branch was in flight, consolidating HTML_BODY_CSS_WIDTH_FIRST_RE/HEIGHT_FIRST_RE from 2 capture groups (digits only) to 4 (prefix text, digits, suffix text, digits) so cli's applyResolutionPreset could reuse the same regex for its in-place replace. detectRootBodySizeMismatch still read groups 1/2 as the digits, now the prefix/suffix text, so Number() on them was NaN and every render's rootBodyMismatch/rootBodyDeltaPxBucket silently came back undefined. Fixed to read groups 2/4, matching lint's core.ts readHtmlBodyCssSize exactly. Removes the temporary diagnostic test (ca4e739, e2e384c) now that its CI log gave the real answer: both a static and dynamic import of the regex resolved to the same file, and that file's own content had 4 groups because this branch's merge-base predates #4012, not because of any resolution or caching difference. A fresh CI run confirmed no CI-only failure remains after merging main and fixing the indices. Co-Authored-By: Miguel Ángel <miguel.sierra@heygen.com> * chore: nudge CI (no workflow run triggered on previous push) * chore: nudge CI again now that #4017 is out of draft --------- Co-authored-by: Miguel Ángel <miguel.sierra@heygen.com>
Summary
Post-merge follow-up to #4005 (portrait-composition body-clip fix), addressing two issues found in review:
root_dimensions_mismatchfired for sub-compositions even though a sub-composition's own<meta viewport>/html/body CSS never reaches the live rendering document — the runtime loader (loadExternalCompositionsinpackages/core/src/runtime/compositionLoader.ts) fetches a sub-composition's HTML withDOMParserand mounts only the matched<template>/<body>subtree into the live document. Added anoptions.isSubCompositionguard (matching the siblingmissing_timeline_registryrule's existing pattern), and split the warning's wording: a viewport-meta-only mismatch is real drift but not a clipping risk (the CDP viewport is set from the root's owndata-width/data-heightinframeCapture.ts, never from<meta viewport>), while an actual html/body CSS mismatch still clips and keeps the original wording.packages/cli/src/commands/init.ts'sapplyResolutionPresetkept its own local prefix-capturing regex literals for the same three scaffold locations (html, body { width; height }CSS and<meta viewport>content) thatpackages/parsers/src/canvasScaffoldPatterns.tsalready defines read-only for lint. Merged to one definition per pattern that captures both the prefix (groups 1/3, forapplyResolutionPreset's in-place replace) and the digits (groups 2/4, for lint's read-only comparison), so the two callers can no longer independently drift on what "the scaffold's resolution" means.Exposure: 0/434 registry+skill items (408 registry blocks/components/examples + 26 skills — none live inside a project's
compositions/directory, solintProject's sub-composition path never reaches them), 1/249 in-repo fixtures (hf2550-video-subcomposition-ghost's flowchart-vertical sub-composition, viewport meta 1440x2560 vs. a 1080x1920 root) affected. Only gates exit undercheck --strict/render --strict-all.Verified against the real fixture: linting
flowchart-vertical-4973fa42.htmlwithisSubComposition: trueno longer reportsroot_dimensions_mismatch(previously did); linting the same file top-level still reports it, so genuine top-level mismatches are not silently suppressed.Known follow-up (not in scope for this PR)
The
isSubCompositionguard only protects callers that thread the option throughLintContext.options— whichpackages/lint/src/project.ts'slintProject(thehyperframes lint/checkCLI path) does correctly forcompositions/*.html. Two other lint entry points do not carry any sub-composition concept at all:packages/studio-server/src/routes/lint.ts'slintUncoveredHtml(studio's live/projects/:id/lintendpoint) andpackages/producer/src/services/hyperframeLint.ts'srunHyperframeLint(a generic single-HTML-string lint API). Both are informational/editor-time surfaces, not thecheck --strict/render --strict-allgates this PR's exposure claim covers, and closing the gap requires a product decision (should these APIs accept an explicit flag, or infer sub-composition-ness from a path convention?) beyond this narrow bugfix.Optional PSNR fixture — skipped
Did not add a producer-level regression fixture (
meta.json+ goldenoutput.mp4) for the original render-side clip fix from #4005. No existing script generates a new golden render; doing so would require running the full headless-Chrome+FFmpeg render pipeline by hand and committing the resulting binary as ground truth (this repo'sno-committed-evidencepre-commit hook specifically gates committed media without an explicit escape hatch). That's meaningful new golden-frame generation infrastructure, not a cheap addition — skipped per the task's own guidance not to let this block issues 1/2. #4005's render-side fix already has attribute-level regression coverage; this PR doesn't touch or reduce that.Test plan
packages/lint/src/rules/core.test.ts: 85/85 passing, including 4 new tests — sub-composition guard suppresses the false positive (both a synthetic fixture and the realhf2550flowchart fixture), a top-level lint of the same shape still reports it, and the message-wording split (viewport-only vs. html/body CSS mismatch) is asserted for both the "absent" and "present-but-matching" html/body CSS cases.packages/parsers/src/canvasScaffoldPatterns.test.ts(new): 6/6 passing — locks in the shared regex group contract (1/3 = prefix, 2/4 = digits) for both callers, and that a replace leaves the surrounding text untouched.packages/cli/src/commands/init.test.ts: 27/27 passing (full suite) — confirms the SSOT consolidation didn't changeapplyResolutionPreset's existing width-first/height-first CSS ordering, viewport meta, or no-op-on-no-fingerprint behavior.tsc --noEmitclean inpackages/lint,packages/parsers,packages/cli.