fix(lint): stop three rules matching code a composition only displays

[xuanruli]

Rebuilt onto today's main (the July branch shared no ancestry after the history rewrite). Cherry-picked clean; no conflicts.

A code-explainer composition renders selectors and API calls as text. New stripJsStringLiterals blanks string and template contents with offsets preserved, so template_literal_selector and requestanimationframe_in_composition scan real code only. split_data_attribute_selector gets comments-only treatment, plus stripCssComments for its <style> half — see the limitation below.

Fail-open is scoped, not global (from review 5127270386)

The scanner returns the source unchanged on any parse it cannot model:

• an unterminated string or template;
• an unterminated regex at EOF;
• a slash read as a regex start that finds no closing slash before the newline.

That last case covers regex-vs-division misdetection generally. of, in, yield and await are contextual keywords and are legal plain identifiers, so var r = of /2; misreads as a regex — bounded by the newline in pretty-printed source, but the whole file in a minified inlined bundle, which is exactly the input this scanner was built for. Rather than enumerate contextual-keyword positions, an unclosed regex is treated as proof the slash was division.

That fail-open is right for a rule that detects presence and wrong for one that detects absence. root_composition_missing_duration_source (severity error) reads absence: unstripped comments make a commented-out gsap.timeline( read as present, the rule short-circuits, and the gate stops firing silently. So stripJsComments keeps only the regex awareness — a strict improvement for all 17 pre-existing call sites, since it stops /^https?:\/\// being read as a line comment — and the fail-open lives in a new stripJsCode, used by the two sandwich sites alone. A test pins the absence rule against a lexer misread in the same script.

stripJsComments previously tracked quotes only, so /^https?:\/\// — the standard protocol test, whose last two characters are // — read as a line comment and blanked everything after it. That mattered because this PR routes two error rules through it.

Verified on the repo corpus: 890 HTML files, 1119 findings, byte-identical to main. On the largest bundled script in the tree (packages/producer/tests/maplibre-adapter/output/compiled.html, 802 KB minified) stripJsComments keeps 99.9% of non-whitespace and does not fail open; before the regex fix it lost 40.9%.

Tests: 588 pass in packages/lint (main: 552), covering every fail-open path, the absence-rule guard, and a linearity guard (the first draft re-scanned accumulated output per candidate slash and was quadratic).

Known limitation: ) is not treated as a regex-start position, so the / in if (x) /re/.test(y) is read as division. Nothing is blanked and the output equals the source for that shape, but a genuine regex there is not tracked.

Not changed: split_data_attribute_selector stays comments-only. Its true positive lives inside a string literal (document.querySelector('[data-composition-id="main" data-start="0"]')), so the sandwich would silence the real defect along with the code-explainer false positive — measured 1 finding as shipped, 0 with the sandwich. A script-only regression test now pins that; the previous test put the same selector in a <style> block and dedupeKeyFor collapsed all three occurrences to one, so it reported 1 whether the rule worked, was sandwiched, or was deleted outright. Distinguishing a selector passed to a DOM query from one merely displayed needs call-site awareness this utility does not have — follow-up.

[james-russo-rames-d-jusso]

:large_green_circle: 3eb3cf6b7 COMMENTED

Hey Xuanru — solid direction (single-pass state machine + fail-open on unbalanced parses is the right shape for an error-severity gate) and the perf-ceiling raise on this push clears the CI red on the linearity guard. One real gap on split_data_attribute_selector — one of the three rules is only half-treated — and one shared-utility contract note. Everything else at HEAD verifies.

Concerns

• split_data_attribute_selector gets stripJsComments, not stripJsStringLiterals for scripts (composition.ts:493) — the other two rules get both (composition.ts:503, :656). A code-explainer that displays const S = '[data-composition-id="main" data-start="0"]'; still fires this rule at :error: severity, which is exactly the false-positive class this PR is meant to end. composition.test.ts only probes the JS-comment path for this rule, so the gap isn't visible in the suite. Suggest: same stripJsStringLiterals(stripJsComments(...)) sandwich for the script scan, plus a "displayed in a string literal" probe. Not tagging as a blocker since the fix is one-line and you can take it on the same head, but the PR title's "three rules" promise doesn't fully land without it.

• stripJsComments fail-open is a shared-utility behaviour change — on any regexMisread / open quote at EOF the function now returns the raw source, so every caller (gsap.ts, media.ts, core.ts, several other composition.ts rules including allScriptTexts at :1024) newly scans comments as code on malformed inputs. The regex-vs-division awareness is a genuine win for /^https?:\/\// (thanks — that bug was real), but the fail-open path is untested for those other rules. Suggest an integration probe: run the corpus scan for gsap.timeline/querySelector/raf rules against a fixture with an unterminated string somewhere upstream and confirm findings don't grow.

Verified as OK

• template_literal_selector (composition.ts:503) — stripJsStringLiterals(stripJsComments(...)) sandwich; truncateSnippet(script.content.slice(tlMatch.index, ...)) slices from the raw source, which works because stripJsStringLiterals's length/newline-preserving contract keeps tlMatch.index valid in both. Discriminator: content sits inside a '/"/` literal (state-machine quote slot), template ${…} bodies are preserved as code (templateBraces stack). Test #2 in composition.test.ts is the "still fires in a template interpolation" regression witness — good pairing.
• requestanimationframe_in_composition (composition.ts:656) — same sandwich, same discriminator. Regression witness at composition.test.ts ("still flags a call inside a template interpolation"); false-positive probe on displayed-as-string form.
• split_data_attribute_selector (styles path only) — stripCssComments handles the "quoted /* as content" pathology correctly; test at composition.test.ts ("keeps a rule sandwiched between comment markers printed as content") is exactly the right shape.
• Discriminator is unambiguous for the state machine's inputs — every char position is unambiguously either inside a string-literal body, a template ${…} code body, a regex body, a comment body, or top-level code. The regex-vs-division tie-breaker (CodeContext.startsRegexLiteral on REGEX_ALLOWED_BEFORE + contextual-keyword set) is the only heuristic, and it correctly fails-open (returns raw source) when the state machine gets stuck (unterminated regex hits newline, quote open at EOF, unclosed template).
• Linearity design — carrying last/prev/word incrementally on CodeContext avoids the quadratic re-scan the first draft had; the ratio assertion (large/small < 24) passes, and the absolute-bound assertion on this HEAD is expect(large).toBeLessThan(2_000); (utils.test.ts:104) — the ceiling raise on this push absorbs the observed 745.86ms cleanly.
• Test surface — it.each cases probe every contextual-keyword branch (of, in, case, new, member access via .new/.in), operator disambiguation (i++ / total, --i / total, b / c / d), regex-with-embedded-quotes, escaped-slash regex, EOF fail-open, unterminated-string fail-open, length/newline preservation.

Questions

• Is the "no ) in REGEX_ALLOWED_BEFORE" limitation you called out ever going to bite the corpus? if (cond) /re/.test(x) is common enough that on a real code-explainer the whole script would fail-open, silently disabling all three rules for that composition. Worth a bug-then-fix rather than fix-in-this-PR, but worth naming in a known-limitations.md next to the utility.

What I didn't verify

• The 890-HTML / 1119-finding byte-identical corpus claim (would want a CI harness, not just body text) — but running it isn't in this PR's scope.
• Whether any downstream @hyperframes/lint consumer needs a coordinated bump; the change is intra-package so I'd guess no.
• Prior state of the PR pre-force-push — reviewed the current single-commit shape only. 3eb3cf6b7's parent is master (7a2a6917, #3735 catalog fix), so the current commit is the entire PR content.
• Commit message rule names: the commit message lists rules; verify against the actual composition.ts rule code names before merge, since the three rules touched at source are split_data_attribute_selector, template_literal_selector, and requestanimationframe_in_composition.

State at HEAD 3eb3cf6b7: isDraft: false, mergeStateStatus: BLOCKED, mergeable: MERGEABLE, reviewDecision: REVIEW_REQUIRED. CI at HEAD: Typecheck pass, Build pass, Lint pass, Test: runtime contract pass. Test / Producer: integration tests / Tests on windows-latest still pending as of read; older run 34069795295 at prior push showed the pre-raise perf-bound failure (utils.test.ts:101 was < 400, observed 745.86ms) — that push was superseded, disregard.

Code LGTM from my side, with the split_data_attribute_selector sandwich as the one thing worth taking on the same head before merge. Stamp goes through a HyperFrames-authorized reviewer — heygen-com/hyperframes runs dismiss_stale=false / require_last_push_approval=true, so pin the stamp to the exact SHA that lands.

— Review by Rames D Jusso

[jrusso1020]

Reviewed at dadb6941bdf4322070742ef624c318a00071613b, independently rather than carrying over the earlier COMMENTED review -- that one is pinned to 3eb3cf6b7c9e00dc7b5d22428b3723a1f5b15c28, i.e. before the force-push, and it is the review whose suggestion this head rejects. A rewritten commit that overrides a reviewer's finding is the last one that should inherit that reviewer's sign-off.

Requesting changes on one contained item. Calling it out up front: it is latent, not live (see reachability below), the fix looks like three lines, and the PR is a large net improvement. If you read the severity differently I will approve on request -- the evidence is below and the shipping call is yours.

First, the thing you were uncertain about: your rejection was right

Verified by execution, running the actual rule regex over the actual fixtures at this SHA.

NEW test, script-only:
  as shipped (stripJsComments only) : 1  ['[data-composition-id="main" data-start="0"]']
  IF the sandwich were applied      : 0

The rule's true positive in JS necessarily lives inside a string literal -- document.querySelector('[...]'), tl.to('[...]') -- so stripJsStringLiterals takes it from 1 to 0 and kills the rule's entire JS-side detection. The one-liner was wrong.

And your read of the old test is exactly right, for a sharper reason than "it used a <style> block." In composition.test.ts:454 the selector text appears once in <style> and twice in <script>, and dedupeKeyFor (hyperframeLinter.ts:67) keys on code|severity|selector|elementId|message where both selector and message embed match[0]. So all three collapse to one finding. Measured across three worlds -- as shipped, with the sandwich, and with the script scan entirely dead -- the count is 1, 1, 1. The old test could not have distinguished a working rule from a deleted one.

The new test is a genuine discriminator: script-only, no <style> fallback, so the sandwich takes it to 0 and expect(findings.length).toBe(1) goes red.

Blocker: the new fail-open reaches 17 pre-existing call sites, and one of them reads absence

utils.ts:459 adds if (quote !== null || inRegex || regexMisread) return source; to stripJsComments. On main that function never failed open. Call sites at this SHA: gsap.ts x11, composition.ts x4, media.ts x2, core.ts x1 -- none written for a no-op return, and the PR body audits none of them.

The body's justification is "an over-aggressive strip does not break a build -- it silently disarms the gate ... Both scanners therefore return the source unchanged on any scan they cannot model." That holds only for presence-detecting rules. composition.ts:1024 reads absence, and its own comment says why:

a commented-out .animate(...) call ... must not satisfy the "has a duration source" check, or the composition still fails at render with zero duration despite lint passing.

Returning the source unchanged is that failure. Executed:

var of = 2;
var r = of / 2;
// const tl = gsap.timeline({ paused: true });
// window.__timelines["main"] = tl;

stripJsComments failed open (returned source unchanged): true
hasGsapTimeline: true   hasRegisteredTimeline: true
=> rule short-circuits with `return []`  : true

control, `of` renamed to `q` (== main's behaviour):
failed open: false  hasGsapTimeline: false  hasRegistered: false

So root_composition_missing_duration_source -- severity error, the rule whose own comment calls it "the exact shape of the 27K zero-duration render failures" -- stops firing at head and fires on main. A false negative on an error gate, at a call site this PR does not touch.

Reachability, stated honestly: 0 of 611 inline scripts in the local corpus fail open. It needs a lexer misread in the same script as a commented-out timeline. So this is latent today, which is why I led with the offer to approve if you'd rather land it and follow up. What tips it for me is that the gate fails silent -- by construction nothing fires when it breaks, so a latent disarm on an error gate is the kind that stays latent until a render fails in production.

Fix that keeps the win: make the fail-open opt-in -- e.g. a stripJsCommentsOrNull used only by the two new sandwich sites -- leaving stripJsComments total for the existing callers.

Two PR-body claims that do not survive checking

1. The "Known limitation" sentence is wrong in direction. It says )-position regexes "fail open, which disables the three rules for that script rather than blanking it." They blank:

IN   : if (u) /^https?:\/\//.test(u); requestAnimationFrame(step);
failed open? false
OUT  : if (u) /^https?:\/\  + 40 spaces
rafVisible: false

That is the exact motivating bug from your own PR body (/^https?:\/\// read as a line comment), surviving unfixed wherever the regex sits after ), ], or a non-whitelisted identifier. Not a regression -- main was broken for all positions, so this is still strictly better -- but the sentence is load-bearing for the design argument, so it should say "blanks" not "fails open".

2. The three rule codes are misnamed. The body names composition_id_selector_in_comment and raf_animation_loop; neither string exists in packages/lint/src at this SHA. The actual codes are split_data_attribute_selector, template_literal_selector, requestanimationframe_in_composition.

Related, and worth knowing since "fail-open is the whole safety story" is the stated design: a misread regex that closes eats real code with no fail-open at all. of / await / yield are legal plain identifiers, so a division after one plus a later / on the same line blanks the span between:

IN   : var of=2, r=of/2; requestAnimationFrame(step); var s=8/4;
OUT  : var of=2, r=of/                                       /4;
rafVisible: false   (main: true)

What held up

Stress-tested the lexer hard, and almost everything is right. a=b/c/d, i++/total, --i/total, x.in/y, o?.in/2, return /re/, typeof /re/, /['"]/, /a\/b/, /[/]/, nested templates, ${} containing a string containing a backtick, } in template static text, unterminated string/template, trailing-backslash string, CSS content:"/*" pairing, CSS escaped quote wrapping a comment, selector split across an interpolation -- all correct.

Length preservation holds on every path. 200,000 random inputs over a JS-hostile alphabet across all three functions: zero length or newline mismatches, including astral-plane chars and CRLF. That matters because composition.ts:514-516 slices script.content using indices computed on the stripped string -- verified aligned in all three cases including the fail-open and a multibyte-before-match.

Blast radius on the existing callers is empirically clean. Old vs new over every inline <script> in the checkout: 611 scripts, 11 differences, 0 fail-opens, 0 length mismatches, and no rule flipping in either direction. Cross-checked with acorn as an oracle -- for every blanked character, assert it lies inside a comment/string/template/regex token: 0 violations across 609 parseable scripts, including the 802 KB minified maplibre bundle. Your headline metric checks out too: on compiled.html the old strip kept 463,427 non-whitespace chars against the new 783,238 of 784,077, so old destroyed 40.9% and new keeps 99.9%.

The perf test is a real guard, slightly looser than it reads. Input grows 8x, so linear is ~8 and quadratic ~64 against a bound of 24. Measured: 5.38ms / 59.68ms, ratio 11.09 -- about 2.2x headroom, so it genuinely would catch the O(n^2) regression you describe. It would not catch mild super-linearity (anything up to ~n^1.53 passes), and the Math.max(..., 0.5) floor never engages at these sizes. Two gaps: stripJsComments gained the same CodeContext and has no scaling test (I measured its 8x ratio at 10.32 -- fine, just unpinned), and "a=b/c;" never enters regex mode since b is a non-keyword word char, so the regex path is unmeasured at scale.

Notes

• fonts.ts:38 still carries a private stripCssComments as css.replace(/\/\*[\s\S]*?\*\//g, " ") -- not length-preserving, not string-aware, and carrying the exact content:"/*" bug the new util was written to fix. Worth swapping in a follow-up.
• stripCssComments has no fail-open: an unterminated /* blanks to EOF. Contrived, since that is a CSS syntax error, but it is the false-negative direction and inconsistent with the stated philosophy of its JS siblings.
• stripJsStringLiterals is only safe on comment-free input -- the stripJsComments-first ordering is load-bearing, since when the comment strip fails open the literal strip receives raw comments it does not model and an apostrophe in a comment opens a "string" that blanks real code. It is a public export with no doc-comment saying so; worth one line on the function.
• The residual on split_data_attribute_selector: a composition that displays a split selector inside a JS string still false-fires. Unavoidable without argument-position parsing, and your comments-only call is right, but worth a source comment recording why it deliberately differs from its two siblings -- otherwise the next reader "fixes" the asymmetry and silently disables the rule again. That is precisely what the new test now prevents, which is the best part of this change.

Method

Read the full files at the SHA above; reproductions were executed against the functions ported verbatim from this SHA, and I verified the call sites and the absence-reading rule at source myself. I did not run the repo suite. All eight required contexts are green at this head, so this is a code finding, not a CI one.

-- Rames

[jrusso1020]

Re-verified at 1397e3d61aaa82d8f4b36d67cd8abe37fbf8d7cf. The blocker is resolved, and the split is the right shape. Approving.

What I checked, and how

1. The absence-reader is no longer reachable by the fail-open. stripJsComments is now scanJsComments(source).out (utils.ts:462) with no return source path, and composition.ts:1024 -- the root_composition_missing_duration_source caller -- takes that version. I ran the adversarial input from my last review (a regex the lexer can misread in the same script as a commented-out gsap.timeline) plus nine other shapes against the real utils.ts:

regex with slash class / division chain / regex after return
template literal / string with slashes / block comment      -> comment stripped, gate fires
regex after ) / regex after ] / regex after ident           -> comment stripped, gate fires

No passthrough on any of them. The two inputs that do come back unchanged are an unterminated regex and an unterminated string -- both syntax errors, so the composition would not run at all. I am not counting those.

2. The fail-open landed with consumers that can absorb it. This was the actual question, and the answer is yes. stripJsCode's only two consumers are composition.ts:503 (template_literal_selector) and composition.ts:656 (requestanimationframe_in_composition), and both are presence-readers -- they scan the stripped output for a pattern and emit a finding when it matches. Confirmed by execution: on unbalanced input stripJsCode returns the source unchanged, so a commented-out requestAnimationFrame( becomes visible to the test and produces an extra finding. For a presence-reader that is the fail-closed direction: noisy, never silent. That is exactly the placement the split needed.

3. Blast radius is packages/lint only. I checked the two files outside it that also mention stripJsComments -- packages/core/src/compiler/htmlBundler.ts and packages/producer/src/services/htmlCompiler.ts -- and both carry their own local implementations (stripJsCommentsParserSafe at htmlBundler.ts:676, a private stripJsComments at htmlCompiler.ts:264). Neither imports this one, so the regex awareness cannot reach the bundler or the producer. Your "strict improvement for all 17 call sites" scope is correct.

4. Corpus differential, since "byte-identical" is the load-bearing claim. I ran main's stripper and this head's over every <script> in the repo's compositions and diffed the outputs, then asked whether either side leaves a duration-source signal (gsap.timeline / .animate( / requestAnimationFrame / lottie.loadAnimation) that the other removed:

scripts scanned=611   outputs differ=11
head-leaves-signal-main-stripped=0    head-strips-signal-main-left=0

11 outputs differ and not one of them changes a signal either way -- which is the useful form of "byte-identical", because it is the property the rules actually depend on.

Body corrections check out: the three rule codes are the real ones, and the ) note is accurate -- a genuine regex after ) is untracked, and at this head that shape returns correct output anyway (verified above), so the residual is a comment in the body rather than a defect.

Good split. The general form is worth keeping: when a shared helper gains a degraded path, the question is never "is this default safe" but "safe for which consumer" -- presence-readers and absence-readers want opposite defaults, and the fix here was to stop making one function serve both.

-- Rames