fix(sdk): code-review follow-ups (WS-B/C/3.C, #1569/#1570/#1572) (#1588)

Addresses the still-outstanding review concerns from merged PRs #1569 / #1570 /
#1572 not already hoisted into #1573.

WS-B (#1569):
- validateVariables requires discriminant fields for object-valued font/image
  ({name,source} / {url}); a {name:42} font or {foo:42} image previously passed
  runtime validation and surfaced as a bogus font-family / missing image.
- Dropped ImageValue's [key:string]:unknown index signature (let any {url}-shaped
  object through, swallowed typos); explicit alt?/fit? instead.
- Documented the OverrideSet widening for SDK consumers.

WS-C (#1570):
- getElementTimings caches parsed GSAP labels by exact script text (avoids a full
  acorn re-parse per read; content-key invalidates on edit).
- Documented end-inclusive label window + best-effort extractGsapLabels catch.

WS-3.C (#1572):
- Added typed Composition.addWithKeyframes / replaceWithKeyframes (was asymmetric
  with addGsapTween; Studio had to use raw dispatch).
- Extracted shared KeyframeSpec type; documented position as seconds/number-only.

Gates: build + core 18/18 + sdk 19/19 + oxlint + oxfmt + fallow + typecheck all green.

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

packages/core/src/parsers/gsapParserAcorn.ts

@@ -1199,6 +1199,8 @@ export function extractGsapLabels(script: string): GsapLabelEntry[] {
 
     return labels;
   } catch {
+    // Labels are best-effort/supplementary, not load-bearing — a malformed or
+    // unparseable script yields no labels rather than failing the caller.
     return [];
   }
 }

packages/core/src/runtime/validateVariables.test.ts

@@ -17,6 +17,8 @@ const DECLS: readonly CompositionVariable[] = [
       { value: "dark", label: "Dark" },
     ],
   },
+  { id: "brandFont", type: "font", label: "Font", default: "Inter" },
+  { id: "hero", type: "image", label: "Hero", default: "https://x/y.png" },
 ];
 
 describe("validateVariables", () => {
@@ -80,6 +82,51 @@ describe("validateVariables", () => {
     ]);
   });
 
+  it("accepts a valid font object and a fallback string", () => {
+    expect(
+      validateVariables({ brandFont: { name: "Inter", source: "https://f/inter.css" } }, DECLS),
+    ).toEqual([]);
+    expect(validateVariables({ brandFont: "Inter" }, DECLS)).toEqual([]);
+  });
+
+  it("flags a font object missing string name/source", () => {
+    const expected = {
+      kind: "type-mismatch",
+      variableId: "brandFont",
+      expected: "font object {name: string, source: string}",
+      actual: "object missing string name/source",
+    };
+    expect(validateVariables({ brandFont: { name: 42 } }, DECLS)).toEqual([expected]);
+    expect(validateVariables({ brandFont: {} }, DECLS)).toEqual([expected]);
+  });
+
+  it("flags a non-object non-string font value", () => {
+    expect(validateVariables({ brandFont: 42 }, DECLS)).toEqual([
+      {
+        kind: "type-mismatch",
+        variableId: "brandFont",
+        expected: "font (object {name, source} or string)",
+        actual: "number",
+      },
+    ]);
+  });
+
+  it("accepts a valid image object and a fallback string", () => {
+    expect(validateVariables({ hero: { url: "https://x/y.png" } }, DECLS)).toEqual([]);
+    expect(validateVariables({ hero: "https://x/y.png" }, DECLS)).toEqual([]);
+  });
+
+  it("flags an image object missing a string url", () => {
+    expect(validateVariables({ hero: { foo: 42 } }, DECLS)).toEqual([
+      {
+        kind: "type-mismatch",
+        variableId: "hero",
+        expected: "image object {url: string}",
+        actual: "object missing string url",
+      },
+    ]);
+  });
+
   it("returns multiple issues at once", () => {
     const issues = validateVariables({ title: 42, theme: "neon", extra: true }, DECLS);
     expect(issues).toContainEqual({

packages/core/src/runtime/validateVariables.ts

@@ -87,7 +87,8 @@ function checkType(value: unknown, decl: CompositionVariable): VariableValidatio
     }
     case "font": {
       // Font value is an object {name: string, source: string} OR a fallback string.
-      if (!isPlainObject(value) && typeof value !== "string") {
+      if (typeof value === "string") return null;
+      if (!isPlainObject(value)) {
         return {
           kind: "type-mismatch",
           variableId: decl.id,
@@ -95,11 +96,23 @@ function checkType(value: unknown, decl: CompositionVariable): VariableValidatio
           actual: jsTypeOf(value),
         };
       }
+      // Object form: require the discriminant fields so a malformed brand-kit
+      // value ({name: 42} / {}) is caught here rather than surfacing as a bogus
+      // font-family at render time.
+      if (typeof value.name !== "string" || typeof value.source !== "string") {
+        return {
+          kind: "type-mismatch",
+          variableId: decl.id,
+          expected: "font object {name: string, source: string}",
+          actual: "object missing string name/source",
+        };
+      }
       return null;
     }
     case "image": {
       // Image value is an object {url: string} OR a fallback string.
-      if (!isPlainObject(value) && typeof value !== "string") {
+      if (typeof value === "string") return null;
+      if (!isPlainObject(value)) {
         return {
           kind: "type-mismatch",
           variableId: decl.id,
@@ -107,6 +120,15 @@ function checkType(value: unknown, decl: CompositionVariable): VariableValidatio
           actual: jsTypeOf(value),
         };
       }
+      // Object form: require the discriminant field.
+      if (typeof value.url !== "string") {
+        return {
+          kind: "type-mismatch",
+          variableId: decl.id,
+          expected: "image object {url: string}",
+          actual: "object missing string url",
+        };
+      }
       return null;
     }
   }

packages/sdk/src/session.dispatch.test.ts

@@ -34,6 +34,13 @@ async function withPatch(html: string): Promise<{ comp: Composition; events: Pat
   return { comp, events };
 }
 
+function expectGsapScriptPatch(id: string, events: PatchEvent[]): void {
+  expect(typeof id).toBe("string");
+  expect(id.length).toBeGreaterThan(0);
+  expect(events).toHaveLength(1);
+  expect(events[0]!.patches.find((p) => p.path.includes("/script/gsap"))).toBeDefined();
+}
+
 // ─── patch event emission ─────────────────────────────────────────────────────
 
 describe("dispatch emits patch event", () => {
@@ -182,10 +189,7 @@ describe("addGsapTween via session", () => {
     const { comp, events } = await withPatch(GSAP_HTML);
     const id = comp.addGsapTween("hf-box", { method: "to", duration: 0.3, properties: { x: 200 } });
 
-    expect(typeof id).toBe("string");
-    expect(id.length).toBeGreaterThan(0);
-    expect(events).toHaveLength(1);
-    expect(events[0]!.patches.find((p) => p.path.includes("/script/gsap"))).toBeDefined();
+    expectGsapScriptPatch(id, events);
   });
 
   it("undo removes the added tween", async () => {
@@ -197,6 +201,40 @@ describe("addGsapTween via session", () => {
   });
 });
 
+// ─── addWithKeyframes / replaceWithKeyframes via session API ──────────────────
+
+describe("keyframe ops via session", () => {
+  it("addWithKeyframes returns an animationId and emits a GSAP script patch", async () => {
+    const { comp, events } = await withPatch(GSAP_HTML);
+    const id = comp.addWithKeyframes('[data-hf-id="hf-box"]', 0, 0.5, [
+      { percentage: 0, properties: { opacity: 0 } },
+      { percentage: 100, properties: { opacity: 1 } },
+    ]);
+
+    expectGsapScriptPatch(id, events);
+  });
+
+  it("replaceWithKeyframes returns the replacement id; undo restores the prior script", async () => {
+    const comp = await openComposition(GSAP_HTML);
+    const before = comp.serialize();
+    const addId = comp.addWithKeyframes('[data-hf-id="hf-box"]', 0, 0.5, [
+      { percentage: 0, properties: { opacity: 0 } },
+      { percentage: 100, properties: { opacity: 1 } },
+    ]);
+    const newId = comp.replaceWithKeyframes(addId, '[data-hf-id="hf-box"]', 0, 0.8, [
+      { percentage: 0, properties: { x: 0 } },
+      { percentage: 100, properties: { x: 100 } },
+    ]);
+
+    expect(typeof newId).toBe("string");
+    expect(newId.length).toBeGreaterThan(0);
+
+    comp.undo(); // undo replace
+    comp.undo(); // undo add
+    expect(comp.serialize()).toBe(before);
+  });
+});
+
 // ─── dispatch with explicit origin ───────────────────────────────────────────
 
 describe("dispatch origin", () => {

packages/sdk/src/session.ts

@@ -18,6 +18,7 @@ import type {
   FontValue,
   GsapTweenSpec,
   ElasticHold,
+  KeyframeSpec,
   HfId,
   ImageValue,
   JsonPatchOp,
@@ -150,13 +151,28 @@ class CompositionImpl implements Composition {
 
   // ── WS-C: timing accessors + typed setHold ───────────────────────────────────
 
+  /**
+   * Cache of parsed GSAP labels keyed by EXACT script text. extractGsapLabels does
+   * a full acorn parse; caching avoids re-parsing on repeated getElementTimings reads
+   * when the script is unchanged. The content (not reference) key means any script
+   * edit changes the text and invalidates the cache, so renumbered tweens never yield
+   * stale label positions.
+   */
+  private _gsapLabelCache: { script: string; labels: ReturnType<typeof extractGsapLabels> } | null =
+    null;
+
   // fallow-ignore-next-line complexity
   getElementTimings(): Record<HfId, ElementTimingSnapshot> {
     const script = getGsapScript(this.parsed.document);
 
-    // Extract all addLabel("name", position) calls from the GSAP script. Parsed
-    // fresh each call so renumbered tweens never yield stale label positions.
-    const allLabels = script ? extractGsapLabels(script) : [];
+    // Extract all addLabel("name", position) calls from the GSAP script (see cache note above).
+    let allLabels: ReturnType<typeof extractGsapLabels>;
+    if (script && this._gsapLabelCache?.script === script) {
+      allLabels = this._gsapLabelCache.labels;
+    } else {
+      allLabels = script ? extractGsapLabels(script) : [];
+      this._gsapLabelCache = script ? { script, labels: allLabels } : null;
+    }
 
     const result: Record<HfId, ElementTimingSnapshot> = {};
     const elements = this.getElements();
@@ -186,7 +202,8 @@ class CompositionImpl implements Composition {
       const enterAt = Number.isFinite(start) ? start : 0;
       const exitAt = enterAt + (Number.isFinite(duration) ? duration : 0);
 
-      // Labels whose position falls within [enterAt, exitAt].
+      // Labels whose position falls within [enterAt, exitAt] (end-inclusive: a
+      // label exactly at exitAt is treated as within the element's window).
       const labels = allLabels
         .filter(({ position }) => position >= enterAt && position <= exitAt)
         .map(({ name }) => name);
@@ -227,6 +244,45 @@ class CompositionImpl implements Composition {
     this.dispatch({ type: "removeGsapTween", animationId });
   }
 
+  addWithKeyframes(
+    targetSelector: string,
+    position: number,
+    duration: number,
+    keyframes: KeyframeSpec[],
+    ease?: string,
+  ): string {
+    const result = this._dispatch(
+      { type: "addWithKeyframes", targetSelector, position, duration, keyframes, ease },
+      ORIGIN_LOCAL,
+    );
+    return result.meta?.animationId ?? "";
+  }
+
+  replaceWithKeyframes(
+    animationId: string,
+    targetSelector: string,
+    position: number,
+    duration: number,
+    keyframes: KeyframeSpec[],
+    ease?: string,
+  ): string {
+    const result = this._dispatch(
+      {
+        type: "replaceWithKeyframes",
+        animationId,
+        targetSelector,
+        position,
+        duration,
+        keyframes,
+        ease,
+      },
+      ORIGIN_LOCAL,
+    );
+    // Position-derived IDs renumber after the remove — this is the NEW id, which
+    // may differ from the input animationId.
+    return result.meta?.animationId ?? "";
+  }
+
   undo(): void {
     this.historyModule?.undo();
   }

packages/sdk/src/types.ts

@@ -53,6 +53,12 @@ export interface SdkDocument {
  * Font and image variable overrides store their object values under the var.{id} key:
  * { "var.brand-font": { name: "Roboto", source: "https://fonts.googleapis.com/…" } }
  */
+/**
+ * A set of variable overrides. The `Record<string, unknown>` member admits
+ * object-valued variables (font/image). NOTE for SDK consumers: this widening
+ * means code reading an OverrideSet value must narrow before assuming a scalar —
+ * an object value will type-check anywhere `unknown` is accepted.
+ */
 export type OverrideSet = Record<
   string,
   string | number | boolean | Record<string, unknown> | null
@@ -194,14 +200,10 @@ export type EditOp =
       /** Insert a new keyframed tween for targetSelector at the given position/duration. */
       type: "addWithKeyframes";
       targetSelector: string;
+      /** Timeline position in seconds. Number-only (unlike GsapTweenSpec.position, which also accepts label-relative strings). */
       position: number;
       duration: number;
-      keyframes: Array<{
-        percentage: number;
-        properties: Record<string, number | string>;
-        ease?: string;
-        auto?: boolean;
-      }>;
+      keyframes: KeyframeSpec[];
       ease?: string;
     }
   | {
@@ -214,17 +216,26 @@ export type EditOp =
       type: "replaceWithKeyframes";
       animationId: string;
       targetSelector: string;
+      /** Timeline position in seconds. Number-only (unlike GsapTweenSpec.position, which also accepts label-relative strings). */
       position: number;
       duration: number;
-      keyframes: Array<{
-        percentage: number;
-        properties: Record<string, number | string>;
-        ease?: string;
-        auto?: boolean;
-      }>;
+      keyframes: KeyframeSpec[];
       ease?: string;
     };
 
+/**
+ * A single keyframe entry for `addWithKeyframes` / `replaceWithKeyframes`.
+ * Single source of truth — Studio-side mirrors (KeyframeEntry/KeyframeSpec) should
+ * import this rather than redeclare the shape.
+ */
+export interface KeyframeSpec {
+  percentage: number;
+  properties: Record<string, number | string>;
+  ease?: string;
+  /** GSAP endpoint flag — emitted as numeric `_auto: 1`, not boolean. */
+  auto?: boolean;
+}
+
 export interface ElasticHold {
   start: number;
   end: number;
@@ -242,11 +253,14 @@ export interface FontValue {
 
 /**
  * Object value for an `image` variable (LOCKED §7 — object-valued, never a CSS string).
- * `url` is the image src; additional fields (alt, fit, etc.) are forward-compatible.
+ * `url` is the image src. Add explicit optional fields here as consumers need them —
+ * an open `[key: string]: unknown` index signature was dropped because it let any
+ * `{url}`-shaped object through and swallowed key typos.
  */
 export interface ImageValue {
   url: string;
-  [key: string]: unknown;
+  alt?: string;
+  fit?: "cover" | "contain" | "fill" | "none" | "scale-down";
 }
 
 export interface GsapTweenSpec {
@@ -416,6 +430,30 @@ export interface Composition {
   addGsapTween(target: HfId, tween: GsapTweenSpec): string;
   setGsapTween(animationId: string, properties: Partial<GsapTweenSpec>): void;
   removeGsapTween(animationId: string): void;
+  /**
+   * Add a keyframed tween. Typed wrapper over the addWithKeyframes op (mirrors
+   * addGsapTween). Returns the newly-minted animationId, or "" if rejected.
+   */
+  addWithKeyframes(
+    targetSelector: string,
+    position: number,
+    duration: number,
+    keyframes: KeyframeSpec[],
+    ease?: string,
+  ): string;
+  /**
+   * Replace an existing keyframed tween. Typed wrapper over replaceWithKeyframes.
+   * Returns the replacement's animationId (treat as NEW — position-derived IDs
+   * renumber after the remove), or "" if rejected.
+   */
+  replaceWithKeyframes(
+    animationId: string,
+    targetSelector: string,
+    position: number,
+    duration: number,
+    keyframes: KeyframeSpec[],
+    ease?: string,
+  ): string;
   undo(): void;
   redo(): void;
   canUndo(): boolean;