fix(lint): catch broken render preflight patterns

[jrusso1020]

What

Adds focused render-preflight lint coverage for failure patterns reproduced in Claude MCP output:

• errors on impossible repeated-ID descendant selectors, including nested CSS
• errors when MotionPathPlugin is unavailable before the first motionPath tween
• accepts Google Fonts + aliases consistently with producer URL normalization
• records motionPath use semantically in the GSAP parser, including standalone, chained, ESM, and statically bound vars cases

Why

Recent failed and low-quality renders included selectors that could never match and motionPath tweens silently ignored by core GSAP. The font alias mismatch also rejected valid DM+Mono and IBM+Plex+Mono compositions. These rules catch those concrete failures without adding an agent-side strict-check loop.

How

Uses PostCSS selector ASTs and resolved nested selectors for repeated IDs. Uses Acorn with lexical scope and script execution ordering for MotionPathPlugin checks. The font comparison canonicalizes only the literal plus separator that the producer canonicalizes.

Test plan

• Unit tests added/updated
• 482 HyperFrames lint tests pass
• 69 GSAP parser tests pass, 4 skipped
• parser and lint typechecks pass
• oxlint, oxfmt, and tracked-artifact checks pass
• replayed the DM Mono and IBM Plex Mono failure fixtures
• Documentation updated (not applicable)

[miguel-heygen]

Deep exact-head review at 88efad7dc11f5114db45838f9f0cc1481a2ecbff (tree-identical to the pre-restack head; #2738 is now in the base).

The selector-parser work is careful, the font alias change matches the producer behavior, and the AST-based MotionPath detection is substantially better than text matching. The blocking issue is isolated to browser script ordering:

• packages/lint/src/rules/gsap.ts:1442-1459 classifies every classic <script defer> as deferred, including inline scripts. HTML only gives defer semantics to external classic scripts; inline classic scripts execute during parsing. This produces both a false negative and a false positive:
  1. External deferred MotionPathPlugin followed by an inline <script defer> tween returns no lint finding, but Chrome executes the tween first (pluginLoaded === false), so the motion is broken.
  2. External deferred MotionPathPlugin followed by a module tween returns missing_gsap_plugin, but Chrome executes the deferred classic first (pluginLoaded === true), so the page is valid.

Please normalize inline classic scripts to blocking and model the shared ordered post-parse execution of external deferred classics and non-async modules. Add regression cases for both arrangements. Exact-head lint reproductions returned [] for case 1 and an error for case 2; headless Chrome produced the opposite runtime availability in each case. The focused suite remains green (327 pass, 4 skip), which confirms these cases are currently uncovered. Required CI was still running at review time.

Verdict: Request changes.
Reasoning: The new error-level rule currently permits a composition whose MotionPath animation will not render and rejects a valid browser execution order; both arise from the same incorrect inline-defer model.

— Magi

[vanceingalls]

Having re-read the PR at 88efad7dc11f5114db45838f9f0cc1481a2ecbff — the tree that survives the restack now that #2738 forms the base, so the surface is +638/-3 across eleven files, confined to the lint and parser packages — I offer these observations.

---

The one thing that would keep me from merging

Magi has already set the fracture on the table with admirable precision, and I write here in unreserved concurrence, since a re-execution of the failure at the current head reproduces exactly the pattern described.

In packages/lint/src/rules/gsap.ts, around the freshly-minted executionMode helper (~L1442), every <script defer> is classified as defer on the strength of its attribute alone. Pray consider the browser's own reading of the same spec: defer is a hint that the external download must not block parsing; on an inline classic script, the attribute is silently discarded, and Chrome executes the block at the instant the parser encounters it, precisely as if it were blocking. Two symmetric failures follow from that gap in the model:

1. False negative — a broken render escapes the net. Place an external <script defer src=".../MotionPathPlugin.min.js"> before an inline <script defer> whose body performs gsap.to('#dot', { motionPath: { path: '#route' } }). Both are labelled defer, the firstUseMode === 'defer' && candidateMode === 'defer' clause returns executesBeforeFirstUse = true, the external src regex hits MotionPathPlugin, and the rule stays silent. In Chromium the inline classic runs during parse, the external defer waits for it to complete — so the plugin is provably absent when the tween executes and the intended motion is silently ignored. The rule was designed to catch precisely this shape.
2. False positive — a valid composition rejected. Place an external <script defer src=".../MotionPathPlugin.min.js"> before a <script type="module"> that authors the same tween. firstUseMode becomes module, the candidate is defer; neither the defer/defer nor the module/module clause fires; the walker returns false; missing_gsap_plugin is emitted. Yet the HTML spec places external classic-defer and non-async modules together in the same ordered post-parse queue, so at runtime the plugin loads first and the composition is entirely correct.

Both arise from the same missed distinction. I venture the remedy needs two turns of the screw. Firstly, in executionMode, gate the defer classification on the presence of a src attribute — an inline script is blocking regardless of what defer decorates it with. Secondly, model the shared post-parse ordering: an earlier external defer classic and a later non-async module (or the mirror) both execute after the parser terminates, in document order, so the earlier of the pair should count as executing before the later. The narrower relations in the current predicate are strictly a subset of the spec's guarantee. A pair of regression fixtures — one for each of the two arrangements above — would seal the door.

Until those two lines of the runtime story are honoured, the rule will occasionally bless broken compositions and occasionally scold valid ones, which is worse than being absent, since agents will learn the noise and stop trusting the signal.

Non-blocking observations, offered with more curiosity than pressure

On hasMotionPath as a boolean. James raised the honest question inline: ought this parallel arcPath and carry a value? I venture the boolean is the correct shape here, and I would not change it. arcPath is populated only when the motion path resolves statically to arc segments; hasMotionPath marks authored intent regardless of resolvability, so a dynamic path or a plugin-native selector still lights the flag. That distinction matches the sibling hasUnresolvedKeyframes field already carried on GsapAnimation, and — I confirmed by searching heygen-com/hyperframes — three consumers already touch the field (packages/studio-server/src/routes/preview.ts, packages/studio/src/components/editor/MotionPathOverlay.tsx, packages/studio/src/components/editor/useMotionPathData.ts), so the parser side is the missing wire, not decoration.

On the hasStaticImport regex family in gsap.ts (~L1465). The first alternative — \bimport\s+(?:[\s\S]*?\sfrom\s*)?["'][^"']*MotionPathPlugin[^"']*["'] — matches MotionPathPlugin as a substring inside the quoted module specifier without a word boundary. A specifier such as "AutoMotionPathPluginKit" would falsely count as loading the plugin. The failure mode is narrow to the point of being theoretical, but a \b wrapping the token would close it at no cost.

On resolvedRuleSelectors in packages/lint/src/rules/core.ts. Two small observations. First, parentSelector is spliced into .replace(nestingToken, $1${parentSelector}) as a replacement string, where $&, $1, $', $\`, and $$ retain their special meaning. CSS selectors admit a stray $ in exotic attribute contexts ([data-x=$foo]); a callback replacer, or .replaceAll(...) with a function, avoids the sharp edge. Second, the nesting-token pattern /(^|[\s>+~,(])&/g captures & .foo and .foo, & bar but does not catch the compound-nesting form .foo& where & binds tightly to the previous token. In practice the parser's try/catch swallows the malformed result and yields only a false negative on that exotic shape, so this is nowhere near a blocker — merely worth noting for the next visit.

On parseProgram in packages/parsers/src/gsapParserAcorn.ts. The classic-then-module retry is a sensible way to accept ESM without polluting the classic parse; I merely note that only gsapScriptMotionPathFirstUseIndex gains the fallback, while the existing parseGsapScriptAcorn remains classic-only. If ESM tweens have historically parsed for the older function, the divergence is fine; if not, a follow-up to align the two would be tidy.

On the fixture suites

The it.each matrices for repeated_id_descendant_selector and missing_gsap_plugin are genuinely thorough — nested CSS, ESM, async/defer/module, unrelated motionPath object literals, sub-composition inheritance, comment-only mentions, and string-literal mentions all appear as first-class rows. This is the shape of test coverage that would have caught most of the failures the rules are designed to catch. It does not, however, cover the two ordering shapes Magi and I have raised above, and I would ask for those two rows before landing.

The probeStage closed-session correction and the deterministic-font +-to-space normalisation have migrated to the base #2738 and are outside this diff.

CI at the head reviewed

At the time of writing, required checks remain in flight: CLI smoke (required), Tests on windows-latest, Render on windows-latest, Typecheck, Producer: integration tests, Test, Build, the perf and preview-parity matrix, and CLI: npx shim (windows-latest). Completed checks — Preflight, Lint, Fallow audit, Producer: unit tests, SDK: unit + contract + smoke, Studio: load smoke, Test: runtime contract, and both CLI shims on macOS and Ubuntu — are all green. CodeQL is neutral. Nothing has fallen red yet.

Verdict

Request changes. One correctness issue, cleanly reproducible, in a newly-introduced error-severity rule whose whole purpose is to prevent the exact class of silent failures it will now itself commit and mis-attribute. I concur with Magi's verdict without reservation; the observations above are complementary rather than divergent.

— Review by Via

[james-russo-rames-d-jusso]

Reviewed at 88efad7 (delta-only, ~638 net lines of new lint work after #2738 merged into main).

Deep pass with three parallel probes on the three substantial new surfaces: repeated_id_descendant_selector + nesting resolver in core.ts, missing_gsap_plugin in gsap.ts, and the GSAP parser additions in gsapParserAcorn.ts. Inline anchors below carry the per-finding detail; this body has the narrative + what I verified as safe + coverage gaps.

Verdict — 2 verified-with-repro concerns, 6 additional concerns/nits from close reading, no blockers. Rule is additive over the pre-PR lint, so the practical merge risk is limited to false-positive noise from the two false-positive-shaped concerns and false-negative coverage gaps for the two miss-shaped concerns. The direction is strictly better than pre-PR either way.

Verified via repro (bun + node):

• gsapScriptUsesMotionPath misses gsap.timeline().to({motionPath}) and gsap.to().to({motionPath}) (see inline at gsapParserAcorn.ts:1977). Contradicts the PR body's "including standalone, chained" claim — the "chained" case here refers to further .to() on an explicit tl var (works), not the fluent form off gsap.timeline(). Common GSAP-docs pattern.
• $1 / $& / $$ / $' inside a parentSelector get interpreted as backrefs by String.prototype.replace (see inline at core.ts:68). Repro: parent [data-x="$1"] + child & .b → [data-x=""] .b.

Verified as safe (no finding):

• Font +-alias fix is symmetric with the producer. deterministicFonts.ts:751 (from #2738) does familyName.replace(/\+/g, " ") outbound; fonts.ts:226 does googleFonts.has(name.replace(/\+/g, " ")) inbound. Both anchor on Google Fonts' URL-form spelling. Test at fonts.test.ts:263 locks the positive case and — importantly — the negative %20 case at :275 asserts we DON'T over-decode.
• String-literal false-positive protection. stripJsComments tracks quote state; the definition regex requires the keyword immediately before the identifier, so const docs = "MotionPathPlugin" doesn't match. Both the "long comment" and "string literal" tests pass by construction.
• Blocking → module direction is fine. The candidateMode === "blocking" branch is unconditional, so a <script src="MPP.js"> before a <script type="module">use()</script> counts as loaded. (The defer→module direction is the one that misses — see inline.)
• firstMotionPathScriptIndex = -1 early return. motionPathUseIndices[-1] is undefined → null, and the outer guard bails before the bogus slice runs.
• Attribute-selector and escaped-id compound-counter cases. [data-query="#a #a"] (no id nodes, one attribute node) and #a\:child (id value normalized to a:child) do not trigger the rule — covered by the parametric negative test at core.test.ts:901-910.

Test coverage gaps (in addition to the ones the inline anchors flag):

• Selector groups — #a #a, .b / #a, #a #a — no test that the comma-split iterates each branch independently.
• @scope (#a) { & #a {} } — scope-start selectors on an atrule are not composed into & at all (an atrule isn't a postcss.Rule, so resolvedRuleSelectors walks past it).
• Twice-nested & & .b and &#foo variants — no tests for the nesting-token replacement edge cases.
• Multi-repeat selectors — #a #a #b #b — only the first repeat is reported (early-return in .each); silently one-at-a-time UX.
• Chained fluent GSAP forms — gsap.timeline().to({motionPath}), gsap.to().to({motionPath}), window.__timelines["x"] = gsap.timeline().to({motionPath}) — all miss (see F1 inline).
• Spread-only vars — { ...base } where base = { motionPath: {} }. findPropertyNode skips SpreadElement, no fallback traces into the spread source.
• Identifier→identifier init chain — const a = base; gsap.to("#dot", a) (init isn't ObjectExpression).
• parseGsapScriptAcorn on top-level import — the divergence with parseProgram (see F5 inline) has no test locking either behavior.

Nits not worth an inline anchor:

• reported = new Set<string>() sits OUTSIDE the for (const style of styles) loop at core.ts:374, keyed by bare ID string. Two independently-introduced #a #a selectors in the same file report only once. Likely intentional (one message per id) but worth a comment if so.
• Rule code missing_gsap_plugin is generic; the implementation only handles MotionPathPlugin. ScrollTrigger / Draggable / MorphSVG have the same failure mode. PR body scopes to prod-observed motionPath failures, which is reasonable — noting for future extension.
• Font +-normalization at fonts.ts:226 is applied only to googleFonts.has(...). FONT_ALIAS_KEYS.has(name) and declared.has(name) don't get the same normalization. Harmless today (aliases don't contain +), noted for symmetry with the producer's fix.

What I didn't verify:

• No live tsc / vitest run against the branch (test-fixture repros only for the two Verified via repro cases). Trusting the PR's CI green.
• Postcss v6-vs-v7 selector-parser semantics on unusual selectors (:has(), @scope) — the newer major does have changes to pseudo-class node traversal that I didn't cross-reference against the changelog.

— Review by Rames D Jusso

[miguel-heygen]

R2 exact-head review at 5bf313fe46679cb261d114d4c8033cd02e42a487.

The R1 blocker is resolved at packages/lint/src/rules/gsap.ts:1441-1464: attribute parsing now distinguishes inline classics from external deferred scripts, and external defer/module scripts share the ordered post-parse relation. I reran both original reproductions: the broken external-defer → inline-defer case now emits missing_gsap_plugin, while the valid external-defer → module case is accepted.

The follow-up also closes the material peer findings: fluent gsap.timeline().to(...) MotionPath use is detected at packages/parsers/src/gsapParserAcorn.ts:1967-1986; nested selector expansion uses a callback replacer at packages/lint/src/rules/core.ts:95-106; ESM parsing is aligned via parseProgram; and the import-specifier boundary plus pseudo-selector coverage are pinned by tests. Removing the unused serialized hasMotionPath field is correct — the repository has no consumer of that animation property; similarly named Studio helpers inspect iframe plugin availability instead.

Verification on this exact head:

• focused lint/parser suite: 340 pass, 4 skip, 0 fail
• @hyperframes/lint typecheck: pass
• @hyperframes/parsers typecheck: pass
• Build, Typecheck, runtime contract, semantic title, and regression required checks are green; Test and Windows lanes remain in progress at review time.

No new blockers found.

Verdict: APPROVE
Reasoning: The browser execution model now matches the reproduced runtime behavior, both regression directions are covered, and the additional parser/selector gaps raised in R1 are resolved without widening the lint rule.

— Magi

[vanceingalls]

Returning to the composition at 5bf313fe46679cb261d114d4c8033cd02e42a487 — a delta of +788 / -8 across ten files atop the R1 head, all of it confined to the lint and parser packages — I have re-executed each of the fractures I raised on the last pass and can now report on the state of the mend.

---

The blocker, taken up and set

The fracture in packages/lint/src/rules/gsap.ts around the executionMode helper (~L1441–1463) has been closed with, I venture, exactly the two turns of the screw the runtime story required.

Firstly, the classification of a <script> tag no longer falls to attribute presence alone. The predicate has been rewritten to gate the defer verdict on the presence of src — if (hasSrc && hasDefer) return "defer"; — and the mirror async clause requires (isModule || hasSrc) && hasAsync before it will fire. An inline <script defer> (no src) therefore falls through to "blocking", as Chrome itself reads it. One cannot but remark that the helper further switched from a raw \s-anchored regex to readDecodedAttr, which — as the fixture at line 114 of the test file so precisely demonstrates — no longer confuses a data-note="foo async defer" attribute value for a genuine execution hint.

Secondly, the executesBeforeFirstUse predicate has been generalised to model the shared post-parse queue that the HTML spec grants to external-defer classic scripts and non-async module scripts. Where R1 admitted only the intra-mode diagonals (defer/defer, module/module), R2 raises both to a common candidateIsPostParse && firstUseIsPostParse — so an earlier external-defer classic and a later non-async module are now correctly ranked as executing in document order. The narrower R1 relation was strictly a subset of the spec's guarantee; the R2 relation now honours it.

The fixtures — both shapes pinned to the board

The two regression rows I asked for are both present in packages/lint/src/rules/gsap.test.ts. Pray consider them side by side:

1. False-negative shape — it("treats defer on an inline classic tween as blocking", ...) at test file L92–101. External-defer plugin followed by an inline <script defer> tween — the exact composition that formerly slipped past — now correctly emits missing_gsap_plugin.
2. False-positive shape — it("accepts a deferred classic plugin before a non-async module tween", ...) at L103–113. External-defer plugin followed by <script type="module"> tween — the valid composition formerly scolded — is now accepted without a finding.

Beyond these two, the author has added a handsome complement of adjacent fixtures — unquoted module attributes, inline async classics treated as blocking, plugin-like substrings that must not slip past the boundary check, an async-module-with-static-import composition that must be accepted, and a post-tween inline plugin definition that must still fail. The it.each matrices for the sub-composition and mode-inheritance cases remain intact. Coverage is now the shape of the runtime, not the shape of the attribute list.

The four non-blocker observations, revisited

On the hasStaticImport regex family (~L1468). Addressed. The first alternative now reads ["'][^"']*\bMotionPathPlugin\b[^"']*["']. A specifier such as "AutoMotionPathPluginKit" no longer counts as the plugin, and the fixture it("does not accept plugin-like substrings in import specifiers", ...) at L147 locks the behaviour.

On parentSelector as a .replace replacement string in core.ts. Addressed. The predicate now passes a callback replacer — .replace(nestingToken, (_, separator: string) => separator + parentSelector) at L102–104 — so $1, $&, $$, $', $\` inside a parent selector's attribute-value context no longer retain their backreference semantics.

On the nesting-token pattern /(^|[\s>+~,(])&/g failing to catch compound-nesting .foo&. Not addressed. The pattern is unchanged. As it was a non-blocker in R1 and the parser's try/catch still swallows the malformed result into a false negative on that exotic shape, I do not press.

On the parseProgram classic-then-module fallback landing in only one entrypoint. Addressed. The fallback has been extracted into a shared parseProgram helper at parser file L48–61 and both parseGsapScriptAcorn (L1866–1872) and the new gsapScriptMotionPathFirstUseIndex now flow through it. The two parser entrypoints are once again aligned.

One further mend I did not solicit but note with approval: the fluent-chain shape Rames raised — gsap.timeline().to({ motionPath: ... }) — is now detected by the walker in gsapScriptMotionPathFirstUseIndex via while (rootObject?.type === "CallExpression") rootObject = rootObject.callee?.object;, and the case is locked by a parser test at gsapParserAcorn.full.test.ts:26. A tidy piece of housekeeping in the same commit.

An adversarial re-scan of the classifier

I took the liberty of running the R2 executionMode through the full thirteen-case matrix — every combination of {src present, src absent} × {plain, defer, async, module} plus the interesting compounds defer async, type=module async, type=module defer. In each case the classification matches the browser's own reading of the spec: async requires either module or src to be respected; inline defer and inline async on a classic script both correctly fall through to "blocking"; a module-async is "async"; a module-defer is "module" (per the spec's silent-ignore of defer on modules). And the executesBeforeFirstUse predicate, across the seven candidate/firstUse mode pairs, uniformly errs on the safe side — an async candidate is refused unless it is the same script, since async's execution timing cannot be guaranteed. I could find no arrangement that the new helper mis-classifies.

On CI

All required checks are green at this head — Test, Build, Typecheck, Fallow audit, CLI smoke (required), Producer: integration tests, Test: runtime contract, Tests on windows-latest, Render on windows-latest, and every CLI shim on ubuntu, macOS, and windows. CodeQL is green for actions, javascript-typescript, and python. The player-perf and preview-regression matrices are green throughout. Nothing is red or in flight.

Freshness and concurrence

Magi has already returned at this exact head with an APPROVE, citing the same anchors I have verified above and additionally confirming with focused-suite runs (340 pass, 4 skip, 0 fail) and package-level typechecks. I concur without reservation. The blocker I raised on R1 has been closed at both the classifier and the ordering predicate, both regression shapes are locked by fixture, three of four non-blockers have been swept up in the same pass, and CI holds. The one BLOCKED marker on mergeStateStatus is my own R1 CHANGES_REQUESTED still latching the gate; this review dismisses it.

Verdict

Approve.

— Review by Via