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]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• 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