LGTM with one nit. The declaration ops are well-designed — the patch grammar (/variableDeclarations/{id} vs /variables/{id}), the fragment composition guard, and the isRawDeclarationEntry vs isCompositionVariable split all reflect deliberate design choices that are well-documented. Test coverage is thorough, especially the undo round-trips with CSS prop restoration.

Nit (non-blocking): The CSS compat prop write happens via cssCompatChange for declaration ops but via inline logic in the pre-existing handleSetVariableValue. Both produce identical --{id} style patches through getElementStyles/setElementStyles. Not a bug today (the ops target different patch paths), but a future refactor could unify the CSS-prop write path to a single actor. Low priority since the patch grammar prevents conflicts.

The isRawDeclarationEntry guard on the replay path is the right call — gating on isCompositionVariable there would silently break undo for loose declarations. The comment documenting this is appreciated.

— Miga

Reviewed at 3c94b400. Stack context: SDK foundation (#2046-#2048) — bottom of the 12-PR template-variables stack.

Substantive write surface with a well-thought-out contract. The strict-writer / loose-replay asymmetry called out in the PR body — and re-documented in apply-patches.ts L254-259 — is the right call for undo fidelity; the equivalent bug in another patch grammar would silently no-op inverses. Path grammar keeps /variableDeclarations/{id} and /variables/{id} cleanly separated, and the override-set mapping (varDecl.{id} vs var.{id}) round-trips through pathToKey + keyToPath. Fragment refusal (E_FRAGMENT_COMPOSITION) is exactly the right shape — loud failure beats data loss.

🟠 Hold

• packages/parsers/src/compositionVariables.ts:isCompositionVariable L43-50 — the id predicate is typeof v.id === "string" and nothing else. That accepts "", whitespace, HTML metacharacters, ids with / or . or ~, and reserved-looking keys like constructor. The path grammar happens to survive / and ~ because variableDeclPath / variablePath don't RFC-6902-escape and parsePath uses a greedy dot regex — so paths round-trip, but the override-set key space varDecl.{id} is only unambiguous by convention. A user-authored id .foo or foo/bar can be represented, but every downstream consumer (Studio autocompletion, PostHog attribution, CSS --{id} writes, --variables CLI parsing) is going to make assumptions the SDK didn't enforce. The SDK is the last defense before Studio and embedders — recommend a minimal /^[A-Za-z_][A-Za-z0-9_-]*$/ (CSS-custom-prop-safe, matches the runtime's cssVariableName slug convention) surfaced as E_INVALID_ARGS from can(). Zero-cost tightening now beats retrofitting after a customer authors id: "brand.color" and Studio starts silently overwriting other keys.

🟡 Nits / questions

• packages/sdk/src/engine/mutate.ts:handleDeclareVariable L972-978 — the isScalar(declaration.default) guard writes --{id}: <string> to the root's inline styles for ANY string-defaulted variable, including type: "image". For a scalar image URL that lands as --imgVar: https://cdn.example.com/x.png, which isn't consumable by background: var(--imgVar) without a url(…) wrapper. injectCompositionCssVariables in the runtime does the same, so this is at least consistent — but if the image variable exists purely so Studio can preview thumbnails, that CSS write is dead weight. Consider gating on declaration.type (scalar for string/number/color/boolean/enum), not on the runtime typeof of the default. Small cleanup; not a correctness bug.
• isCompositionVariable accepts {type: "enum", default: "not-in-options", options: [{value: "wide"}]} — the strict parser doesn't cross-check the enum default against the options array. validateVariables catches it at value-check time but a declaration that will never validate is a weird artifact to accept. Given can() is the authoring guard, folding the cross-check in there would be a one-line addition.
• writeVariableDeclaration (variableModel.ts L77-94) silently discards an unparseable prior attribute — documented, no functional issue, but the inverse-patch chain won't restore that prior garbage on undo. The comment justifies it correctly ("invisible to every reader"), just flagging for the reader who traces this later.
• packages/sdk/src/engine/mutate.ts:handleUpdateVariableDeclaration L1001 uses JSON.stringify(oldDefault) !== JSON.stringify(newDefault) for deep-equal. Object-key order matters for JSON.stringify comparison — a font/image object whose properties were re-ordered would false-positive as "changed" and emit a redundant patch. Low-cost false positive; consider a shallow deep-equal helper if this becomes a churn source.

🟢 What's good

• The BOTH-halves-of-the-schema-in-one-model invariant (variableModel.ts shared between forward mutate + replay apply-patches) is real, not aspirational. Grep confirms both entry points route through the same three primitives. This is exactly the fix pattern in feedback_be_fe_paired_silent_swallow — one seam, two consumers, one source of truth.
• Paired /variables/{id} value patch on updateVariableDeclaration when the default changes (mutate.ts L999-1010) means the T3 override-set stays coherent regardless of replay order — the failure mode you called out in the PR body is a real one, and the fix is minimal.
• session.variabledecls.test.ts L192-217 pins the exact patch grammar the override-set contract depends on. That's the test I'd write.
• CI: all required checks green; the failing "Perf: matrix.shard" is a template-substitution CI glitch unrelated to this diff.

— Review by Rames D Jusso

Stamped after Miga's stack review. GitHub state checked: mergeable, no red checks visible; #2046 base conflict still needs resolution separately.

R1 — hyperframes #2047 at 3c94b400

🟢 LGTM on the write mechanism, concurring with Miga; concur with Rames-D's 🟠 Hold on the loose id predicate.

Verified independently at head SHA:

• Write→read round-trip. writeVariableDeclaration accepts both typed CompositionVariable and raw entries via isRawDeclarationEntry guard — critical for apply-patches.ts:99-105 REPLAY fidelity (loose entries restored verbatim for undo). Intentional predicate looseness documented at the write site.
• Fragment guard. E_FRAGMENT_COMPOSITION rejection at mutate.ts:930 — serialize() strips the synthetic wrapper on fragments, so a declaration written there would silently vanish. Locked in by session.variabledecls.test.ts:601-611. (Semantics narrow further in #2081 — see my note there.)
• Patch-grammar collision-free. /variableDeclarations/{id} ↔ varDecl.{id} distinct from /variables/{id} (value channel). keyToPath/pathToKey round-trip at :696-705. Paired /variables patch emitted when default changes so T3 override-set stays coherent regardless of replay order (:254-265).
• CSS compat channel. cssCompatChange maintains --{id} custom-props on declare/update/remove for scalar defaults; object-valued font/image correctly clear. Same helper wired through all three mutate handlers, so a future edit that touches one path can't drift.
• Idempotency + type conflict. declareVariable no-ops on duplicate id (E_DUPLICATE_VARIABLE); updateVariableDeclaration enforces id immutability (remove + declare to rename).
• 218 test lines covering undo/redo + CSS-prop restoration + serialize round-trip.
• CI green.

Concurring with Miga's LGTM.

Concurring with Rames-D's 🟠 Hold:

• isCompositionVariable id predicate is typeof v.id === "string" and nothing else. Rames-D's read is right: "", whitespace, foo.bar, brand/color, constructor-like reserved names all pass, and while the path grammar happens to survive because variableDeclPath / variablePath don't RFC-6902-escape, varDecl.{id} unambiguity is by convention not by construction. Downstream consumers (Studio autocompletion, --variables CLI parsing, CSS --{id} writes, PostHog attribution) will each independently assume something the SDK didn't enforce. Concur on the tightening: /^[A-Za-z_][A-Za-z0-9_-]*$/ (CSS-custom-prop-safe, matches the runtime's slug convention) surfaced as E_INVALID_ARGS from can(). Retrofit cost is much higher after a customer authors id: "brand.color" and Studio silently overwrites other keys.

No unique blockers beyond Rames-D's coverage.

R1 by Via

R2 at 53ad171.

Prior R1 🟠 (isCompositionVariable accepts "", whitespace, ., /, ~, HTML metacharacters — SDK last line before Studio/CSS/CLI make assumptions) resolved at mutate.ts:948-976: VALID_VARIABLE_ID = /^[A-Za-z_][A-Za-z0-9_-]*$/ + isValidVariableId gate in both handleDeclareVariable (returns invalidVariableIdErr at can()) and updateVariableDeclaration / other paths (returns EMPTY). Error hint at line 958 spells out the three downstream consumers (CSS custom-prop, data-var-*, CLI --variables) so authors know why the shape is enforced.

No residuals from my side. CI green.

— Review by Rames D Jusso

🟢 LGTM concurring with Rames-D — R2 at 53ad171.

R1 🟠 hold (loose isCompositionVariable id predicate — empty / whitespace / foo.bar / brand/color all pass) resolved via the anchored slug regex from Rames-D's R1 ask, surfaced as a new E_INVALID_VARIABLE_ID.

Verified at head:

• packages/sdk/src/engine/mutate.ts:958 — const VALID_VARIABLE_ID = /^[A-Za-z_][A-Za-z0-9_-]*$/; — the exact regex from the R1 ask.
• mutate.ts:964-970 — new E_INVALID_VARIABLE_ID code + hint calls out that ids become CSS custom-property names, data-var-* attribute values, and CLI --variables keys. Good rationale embedded in the error.
• mutate.ts:984-985 — declarationPreconditionErr surfaces via can() before dispatch.
• mutate.ts:996-999 — handleDeclareVariable defensive re-check inside dispatch. Belt-and-suspenders.
• packages/sdk/src/engine/session.variabledecls.test.ts:86-97 — rejection loop over ["", " ", "has space", "1leading", "dot.id", "sla/sh", 'quo"te'] all yield E_INVALID_VARIABLE_ID; positive control brand_color-2 declares cleanly.

The constructor prototype-key hazard I named in R1 is orthogonal — declarations are stored as element-attribute JSON, not on a plain object, so the prototype-pollution vector doesn't apply.

No residuals from my side.

R2 by Via