fix(player): report and fail closed on runtime delivery errors

[vanceingalls]

Part 2 of 3 in the HyperFrames caption convergence stack. Depends on #3471.

What changes

• Emits application and error events for runtime-data delivery.
• Fails closed when payload cloning or postMessage delivery fails.
• Pins standard and opaque-origin sandbox behavior and reloads the iframe when that policy changes.
• Correlates runtime-data generations so superseded async attaches cannot emit a false applied event.

Review size

607 additions, 56 deletions; 663 changed lines across 19 files.

Verification

Current-head required checks are green, including CI, regression, Windows rendering, preview regression, player performance, and CodeQL. Player coverage includes sandbox reloads, request correlation, delivery timeouts, and clone/postMessage failures.

[miga-heygen]

Reviewed the diff (all 9 files) plus the unmodified context around the changed methods (_deliverRuntimeData, _trySetRuntimeDataDirect, _replayRuntimeData, _runtimeBridgeReady gating) to check whether "fail closed" actually holds end-to-end. Solid direction — the structuredClone guard, the applied/error event pair, and the opaque-sandbox option are all good primitives. Found a few real gaps and one design inconsistency worth addressing before/after merge.

1. _sendControl still fails open when contentWindow is null
packages/player/src/hyperframes-player.ts (_sendControl, ~L552-578):

this.iframe.contentWindow?.postMessage(...)
return true;

If contentWindow is null at call time, the optional chaining short-circuits the whole postMessage call — nothing throws, execution falls through to return true, and for set-runtime-data/clear-runtime-data no runtimedataerror fires. That's exactly the silent-drop this PR is trying to close everywhere else. _runtimeBridgeReady narrows the window (it's only true after a ready message), but it doesn't eliminate it — e.g. iframe torn down/mid-navigation between ready and the next attribute-driven reset, or any host embedding scenario where contentWindow transiently goes null without isConnected flipping false in the same tick. Suggest treating contentWindow == null as a failure for these two actions (return false / dispatch runtimedataerror) rather than treating it as a no-op success.

2. No timeout — a hung or never-delivered message reports neither success nor failure
The whole applied/error signal pair depends on the iframe's runtime actually running and posting back (runtime-data-applied / runtime-data-error in runtimeData.ts → init.ts → runtime-message-handler.ts). There's no timeout anywhere in this stack. If the postMessage is delivered but the iframe hasn't registered its message listener yet (classic race, already called out in the _replayBridgeState doc comment for other control messages), if the handler promise never settles, or if the iframe crashes/unloads mid-flight, the caller gets silence forever — not an error, not an applied event. For an API whose stated goal is "fail closed on runtime delivery errors," an indefinite hang is arguably the worst failure mode since it's indistinguishable from "still pending." Worth at least documenting the guarantee explicitly (fire-and-forget beyond N ms is unconfirmed), or adding a timeout that emits runtimedataerror if neither signal arrives within a bound.

3. Two different failure-reporting mechanisms for the same conceptual error
setRuntimeData() throws synchronously when structuredClone is unavailable or the payload isn't cloneable, but postMessage-delivery failures and handler failures (sync throw, async rejection) are reported asynchronously via runtimedataerror CustomEvent. A caller that wraps player.setRuntimeData(...) in try/catch (the natural reflex given it's documented to throw) will only catch the clone-failure case and will believe delivery succeeded even when it silently failed downstream unless they've also wired up addEventListener("runtimedataerror", ...). Consider documenting this split explicitly in the README (both failure channels, not just the throw), since it's easy to get partial coverage.

4. runtimedataapplied/runtimedataerror carry no correlation id — concurrent updates on the same channel are ambiguous
deliver() in runtimeData.ts fires reportApplied(channel) / reportError(channel, error) keyed only by channel name. If setRuntimeData("captions", A) is followed quickly by setRuntimeData("captions", B) before the first async handler invocation resolves, both in-flight handler calls eventually resolve/reject independently, and both report against the same channel string with no way for a listener to tell which payload the applied/error event refers to. A slow/superseded resolution for A arriving after B was already applied would look like a fresh confirmation of the latest state. The new test (runtimeData.test.ts) only exercises sequential calls (await vi.waitFor(...) between the two setRuntimeData calls), so this ordering case isn't covered. If rapid same-channel updates are a realistic use case (captions certainly sounds like one), consider a monotonic sequence number or generation token in the message payload so late-resolving stale attempts can be ignored/identified by the caller.

5. Sandbox-origin: unrecognized attribute values silently fall back to the more permissive mode
_applySandboxOriginPolicy():

if (policy === "opaque") { this.iframe.sandbox.remove("allow-same-origin"); return; }
this.iframe.sandbox.add("allow-same-origin");

Any value other than exactly "opaque" (including a typo like "Opaque" or "opaqu") falls through to the same-origin-allowed branch — i.e., the less isolated default. For a security-relevant toggle in a PR themed around failing closed, an unrecognized value silently choosing the less restrictive posture is the wrong default direction. Not high severity since the attribute is developer-set and not attacker-controlled in the common case, but worth a defensive console.warn or treating unrecognized non-null values as opaque instead of same-origin.

Nit: _sendControl's catch branch only special-cases action === "set-runtime-data" || action === "clear-runtime-data" for event dispatch; other control actions (play, seek, set-volume, etc.) keep the pre-existing fully-silent failure behavior. That's consistent with the PR's stated scope (runtime-data delivery specifically), just flagging so it's not read as "all control messages now fail closed" — they don't, only these two.

Nice test additions overall (structuredClone-unavailable throw, postMessage DataCloneError → runtimedataerror, sandbox attribute toggling, async apply/reject reporting) — the coverage gap is specifically the concurrent-update-ordering and null-contentWindow cases above, which aren't hit by the current suite.

[miguel-heygen]

Additive exact-head review at 0dced883. Miga already covers the null-contentWindow, no-timeout, fail-open unknown sandbox value, mixed failure channels, and uncorrelated concurrent-update gaps. Two concrete consequences make this a request-changes verdict.

The structured-clone guard and async handler rejection reporting are good primitives, and the full current check set is green.

blocker — dynamic sandbox-origin changes do not change the active document's sandbox. _applySandboxOriginPolicy() only adds/removes the allow-same-origin token (hyperframes-player.ts:281-289), and the new test changes the attribute after the connected player has loaded while checking only the DOMTokenList. HTML sandbox flags take effect when the iframe navigates; changing/removing tokens has no effect on the already-loaded document. A caller switching a live same-origin player to opaque therefore sees the attribute/test say isolated while the composition retains parent-DOM access. Reload/recreate the iframe on policy change, or make the policy immutable/pre-navigation-only, and prove the origin-access boundary in a real-browser test. Unrecognized non-null values should resolve to the restrictive policy, not same-origin.

blocker — a superseded async attach can emit a false applied event. runtimeData.ts:14-21 reports runtime-data-applied whenever the handler promise fulfills, with only the channel attached. The caption consumer's hfCV2Attach intentionally fulfills without applying when its sequence is stale (if (seq !== hfCV2Attach.seq) return). Under rapid A→B updates, stale A can therefore emit “captions applied” after B even though A was discarded, and the host cannot distinguish it. Add a per-channel generation/request token and suppress or identify stale completions; pin out-of-order resolution with a concurrent test.

The null-window and unbounded-no-response paths Miga identified remain independently blocking for a fail-closed delivery contract; I am not duplicating their full write-up here.

— Magi

Verdict: REQUEST CHANGES
Reasoning: The new signals can claim isolation/application that did not actually occur, so the API does not yet fail closed at its security or async completion boundaries.

[vanceingalls]

Re-review requested at 8a7ddcb. The blocking sandbox and async-delivery findings are addressed: live policy changes reload the iframe; unknown values fail closed to opaque; the real-browser fixture proves the origin boundary; runtime messages carry request IDs with stale-completion suppression; null contentWindow and timeout paths emit correlated errors. The CodeQL-safe tag scan now passes Fallow, and the latest formatter-only commit applies the repository Oxfmt style. Focused scanner tests pass 9/9 and player typechecks pass; CI has restarted.

[miga-heygen]

Re-reviewed at 8a7ddcb. Diffed against the previous review's 5 findings, plus checked CI.

1. _sendControl fails open on null contentWindow — RESOLVED
_sendControl now captures contentWindow once, and on !frameWindow for set-runtime-data/clear-runtime-data it calls _rejectRuntimeDataDelivery(..., "Composition iframe is unavailable") and returns false instead of silently no-oping. Same treatment in the catch branch (postMessage throw, e.g. DataCloneError) — both paths now fire runtimedataerror. Covered by hyperframes-player.test.ts: "reports a null iframe window as a delivery failure" and "reports postMessage delivery failures instead of silently dropping runtime data."

2. No timeout in the applied/error signal chain — RESOLVED
_beginRuntimeDataDelivery arms a 10s window.setTimeout per channel that rejects with "Runtime data delivery timed out after 10000ms" if neither runtime-data-applied nor runtime-data-error arrives. Pending deliveries are also proactively rejected on disconnect (disconnectedCallback), src/srcdoc navigation, and sandbox-origin reload (_rejectAllRuntimeDataDeliveries), so a torn-down iframe doesn't just wait for the timeout to fire. Covered by "reports a bounded error when the runtime never responds" (fake timers).

3. Split error-reporting design (sync throw vs. async CustomEvent) — PARTIALLY ADDRESSED
Structurally unchanged: invalid channel and non-cloneable payload (structuredClone throwing, or now-missing structuredClone support) still throw synchronously from setRuntimeData; delivery/application failures after that point still report async via runtimedataerror. What's new is documentation — the README now explicitly calls out "Invalid channels and non-cloneable payloads throw synchronously. Failures after the call returns are reported with runtimedataerror..." and tells callers to listen for both. That mitigates the "easy to miss a path" risk but doesn't unify the two error channels into one contract. I'd call this closed-enough for this PR (it's a design tradeoff, now at least documented) rather than a blocking gap.

4. No correlation ID — RESOLVED
requestId is threaded end-to-end: player generates a monotonic id in _beginRuntimeDataDelivery, passes it through _trySetRuntimeDataDirect/_sendControl → bridge.ts control handler → runtimeData.ts (resolveRequestId honors the caller-supplied id) → back out via runtime-data-applied/runtime-data-error → _takeRuntimeDataDelivery matches on (channel, requestId) before resolving/rejecting, so stale completions from a superseded request are dropped. runtimeData.ts additionally tracks a generation per channel so an in-flight async handler promise from an older setRuntimeData call can't fire reportApplied/reportError after a newer call has superseded it. Covered by "reports only the latest concurrent delivery on a channel" (runtimeData.test.ts) and "ignores a superseded completion and correlates the latest application" (hyperframes-player.test.ts).

5. _applySandboxOriginPolicy treats unrecognized values as permissive — RESOLVED
Switched from a value-specific check to hasAttribute(SANDBOX_ORIGIN_ATTR) — any non-null value (including a typo like "opaqu") now removes allow-same-origin, i.e. fail-closed. Test "treats every non-null sandbox-origin value as restrictive" confirms. This PR also closes the reload gap Via mentioned: attributeChangedCallback now triggers _reloadForSandboxOriginPolicy() when the attribute value actually changes on a connected element, since sandbox flags only take effect on navigation — otherwise toggling the attribute would silently do nothing to a live iframe. There's a real browser-driven test for this (tests/browser/sandbox-origin.ts, wired into player-perf.yml on the load shard) that actually loads a fixture in a real sandboxed iframe and asserts window.parent access is blocked/allowed across default → opaque → typo'd-opaque → removed, rather than just asserting DOMTokenList contents. Good — that's the kind of verification a unit test alone can't give you here (jsdom doesn't enforce sandbox), so I'm reasonably confident this actually works, not just "looks right."

Summary: 4/5 resolved, 1/5 mitigated via documentation rather than structurally fixed (acceptable).

---

CI: Typecheck is red — must be fixed before merge.

packages/player/src/runtime-in-srcdoc.ts(47,36): error TS2345: Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
  Type 'undefined' is not assignable to type 'string'.

This is in the new findOpeningTag helper added by the "refactor(player): simplify runtime tag scanner" commit:

const boundary = lower[index + prefix.length];
if (OPENING_TAG_BOUNDARIES.has(boundary)) {

lower[...] indexing is typed string | undefined under the stricter scripts/tsconfig.json (noUncheckedIndexedAccess), but OPENING_TAG_BOUNDARIES is Set<string>, so .has(boundary) doesn't type-check there even though it presumably passes under the player package's own (looser) tsconfig. Fix is small — e.g. OPENING_TAG_BOUNDARIES.has(boundary as string) is not it; better to widen the set to Set<string | undefined>, or guard with boundary !== undefined && OPENING_TAG_BOUNDARIES.has(boundary). This needs to be fixed and the Typecheck job green before merge.

[vanceingalls]

The remaining exact-head typecheck failure is fixed in 5922da3. The opening-tag boundary set now admits the indexed undefined type without treating it as a valid boundary, preserving the one-pass scanner and its complexity score. Validation: scripts typecheck passed, 9/9 scanner tests passed, Oxfmt check passed.

[miga-heygen]

Re-reviewed at head 5922da3c. Diffed exactly against the last fully-reviewed commit (8a7ddcbb) — the only change since then is a single line in packages/player/src/runtime-in-srcdoc.ts.

1. Typecheck fix — CORRECT

-const OPENING_TAG_BOUNDARIES = new Set([">", " ", "\t", "\n", "\r", "\f"]);
+const OPENING_TAG_BOUNDARIES = new Set<string | undefined>([">", " ", "\t", "\n", "\r", "\f"]);

lower[index + prefix.length] is string | undefined under noUncheckedIndexedAccess (the stricter scripts/tsconfig.json), so widening the Set's element type is the correct, minimal fix — no as cast, no runtime branch added. Semantically it's also sound: the set's members are still only real boundary characters, so .has(undefined) still evaluates to false when the tag name runs off the end of the string (e.g. a truncated <head with no trailing >), which is exactly the "not a valid boundary, keep scanning / eventually return null" behavior the original /<head[^>]*>/i regex had (it also requires a literal > to match). No behavior change, just makes the existing logic type-check. Typecheck is green in CI at this head.

2. New issues since last review — none. The commit-to-commit diff from 8a7ddcbb → 5922da3c is a 1-line, 1-file change (confirmed via gh api .../compare/8a7ddcbb...5922da3c). Nothing else moved, so there's nothing new to flag in hyperframes-player.ts, runtimeData.ts, bridge.ts, or the sandbox-origin path.

3. Prior 5 findings — still resolved. Since none of that code was touched again after the last review confirmed 4/5 structurally resolved (null contentWindow, timeout, correlation ID via requestId/generation, sandbox-origin fail-closed + reload-on-change with a real-browser boundary test) and 1/5 mitigated via README documentation (split sync-throw vs. async runtimedataerror reporting — accepted as a documented design tradeoff, not a blocker), nothing in this commit could have regressed them.

CI is fully green across the board (Typecheck, Test, Build, Lint, CodeQL, the new Verify sandbox origin boundary (load shard only) browser job, all regression shards).

Verdict: looks good to me — no outstanding blockers from my side.

— Miga

[miguel-heygen]

Re-review at exact 5922da3c; my stale change request is closed.

• hyperframes-player.ts:299-319 applies restrictive sandboxing for every non-null policy and reloads the active src/srcdoc; tests/browser/sandbox-origin.ts:12-49 proves default, opaque, typoed-non-null, and removal against real parent-origin access.
• runtimeData.ts:22-50 binds each delivery to channel generation + request ID and suppresses stale handler completions. The player matches (channel, requestId), rejects null windows and postMessage errors, bounds no-response at 10 seconds, and drains pending deliveries on navigation, sandbox reload, and disconnect.
• runtime-in-srcdoc.ts:35-54 keeps the linear tag scanner behavior while widening the boundary set type so indexed undefined remains a non-member; this is the sole delta after Miga's full fix-head pass.

Audited: player delivery lifecycle, runtime data generation/reporting, bridge/message correlation, sandbox reload policy and browser proof, final scanner type fix.

Trusting: unchanged ancillary README/workflow wiring and the fully green current CI/test matrix.

Verdict: APPROVE
Reasoning: isolation and application are now proven/correlated outcomes with bounded failure paths, and the final type-only delta preserves scanner semantics. — Magi

[james-russo-rames-d-jusso]

Rames-lane review at HEAD 7b1e4ff6. Universal never-APPROVE; deliverable is COMMENT for OG to route.

Layering on top of Miga's fix-head audit (5922da3c COMMENTED — "looks good to me, no outstanding blockers") and Magi's approval at the same content SHA. This branch has since been rebased onto main (5922da3c no longer exists in history — the current 7-commit chain terminates at 7b1e4ff6), so Magi's APPROVE is stale under HF OSS require_last_push_approval=true (reviewDecision is null at HEAD, consistent with a stale-carry). Reviewed the current HEAD directly rather than trusting the pre-rebase snapshot.

Fail-closed mechanics I verified at HEAD, symbol-by-symbol:

• _beginRuntimeDataDelivery in packages/player/src/hyperframes-player.ts — arms a window.setTimeout(RUNTIME_DATA_DELIVERY_TIMEOUT_MS) per channel, records { requestId, timeoutId } in _pendingRuntimeData, and — worth calling out — proactively clears the previous timeout on same-channel overwrite (if (previous) window.clearTimeout(previous.timeoutId)). No leaked timer when a rapid A→B on the same channel fires. Correlation via monotonic _runtimeDataRequestId.
• _takeRuntimeDataDelivery — matches on (channel, requestId) strictly (Number.isSafeInteger guard, exact pending.requestId === requestId compare, clears the timer, deletes the map entry). A stale/superseded resolution can't consume the current pending entry. Both _resolveRuntimeDataDelivery and _rejectRuntimeDataDelivery route through this — one code path, one contract.
• _rejectAllRuntimeDataDeliveries — drained on disconnectedCallback ("Player disconnected before runtime data was applied"), on src/srcdoc navigation (the two attributeChangedCallback branches), and on _reloadForSandboxOriginPolicy ("Sandbox policy changed before runtime data was applied"). No forgotten teardown site.
• _sendControl — captures frameWindow = this.iframe.contentWindow once and, for set-runtime-data/clear-runtime-data, rejects on both !frameWindow ("Composition iframe is unavailable") and postMessage throw. Other control actions (play, seek, etc.) retain the pre-existing silent-drop-on-failure behavior — this is intentional scope, not a fail-closed regression Miga already flagged as a scope-note.
• _applySandboxOriginPolicy(reloadActiveDocument) — flipped to hasAttribute(SANDBOX_ORIGIN_ATTR) gating rather than value-string matching, so any non-null value (including typos) resolves restrictive. The attributeChangedCallback for SANDBOX_ORIGIN_ATTR passes this.isConnected && oldVal !== val — only reloads when a live iframe's policy actually changes. _reloadForSandboxOriginPolicy re-navigates the active src/srcdoc. Real-browser test at packages/player/tests/browser/sandbox-origin.ts (wired into player-perf.yml on the load shard) exercises the actual parent-origin access boundary, not a DOMTokenList shape check — the right kind of proof for a security-relevant toggle jsdom can't verify.
• runtimeData.ts deliver — the isCurrent closure captures both generations.get(channel) === retainedData.generation AND handlers.get(channel) === handler. A late-resolving stale handler promise cannot fire reportApplied/reportError after a newer generation supersedes it. nextGeneration increments per-channel; resolveRequestId honors the caller-supplied requestId when it's a positive safe integer, so end-to-end correlation from _beginRuntimeDataDelivery → bridge → deliver → _take* is a single monotonic id.
• runtime-in-srcdoc.ts — findOpeningTag linear scanner with OPENING_TAG_BOUNDARIES: Set<string | undefined> (Magi's/Miga's final delta). .has(undefined) correctly returns false, so a truncated <head with no boundary/> falls through — same behavior as the original /<head[^>]*>/i regex, which also required a literal >. Type-only fix, no semantic drift.

Concern (worth-fixing, not blocker) — superseded-request never settles for the caller's Event listener.

_beginRuntimeDataDelivery clears the previous timer on same-channel overwrite, but that's the ONLY thing that happens to request A when request B replaces it — A's requestId is silently dropped from _pendingRuntimeData without dispatching either runtimedataapplied or runtimedataerror. A caller that awaits A's completion via addEventListener("runtimedataerror", ...) on requestId A will hang forever — the 10s timeout no longer fires (cleared), the runtimeData.ts side won't emit (isCurrent returns false), and the player side never dispatches a "superseded" signal. For rapid successive calls on the same channel (the classic caption use case — a stream of caption-block updates), any caller who tracks per-request settlement will see one settled event per burst and N-1 orphans. Not a fail-closed hole (the newer request's contract still holds cleanly, and the caller has proactively invalidated A by calling B), but the promise-like Event contract exposed by the two custom events is incomplete for anyone who cares about per-request completion. Consider dispatching a runtimedataerror with message: "superseded by newer request" for the previous requestId when _beginRuntimeDataDelivery overwrites, so per-request awaiters settle deterministically. README already documents the split sync-throw vs. async-event failure channels; this would round out the async channel.

Nit — the _runtimeDataRequestId counter is a plain instance number. No wraparound guard. A player instance would need Number.MAX_SAFE_INTEGER (2^53 − 1) rapid setRuntimeData calls to overflow, which is astronomical — mentioning only for completeness; not worth code.

CI at HEAD — 1 required check red, unrelated to this PR.

Test is failing on latest attempt with one assertion failure: useStudioAgentTools > registers nothing when the browser has no WebMCP at packages/studio/src/webmcp/useStudioAgentTools.test.tsx:161 — expected Document to not have property "modelContext". The received value is a fully-hydrated BrowserMcpServer on document.modelContext, which points at test-isolation leakage from an earlier test in the same file/suite (a modelContext polyfill set on document isn't being torn down before this test). This file is not touched by the PR (the PR owns packages/player/*, packages/core/src/runtime/*, .github/workflows/player-perf.yml, packages/player/README.md) — the studio webmcp path is an entirely separate module. Every other required context (Lint, Format, Typecheck, Build, Producer: unit tests, Producer: integration tests, Studio: load smoke) is green on latest attempt; the new Verify sandbox origin boundary (load shard only) browser job is green as well. Re-run Test to clear the flake — I'd be surprised if it doesn't turn green next attempt. If it recurs, that's a separate isolation-bug ticket in packages/studio/src/webmcp/, still not this PR's blocker.

Bookkeeping:

• Magi's APPROVE at pre-rebase 5922da3c doesn't count against HEAD 7b1e4ff6 under HF OSS require_last_push_approval=true. Miguel/Magi will need to re-stamp at current HEAD for the approval to count.
• I read the current HEAD content directly (not the pre-rebase snapshot) — the fail-closed mechanics land intact, and the interdiff on PR-owned files between "what Magi approved" and HEAD is either zero or trivial (Set<string | undefined> typing, formatter pass).

Verdict :large_green_circle: from where I sit — the security/correctness posture change is structurally sound at HEAD, peer-verified end-to-end, and the CI red is an unrelated useStudioAgentTools test-isolation flake in a file this PR doesn't touch. Blockers for merge from my seat: (a) re-run Test to green on latest attempt, (b) fresh approval-stamp at current HEAD, (c) at author's discretion, consider the "superseded-request never settles" completeness note above.

— Review by Rames D Jusso

[jrusso1020]

Read the 19 files plus the surrounding runtime at head 7b1e4ff6a97ed0af7d64f87699585279b45fbe2f. The design is right: generation-stamping the guest deliveries, correlating by request id, and giving every delivery a terminal outcome is the correct shape for this, and the sandbox-origin work defaults to today's behavior so it is opt-in.

One blocking finding, one calibration of an earlier flag, and one note on the CI claim.

---

1. Blocking: guest-local and host request ids share one namespace, which produces the exact false applied the PR says it prevents

runtimeData.ts falls back to a module-local counter when no request id is supplied:

let localRequestId = 0;
function resolveRequestId(requestId: number | undefined): number {
  if (typeof requestId === "number" && Number.isSafeInteger(requestId) && requestId > 0)
    return requestId;
  localRequestId += 1;
  return localRequestId;
}

The player's counter starts in the same place: private _runtimeDataRequestId = 0; then += 1, so its first delivery is id 1. Both sides mint 1, 2, 3, ... into one id space, and _takeRuntimeDataDelivery matches on pending.requestId === requestId alone.

The two-arg call is the documented composition-facing form. entry.ts puts setRuntimeData on window.__hyperframes, and window.d.ts:38 still declares it as (channel: string, payload: unknown) => void. That declaration was not updated by this PR, so the colliding shape is the published one.

Sequence, all on one channel:

1. Host calls player.setRuntimeData("captions", A), mints id 1, arms the 10s timer.
2. The guest handler for A is async and has not settled.
3. Composition code calls window.__hyperframes.setRuntimeData("captions", B) with no id. resolveRequestId returns local 1 and nextGeneration bumps the channel.
4. B applies and isCurrent() is true, so it reports applied("captions", 1).
5. Host matches pending id 1, clears the timeout, dispatches runtimedataapplied.
6. A then rejects. isCurrent() is now false, so the rejection is dropped.

The host is told its payload applied. It failed, and its error is unreportable.

I ran your runtimeData.ts at head under node to avoid arguing this from reading. Taking your own test reports only the latest concurrent delivery on a channel and changing only the superseding call to the two-arg public form:

setRuntimeData("captions", "first", 1);   // host request id 1
setRuntimeData("captions", "latest");     // composition-side call, no requestId

applied reports: [["captions",1]]
host awaiting id 1 was told APPLIED: true

This falsifies two stated invariants:

• PR body: "Correlates runtime-data generations so superseded async attaches cannot emit a false applied event."
• packages/player/README.md: "Only the latest in-flight update for a channel can emit a completion."

The generation check does stop the superseded delivery from reporting. It does not stop the superseding delivery from reporting under the superseded delivery's id, which is the same false applied by a different route.

The existing test cannot catch it because it supplies explicit ids 101 and 102, both host-shaped and both far above the local counter.

Smallest fix that closes it, given resolveRequestId already requires host ids to be > 0:

localRequestId -= 1;
return localRequestId;   // guest-local ids are negative, so they can never match a pending host id

_takeRuntimeDataDelivery accepts any safe integer and compares exactly, so negative local ids fall through to no match with no other change. Worth updating window.d.ts to carry the optional third argument at the same time.

If you consider a composition-side setRuntimeData on a host-driven channel to be out of contract, that is a defensible call, but then the fix is in window.d.ts and the README rather than the counter. Either way the current combination of a two-arg public declaration and a shared id space does not hold the invariant the PR claims.

2. Calibration on the superseded-awaiter flag

A prior review flagged that a superseded request never dispatches runtimedataerror for orphaned per-request awaiters. The mechanism is real: _beginRuntimeDataDelivery clears the previous timeout and overwrites the entry without dispatching anything, and the guest drops it too, so a superseded delivery emits nothing on either side.

I do not think it is actionable as stated, and it is worth saying why so it does not get fixed twice. setRuntimeData returns void and never hands the caller its request id, so no caller can await a specific request through the public API. The events are channel-level, the latest delivery always reports, and the README already documents the invariant in the sentence quoted above. It becomes a live bug the day setRuntimeData returns its request id, not before.

3. The verification claim is no longer true at head, and it is not your fault

PR body: "Current-head required checks are green, including CI."

That was true when written. CI run 32944541317 at this exact head was green on 08-26. CI run 33142742985 at the same head failed on 08-28, and the check list currently shows Test red.

The failure is not from this PR and is not a flake:

• The only failing test is packages/studio/src/webmcp/useStudioAgentTools.test.tsx:161, expect(document).not.toHaveProperty("modelContext"). This PR touches no file under packages/studio.
• The 08-26 run passed because that file did not exist yet. It 404s at that run's base 4f00336c and is added in the base advance to e5c7dc75, so the two runs are not comparable evidence.
• #3511 added that assertion on 08-27, #3514 added a polyfill that installs document.modelContext when the browser has none, and the two contradict. main itself went red on it: runs 33130451269 and 33130004847 both failed on that same file.
• da6514d458 ("fix(studio): update WebMCP test for polyfill fallback", 08-28 03:34) fixed it on main, and main has been green since.
• This PR's CI base e5c7dc75 is 7 commits behind that fix, so it inherited a breakage that was already repaired upstream.

Practical consequence: this is deterministic rather than intermittent, so updating the branch onto current main is what clears it. Worth not spending a re-run on it.

---

Requesting changes on the id namespace in 1. Nothing else in the diff blocks: _rejectAllRuntimeDataDeliveries copies before iterating, the direct-bridge path falls through to postMessage correctly on the cross-origin throw, the structuredClone hard failure matches the fail-closed intent and is documented, and sandbox-origin treating any non-null value as opaque is the right direction for a typo to fail in.

Miguel's approval is at 5922da3c24 rather than the current head, so it will need a fresh pass regardless.

— Review by Rames

[jrusso1020]

Re-reviewed at f2c885294f4d29e7e21e5a3af1840f1023aebc59. The blocking finding is fixed, the fix is correct end to end, and the test that ships with it genuinely catches the old behavior. Approving.

1. The id-namespace defect is closed

3beb0f63b8 mints guest-local ids downward, into a space host ids cannot reach:

localRequestId -= 1;
return localRequestId;

I re-ran the same falsifying sequence against your source at this head rather than reading it:

applied reports: [["captions",-1]]
host awaiting id 1 was told APPLIED: false

Last pass those two lines read [["captions",1]] and true. The composition-side delivery now reports under -1, which no pending host entry can ever match.

I checked the whole return path for anything that would choke on a negative id, since a sign-based fix usually leaks at the first validator downstream of it:

• _beginRuntimeDataDelivery pre-increments from 0, so host ids are always >= 1.
• _takeRuntimeDataDelivery gates on typeof requestId === "number" && Number.isSafeInteger(requestId) with no positivity test, then compares exactly. A negative id passes the gate and falls through to no match, which is the outcome you want.
• bridge.ts and runtime-message-handler.ts pass requestId through untouched. Nothing on either path tests sign.

So the two spaces are disjoint by construction rather than by convention, which is the version that survives someone editing one side later.

Widening window.d.ts:38 to (channel, payload, requestId?) closes the other half of it. The two-arg call being the published shape is what made the collision the documented path rather than a misuse, so the declaration mattered as much as the counter.

2. The new test actually pins the fix

Worth its own heading, because a regression test that passes both before and after is the common way this goes wrong. I transcribed never reports a composition-side delivery under a pending host request id and ran it against both heads, with both modules fetched unpatched (the module has zero imports, so nothing needed adapting):

OLD head 7b1e4ff6: reportedId=1   expect(reportedId).not.toBe(1) => FAIL
NEW head f2c88529: reportedId=-1  expect(reportedId).not.toBe(1) => PASS

It fails against the code it was written for. That is real coverage.

One non-blocking suggestion: not.toBe(1) pins this regression but not the rule. expect(reportedId).toBeLessThan(0) would pin the invariant your comment states, and would still catch a future change that happened to mint 2.

3. What the host now sees on that sequence

For the record, since this is the fail-closed path. The superseded host delivery emits nothing from the registry, which is what I verified above. Its terminal outcome comes from the player's own 10s timer, armed by _beginRuntimeDataDelivery and cleared by nothing on that path, so the host gets runtimedataerror with the timeout message instead of a false success. I read that half rather than executed it. A late error beats a wrong success, so this is the right direction to be wrong in.

The superseded-awaiter point from my last pass is unchanged and still not actionable for the same reason: setRuntimeData returns void and never hands out its request id. Repeating it so it does not get fixed twice.

4. Stale base cleared

The branch is current with main (behind_by: 0), so the inherited useStudioAgentTools.test.tsx failure is gone. That was the deterministic-not-flake item from last pass, and merging main was the thing that cleared it.

5. Two notes on the body, neither blocking

• Review size is stale by exactly the fix commit. It reads 19 files / 607 additions / 56 deletions; at this head it is 20 / 631 / 57. window.d.ts is the twentieth file.
• Verification says current-head required checks are green. At this head everything has concluded green or skipped except two regression shards still running when I checked, and nothing failed. Test in particular went green, and step 12 of that job (bun run --filter '!@hyperframes/producer' test) puts packages/core in scope, so the new case really did execute in CI rather than only on my machine: runtimeData.test.ts (7 tests) passed, up from 6.

Nothing else moved. The merge of main did not pull anything unrelated into the PR's own diff: still the same 20 runtime-data and player files.

— Review by Rames