fix(studio): make sdk cutover transactional

[jrusso1020]

What

Make Studio SDK cutover edits transactional and replace the ambiguous boolean fallback contract.

Why

A persistence or history failure after mutating the live session could return false and then execute the legacy backend too.

How

Return declined/committed/failed, edit a candidate session, persist and record history first, then publish the live session; only declined may fall back.

Test plan

• Studio cutover failure-injection and exactly-one-backend tests
• Stack-wide lint, format, build, typecheck, and relevant integration gates

[jrusso1020]

• refactor(repo): resolve changed-code audit #2174
• test(engine): isolate peer abort regression #2173
• refactor(cli): decompose render phases #2172
• refactor(cli): centralize process lifecycle #2171
• fix(cloudrun): publish adapter contract #2170
• test(repo): execute packed subpaths #2169
• refactor(repo): centralize package subpaths #2168
• refactor(core): own edit protocol contract #2167
• test(compiler): enforce preview render parity #2166
• refactor(studio): gate SDK operation families #2165
• refactor(studio): gate Acorn writer migration #2164
• refactor(producer): make capture plans immutable #2163
• refactor(producer): unify render requests #2162
• refactor(engine): type capture failures #2161
• refactor(engine): manage child process lifecycles #2160
• refactor(engine): add fingerprinted browser leases #2159
• refactor(render): centralize execution ownership #2158
• refactor(core): unify composition contract #2157
• fix(studio): enforce optimistic file concurrency #2156
• fix(studio): make sdk cutover transactional #2155 👈 (View in Graphite)
• fix(render): publish artifacts atomically #2154
• fix(render): surface structured outcomes #2153
• fix(player): own connection and media resources #2152
• fix(core): preserve runtime transport contract #2151
• fix(player): version runtime protocol #2150
• test(repo): verify packed consumers #2149
• test(repo): enforce workspace contracts #2148
• fix(core): enforce strict runtime safety #2147
• test(producer): gate source tests by execution lane #2146
• fix(core): make timing masks safe for source loaders #2145
• chore(repo): forbid tracked generated artifacts #2144
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[james-russo-rames-d-jusso]

Inline finding — see the consolidated review for the full summary + nits.

[james-russo-rames-d-jusso]

Inline finding — see the consolidated review for the full summary + nits.

[james-russo-rames-d-jusso]

🟡 Perf / dead-read: sdkTimingPersist here (plus sdkTimingBatchPersist at L144 and dispatchGsapOpAndPersist at L255) each do their own captureOnDiskBefore BEFORE calling persistSdkCandidateMutation, and pass the result in as originalContent. But inside persistSdkCandidateMutation at sdkEditTransaction.ts:261-266, originalContent is only used as sourceSnapshot ?? originalContent — and since these entry points always pass a truthy sourceSnapshot (the sdkSession.serialize() on the line above), the ?? always short-circuits and the outer undoBefore is dead.

Net effect: one wasted readProjectFile HTTP round-trip OUTSIDE the mutex per timing tick — timing edits fire on every mouseup, so it adds up.

Not a correctness bug (the inner captureOnDiskBefore inside the mutex is the authoritative one). Suggest dropping the outer capture and passing just serializedBefore as sourceSnapshot; persistSdkCandidateMutation already owns the disk read where it belongs.

— Rames D Jusso

[jrusso1020]

Addressed in 0f2cb32. Removed the pre-lock disk reads from sdkTimingPersist, sdkTimingBatchPersist, and the GSAP transaction path; persistSdkCandidateMutation remains the sole authoritative read inside the per-path queue. The undo-baseline tests now assert exactly one read, and the no-longer-public capture helper was made private.

[james-russo-rames-d-jusso]

Reviewed at 517f277f20dea23ee8643fd05d8f905776ecce9d. (My initial fetch was at c60841380d0e73aa3ef1e524969a35375ceb1a7a; the PR was rebased between then and now — producer-side artifactTransaction.ts / artifactTransaction.test.ts / the renderOrchestrator hunk I originally traced through no longer show in this PR's diff because they landed on main via a sibling merge. This review focuses on what the PR head currently contains: the Studio-side transactional cutover.)

(Five short reviews of mine above this one carry one inline finding each — that batch shape is a tooling artifact from before the rebase became visible to me; each is a real substantive finding, see them inline in the diff.)

What I verified

Ran an independent read of the transactional core (sdkCutover.ts + sdkEditTransaction.ts), walked every caller of the tri-state API across the modified hook/component files, and dispatched a parallel adversarial pass over both the transactional invariants and the caller adaptation. Verified the mechanism claims:

• Commit ordering is write → history → publish → refresh, tested via it.each at sdkCutover.test.ts:404 (6 op types) and the ordering-verifying test at L470.
• cutoverCommittedOrThrow(...) throws on failed, so no caller can silently fall through to the legacy backend after a partial-mutation-then-persist-error — that's the exact bug this PR retires.
• Concurrent same-path writes serialize via serializeCandidateMutation (per-(writeProjectFile, path) mutex, WeakMap keyed on writer identity — writeProjectFile from useFileManager.ts:79 is a useCallback([]), so identity is stable across renders). Verified end-to-end at sdkCutover.test.ts:540.
• 15 caller sites in the diff use cutoverCommittedOrThrow(...) (or its ! inversion → fallback pattern) correctly. Workspace grep turns up no un-migrated callers.

Concerns (inline above)

None rise to blocker. Two highest-value findings are the "silent-failure" ones — the tri-state contract itself is right, but the caller-side adaptation to the new throw semantics is incomplete in a handful of fire-and-forget UI handlers, which turns some real failure modes invisible-to-user (matching PR intent for no-double-write, but losing the toast).

Summary of the 5 inline findings:

• 🟠 useGsapAnimationOps.ts — 5 hooks + their handlers in useGsapSelectionHandlers.ts:139-169 throw on failed without a .catch.
• 🟠 useGsapPropertyDebounce.ts — 4 non-flush handlers + their handlers in useGsapSelectionHandlers.ts throw on failed without a .catch. Fix pattern already exists in this PR at the flush seam (onFlushError).
• 🟠 DesignPanelPromoteProvider.tsx / VariablePromoteContext.tsx — right-click promote and setDefault call void persist(...) which swallows the failed-throw. VariablesPanel.tsx correctly wraps its persist in try/catch + toast; the design-panel path doesn't.
• 🟡 sdkCutover.test.ts — missing prod-mode writeProjectFile rejects test with a real candidate. The existing it.each at L404 covers recordEdit failing; the primary invariant (mutate-then-persist-fails → live untouched, no publish, failed result) isn't pinned with a real openComposition case.
• 🟡 sdkCutover.ts — outer captureOnDiskBefore in sdkTimingPersist/sdkTimingBatchPersist/dispatchGsapOpAndPersist is a dead read (short-circuited by sourceSnapshot ?? inside persistSdkCandidateMutation). One wasted HTTP round-trip per timing tick.

Nits (not inline)

• markSelfWrite fires BEFORE its writeProjectFile on both commit (sdkEditTransaction.ts:170-171) and rollback (L189-192). Intentional per the registry comment (sdkSelfWriteRegistry.ts:32-34) to beat the SSE-echo race, but a rollback failure leaves two stale hashes in the 2s TTL window. Real-world hit chance is low; worth an inline comment referencing the registry rationale so a future refactor doesn't "fix" the ordering.
• sdkCutoverPersist (DOM patch entry point) lacks the defensive try/catch its peers have. sdkTimingPersist, sdkTimingBatchPersist, and dispatchGsapOpAndPersist all wrap persistSdkCandidateMutation in try/catch and wrap thrown errors as {status: "failed"}. sdkCutoverPersist doesn't (the inner buildCandidateEdit catches synchronously, so no active hazard). Defensively-consistent addition; low priority.
• declinedCutover(reason: string) is stringly-typed. No callers switch on reason, so no active hazard. If future callers want to differentiate declines, tighten to a union of the ~8 concrete reasons emitted.

What I didn't verify

• Interactive studio smoke test — didn't spin up a full session in this review turn. All verification is static + adversarial-subagent-driven, backed by the existing 65+ test cases including the three critical transactional paths (recordEdit-throws, publisher-throws-post-commit, concurrent-overlapping-edits).
• Producer-side artifactTransaction — no longer in this PR's diff. Traced through it before the rebase became visible; it looked structurally analogous to the Studio pattern (staged sibling = candidate, encode+validate = persist+record, atomic rename = swap) with a documented weaker guarantee for the directory-replacement case. If the merging PR is still open, happy to review it separately.
• Cross-repo consumers of the exported CutoverResult / CutoverDeps types — grepped the workspace, no external hits.

LGTM from my side aside from the caller-adaptation concerns (silent-failure GSAP handlers + void persist in the promote provider). Clean transactional design; the tri-state contract itself holds.

— Review by Rames D Jusso

[jrusso1020]

@miguel-heygen Thanks for the adversarial pass — all three blockers are fixed on the current head (d0da6ea02).

1. Authoritative read failures now fail closed. When a production readProjectFile is supplied, its rejection propagates through the transaction as failed; the stale live-session fallback is used only when no reader exists. The regression test commits A, rejects B's in-lock read, and asserts B performs no write and cannot erase A.
2. Whole-file writers now share the same per-writer/per-path transaction queue. persistSdkSerialize takes an in-lock builder, rereads authoritative bytes after acquiring the queue, derives after from those bytes, and keeps write/history/rollback inside that lock. Slideshow and arbitrary-file variable writes use that path. Regressions cover overlapping whole-file transforms preserving A+B and whole-file/candidate interleaving on the same path.
3. Session publication is path- and generation-aware. Transactions publish { candidate, expectedSession, targetPath }; useSdkSession accepts only when the active path and current live-session identity still match. A rejected candidate is disposed without touching the newer session. The delayed-A / switch-to-B regression asserts A cannot replace or dispose B.

Additional validation:

• fix(studio): make sdk cutover transactional #2155 layer: full Studio suite green (190 files passed, 1 skipped; 2,146 tests passed, 18 todo), plus Studio typecheck.
• Fully restacked top: 107 focused cutover/session regressions green; full Studio suite green (194 files passed, 1 skipped; 2,156 tests passed, 18 todo).
• Full workspace production build, all-package typecheck, lint, format, and all 78 repository script tests green.

Fresh GitHub CI is running on the submitted head. Ready for re-review once you have a moment.

[vanceingalls]

Review from the resolver-shadow/cutover-telemetry side (I own dashboard 1732039 and the divergence classes this contract interacts with). Direction is right — the boolean contract genuinely conflated "declined" with "failed after mutation," and the candidate-session + publish-ownership model fixes the double-apply hazard cleanly. The slideshow rewrite (read-modify-write inside the per-path queue) also fixes a real lost-update. Four findings, one substantive:

1. Pre-write failures now hard-fail edits that used to self-heal via the server path (main ask)

buildCandidateEdit maps ANY mutation error to failed (sdkEditTransaction.ts catch → {status:"failed"}), and captureOnDiskBefore read errors do the same (persistSdkCandidateMutation line ~281). cutoverCommittedOrThrow then throws — no server fallback.

But at both of those points nothing is durable yet — no write, no history entry. Falling back to the legacy backend is provably safe there, exactly as safe as the old contract. The "only declined may fall back" rule is needed for post-write faults (history-fail, rollback-fail), not pre-write ones.

This matters in practice because the candidate is now cloned from on-disk bytes while the eligibility gates check the live session (sdkSession.getElement(hfId) etc.). Any live/disk skew — an external agent writing the file, a reload race — passes the gate and then throws inside the candidate mutation. Old behavior: caught → false → server path patches by selector → user's drag succeeds. New behavior: hard failure surfaced to the user. From telemetry I can tell you live/disk id skew is a real, recurring state, not a hypothetical (it's the element_not_found divergence family).

Suggestion: classify pre-persistence errors as fallback-eligible — either declined with reasons like candidate_mutation_failed / source_read_failed, or a third pre-write status that cutoverCommittedOrThrow treats as fallback. Keep hard-failed for writeAndRecord/rollback faults where durability is ambiguous.

2. captureOnDiskBefore silently lost its never-throws contract

The old implementation documented and guaranteed "Never throws — a failed read degrades to the serialized fallback." The moved copy in sdkEditTransaction.ts removed the try/catch (and the doc line). It's now exported and called directly by sdkTimingPersist/sdkTimingBatchPersist. If the throwing behavior is intentional (it feeds finding 1), the doc comment should say so; if not, restoring the degrade-to-fallback behavior removes one branch of finding 1 for free.

3. Publish-declined leaves the live session stale with its reload suppressed

In commitCandidateEdit, when publishSession returns false the write + history are already committed and markSelfWrite(targetPath, after) has already suppressed the file-change reload for that exact content. The surviving live session never learns the new bytes: subsequent edits stay correct (the queue re-reads disk), but every in-memory consumer — selection, panels, resolver-shadow parity checks — runs against a stale document indefinitely. Narrow window (session swapped concurrently / comp switched), but the fix is cheap: on published === false for the still-active comp, schedule a session reload (forceReload) instead of only disposing the candidate.

4. Mutation queue is keyed by writeProjectFile function identity

serializeCandidateMutation uses WeakMap<writeProjectFile, Map<path, …>>. Cross-hook serialization therefore only holds if every call site passes the same stable function reference. Nothing enforces that — a hook wrapping the writer in a fresh closure (the way readProjectFile: (path) => readFileContent(…) is inlined today) would silently opt out of the queue and reintroduce the lost-update race this PR closes elsewhere. Since one studio instance has one project, a module-level map keyed by path alone would be strictly safer; failing that, an invariant comment on CutoverDeps.writeProjectFile ("must be referentially stable — it keys the write queue") would at least make the contract visible.

---

Nit: the committed/failed telemetry blocks are now copy-pasted into every sdk*Persist; an opLabel param on persistSdkCandidateMutation could emit once, centrally — and would also give the cutover dashboard a uniform per-op success/failed/declined breakdown, which pairs nicely with the sdk_resolver_shadow_attempt denominator we already collect.

🤖 Generated with Claude Code

[jrusso1020]

Follow-up on the latest review round:

• The four timeline move/resize rollback paths now show an error toast, preserve rollback behavior, and still rethrow. Regression assertions cover all four paths.
• Async GSAP/property fire-and-forget handlers now share rejection observation with telemetry and one user-visible toast. GSAP save failure handling is consolidated, including keyframes, without duplicate request-layer toasts.
• Variable promote and set-default persistence failures are caught and surfaced through the Design Panel provider.
• The redundant outer pre-lock reads were removed. The remaining on-disk capture helper is private and explicitly documents that reader/parse failures abort the transaction before any write.
• A rejected same-path publication caused by expected-session mismatch now schedules a live reload so a newly installed same-path session cannot remain stale; a path-switch regression confirms that an obsolete path does not reload the new project.
• The writer dependency now documents the stable/shared function-identity requirement used to scope the per-path queue.

I intentionally did not add selector-snapshot fallback when the pre-write disk capture or parse fails. That would weaken the strict fail-closed behavior requested in the earlier blocker: if the live selector snapshot is stale relative to disk, fallback can publish stale state after a write. The transaction therefore aborts before writing and reports the failure instead.

Validation on the final restacked head is green: workspace build, all package typechecks, repo script tests, and the full Studio suite (196 files passed, 1 skipped; 2,163 tests passed, 18 existing todos).

[miguel-heygen]

Re-review at 13f440e86bf8422125f0c5a5e7fdec645f084d6a

The previous three blockers are materially addressed: authoritative reader failures now fail closed, persistSdkSerialize joins the candidate queue and rebuilds from in-lock bytes, and publication is guarded by both path and expected-session identity. The caller-adaptation round also lands cleanly: timeline, GSAP/property/keyframe, and variable-promote failures are now observed and surfaced, and the requested real-candidate write-failure test is present.

Audited end-to-end: all 34 changed files (+1,682/-472) and deletions; transaction/cutover core; every tri-state consumer; session ownership/publication; project-bound I/O/history; GSAP SDK/legacy coordination; slideshow/variable full-file writers; timeline and toast/error adapters; every existing review/comment and the PR-body claims. Trusting: unrelated unchanged Studio subsystems and non-focused packages beyond their changed integration points.

Blocking findings

1. [P1] A queued edit can switch projects before its authoritative read/write and mutate the newly active project. serializeCandidateMutation keys only on (writeProjectFile identity, targetPath) (packages/studio/src/utils/sdkEditTransaction.ts:65-95), then performs the authoritative read and write/history later (:275-296, :193-218) without a project/generation check. Production's stable I/O callbacks dereference the current projectIdRef on every invocation (packages/studio/src/hooks/useFileManager.ts:49-50,69-96), and recordEdit similarly delegates to the current history store (usePersistentEditHistory.ts:352-354). The publication guard runs only after disk/history commit, so it cannot protect durable state.

   A deterministic exact-head proof queued two A/index.html transactions behind a held first writer, switched the shared project ref to B, then released the queue: both returned committed, and B/index.html received the second A gesture. Common projects all sharing index.html makes this reachable, not a synthetic key collision. Bind transactions to project identity (queue key plus fail-closed generation checks before read and before write/history), or pass project-bound I/O/history closures; add delayed-A → switch-to-B-same-path coverage asserting no B read/write/history.

2. [P1] Published candidates escape the session effect's ownership and leave a stale expected session alive across path changes. The open effect only records its originally opened composition in local compRef; cleanup disposes/clears that object (packages/studio/src/hooks/useSdkSession.ts:168-205). Publication replaces sessionRef.current with the candidate but never updates the effect's owner (:209-243). After a successful publish, changing A→B therefore leaves candidate A in sessionRef until B finishes opening (and leaks it entirely on unmount). During that window, an edit rendered for B can receive stale session A; if IDs overlap, the guard accepts it because active path is B and currentSession === expectedSession (both A). The transaction writes B, then the already-started B open can install its pre-edit snapshot and dispose the just-published B candidate; the self-write echo is suppressed, so memory/preview stays stale. Track ownership by path/generation and make cleanup clear/dispose the current session owned by that generation; test publish-A → switch-to-B → edit-before-B-open and unmount cleanup.

3. [P1] The whole-file transaction boundary still excludes legacy GSAP RMWs when they race non-GSAP SDK writes. GSAP SDK callers take both the module queue and the legacy serializer (sdkEditTransaction.ts:299-302; useGsapScriptCommits.ts:282-295,341-370), but timeline/style/delete/variables callers take only the module queue; timeline batch deps, for example, omit serialize (useTimelineGroupEditing.ts:188-206). A declined GSAP operation can therefore run under the independent legacy serializer while a timeline/style SDK transaction is active. The server mutation route reads, awaits parsing/mutation work, then writes without a per-file lock/CAS (packages/studio-server/src/routes/files.ts:941-979), so either stale RMW can overwrite the other. Use one (project,path) coordinator for every Studio RMW/rollback and legacy request, or enforce server-side CAS/serialization; pin a legacy-decline + SDK-style/timing interleaving test.

Important follow-up

• Rejected publication still refreshes the current iframe with the rejected candidate's bytes. commitCandidateEdit disposes a rejected candidate but unconditionally calls refreshCommittedEdit(edit.after, ...) (sdkEditTransaction.ts:242-263). Path mismatch is one rejection case (useSdkSession.ts:209-227); GSAP's refresh then applies stale A script text to previewIframeRef.current, which may now be B (useGsapScriptCommits.ts:319-338). Suppress target-sensitive refresh when publication was rejected for another path; same-path generation rejection already schedules an explicit reload. The delayed-switch test currently asserts publication/disposal only, not that A can never refresh B.

Verification and merge state

• Focused exact-head tests: 6 files, 104/104 passed.
• Full @hyperframes/studio suite: 192 files passed, 1 skipped; 2,153 tests passed, 18 todo.
• Studio typecheck: passed.
• GitHub checks: all completed green/skipped at the exact head.
• The PR is currently CONFLICTING / DIRTY against main; after fixes it needs a rebase and fresh current-head CI.

Verdict: REQUEST CHANGES

Reasoning: The addressed feedback substantially improves the candidate transaction, but project switches, published-session ownership, and cross-coordinator writes still leave reachable paths that can mutate the wrong project, lose a concurrent edit, or install stale runtime state. The current head also conflicts with main.

— Deepwork

[jrusso1020]

Valid

[jrusso1020]

Addressed all four findings on the new head:

• File readers/writers and edit-history callbacks are now bound to the project that created them; optimistic file-version state is project-scoped too. Delayed project-A work cannot read, write, or record history in project B.
• SDK sessions now have explicit project/path/reload-generation ownership. Published candidates stay owned by that generation, stale sessions are hidden immediately on project/path switches, and cleanup disposes the currently owned candidate.
• SDK and legacy GSAP whole-file mutations now use one shared per-project/file coordinator, so a legacy fallback cannot race SDK style/timing/delete/variables writes. Explicit GSAP ordering keys add ordering without bypassing the file lock.
• Publication now returns an explicit active-vs-inactive rejection. A commit rejected because its target is inactive disposes its candidate and skips target-sensitive preview refresh; same-target generation replacement forces a reload.

Regression coverage includes delayed A → B callbacks, cross-project history isolation, publish A → switch B before B opens, published-session unmount cleanup, inactive refresh suppression, and legacy GSAP waiting behind an SDK write. Validation: Studio typecheck, repository pre-commit gates, and the full Studio suite (2,352 passing tests). The full #2155–#2174 stack was also rebased/restacked onto main at e05debe and resubmitted.

[james-russo-rames-d-jusso]

Reviewed at dff0c1df81a001af7465aaf9b1af8f02ff716f10 — critical-thinking pass with Magi R2 (4697719100), my R1 (4696833443), and Via's R1 (4696768227) all in scope. Framed as "given four independent reviewers converge, what class of bug do we all have blind spots for?"

Magi R2 P1 verification

P1-1 — queued edit switching projects (RESOLVED). useFileManager.ts was refactored so readProjectFile / writeProjectFile / readOptionalProjectFile now close over projectId via useCallback deps ([projectId]) rather than dereferencing projectIdRef.current at each call. writeProjectFile additionally captures writeProjectId = projectId and gates the post-write setEditingFile on projectIdRef.current === writeProjectId. usePersistentEditHistory added storeProjectIdRef + activeProjectIdRef and recordEdit throws if the current active project doesn't match the closure-captured project. This closes the durable-write leak Magi's deterministic proof exploited — an in-flight writeProjectFile from project A now targets A's disk (correct), and B's iframe/state is never poisoned with A's bytes.

P1-2 — published candidate ownership across path change (RESOLVED). useGsapScriptCommits.ts now captures activeProjectId at commitMutation invocation (const activeProjectId = projectIdRef.current at hook body top) plus mirrors activeCompPath into a ref (activeCompPathRef.current = activeCompPath). runCommit/runBatchCommit were parameterized to take (pid, compositionPath, targetPath, ...) — captured at invocation, not read from ref at call time. finalizeSuccessfulMutation guards twice — at entry (projectIdRef.current !== projectId → return) and again after the await recordMutationEdit (isActiveCommitTarget(projectIdRef, activeCompPathRef, projectId, compositionPath) → skip preview/iframe refresh). Both guards are needed because the await yields the microtask queue.

P1-3 — legacy vs SDK coordinator gap (RESOLVED). New packages/studio/src/utils/studioFileMutationCoordinator.ts provides serializeStudioFileMutation(writer, targetPath, task) — a WeakMap<writer, Map<targetPath, Promise>> where writer identity is the effective project identity (since writeProjectFile is now closure-bound to projectId). useGsapScriptCommits.serializeFile and sdkEditTransaction.ts:299-302 both route through this single serializer for the same (writer, path). Server-side, new commitElementPatchBatches in studio-server/src/routes/files.ts adds atomic multi-file element-patch commits with rollback on write failure. The two GSAP-only serializers Magi identified are now unified with the SDK path.

Important followup — rejected publication no longer refreshes stale iframe (RESOLVED). commitCandidateEdit in sdkEditTransaction.ts now discriminates rejected-active-target vs rejected-inactive-target; the inactive-target branch flips refreshTarget = false, so refreshCommittedEdit(edit.after, ...) is skipped entirely. Matches Magi's specific ask.

My R1 caller-adaptation concerns (RESOLVED)

James's inline comments on the file confirm Vance addressed both my orange concerns from R1 in commit 0f2cb3298:

• useGsapAnimationOps.ts:56 — shared rejection observer emits telemetry + one error toast; covers metadata, delete, remove-property, update-from-property, add-from-property.
• DesignPanelPromoteProvider.tsx:48 — VariablePromoteProvider requires an onPersistError seam; DesignPanelPromoteProvider supplies it. Silent-throw closed.

Adversarial pass — what four reviewers might all miss

Ran the "review-of-the-review" framing (Rule 15 adversarial pass) on top of the three prior reviews' converging read. Two things stand out that no prior review flagged:

• 🟡 Un-caught recordEdit throw during project switch. usePersistentEditHistory.recordEdit now throws (Cannot record an edit for inactive project ${projectId}) if the active project changes between callback creation and invocation. useGsapScriptCommits.recordMutationEdit awaits editHistory.recordEdit(...) with no try/catch (useGsapScriptCommits.ts:278-287); finalizeSuccessfulMutation awaits it (line 305) with no catch either. The rejection propagates up through runCommit → commitMutation → all 10+ await commitMutation(...) sites in gsapDragCommit.ts. Drag handlers are fire-and-forget from React events, so the rejection lands as unhandled. Scenario: mid-drag, user switches project via keyboard shortcut before the coalesced persist settles → uncaught error. The correct-behavior side (no wrong-project write) is safe because the throw fires inside the projectId-guarded recordEdit; the console-noise/telemetry side is what's exposed. Wrap with .catch(logStudioTelemetry(...)) or add an onRecordError seam matching the onPersistError shape you just added.

• 🟡 commitElementPatchBatches server-side atomicity relies on single-process Node. The docstring says "another route cannot interleave once the commit section begins" — true within one Node event loop, but if studio-server is ever multi-process/clustered (or if PM2/cluster mode is enabled for prod), two concurrent HTTP requests could race the readFileSync → writeFileSync window. resolveWithinProject + the in-batch dedup handles same-request duplicates, but not cross-request. Studio-server today is dev-time single-process — flagging in case that assumption ever shifts. Fair to defer via a doc-comment declaring the invariant load-bearing.

Nits

• readProjectFile error message "No active project" fires when the closure's captured projectId is null. If a stale readProjectFile from an old null-projectId state is invoked, the error is misleading (current project may be non-null). Cosmetic.
• storeProjectIdRef.current !== projectId throws inside recordEdit when the store hasn't finished loading (storeRef.current === null returns silently, but if storeRef.current exists for a DIFFERENT projectId briefly during the async load, throws). Edge race, unlikely in practice.

What I didn't verify

• Full test coverage of the delayed-A → switch-to-B → publish-B scenario Magi's proof exercised. Grepping for the shape didn't turn up explicit tests, but sdkEditTransaction.test.ts and sdkCutover.test.ts were extended per James's inline notes; I trusted those additions rather than re-running the harness locally in this pass.
• The end-to-end interaction between the new commitElementPatchBatches server route and the client's shared coordinator (i.e. does a partial batch failure with rollback correctly re-emit tri-state to the client). Trusting the test surface.
• Windows/CRLF and the .fallowrc.jsonc addition — unrelated to the concurrency invariants.

Verdict framing

Magi's three P1s are substantively closed with mechanisms that match the identified root causes (closure-bound writers, capture-at-issue-time project identity, unified serializer). The R6 fixes are load-bearing and hold up under an adversarial re-read.

LGTM from my side — the two 🟡s above are non-blocking (both are error-handling / operational hardening, not correctness on the concurrency invariant this PR is fixing). Since James pulled me and Via into this round specifically, deferring the stamp decision to him with Magi's original call context.

— Review by Rames D Jusso

[vanceingalls]

Adversarial pass on the prior reviews

Reviewed at head dff0c1df81a001af7465aaf9b1af8f02ff716f10. All required CI green; unmerged. Prior review coverage on this PR:

• Miguel R1 (blockers × 3): in-lock read-failure fallback erasing prior commits; persistSdkSerialize outside the queue; publish escaping session ownership.
• Miguel R2 (blockers × 3): project-switch across a queued edit; published-candidate ownership on path change; whole-file boundary excluding legacy GSAP RMWs. Also flagged a rejected-publication refresh leak.
• Rames-D R1 (inline × 5): silent-failure caller adaptation in the GSAP hooks; missing real-candidate write-failure test; dead captureOnDiskBefore reads at three sites.
• Rames-D R2 (this-head adversarial): all three Miguel R2 P1s materially closed; flagged uncaught recordEdit throw during project switch and single-process server-atomicity assumption as 🟡 non-blocking.
• Via R1 (my prior): missing onFlushError-equivalent on timeline hooks; publishSession non-throwing JSDoc; artifact-directory visibility nit.
• Via R2 (my prior): pre-write failures should be fallback-eligible; captureOnDiskBefore never-throws contract; publish-declined leaving live stale; queue keyed on writer identity.
• CodeQL: three client-side request-forgery flags on useFileManager.ts (project-id path segments) — all closed with encodeURIComponent.

James's latest response claims Miguel R2's four findings addressed on the new head, with a new studioFileMutationCoordinator.ts acting as the shared per-(writer,path) queue, project-bound I/O closures, session ownership by generation, and explicit rejected-active-target / rejected-inactive-target publication rejection. New tests for delayed A→B publish, cross-project history isolation, unmount cleanup, inactive-refresh suppression, and legacy GSAP behind an SDK write. Rames R2 independently converged on RESOLVED for all three P1s.

Cross-referencing the union of what all five reviewers said and never said against the diff, the class most-audited was candidate-transaction correctness + session ownership + caller adaptation. The class least-audited on this pass is who else besides SDK+legacy-GSAP writes whole files, and does the coordinator cover them. That's the hunt I ran; the two findings below are distinct from Rames R2's uncaught-recordEdit and single-process-server-atomicity 🟡s.

Findings

1. Important — undo/redo writes bypass the shared coordinator

Miguel R2 blocker #3 asked for (project,path) coordination across "every Studio RMW/rollback and legacy request". The fix landed SDK↔legacy-GSAP coordination via studioFileMutationCoordinator.ts, but the rollback side of Miguel's ask — undo/redo writes — is not routed through it.

• usePersistentEditHistory.ts:133,139 — writeFilesWithRollback calls writeFile(path, content) directly. The writeFile callback in useAppHotkeys.ts:363-370 (writeHistoryFile) is deps.writeProjectFile unchanged; no serializeStudioFileMutation wrap on the way in or out.
• useAppHotkeys.ts:376-380 — applyHistory awaits waitForPendingDomEditSaves() (usePreviewPersistence.ts:163-166), which drains only (a) flushStudioPendingEdits() — one registrant workspace-wide: propertyPanelColorGradingSection.tsx:389 — and (b) domEditSaveQueueRef.current?.waitForIdle(). Neither the SDK cutover queue nor the legacy-GSAP queue in studioFileMutationCoordinator.ts is drained here.

Reachable scenario: user drops a timeline element (useTimelineEditing.ts cutover enters the coordinator, mid-flight HTTP PUT), hits Ctrl+Z within the network window. The undo readCurrentFileHashes (usePersistentEditHistory.ts:113-118) fires against the file. If the cutover PUT has landed by then, the hash mismatch guard at line 174 catches it → toast + no-op (safe). If the PUT hasn't landed, the read returns pre-cutover bytes, hashes match, undo proceeds and PUTs the restored content. The cutover PUT then lands after the undo PUT and silently overrides the undo. The race window is server-processing-time wide (tens to hundreds of ms on a slow save).

Not a P0 — the mismatch guard covers one direction, and the reachable window is short. But it's a concrete gap vs. Miguel R2's stated ask ("every Studio RMW/rollback and legacy request"). Fix shape: wrap writeHistoryFile in serializeStudioFileMutation(writeProjectFile, path, task), or have applyHistory await the coordinator's tail for every path in the target entry before invoking editHistory[direction].

2. Important — code-editor content save bypasses the shared coordinator

Same class as #1 in a different lane. The code-editor debounced content-change save is the third peer whole-file writer to index.html and friends, alongside SDK cutover and legacy GSAP.

• useEditorSave.ts:48-59 — handleContentChange (rAF-debounced per keystroke) calls saveProjectFilesWithHistory, passing writeFile: writeProjectFile.
• studioFileHistory.ts:72,80 — writeFilesWithRollback calls writeFile(path, ...) directly. Not wrapped by serializeStudioFileMutation.

Reachable scenario: user is typing in the code editor for the active composition while a property-panel or timeline change fires an SDK cutover for the same file. Both writes are in flight; last-writer-wins on disk. The code-editor path also skips the coordinator's in-lock re-read, so the editor payload can be built against pre-cutover bytes and silently overwrite the cutover's committed edit.

Less reachable than #1 (typing rarely overlaps preview-panel interaction), but the same fix shape resolves it: saveProjectFilesWithHistory's per-file writeFile call should be wrapped in serializeStudioFileMutation(writeFile, path, task), or the caller should thread the coordinator in.

3. Watchpoint — legacy DOM edit fallback POST is outside the coordinator

useDomEditCommits.ts:349-355 — the legacy DOM patch fallback POSTs /api/projects/{id}/file-mutations/patch-element/{path} when SDK cutover declines (or is skipped for prepareContent). Server-side that handler is a readFileSync → patchElementInHtml → writeFileSync chain (packages/studio-server/src/routes/files.ts:2072-2119) — atomic within one request under Node's single-threaded loop, but the client can send it concurrently with a coordinator-queued PUT /files/{path} from another hook (timeline/GSAP/style running via SDK). The two hit different route handlers; the server enforces no cross-endpoint per-file lock (writeFileSync calls at both 1953 and 2111). Last-writeFileSync-wins on disk.

The prepareContent path (line 393-405) is worse — it does writeProjectFile(preparedContent) after the server POST, uncoordinated on both sides. Only fires for font-injection today, so the hot path is narrow.

Miguel R2 blocker #3 named packages/studio-server/src/routes/files.ts:941-979 explicitly and offered "or enforce server-side CAS/serialization" as an alternative fix. That option wasn't taken and this asymmetry remains.

4. Nit — rollbackWrite throw taxonomy leaves disk in a partial-committed state

sdkEditTransaction.ts:148-165 — if writeAndRecord sees recordEdit throw and rollbackWrite itself throws, the caller receives AggregateError wrapped in { status: "failed" }. Disk holds the new bytes; the coordinator's in-lock re-read on the next edit will pick them up as the baseline, but no history entry ever recorded them. History-vs-disk divergence for one path until the next successful edit. Consider a sdk_cutover_rollback_failed telemetry + a JSDoc note that failed after rollbackError means disk is committed-without-history.

5. Watchpoint — writer-identity keying is per-process

studioFileMutationCoordinator.ts:7 — WeakMap<StudioProjectFileWriter, ...>. All three coordination guarantees (SDK↔GSAP, cross-hook, cross-op) hinge on one stable writer reference per project. That's now enforced by useFileManager.ts:83-110's useCallback([projectId]). Two independent-process consumers (a second Studio tab against the same project, an out-of-band tool posting via the same PUT endpoint) will not share the WeakMap and will race. Not new to this PR — worth documenting the invariant on CutoverDeps.writeProjectFile's JSDoc so a future consumer doesn't factor it out.

Cleared classes

I hunted these blind-spot classes and cleared them at this head:

• Publication ownership on path switch / same-path reload / unmount — useSdkSession.ts:274-283 cleanup + useSdkSession.lifecycle.test.tsx:45-102+104-136 pin the scenarios Miguel R2 requested. sessionOwnersRef WeakMap keys publication by composition identity; ownsExpectedSession + isSessionOwnerActive gate both target-active and same-target-generation cases.
• Publish-refresh leak on rejected candidate — sdkEditTransaction.ts:224-226,238 suppresses refreshCommittedEdit on rejected-inactive-target; same-target rejection triggers setReloadToken internally instead. Miguel R2's "Important follow-up" is closed.
• Project-bound I/O + history closures — useFileManager.ts:83-108 captures writeProjectId at closure time; usePersistentEditHistory.ts:361-371 guards recordEdit on activeProjectIdRef and storeProjectIdRef. useFileManager.projectOwnership.test.tsx + usePersistentEditHistory.projectOwnership.test.tsx pin the delayed-A→B scenarios Miguel R2 blocker #1 requested.
• Caller adaptation to the failed-throw contract — Rames R1's five inline findings are answered by the shared handleGsapSaveFailure observer (useGsapScriptCommits.ts:383-396) and the onPersistError seam on VariablePromoteProvider.tsx. Regression tests in useGsapSelectionHandlers.test.tsx (new).
• CodeQL project-id path segments — all three flags closed via encodeURIComponent at the three sites (useFileManager.ts:73,91,122) and a fourth at useSdkSession.ts:25.

Verdict

Verdict: COMMENT
Reasoning: Miguel R2's four findings, Rames R1's caller-adaptation gaps, my prior R1/R2 findings, and CodeQL's forgery flags are all substantively addressed on this head. What remains — findings #1 and #2 — is that the shared coordinator's scope is narrower than "every Studio RMW/rollback and legacy request." Undo/redo and the code-editor whole-file save are peer writers that still bypass it. Not P0 blockers (narrow reachable windows, hash-mismatch mitigation on undo direction), but worth landing in the same coordinator before flipping the SDK cutover flag on for real traffic.

— Review by Via

[miguel-heygen]

Final blocker-closure review at dff0c1df81a001af7465aaf9b1af8f02ff716f10

Audited end-to-end: the transactional core, project-bound file/history ownership, session publication ownership, shared SDK/legacy-GSAP coordinator, rejected-publication refresh behavior, all prior review bodies/inlines, and the focused regression tests. Trusting: unrelated Studio subsystems outside the 40-file PR diff beyond their writer/caller seams.

The three P1s from my prior request are materially closed:

• useFileManager.ts:69-110 binds I/O closures to the issuing project, while usePersistentEditHistory.ts:308-371 rejects stale-project history writes.
• useSdkSession.ts:239-316 tracks generation/path ownership across candidate publication, disposes the session owned by the departing generation, and rejects inactive/stale publishers.
• studioFileMutationCoordinator.ts:1-31, sdkEditTransaction.ts:251-290, and useGsapScriptCommits.ts:341-352 put SDK candidates and legacy GSAP RMWs on one per-writer/per-path queue. sdkEditTransaction.ts:219-238 also suppresses target-sensitive refresh after inactive-target rejection.

The regression surface is strong: delayed A→B project ownership, inactive/stale publication, authoritative-read failure, real-candidate write failure, rejected-target refresh, and SDK/legacy interleaving are pinned. Local focused verification passed 91/91 tests after building Core, and Studio typecheck passed. GitHub required checks are all green; there are zero unresolved current review threads.

Important follow-ups (non-blocking for this PR): Via correctly identified that undo/redo (useAppHotkeys.ts:363-380) and code-editor saves (useEditorSave.ts:40-71 → studioFileHistory.ts:48-89) remain outside this client coordinator. Those are narrow, pre-existing cross-feature race windows rather than regressions in the cutover transaction, but should join the same serialization boundary before broad cutover rollout. Rames also flagged the stale-project recordEdit rejection telemetry path and the single-process server-lock assumption; both deserve explicit hardening/documentation.

The code verdict is clean. GitHub currently reports CONFLICTING / DIRTY against main, so this exact-head approval does not waive the required rebase and fresh CI/review of any substantive conflict resolution.

Verdict: APPROVE

Reasoning: Every blocker from my prior changes-requested review is closed with root-cause mechanisms and regression coverage; remaining findings are narrow operational/cross-feature follow-ups, while required CI is green at this exact head.

— Deepwork

[jrusso1020]

Follow-up on the final review sweep, now on 4ae8a8749 (rebased onto current main and restacked through #2174):

• Via Initial repo setup #1 — undo/redo bypass: fixed. An undo/redo step now acquires the shared coordinator for every affected path in deterministic sorted order before its authoritative read/hash check, writes, and rollback.
• Via initial code #2 — code-editor/whole-file save bypass: fixed at the shared saveProjectFilesWithHistory boundary, so code-editor and every other caller coordinate the entire read → write → history → rollback transaction with SDK/legacy GSAP writes.
• Added deterministic regressions proving both paths wait behind an earlier same-file mutation and observe its committed bytes. Validation is green: Studio typecheck; 85 focused transaction/session tests; full Studio suite (2,564 passing, 18 existing todos); optimistic-concurrency integration (87 Studio tests + 58 Studio Server tests); repository lint, format, and changed-code audit.
• Rames recordEdit watchpoint: no code change needed. finalizeSuccessfulMutation checks projectIdRef.current !== projectId immediately before calling recordMutationEdit, with no await/yield between that guard and the history store's synchronous ownership check. A switch after the history promise begins is handled by the existing post-await active-target guard.
• Single-process server atomicity: documented as a load-bearing process-local invariant; the contract now explicitly requires a shared per-project file lock if Studio Server ever becomes multi-process.
• Legacy DOM fallback / second-tab watchpoints: intentionally not expanded in this patch. A client-only wrapper would not solve a second process/tab, and wrapping the SDK attempt itself would risk re-entering the same path lock. The existing server CAS in fix(studio): enforce optimistic file concurrency #2156 protects versioned whole-file writes; a true cross-endpoint/cross-process guarantee should be a separate server locking/CAS change rather than an incidental expansion of this already-large PR.
• The rollback-error taxonomy and writer-identity notes remain non-blocking observability/deployment watchpoints; writer stability/process scope is documented at the coordinator/dependency boundary.

There are currently no unresolved, non-outdated review threads on #2155–#2174. Fresh CI is running on the resubmitted heads; requesting a current-head re-review because the prior approval correctly became stale after the rebase/fixes.

[jrusso1020]

Final rebase follow-up: main advanced again after the prior comment, so the full stack has now been rebased/restacked once more onto 04514116c. Current #2155 head is 3981498c9.

The only #2155 conflict was useDomEditCommits.ts; resolution keeps current main's extracted helper module while retaining this PR's transactional cutover behavior. The #2174 conflict likewise keeps main's extracted inspector-resize hook.

Validation on the exact submitted restack is green: Studio typecheck, full Studio suite (2,600 passing; 18 existing todos), Studio Server conflict/concurrency coverage (59/59), repository lint/contracts, format, and changed-code audit. The bottom branch merge-base exactly matches origin/main. Fresh CI has restarted on these heads.