From 17f3e5beb0dffd92ea68b2437f8ffcaf62998d46 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 14 Jul 2026 13:13:09 -0700 Subject: [PATCH] fix(studio): apply cross-file rule to all tripwire entry points; harden disk-truth check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round on PR #2442: - miguel (blocker): the cross-file eligibility rule only guarded the dom-edit tripwire; recordResolverParity and recordAnimationResolverParity ran before wrongCompositionFile at every cutover surface, so cross-file ops still emitted false element_not_found (id present in the OTHER file's source passes the runtime-node filter) and polluted the attempt denominator. The rule now lives in one shared isCrossFileEdit guard applied by all three entry points, wired with { targetPath, compositionPath } at all six sdkCutover call sites (timing, timing-batch, gsap add/set/remove, keyframe chokepoint, delete). - Rames (race): the disk-truth read is now dispatched SYNCHRONOUSLY in the same prologue as the miss check, before control returns to the caller whose cutover persist writes the same file moments later — a post-write read would see a remove op's target legitimately gone and misclassify it as a genuine divergence. Sync reader throws become rejections (IIFE), not exceptions into the swallow-all catch. - Rames (parse failure): openComposition failure inside the disk check now fails open as sourceReadFailed (unparseable source is not ground truth), instead of the outer catch dropping the divergence event entirely. recordResolverParity's source check extracted to checkHfIdInSource (complexity gate), mirroring checkAnimationIdOnDisk's error discipline. Co-Authored-By: Claude Fable 5 --- packages/studio/src/utils/sdkCutover.ts | 13 ++- .../src/utils/sdkResolverShadow.test.ts | 54 +++++++++ .../studio/src/utils/sdkResolverShadow.ts | 109 +++++++++++++----- 3 files changed, 143 insertions(+), 33 deletions(-) diff --git a/packages/studio/src/utils/sdkCutover.ts b/packages/studio/src/utils/sdkCutover.ts index e76283aa0..b17b03fe2 100644 --- a/packages/studio/src/utils/sdkCutover.ts +++ b/packages/studio/src/utils/sdkCutover.ts @@ -189,6 +189,7 @@ export async function sdkTimingPersist( hfId, "setTiming", timingSrc ? () => timingSrc(targetPath) : undefined, + { targetPath, compositionPath: deps.compositionPath }, ); // Dark-launch gate: without this, timing cutover runs whenever an SDK session // exists (it always does, for shadow/selection) — flipping the flag OFF would @@ -231,6 +232,7 @@ export async function sdkTimingBatchPersist( change.hfId, "setTiming", timingSrc ? () => timingSrc(targetPath) : undefined, + { targetPath, compositionPath: deps.compositionPath }, ); } if (!STUDIO_SDK_CUTOVER_ENABLED) return false; @@ -283,6 +285,7 @@ export function sdkGsapTweenPersist( op.target, "addGsapTween", gsapSrc ? () => gsapSrc(targetPath) : undefined, + { targetPath, compositionPath: deps.compositionPath }, ); } else { void recordAnimationResolverParity( @@ -290,6 +293,7 @@ export function sdkGsapTweenPersist( op.animationId, op.kind === "set" ? "setGsapTween" : "removeGsapTween", gsapReadSource(deps, targetPath), + { targetPath, compositionPath: deps.compositionPath }, ); } // Leading dark-launch gate so flag-off does no SDK touch (getElement) at all — @@ -329,6 +333,7 @@ async function dispatchGsapOpAndPersist( resolverTarget.animationId, resolverTarget.opLabel, gsapReadSource(deps, targetPath), + { targetPath, compositionPath: deps.compositionPath }, ); } // Dark-launch gate (shared chokepoint for every GSAP-op cutover persist): @@ -553,8 +558,12 @@ export async function sdkDeletePersist( deps: CutoverDeps, ): Promise { // Resolver tripwire — runs BEFORE the cutover gate (decoupled). - void recordResolverParity(sdkSession, hfId, "removeElement", () => - Promise.resolve(originalContent), + void recordResolverParity( + sdkSession, + hfId, + "removeElement", + () => Promise.resolve(originalContent), + { targetPath, compositionPath: deps.compositionPath }, ); // Dark-launch gate: flag OFF → legacy server delete path. if (!STUDIO_SDK_CUTOVER_ENABLED) return false; diff --git a/packages/studio/src/utils/sdkResolverShadow.test.ts b/packages/studio/src/utils/sdkResolverShadow.test.ts index 5401afd9d..8264d3bf4 100644 --- a/packages/studio/src/utils/sdkResolverShadow.test.ts +++ b/packages/studio/src/utils/sdkResolverShadow.test.ts @@ -432,6 +432,18 @@ describe("F. recordResolverParity", () => { expect(trackedEvents.filter((e) => e.event === "sdk_resolver_shadow")).toHaveLength(0); }); + it("skips entirely (no event, no attempt) for a cross-file op", async () => { + mockFlags.STUDIO_SDK_RESOLVER_SHADOW_ENABLED = true; + flushAttemptCounts(); + const session = await openComposition(BASE_HTML); + await recordResolverParity(session, "hf-cross", "setTiming", undefined, { + targetPath: "compositions/other.html", + compositionPath: "index.html", + }); + expect(trackedEvents.filter((e) => e.event === "sdk_resolver_shadow")).toHaveLength(0); + expect(flushAttemptCounts()).toBeNull(); + }); + it("emits with sourceHfIdCount=1 when the hfId IS in source but missing from the session", async () => { mockFlags.STUDIO_SDK_RESOLVER_SHADOW_ENABLED = true; const session = await openComposition(BASE_HTML); @@ -616,6 +628,48 @@ describe("G. recordAnimationResolverParity", () => { expect(ev?.sourceReadFailed).toBe(true); expect(ev?.diskChecked).toBeUndefined(); }); + + it("fails open with sourceReadFailed when the reader throws SYNCHRONOUSLY", async () => { + mockFlags.STUDIO_SDK_RESOLVER_SHADOW_ENABLED = true; + const session = await openComposition(GSAP_HTML); + await recordAnimationResolverParity(session, "no-such-anim", "removeGsapKeyframe", () => { + throw new Error("sync read failed"); + }); + const ev = lastShadow(); + expect(ev?.mismatchCount).toBe(1); + expect(ev?.sourceReadFailed).toBe(true); + }); + + it("dispatches the disk read synchronously on a miss — before the caller's write can land", async () => { + // The tripwire is fire-and-forget and the caller's cutover persist writes + // this same file moments after it returns. The read request must be + // dispatched in the same sync prologue as the miss check, so it observes + // the PRE-write file (a post-write read would see a remove op's target + // legitimately gone and misclassify it as a genuine divergence). + mockFlags.STUDIO_SDK_RESOLVER_SHADOW_ENABLED = true; + const session = await openComposition(GSAP_HTML); + const read = vi.fn(() => Promise.resolve(GSAP_DISK_MOVED_HTML)); + const pending = recordAnimationResolverParity( + session, + "no-such-anim", + "removeGsapKeyframe", + read, + ); + expect(read).toHaveBeenCalledTimes(1); // already dispatched, no await yet + await pending; + }); + + it("skips entirely (no event, no attempt) for a cross-file op", async () => { + mockFlags.STUDIO_SDK_RESOLVER_SHADOW_ENABLED = true; + flushAttemptCounts(); + const session = await openComposition(GSAP_HTML); + await recordAnimationResolverParity(session, "no-such-anim", "removeGsapKeyframe", undefined, { + targetPath: "compositions/other.html", + compositionPath: "index.html", + }); + expect(trackedEvents.filter((e) => e.event === "sdk_resolver_shadow")).toHaveLength(0); + expect(flushAttemptCounts()).toBeNull(); + }); }); // ─── G2. runResolverShadow cross-file guard ─────────────────────────────────── diff --git a/packages/studio/src/utils/sdkResolverShadow.ts b/packages/studio/src/utils/sdkResolverShadow.ts index f26db5817..6508e683f 100644 --- a/packages/studio/src/utils/sdkResolverShadow.ts +++ b/packages/studio/src/utils/sdkResolverShadow.ts @@ -300,29 +300,41 @@ function reportEmptySession(session: Composition, opLabel: string): boolean { return true; } +/** Shape shared by every resolver-shadow entry point's optional path pair. */ +export interface ResolverShadowPaths { + targetPath?: string; + compositionPath?: string | null; +} + +/** + * Cross-file edit: the session models ONLY the active composition, so a + * target living in another file is structurally unresolvable — the cutover + * gates decline it (wrongCompositionFile) and it can never cut over. Every + * tripwire entry point skips entirely (no event, no attempt): the op belongs + * in neither the divergence count nor the attempt denominator of the soak + * rate. PostHog 0.7.41: one cross-file editing session emitted 479 false + * element_not_found events before this rule existed. Callers that pass no + * paths (isolated consumers/tests) run as before. + */ +function isCrossFileEdit(paths?: ResolverShadowPaths): boolean { + return ( + paths?.targetPath !== undefined && + paths.compositionPath != null && + paths.targetPath !== paths.compositionPath + ); +} + export function runResolverShadow( session: Composition, hfId: string | null | undefined, ops: PatchOperation[], sourceContent?: string, - paths?: { targetPath?: string; compositionPath?: string | null }, + paths?: ResolverShadowPaths, ): void { if (!STUDIO_SDK_RESOLVER_SHADOW_ENABLED) return; if (!hfId) return; try { - // Cross-file edit: the session models ONLY the active composition, so a - // target living in another file is structurally unresolvable — the cutover - // gates decline it (wrongCompositionFile) and it can never cut over. Skip - // entirely (no event, no attempt), mirroring the empty-session rule below. - // PostHog 0.7.41: one cross-file editing session emitted 479 false - // element_not_found events through this path. - if ( - paths?.targetPath !== undefined && - paths.compositionPath != null && - paths.targetPath !== paths.compositionPath - ) { - return; - } + if (isCrossFileEdit(paths)) return; if (reportEmptySession(session, "dom-edit")) return; recordAttempt("dom-edit"); const mismatches = sdkResolverShadowCheck(session, hfId, ops, sourceContent); @@ -368,15 +380,41 @@ export function runResolverShadow( * * No-op when the shadow flag is off; never throws; never mutates the session. */ +/** + * Source-truth check for a missed hf-id: read the target file and decide + * whether the miss is a runtime-generated node (id absent from source — + * suppress, the SDK cannot model it by design) or a reportable divergence + * (id present → strict attribute count for the event; read failure → fail + * open, tagged). Mirrors checkAnimationIdOnDisk's error discipline. + */ +async function checkHfIdInSource( + hfId: string, + readSource?: () => Promise, +): Promise<{ suppress: boolean; strictCount?: number; sourceReadFailed: boolean }> { + if (!readSource) return { suppress: false, sourceReadFailed: false }; + let source: string | undefined; + try { + source = await readSource(); + } catch { + return { suppress: false, sourceReadFailed: true }; // fail-open + } + if (source === undefined) return { suppress: false, sourceReadFailed: false }; + // Loose substring match — biased toward keeping signal (see sourceLooseMatchOnly). + if (!source.includes(hfId)) return { suppress: true, sourceReadFailed: false }; + return { suppress: false, strictCount: countHfIdInSource(source, hfId), sourceReadFailed: false }; +} + export async function recordResolverParity( session: Composition | null | undefined, hfId: string | null | undefined, opLabel: string, readSource?: () => Promise, + paths?: ResolverShadowPaths, ): Promise { if (!STUDIO_SDK_RESOLVER_SHADOW_ENABLED) return; if (!session || !hfId) return; try { + if (isCrossFileEdit(paths)) return; if (reportEmptySession(session, opLabel)) return; recordAttempt(opLabel); if (resolveSnapshot(session, hfId)) return; // resolves — parity, nothing to record @@ -387,19 +425,10 @@ export async function recordResolverParity( // state this field exists to diagnose. const sessionElementCount = session.getElements().length; // Cheap check passed above, so the source read only runs on a real divergence. - let source: string | undefined; - let sourceReadFailed = false; - if (readSource) { - try { - source = await readSource(); - } catch { - source = undefined; // fail-open: a read error must not drop a real divergence - sourceReadFailed = true; - } - } - // Runtime-generated node the static parse can't model — suppress (mirrors the dom-edit path). - if (source !== undefined && !source.includes(hfId)) return; - const strictCount = source !== undefined ? countHfIdInSource(source, hfId) : undefined; + // Runtime-generated node the static parse can't model → suppressed inside. + const verdict = await checkHfIdInSource(hfId, readSource); + if (verdict.suppress) return; + const { strictCount, sourceReadFailed } = verdict; trackStudioEvent("sdk_resolver_shadow", { hfId, opLabel, @@ -452,18 +481,27 @@ export async function recordResolverParity( */ async function checkAnimationIdOnDisk( animationId: string, - readSource: () => Promise, + sourcePromise: Promise, ): Promise<{ staleSession: boolean; diskChecked: boolean; sourceReadFailed: boolean }> { let source: string | undefined; try { - source = await readSource(); + source = await sourcePromise; } catch { return { staleSession: false, diskChecked: false, sourceReadFailed: true }; } if (source === undefined) { return { staleSession: false, diskChecked: false, sourceReadFailed: false }; } - const disk = await openComposition(source, { history: false }); + // A parse failure (malformed script the user just introduced) means the + // source can't serve as ground truth — treat it exactly like a read failure + // (fail open, tagged) instead of letting the outer never-propagate catch + // swallow the whole divergence event. + let disk: Composition; + try { + disk = await openComposition(source, { history: false }); + } catch { + return { staleSession: false, diskChecked: false, sourceReadFailed: true }; + } try { return { staleSession: disk.getAllAnimationIds().has(animationId), @@ -480,10 +518,12 @@ export async function recordAnimationResolverParity( animationId: string, opLabel: string, readSource?: () => Promise, + paths?: ResolverShadowPaths, ): Promise { if (!STUDIO_SDK_RESOLVER_SHADOW_ENABLED) return; if (!session || !animationId) return; try { + if (isCrossFileEdit(paths)) return; recordAttempt(opLabel); const elements = session.getElements(); const resolves = @@ -495,7 +535,14 @@ export async function recordAnimationResolverParity( let diskChecked = false; let sourceReadFailed = false; if (readSource) { - const verdict = await checkAnimationIdOnDisk(animationId, readSource); + // Start the read SYNCHRONOUSLY, before returning control to the caller: + // the caller's cutover persist writes the new content to this same file + // moments later, and a read dispatched after that write would check the + // POST-edit disk (e.g. a remove op's target legitimately gone → false + // "genuine divergence"). Dispatching the request here, in the same sync + // prologue as the miss check, orders it ahead of the caller's write. + const sourcePromise = (async () => readSource())(); + const verdict = await checkAnimationIdOnDisk(animationId, sourcePromise); if (verdict.staleSession) return; // sync gap, not a resolver bug — suppress diskChecked = verdict.diskChecked; sourceReadFailed = verdict.sourceReadFailed;