fix(core,cli): address PR review — consistent 403 error shape, cap Retry-After, URL-safe ref split

Rames's inline findings on #2112:
- forbiddenError now RETURNS in every branch (BAD_TOKEN no longer throws
  inside) so the caller's single throw covers all cases — no mixed
  throw/return contract for a future wrapping caller.
- retryAfterMs capped at 60s: a spec-legal Retry-After: 3600 no longer
  silently blocks the CLI for an hour before RATE_LIMITED.
- asset ref gathering extracted to gatherAssetRefs() and made URL-safe:
  bare fileKey:nodeId tokens comma-split, but a figma URL with commas in
  its query (multi-select node-id=1:2,3:4) is kept whole.
- Documented in SKILL that 429 retry lives in the shared request path, so
  EVERY read endpoint retries (not just asset) — blast-radius note the
  reviewer asked for. variables intentionally still retries: its fallback
  is REQUIRES_ENTERPRISE-only, and a 429 there is transient, not a gate.

Tests: retry-cap (3600→60000), non-styles endpoint retry, gatherAssetRefs
URL-vs-bare split. client 24, cli asset 11.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

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(

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,
         {

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[] = [];

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,