fix(lint): consolidate lint and audit correctness

[miguel-heygen]

What

Consolidates the remaining lint and audit correctness fixes into one modern replacement for #2381, #2319, #2074, and #1897.

• reports duplicate live data-composition-id values, including entity-equivalent values, while ignoring inert templates
• prevents CSS comments inside <style> from creating phantom root tags
• audits only directly painted text for caption visibility, overflow, overlap, invisibility, and occlusion
• preserves the 5% text-occlusion floor
• resolves GSAP targets lexically and distinguishes real proxy instances without treating descriptive proxy labels as identity
• preserves browser-first duplicate attribute semantics

Why

The original PRs address one lint/audit contract from different angles. Keeping them separate leaves overlapping implementations, partial fixes, and inconsistent regression coverage. This replacement preserves every unique valuable behavior while removing false positives and false negatives found during adversarial review.

How

The implementation uses structural HTML parsing and canonical decoded composition IDs, direct text-node ranges instead of aggregate descendant geometry, and lexical GSAP binding/provenance data to identify stable proxy targets. Helper-local proxy declarations receive per-expansion identity; shared outer proxies retain one identity across helper calls.

Test plan

• Unit tests added/updated
• Manual testing performed
• Documentation updated (not applicable)

Validation completed:

• parsers: 820 passed, 5 skipped, 3 todo
• lint: 374 passed
• CLI: 1,749 passed, 2 skipped
• parsers/lint/CLI typechecks
• parser production build
• formatting and git diff --check
• fallow changed-file audit
• independent adversarial review

[vanceingalls]

Runtime-interop + cross-layer contract lens (Rames is covering the other half, test/observability). No prior reviews on head. All 30+ required checks green at 20c5ee2; mergeable_state=blocked is reviewer-gate only. Two independent adversarial passes run over the whole diff before drafting.

Audited: packages/lint/src/rules/gsap.ts, packages/lint/src/rules/composition.ts, packages/lint/src/utils.ts, packages/parsers/src/gsapParserAcorn.ts, packages/parsers/src/gsapInline.ts, packages/parsers/src/gsapSerialize.ts, packages/cli/src/commands/layout-audit.browser.js (read end-to-end); base-vs-head diffs on all seven; source PRs #2381/#2319/#2074/#1897 contract preservation checked.

Trusting: the four test files (skimmed for coverage direction, not full walk).

Absorbed-contract verification (Rule 2 across four sources):

• #2381 proxy overlap identity — preserved. targetHasNoStableIdentity(selector, identity) at gsap.ts:69-74 correctly treats dwell/hold and proxy → … labels as non-identities absent a targetIdentity stamp; comparison at gsap.ts:614-615 uses targetIdentity ?? targetSelector symmetrically for both sides. Asymmetry-that-isn't: outer loop early-skips left with unstable identity, inner loop only compares identities — a right-side unstable identity can never string-equal a left-side stable identity, so the missing symmetric guard is safe.
• #2319 direct-text audit geometry — preserved for the four audits called out in the PR body (overflow / occlusion / invisibility / overlap via isSolidTextBlock), plus lexical-scope preservation via new BlockStatement in SCOPE_NODE_TYPES + expandBody block-wrapping.
• #2074 duplicate composition ids — preserved via the new readDecodedAttr for entity-equivalent detection and isInsideInertTemplate for <template> skipping.
• #1897 CSS-comment phantom root tag — the actual fix (parse via htmlparser2 in parseHtmlStructure) already lives at base (utils.ts:55-106); this PR contributes the regression test at core.test.ts:214+. Correct outcome for a consolidation.

Rule 12 identity-cluster audit: proxy overlap identity (gsap.ts:614) and duplicate composition-id identity (composition.ts:176) are orthogonal concerns keyed on different data (tween-target provenance vs. HTML attribute value) — no cross-contamination, no shared function required. Cluster analysis clean.

Strengths

• gsapParserAcorn.ts:250-311 — collectIdentifierBindingIndex is a genuinely clean two-pass design: declarations first (with per-declaration includeBlocks for var hoisting), reassignments second. The reassignedDeclarations set correctly gates identity emission at gsapParserAcorn.ts:1338-1348 so a rebound let driver = ... doesn't fake stability.
• gsapInline.ts:341-354 — wrapping expanded bodies in their own BlockStatement with tagProvenance is exactly the primitive needed for iteration-local target isolation, and it composes with the new BlockStatement scope entry. Test coverage at gsapParserAcorn.computed.test.ts:75-176 genuinely discriminates the change from a byte-clean revert.
• composition.ts:172-199 — readDecodedAttr addresses a real class of authoring collision (entity-encoded ids sharing runtime identity with plain ids); the <template> exclusion via closeIndex is the right shape.

Findings

important — 1

• cross-rule — entity-decoding is a duplicate_composition_id-only contract (composition.ts:176 vs. context.ts:69, core.ts:43,206, gsap.ts:275, slideshow.ts:33, media.ts:311, utils.ts:124,151,219, plus composition.ts:52,329,444). Before this PR every rule read data-composition-id raw via readAttr, so the codebase was uniformly non-decoding. This PR introduces decoding on exactly one call site. For an input like <div data-composition-id="&#99;1"> and <div data-composition-src="./x.html" data-composition-id="c1">, duplicate_composition_id correctly flags the collision under browser semantics, but unscoped_gsap_selector (gsap.ts:275), collectCompositionIds (utils.ts:219), and every follow-on rule still see two distinct ids and emit messages quoting c1 back to the author. Fix option A: swap all data-composition-id reads to readDecodedAttr (short and load-bearing — the runtime does decode). Fix option B: narrow this rule's entity-equivalent detection to a same-decoded-value check and leave the raw-string world intact, adding a // browser-semantics only here comment so future readers don't propagate the inconsistency. Option A is the honest fix; option B ships less risk if runtime-decoding parity is uncertain.

nits — 7

• layout-audit.browser.js:411 — clippedTextIssue still labels the finding with textContentFor(element) (aggregate innerText). The gate at line 1382 (hasOwnTextCandidate(element) — see next nit) ensures direct text exists so this isn't a false-fire, but a container with direct text "Foo" and a hidden descendant span "Bar" reports text: "Foo Bar", contradicting the PR body's direct-text discipline. clipped_text isn't in the explicit five-audit scope so this is defensible, but the label mismatch is user-visible.
• layout-audit.browser.js:218-226 — hasOwnTextCandidate(el, directOnly) collapses to a single semantic: both directOnly=true and directOnly=false/undefined branches reduce to textContentFor(el, true).length > 0. The parameter is now dead code. Either delete the arg and rename to hasDirectText, or restore the aggregate-fallback branch — leaving the two-mode API as-is invites future readers to assume the modes differ.
• layout-audit.browser.test.ts:1690-1780 — installOcclusionGeometry returns the same rect regardless of whether selected is a text node or the element itself (parent lookup collapses both). By contrast installGeometry (line 1935-2010) uses rangeTextRect which does discriminate via ${id}Text. So an implementation regression on the occlusion path (textClientRects reverted from directTextNodes(el) to [element]) would still leave every new occlusion test green. Testing-quality gap.
• composition.ts:160-168 + 172-183 — isInsideInertTemplate does a linear scan over all tags for every candidate tag: O(N²) on lint. On path-rich SVG compositions (a real HF corpus) N can hit low-thousands. Fast-exit with const hasTemplate = tags.some(t => t.name === "template"); if (!hasTemplate) return false; at the top of isInsideInertTemplate handles the common no-template case in O(1).
• composition.ts:195 — snippet: truncateSnippet(matchingTags[0] ?? "") always cites the first duplicate. If a registry-installed comp accidentally seeds tag 1 and the real meta-vs-root collision is on tags 2 and 3, the snippet points at an innocent bystander. Consider joining the last two matching tags into the snippet, or naming them by index.
• gsapParserAcorn.ts:520-521 — Pass 3 (collectTargetBindings collection-alias) still calls enclosingScopeNodeFromAncestors(ancestors) with the new includeBlocks=true default. Pass 1 (line 431-438) correctly threads includeBlocks = kind !== "var". A var alias = coll[i] inside a block now binds at that block scope instead of the function scope, so tl.to(alias, ...) at outer scope will resolve to __unresolved__. Anti-pattern authoring but a silent semantic drift caused by this PR.
• gsapParserAcorn.ts:455-458 — AssignmentExpression fallback chain hits enclosingScopeNodeFromAncestors(ancestors) when neither the declaration nor the expanded-block heuristic matches; same block-default hazard as the previous nit. Implicit-global-style { card = document.getElementById("x"); } tl.to(card, ..., 0); used to bind at Program and now binds inside {...}.

Adversarial delta (both passes ran independently, no verdict-changing divergence)

• Pass A (clippedTextIssue aggregate; test-stub occlusion parity; hasOwnTextCandidate dead-param; isInsideInertTemplate O(N²); Pass-3/AssignmentExpression block-default hazards) and pass B (cross-rule readDecodedAttr contract skew; snippet-cites-first-tag) both converged on the direct-text label mismatch and each surfaced adjacent gaps a single-pass reviewer would miss. Pass B's higher-severity find (cross-rule contract skew) is folded above as the one important finding. Pass A's claim that inline object-proxy overlap detection is now silently disabled is intended per #2381's stated theme (two independent inline {value:0} proxies are not the same driver — this is the false-positive being removed), so I'm not raising it. Pass B's claim of a parseForHeader/loopSatisfied loop-body-dropping bug is real but pre-existing at base — not introduced by this PR and out of scope; worth a separate follow-up ticket (for (let i=0; i!==3; i++) currently parses to zero iterations and drops the loop).

Verdict: COMMENT — no blockers, but the cross-rule readDecodedAttr skew is worth addressing before merge (or explicitly acknowledging in the PR body) so the entity-equivalent contract holds beyond duplicate_composition_id.

Reasoning: Every source-PR contract is preserved, identity-cluster analysis is clean, direct-text discipline is applied consistently at the four audits the PR body scopes it to, and CI is fully green. The important finding is a cross-rule inconsistency introduced by this PR — recoverable in a follow-up but worth deciding on now while the entity-equivalent contract is fresh.

— Via

[james-russo-rames-d-jusso]

Reviewed at 20c5ee22643331a8bfb25e8735d39e30112870db — consolidates #2381 / #2319 / #2074 / #1897. Adversarial pass over the parser layer via a fresh subagent (multi-file consolidation + parser correctness triggers Rule 15), plus my own read of composition + audit changes.

Verified clean

• composition.ts — duplicate_composition_id rule uses readDecodedAttr (htmlparser2 with decodeEntities: true) so entity-equivalent values (c1 vs c1) collapse to the same key. <template> contents excluded via isInsideInertTemplate. Test coverage in composition.test.ts covers the meta-tag/root collision case cleanly.
• core.test.ts:214 regression test for <style> CSS-comment phantom-tag suppression asserts root_missing_composition_id / root_missing_dimensions / head_leaked_text all suppressed on a valid sub-composition whose stylesheet references <g> in a comment. Fix is load-bearing for any composition file with SVG references in CSS comments.
• layout-audit.browser.js shift to direct-only text across hasOwnTextCandidate / textRectFor / textContentFor / isSolidTextBlock / textOverflowIssues / occludedTextIssue matches the PR body's "audits only directly painted text" scope. The five now-directOnly=true call sites are consistent — text-issue audits now key off the container's own text ink, not aggregated descendants. Preserves the 5% floor via isVisibleElement(element, 0.05) at line 1375.
• gsapParserAcorn.ts — happy-path proxy identity works as advertised for the two invariants the tests exercise: helper-local proxies get distinct per-expansion identity (.test.ts:141-151), shared outer proxies keep one identity across helper calls (.test.ts:153-166). The reassignedDeclarations guard correctly withholds identity when the binding is lexically reassigned.
• gsapParserAcorn.ts — nearestExpandedScopeFromAncestors (line 527-533) is the right primitive shape — an ancestor walk that stops at the nearest provenance-tagged BlockStatement. Used correctly by the target-selector binder at line 457.
• isTransientBrowserError / GSAP inline serializer tweaks look uncontroversial. No pattern regressions in the transient-error test additions.

Findings (2 CONFIRMED, inline)

• 🟠 gsapParserAcorn.ts:1340 — readProvenance(declaration.scopeNode) inspects only the immediate scope node — a proxy declared inside a nested block inside a helper (function foo() { if (x) { const p = {}; tl.to(p, ...) } }) reads scopeNode as the if-block (untagged), collapses instanceIdentity to "", and collides with sibling helper expansions. Reintroduces the false-positive overlapping_gsap_tweens the invariant claims to fix.
• 🟠 gsapParserAcorn.ts:1340 (same call, var variant) — var declarators use includeBlocks=false so scopeNode hoists to the enclosing FunctionDeclaration or Program — never a provenance-tagged BlockStatement. Same collision. Lower realism since HF studio-emitted GSAP is unlikely to use var, but the invariant is stated broadly.

Both findings share a root cause and a single fix: replace the direct read with an ancestor-walk that finds the nearest provenance-tagged BlockStatement. nearestExpandedScopeFromAncestors at line 527 is the primitive that already does this for the target-selector binder — the object-proxy identity path should call it too.

What I didn't verify

• Live regression of any of the four consolidated PRs (#2381, #2319, #2074, #1897) — trusting the test surface additions.
• Full pass over the 116-line CLI layout-audit browser test; I verified structural correctness of the direct-only shift, not each individual test assertion.
• CI is only just green at review time — no re-verification of the 1,749 CLI / 374 lint / 820 parser test counts from the PR body.

Verdict framing

The four semantic aims are cleanly delivered (comp-id dedup, CSS-comment guard, direct-text audits, proxy identity). The two parser findings both target the object-proxy identity path — same fix closes both. Not blocking merge from my side; deferring the stamp decision to the human reviewer while the fixes land.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

Reviewed at 88a933f316a2dfafb238917bad03fe1884dcbfd7 — R2 delta from my R1 (20c5ee22).

R2 delta scope

Two structural changes to packages/parsers/src/gsapParserAcorn.ts + two new tests:

1. nearestExpandedScopeFromAncestors hoisted from nested-helper to file-scope function (line 267-273) — was a private local inside collectTargetBindings; now callable from index-time and query-time code paths.
2. IdentifierDeclaration gains expandedScopeNode?: any (line 57) — captured at index-time in collectIdentifierBindingIndex (line 312) and threaded through the visibility filter (line 288) + the proxy-identity fallback (line 1344-1345).
3. Visibility filter — findVisibleIdentifierDeclaration computes the usage's expandedScopeNode and only accepts declarations whose expandedScopeNode is either undefined (declared outside any expansion) or matches the usage's.
4. Proxy-identity fallback — readProvenance(declaration.scopeNode) ?? readProvenance(declaration.expandedScopeNode) — falls back to the expansion-tagged block ancestor when the immediate scope isn't tagged.
5. Tests — two new tests at gsapParserAcorn.computed.test.ts:153-177 reproducing the exact repros I named in R1: nested-block const proxy inside helper, and var proxy inside helper. Both use the same expectDistinctProxyIdentities invariant helper that guards the R1 shared-outer-proxy test.

Verification

• Nested-block const/let path (R1 finding #1) — the new expandedScopeNode field is populated at index-time for every declarator via nearestExpandedScopeFromAncestors(ancestors). For a const driver = ... inside if (at >= 0) { ... } inside a helper: declaration.scopeNode is the if-BlockStatement (untagged), and declaration.expandedScopeNode is the helper's expansion-wrapper BlockStatement (tagged with kind: "helper"). Line 1344's ?? fallback resolves to the wrapper's provenance, and each helper call spawns a fresh wrapper → distinct expandedScopeNode refs → distinct instance identities.
• var path (R1 finding #2) — var driver inside a helper: enclosingScopeNodeFromAncestors(..., includeBlocks=false) hoists scopeNode to the FunctionDeclaration (untagged). Same ?? fallback lands on the expansion wrapper. ✓
• Existing "shared outer proxy" invariant preserved — outer proxies declared before any expansion have expandedScopeNode = undefined. Visibility filter's !declaration.expandedScopeNode || … short-circuits true; identity computation still reads readProvenance(declaration.scopeNode) = outer-scope provenance → same identity across helper calls. R1's keeps one outer object proxy stable test still passes.
• Visibility filter correctness — a declaration inside expansion A cannot be found by usages in expansion B (A !== B). Prevents cross-expansion binding leaks that would otherwise conflate distinct instances.
• Test coverage — both expectDistinctProxyIdentities calls exercise the exact false-positive overlapping_gsap_tweens scenarios my R1 repros described. TDD-red-then-green as claimed.
• ancestors[index] starts at length - 2 in nearestExpandedScopeFromAncestors — correctly skips the current node and walks ancestors leaf-most first.

Adversarial pass

• Nested-helper expansion (helper A calls helper B, both expanded) — the visibility filter would hide A-declared proxies from B-nested usages, since expandedScopeNode picks B (nearest wrapper). Untested by this PR. Realistic risk depends on whether nested-helper expansions get emitted by HF studio; if the studio flattens helpers before expansion, this stays theoretical. Not blocking — note for the next iteration if that pattern surfaces.
• AST-mutation ordering — expandedScopeNode is captured at index-time. If tagProvenance ever ran AFTER collectIdentifierBindingIndex, indexed declarations would miss the newer tags. Looking at the existing shared-outer-proxy test still passing suggests ordering is tagProvenance → index, so no regression. Worth an explicit code comment on that invariant, but not required.
• declaration.expandedScopeNode === expandedScopeNode uses reference identity — both sides come from the same AST-walk pass (the ancestors array is shared across visitors of that tree). Reference equality is safe as long as no AST cloning happens between passes. Consistent with the existing scopeNode reference-equality usage at line 285.
• Loop-tagged expansions — the identity path at line 1345-1348 already handles kind: "helper" || kind: "loop". Loops-inside-helpers should compose correctly: the nearest expansion is whichever wrapper block is closest.

Verdict framing

Both R1 correctness findings closed with the exact fix shape I suggested — walk up from the declaration's scope to the nearest provenance-tagged block — implemented via index-time capture (better than my query-time-walk suggestion for perf). Tests reproduce my R1 repros and are load-bearing. LGTM from my side.

— Review by Rames D Jusso

[vanceingalls]

R2 re-review at 88a933f316a2dfafb238917bad03fe1884dcbfd7. Runtime-interop + cross-layer contract lens (Rames covering test/observability). Fresh adversarial sub-agent over Rames's two proxy-identity findings and my own R1 nits (Rule 15 — follow-up upgraded to in-scope + parser semantics).

Audited (delta only): packages/parsers/src/gsapParserAcorn.ts (new collectIdentifierBindingIndex, findVisibleIdentifierDeclaration, nearestExpandedScopeFromAncestors, updated collectTargetBindings and tweenCallToAnimation), packages/parsers/src/gsapParserAcorn.computed.test.ts (new regressions at :90-113, :141-207), packages/parsers/src/gsapInline.ts (unchanged from R1 — re-read expandBody + tagProvenance to confirm expansion-wrapper tagging holds).

Trusting: the other 10 files (unchanged since R1's clean read).

R1-finding disposition at 88a933f31

• Rames's 🟠 nested-block helper-local const/let proxy identity collapse (gsapParserAcorn.ts:1340) — RESOLVED. declaration.expandedScopeNode is recorded at index build (gsapParserAcorn.ts:312) via nearestExpandedScopeFromAncestors walking from the declarator up. Proxy-identity computation (:1344-1345) now falls back readProvenance(scopeNode) ?? readProvenance(expandedScopeNode), so an inner untagged block on the lexical scopeNode still resolves through the expansion wrapper above. Regression pinned at .computed.test.ts:153-165 (if (at >= 0) { const driver = { value: 0 }; … }).
• Rames's 🟠 helper-local var proxy identity collapse (same site) — RESOLVED by the same mechanism: var declarators get scopeNode = FunctionDeclaration (via includeBlocks = kind !== "var"), and expandedScopeNode is still the expansion wrapper above. Regression pinned at .computed.test.ts:167-177.
• My R1 important — cross-rule readDecodedAttr contract skew (composition.ts:176) — NOT ADDRESSED at this head (confirmed by re-read; every other data-composition-id reader is still raw readAttr, including gsap.ts:275, utils.ts:219, context.ts:69, core.ts:43,206, slideshow.ts:33, media.ts:311, utils.ts:124,151, composition.ts:52,329,444). Not re-blocking — R1 verdict was COMMENT, and the practical impact stays scoped to inconsistent entity-decoded reporting across rules rather than a correctness bug in duplicate_composition_id itself. Worth a separate follow-up ticket to swap the reads to readDecodedAttr or narrow the entity-equivalent check.
• My R1 nit — Pass 1 VariableDeclarator in collectTargetBindings block-defaulting on var — RESOLVED. :446-447 now threads includeBlocks = declaration.kind !== "var", and .computed.test.ts:90-100 pins the if (true) { var card = document.getElementById(...); } tl.to(card, ..., 0) regression.
• My R1 nit — AssignmentExpression path block-defaulting on implicit-global assignment — PARTIALLY RESOLVED. The common case (let card; { card = document.getElementById("x"); }) now resolves via findVisibleIdentifierDeclaration → the outer let card declaration's scopeNode, since collectIdentifierBindingIndex correctly threads includeBlocks=false for var at :310 and defaults it to true for let/const (so the outer let at Program scope is discoverable regardless of the assignment's block). Regression pinned at .computed.test.ts:102-113 and :194-207. Only the pathological true-implicit-global case ({ card = document.getElementById("x"); } tl.to(card, ..., 0) with no card declaration anywhere) still falls through to enclosingScopeNodeFromAncestors(ancestors) at :470 with default includeBlocks=true and block-scopes. Nit-level residual — unauthored implicit globals aren't real HF-emitted code.
• My R1 nit — Pass 3 (collectTargetBindings collection-alias) block-defaulting on var alias = coll[i] — UNCHANGED. :533 still calls enclosingScopeNodeFromAncestors(ancestors) with default includeBlocks=true, so if (…) { var alias = coll[i]; } tl.to(alias, …, 0) still block-scopes alias. Symmetry with Pass 1 (already fixed) would take three lines. Nit — HF studio scripts don't var alias = coll[i], but the semantic drift is intact from R1.

Adversarial pass (fresh sub-agent, 15 constructed inputs)

Sub-agent walked the fix independently over cases 1–15 (redeclared var collision, sibling-block var, hoisted-fn-in-block, let/var shadow, catch-clause, for-var/for-let, switch-fallthrough, destructuring, sibling-block same-name const, chained assign, Pass 3 var alias, implicit-global assign, param reassign + identity, non-object-init proxy, adjacent tl.to in nested block × two expansions). All Rames-flagged blockers hold. Sub-agent confirmed no new correctness bug reachable from HF-emitted script shapes, and independently rediscovered my Pass 3 (:533) and AssignmentExpression-fallback (:470) nits.

One additional obscure nit surfaced from the sub-agent: after inlineComputedTimelines drops the helper's FunctionDeclaration, a helper-local var driver = {…} clone's scopeNode hoists past the expansion wrapper (since includeBlocks=false skips it) up to Program. If a same-name top-level var driver = {…} exists at Program with a later .start, the descending-start sort in findVisibleIdentifierDeclaration picks the top-level declarator for every expansion, readProvenance(Program) is undefined, and multiple helper calls collapse to one identity (false-positive overlap). Requires the specific combination of a Program-level var driver declared AFTER an inlined helper's var driver — plausibly unauthored HF output but worth pinning if this pattern is emitted by any Studio codepath. Nit.

CI

At review time: 44 checks pass, 0 failure, 8 regression-shards in progress, 1 skipped. All fast checks (Typecheck, Lint, Format, Fallow audit, unit test suites, Preflight, Skills manifest, Test: runtime contract, Studio load smoke, Producer unit, SDK unit+contract+smoke) are green. Regression-shards are still running — no CI-gate failure at this head, but not fully clean either.

Findings

nits — 3, all unfixed since R1 or newly discovered by the adversarial pass

• gsapParserAcorn.ts:533 — Pass 3 collection-alias addBinding(bindings, enclosingScopeNodeFromAncestors(ancestors), name, selector) still uses the default includeBlocks=true. Should mirror Pass 1's threading: const declaration = ancestors.at(-2); const includeBlocks = declaration?.type !== "VariableDeclaration" || declaration.kind !== "var";. Fixes var alias = coll[i] block-scope drift.
• gsapParserAcorn.ts:470 — AssignmentExpression's terminal fallback (after declaration lookup and expanded-scope lookup) still uses the default includeBlocks=true. Only reached when there's no matching declaration anywhere in the file. Cheap to close with enclosingScopeNodeFromAncestors(ancestors, false).
• gsapParserAcorn.ts:275-297 (findVisibleIdentifierDeclaration) — post-inline var orphaning: when the helper FunctionDeclaration is dropped by inline, a helper-local var driver clone's scopeNode walks past the expansion wrapper up to Program; a same-name later Program var driver outranks it in the descending-start sort and collapses helper-call identity. Not exercised by the current test suite. Consider adding the expansion wrapper as a preferred anchor for var clones whose FunctionDeclaration was dropped, or filtering candidates whose expandedScopeNode differs from the usage's when the usage sits inside an expansion.

Verdict: COMMENT — no blockers, Rames's two 🟠 findings genuinely resolved with symmetric regression coverage, and adversarial pass produced no new correctness bug. CI is still spinning up the regression-shards, and the R1 readDecodedAttr contract skew plus three residual nits are worth folding into a follow-up rather than gating this consolidation.

Reasoning: The R2 fix is the right shape — declaration-side expansion-scope tagging closes the block-default hazard for the proxy-identity path without changing ordinary JS scope semantics. Residual nits are cosmetic parity gaps against the same block-default hazard on adjacent code paths; none is a false-positive generator on real HF scripts.

— Via

Reviewed at 88a933f316a2dfafb238917bad03fe1884dcbfd7. Fresh adversarial sub-agent over the R2 delta per Rule 15 (follow-up landed in-scope, parser semantics). CI 44 pass / 0 fail / 8 regression-shards in progress at review time.

[miguel-heygen]

Addressed Via’s cross-rule decoded-ID finding at 3221cc4: every semantic data-composition-id read now uses browser-equivalent decoding across context, root/mount discovery, composition collection, core, GSAP, media, slideshow, and composition rules. timeline_id_mismatch now consumes the decoded composition-ID set instead of reparsing raw attributes. Added an entity-encoded-ID/timeline-key regression. 289 focused rule tests pass; lint package typecheck/lint/format pass.

[vanceingalls]

Approving on behalf of vance for merge. Prior R2 cleared Rames's block-scope hazards + Magi's uniform decoded-attr contract across every data-composition-id reader (context/discovery/core/GSAP/media/slideshow/composition) closed my R1 cross-rule finding at 3221cc4.

[james-russo-rames-d-jusso]

Reviewed at 3221cc4bc6f4b9cc75870bc522909d25c27ee969 — R3 delta from my R2 (88a933f3).

R3 delta scope

Systematic readAttr → readDecodedAttr migration for every data-composition-id read across the lint package. 16 call sites across 8 files:

• context.ts:69 — rootCompositionId populated with decoded value.
• utils.ts:124, 151, 219 — findRootTag (body eligibility + svg-root skip) and collectCompositionIds all decode.
• rules/composition.ts:52, 329, 444, 527, 963 — 5 sites (root/mount detection, timing-attribute exclusion, root-composition dimensions rule, duration-contract rule).
• rules/core.ts:44, 207, 368, 451-452 — describeStudioElement, root_missing_composition_id, host_missing_composition_id, studio_missing_editable_id selector.
• rules/core.ts timeline_id_mismatch — REFACTORED: dropped the ad-hoc regex /data-composition-id\s*=\s*["']([^"']+)["']/gi over raw source, now consumes ctx.compositionIds (which is already populated via readDecodedAttr at utils.ts:219). Cross-context (HTML ↔ JS) key comparison now decoded on both sides.
• rules/gsap.ts:276 — collectCompositionRanges decodes range IDs.
• rules/media.ts:311 — timed-tag exclusion for composition roots.
• rules/slideshow.ts:33 — collectCompositionIdScenes decodes scene IDs.

New test at rules/core.test.ts:804-814 — <div data-composition-id="&#99;1"> + JS window.__timelines["c1"] = tl — asserts no timeline_id_mismatch finding. Would fail against R2's raw-regex approach (raw c1 !== decoded JS key "c1").

Verification

• Comprehensive migration — verified via grep -rn 'readAttr.*"data-composition-id"' packages/lint/src/: zero remaining readAttr calls for data-composition-id. All 16 sites use readDecodedAttr. Complete.
• timeline_id_mismatch refactor — the raw-regex approach in R2 grepped source for data-composition-id="..." literally, so entity-encoded IDs would mismatch even when the JS timeline key matched the DECODED intent. R3 consumes ctx.compositionIds (a Set<string> populated once at context build time, already decoded), avoiding both the regex duplication and the encoding asymmetry.
• Selector output for studio_missing_editable_id — emits [data-composition-id="c1"] instead of the raw [data-composition-id="c1"]. CSS selector matching in browsers uses the DECODED attribute value, so this is BOTH friendlier AND functionally correct — the emitted selector actually matches the intended element when pasted into DevTools.
• Selective decoding preserved — class, style, data-composition-src, data-start, data-track-index still use readAttr (ASCII-only in practice, faster path). The narrow scope of decoded reads keeps the perf cost proportional to what actually needs cross-context semantic matching.
• Test load-bearing — asserts absence of timeline_id_mismatch on the decoded case. Would fail if htmlCompIds fell back to raw-source values.

Adversarial pass

• Coverage of other data-composition-id-comparing rules — searched for any other rule that dedups/compares data-composition-id values across contexts. Beyond timeline_id_mismatch, collectCompositionIdScenes (slideshow.ts) dedups via seen.add(compositionId) — now decoded, so entity-encoded duplicates deduplicate correctly.
• readDecodedAttr cost — parses via htmlparser2 vs the regex-based readAttr. Lint runs at edit/save/CI cadence; the added parse per read is per-tag not per-frame. Acceptable for CI, negligible for local watch mode.
• Regex-drop concern — the R2 rule scanned the raw source; if a data-composition-id value contained an escaped quote (" inside a "-delimited attr, or the attr was '-delimited), the regex might have partial matches. The ctx.compositionIds set comes from a proper HTML parser, so it handles those edge shapes correctly. Small quality-of-parse win.
• readDecodedAttr on data-composition-src — remains readAttr (line 52 & 368 keep readAttr(rawTag, "data-composition-src")). Composition src is a URL; URL-encoded characters have their own decoding chain later; entity-decoding a URL attr would be wrong. Correct division of labor.
• describeStudioElement output — the description string now shows the decoded ID. Consistent with the selector fix and with user-facing readability. No regression.
• My R2 findings — nothing about R3 disturbs the expandedScopeNode + visibility-filter fix from R2. Parser layer untouched. R2 LGTM stands.

Verdict framing

Clean R3 — the semantic-key migration is comprehensive, the refactored timeline_id_mismatch is now a single source of truth (ctx.compositionIds), and one load-bearing test guards the exact cross-context regression class. LGTM from my side.

— Review by Rames D Jusso