feat(core,cli): figma tokens import with alias-aware binding records (M2)

[vanceingalls]

What

M2: Phase-2 tokens import with alias-aware binding records.

• tokensToVariables.ts — pure translator: Figma variables payload → composition brand-variable entries (COLOR→hex/rgba(), FLOAT/STRING/BOOLEAN), a human-readable figma-tokens.json sidecar, and binding-index records. Alias chains are walked cycle-safe to the leaf value, but the binding keeps the semantic id the designer bound — swapping the primitive underneath doesn't orphan the link (spec §7.1 rule 4). Alias cycles skip the variable instead of hanging.
• hyperframes figma tokens <fileKey> — variables path writes entries + sidecar + .media/figma-bindings.jsonl; REQUIRES_ENTERPRISE (variables are Enterprise-gated upstream) degrades cleanly to published-styles metadata (style values resolve at component-import time, Phase 3); any other failure propagates.

Why the binding records matter

They're the join that lets #1872's component import emit var(--brand-role, #literal) instead of duplicating hexes — the thing that makes a later brand refresh propagate through imported components with zero re-touch. Design: spec §7.1 (tokens-before-components, exact-ID matching only).

Tests

Translator fixtures (hex/rgba conversion, alias chain + recorded chain, cycle survival, provenance stamping, sidecar completeness) + CLI tests for both paths and error propagation.

---

Stack (4/6): #1868 → #1869 → #1870 → this PR → #1872 → #1873

🤖 Generated with Claude Code

[vanceingalls]

• feat(skills): reroute /figma by capability - REST/CLI for phases 1-3, MCP for 4-5 (M4) #1873
• feat(core,cli): figma component import with binding-aware node-to-html mapper (M3) #1872
• feat(core,cli): figma tokens import with alias-aware binding records (M2) #1871 👈 (View in Graphite)
• feat(core,cli): figma REST client, asset import command, binding index (M0+M1) #1870
• feat(core): figma motion → GSAP translator + /figma skill v1 #1869
• feat(core): figma module foundations — types, parseFigmaRef, freeze, manifest, asset snippet #1868
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[miga-heygen]

Review — figma tokens import with alias-aware binding records (M2)

Solid PR. The translator is clean, the alias resolution is cycle-safe, the binding index records are well-structured, and the enterprise fallback path is correctly scoped. Tests cover the important cases. A few observations:

Findings

1. firstModeValue ignores defaultModeId from the collection (minor)
tokensToVariables.ts:85 — firstModeValue grabs whichever mode Object.values yields first, which relies on insertion order. The variableCollections payload carries defaultModeId — the correct "which mode is the base" signal — but the translator never consults it. For single-mode files this is fine (the test fixtures all use one mode), but multi-mode files (Light/Dark) will silently resolve to whichever mode V8's property order delivers. Consider passing defaultModeId through and resolving that mode explicitly. Not blocking since the spec says "first mode" is the MVP behavior for Phase 2 and multi-mode comes later, but worth a comment.

2. Duplicate isRecord helper (nit)
tokensToVariables.ts:56 defines isRecord — identical to bindings.ts:43. Both are private, so no runtime cost, but it's the third copy in packages/core/src/figma/. A shared util would keep it DRY.

3. Sidecar written twice on enterprise fallback (nit)
In tokens.ts:47-48, the variables path writes the sidecar inside the try block, then the styles fallback writes it again. If the variables call succeeds but the sidecar write fails (disk error), the catch rethrows correctly — no issue. But the asymmetry means a hypothetical future "append" mode would need to reason about both write sites. Fine for now; just noting the split.

4. styles() response shape — node_id vs key fallback (observation)
tokens.ts:59 uses s.node_id ?? s.key as the figmaId for style sidecar entries. The Figma REST API's published styles endpoint returns node_id as optional — some styles genuinely lack it. Using key as fallback is reasonable, but the binding index's figmaId field is spec'd as "the figma variable/style id as it appears in node data (exact match key)." A key there won't match what Phase 3's component import sees in node property bindings (boundVariables use the variable id, not the key). Since the styles path emits no binding records (entries is []), this doesn't affect resolution — but if that ever changes, the mismatch would bite. Worth a comment to lock it down.

5. Test fixtures don't exercise variableCollectionId paths (observation)
The VARS fixture in tokensToVariables.test.ts includes variableCollectionId: "c1" and a variableCollections map with defaultModeId, but the translator doesn't use either. The test fixtures are accurate for the current implementation — just noting the unused data as a breadcrumb for when multi-mode lands.

What I like

• Alias resolution is exactly right. Walking to the leaf value for the CSS default while recording the semantic bind-point on the binding record — this is the design that makes brand-refresh propagation work without re-import. The cycle detection via chain.includes(currentId) is simple and correct for DAG-shaped alias graphs.
• Clean separation of concerns. Pure translator function, DI-injectable client in the CLI command, binding records as JSONL append — each piece is testable in isolation and the CLI test proves the integration works.
• Error boundary is tight. Only REQUIRES_ENTERPRISE triggers fallback; everything else propagates. The test for RATE_LIMITED propagation is a good contract test.

Ponytail

The code is already lean for what it does. The isRecord dedup would save ~5 lines across the figma module. net: -5 lines possible — not worth blocking on.

---

Review by Miga

[james-russo-rames-d-jusso]

Reviewed at ed3f672116 (stack member 4/6; sibling PRs #1868-#1870, #1872-#1873).
Note: body has 🤖 Claude Code footer + AI-trailer; HF convention flags this.

Summary — Adds tokensToVariables (pure Figma variables → composition brand-variable entries + sidecar + binding records) and hyperframes figma tokens <fileKey> CLI with Enterprise-gated variables path and styles-metadata fallback. Alias resolution is cycle-safe. Concerns are around name-collision in composition IDs, multi-mode value selection, and idempotency of the binding index.

Concerns

• 🔴 compositionVariableId = "figma:" + payload.name — silent collision across collections. tokensToVariables.ts:121. Figma allows the same variable name in different collections (a "Semantic" collection with Blue/500 and a "Primitive" collection with Blue/500 is a common brand pattern). Two variables with different figmaIds produce the same compositionVariableId, then entries[] gets two rows with duplicate id, and the later bindings[] row overwrites the runtime semantics of the earlier one. Design spec §7.1 rule 4 (alias binding survives primitive swaps) relies on the composition id being 1:1 with the semantic binding — a collision breaks that guarantee. Suggest namespacing by collection: figma:<collectionName>/<name> or figma:<figmaId> (loses human readability but guaranteed unique) or dedupe with Map<name, existing> and warn.

• 🔴 Rerunning hyperframes figma tokens appends duplicates to .media/figma-bindings.jsonl. tokens.ts:45 — for (const b of out.bindings) appendBinding(deps.projectDir, b) — no dedup, no truncate. Second run doubles the file, third triples. findBindingByFigmaId in bindings.ts:89-93 returns the FIRST match (line iteration order = insertion order), so the stale record from the first run wins on subsequent lookups. For the brand-refresh workflow this PR exists to enable ("swap the primitive underneath doesn't orphan the link"), the stated goal fails on re-import. Suggest either (a) truncate .media/figma-bindings.jsonl at the top of runTokensImport before writing (rebuild-from-scratch semantics — matches sidecar's writeFileSync), or (b) upsert by figmaId in appendBinding.

• 🟠 Multi-mode fidelity: firstModeValue returns the first Object.values entry, ignoring defaultModeId. tokensToVariables.ts:73-77. FigmaVariablesResult.variableCollections[collectionId].defaultModeId is the correct authority for "what value should the composition use" — the test at tokensToVariables.test.ts:33 even sets it, but the code never reads it. For a single-mode collection this happens to work; for light/dark or any multi-mode setup, the value picked depends on JS property-iteration order (insertion order in V8) — so Dark before Light in the REST payload silently gives the composition the dark hex as its default. Fix path: narrow variableCollections in client.ts to expose defaultModeId, and have firstModeValue prefer valuesByMode[defaultModeId] with the current behavior as fallback.

• 🟠 toEntryValue accepts string for FLOAT. tokensToVariables.ts:100-107. If payload.resolvedType === "FLOAT" but the resolved value comes through as "8" (Figma occasionally stringifies), the function returns the string and the entry gets { type: "number", default: "8" } — CompositionVariableEntry contract is broken silently. Tighten: if (resolvedType === "FLOAT") return typeof raw === "number" ? raw : null; and similar for BOOLEAN/STRING.

• 🟠 CLI try block swallows more than the client call. tokens.ts:42-50 — the try wraps client.variables(...) AND tokensToVariables(...) AND appendBinding(...) AND writeFileSync(...). If any of those throw for reasons OTHER than REQUIRES_ENTERPRISE on variables(), control falls through to the styles fallback, which then makes a second network call and writes a different sidecar over the (possibly partial) one from the first attempt. Scope the try to deps.client.variables(fileKey) only; move the translator + writes outside.

Nits

• 🟡 sidecar.tokens includes unresolvable variables with value: null. tokensToVariables.ts:123-129, pushed before the if (!entryType || value === null || !resolved) continue. Likely intentional (designer visibility into what didn't map), but not obvious from reading; a one-line comment above the push would help future readers understand the invariant divergence between sidecar and entries.
• 🟡 Cycle detection chain.includes(currentId) is O(n²) per chain. tokensToVariables.ts:86. Token counts are tiny (~hundreds), fine — but a Set<string> would be cheap and future-proof, especially for larger design systems.
• 🟡 Binding aliasChain includes figmaId as its first element. tokensToVariables.ts:144 gates on resolved.chain.length > 1, so for a directly-resolved variable the field is omitted (good). For an alias, though, aliasChain[0] === figmaId; bindings.ts:91 then does b.aliasChain?.includes(figmaId) which will match a binding via its own figmaId — redundant with the b.figmaId === figmaId check on the line above, harmless.
• 🟡 Blue/500 in id: "figma:Blue/500" embeds a / in an identifier. Not invalid, but any downstream consumer that treats these ids as HTTP paths or CSS classes will trip. If the runtime's getVariables() returns them as raw ids there's no risk — worth a docstring note.

Questions

• ↩️ Design spec §7.1 rule 4: "the binding keeps the semantic id the designer bound — swapping the primitive underneath doesn't orphan the link." I read the code as recording the original figmaId (which is the semantic-bound id) with the alias chain intact, and always resolving to the leaf for the runtime value. That's correct as long as re-imports respect the existing binding when the primitive value changed but the semantic id didn't — which loops back to the duplicate-append concern above. Is the intended flow "always re-import from scratch" or "upsert / diff against existing bindings"?
• ↩️ Styles-fallback sidecar has value: null for every entry (tokens.ts:61), on the grounds that "values resolve at component-import time (Phase 3)". Where does that resolution actually happen for a style:FILL — is Phase 3 (#1872 component) going to write over this sidecar with resolved values, or is the composition's runtime expected to defer to some Figma-side lookup?
• ↩️ payload.name is used as both label and part of the id. Figma variable names contain / for grouping (e.g. Blue/500), spaces, unicode. entries[].id and label both get the raw name. Is that intentional for both, or should label be human-readable (Blue/500) and id be a slugified form?

What I didn't verify

• Did not read #1868 (foundations) client shape end-to-end for FigmaVariablePayload completeness — took the types in client.ts at their word.
• Did not verify the actual data-composition-variables runtime contract; CompositionVariableEntry shape is asserted here but I didn't cross-read the studio/runtime side to confirm type values line up with the getVariables consumer.
• Did not run the tokens command against a real Figma file or verify that a REQUIRES_ENTERPRISE 403 is the exact response Figma returns for the variables endpoint on non-Enterprise plans.
• Did not read #1872 (component import) to see how the binding records are actually consumed — the "makes brand refresh propagate" claim is unverified here.

— Rames D Jusso

[vanceingalls]

Review feedback addressed (pushed in the absorbed update):

Fixed — both 🔴s

• Cross-collection name collision (Rames 🔴): composition ids are now namespaced by collection — figma:Semantic/Blue/500 vs figma:Primitive/Blue/500. Same-name variables in different collections can no longer merge. Test locks in two distinct ids for the classic Semantic/Primitive pattern.
• Re-run duplicate appends (Rames 🔴): appendBinding-in-a-loop replaced with upsertBindings — rewrites rows whose figmaId is being re-imported, keeps other files' bindings and library rows. Stale-record-wins is gone; test covers replace + survivor + library-row retention. This is also the answer to your Q1: re-import is upsert-by-figmaId, so a primitive-value change under an unchanged semantic id updates in place.
• defaultModeId (miga Initial repo setup #1, Rames 🟠): baseModeValue prefers valuesByMode[collection.defaultModeId], falling back to insertion order only when the collection/mode is missing. Light/Dark test added.
• toEntryValue type fidelity (Rames 🟠): FLOAT/BOOLEAN/STRING each require the matching JS type — a stringified "8" for a FLOAT is rejected instead of silently breaking the entry contract. Test added.
• Over-wide try block (Rames 🟠): try now wraps only client.variables(); translator/write failures propagate instead of falling through to a second network call + sidecar overwrite.
• Cycle detection uses a Set (Rames 🟡); sidecar's include-unresolvable-tokens invariant has the explaining comment (Rames 🟡).

Answers

• Styles-fallback value: null: resolution happens at component-import time (feat(core,cli): figma component import with binding-aware node-to-html mapper (M3) #1872's resolveBindings against node data); the sidecar is not overwritten with resolved values.
• Blue/500 in ids: intentional — the runtime treats these as opaque ids; the CSS var name goes through slugify at emission (--figma-semantic-blue-500), so / never reaches CSS.
• aliasChain[0] === figmaId redundancy: harmless as you note; left as-is.

🤖 Generated with Claude Code

[james-russo-rames-d-jusso]

R2 verification — reviewed at 50c099fee5 (R1 was ed3f672116). Prior review 4621681203.

Verdict — 2× 🔴 blockers CLEARED, 1× 🟠 CLEARED. One test-coverage nit remains. Ready to merge on my end after Vance confirms the missing idempotency test is intentional (or adds it).

🔴 F1 — compositionVariableId collision — CLEARED ✅

R1: raw variable name → same-name variables across Semantic vs Primitive collections would collide.

Fix at packages/core/src/figma/tokensToVariables.ts:154-156:

const compositionVariableId = collection
  ? `figma:${collection}/${payload.name}`
  : `figma:${payload.name}`;

Namespaced by collection name (via new collectionName helper at :96-102 which reads from variableCollections[variableCollectionId].name). Fallback to figma:${name} only when the collection is missing from the payload — matches the pre-fix behavior for the no-collections test fixture.

Test at tokensToVariables.test.ts:107-120 explicitly exercises the collision case:

Blue/500 in "sem" collection + Blue/500 in "prim" collection
→ expect(new Set(ids).size).toBe(2)
→ expect(ids).toContain("figma:Semantic/Blue/500")
→ expect(ids).toContain("figma:Primitive/Blue/500")

This is the exact regression I flagged in R1. Fix + test both correct.

One theoretical residual: collision by identical collection-name (two collections literally named "Semantic") still produces the same namespace. Figma allows this — enterprise workspaces with imported team libraries sometimes end up with duplicate collection names. Would be worth using collection.key (stable cross-file identity) instead of .name at some point. Not a blocker — none of the R1 evidence points at this being a real user hit, and the fix as-shipped covers the reported failure mode. Filing as a followup for the Phase 3 library-map work.

🔴 F2 — jsonl duplicate-append — CLEARED ✅

R1: for (const b of out.bindings) appendBinding(...) in a loop meant re-running the tokens import (which happens on every brand refresh) appended duplicate rows, and findBindingByFigmaId returns the first match, pinning lookups to stale records forever.

Fix at packages/core/src/figma/bindings.ts:88-98 — new upsertBindings:

const incoming = new Set(records.map((r) => r.figmaId));
const survivors = readLines(projectDir).filter(
  (line) => !(isBindingRecord(line) && incoming.has(line.figmaId)),
);
mkdirSync(mediaDir(projectDir), { recursive: true });
const lines = [...survivors, ...records].map((r) => JSON.stringify(r)).join("\n");
writeFileSync(bindingsPath(projectDir), lines.length > 0 ? lines + "\n" : "");

Read-filter-rewrite via writeFileSync — no more append, so re-runs produce single-copy content. Library rows (kind: "library") and other files' bindings are correctly preserved because the filter only strips rows whose figmaId is in the incoming set. CLI at tokens.ts:51 switched from the appendBinding loop to upsertBindings(deps.projectDir, out.bindings). Correct.

Coverage gap (nit, not blocker) — the CLI test at tokens.test.ts:41-53 runs the importer ONCE and asserts the jsonl contains the expected figmaId. It doesn't run it twice and assert single-copy content. The code is inspection-correct but there's no regression pin. Suggest:

// second run with same input → identical file content
await runTokensImport("FILE", { projectDir: dir, client: client({}) });
const secondRun = readFileSync(join(dir, ".media", "figma-bindings.jsonl"), "utf8");
expect(secondRun).toBe(bindings); // same as first run

One-line follow-up, non-blocking — the fix is right, just would prevent future regression.

🟠 F3 — firstModeValue vs defaultModeId — CLEARED ✅

R1: reading Object.values(modes)[0] ignored Figma's canonical defaultModeId — multi-mode files (Light/Dark) would silently pick whichever mode V8's property order delivered.

Fix at tokensToVariables.ts:83-94 — new baseModeValue:

const collection = collections[payload.variableCollectionId ?? ""];
if (isRecord(collection) && typeof collection.defaultModeId === "string") {
  const preferred = modes[collection.defaultModeId];
  if (preferred !== undefined) return preferred;
}
for (const value of Object.values(modes)) return value;

Reads defaultModeId from the collection payload; falls back to first-inserted when the collection or its default mode isn't in the payload (which matches existing test fixtures that pass no variableCollections). Signature threaded through resolveValue so alias walks also respect the default mode.

Test at tokensToVariables.test.ts:122-140 explicitly asserts:

Ink variable with valuesByMode: { dark: white, light: black },
variableCollections.c.defaultModeId: "light"
→ expect(out.entries[0]?.default).toBe("#000000")

Insertion order would give the dark value first; picking defaultModeId explicitly is what makes this test pass. Fix + test both correct.

Cross-stack (#1872)

#1872's nodeToHtml reads bindings via findBindingByFigmaId, which returns the first match. F2's upsert fix means there's only ever one record per figmaId, so downstream is safe by construction. No changes needed on #1872's side.

Miga's R1 layering

Miga posted a COMMENTED review at 4621672114 three minutes before mine, flagging F3 as a minor observation (correctly — the R1 code was silently picking firstMode and Miga called out defaultModeId as the fix). She did not flag F1 (collection collision) or F2 (jsonl append duplication) — those were unique to my R1. Her nits 2/3/4/5 (isRecord dedup, sidecar-double-write on fallback, node_id ?? key for styles, unused variableCollectionId in fixtures) all still apply on the R2 commit — none are blockers. Worth Vance's judgement whether to sweep any of them in this PR or defer.

AI-trailer

PR body still has 🤖 Generated with [Claude Code](https://claude.com/claude-code) footer, and the merge commit message carries Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>. HF convention is to squash-strip both — nit for the merge step, not a code-review blocker.

What I didn't verify

• Runtime end-to-end: didn't spin up a real project directory and run the CLI twice against a live-ish Figma payload. Verification was code-inspection + test-reading at HEAD.
• Whether the "duplicate collection name" collision-collision (two collections named "Semantic") is a real workspace hit — flagged as followup, not blocker.

— Rames D Jusso

[miguel-heygen]

LGTM. Stamping after Miga R2 verified the prior Figma-stack blockers are addressed; live checks are green, with only Graphite stack mergeability pending where applicable.

The base branch was changed.