From 61db4531337f6586cff80c130fb459e8d0bc6360 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Fri, 18 Sep 2026 10:37:30 -0700 Subject: [PATCH] feat(studio): split ineligible_operation into its four real causes The first 24h of v0.8.47 telemetry showed 30 declines against 25 successes on the dom family, and 26 of those 30 were `ineligible_operation` -- a single opaque string standing in for four unrelated causes. That made the number unactionable: a structural edit the SDK has no vocabulary for (expected, and permanent until the op mapping grows) looked exactly like a reserved-attribute or unsafe-attribute decline (narrow, and possibly worth fixing). `sdkCutoverIneligibleReason` now names which check failed: unsupported_op_type, child_scoped_op, reserved_attribute, unsafe_html_attribute, no_ops, or target_unaddressable. `shouldUseSdkCutover` is derived from it rather than duplicating the conjunction, so the predicate and its explanation cannot drift -- a test pins that agreement across every case. Also separates the two conditions that are not properties of the op batch at all: with the dom family disabled, sdkCutoverPersist reported `ineligible_operation`, which read as "the SDK cannot express this edit" when the truth was "this family is switched off". It now reports feature_disabled and session_unavailable distinctly, matching the other four chokepoints. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/utils/sdkCutover.familyGate.test.ts | 6 +- packages/studio/src/utils/sdkCutover.ts | 11 ++- .../src/utils/sdkCutoverEligibility.test.ts | 97 ++++++++++++++++++- .../studio/src/utils/sdkCutoverEligibility.ts | 45 ++++++--- 4 files changed, 142 insertions(+), 17 deletions(-) diff --git a/packages/studio/src/utils/sdkCutover.familyGate.test.ts b/packages/studio/src/utils/sdkCutover.familyGate.test.ts index 2046998b06..f2dcca7091 100644 --- a/packages/studio/src/utils/sdkCutover.familyGate.test.ts +++ b/packages/studio/src/utils/sdkCutover.familyGate.test.ts @@ -41,6 +41,10 @@ describe("SDK family gate mapping", () => { {} as never, deps, ), - ).resolves.toMatchObject({ status: "declined", reason: "ineligible_operation" }); + // The dom family is off, so the gate declines on the FLAG, not on the op + // batch (which is a perfectly eligible inline-style edit). Before the + // reason split this reported `ineligible_operation`, which read as "the + // SDK cannot express this edit" when the truth was "this family is off". + ).resolves.toMatchObject({ status: "declined", reason: "feature_disabled" }); }); }); diff --git a/packages/studio/src/utils/sdkCutover.ts b/packages/studio/src/utils/sdkCutover.ts index faf33b4b1f..14d3a46cbe 100644 --- a/packages/studio/src/utils/sdkCutover.ts +++ b/packages/studio/src/utils/sdkCutover.ts @@ -7,8 +7,8 @@ import { patchOpsToSdkEditOps } from "./sdkOpMapping"; import { recordResolverParity, recordAnimationResolverParity } from "./sdkResolverShadow"; import { isResolverDisagreement, + sdkCutoverIneligibleReason, shouldDeclineTextCutoverForTarget, - shouldUseSdkCutover, } from "./sdkCutoverEligibility"; import { asCutoverError, @@ -81,11 +81,14 @@ export async function sdkCutoverPersist( deps: CutoverDeps, options?: CutoverOptions, ): Promise { - if (!shouldUseSdkCutover(sdkFamilyEnabled("dom"), !!sdkSession, selection.hfId, ops)) - return declinedCutover("ineligible_operation", "dom"); + // Name WHICH eligibility check failed: on v0.8.47 this one string covered 26 of + // 30 dom declines. The two below aren't properties of the batch, so they differ. + if (!sdkFamilyEnabled("dom")) return declinedCutover("feature_disabled", "dom"); if (!sdkSession) return declinedCutover("session_unavailable", "dom"); const hfId = selection.hfId; - if (!hfId) return declinedCutover("target_unaddressable", "dom"); + const ineligible = sdkCutoverIneligibleReason(hfId, ops); + // `!hfId` is already a reason; testing it again narrows the type for the rest. + if (ineligible || !hfId) return declinedCutover(ineligible ?? "target_unaddressable", "dom"); const target = sdkSession.getElement(hfId); if (!target) return declinedCutover("target_not_found", "dom", isResolverDisagreement(sdkSession, hfId)); diff --git a/packages/studio/src/utils/sdkCutoverEligibility.test.ts b/packages/studio/src/utils/sdkCutoverEligibility.test.ts index fadd02cf12..e3efd20f2f 100644 --- a/packages/studio/src/utils/sdkCutoverEligibility.test.ts +++ b/packages/studio/src/utils/sdkCutoverEligibility.test.ts @@ -1,6 +1,27 @@ import { describe, expect, it } from "vitest"; import type { PatchOperation } from "./sourcePatcher"; -import { shouldUseSdkCutover } from "./sdkCutoverEligibility"; +import { shouldUseSdkCutover, sdkCutoverIneligibleReason } from "./sdkCutoverEligibility"; + +const styleOp = (property: string, value: string): PatchOperation => ({ + type: "inline-style", + property, + value, +}); +const textOp = (value: string): PatchOperation => ({ + type: "text-content", + property: "text", + value, +}); +const attrOp = (property: string, value: string): PatchOperation => ({ + type: "attribute", + property, + value, +}); +const htmlAttrOp = (property: string, value: string): PatchOperation => ({ + type: "html-attribute", + property, + value, +}); const childStyleOp: PatchOperation = { type: "inline-style", @@ -15,3 +36,77 @@ describe("shouldUseSdkCutover child-scoped operations", () => { expect(shouldUseSdkCutover(true, true, "hf-parent", [childStyleOp])).toBe(false); }); }); + +describe("sdkCutoverIneligibleReason", () => { + // `ineligible_operation` was one string covering four distinct causes, which + // made the post-flip decline telemetry unactionable: 26 of 30 declines on + // v0.8.47 landed there, with no way to separate a structural edit the SDK has + // no vocabulary for (expected, permanent) from a reserved-attribute or + // unsafe-attribute decline (narrower, and possibly fixable). + it("names an op type the SDK cannot express", () => { + expect( + sdkCutoverIneligibleReason("hf-abc", [ + { type: "split", property: "x", value: "1" } as unknown as PatchOperation, + ]), + ).toBe("unsupported_op_type"); + }); + + it("names a child-scoped op", () => { + expect(sdkCutoverIneligibleReason("hf-parent", [childStyleOp])).toBe("child_scoped_op"); + }); + + it("names a reserved attribute", () => { + expect(sdkCutoverIneligibleReason("hf-abc", [attrOp("end", "2")])).toBe("reserved_attribute"); + expect(sdkCutoverIneligibleReason("hf-abc", [htmlAttrOp("DATA-START", "1")])).toBe( + "reserved_attribute", + ); + }); + + it("names an unsafe html attribute", () => { + expect(sdkCutoverIneligibleReason("hf-abc", [htmlAttrOp("onclick", "alert(1)")])).toBe( + "unsafe_html_attribute", + ); + expect(sdkCutoverIneligibleReason("hf-abc", [htmlAttrOp("href", "javascript:alert(1)")])).toBe( + "unsafe_html_attribute", + ); + }); + + it("names an empty batch and an unaddressable target", () => { + expect(sdkCutoverIneligibleReason("hf-abc", [])).toBe("no_ops"); + expect(sdkCutoverIneligibleReason(null, [styleOp("color", "red")])).toBe( + "target_unaddressable", + ); + }); + + it("returns null when the batch is eligible", () => { + expect(sdkCutoverIneligibleReason("hf-abc", [styleOp("color", "red")])).toBeNull(); + expect( + sdkCutoverIneligibleReason("hf-abc", [ + styleOp("color", "red"), + textOp("hello"), + attrOp("x", "1"), + htmlAttrOp("class", "foo"), + ]), + ).toBeNull(); + }); + + // The reason function must not drift from the predicate it explains: a batch + // the gate accepts must have no reason, and one it declines must have one. + it("agrees with shouldUseSdkCutover on every case", () => { + const cases: PatchOperation[][] = [ + [styleOp("color", "red")], + [textOp("hello")], + [attrOp("x", "1")], + [htmlAttrOp("class", "foo")], + [attrOp("end", "2")], + [htmlAttrOp("onclick", "alert(1)")], + [childStyleOp], + [{ type: "split", property: "x", value: "1" } as unknown as PatchOperation], + [], + ]; + for (const ops of cases) { + const eligible = shouldUseSdkCutover(true, true, "hf-abc", ops); + expect(sdkCutoverIneligibleReason("hf-abc", ops) === null).toBe(eligible); + } + }); +}); diff --git a/packages/studio/src/utils/sdkCutoverEligibility.ts b/packages/studio/src/utils/sdkCutoverEligibility.ts index 2dbc0aee63..1e57bc9dca 100644 --- a/packages/studio/src/utils/sdkCutoverEligibility.ts +++ b/packages/studio/src/utils/sdkCutoverEligibility.ts @@ -123,21 +123,44 @@ export function isResolverDisagreement(session: Composition, hfId: string): bool } } +/** + * Why a batch cannot take the SDK cutover path, or null when it can. + * + * `ineligible_operation` used to be a single decline reason covering all of + * these, which made the post-flip telemetry unactionable — a structural edit the + * SDK has no vocabulary for (expected and permanent) was indistinguishable from + * a reserved-attribute decline (narrow, and possibly worth fixing). The order + * mirrors `shouldUseSdkCutover`'s conjunction, so the first failing check names + * the reason. + */ +export type SdkCutoverIneligibleReason = + | "target_unaddressable" + | "no_ops" + | "unsupported_op_type" + | "child_scoped_op" + | "reserved_attribute" + | "unsafe_html_attribute"; + +export function sdkCutoverIneligibleReason( + hfId: string | null | undefined, + ops: PatchOperation[], +): SdkCutoverIneligibleReason | null { + if (!hfId) return "target_unaddressable"; + if (ops.length === 0) return "no_ops"; + if (!ops.every((o) => CUTOVER_OP_TYPES.has(o.type))) return "unsupported_op_type"; + if (hasChildScopedOp(ops)) return "child_scoped_op"; + if (ops.some(mapsToReservedAttr)) return "reserved_attribute"; + if (hasUnsafeHtmlAttributeOp(ops)) return "unsafe_html_attribute"; + return null; +} + export function shouldUseSdkCutover( flagEnabled: boolean, hasSession: boolean, hfId: string | null | undefined, ops: PatchOperation[], ): boolean { - return ( - flagEnabled && - hasSession && - !!hfId && - ops.length > 0 && - ops.every((o) => CUTOVER_OP_TYPES.has(o.type)) && - // SDK edit ops target only the element hfId; child-scoped patch ops need the server path. - !hasChildScopedOp(ops) && - !ops.some(mapsToReservedAttr) && - !hasUnsafeHtmlAttributeOp(ops) - ); + // Derived from the reason function so the two can never disagree about which + // batches are eligible. + return flagEnabled && hasSession && sdkCutoverIneligibleReason(hfId, ops) === null; }