fix(client): classify 401 as terminal auth failure, never retryable (RBR-1036)

cli/src/__tests__/http.test.ts

@@ -1,5 +1,5 @@
 import { afterEach, describe, expect, it, vi } from "vitest";
-import { ApiConnectionError, ApiRequestError, PaperclipApiClient } from "../client/http.js";
+import { ApiAuthError, ApiConnectionError, ApiRequestError, PaperclipApiClient } from "../client/http.js";
 
 describe("PaperclipApiClient", () => {
   afterEach(() => {
@@ -82,6 +82,88 @@ describe("PaperclipApiClient", () => {
     );
   });
 
+  it("throws ApiAuthError (not the generic ApiRequestError) on a 401", async () => {
+    const fetchMock = vi.fn().mockImplementation(
+      async () => new Response(JSON.stringify({ error: "Token expired" }), { status: 401 }),
+    );
+    vi.stubGlobal("fetch", fetchMock);
+
+    const client = new PaperclipApiClient({ apiBase: "http://localhost:3100" });
+
+    await expect(client.post("/api/issues/1/checkout", {})).rejects.toBeInstanceOf(ApiAuthError);
+    await expect(client.post("/api/issues/1/checkout", {})).rejects.toMatchObject({
+      status: 401,
+      message: "Token expired",
+    } satisfies Partial<ApiAuthError>);
+  });
+
+  it(
+    "stops after one attempt on a 401 mid-retry-loop with a distinguishable error, not the generic timeout classification (RBR-1036)",
+    async () => {
+      // Simulate the API going from "slow under load" (network-error/timeout
+      // territory) to a rejected credential (401) mid-run, then a caller-side
+      // retry/backoff wrapper (the kind a heartbeat/background retry loop
+      // would use) driving several attempts against it. Before RBR-1036, a
+      // wrapper could treat every non-2xx the same way and burn its retry
+      // budget on a token that can never succeed. After the fix, the wrapper
+      // must special-case ApiAuthError and fail fast on the first occurrence.
+      const fetchMock = vi
+        .fn()
+        .mockRejectedValueOnce(new TypeError("fetch failed")) // transient network hiccup — retryable
+        .mockResolvedValueOnce(
+          new Response(JSON.stringify({ error: "JWT expired" }), { status: 401 }),
+        ) // credential rejected — terminal, must NOT retry
+        .mockResolvedValueOnce(new Response(JSON.stringify({ ok: true }), { status: 200 })); // would "succeed" if wrongly retried
+      vi.stubGlobal("fetch", fetchMock);
+
+      const client = new PaperclipApiClient({ apiBase: "http://localhost:3100" });
+
+      const maxAttempts = 6;
+      let attempts = 0;
+      let caughtError: unknown;
+      for (; attempts < maxAttempts; attempts++) {
+        try {
+          await client.post("/api/issues/1/comments", { body: "hi" });
+          break;
+        } catch (error) {
+          caughtError = error;
+          // The bounded retry/backoff wrapper's own special-case: a 401 is
+          // terminal, so it must fail fast instead of looping like it would
+          // for ApiConnectionError/5xx.
+          if (error instanceof ApiAuthError) break;
+        }
+      }
+
+      expect(caughtError).toBeInstanceOf(ApiAuthError);
+      expect((caughtError as ApiAuthError).status).toBe(401);
+      // One retryable network failure, then the terminal 401 — the wrapper
+      // must stop there rather than continuing to the would-be-successful
+      // third call.
+      expect(fetchMock).toHaveBeenCalledTimes(2);
+      expect(attempts).toBeLessThan(maxAttempts);
+    },
+  );
+
+  it("recoverAuth still gets exactly one bounded recovery attempt on a 401", async () => {
+    const fetchMock = vi
+      .fn()
+      .mockResolvedValueOnce(new Response(JSON.stringify({ error: "Token expired" }), { status: 401 }))
+      .mockResolvedValueOnce(new Response(JSON.stringify({ ok: true }), { status: 200 }));
+    vi.stubGlobal("fetch", fetchMock);
+
+    const recoverAuth = vi.fn().mockResolvedValue("fresh-token-456");
+    const client = new PaperclipApiClient({
+      apiBase: "http://localhost:3100",
+      recoverAuth,
+    });
+
+    const result = await client.post<{ ok: boolean }>("/api/test", { hello: "world" });
+
+    expect(result).toEqual({ ok: true });
+    expect(recoverAuth).toHaveBeenCalledOnce();
+    expect(fetchMock).toHaveBeenCalledTimes(2);
+  });
+
   it("retries once after interactive auth recovery", async () => {
     const fetchMock = vi
       .fn()

cli/src/client/http.ts

@@ -13,6 +13,31 @@ export class ApiRequestError extends Error {
   }
 }
 
+/**
+ * Thrown for a `401` response from the Paperclip API. This is a distinct,
+ * unmistakable error class from `ApiRequestError`/`ApiConnectionError` so a
+ * rejected/expired credential can never be mistaken for — or folded into —
+ * the timeout/5xx/network-error retry path (RBR-1036). A 401 is a terminal
+ * auth failure: it means the credential is dead, not that the server is
+ * slow or unreachable, and it must never be retried on the assumption that
+ * trying again will help.
+ *
+ * `request()` always throws this immediately on the first `401` and does not
+ * loop or retry it (the one exception being the single, explicit interactive
+ * `recoverAuth` exchange used by the human-facing CLI board-login flow, which
+ * swaps in a fresh credential before making exactly one bounded retry — see
+ * `recoverAuth` on `PaperclipApiClient`). Callers that want to special-case
+ * auth failures (e.g. to avoid burning a run's retry budget against a token
+ * that can never succeed) should check `instanceof ApiAuthError` rather than
+ * inspecting `status === 401` on the generic `ApiRequestError`.
+ */
+export class ApiAuthError extends ApiRequestError {
+  constructor(message: string, details?: unknown, body?: unknown) {
+    super(401, message, details, body);
+    this.name = "ApiAuthError";
+  }
+}
+
 export class ApiConnectionError extends Error {
   url: string;
   method: string;
@@ -198,9 +223,15 @@ async function toApiError(response: Response): Promise<ApiRequestError> {
       (typeof body.message === "string" && body.message.trim()) ||
       `Request failed with status ${response.status}`;
 
+    if (response.status === 401) {
+      return new ApiAuthError(message, body.details, parsed);
+    }
     return new ApiRequestError(response.status, message, body.details, parsed);
   }
 
+  if (response.status === 401) {
+    return new ApiAuthError(`Request failed with status ${response.status}`, undefined, parsed);
+  }
   return new ApiRequestError(response.status, `Request failed with status ${response.status}`, undefined, parsed);
 }
 

packages/mcp-server/src/client.test.ts

@@ -0,0 +1,96 @@
+import { afterEach, describe, expect, it, vi } from "vitest";
+import { PaperclipApiAuthError, PaperclipApiClient, PaperclipApiError } from "./client.js";
+
+function makeClient() {
+  return new PaperclipApiClient({
+    apiUrl: "http://localhost:3100/api",
+    apiKey: "token-123",
+    companyId: "11111111-1111-1111-1111-111111111111",
+    agentId: "22222222-2222-2222-2222-222222222222",
+    runId: "33333333-3333-3333-3333-333333333333",
+  });
+}
+
+function jsonResponse(body: unknown, status: number) {
+  return new Response(JSON.stringify(body), {
+    status,
+    headers: { "Content-Type": "application/json" },
+  });
+}
+
+describe("PaperclipApiClient", () => {
+  afterEach(() => {
+    vi.unstubAllGlobals();
+    vi.restoreAllMocks();
+  });
+
+  it("throws the generic PaperclipApiError for a non-401 failure", async () => {
+    const fetchMock = vi.fn().mockResolvedValue(jsonResponse({ error: "Not found" }, 404));
+    vi.stubGlobal("fetch", fetchMock);
+
+    const client = makeClient();
+    await expect(client.requestJson("GET", "/issues/missing")).rejects.toBeInstanceOf(PaperclipApiError);
+    await expect(client.requestJson("GET", "/issues/missing")).rejects.not.toBeInstanceOf(PaperclipApiAuthError);
+  });
+
+  it("classifies a 401 as PaperclipApiAuthError, a distinct class from the generic PaperclipApiError", async () => {
+    const fetchMock = vi.fn().mockImplementation(async () => jsonResponse({ error: "JWT expired" }, 401));
+    vi.stubGlobal("fetch", fetchMock);
+
+    const client = makeClient();
+    await expect(client.requestJson("GET", "/agents/me")).rejects.toBeInstanceOf(PaperclipApiAuthError);
+    await expect(client.requestJson("GET", "/agents/me")).rejects.toMatchObject({
+      status: 401,
+      message: expect.stringContaining("JWT expired"),
+    });
+  });
+
+  it(
+    "a bounded retry/backoff wrapper around the client must stop after one attempt on a 401, not fold it into the timeout/5xx retry path (RBR-1036)",
+    async () => {
+      // First call: transient 503 (legitimately retryable). Second call: the
+      // credential has since expired and the API now rejects with 401 — this
+      // must be treated as terminal, not retried like the 503 was. Third call
+      // (never reached if the fix holds) would "succeed", proving a naive
+      // retry loop would have masked the real failure.
+      const fetchMock = vi
+        .fn()
+        .mockResolvedValueOnce(jsonResponse({ error: "Service unavailable" }, 503))
+        .mockResolvedValueOnce(jsonResponse({ error: "JWT expired" }, 401))
+        .mockResolvedValueOnce(jsonResponse({ ok: true }, 200));
+      vi.stubGlobal("fetch", fetchMock);
+
+      const client = makeClient();
+
+      async function withBoundedRetry<T>(fn: () => Promise<T>, maxAttempts: number): Promise<T> {
+        let lastError: unknown;
+        for (let attempt = 1; attempt <= maxAttempts; attempt++) {
+          try {
+            return await fn();
+          } catch (error) {
+            lastError = error;
+            // The retry wrapper's whole point under test: a 401 is terminal
+            // and must fail fast on the first occurrence rather than looping
+            // through the remaining attempt budget.
+            if (error instanceof PaperclipApiAuthError) throw error;
+          }
+        }
+        throw lastError;
+      }
+
+      const maxAttempts = 6;
+      let caughtError: unknown;
+      try {
+        await withBoundedRetry(() => client.requestJson("POST", "/issues/PAP-1/comments", { body: {} }), maxAttempts);
+      } catch (error) {
+        caughtError = error;
+      }
+
+      expect(caughtError).toBeInstanceOf(PaperclipApiAuthError);
+      expect((caughtError as PaperclipApiAuthError).status).toBe(401);
+      // One retried transient 503, then the terminal 401 — never reaches the
+      // third, would-be-successful call.
+      expect(fetchMock).toHaveBeenCalledTimes(2);
+    },
+  );
+});

packages/mcp-server/src/client.ts

@@ -22,6 +22,22 @@ export class PaperclipApiError extends Error {
   }
 }
 
+/**
+ * Thrown for a `401` response from the Paperclip API. Distinct from the
+ * generic `PaperclipApiError` so a rejected/expired agent credential is
+ * classified as a terminal auth failure immediately, never mistaken for a
+ * timeout/5xx/network hiccup that is safe to retry (RBR-1036). MCP tool
+ * callers should surface this error class as-is rather than retrying —
+ * retrying a dead credential only burns wall clock a live run needs
+ * elsewhere.
+ */
+export class PaperclipApiAuthError extends PaperclipApiError {
+  constructor(input: { method: string; path: string; body: unknown; message: string }) {
+    super({ ...input, status: 401 });
+    this.name = "PaperclipApiAuthError";
+  }
+}
+
 export interface JsonRequestOptions {
   body?: unknown;
   includeRunId?: boolean;
@@ -100,6 +116,14 @@ export class PaperclipApiClient {
     const parsedBody = await parseResponseBody(response);
 
     if (!response.ok) {
+      if (response.status === 401) {
+        throw new PaperclipApiAuthError({
+          method: method.toUpperCase(),
+          path,
+          body: parsedBody,
+          message: buildErrorMessage(method.toUpperCase(), path, response.status, parsedBody),
+        });
+      }
       throw new PaperclipApiError({
         status: response.status,
         method: method.toUpperCase(),

packages/mcp-server/src/format.ts

@@ -1,4 +1,4 @@
-import { PaperclipApiError } from "./client.js";
+import { PaperclipApiAuthError, PaperclipApiError } from "./client.js";
 
 type McpTextResponse = {
   content: Array<{ type: "text"; text: string }>;
@@ -16,6 +16,16 @@ export function formatTextResponse(value: unknown): McpTextResponse {
 }
 
 export function formatErrorResponse(error: unknown): McpTextResponse {
+  if (error instanceof PaperclipApiAuthError) {
+    return formatTextResponse({
+      error: error.message,
+      errorClass: "auth_failed",
+      status: error.status,
+      method: error.method,
+      path: error.path,
+      body: error.body,
+    });
+  }
   if (error instanceof PaperclipApiError) {
     return formatTextResponse({
       error: error.message,

skills/paperclip/SKILL.md

@@ -67,6 +67,8 @@ Headers: Authorization: Bearer $PAPERCLIP_API_KEY, X-Paperclip-Run-Id: $PAPERCLI
 
 If already checked out by you, returns normally. If owned by another agent: `409 Conflict` — stop, pick a different task. **Never retry a 409.**
 
+**Never retry a 401.** A `401` from any Paperclip API call means your credential (JWT/API key) was rejected or has expired — not a slow/unreachable server. The bundled `PaperclipApiClient` (CLI `client/http.ts` and the MCP server client) classifies a `401` as a distinct auth-failure error (`ApiAuthError` / `PaperclipApiAuthError`) immediately and does not fold it into timeout/5xx/network-error retry logic. If you write your own retry/backoff wrapper around Paperclip API calls, special-case that class and fail fast on the first `401` rather than looping — retrying a dead credential only burns the heartbeat's wall clock on a call that can never succeed.
+
 **Step 6 — Understand context.** Prefer `GET /api/issues/{issueId}/heartbeat-context` first. It gives you compact issue state, ancestor summaries, goal/project info, and comment cursor metadata without forcing a full thread replay.
 
 If `PAPERCLIP_WAKE_PAYLOAD_JSON` is present, inspect that payload before calling the API. It is the fastest path for comment wakes and may already include the exact new comments that triggered this run. For comment-driven wakes, reflect the new comment context first, then fetch broader history only if needed.