fix(studio): reject unsafe keyframe values

[miguel-heygen]

Problem

Keyframe and arc-motion edits can derive non-finite layout values from the live preview. If those values reach GSAP mutation serialization, source HTML can be corrupted with invalid layout data and the playhead can jump away from where the user is editing.

Fixes #1382.

What this fixes

• Adds a Studio-side finite numeric guard before GSAP mutations are POSTed.
• Shows the existing Studio toast with Couldn't read element layout — try again at a different playhead time and aborts the mutation when a non-finite value is present.
• Adds server-side mutation protection for unsafe scalar values, including null, because JSON.stringify(NaN) and JSON.stringify(Infinity) become null.
• Uses the soft-reload path for convert-to-keyframes, so the existing applySoftReload __player.getTime() restore keeps the playhead in place for keyframe conversion.

Root cause

The mutation boundary trusted numeric values collected from the live preview. JavaScript can produce NaN/Infinity, and JSON silently serializes those values as null, so direct writes could mutate source without an explicit validation failure. Separately, keyframe conversion used the normal reload path even though soft reload already preserves the current player time.

Verification

Local checks

• bun run --filter @hyperframes/core test -- src/studio-api/helpers/finiteMutation.test.ts src/studio-api/routes/files.test.ts
• bun run --filter @hyperframes/studio test -- src/utils/finiteGuard.test.ts src/utils/gsapSoftReload.test.ts
• bunx oxfmt <touched files>
• bunx oxlint <touched files>
• bun run --filter @hyperframes/core typecheck
• bun run --filter @hyperframes/studio typecheck
• Lefthook pre-commit: filesize, lint, largefiles, format, fallow, typecheck

Browser verification

• Started a real Studio preview for /tmp/hf-1382-studio-project.
• Used agent-browser to open http://localhost:5190, record the flow, read the live GSAP animation id, and submit an update-property mutation whose value was Number.NaN before JSON serialization.
• Confirmed the live Studio API returned 400, reported body.value, left the source unchanged, and did not write null or NaN into index.html.
• Screenshot: artifacts/1382/studio-loaded.png, artifacts/1382/studio-after-rejected-nan.png (local verification artifacts)
• Recording: artifacts/1382/nan-guard.webm (local verification artifact)

Notes

• This is a guard at the destructive boundary, not a full root-cause fix for the nondeterministic arc-motion layout race.
• The fresh worktree has unrelated LFS pointer noise in packages/producer/tests/webm-transparency/output/output.webm; it was not staged or committed.

[james-russo-rames-d-jusso]

hyperframes#1389 — fix(studio): reject unsafe keyframe values — Studio-side finite guard + server-side null/NaN/Infinity rejection at the GSAP mutation boundary. CI green across the board (Test, Typecheck, Lint, Format, Render/Tests on windows-latest, all regression-shards, all Perf:*). Approach is sound — guard at the destructive boundary, error toast on the client, 400 on the server — and the test coverage is right-shaped for both packages.

Concerns

• packages/studio/src/utils/finiteGuard.ts vs packages/core/src/studio-api/helpers/finiteMutation.ts — client/server validators have diverged semantics. Server's findUnsafeMutationValues flags null (correctly, since JSON.stringify(NaN) → null). Client's findNonFiniteNumericFields does not. So a mutation containing an explicit null field passes the client guard, hits the server, gets a 400 unsafe numeric values — and mutateGsapScript in packages/studio/src/hooks/useGsapScriptCommits.ts:65-74 only checks if (!res.ok) return null and bubbles up as Mutation failed: ${mutation.type} thrown from commitMutation. The user sees no toast in this branch — the inline showToast only fires on the client-side guard hit. Either align the client helper to also flag null (export a shared findUnsafeMutationValues and call it from both sides), or teach mutateGsapScript to read the server's 400 body and surface the same toast on the unsafe numeric error code. The latter is the more durable shape — server is the authority on what's safe.

• Stack-merge collision with #1366. #1366 (stack base, merges first) heavily refactors packages/studio/src/hooks/useGsapScriptCommits.ts (80/89 lines) — extracts ensureElementAddressable/PROPERTY_DEFAULTS to gsapScriptCommitHelpers.ts, rewrites mutateGsapScript to throw createStudioSaveHttpError instead of returning null, and wraps commitMutation in useSafeGsapCommitMutation which .catch()-swallows all errors to telemetry. After #1366 lands, this PR's commitMutation definition will need to fold its non-finite guard into the new file shape; the throw new Error(...) branch becomes a swallowed telemetry event (toast still fires inline, so UX is preserved, but the gsap_commit save_failure telemetry will fire a false positive save_failure event every time the guard rejects). Flag for Miguel: when rebasing on top of #1366, either short-circuit before commitMutationSafely so telemetry isn't polluted with guard-rejections, or tag the telemetry event with a reason: "non-finite" discriminator. Otherwise dashboards from #1366 will get noisy.

• packages/studio/src/hooks/useGsapScriptCommits.ts:130-141 — guard runs inside the inner commitMutation, but commitMutation is called by callers wrapped in executeOptimistic (drag/resize via gsapRuntimeBridge.ts → gsapDragCommit.ts). executeOptimistic.persist() catches the thrown error, rolls back the optimistic apply, and console.warns. In that flow the user gets toast + rollback — fine. But commitMutation is also called directly from many places (updateGsapMeta, deleteGsapAnimation, etc., all wrapped in void commitMutation(...).catch(...) patterns that don't exist yet on main). Right now on main, a thrown error from updateGsapProperty's debounced flush becomes an unhandled promise rejection because flushPendingPropertyEdit does void commitMutation(...) with no .catch. After #1366 the commitMutationSafely wrapper covers this, but until then the order matters. Worth confirming this PR is merged AFTER #1366 (Graphite should handle that; just calling it out).

Questions

• Scope clarification: the PR title says "reject unsafe keyframe values" and the diff guards the gsap-mutations/* route, but file-mutations/patch-element/* (called from useGsapScriptCommits.ts:289-305 for autoId patches and from useDomEditCommits for DOM-attribute edits) has no equivalent guard. The PR body says "guard at the destructive boundary, not a full root-cause fix," so I'm assuming file-mutations/* is deliberately out of scope here — confirm? If a downstream PR is planned, no action needed; otherwise the same NaN/Infinity corruption window exists on that route.

• packages/studio/src/hooks/useGsapScriptCommits.ts:137-139 — when options.skipReload is true and the guard fires, the function silently returns (no throw, just showToast + return). Is the optimistic-update layer for skipReload-callers (gesture/drag) expected to know that "no exception" means "no apply" here, or do they get into a state where the local apply persists without the server write? Reading useGestureCommit.ts and useEnableKeyframes.ts the callers pass softReload not skipReload, so likely a non-issue, but the asymmetry between "throw on normal path" and "silent return on skipReload" is worth a comment.

Nits

• packages/core/src/studio-api/helpers/finiteMutation.ts + packages/studio/src/utils/finiteGuard.ts — same findNonFiniteNumericFields exists in both packages with subtly different default path argument ("body" vs "mutation"). Consider exporting from @hyperframes/core/studio-api/helpers/finiteMutation and re-using on the studio side; eliminates the divergence vector from Concern #1 mechanically. (nit)

• packages/studio/src/hooks/useGsapScriptCommits.ts — the dropped JSDoc comment on ensureElementAddressable (deleted in this diff) was useful context. Looks like a rebase artifact; #1366 moves it to gsapScriptCommitHelpers.ts with the JSDoc intact, so this is harmless once #1366 lands. (nit)

• Test coverage gap: packages/core/src/studio-api/helpers/finiteMutation.test.ts covers null and NaN/Infinity on simple shapes, but doesn't cover the -Infinity or Number.NEGATIVE_INFINITY path explicitly. Number.isFinite handles all three correctly so this is purely defensive — add a one-liner if you want belt-and-suspenders. (nit)

What I didn't verify

• Runtime/SDK/player semantics of how a soft-reload after a convert-to-keyframes mutation interacts with the current playhead — that's Vai's lane; flagging there.
• Whether showToast is reliably non-null in all rendering contexts of useDomEditSession (e.g. embedded vs. standalone Studio). The prop is required upstream in UseDomEditSessionParams so should be fine, but I didn't trace every mount site.
• agent-browser verification artifacts in PR body — local-only, I trust the description.

Review by Rames D Jusso

[james-russo-rames-d-jusso]

Re-classifying per author's request to evaluate long-term-vs-bandaid framing.

PR body says "guard at the destructive boundary, not a full root-cause fix." That's bandaid scope by definition. To land as long-term, one of:

1. Root-cause fix: identify + fix the studio code path that creates NaN / Infinity values in the first place. Guard stays as defense-in-depth, but the underlying bug doesn't recur.
2. Defense-in-depth fully committed: extend the boundary guard to all destructive routes (currently file-mutations/patch-element/*, autoId patch, DOM attribute edits have no equivalent guard), share findUnsafeMutationValues between @hyperframes/core and @hyperframes/studio (kill the client/server divergence where server flags null and client doesn't), and surface server-side 400 as a toast.

Current shape covers one route with two diverged validators and a path where the user sees no feedback on guard rejection. See my earlier comment review for the full file:line breakdown.

Review by Rames D Jusso

[james-russo-rames-d-jusso]

R2 verification — Approve-with-concerns. HEAD fa029db8 (was 6b41ec2b at R1) materially escalates this from bandaid to long-term defense-in-depth. All three R1 escalation paths landed on the same shape (Path 2: defense-in-depth fully committed). One R2-NEW heads-up + one carry-over question; neither is a blocker.

R1 long-term-vs-bandaid resolution

• Root cause: ❌ — PR body still reads not a full root-cause fix for the nondeterministic arc-motion layout race. Not the chosen path.
• Defense-in-depth fully committed: ✅ — chosen path, executed cleanly.
• PR-body re-framing: ⚠️ — PR body still says guard at the destructive boundary, not a full root-cause fix. Now that defense-in-depth covers both routes with a shared validator and surfaces server 400s as toasts, the framing under-sells what landed. Worth a one-line edit so the reviewer-next isn't misled by the original framing.

Findings details

• Validator divergence (client null flag): ✅ resolved. New packages/core/src/studio-api/helpers/finiteMutation.ts exports findUnsafeMutationValues + findUnsafeDomPatchValues with package export @hyperframes/core/studio-api/finite-mutation. Both useGsapScriptCommits.ts:3-6 and useDomEditCommits.ts:2 import the shared helpers. Single source of truth — null is flagged identically on client and server. The studio-local finiteGuard.ts is gone from the diff.
• All-destructive-routes coverage: ✅ resolved. packages/core/src/studio-api/routes/files.ts:940-943 guards file-mutations/patch-element/* with findUnsafeDomPatchValues; the GSAP route at :1103-1106 guards with findUnsafeMutationValues. Studio-side mirrors the same guards in useDomEditCommits.ts:184-189 (autoId path also re-guarded at useGsapScriptCommits.ts:285-294). The allowNullPath regex (/^body\.operations\[\d+\]\.value$/) is tight — only the explicit-style-removal slot allows null; nested nulls inside value.color etc. still get caught because the path differs. No bypass.
• 400 surfaced as toast: ✅ resolved. mutateGsapScript no longer swallows to null — it throws GsapMutationHttpError carrying statusCode + responseBody. commitMutation catches and calls showToast?.(formatGsapMutationRejectionToast(error), "error") which renders Couldn't save animation: <server error> (<offending fields>). useDomEditCommits.ts follows the same shape via formatPatchRejectionMessage. User now gets actionable feedback on guard rejection regardless of which side caught it.
• Stack-merge collision with #1366: ✅ direction flipped. gsapScriptCommitHelpers.ts (containing ensureElementAddressable, PROPERTY_DEFAULTS, plus new GsapMutationHttpError / readJsonResponseBody / formatGsapMutationRejectionToast) is NEW in this PR and does NOT exist on main. #1366 is still OPEN — so this PR now lands ahead of it, and #1366 will need to rebase onto a tree that already has the helper extraction. The useSafeGsapCommitMutation telemetry-pollution concern from R1 becomes #1366's rebase problem (they'll need to decide whether to special-case the guard's throw new Error(\"Mutation contains unsafe values: ...\") so it doesn't fire save_failure). Heads-up for Miguel to flag on #1366's thread when rebasing.
• convert-to-keyframes softReload: ✅ — useGsapScriptCommits.ts:500 now passes softReload: true per PR body's claim, so applySoftReload restores __player.getTime() and the playhead stays where the user was editing.

R2-NEW finding

• useGsapScriptCommits.ts:108-117 — when the guard rejects and options.skipReload is false, commitMutation throw new Error(...). The toast already fired (good), but flushPendingPropertyEdit at :194-205 calls void commitMutation(...) with no .catch, so the throw becomes an unhandled promise rejection on the debounced property-edit path. Same shape as the pre-existing throw new Error(\"Mutation failed: ...\") so it's not a regression introduced here — just expanded surface (one more throw site reachable via the same uncaught path). Cheap fix: wrap the inner void commitMutation(...) in .catch((err) => { /* already toasted */ }), or move the catch into flushPendingPropertyEdit. Not blocking — fires only on NaN/Infinity/null values, which are the exact cases this PR exists to gate, but it'll show up in Sentry as noise. Worth a follow-up; can land in #1366's rebase since that PR rewrites this very function.

Carry-over question (R1, still standing)

• useGsapScriptCommits.ts:115 — skipReload asymmetry preserved: throws on the normal path, silent return after toast on the skipReload path. Looks intentional and the only skipReload: true caller in tree is addGsapProperty's autoId-resolution sub-call, which immediately re-issues the real mutation — so the silent-return is correct. A one-line comment near :116 (// skipReload callers retry with a different value; toast is the only signal we owe them) would settle the rubber-duck question.

CI: 8/8 regression-shards still IN_PROGRESS at time of review; all other required checks (Test, Typecheck, Lint, Format, CodeQL, Render windows-latest, Player perf, Preview parity, CLI smoke, Studio load smoke, etc.) are SUCCESS. Wait for regression-shards green before stamping.

Stamp-ready? Yes once regression-shards finish green. The R2-NEW unhandled-rejection note is non-blocking — flagging for follow-up. PR-body re-framing nit is non-blocking. R1 request-changes is resolved on the substantive escalation criteria.

Review by Rames D Jusso

[vanceingalls]

LGTM

[jrusso1020]

Approved.

[miguel-heygen]

Merge prep update: conflicts are resolved and the branch is git-mergeable, but GitHub still reports reviewDecision=CHANGES_REQUESTED from the earlier james-russo-rames-d-jusso review. The later approval from jrusso1020 does not clear that requester in branch policy. Please approve/dismiss from the original requesting account so this can merge once checks finish green.