diff --git a/packages/cli/src/commands/figma/asset.test.ts b/packages/cli/src/commands/figma/asset.test.ts index 13947b2b0..6e0e1d44d 100644 --- a/packages/cli/src/commands/figma/asset.test.ts +++ b/packages/cli/src/commands/figma/asset.test.ts @@ -3,7 +3,12 @@ import { describe, expect, it, afterEach } from "vitest"; import { mkdtempSync, readFileSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { runAssetImport, runAssetImportMany, type AssetImportDeps } from "./asset.js"; +import { + gatherAssetRefs, + runAssetImport, + runAssetImportMany, + type AssetImportDeps, +} from "./asset.js"; import type { FigmaClient } from "@hyperframes/core/figma"; const dirs: string[] = []; @@ -174,6 +179,18 @@ describe("runAssetImport", () => { expect(new Set(results.map((r) => r.record.id)).size).toBe(3); }); + it("gatherAssetRefs splits bare comma-joined ids but keeps URLs whole", () => { + // bare tokens comma-split + expect(gatherAssetRefs(["KEY:1-2,KEY:3-4"])).toEqual(["KEY:1-2", "KEY:3-4"]); + // space-separated positionals preserved + expect(gatherAssetRefs(["KEY:1-2", "KEY:3-4"])).toEqual(["KEY:1-2", "KEY:3-4"]); + // a URL with a comma in its query is NOT torn apart + const url = "https://www.figma.com/design/KEY/F?node-id=1:2,3:4"; + expect(gatherAssetRefs([url])).toEqual([url]); + // mixed: URL stays whole, bare token splits + expect(gatherAssetRefs([url, "KEY:5-6,KEY:7-8"])).toEqual([url, "KEY:5-6", "KEY:7-8"]); + }); + it("splits comma-joined refs and rejects a cross-file batch", async () => { const dir = scratch(); await expect( diff --git a/packages/cli/src/commands/figma/asset.ts b/packages/cli/src/commands/figma/asset.ts index 9da7bc33b..8e3465bea 100644 --- a/packages/cli/src/commands/figma/asset.ts +++ b/packages/cli/src/commands/figma/asset.ts @@ -50,6 +50,20 @@ export interface AssetImportResult { reused: boolean; } +/** + * Flatten CLI positionals into asset refs. Comma-splits bare + * `fileKey:nodeId` tokens (so `asset A,B` batches) but leaves URL tokens + * whole — a figma URL can carry commas in its query (multi-select + * `node-id=1:2,3:4`), and splitting those would tear the URL apart. To batch + * URLs, pass them as separate positional args. + */ +export function gatherAssetRefs(positionals: string[]): string[] { + return positionals + .flatMap((r) => (/^https?:/i.test(r.trim()) ? [r] : r.split(","))) + .map((r) => r.trim()) + .filter((r) => r.length > 0); +} + function requireNodeRef(refInput: string): { fileKey: string; nodeId: string } { const ref = parseFigmaRef(refInput); if (!ref.nodeId) @@ -271,12 +285,10 @@ export default defineCommand({ // named `ref`), so use `_` as the source of truth — reading both would // double-count the first. Split any comma-joined ids, so `asset A B`, // `asset A,B`, and `asset URL1 URL2` all batch into ONE /v1/images call. - const positionals = - Array.isArray(args._) && args._.length > 0 ? (args._ as string[]) : [args.ref]; - const refs = positionals - .flatMap((r) => String(r).split(",")) - .map((r) => r.trim()) - .filter((r) => r.length > 0); + const positionals = ( + Array.isArray(args._) && args._.length > 0 ? (args._ as string[]) : [args.ref] + ).map(String); + const refs = gatherAssetRefs(positionals); const results = await runAssetImportMany( refs, { diff --git a/packages/core/src/figma/client.test.ts b/packages/core/src/figma/client.test.ts index 6bf5c7dd1..3af01728a 100644 --- a/packages/core/src/figma/client.test.ts +++ b/packages/core/src/figma/client.test.ts @@ -153,6 +153,47 @@ describe("error mapping", () => { expect(waits).toEqual([1000, 2000]); // exponential backoff }); + it("caps an oversized Retry-After at 60s so the CLI can't block for an hour", async () => { + let n = 0; + const waits: number[] = []; + const client = createFigmaClient({ + token: "t", + fetch: (() => { + n += 1; + return Promise.resolve( + n === 1 + ? new Response("{}", { status: 429, headers: { "retry-after": "3600" } }) + : jsonResponse(200, { meta: { styles: [] } }), + ); + }) as FigmaFetch, + sleep: (ms) => { + waits.push(ms); + return Promise.resolve(); + }, + }); + await client.styles("F"); + expect(waits).toEqual([60_000]); // 3600s clamped, not 3_600_000 + }); + + it("retries 429 on non-styles endpoints too (retry lives in the shared get)", async () => { + let n = 0; + const client = createFigmaClient({ + token: "t", + fetch: (() => { + n += 1; + return Promise.resolve( + n < 2 + ? jsonResponse(429, {}) + : jsonResponse(200, { images: { "1:2": "https://cdn/a.png" } }), + ); + }) as FigmaFetch, + sleep: () => Promise.resolve(), + }); + const out = await client.renderNodes("F", ["1:2"], { format: "png" }); + expect(out[0]?.url).toBe("https://cdn/a.png"); + expect(n).toBe(2); // one 429 then success + }); + it("honors Retry-After (seconds) over the backoff default", async () => { let n = 0; const waits: number[] = []; diff --git a/packages/core/src/figma/client.ts b/packages/core/src/figma/client.ts index 35731ea63..29efadd6d 100644 --- a/packages/core/src/figma/client.ts +++ b/packages/core/src/figma/client.ts @@ -122,15 +122,23 @@ const SCOPE_HINTS = { libraryContent: "Library content: Read-only (library_content:read)", } as const; +/** Longest we'll auto-wait on a single Retry-After before giving up — past a + * minute the user is better off cancelling and reducing batch size (the + * RATE_LIMITED message says so) than watching the CLI block silently. */ +const MAX_RETRY_WAIT_MS = 60_000; + /** Parse a Retry-After header (figma sends integer seconds; the HTTP spec - * also allows a date) into ms, or null when absent/unparseable. */ + * also allows a date) into ms, capped at MAX_RETRY_WAIT_MS, or null when + * absent/unparseable. The cap keeps a spec-legal `Retry-After: 3600` (tier + * quota exhaustion) from silently blocking the CLI for an hour. */ function retryAfterMs(res: Response): number | null { const raw = res.headers.get("retry-after"); if (raw === null) return null; const secs = Number(raw); - if (Number.isFinite(secs)) return Math.max(0, secs * 1000); + if (Number.isFinite(secs)) return Math.min(MAX_RETRY_WAIT_MS, Math.max(0, secs * 1000)); const date = Date.parse(raw); - return Number.isNaN(date) ? null : Math.max(0, date - Date.now()); + if (Number.isNaN(date)) return null; + return Math.min(MAX_RETRY_WAIT_MS, Math.max(0, date - Date.now())); } /** Figma's error bodies are precise — "Invalid token", or "Invalid scope(s): @@ -217,8 +225,11 @@ export function createFigmaClient(options: FigmaClientOptions): FigmaClient { * surfaced verbatim. Falls back to the endpoint's scopeHint when the body * is silent. */ function forbiddenError(body: string | null, opts: GetOptions): FigmaClientError { + // Every branch RETURNS the error (the caller throws once) — no mixed + // throw/return, so a future caller that wraps the result gets consistent + // behavior across all three cases. if (body && /invalid token/i.test(body)) - throw new FigmaClientError( + return new FigmaClientError( "BAD_TOKEN", "figma rejected the token (403 Invalid token) — it is invalid, expired, or revoked. Re-mint at figma.com/settings → Security, then update FIGMA_TOKEN.", 403, diff --git a/skills-manifest.json b/skills-manifest.json index 427739bcd..b843af82f 100644 --- a/skills-manifest.json +++ b/skills-manifest.json @@ -10,7 +10,7 @@ "files": 18 }, "figma": { - "hash": "5581f824cc5deecc", + "hash": "99538ee56a4ca553", "files": 2 }, "general-video": { diff --git a/skills/figma/SKILL.md b/skills/figma/SKILL.md index e5c7e1462..b08f77174 100644 --- a/skills/figma/SKILL.md +++ b/skills/figma/SKILL.md @@ -31,7 +31,7 @@ While onboarding, also set expectations in one breath: every import lands as a * - **Phases 4–5 (motion/shaders):** the Figma MCP connector (one-click OAuth), a separate credential from the token. If MCP tools error unauthenticated, tell the user to connect the Figma connector and stop. - Say exactly which credential a failing phase needs — never present the split as broken. -- `BAD_TOKEN` (401) mid-flow → the token is expired/revoked; re-mint. `FORBIDDEN` (403) → the message names the exact missing scope (e.g. `library_content:read` for the styles fallback) — add it, or the file isn't visible to the account. `REQUIRES_ENTERPRISE` (403 on variables) → not a failure: styles fallback already ran. `RATE_LIMITED` (429) → the client already retried with backoff; if it still surfaces, wait a minute or import fewer nodes per call. +- `BAD_TOKEN` (401) mid-flow → the token is expired/revoked; re-mint. `FORBIDDEN` (403) → the message names the exact missing scope (e.g. `library_content:read` for the styles fallback) — add it, or the file isn't visible to the account. `REQUIRES_ENTERPRISE` (403 on variables) → not a failure: styles fallback already ran. `RATE_LIMITED` (429) → the client already retried with backoff (this applies to EVERY read — assets, tokens, styles, node trees, versions — the retry lives in the shared request path; `Retry-After` is honored, capped at 60s); if it still surfaces, wait a minute or import fewer nodes per call. **Rate-limit awareness (spec §2.1):** MCP on a Starter plan is 6 tool calls/**month** (figma plan matrix as of 2026-07 — re-verify if quotas look off) — batch with `recursive:true` on the parent node, skip verification screenshots unless asked, and cache raw MCP responses so re-derivation never spends a second call. REST is per-minute (10+/min, per-endpoint buckets) — fine at volume, back off on 429.