Skip to content

feat(core,cli): media-use interop — shared index.md regen + description/entity on figma imports#1927

Merged
vanceingalls merged 1 commit into
mainfrom
07-03-feat_core_cli_media-use_interop_shared_index.md_regen_description_entity_on_figma_imports
Jul 4, 2026
Merged

feat(core,cli): media-use interop — shared index.md regen + description/entity on figma imports#1927
vanceingalls merged 1 commit into
mainfrom
07-03-feat_core_cli_media-use_interop_shared_index.md_regen_description_entity_on_figma_imports

Conversation

@vanceingalls

@vanceingalls vanceingalls commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

What

Post-release interop pass between the figma import module and the media-use skill — they share one .media/ inventory, and this PR makes the two writers actually behave like it (spec §13.1 records the full review; items 1–2 shipped here, 3–5 queued).

index.md regeneration (core)

hyperframes figma asset now regenerates .media/index.md — the agent-readable inventory table media-use maintains — after every import, fresh or reused. New packages/core/src/figma/mediaIndex.ts mirrors media-use's index-gen.mjs including its render-every-parseable-row selection (a shape filter would make the file flip-flop between writers). Parity is enforced, not assumed: a test executes the actual index-gen.mjs on identical rows (duration, width×height, icon+transparent, junk-row branches) and asserts byte-for-byte equality. Regen is best-effort — the import is durable before it runs, so an index write failure warns instead of failing the command.

--description / --entity on figma imports

Both flags land in the manifest record, the index table, and the <img alt> (previously alt="image_001"). Component rasterize auto-describes with the node name. entity makes figma-imported brand marks discoverable by media-use's resolve --entity cascade — and to make that promise real, media-use's entity gate now treats icon/image as equivalent (figma rows are always type: image; agents ask for logos as type: icon). Descriptions are whitespace-flattened so a newline can't corrupt the table.

Reuse-path correctness (from max-effort review, 11/13 findings fixed)

  • Metadata upsert on reuse — flags passed on a re-import update the existing row (updateRecord, in-place line rewrite preserving other writers' rows) instead of being silently dropped.
  • Tuple-aware reuse gate — the gate previously compared only the oldest row per node, so a second (format, scale, version) tuple defeated idempotency permanently and every import minted duplicate records/files/index rows. Now any matching tuple with an existing file reuses (findAllByFigmaNode).
  • registry-item.json honesty — rasterized assets now listed at the image_NNN.svg paths actually frozen (and referenced by the emitted HTML) instead of phantom ${slug}.svg files never written.
  • Windows-portable parity test — script path resolved relative to the test file and passed as a file:// URL (the cwd-regex + raw-path interpolation was fatal on windows-latest).
  • SKILL.md synopsis shows the flags its own prose mandates; dead barrel exports trimmed.

Deferred (spec §13.1 follow-up queue): shared media-ledger module (the generator duplication is guarded by the parity test until then), content-addressed global cache for figma assets, media-use version-keyed idempotency.

Testing

  • 109 figma tests (core + cli) including: reuse-path metadata upsert, any-tuple reuse, whitespace flattening, index regen on both paths, byte-parity vs the real media-use generator across all format branches + junk-row selection.
  • 13 media-use tests including a new cross-type entity hit (figma-imported image row resolved via --type icon --entity).
  • tsc --noEmit clean on core + cli; oxlint/oxfmt clean.

🤖 Generated with Claude Code

Copy link
Copy Markdown
Collaborator Author

…on/entity on figma imports

Post-release review of media-use ↔ figma coupling (spec §13.1):
- figma asset imports now regenerate .media/index.md, the agent-readable
  inventory media-use maintains — format locked byte-identical via a
  cross-runner parity test against media-use's own index-gen.mjs
- figma asset --description/--entity land in the manifest record, the
  index table, and <img alt>; component rasterize auto-describes with
  the node name. Named brand marks become visible to media-use's
  resolve --entity lookups.
- spec §13.1 records the review verdict (loose coupling correct) and
  the follow-up queue (shared media-ledger module, global cache for
  figma assets, media-use version-keyed idempotency)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@vanceingalls vanceingalls force-pushed the 07-03-feat_core_cli_media-use_interop_shared_index.md_regen_description_entity_on_figma_imports branch from d23464d to 45c0a2e Compare July 4, 2026 05:11

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — media-use interop: shared index.md regen + description/entity on figma imports

Verdict: LGTM with nits — solid interop work. The core design decisions (byte-parity via test, best-effort index regen, tuple-aware reuse, icon/image equivalence for entity hits) are all well-considered. Two findings worth addressing before merge; the rest is nits.


Findings

1. Row-selection parity gap between generators (low severity, nit)

mediaIndex.ts filters through isRow() which requires typeof value === "object" && value !== null, while media-use's readManifest() pushes every JSON.parse() result (including bare primitives like "hello" or 42 if someone hand-edited the JSONL). The byte-parity test covers the JUNK_ROW object case but not bare-primitive JSONL lines.

In practice this doesn't matter — no writer emits bare primitives — and the test guards the realistic scenarios. But if the stated goal is "selection parity, not assumed," tightening isRow to match (accept anything JSON.parse returns) or adding a bare-primitive JSONL line to the parity test would close the gap completely. Leaving as a nit since the parity test is the real guard.

2. updateRecord uses non-atomic write pattern (medium severity)

updateRecord does readFileSyncmapwriteFileSync. If the process crashes between read and write, the file could be truncated/corrupted. The appendRecord path is crash-safe (append-only), but updateRecord rewrites the entire file. Consider:

  • Writing to a temp file + renameSync for atomicity, or
  • At minimum, documenting this as a known limitation.

This matters more if concurrent processes (media-use scripts + figma CLI) could both write to manifest.jsonl simultaneously. The regenerateIndex path has the same pattern but is guarded by safeRegenerateIndex's try-catch so a partial write to index.md is recoverable (regen on next import). The manifest itself is the durable record, so corrupting it during updateRecord is the real risk.

3. isFigmaManifestRecord won't match media-use rows (by design, confirming)

The type guard requires provenance.source === "figma" with fileKey and nodeId. This means readManifest() in manifest.ts intentionally filters out media-use rows (bgm, sfx, voice, non-figma images). That's correct for figma operations — but regenerateIndex needs ALL rows, and it correctly uses readJsonlValues directly instead of readManifest. Just confirming this is intentional and correct.

4. cols.desc omitted from TypeScript generator (harmless)

Media-use has cols = { id: 4, type: 5, dur: 4, dims: 5, path: 5, desc: 11 } while mediaIndex.ts omits desc. Since desc is computed but never read in either implementation (description is the final unpadded column), this produces identical output. But if media-use ever starts padding descriptions, the drift would be silent — the parity test would catch it, so this is fine as-is.

5. typesMatch — consider extracting the visual set (nit)

function typesMatch(a, b) {
  if (a === b) return true;
  const visual = new Set(["icon", "image"]);
  return visual.has(a) && visual.has(b);
}

The Set is recreated on every call. This is called in a hot path (entity search iterates all records). For a manifest with dozens of records this is negligible, but hoisting const VISUAL_TYPES = new Set(["icon", "image"]) to module scope is free and idiomatic. Pure nit.

6. Component registry-item.json fix is correct and important

The old code listed phantom ${slug}.svg paths that were never written. The new code uses frozenAssets — the actual paths the asset import wrote. This is a real bug fix that prevents agents from referencing nonexistent files. Well done.

7. Test coverage is thorough

109 figma tests + 13 media-use tests covering: reuse metadata upsert, any-tuple reuse, whitespace flattening, index regen on both paths, byte-parity vs the real generator across all format branches + junk-row selection, and the new cross-type entity hit. The byte-parity test that actually executes index-gen.mjs and asserts string equality is the right guard for format drift.


Ponytail lens (over-engineering audit)

The PR is lean. normalizeMeta is a 3-line function that does exactly what it needs to. safeRegenerateIndex is a minimal best-effort wrapper. The len helper avoids repeating String(value ?? "").length. The isRow type guard is one line. No unnecessary abstractions, no premature generalization.

The only candidate for trimming is the desc: 11 difference (mentioned above) — adding it to the TS version for visual parity would add 1 line but doesn't change behavior.

Lean already. Ship.


Diff stats

12 files changed, +418 / -33

— Miga

@james-russo-rames-d-jusso james-russo-rames-d-jusso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 45c0a2e.

Verdict

🟢 approve with one 🟡 nit and two low-severity concerns. The two load-bearing claims — byte-identical index-gen and any-tuple reuse — both hold up, and the parity test is real (executes skills/media-use/scripts/lib/index-gen.mjs on shared input and asserts ===). The 11/13 reuse-path findings from the max-effort review look correctly landed.

What this PR actually does

  • Adds packages/core/src/figma/mediaIndex.ts as a second implementation of generateIndexContent mirroring media-use's index-gen.mjs, and calls it from runAssetImport on both mint and reuse paths via a safeRegenerateIndex wrapper (try/catch → warn, per PR body: "the import is durable before it runs").
  • Replaces the findByFigmaNode (oldest row) reuse gate with findAllByFigmaNode(...).find(matching tuple). Adds updateRecord to rewrite a manifest row in place by id, preserving other writers' rows.
  • Adds description + entity to FigmaManifestRecord and threads --description/--entity through runAssetImport → manifest, index.md, and <img alt>. Component rasterize auto-fills description from the figma node name.
  • component.ts now emits registry-item.json entries at the paths files actually landed at (image_NNN.svg) instead of the phantom ${slug}.svg.
  • skills/media-use/scripts/resolve.mjs — entity gate treats icon/image as interchangeable via a new typesMatch helper.

Byte-identical format claim

Verified via the parity test at packages/core/src/figma/mediaIndex.test.ts:97-107, which builds ALL_ROWS (media-use bgm/image/icon shapes + figma image + junk row), calls the local generator, spawns node --input-type=module on the actual skills/media-use/scripts/lib/index-gen.mjs, and asserts expect(ours).toBe(theirs). Covers the durations / width×height / icon+transparentsvg / no-dims / junk-row branches. The Windows fix (pathToFileURL + resolve relative to import.meta.url) is correct — matches the shape the prior review flagged.

Manual diff of generateIndexContent between mediaIndex.ts and index-gen.mjs shows only cosmetic drift (String(x) template-literal wrappers, and media-use's unused desc: 11 col-init). Both produce identical strings for every input the parity test exercises. Rendering-every-parseable-row is preserved — no shape filter, so the file doesn't flip-flop by writer.

Nit: the local generator's formatDur / formatDims use String(r.duration) / String(r.width) while media-use relies on template-literal coercion. Same output today, but drift-prone — the two generators are now the parity-test's job forever. packages/core/src/figma/mediaIndex.ts:32-39.

Reuse dedup key

(format, scale, version) tuple compared with existing.provenance.scale ?? 1 on both sides (canonical default). File-existence check (existsSync) is preserved — a deleted frozen file falls through to re-import. .find on the filtered node rows correctly picks the first matching tuple, so pre-existing duplicate rows from the mint-forever era stop growing without needing a backfill.

Version comparison is === on the string returned by client.fileVersion(ref.fileKey) — figma's file version is a monotonic string, so equality is right. No timestamp / semver comparison needed.

Upsert semantics

packages/cli/src/commands/figma/asset.ts:79-94 — the upsert applies only if description or entity was actually supplied on the re-import and differs from the existing row. Absent flags don't clobber prior values (...(description !== undefined && { description })). --description "" normalizes to undefined and thus can't clear a stored description — a known CLI ergonomics limitation, not a bug.

Concurrency note: updateRecord is read-modify-write (readFileSyncmapwriteFileSync, not atomic). If two hyperframes figma asset invocations upsert the same node concurrently, the later writeFileSync wins and the earlier update is silently lost. Both writers regenerate index.md from the file, so it stays JSONL-valid, but the losing row's metadata evaporates. Probably fine (interactive CLI, not a service), but worth noting since media-use's appendRecord is append-only and colision-free, and now this shared file has one writer that does full rewrites.

Pre-PR duplicate cleanup

PR body doesn't mention it. Existing repos with duplicate image_NNN files/rows from the pre-PR mint-forever bug keep the stale rows and files forever. The gate stops growing (verified above), and index.md dedupes visually only insofar as .find picks one, but the extra rows stay in the manifest and render as extra index.md rows. Suggest a one-line note in the PR body ("existing duplicates are not backfilled; new imports stop growing them") or a short one-shot dedup pass. Low severity — cosmetic + disk.

media-use cross-repo contract

Same repo (skills/media-use/), so no cross-repo semver problem. The shared contract is: both writers regenerate .media/index.md from the full manifest.jsonl after their own writes. Verified:

  • media-use writers regenerate: resolve.mjs:98,111,125,196 + adopt.mjs:95 all call regenerateIndex.
  • figma writer regenerates: runAssetImportsafeRegenerateIndex on both mint and reuse paths.
  • Both use the same row-selection rule (every JSON-parseable row). manifest.ts:8-12 docstring explicitly points at the media-use .media/ layout for interop.

New consumer of the shared entity gate — typesMatch(a, b) treats {icon, image} as one bucket in resolve.mjs:76-79,124. Sound: media-use's TYPE_DIRS already collapses image/icon/brand to images/, and figma always writes type: "image". Test at skills/media-use/scripts/resolve.test.mjs:71-102 pins the cross-type hit.

updateRecord — byte-preservation nit

Docstring at packages/core/src/figma/manifest.ts:89-90 says "preserving every other line (other writers' rows included) byte-for-byte". Implementation is split(/\r?\n/) + writeFileSync(out.join("\n")). On a CRLF-terminated manifest (Windows-native fresh write, or a repo committed with core.autocrlf=true), the \r is stripped by the split regex and the rewrite emits LF-only. JSONL stays valid, but "byte-for-byte" is stronger than the code delivers. Options: soften the docstring, or capture the terminator via a non-regex split. Nit — no functional impact.

Findings (severity-ordered)

  • 🟡 packages/core/src/figma/manifest.ts:105updateRecord silently converts CRLF to LF on Windows-terminated manifests; docstring claims byte-for-byte preservation of other writers' rows. Soften docstring or preserve terminator.
  • 🟡 PR body / release notes — pre-PR duplicate rows/files are NOT backfilled. Fix stops new mints but leaves stale entries. One-line acknowledgement in the PR description (or a one-shot dedup helper) would close the loop.
  • 🟡 packages/cli/src/commands/figma/component.ts:71-77 — each rasterize target now triggers a full regenerateIndex (read whole manifest + write index.md). For a component with N rasterize targets, that's N regens; O(N × manifest_size). Same shape as media-use adopt.mjs, so consistent. Could hoist to a single regen after the loop; nit only.
  • 🟡 packages/core/src/figma/mediaIndex.ts:32-39String(r.duration) / String(r.width) wrappers diverge cosmetically from media-use's template-literal coercion. Parity test catches functional drift; suggest a comment noting the parity test is the contract enforcer so future maintainers don't "clean up" one side and break the other.

Test coverage

Strong. Every load-bearing branch has a test:

  • applies --description/--entity on a reuse hit — pins the upsert (asset.test.ts:25-40).
  • reuses against ANY matching tuple — pins the multi-tuple gate (asset.test.ts:42-50).
  • flattens whitespace in descriptions — pins normalizeMeta (asset.test.ts:52-60).
  • matches media-use's index-gen output byte-for-byte — pins the parity contract (mediaIndex.test.ts:73-107).
  • entity hit matches across icon/image — pins the cross-type gate on the media-use side (resolve.test.mjs:71-102).

Gaps I noticed but wouldn't block on:

  • No test for the CRLF/LF preservation claim on updateRecord.
  • No test for concurrent upserts to the same row (last-writer-wins is arguably the intended behavior; would just be documenting).
  • No test for the "pre-existing duplicates keep working, don't grow" path — a fixture with two pre-PR rows for the same node would nail it.

Cross-repo consumer awareness

Same repo. media-use .media/index.md and manifest.jsonl are the shared surface, both correctly co-owned.

AI-trailer sweep

Clean — no Claude/Codex trailers in the PR files.

Peer awareness

No prior reviews on this PR at HEAD. Verified via gh pr view 1927 --json reviews[].

— Rames D Jusso

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the interop pass at 45c0a2ec. The load-bearing pieces are sound: figma asset import now regenerates the shared .media/index.md on fresh and reuse paths, threads normalized description / entity through manifest, index, and escaped <img alt>, and the reuse gate now checks all matching (format, scale ?? 1, version) tuples with file existence instead of only the oldest figma row.

The byte-parity guard is the right contract: packages/core/src/figma/mediaIndex.test.ts runs the actual skills/media-use/scripts/lib/index-gen.mjs against the same rows and asserts exact string equality. regenerateIndex intentionally reads raw JSONL values instead of figma-filtered readManifest, so media-use rows stay in the shared inventory. The registry-item.json path fix in component.ts now points to actual frozen assets, and the media-use icon/image entity bridge is scoped to entity reuse.

Nits from Miga/Rames still stand and are non-blocking: updateRecord is read-map-write / CRLF-normalizing despite the “byte-for-byte” docstring, pre-PR duplicate rows are stopped but not backfilled, component rasterize can regenerate index N times, and typesMatch can hoist its visual set.

CI is green at 45c0a2ec.

Verdict: APPROVE
Reasoning: The shared media-index and reuse semantics are correctly implemented and tested, and the remaining issues are durability/perf/documentation nits rather than merge blockers.

@vanceingalls vanceingalls merged commit 3900caa into main Jul 4, 2026
68 of 76 checks passed
@vanceingalls vanceingalls deleted the 07-03-feat_core_cli_media-use_interop_shared_index.md_regen_description_entity_on_figma_imports branch July 4, 2026 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants