fix(sdk): address PR #2098 review feedback on variable CRUD

- validateOp now handles declareVariable/removeVariable (E_NO_ROOT when no
  composition root), matching setVariableValue's existing case — previously
  comp.can() returned E_UNKNOWN_OP for both.
- removeVariable's undo-inverse now tags its {decl, index} reinsert payload
  with __kind: "reinsert" instead of relying on structural "decl"/"index"
  key presence to disambiguate it from a plain declareVariable patch.
  VariableDecl has an open index signature, so a real variable schema could
  legally declare its own "decl"/"index" fields and be misinterpreted by the
  old structural check; a regression test pins the exact collision.
- getVariableValue's return type tightened from `unknown` to
  `string | number | boolean | FontValue | ImageValue | undefined`, matching
  setVariableValue's parameter type for round-trip symmetry. The underlying
  unknown-typed read is cast once at this SDK boundary.
- Added a redo test for declareVariable/removeVariable (existing tests only
  covered undo).

packages/sdk/src/engine/apply-patches.ts

@@ -256,13 +256,19 @@ function applyOne(parsed: ParsedDocument, patch: JsonPatchOp, p: ParsedPath): vo
       if (patch.op === "remove") {
         removeVariableDecl(parsed.document, p.id);
       } else {
-        // Undo of removeVariable bundles {decl, index} to reinsert at the
-        // original array position; a plain declareVariable forward/replace
-        // patch carries the bare decl. Disambiguate on shape — VariableDecl's
-        // own index signature means "in" narrowing can't fully eliminate it
-        // from the union, so re-cast explicitly once shape is confirmed.
+        // Undo of removeVariable bundles {__kind: "reinsert", decl, index} to
+        // reinsert at the original array position; a plain declareVariable
+        // forward/replace patch carries the bare decl. Disambiguate on the
+        // __kind tag, not structural "decl"/"index" key presence — VariableDecl
+        // has an open index signature, so a genuine variable schema could
+        // legally declare its own "decl"/"index" fields, and "in" narrowing
+        // alone can't rule that out.
         const value = patch.value;
-        if (value && typeof value === "object" && "decl" in value && "index" in value) {
+        if (
+          value &&
+          typeof value === "object" &&
+          (value as { __kind?: unknown }).__kind === "reinsert"
+        ) {
           const wrapped = value as { decl: VariableDecl; index: number };
           declareVariableDecl(parsed.document, wrapped.decl, { atIndex: wrapped.index });
         } else {

packages/sdk/src/engine/mutate.test.ts

@@ -999,6 +999,25 @@ describe("declareVariable", () => {
     applyPatchesToDocument(parsed, edited.inverse);
     expect(serializeDocument(parsed)).toBe(before);
   });
+
+  it("a decl whose own extension data includes 'decl'/'index' keys isn't mistaken for a wrapped reinsert", () => {
+    // VariableDecl has an open index signature, so a real, weird schema can
+    // legally carry fields named "decl"/"index" as ordinary extension data —
+    // this must NOT be confused with removeVariable's {__kind: "reinsert",
+    // decl, index} inverse-patch wrapper, which is disambiguated by __kind,
+    // not by structural key presence.
+    const parsed = fresh();
+    const pathologicalDecl = {
+      id: "weird-var",
+      type: "string",
+      label: "Weird",
+      default: "x",
+      decl: "not-a-real-decl",
+      index: 999,
+    };
+    applyOp(parsed, { type: "declareVariable", decl: pathologicalDecl });
+    expect(readVarDecl(parsed, "weird-var")).toEqual(pathologicalDecl);
+  });
 });
 
 describe("removeVariable", () => {
@@ -1129,6 +1148,29 @@ describe("validateOp", () => {
   it("returns ok:true for setCompositionMetadata (no target)", () => {
     expect(validateOp(fresh(), { type: "setCompositionMetadata", width: 100 }).ok).toBe(true);
   });
+
+  it("returns ok:true for declareVariable / removeVariable when a root exists", () => {
+    expect(
+      validateOp(fresh(), {
+        type: "declareVariable",
+        decl: { id: "v1", type: "string", label: "V1", default: "x" },
+      }).ok,
+    ).toBe(true);
+    expect(validateOp(fresh(), { type: "removeVariable", id: "v1" }).ok).toBe(true);
+  });
+
+  it("returns ok:false / E_NO_ROOT for declareVariable / removeVariable with no root", () => {
+    const parsed = parseMutable(`no elements at all — just text`);
+    const r1 = validateOp(parsed, {
+      type: "declareVariable",
+      decl: { id: "v1", type: "string", label: "V1", default: "x" },
+    });
+    expect(r1.ok).toBe(false);
+    if (!r1.ok) expect(r1.code).toBe("E_NO_ROOT");
+    const r2 = validateOp(parsed, { type: "removeVariable", id: "v1" });
+    expect(r2.ok).toBe(false);
+    if (!r2.ok) expect(r2.code).toBe("E_NO_ROOT");
+  });
 });
 
 // ─── Phase 3b ops — graceful when no GSAP script, feature-detectable ────────

packages/sdk/src/engine/mutate.ts

@@ -933,10 +933,13 @@ function handleRemoveVariable(parsed: ParsedDocument, id: string): MutationResul
   const path = variableDeclPath(id);
   // Bundle the original array index so undo reinserts at the same position
   // instead of appending — mirrors handleRemoveElement's {html, parentId,
-  // siblingIndex} inverse value.
+  // siblingIndex} inverse value. Tagged with __kind (rather than relying on
+  // structural "decl"/"index" key presence) because VariableDecl has an open
+  // index signature — a genuine variable schema could legally declare its own
+  // "decl"/"index" fields, which a structural check alone can't rule out.
   return {
     forward: [patchRemove(path)],
-    inverse: [patchAdd(path, { decl: removed.decl, index: removed.index })],
+    inverse: [patchAdd(path, { __kind: "reinsert", decl: removed.decl, index: removed.index })],
   };
 }
 
@@ -1546,6 +1549,8 @@ export function validateOp(parsed: ParsedDocument, op: EditOp): CanResult {
       return CAN_OK;
     }
     case "setVariableValue":
+    case "declareVariable":
+    case "removeVariable":
       if (findRoot(parsed.document) === null)
         return canErr("E_NO_ROOT", "Composition root element not found.");
       return CAN_OK;

packages/sdk/src/session.test.ts

@@ -625,4 +625,23 @@ describe("variable declarations (Composition API)", () => {
       { id: "brand-color", type: "color", label: "Brand color", default: "#0066cc" },
     ]);
   });
+
+  it("declareVariable / removeVariable both support redo after undo", async () => {
+    const comp = await openComposition(VARIABLES_HTML);
+
+    comp.declareVariable({ id: "brand-title", type: "string", label: "Title", default: "Hi" });
+    comp.undo();
+    comp.redo();
+    expect(comp.listVariables()).toEqual([
+      { id: "brand-color", type: "color", label: "Brand color", default: "#0066cc" },
+      { id: "brand-title", type: "string", label: "Title", default: "Hi" },
+    ]);
+
+    comp.removeVariable("brand-color");
+    comp.undo();
+    comp.redo();
+    expect(comp.listVariables()).toEqual([
+      { id: "brand-title", type: "string", label: "Title", default: "Hi" },
+    ]);
+  });
 });

packages/sdk/src/session.ts

@@ -155,8 +155,17 @@ class CompositionImpl implements Composition {
     this.dispatch({ type: "setVariableValue", id, value });
   }
 
-  getVariableValue(id: string): unknown {
-    return readVariableDefault(this.parsed.document, id);
+  getVariableValue(id: string): string | number | boolean | FontValue | ImageValue | undefined {
+    // readVariableDefault genuinely can't narrow beyond unknown — the schema
+    // isn't validated at read time — so the cast lives here at the SDK
+    // boundary rather than pushing it onto every caller of getVariableValue.
+    return readVariableDefault(this.parsed.document, id) as
+      | string
+      | number
+      | boolean
+      | FontValue
+      | ImageValue
+      | undefined;
   }
 
   listVariables(): CompositionVariable[] {

packages/sdk/src/types.ts

@@ -410,7 +410,7 @@ export interface Composition {
   addElement(parent: HfId | null, index: number, html: string): HfId;
   setVariableValue(id: string, value: string | number | boolean | FontValue | ImageValue): void;
   /** Current `default` value for a declared variable, or undefined if undeclared/unset. */
-  getVariableValue(id: string): unknown;
+  getVariableValue(id: string): string | number | boolean | FontValue | ImageValue | undefined;
   /** Every declared variable's full schema (id/type/label/default/…), or [] when none. */
   listVariables(): CompositionVariable[];
   /**