fix(skills,lint): correct composition-contract claims the code contradicts - #3468
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
The runtime has absorbed a series of authoring mistakes over time (array `__timelines`, a missing root `data-start`, a mistyped timeline key, unnested child timelines), and `init.ts` says so in its own comments. The skills kept teaching the old rules, so agents were told to satisfy constraints that no longer exist and, in four cases, were told to do things the gates now reject. Four instructions actively cost an agent a failing run: - `variables-and-media.md` said to add `crossorigin="anonymous"` for canvas capture. `media_crossorigin_breaks_preview` rejects it unconditionally, with no suppression, even for the readback case its own fixHint mentions. - `determinism-rules.md` banned building a timeline inside an async callback. `gsap_timeline_registered_before_async_build` calls that build the documented contract; what it rejects is registering the key before the build finishes. - `determinism-rules.md` banned `gsap.set()` on later-scene clips as "not in the DOM at page load". They are in the DOM, and two lint fixHints instruct exactly that set, so following the rule left two findings unfixable. - `creator-editing-recipes.md` shipped 12 copyable `<audio>`/`<video>` snippets with no `id`. `media_missing_id` is an error, and an id-less `<audio>` is never picked up by the mixer, so a copied recipe renders silent. Corrected elsewhere, in every place each claim appeared: - `data-track-index` is a Studio display lane, not a timing constraint. The render never reads it, no rule forbids same-track overlap, and the allocation ritual around it is removed. 11 sites. - `class="clip"` is a layout and tooling convention. Visibility keys on `data-start`. The class stays recommended, because the shared `.clip` rule is what gives a scene its box. - Timed elements may nest. The "must be DIRECT children of the root" rule was false; what is true is that only root children with `data-start` get automatic layout, which is why the `full-screen-motion.md` background snippet rendered 0px tall. That snippet now carries the CSS it always needed. - The visibility window is half-open, `[start, start + duration)`, pinned by `init.test.ts`. It was documented as inclusive, so an animation landing exactly on `data-duration` never rendered its last frame. - The relative-start grammar has four silent failure modes, all resolving to 0, none checked by lint. `intro-0.5` without spaces parses as an id, references resolve document-wide, cycles resolve to 0, and an unresolvable target duration lands on the target's start rather than its end. - Sub-composition host `data-width`/`data-height` are backfilled by the compiler, and a host id that differs from the file's is supported. Both were "Required". - The root background is stripped only on the layered-composite path (HDR content or shader transitions), not on every render. - Sub-composition Pitfall 3 kept a symptom fixed by #1886 and pinned by `packages/producer/tests/sub-comp-class-selector/`. The lint rule is still the reason to use `#root`; the render is not. - Cross-file duplicate media ids no longer render blank for `src`-bearing media, which the compiler stamps with `data-hf-render-id`. Media using `<source>` children is not stamped, so that case is called out instead. The three workflow `assemble-index.mjs` and `transitions.mjs` copies attributed their same-track lane rule to `timeline_track_too_dense`, which counts elements per lane and never compares windows. The checks are kept, since each workflow's frame injector assumes one clip per lane, but they now say it is their own convention rather than a framework rule. The scaffolded project instructions written into every `hyperframes init` project carried the same two false lines and are corrected in lockstep. `coreSkillContent.test.ts` pinned the literal sentence that listed root `data-start` as required, so every correction here would have reddened CI to protect a false claim. It now pins structure plus the regression it genuinely catches.
…ostics
Follow-on to the skills correction in this branch, closing the items it had
listed as not covered. Each behaviour change here is backed by a render, not
by reading the code.
Human-facing docs. `packages/cli/src/docs/data-attributes.md` (served by
`hyperframes docs`), `docs/reference/html-schema.mdx` and
`docs/concepts/data-attributes.mdx` carried the same three claims the skills
just dropped: `data-track-index` required and preventing overlap, `class="clip"`
required for visibility, and `data-volume` topping out at `1` when the shared
ceiling is `3.98` (+12 dB, `packages/core/src/audioGain.ts`).
`timeline_registry_missing_init` deleted. It demanded
`window.__timelines = window.__timelines || {}` before any assignment, but
`runtime/entry.ts` creates the registry at script-evaluation time, ahead of
every inline composition script. Rendered a composition whose only registration
is a bare `window.__timelines["main"] = gsap.timeline(...)`: it renders and
animates correctly. The rule failed a working file, and since a lint error also
suppresses the layout and contrast audits in `check`, it cost more than the line
it asked for. Its two tests now assert the opposite. The skill's skeleton drops
the guard line and still passes `check` with zero findings.
`timed_element_missing_clip_class` is deliberately kept. It is already a warning
and its own comment already states that the runtime keys off `data-start`, so it
is an honest convention nudge rather than a false claim.
`video_nested_in_timed_element` kept, message corrected. The audit this branch
came from suggested deleting it. A render says otherwise. Three variants in one
rendered MP4, compared against a counter burned into the source:
- video as a direct child of the root: correct.
- video nested in a timed wrapper, start in the wrapper's timebase: correct.
- video nested in a timed wrapper, start local to the wrapper: wrong source
frames, then the clip vanishes partway through its slot.
So the rule guards a real defect and stays. What was wrong is its stated cause:
the framework manages playback fine and nothing is "FROZEN". The frame extractor
resolves the video's start from its own `data-start` without the wrapper's
offset, while visibility uses the wrapper's window, so the two disagree. Message
and fixHint now say that, and the skill says the same.
Sub-composition media proven to work. Rendered a `<video>` inside a
sub-composition beside an identical one at the host root: identical frames
throughout. A composition host propagates its offset; only plain timed wrappers
break. The `assemble-index.mjs` guard that claimed sub-comp media renders blank
keeps its check, because that workflow's frame injector owns hoisted media, but
its comment now cites the render instead of asserting a framework limit.
`subcomposition_root_styled_by_class` message corrected. It still described the
scene rendering unstyled. #1886 fixed that, pinned by
`packages/producer/tests/sub-comp-class-selector`. The rule stays an error as a
consistency constraint; the message no longer teaches a symptom that cannot
happen.
`mediaRenderIds` `<source>` gap closed. The selector was
`video[src], audio[src], img[src]`, so a `<video>` whose source is a `<source>`
child was never stamped with a document-unique `data-hf-render-id` and two
inlined scenes kept colliding ids, which is the exact failure the module exists
to prevent. Media is now matched by tag and filtered on having any playable
source. Three tests cover it; two fail without the fix.
Also corrected the `deprecated_data_layer` fixHint, which claimed the runtime
reads `data-track-index`.
fdfe269 to
3d7e602
Compare
terencecho
left a comment
There was a problem hiding this comment.
Approved at fdfe269d. The technical case is very tight: every claim I spot-checked in the diff lines up with the code at head.
Verified independently at head
runtime/entry.ts:18unconditionally does(window as HyperframeWindow).__timelines = (window as HyperframeWindow).__timelines || {}, so the retirement oftimeline_registry_missing_initis safe — the registry is initialized before any inline composition script evaluates. The two flipped tests now assert the negative correctly.runtime/timeline.ts:596:// Track index is display-only; render never reads it— matches the skill rewrites intracks-and-clips.md,data-attributes.md, and the four workflow scripts.runtime/init.ts:2995:state.currentTime >= start && state.currentTime < end— the half-open window claim is real, and there's a dedicated test atinit.test.ts:638("uses a half-open interval around a timed element's end boundary").packages/parsers/src/compositionContract.ts:106:REFERENCE_ID_PATTERN = /^[A-Za-z0-9_.:-]+$/.intro-0.5matches (no operator split),intro - 0.5doesn't (spaces trip the pattern), soparseStartExpressionclassifies them exactly as the docs now warn — silent-fail trap is real.packages/core/src/audioGain.ts:MAX_AUDIO_GAIN_DB = 12andMAX_AUDIO_GAIN = 10 ** (12/20)(~3.981), pinning the docs' newdata-volumeceiling of3.98(+12 dB) indata-attributes.mdx,html-schema.mdx, andpackages/cli/src/docs/data-attributes.md.packages/lint/src/rules/media.ts:567-582:media_crossorigin_breaks_previewfires unconditionally (if (!hasAttrName(tag.raw, "crossorigin")) continue;then always push, no suppression), so "nevercrossorigin" is enforced. The docs change is honest.packages/producer/tests/sub-comp-class-selector/output/compiled.htmlcontains adata-hf-inner-rootwrapper — the #1886 fixture the corrected Pitfall-3 prose cites is present and pinned.mediaRenderIds.tschange: newhasPlayableSourcefilter correctly handlessrc-attribute +<source>-child + no-src-at-all cases; the three added tests exercise all three. Selector broadening from[src]to openvideo, audiois safe because the filter gates output.
Not what the title says (but the body is transparent)
The PR title is docs(skills): … but the diff also touches:
packages/core/src/compiler/mediaRenderIds.{ts,test.ts}— real behaviour change (stamps<source>-child media)packages/lint/src/rules/{composition,core,core.test,media}.ts— retirestimeline_registry_missing_init, corrects two diagnostics, rewritessubcomposition_root_styled_by_classprosepackages/cli/src/commands/coreSkillContent.test.ts— narrowed to a structural pin so docs corrections don't red the test
The PR body's "Also in this PR (the follow-ups, now closed)" section covers all of these, so scope drift is disclosed, not hidden.
Please handle before merging
- Merge conflict:
mergeStateStatus=DIRTY,mergeable=CONFLICTING. Since branch-fork, main landed the grouped-audio work (6faf4d608and54091b501) which extendedmediaRenderIds.tswith anAUDIO_GROUP_RENDER_ID_ATTRbus-stamping pass and touchedmedia.ts. The conflicts are in exactly the four files you also changed:mediaRenderIds.ts,mediaRenderIds.test.ts,media.ts,skills-manifest.json. The rebase is straightforward semantically (the bus pass is orthogonal to your selector broadening;hasPlayableSourceslots in ahead ofassignAudioGroupRenderIds), but you'll need to reconcile. - No CI on head SHA: last full CI ran on
ac6e5bdand everything substantive was green (Skills: manifest in syncflipped from failure on078c951to success after the resync — good). On6b3637beandfdfe269no CI fired at all, most likely because the merge conflict prevents GitHub from computing thepull_requestmerge commit. Rebasing will unblock CI and re-run the lint + Test: skills + regression suites on the actual combined SHA — which matters because the lint-rule retirement and the<source>-child fix landed in a commit that hasn't been CI-verified.
Happy to re-stamp automatically after rebase — the code review here holds.
— Review by tai (pr-review)
vanceingalls
left a comment
There was a problem hiding this comment.
Review — PR #3468
Verified the diff against the code at head (fdfe269), not just the PR description. Cloned to a scratch dir, no working-tree edits.
Verified correct (spot-checked against source, not taken on faith)
- Visibility window is half-open
[start, start+duration):runtime/init.tsisTimedElementVisibleAt—currentTime >= start && currentTime < computedEnd. Matches the corrected doc. data-track-indexis never read by the render: no reference to it anywhere inpackages/producer/srcorpackages/engine/src; only inruntime/timeline.ts(Studio) and lint.overlapping_clips_same_trackfinding is already absent pre-PR (pre-existing, unmodified test incomposition.test.ts) — same-track overlap was never rejected by lint.applyClipLayoutinruntime/init.tsonly auto-positionsArray.from(rootEl.children)gated onel.hasAttribute("data-start")— matches the new "root-level clips get automatic layout" claim precisely, including the full-screen-motion.md fix.media_crossorigin_breaks_previewis unconditional, no suppression path, inlint/rules/media.ts.- The removed claim "root must carry
data-start=\"0\", lint:root_composition_missing_data_start" was already false before this PR — that finding code doesn't exist anywhere inpackages/lint/srceven at the merge-base commit, only in a stale test asserting a negative. The new text ("runtime stamps it when absent") is correct:runtime/init.ts:305-306stampsdata-start="0"on the root when missing. - Deleting
timeline_registry_missing_init(lint/rules/core.ts) is justified:runtime/entry.tssetswindow.__timelines = window.__timelines || {}at module top-level with a comment explaining why, andhtmlBundler.ts'sinjectInterceptoralways places the runtime<script>inside<head>, ahead of body scripts, in all three injection modes (inline/src/placeholder). The registry-before-any-inline-script invariant holds.
Blocker: the <source>-child stamping claim is false, contradicted by code in this same diff
skills/hyperframes-core/SKILL.md now says:
the compiler stamps a document-unique
data-hf-render-idon everyvideo[src]/audio[src]/img[src]. Media that uses<source>children instead of asrcattribute is not stamped, so unique ids still matter there.
Both halves of that sentence are wrong against packages/core/src/compiler/mediaRenderIds.ts as shipped in this PR:
- The selector is no longer
video[src]/audio[src]— it was changed in this PR to"video, audio, img[src]", specifically to stop gating on[src]. hasPlayableSource(), added in this PR, explicitly treats a<source src>child as sufficient, andassignMediaRenderIdsstamps such elements (if (!hasPlayableSource(el)) continue;is the only skip condition).- The PR's own new tests in
mediaRenderIds.test.tsassert exactly the opposite of the doc line:"stamps media whose source is a <source> child rather than a src attribute"and"stamps <audio> with a <source> child too".
This is a doc-accuracy PR asserting a claim that the code committed in the same diff falsifies — not a subtle judgment call, a direct contradiction checkable by reading the two files side by side. Suggested fix: drop the [src] qualifiers from the first sentence and delete (or invert) the second sentence. Given that both the doc line and the module docstring hinge on this exact case (<video id="clip"><source></video> duplicated across scenes), it's also worth Miguel double-checking downstream: packages/engine/src/services/videoFrameExtractor.ts's parseVideoElements still queries video[src] and reads only the src attribute, and audioMixer.ts still queries audio[id][src] — so even though the compiler now stamps <source>-child media, I did not verify the actual frame-extraction/audio-mix path consumes that stamp for such elements (as opposed to just the browser-discovery fallback in probeStage.ts, which keys off the raw, unrewritten DOM id). Worth a real render test with two <source>-only clips sharing an id before trusting either the old or the corrected claim fully.
Not a blocker, but flag: this isn't actually a docs-only PR
The title docs(skills): undersells it: packages/core/src/compiler/mediaRenderIds.ts (selector broadening) and packages/lint/src/rules/core.ts (deletes the timeline_registry_missing_init error-severity rule entirely, not just its message) are real behavior changes, not prose corrections. I verified both are technically sound (see above), but a PR titled and described as doc-only will not get the same scrutiny a compiler/lint behavior change deserves from reviewers skimming the title. Worth splitting compiler/lint code changes from doc corrections in future, or being explicit about it in the description.
Nits
packages/lint/src/rules/composition.tscomment rewrite reads a little awkwardly ("match the SCOPED element itself" appears twice in adjacent sentences) but is accurate to the#1886inner-wrapper behavior — not worth blocking on.
— Vai
What
Corrects the composition-contract claims in
hyperframes-core(and the same claims where they are repeated inhyperframes-animation, three workflow scripts, and the scaffolded project instructions) so the skills describe what the code actually enforces.No format change, no restructure, no file moves. Every claim below was checked against
packages/on this branch's base, and the ones that change agent behaviour were reproduced with the real CLI.Why
The runtime has absorbed a series of authoring mistakes over time, and
packages/core/src/runtime/init.tssays so in its own comments ("Agents often writewindow.__timelines = [tl]", "Agents sometimes omit data-start on the root composition element"). Each time, the runtime learned to cope and the skill kept the old rule. The result is a skill that costs tokens and DOM restructuring for constraints that no longer exist, and in four places instructs something the gates now reject.Four instructions that cost an agent a failing run:
crossorigin="anonymous"for canvas capturemedia_crossorigin_breaks_previewrejects it unconditionally (error), no suppression, even for the readback case its own fixHint mentionsasyncgsap_timeline_registered_before_async_buildcalls that build "the documented async-setup contract"; what it rejects is registering the key before the build finishesgsap.set()later-scene clips, "they are not in the DOM at page load"<audio>/<video>recipe snippets with noidmedia_missing_idis an error, and an id-less<audio>is never picked up by the mixer, so a copied recipe renders silentClaims corrected in every place they appeared:
data-track-indexis a Studio display lane, not a timing constraint. The render never reads it (runtime/timeline.ts: "Track index is display-only; render never reads it"), no rule forbids same-track overlap, and the allocation ritual around it is removed. 11 sites.class="clip"is a layout and tooling convention; visibility keys ondata-start. Still recommended, because the shared.cliprule is what gives a scene its box.data-startget automatic layout, which is why thefull-screen-motion.mdbackground snippet rendered 0px tall. That snippet now ships the CSS it always needed.[start, start + duration), pinned byruntime/init.test.ts. It was documented as inclusive, so an animation landing exactly ondata-durationnever rendered its last frame.intro-0.5without spaces parses as an id, lookup is document-wide, cycles resolve to 0, and an unresolvable target duration lands on the target's start rather than its end.data-width/data-heightare backfilled by the compiler, and a host id differing from the file's is supported. Both were marked "Required".packages/producer/tests/sub-comp-class-selector/. The lint rule is still the reason to use#root; the render is not.src-bearing media, which the compiler stamps withdata-hf-render-id. The<source>-child gap that left is fixed below.How
assemble-index.mjs/transitions.mjscopies attributed their same-track lane rule totimeline_track_too_dense, which counts elements per lane and never compares windows. The checks are kept (each workflow's frame injector assumes one clip per lane) but now say it is their own convention, not a framework rule. Changing that generation behaviour is deliberately out of scope.packages/cli/src/templates/_shared/{CLAUDE,AGENTS}.mdcarried the same two false lines into everyhyperframes initproject. Corrected in lockstep; verified still byte-identical to each other.coreSkillContent.test.tspinned the literal sentence listing rootdata-startas required, so every correction here would have reddened CI in defence of a false claim. Narrowed to structural assertions plus the regression it genuinely catches (the skill must namenpx hyperframes checkand must not re-teach deprecated commands).skills-manifest.jsonregenerated.Test plan
Reproduced the
iddefect end to end with the real CLI, since that one changes rendered output:Confirmed the
data-startgrammar trap against the real parser rather than by reading it:parseStartExpression("intro-0.5")returns{kind:"reference", refId:"intro-0.5", offset:0}, while"intro - 0.5"returns{refId:"intro", offset:-0.5}.The skeleton the skill teaches still passes every gate:
Also green:
lint:skills(32 files),check:skill-mirror(24 files byte-identical),gen-skills-manifest --check(20 skills in sync),coreSkillContent.test.ts(11),init.test.ts(25),inlineSubCompositions.test.ts(29),oxlint+oxfmton all changed code files.One pre-existing failure, not introduced here:
skills/media-use/scripts/resolve.test.mjsfails on a colour-grading assertion (smart grade merges measured adjust, expected stderr matching/media-use: measured/, got''). It is flaky across runs (a second run had it pass and a different grading case fail), and this diff touches neitherskills/media-use/**norpackages/core/**, which is where that path lives.Also in this PR (the follow-ups, now closed)
All four items this PR originally deferred are done. Each behaviour change is backed by a render, not by reading code.
Human-facing docs
packages/cli/src/docs/data-attributes.md(served byhyperframes docs),docs/reference/html-schema.mdxanddocs/concepts/data-attributes.mdxcarried the same claims:data-track-indexrequired and preventing overlap,class="clip"required for visibility, anddata-volumetopping out at1when the shared ceiling is3.98(+12 dB,packages/core/src/audioGain.ts).Lint rules that contradicted the runtime
timeline_registry_missing_initdeleted. It demandedwindow.__timelines = window.__timelines || {}, butruntime/entry.tscreates the registry at script-evaluation time. I rendered a composition whose only registration is a barewindow.__timelines["main"] = gsap.timeline(...)and it animates correctly. Its two tests now assert the opposite, and the skill's skeleton drops the guard line and still passescheckwith zero findings.timed_element_missing_clip_classdeliberately kept. It is already a warning and its own comment already says the runtime keys offdata-start, so it is an honest convention nudge, not a false claim. This is a deviation from the audit's recommendation.subcomposition_root_styled_by_classmessage corrected. It still described the scene rendering unstyled; fix: producer render diverges from preview for sub-composition root styling #1886 fixed that, pinned bypackages/producer/tests/sub-comp-class-selector. The rule stays an error as a consistency constraint.deprecated_data_layerfixHint corrected ("The runtime reads data-track-index").video_nested_in_timed_element: kept, and the audit was wrongThe audit suggested deleting it. A render says otherwise. Three variants in one MP4, compared against a counter burned into the source:
Repro recipe, if you want to re-run it: a 6s
testsrcwithdrawtextprinting the frame number, three<video>elements in one composition (direct child, nested with a wrapper-timebase start, nested with a local start),hyperframes render, then compare the burned-in counter across extracted frames. The nested-local clip reads a counter value matching global time rather than its own slot, and stops being composited once its unoffset window ends.So the rule guards a real defect and stays. What was wrong is its stated cause: nothing is "FROZEN" and the framework manages playback fine. The frame extractor resolves the video's start from its own
data-startwithout the wrapper's offset, while visibility uses the wrapper's window, so the two disagree. The message, the fixHint and the skill now say that.Sub-composition media, and the
<source>gap<video>inside a sub-composition beside an identical one at the host root: identical frames throughout. A composition host propagates its offset; only plain timed wrappers break. Theassemble-index.mjsguard keeps its check (that workflow's frame injector owns hoisted media) but its comment now cites the render instead of asserting a framework limit.mediaRenderIds<source>gap closed. The selector wasvideo[src], audio[src], img[src], so a<video>whose source is a<source>child was never stamped with a document-uniquedata-hf-render-id, and two inlined scenes kept colliding ids. Media is now matched by tag and filtered on having any playable source. Three tests cover it; two fail without the fix.Verification for this half
The skill's skeleton, now without the registry guard line and without
data-track-index:Still not covered
One thing surfaced by the repro above and deliberately left out, because it is a behaviour change with a deprecation cost rather than a correctness fix:
runtime/init.tsreads a media element'sdata-startinside a host as global when it overlaps the host window and local otherwise, and its own comment says "Both timing conventions exist in shipped projects". The extractor does not apply the same rule for plain timed wrappers, which is the divergence behindvideo_nested_in_timed_element. Making media starts unconditionally local would delete the guesswork and the lint rule with it, but it changes the meaning of existing compositions and needs a deprecation window. Worth its own initiative; flagging rather than sneaking it in.