fix(core): name missing figma scope in 403, retry 429 with backoff

[vanceingalls]

Two bugs surfaced running the figma integration live.

Bug 1 — tokens styles fallback 403s on non-Enterprise with a useless message

The docs promised the published-styles fallback works with just File content + File metadata scopes. It doesn't: /v1/files/:key/styles also needs library_content:read. Worse, the generic 403 said "the token is missing a read scope" without naming which — a dead end for the user.

Fix: every endpoint now passes a scope hint, and the FORBIDDEN message names the exact scope. The styles 403 now reads "This endpoint needs the "Library content: Read-only (library_content:read)" scope — add it at figma.com/settings…". The NO_TOKEN setup text and the /figma skill's scope list both gained the Library content line.

Bug 2 — asset (and per-node component renders) never retried 429

The 429 message said "back off and retry" but the client didn't — two imports in a row tripped Figma's per-minute limit and hard-failed. Since a component import renders each rasterized node as a separate /v1/images call, this hit the many-node path hardest.

Fix: get() retries 429 with exponential backoff (1s → 2s → 4s), honoring Retry-After when Figma sends it, before surfacing RATE_LIMITED after maxRetries (default 3). sleep is injectable so tests don't actually wait.

Tests

client suite 17/17 (up from 13): retry-then-succeed, Retry-After honored over the default, RATE_LIMITED only after exhausting retries (asserts attempt count), and the library_content:read scope named in the styles 403.

Not in this PR

Batch multi-node asset syntax (the documented /v1/images comma-separated-ids rate workaround) is a real enhancement but a separate change — it needs command-UX + per-node cache/manifest wiring. The retry fix makes the reported failure self-heal without it.

🤖 Generated with Claude Code

[vanceingalls]

Extended this PR with the rest of the valid bug-bash cluster (the second report interpreted its own findings against the raw MCP tools without the /figma skill — declared invalid, discarded; everything below traces to the CLI-driven report + the inline scope/retry/batch asks):

403-body parse (bug 4 + closes 1 & 9's messaging). Figma returns 403 {"err":"Invalid token"} for bad PATs — not 401 — so the old BAD_TOKEN branch was dead code and typo'd tokens got sent to the scopes screen. get() now reads the 403 body: "Invalid token" → BAD_TOKEN + re-mint advice; Invalid scope(s)… requires X → surfaces figma's own diagnosis verbatim; otherwise the endpoint scope hint. Reads both err and message (variables endpoint uses message). Verified live: figma tokens with a bogus token now prints "403 Invalid token — Re-mint…" instead of scope advice.

Batch asset fetch (requested). figma asset now takes multiple refs — space-separated or comma-joined — of one file and renders them in a SINGLE /v1/images call via a new client.renderNodes, figma's documented rate-limit workaround. runAssetImport delegates to runAssetImportMany; cache-checks per node, batches only the misses, regenerates index.md once. Cross-file batch errors clearly. Verified: 3 refs → 3 frozen assets → 1 request; re-run reuses.

NO_TOKEN box (bug 8). errorBox indented only the first hint line, so the numbered setup list rendered ragged. Now indents every line; single-line hints unchanged (no impact on ~80 other callers).

Deliberately not here: the mapper visual-fidelity cluster — IMAGE fills dropped to empty divs (bug 2, P0), rasterized-vector double-paint (bug 3), font affordance (bug 6) — is a separate subsystem (nodeToHtml.ts); I'll open a focused PR for those. Bug 7 (unknown-flag stack trace) is the shared CLI dispatcher, also separate. Bug 5 (registry paths) rides with the mapper PR.

Suites: core figma 114, cli figma 33, all green.

[miga-heygen]

Review — fix(core): name missing figma scope in 403, retry 429 with backoff

Two real bugs fixed, plus a batch optimization that's the natural rate-limit workaround. Solid work.

Bug 1 — 403 scope naming

Clean design. SCOPE_HINTS gives each endpoint a named scope, readFigmaErrorMessage reads figma's own body (err and message fields), and forbiddenError classifies the 403 into BAD_TOKEN / REQUIRES_ENTERPRISE / FORBIDDEN-with-scope. The styles endpoint's library_content:read omission was a real trap — the docs said it wasn't needed, it was. Good that the NO_TOKEN setup text and the /figma skill's scope list both gained the line.

The forbiddenError reclassification of figma's "Invalid token" 403 as BAD_TOKEN is a nice catch — figma returns 403 (not 401) for bad PATs on file endpoints, so without this the user gets a confusing "missing scope" message when the real problem is a dead token.

Nit: forbiddenError mixes throw (for the "Invalid token" path) and return (for everything else). Both work — the caller already wraps the return in throw — but the inconsistency is confusing to read. Either all-throw or all-return would be cleaner.

Bug 2 — 429 retry

get() now retries 429 with exponential backoff (1s → 2s → 4s), honors Retry-After when present, and sleep is injectable for testing. retryAfterMs handles both integer-seconds and HTTP-date forms. Clean.

Test coverage is thorough: retry-then-succeed, Retry-After honored over the default, attempt count verified (1 initial + 3 retries = 4), RATE_LIMITED only after exhausting retries.

Batch renderNodes

Good refactoring. renderNode delegates to renderNodes via this (correct for test fakes that override renderNode). runAssetImport delegates to runAssetImportMany. The extracted helpers all carry new facts:

• requireNodeRef — validation + error message
• reuseExisting — cache-hit logic with metadata upsert (needed by both single and batch)
• freezeAndRecord — download + sanitize + freeze + record, deliberately omits index regeneration so the caller does it once

The batch path in runAssetImportMany is clean: resolve cache hits first, batch-render only the misses in one /v1/images call, fill in slots, regen index once. Per-node failures come back as url:null rather than aborting the batch — matches the renderNodes contract.

Cross-file batch rejection (all refs must share a fileKey) is correctly enforced with a clear error message.

SSOT check

• No duplicated decisions. Cache-hit logic has one owner (reuseExisting), 403 classification has one owner (forbiddenError), retry has one owner (get()).
• No stale concepts. The old inline status checks in get() are fully replaced by throwForStatus. The old inline /v1/images call in renderNode is replaced by delegation to renderNodes.
• safeRegenerateIndex is called exactly once per import path (end of runAssetImportMany), not N times per node. Good.

Tests

17/17 (up from 13). New tests cover retry-then-succeed, Retry-After honored, styles 403 scope naming, figma's own err field surfaced verbatim, "Invalid token" reclassified as BAD_TOKEN, REQUIRES_ENTERPRISE preserved, batch renderNodes (one call for many nodes, url:null for failed nodes), runAssetImportMany (batch import, cross-file rejection). All the new behavior is covered.

Verdict

LGTM. The forbiddenError throw/return inconsistency is a nit — take it or leave it.

— Review by Miga

[james-russo-rames-d-jusso]

Reviewed at 4fc699f.

Solid bug-bash bundle — 403-body parsing, batch /v1/images, retry-with-backoff, and the multi-line NO_TOKEN indent are all well-motivated, and the test suite meaningfully covers the interesting shapes (Retry-After honored, "Invalid token" → BAD_TOKEN reclassification, batch → one call).

Blockers

• CI Typecheck is red. packages/cli/src/commands/figma/tokens.test.ts:16-36 builds a FigmaClient-typed fake with renderNode / imageFills / variables / styles / nodeTree / fileVersion — but the new renderNodes method wasn't added, so TS reports the literal isn't assignable to FigmaClient. The asset.test.ts and component.test.ts fakes both got the delegation shim; tokens.test.ts didn't. Same one-liner fix — either add renderNodes: () => Promise.reject(new Error("unused")) (tokens tests never render nodes), or the this.renderNode-delegating stub used in component.test.ts for symmetry. bun run --filter '@hyperframes/cli' typecheck locally to confirm before pushing.

Concerns

• Retry-loop scope widening (client.ts:276): the retry is at the get() level, so all six read endpoints inherit it. Likely intentional, but blast-radius isn't obvious from PR title/body and only the styles path has an explicit retry test.
• forbiddenError throw/return asymmetry (client.ts:219): BAD_TOKEN branch throws; FORBIDDEN / REQUIRES_ENTERPRISE branches return and rely on the caller's throw. Works today, but fragile if wrapping ever gets added at the callsite.

Nits

• retryAfterMs uncapped (client.ts:127): a spec-legal huge Retry-After silently blocks the CLI.
• CLI positional comma-splitter is URL-unsafe (asset.ts:277): a figma URL containing a literal , gets torn.

Questions

• Was component.ts intentionally left alone (relying on renderNode → renderNodes delegation for the retry inheritance), or is there a follow-up to make it use renderNodes directly for batch rate-limit gains on many-node component imports? The PR body implies retry-only, which is what the delegation gives you — just checking.
• Any reason runAssetImportMany rejects cross-file batches with a throw new Error(...) instead of a typed FigmaClientError? Not a bug, but the surrounding module uses typed errors elsewhere.

What I didn't verify

• Live figma behavior of Retry-After (only inferred from HTTP spec / figma docs).
• Whether any other test file has a FigmaClient fake that also needs the shim — I only inspected the failure the CI report named, but if tokens.test.ts was missed, worth rg 'FigmaClient' packages before pushing to catch any sibling.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

🟠 forbiddenError has an asymmetric throw/return contract that will bite a future reader. The BAD_TOKEN branch throws inside the function; the REQUIRES_ENTERPRISE and FORBIDDEN branches return and rely on the caller's throw forbiddenError(...) at line 257 to actually throw. It works today because the caller unconditionally re-throws — but the function's return type says FigmaClientError, so a reader can't tell the two branches diverge without stepping through both call sites. If someone later adds a caller that does const e = forbiddenError(...) (e.g. wrapping / decorating the error), the BAD_TOKEN branch silently bypasses their wrapping and the FORBIDDEN branch doesn't. Pick one shape — either throw in every branch (return type becomes never) or return in every branch (caller does the throw once).

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

🟠 The retry loop lives inside get(), so this PR silently changes the 429 behavior of every read endpoint — imageFills, variables, styles, nodeTree, fileVersion — not just renderNodes. That's likely the right call (per-minute limits are per-endpoint but the client's callers all benefit), but the PR title / body scopes the fix to asset and per-node component renders, and the test suite only asserts retry on the styles path. Two suggestions: (a) mention in the PR body / SKILL.md that all reads retry now — the RATE_LIMITED line already says the client retries with backoff, but the widening isn't obvious to someone reading the diff to understand blast radius; (b) if any endpoint is intentionally meant to fail fast on 429 (e.g. variables because callers already have a styles fallback and a 3-retry wait delays the fallback by up to 7s), it needs an opt-out. Otherwise the current shape looks correct.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

🟡 retryAfterMs has no upper bound. If figma ever sends Retry-After: 3600 (spec-legal — it's the value they emit for tier-quota exhaustion on some paid-endpoint stacks), the CLI silently blocks for an hour before returning RATE_LIMITED, with no output. Same for a Retry-After date parsed to "1 hour from now." Consider capping at ~60s and using min(retryAfterMs, cap): past a minute the user's better off Ctrl-C'ing and either waiting explicitly or reducing batch size (which the message already suggests). Non-blocking, but a common footgun with header-driven waits.

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

🟡 The comma-splitter is greedy: .flatMap((r) => String(r).split(",")) treats every literal , in argv as a delimiter, even inside a figma URL query. A user pasting hyperframes figma asset 'https://…/file/KEY?node-id=1%3A2,3%3A4' (figma sometimes uses comma-separated node-id= in shared multi-select URLs — verify against a real link, but it's a plausible copy-paste) would get the URL torn in half at the comma → parse errors on both halves. Two options: (a) only split when the token doesn't look like a URL (!/^https?:/i.test(r)); (b) require the user to be explicit and pass separate positional args when using URLs, then only split unquoted fileKey:nodeId positionals. Also worth a test where one positional is a URL — the current batch test uses raw KEY:1-2 refs, which don't exercise this path.

— Review by Rames D Jusso