fix(core): harden audio FX and group identity

[vanceingalls]

Part 1 of 12 replacing #3439. Review this PR against main; every later PR is based on the previous stack branch.

Why

The remaining audio work needs a stable core contract before preview, rendering, or Studio can consume it. This slice keeps the model and its tests together instead of distributing exports across directory-sized buckets.

What

• harden audio-group identity across inlined composition instances
• align group membership, volume, hidden state, and carve-source resolution
• keep the pitch-shift worklet and FX codec changes with their tests
• retain the existing solo predicates as a compatibility bridge; PR 11 removes them after every consumer is gone

Verification

• core, core runtime, engine, lint, Studio, producer, and CLI TypeScript checks
• fallow audit --base main --fail-on-issues
• audioGroups.test.ts (26 tests)
• oxfmt and oxlint on the compatibility bridge

The final stack tip preserves the verified #3439 replacement and includes the review fixes landed across the stack.

Stack: #3444 → #3445

[jrusso1020]

Reviewed at 9afb4a4a84 against main. No prior reviews on this PR, so nothing here is additive to a peer.

Verdict up front: one thing to fix before #3445 lands, plus two notes. The FX-codec refactor and the pitchshift work are good, and the CI "reds" flagged on this PR are a demonstrated flake rather than a regression — details at the bottom so they don't bury the finding.

isMemberGroupHidden is instance-blind, and #3445 is what makes it live

This is the one that matters, and it lands on the PR's own headline — "harden audio-group identity across inlined composition instances."

resolveAudioGroups resolves a member's bus the new way:

const groupId =
  member.getAttribute(AUDIO_GROUP_RENDER_ID_ATTR) ?? member.getAttribute(HF_AUDIO_GROUP_ATTR);

isMemberGroupHidden, twelve lines up in the same file, resolves it the old way:

const groupId = el?.getAttribute?.(HF_AUDIO_GROUP_ATTR);   // author id only

On a stamped document those disagree, because resolveGroupElement matches the author id against MEDIA_RENDER_ID_ATTR and instance 1's stamp is its author id — so every member of instance 2 resolves instance 1's bus. I ran the shipped modules against this PR's own two-instance fixture, with only instance 2 muted:

bus render ids   : [ 'bed', 'bed__hf2' ]
bus hidden       : [ false, true ]
member group keys: [ 'bed', 'bed__hf2' ]

resolveAudioGroups (render):
   id=bed       members=["m1"] hidden=false
   id=bed__hf2  members=["m2"] hidden=true      <- correct

isMemberGroupHidden (preview):
   m1: author="bed" stamped="bed"      -> resolves bus "bed" -> hidden=false
   m2: author="bed" stamped="bed__hf2" -> resolves bus "bed" -> hidden=false   <- wrong

m2's bus is muted and the render drops it; this reader says it is audible. That is the same preview-vs-render split resolveGroupElement's own docblock describes closing — "the render came to drop a hidden group's members while the preview fallback played them at full level" — surviving in the sibling reader.

It is unwired in this PR, and #3445 wires it into the mute decision at two sites:

el.closest("[data-hidden]") !== null || isMemberGroupHidden(el.ownerDocument, el);

So this is latent here and live one rung up. I am filing it here rather than on #3445 because the one-line fix belongs next to the reader it mirrors:

const groupId =
  el?.getAttribute?.(AUDIO_GROUP_RENDER_ID_ATTR) ?? el?.getAttribute?.(HF_AUDIO_GROUP_ATTR);

I have graded it a comment rather than a changes-request on this PR because the function is added-only and reaches nothing at this head. I will hold #3445 on it, since that is where it becomes reachable — flagging that now so it is not a surprise.

Same function, smaller: an author id goes into a selector unescaped

doc?.querySelector?.(`${HF_AUDIO_GROUP_TAG}[${MEDIA_RENDER_ID_ATTR}="${groupId}"]`)

groupId reaches this from a raw data-audio-group value, so a composition written as <audio data-audio-group='vo"x'> makes the selector unparseable. Executed against linkedom:

"vo"    -> ok, null
"vo\""  -> THREW: Attribute selector didn't terminate
"vo\"]" -> THREW: Unmatched selector: "]

A throw here is out of character for a module that documents itself as degrading gracefully and tolerating partial Element doubles — the surrounding code returns null for every other malformed input. Cheapest fix is to skip the stamped lookup when the id contains a quote or backslash, or run it through CSS.escape where available. Hand-authored HTML only, so this is a robustness nit, not a security one.

Three test guards left, none mentioned in the body

The Verification section lists audioGroups.test.ts (26 tests), which I checked and is exact — 21 → 26, a real net gain. Underneath that, three guards go:

• canary.test.ts, the whole the audio rollout block. Its own comment is the argument against removing it: "Pinned as a test because the failure mode is silent: re-registering one of these re-hides a shipped feature, and nothing else would say so." Both assertions still compile and pass at this head — CANARIES and findCanary are still imported and still used elsewhere in the file, so nothing forced this. I checked the whole stack: no rung touches any canary file, canaryRegistry.ts is byte-identical from main through the tip of #3455, and the guard is not restored anywhere. So nothing is being re-hidden and this is a coverage regression rather than a defect. Worth keeping the prefix-based half at least — it was the derived guard, not a hardcoded roster.
• returns null for the group element even when it carries the attribute — deleted outright, no replacement. It pinned "groups do not nest / a group element is not a member of itself", which still holds via audioGroupOf's tagName === "audio" check.
• resolveAudioGroups(document)).toEqual([]) dropped from the empty-attribute test during its rename.

Each is small. The reason to raise them together is that they are elective, they are the kind of guard that only pays off years later, and a Verification section quoting a test count is the place a reader would expect the removals to be mentioned.

Checked and correct

• The Perf: scrub / player-perf reds are a flake, and the job proves it rather than my inferring it. Attempt 1 failed at the step Run bun run build — not at a measurement — with TS2306 ... 'runtime-inline.ts' is not a module on three generated files. Those files are not committed (packages/core/src/generated/ holds only position-edits-render-inline.ts), so they are emitted during the build and engine's tsc raced ahead of them. Two of the three error sites are pre-existing imports this PR does not touch. Attempt 2 is green on Perf: scrub, Perf: fps and Perf: load at the identical head. Same commit, same job, opposite outcome. All 8 required contexts are green.
• The node-codec extraction is behaviour-preserving, and I checked it field by field rather than by shape. Two latent improvements come with it: presetAmount: NaN used to serialize to null (old code emitted it because typeof NaN === "number" && NaN !== 1) and is now omitted, and the value is clamped on the way out as well as in. Only reachable via an in-memory node, since parsing already clamps.
• The pitchshift bypass reasoning holds. The fast path keeps filling the ring while dry, so a later shift does not start cold, and wet/wetTarget ramping in both directions is what lets it reach true bypass rather than latching — which is the thing that would have made preview carry a 50 ms offset the render does not. The added tests cover both directions plus the warm-up, and the ~50 ms latency note in the effect description is the right call given nothing in the graph compensates for it.
• couldBeCarveBed / isNamedCarveBed — the split between what may be offered and what may be self-applied mirrors the existing source-side split, and unknown staying eligible for both is consistent with couldBeCarveSource.

— Review by Rames (pr-review), James's assistant

[vanceingalls]

Addressed the review feedback in the latest stack push:

• hidden-state lookup now honors stamped render IDs for repeated composition instances
• group lookup compares raw render-ID attributes instead of interpolating IDs into selectors
• restored the audio canary, explicit group-element exclusion, and empty-membership guards
• added regression coverage for selector metacharacters and repeated-instance mute state

Validated with the core typechecks and 132 focused core tests.

[jrusso1020]

Re-review at 62b6d35bdd. My earlier review (5003519240) was pinned to 9afb4a4a84; one commit since — 62b6d35b "address audio group review feedback".

The finding is closed.

isMemberGroupHidden now resolves the way its three siblings do

packages/core/src/audioGroups.ts:117-127:

const groupId =
  el?.getAttribute?.(AUDIO_GROUP_RENDER_ID_ATTR) ?? el?.getAttribute?.(HF_AUDIO_GROUP_ATTR);

Same expression as resolveAudioGroups, so all four readers that map a member to its bus now agree. The new test pins it on the exact fixture the finding was written against — two bed instances, only the second muted, m1 → false and m2 → true. That is the assertion, not a proxy for it.

The selector-injection half is closed too

I raised this as a cross-reference on #3446 rather than as a finding here, so I want to credit it explicitly. resolveGroupElement no longer interpolates an author-controlled id into a selector string:

const stamped = Array.from(
  doc?.querySelectorAll?.(`${HF_AUDIO_GROUP_TAG}[${MEDIA_RENDER_ID_ATTR}]`) ?? [],
).find((candidate) => candidate.getAttribute(MEDIA_RENDER_ID_ATTR) === groupId);

Raw attribute comparison, so quotes and backslashes are values rather than syntax — and matches a stamped id containing selector syntax without throwing uses vo"\instance, which is the payload that actually reproduced it. #3446's own docblock establishes that this value "reaches this file straight from the document" unvalidated, so this closes it at the reader rather than at one call site.

The canary deletion is out of the PR

packages/core/src/canary.test.ts is no longer in the diff, and describe("the audio rollout") is intact at head (:439-453). Both guards — the three named retired canaries and the audio- family check — survive. Worth saying because the earlier revision removed the tests that would notice one of them being re-registered.

Checked because the fix relocated work, and it is fine

resolveGroupElement went from one indexed querySelector to Array.from(querySelectorAll(...)) plus a .find, i.e. a document scan and an allocation on every call. That is the usual hidden cost of this shape, so I looked rather than assumed: isMemberGroupHidden has zero non-test callers at this head (#3445 is what puts it on isSilencedByHidden, once per media element per sync), and the scanned set is stamped <hf-audio-group> elements — buses, not clips — so it is bounded by the composition's bus count and discarded immediately. Not a problem. Recording the negative so nobody re-derives it.

Nothing open from me on this rung.

— Review by Rames (pr-review), James's assistant

[jrusso1020]

Re-review at 62b6d35bdd — the head has not moved since review 5003703198, so that review is still pinned to the exact head and there is nothing new to read.

Restating the close for the record: isMemberGroupHidden now prefers the compiler stamp over the author id and is tested on the two-instance fixture, the selector-injection throw is fixed at the reader, and the canary.test.ts deletion is out so the audio-rollout guards survive.

No blockers.

— Review by Rames (pr-review), James's assistant

[jrusso1020]

Approving at 62b6d35bdd, the exact head of my re-review above — that review is the audit; this is the stamp <@vanceingalls> asked for in Slack.

No blockers on the code. The one nit I raised on #3445 (the mock gain defaulting to 1, so the toBe(1) solo assertions are weaker than they read) is non-blocking and that code is deleted at #3454.

Scope of this approval, stated so it is checkable: it covers 62b6d35bdd71fe709d261d56323971004491757a only. main runs require_last_push_approval: true, so if this head moves the PR returns to REVIEW_REQUIRED rather than carrying my stamp forward — re-ping me and I will re-review at the new head.

— Review by Rames (pr-review), James's assistant

[miguel-heygen]

Independent exact-head pass at 62b6d35.\n\nThe identity repair is coherent across the boundary: mediaRenderIds.ts stamps each bus instance and its members with the same document-unique key, while audioGroups.ts prefers that key in both resolution paths and compares raw attributes instead of interpolating selector syntax. I also checked the pitch-shift warm-up/bypass transition and the FX codec/affordance integration; the new state is cleaned up at its owning boundaries and the compatibility solo predicates remain until their consumers leave.\n\nThe current required check set is green, including a latest successful regression run; the older cancelled-shard rollup is superseded.\n\nVerdict: APPROVE\nReasoning: The core audio/group contract is internally consistent, its earlier instance and selector gaps are closed, and no blocking issue remains at this exact head.\n\n— Magi