fix(parsers): preserve safe GSAP helper defaults

[miguel-heygen]

Independent parser foundation. Preserves statically safe GSAP helper defaults and refuses computed runtime ownership. 186 changed lines. Split from #2984.

[vanceingalls]

APPROVE — parser foundation delivers on scoped claim (safe Literal/Identifier/arithmetic/object/array defaults preserved; computed/effectful defaults refused via SAFE_DEFAULT_NODES allowlist).

Four P2 follow-ups (no blockers):

• packages/parsers/src/gsapInline.ts:437-439 — isExplicitUndefined only recognizes the undefined Identifier; a call like slam("#a", 1, void 0) binds opts to UnaryExpression void 0 instead of applying the default. JS runtime treats them identically, but the substituted body becomes (void 0).y which downstream selector resolution processes differently from {}. Niche pattern. Add void-op detection or lock current behavior with a test.
• packages/parsers/src/gsapInline.ts:458-463 — the unresolved walkNodes scan doesn't skip non-value identifier slots (unlike substituteParams / isSafeDefaultExpression). A safe default like opts = { at: 1 } whose key matches an earlier param name at trips unresolved=true and refuses inlining. Over-refusal, not incorrect — but inconsistent with sibling walkers.
• packages/parsers/src/gsapInline.ts:441-445 — supportedParams(fn) is recomputed inside inlineHelper for every call site, even though isShapeEligible already validated the helper. Cache on the fn node or on the helpers Map value to avoid O(callSites * paramShape).
• gsapInline.test.ts / gsapParserAcorn.computed.test.ts — no negative-path assertion that helpers with UNSAFE defaults (opts = someGlobal, opts = new X(), opts = a + b()) are actually refused. Existing "effectful or forward-reference" test only covers Date.now() + forward ref; add coverage for MemberExpression call, NewExpression, AssignmentExpression, SequenceExpression to lock the allowlist.

Standards: Fallow audit + Lint + Typecheck + Format all SUCCESS. Diff cleanly scoped to 3 files matching "Independent parser foundation" description.

— Review by Via

[miga-heygen]

R1 — GSAP helper default parameters (head ad82049)

Verdict: Approve (confirming Via's P2s)

Clean extension. The parser now handles function slam(selector, at, opts = {}) { ... } by evaluating safe default expressions when arguments are omitted or explicitly undefined. The safe-expression allowlist (SAFE_DEFAULT_NODES) plus the left-to-right parameter binding check (earlierParams) is the right conservative approach.

Cross-checked Via's findings

1. isExplicitUndefined misses void 0 — confirmed. void 0 is a UnaryExpression with operator: "void", not an Identifier. Vanishingly rare in GSAP helpers but technically a gap. P2.

2. Unresolved scan over-refuses on shadowed property keys — confirmed, and this is a SSOT gap. isSafeDefaultExpression correctly uses isNonValueIdentifierSlot(parent, key) to distinguish property keys from value references. The unresolved check in inlineHelper does NOT — it walks all Identifier nodes and matches against params.slice(0, i) without checking if the identifier is a property key. So function f(x, opts = { x: 1 }) would be incorrectly marked unresolved. The "is this identifier a value reference?" decision is made in two places with different logic.

3. supportedParams(fn) recomputed — confirmed. Called in isShapeEligible (line ~154) and inlineHelper (line ~170). Cache on the function node would avoid re-walking the param list.

4. No negative-path test for unsafe defaults — confirmed. The test covers Date.now() (CallExpression) and forward-reference, but no test for NewExpression, AssignmentExpression, or SequenceExpression as defaults.

Own observations

Test coverage for the positive paths is strong: omitted args, explicit undefined, null passthrough, cross-parameter defaults (end = at), effectful defaults rejected. The null vs undefined distinction (null is kept, undefined triggers default) correctly mirrors JavaScript semantics.

---

Review by Miga

[james-russo-rames-d-jusso]

Reviewed at ad820493.

Parser foundation looks solid — the SAFE_DEFAULT_NODES allow-list is deliberately narrow (Literal / TemplateLiteral / Identifier-of-earlier-param / MemberExpression on Identifier chains only), the unresolved branch in inlineHelper is the right belt-and-suspenders for the safe-eligibility gate, and the tests cover the omitted / explicit-undefined / null / earlier-param cases at the expansion level. Two concerns worth surfacing plus a couple of nits — nothing blocker.

Dangling-helper-call risk. isShapeEligible accepts a helper whose defaults reference earlier params, and safelyDroppable then removes the helper's declaration from ast.body up front. But inlineHelper can still return null at a specific call site if substituteParams can't bind (e.g. slam() against function slam(a, b = a) { tl.to("#x", {}, b); } — call under-supplied, b unbound). In that shape the decl is gone AND the call stays as-is (slam() in the tree with no declaration), which erases both the tween AND the source function that downstream ("Open Code" navigation, source-range lookups, findAllTweenCalls) would recover the shape from. Detail inline.

Test coverage gap on the unresolved branch. The safety net that produces the scenario above is unreachable in the current test file. Add a test that under-supplies args on a shape-eligible helper and asserts (a) call preserved unchanged, (b) helper decl re-emitted or not-dropped, (c) no crash. Also worth adding negative-path asserts for CallExpression / NewExpression / AssignmentExpression / SequenceExpression defaults being REFUSED — per Via's convergent finding.

Convergence with Via's R1 — endorsing all four of Via's P2 items: void 0 should be treated as explicit-undefined; key-shadowing case in the unresolved scan; supportedParams(fn) cache on helper node; negative UNSAFE-defaults tests. Landing my two overlapping ones inline so they anchor to specific hunks.

Downstream wire-contract note for #2986/#2987 reviewers: No exported types change; the behavioral delta is (a) more tweens resolve to provenance.kind === "helper" for the function foo(sel, at, opts = {}) {…} idiom, and (b) helper(x, y, null) where the callee takes opts = {} produces tl.to(sel, null, at) in the synthetic AST — objectExpressionToRecord returns {} for null. Studio consumers must decide whether a zero-property tween is "no motion" (editable) or "unknown motion" (refused). Worth a downstream test.

— Review by Rames D Jusso

[miguel-heygen]

Addressed the complete R1 review set in 07708c5e:

• void 0 now activates defaults exactly like undefined.
• helper shape analysis is cached per function node.
• binding and declaration removal share one call-site check; omitted required params bind to explicit undefined, and spread calls fall back without dropping the helper.
• the inconsistent unresolved scan is gone because every earlier parameter now has a binding; property keys therefore cannot cause false refusals.
• negative tests lock refusal of calls, constructors, assignments, sequences, and ambient members.
• a load-bearing ponytail: comment documents the safe-default boundary.

Evidence: gsapInline.test.ts 29/29, @hyperframes/parsers typecheck, targeted Oxlint/format, full pre-commit Fallow audit. The downstream zero-property/null interpretation remains intentionally unchanged here because JavaScript requires null not to activate a default; I will exercise that contract while validating #2986/#2987.

[miguel-heygen]

Fresh stacked browser evidence for the default-parameter parser path: a scratch fixture using addTween(target, vars = {...}); addTween("#qa-tween-box") was recognized as helper-owned in the real Studio. An attempted Inspector X edit showed the Unroll guidance, sent no GSAP mutation, rolled back to the prior value, and left source unchanged. This exercises default-parameter helper resolution through the downstream ownership gate.

[miga-heygen]

R2 — Helper inlining call-safety (head 07708c5)

Verdict: Approve

All four R1 findings resolved:

1. void 0 support — isExplicitUndefined now detects UnaryExpression with operator: "void" and argument: Literal(0). Tested.
2. Property key shadowing — Fixed by design: the separate unresolved-identifier walkNode check is gone. resolveHelperBindings binds omitted required params to undefinedIdentifier(), so substituteParams always has a binding. The SSOT split between isSafeDefaultExpression and the old unresolved check no longer exists.
3. supportedParams caching — SUPPORTED_PARAMS_CACHE = new WeakMap<object, SupportedParam[] | null>(). Computed once per function node.
4. Negative-path tests — it.each covers call, constructor, assignment, sequence, and ambient member expressions as unsafe defaults.

Additional improvements:

• resolveHelperBindings extracted as standalone binding resolver (clean separation from inlining)
• safelyDroppable now checks bindability — calls with spread or unbindable params prevent the helper declaration from being dropped
• statementHelperCall extracted for reuse
• Spread argument test added

No new findings.

---

Review by Miga

[james-russo-rames-d-jusso]

Reviewed at 07708c5e — delta since ad820493.

All four R1 findings addressed cleanly:

• Dangling-helper-call risk (gsapInline.ts:470) — belt-and-suspenders fix. safelyDroppable now dry-runs resolveHelperBindings per statement-level call and marks any un-bindable helper as unbindable, preserving the declaration when any call site would fail. Suspenders: inlineHelper returns Node[] | null, and expandStatements falls back to the original statement on null. R1's dangling-call scenario cannot happen.
• Test coverage gap (gsapInline.test.ts:147) — it.each now covers Call / New / Assignment / Sequence / ambient MemberExpression unsafe defaults, plus a dedicated spread-args preservation test. The safety-net path was refactored so unresolved-required params synthesize undefined identifiers — the remaining null-return path (SpreadElement) is exercised via expectHelperPreserved.
• SpreadElement in caller args (gsapInline.ts:449) — first line of resolveHelperBindings now bails on any SpreadElement in call.arguments. safelyDroppable runs the same check per statement-level call, so spread calls block the declaration drop.
• SAFE_DEFAULT_NODES ponytail: nit — ponytail: comment added explaining calls / assignments / updates / constructors are excluded because they'd execute author code or resolve at runtime, with a "negative-path tests required" clause for widening.

29 parser tests + typecheck/lint green. Clean execution — looks good from my side, leaving as COMMENTED.

— Review by Rames D Jusso

[jrusso1020]

Approving to satisfy require_last_push_approval at 07708c5e.

What I verified at this head, rather than re-reviewing the diff: all eight required contexts are terminal-green (Semantic PR title, Test: runtime contract, Typecheck, Build, regression, Test, Render on windows-latest, Tests on windows-latest), there is no outstanding change request, and the follow-up passes from both requested reviewers land at this exact SHA.

One correction on the framing: the earlier approval was never dismissed. dismiss_stale_reviews_on_push is false on this branch, so it is still recorded as APPROVED — it simply predates the most recent push, and require_last_push_approval gates on that rather than on staleness.

[vanceingalls]

R2 APPROVE at 07708c5e. All 4 P2 findings from R1 addressed cleanly.

• F1 (isExplicitUndefined) — FIXED. Now matches both Identifier{name:"undefined"} and UnaryExpression{operator:"void", argument:Literal{value:0}}. Test at test.ts:135 asserts slam("#a", void 0) binds the default.
• F2 (unresolved walkNodes over-refusal) — FIXED. The walkNodes-based unresolved scan is gone; resolveHelperBindings now uses substituteParams with the shared isNonValueIdentifierSlot gate. Test at test.ts:172 covers opts = { at: at }.
• F3 (supportedParams cache) — FIXED. SUPPORTED_PARAMS_CACHE = new WeakMap short-circuits on cached lookups, stores both positive and negative results.
• F4 (adversarial UNSAFE-default tests) — FIXED. it.each covers CallExpression, NewExpression, AssignmentExpression, SequenceExpression, plus MemberExpression — all assert expectHelperPreserved (helper NOT dropped).

Bonus tests I noticed: spread arguments rejected, omitted required params bound to synthetic undefined, forward-reference defaults uninlined — all align with the reshaped design. ponytail load-bearing comment at head.ts:227-230 documents the allowlist invariant.

Standards: CI 44/44 green (Format, Lint, Typecheck, Fallow audit, Build, Producer, SDK, CLI smoke, Windows render, preview-regression, regression). Fallow audit PASS, oxfmt-clean, no dependency changes.

— Review by Via