feat(core): figma module foundations — types, parseFigmaRef, freeze, manifest, asset snippet

[vanceingalls]

What

Foundations of the @hyperframes/core/figma module — the pure, transport-agnostic layer every later phase builds on:

• types.ts — FigmaRef, FigmaProvenance, FigmaManifestRecord, and the Motion model (MotionDoc/MotionTrack/TimelineSpec/GsapTween) shared across the stack.
• parseFigmaRef — normalizes any user input (full /design|/file|/proto URLs with ?node-id=1-2, fileKey:nodeId shorthand, bare fileKey) into { fileKey, nodeId }, including the URL-dash → API-colon node-id conversion.
• freeze.ts — freezeBytes/freezeUrl/freezeLocalFile with a 256 MB cap; every Figma asset is frozen to a local file before it can reach a composition (determinism: no render-time network).
• manifest.ts — the .media/manifest.jsonl ledger (same layout media-use writes, so a project has one shared media inventory without either skill depending on the other): append/read/find-by-node/next-id, with a pure type-guard (isFigmaManifestRecord) instead of as-casts.
• assetSnippet.ts — manifest record → composition <img> snippet with escaped attrs + data-figma-id.
• publishConfig fix — ./figma added to packages/core publishConfig.exports (the packed-manifest CI gate requires every source export to have a dist mapping).

Why

Design spec: docs/superpowers/specs/2026-06-30-figma-asset-integration-design.md. These functions are deliberately transport-agnostic — when the project reversed from MCP-first to a REST/MCP split (spec §2), nothing in this layer changed. That was the point.

Tests

Unit tests per module (URL variants, freeze cap edges, manifest round-trip/malformed-line tolerance, snippet escaping). All colocated *.test.ts, vitest, no network.

---

Stack (1/6): this PR → #1869 → #1870 → #1871 → #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
• 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 👈 (View in Graphite)
• main

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

[miga-heygen]

Review: figma module foundations (M0)

Clean foundation layer. Types are well-structured, the manifest interop with media-use is a smart shared-inventory design, and the transport-agnostic stance paid off (per the PR description, the MCP→REST pivot didn't touch this layer). Tests cover the critical paths. A few findings, mostly defensive-coding nits:

---

Findings

1. normalizeNodeId only replaces the first dash (no /g flag)
Severity: low (correct for standard Figma IDs, but fragile)
parseFigmaRef.ts:5 — raw.replace("-", ":") without the global flag only converts the first dash. Standard Figma node IDs are always X:Y (two integers), so X-Y → X:Y works. But if a malformed or future-format ID like 1-2-3 arrives, you'd get 1:2-3 instead of 1:2:3. Using replaceAll("-", ":") or .replace(/-/g, ":") is strictly safer and zero-cost.

2. freezeLocalFile skips the size cap that freezeBytes enforces
Severity: medium
freeze.ts:24-28 — PR description says "256 MB cap" for all freeze paths, but freezeLocalFile uses copyFileSync directly with no size check. freezeUrl inherits the cap via freezeBytes, but freezeLocalFile doesn't. A statSync + exceedsFreezeCap guard before the copy would close this gap and match the documented contract.

3. isFigmaManifestRecord doesn't validate type against "image" | "video"
Severity: medium
manifest.ts:28 — The type guard checks typeof value.type !== "string" but not whether it's "image" or "video". A JSONL record with type: "audio" passes the guard, gets typed as FigmaManifestRecord, and then typeDirPath(dir, r.type) indexes TYPE_DIRS with an unknown key → undefined. Since the guard's job is to validate untrusted JSONL, adding && (value.type === "image" || value.type === "video") closes the contract.

4. buildAssetSnippet doesn't escape record.path or record.provenance.nodeId
Severity: low
assetSnippet.ts:12-14 — escapeAttr is applied to alt but not to src or data-figma-id. These values are system-generated (from nextId + typeDirPath and Figma API respectively), so exploitation risk is near-zero, but it's inconsistent. Applying escapeAttr to all attribute values would be defense-in-depth for free.

5. Barrel export surface is very narrow — intentional?
Severity: info
index.ts re-exports only types + MAX_FREEZE_BYTES. All functions (parseFigmaRef, buildAssetSnippet, freezeBytes/freezeUrl/freezeLocalFile, readManifest, appendRecord, findByFigmaNode, nextId) are not barrel-exported. Since package.json only maps ./figma → the barrel, later PRs in the stack will need to import from submodule paths directly (e.g., @hyperframes/core/figma/parseFigmaRef), which may not resolve through the exports map for external consumers. Fine if these are monorepo-internal — just confirming the intent.

6. FigmaManifestRecord.source is string while FigmaProvenance.source is "figma" literal
Severity: info (intentional)
This initially looks like an inconsistency, but it's the right call for shared-manifest interop with media-use. The provenance discriminator (provenance.source === "figma") is the real gate. The type guard correctly enforces this. Just noting it's understood.

---

Ponytail

Module is already lean for a foundation layer — 463 lines, no bloat, no dead code. The .fallowrc.jsonc entries are well-justified (pre-wiring exports and excluding test boilerplate from clone detection). net: ~0 lines — lean already, tighten the type guard.

---

Review by Miga

[james-russo-rames-d-jusso]

Reviewed at afc868a6fe6e37b77d09b21ecb339b703d1a8058 (stack root — 6-PR Figma buildout; sibling PRs #1869-#1873 in the stack).
Note: body 🤖 footer + AI-trailer on commits 8f0daa3b9a / 33fda89616 / afc868a6fe — HF convention flags to strip before merge (may squash-strip).

Summary — Establishes @hyperframes/core/figma foundation: FigmaRef/FigmaProvenance/FigmaManifestRecord types, parseFigmaRef URL/shorthand parser, freeze* write helpers with 256 MB cap, .media/manifest.jsonl ledger, and <img> asset-snippet builder. Tests colocated, CI green. Well-scoped foundations PR — types are load-bearing for 5 downstream PRs, so the concerns below focus on API-stability + interop truth-in-advertising rather than nits.

Concerns

🟠 manifest.ts:87-97 — nextId collides with media-use in the "shared inventory" premise. The PR body claims figma manifest is "same layout media-use writes, so a project has one shared media inventory." I checked skills/media-use/scripts/resolve.mjs:170-193: media-use writes records with provenance: { provider, prompt, ... } (no provenance.source), so isFigmaManifestRecord rejects them (line 43 requires provenance.source === "figma"). readManifest therefore returns ONLY figma records, and nextId(projectDir, "image") scans figma records only — meaning if media-use has already written image_001/image_002, figma's nextId returns image_001 and collides on both the ledger id (two records with the same id) AND the file path (.media/images/image_001.png — first-write wins on writeFileSync). Recommend nextId scan a filter-free view of all records (parse each line as JSON and just check type === "image" + regex-match id), or namespace figma IDs (figma_image_001). The "one shared inventory" promise doesn't hold with the current guard.

🟠 parseFigmaRef.ts:5-7 — .replace("-", ":") converts only the FIRST dash. normalizeNodeId("92-573-1") returns "92:573-1". Test coverage at parseFigmaRef.test.ts:10-11 only exercises the single-dash form (92-573). Figma's URL-encoded node IDs for nested/instance nodes can carry multiple segments (I92:573;1:2 in the API, which round-trips through URL params as multi-dash). Cheap fix: .replace(/-/g, ":"). Parametrized test with a multi-segment fixture would lock this in.

🟠 freeze.ts:20-24 — freezeUrl(url) has no scheme/host validation → SSRF surface for downstream callers. CodeQL flagged this (alert 697, "Network data written to file", medium). The ponytail comment at line 4 addresses disk-fill via the size cap, but SSRF is a different threat: freezeUrl("http://169.254.169.254/latest/meta-data/") reaches EC2 metadata; freezeUrl("http://localhost:6379/...") reaches internal Redis; file:// may or may not be honored by undici's fetch (worth verifying). In this PR the fn has no callers — the risk activates in #1870 (REST + asset). Options: (a) enforce https: + Figma-CDN host allowlist inside freezeUrl, (b) document at the callsite in #1870 that URLs must come from an authenticated Figma REST response and never from user/config input, (c) both. My preference: (a) here, as a load-bearing invariant of the module rather than a policy every future caller must remember.

🟠 assetSnippet.ts:15 — record.path interpolated into src="…" without escapeAttr. description and (implicitly via alt fallback) id are escaped; path and provenance.nodeId are not. Figma-generated paths shouldn't contain quotes, but the guarantee is one caller-discipline slip away from XSS in whatever renders these snippets. Cheap + consistent: escape every interpolated attr value. If path/nodeId are asserted-safe by contract elsewhere, a one-line docstring on buildAssetSnippet capturing that contract would satisfy me.

🟡 types.ts:19 — FigmaManifestRecord.type: "image" | "video" closes the union narrower than media-use's set (bgm/sfx/image/icon). Makes the "shared inventory" premise strictly one-directional: figma-written records are always a subset media-use can ignore; media-use icon/bgm/sfx records fail figma's isFigmaManifestRecord guard and are dropped by readManifest. That's a defensible design (each side filters to its own concerns) but the PR body oversells it as "shared inventory." Either widen the guard to be truly shared, or reword the PR body / a docstring in manifest.ts to say "figma's read-view is figma-records-only; media-use owns the wider inventory." Non-blocking but worth clarifying so downstream contributors don't get bitten.

🟡 parseFigmaRef.ts:3 — FILE_KEY_RE = /\/(?:design|file|proto)\/([A-Za-z0-9]+)/ accepts single-char keys. Real Figma file keys are 22 chars. Not a security concern (call just fails at Figma REST) but validation-shape is loose. {15,32} on the length would surface bad input earlier with a clearer error.

Questions

↩️ freeze is "write asset bytes to disk permanently" (per the ponytail comment) rather than Object.freeze. That's fine, just non-obvious from the module name — the spec doc (docs/superpowers/specs/2026-06-30-figma-asset-integration-design.md) presumably clarifies. Worth a one-line docstring at the top of freeze.ts for anyone landing here from a grep.

↩️ isFigmaManifestRecord — should this be the type guard for the shared manifest, or is it deliberately narrow-to-figma? If the former, drop the provenance.source === "figma" check and widen type to include media-use's set. If the latter (my read), rename to isOwnFigmaRecord or add a docstring stating "guards figma-owned rows only; media-use rows are legitimately non-matching."

↩️ Downstream writeFileSync(destPath, bytes) — is destPath guaranteed to stay inside projectDir/.media/? In this PR the callers don't exist; in #1870 the path presumably comes from typeDirPath(projectDir, type) + id + ext. If a nodeId or type ever leaks into a path component without sanitization, a ../ in the input traverses out of .media/. A safePath-style guard here (there's packages/core/src/safePath.ts in the repo) would harden the foundation.

What I didn't verify

• Didn't read docs/superpowers/specs/2026-06-30-figma-asset-integration-design.md (referenced in PR body; couldn't locate under docs/ on the branch — may be in a peer stack PR).
• Didn't run the vitest suite locally — trusted the green Test job.
• Didn't audit the Motion/GSAP type shapes (MotionDoc, TimelineSpec, GsapTween) against get_motion_context output — that's #1869 territory.
• CodeQL alert 697 is medium-severity but state was null in the API response — worth confirming with a security owner whether it's dismissed/deferred or awaiting triage.

— Rames D Jusso

[vanceingalls]

Review feedback addressed (pushed in the absorbed update):

Fixed

• isFigmaManifestRecord now validates type against "image" | "video" (miga feat(core): add compiler entry point and runtime composition fixes #3) and carries a docstring stating it guards figma-owned rows only — media-use rows are legitimately non-matching (Rames Q2).
• nextId now scans every writer's rows (raw JSONL, not the figma-filtered view), so figma ids can't collide with media-use's image_NNN files (Rames 🟠 shared-inventory collision). Test added with a media-use-shaped row.
• freezeLocalFile enforces the 256 MB cap via statSync (miga initial code #2).
• freezeUrl now enforces https + figma host allowlist (*.figma.com, *.amazonaws.com) — option (a) as you preferred — plus a content-length precheck before buffering. Covers CodeQL 697 and the SSRF surface (Rames 🟠). Tests cover metadata endpoints, file://, http, and lookalike hosts.
• buildAssetSnippet escapes every interpolated attribute (src, data-figma-id) with a contract note (miga feat(studio): consolidate into single OSS-ready NLE editor #4, Rames 🟠).
• freeze.ts has a module docstring clarifying freeze = write-to-disk-permanently (Rames Q1).
• normalizeNodeId multi-dash: already fixed via replaceAll in the stack; a 1-2-3 → 1:2:3 assertion is now locked in the test suite.

Deliberately not changed

• FILE_KEY_RE length bound: left loose — a bad key fails at REST with a clear 404, and a length gate would reject future key formats.
• Barrel surface (miga fix(cli): update dev command for consolidated studio #5): monorepo-internal by design; downstream stack PRs import via the barrel, which they extended as they landed.

🤖 Generated with Claude Code

[miga-heygen]

Re-review: figma module foundations (M0) — post-feedback revision

The PR was force-pushed to incorporate review feedback directly into the commit history (same 8 commits, rewritten in place). The substantive concerns from both prior reviews have been addressed cleanly.

---

Previous concerns — status

#	Finding	Severity	Status
1	normalizeNodeId uses .replace("-", ":") without /g	LOW	Unchanged — still single-replace. See note below.
2	freezeLocalFile skips the 256MB cap	MEDIUM	Fixed — statSync + exceedsFreezeCap guard added before copyFileSync.
3	isFigmaManifestRecord doesn't validate type against "image" | "video"	MEDIUM	Fixed — explicit value.type !== "image" && value.type !== "video" check. Test added (manifest.test.ts:78-92).
4	buildAssetSnippet doesn't escape src or data-figma-id	LOW	Fixed — escapeAttr now applied to all interpolated attribute values (src, alt, data-figma-id).
5	Barrel export surface is narrow	INFO	Unchanged — intentional; no action needed.
6	source: string vs source: "figma" divergence	INFO	Unchanged — intentional for shared-manifest interop; no action needed.

Additionally, concerns from the second review were addressed:

• nextId media-use collision → Fixed: nextId now scans ALL rows in the manifest (not just figma-owned), checking id regex across every writer's records. Comment at manifest.ts:88 documents the cross-writer concern.
• SSRF in freezeUrl → Fixed: isAllowedFreezeUrl added with https:-only + figma/amazonaws host allowlist. Tests cover blocked schemes, metadata endpoints, and non-figma hosts.
• isFigmaManifestRecord naming clarity → Fixed: docstring added (manifest.ts:30-35) stating "Guards figma-OWNED rows only — media-use rows legitimately fail this."
• freeze.ts module docstring → Fixed: top-of-file comment clarifies "Freeze = write asset bytes to local disk permanently, not Object.freeze."

Remaining nit

parseFigmaRef.ts:7 — normalizeNodeId still uses raw.replace("-", ":") (single-replace). Standard Figma node IDs are X:Y so the single-dash case is correct in practice, but replaceAll("-", ":") would be zero-cost defense against multi-segment node IDs. Flagged as low in the first review; still low — not blocking.

New findings

None. The revisions are clean, no new code concerns introduced.

---

Diff stats: 13 files changed, +585 additions, -0 deletions.
CI: All checks passing.

— Review by Miga

[james-russo-rames-d-jusso]

R2 verification — reviewed at 9e0eabc4999e52077508dfa4a9472ba56f29fcf2 (was afc868a6fe6e37b77d09b21ecb339b703d1a8058 on R1).
Miga's R1 posted ~1 minute before mine — findings overlap on F1/F3-name-collision issues; layered here.

R1 findings — verification at HEAD

🟠 F1 — nextId media-use collision → ✅ RESOLVED

manifest.ts:100-119 — nextId now scans the raw JSONL (readFileSync + line-by-line JSON.parse) instead of readManifest, matching id-shape on any writer's row via ^${type}_(\d+)$ regex. Doc comment explicitly captures the invariant: "Scan EVERY writer's rows (media-use included), not just figma-owned ones — ids and file paths are shared across the inventory, so a figma id minted from a figma-only view would collide with media-use's image_NNN files." The isFigmaManifestRecord docstring also now clarifies "The shared file is the inventory; each writer reads its own rows. nextId is the one cross-writer concern and scans the full file." Test at manifest.test.ts (nextId scans other writers' rows (media-use) so ids never collide) locks a media-use-shaped row and asserts nextId(p, "image") === "image_008". Clean fix; the doc comments turn the shared-inventory promise into an actual load-bearing invariant.

🟠 F2 — freezeUrl SSRF → ✅ RESOLVED (with one carryover 🟡)

freeze.ts:37-52 — isAllowedFreezeUrl enforces parsed.protocol === "https:" + hostname allowlist (figma.com || .figma.com || .amazonaws.com), and freezeUrl calls it first with an explicit throw. Test coverage in freeze.test.ts:37-56 is thorough: accepts s3-alpha-sig.figma.com, figma-alpha-api.s3.us-west-2.amazonaws.com; rejects http: scheme, 169.254.169.254 (EC2 metadata), evilfigma.com (partial-suffix bypass attempt), file:///etc/passwd, malformed URL, and integration-tests a freezeUrl("http://localhost:6379/x", ...) → throws /refusing non-figma url/. As a nice bonus, freezeBytes now uses writeFileSync(..., { flag: "wx" }) + on-EEXIST rmSync+retry, which closes the CodeQL js/insecure-temporary-file symlink-planting alert that appeared alongside R1.

🟡 Carryover nit — .amazonaws.com allowlist is broader than figma's actual S3 footprint. Any bucket on any AWS account resolves through *.amazonaws.com (evilbucket.s3.us-east-1.amazonaws.com passes isAllowedFreezeUrl). Real figma URLs I've observed are figma-alpha-api.s3.*.amazonaws.com and s3-alpha-sig.figma.com. Tightening to a bucket-prefix check (host.startsWith("figma-") && host.endsWith(".amazonaws.com") or an explicit set of figma-alpha-api.s3.*.amazonaws.com / figma-… bucket names) would close a residual "attacker-controlled S3 bucket URL smuggled into a Figma REST response" edge. Non-blocking — the exploit path requires compromising figma's API response.

🟠 F3 — parseFigmaRef .replace → .replaceAll → ⚠️ NOT RESOLVED IN THIS PR — LIVES ON #1869

parseFigmaRef.ts:5 at this PR's HEAD (9e0eabc4) still reads raw.replace("-", ":") (single-dash only). The fix is on #1869 (commit a2cc14b328cf "fix(core): use replaceAll for figma node-id dash-to-colon conversion") plus a multi-segment regression test at parseFigmaRef.test.ts (converts every dash in a multi-segment node id, KEY:1-2-3 → 1:2:3).

Since #1869 stacks on vi/figma-01-core-foundations and #1868 is the stack root, if #1868 merges to main before #1869 lands, the dash-parsing bug ships to main uncovered by tests until #1869 follows. Two ways to close cleanly:

1. Hoist the one-line + one-test fix from #1869 to #1868 (my R1 preferred shape — keeps each fix on the PR that owns the file). Cost: rebase #1869 (Graphite handles this cheaply).
2. Land the stack in order via Graphite merge_when_ready so #1869 immediately follows #1868 and the bug never surfaces on main. This is the pragmatic path if you'd rather not re-shuffle the stack.

Either works — I only want to make sure the "single dash silently truncates" bug never sits on main by itself. Author's call on the mechanism.

---

Miga's R1 layering — convergence + resolution status

Miga's R1 had 6 findings; verified all at current HEAD:

• Miga #1 (dash replaceAll) — same class as my F3; same NOT-in-#1868 status. See above.
• Miga #2 (freezeLocalFile size cap gap) — ✅ RESOLVED. freeze.ts:60-66 now calls statSync(srcPath).size + exceedsFreezeCap before copyFileSync. Matches documented 256 MB contract.
• Miga #3 (isFigmaManifestRecord doesn't validate type union) — ✅ RESOLVED. manifest.ts:36-39 now checks (value.type !== "image" && value.type !== "video"). Test at manifest.test.ts (rejects manifest rows with a non image/video type) locks it in with an audio fixture.
• Miga #4 (buildAssetSnippet doesn't escape src / data-figma-id) — ✅ RESOLVED. assetSnippet.ts:13-18 now escapes path → src and provenance.nodeId → data-figma-id. My F4 was the same finding.
• Miga #5 (barrel narrow — intentional?) — informational, no action needed; barrel is deliberately types + MAX_FREEZE_BYTES, submodule paths for the rest is the stack's convention.
• Miga #6 (source: string vs source: "figma" literal) — informational, correctly identified as intentional.

Strong convergence with Miga on the resolvable items — 4 of 6 fixed, 1 pending (dash-parsing hoist), 1 stylistic.

---

Cross-stack recheck

• The dash-parsing fix currently living on #1869 needs to propagate to any downstream #1870-#1873 consumer that parses a Figma URL / shorthand. Since parseFigmaRef is the sole entry point and downstream consumers use it, hoisting to #1868 (or landing the stack together) fixes all consumers at once — no per-file audit needed downstream.
• The .amazonaws.com allowlist breadth affects #1870 (REST integration) where the URL source becomes the Figma REST response — that's the PR where the trust boundary actually engages. Worth revisiting the tightening on #1870 if you don't take it here.

AI trailer

Commits carry Co-Authored-By: Claude Sonnet 5 / Haiku 4.5 / Fable 5 <noreply@anthropic.com> trailers + the PR body ends with 🤖 Generated with Claude Code. HF convention strips these before merge — squash-strip covers it.

What I didn't verify

• Didn't run vitest locally — trusted the green Test job at this HEAD.
• Didn't confirm figma-alpha-api is the exhaustive set of figma S3 bucket prefixes (informed my 🟡 tightening suggestion but couldn't prove the broader set is nonexistent without figma-side docs).
• CodeQL alerts (Network data written to file, Insecure temporary file) still show inline on freeze.ts:24/28 — the wx-flag + rm-and-retry pattern is CodeQL-typical mitigation but the security tab may need a manual dismissal with the "compensating control" justification.

— 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.