mcpdo: keep the sign-in helper's failure classification without a TTY (#2962)

clients/mcpdo/__tests__/auth-helper.test.ts

@@ -4,6 +4,11 @@ import * as os from "node:os";
 import * as path from "node:path";
 import { PassThrough } from "node:stream";
 import type { CallbackNavigation } from "@inspector/core/auth/index.js";
+import { FetchTargetError } from "@inspector/core/auth/fetchRefusal.js";
+import {
+  classifyError,
+  EXIT_CODES,
+} from "@inspector/core/cli/error-handler.js";
 
 const authorizeInFrontend = vi.fn();
 
@@ -21,9 +26,11 @@ vi.mock("../src/connection/authorize.js", () => ({
 
 import {
   AUTH_HELPER_COMMAND,
+  helperErrorEvent,
   obtainPendingAuthUrl,
   pendingAuthMarkerPath,
   readLivePendingAuthMarker,
+  relayedHelperError,
   removeOwnPendingAuthMarker,
   runAuthHelper,
   type PendingAuthMarker,
@@ -195,6 +202,40 @@ describe("auth-helper", () => {
       });
     });
 
+    it("keeps a helper's relayed classification instead of auth_required (#2962)", async () => {
+      const script = writeHelperScript(`
+        process.stdin.resume();
+        process.stdin.on("end", () => {
+          process.stdout.write(
+            JSON.stringify({
+              event: "error",
+              message: "Refusing to fetch from a link-local or metadata address (169.254.169.254)",
+              exitCode: 12,
+              envelope: {
+                code: "fetch_refused",
+                url: "http://169.254.169.254/.well-known/oauth-authorization-server",
+              },
+            }) + "\\n",
+          );
+        });
+      `);
+      await expect(
+        obtainPendingAuthUrl(
+          { type: "streamable-http", url: SERVER_URL },
+          undefined,
+          { helperArgv1: script },
+        ),
+      ).rejects.toMatchObject({
+        exitCode: EXIT_CODES.FETCH_REFUSED,
+        message:
+          "Sign-in helper failed: Refusing to fetch from a link-local or metadata address (169.254.169.254)",
+        envelope: {
+          code: "fetch_refused",
+          url: "http://169.254.169.254/.well-known/oauth-authorization-server",
+        },
+      });
+    });
+
     it("relays fetch_refused events as sanitized warnings before the URL (#2905)", async () => {
       const script = writeHelperScript(`
         process.stdin.resume();
@@ -687,6 +728,42 @@ describe("auth-helper", () => {
       expect(fs.readdirSync(dir).filter((f) => f.includes("auth"))).toEqual([]);
     });
 
+    it("relays the failure's classification on the error event (#2962)", async () => {
+      authorizeInFrontend.mockRejectedValueOnce(
+        new TypeError("fetch failed", {
+          cause: new FetchTargetError(
+            "Refusing to fetch from a link-local or metadata address (169.254.169.254)",
+            "http://169.254.169.254/.well-known/oauth-authorization-server",
+          ),
+        }),
+      );
+      const restoreStdin = stubStdin(
+        JSON.stringify({
+          serverConfig: { type: "streamable-http", url: SERVER_URL },
+        }),
+      );
+      const stdout = captureStdout();
+      try {
+        await expect(runAuthHelper()).rejects.toThrow("fetch failed");
+      } finally {
+        stdout.restore();
+        restoreStdin();
+      }
+      expect(stdout.lines().map((l) => JSON.parse(l) as unknown)).toEqual([
+        {
+          event: "error",
+          message: "fetch failed",
+          exitCode: EXIT_CODES.FETCH_REFUSED,
+          envelope: {
+            code: "fetch_refused",
+            cause:
+              "Refusing to fetch from a link-local or metadata address (169.254.169.254)",
+            url: "http://169.254.169.254/.well-known/oauth-authorization-server",
+          },
+        },
+      ]);
+    });
+
     it("stringifies a non-Error flow failure in the error event", async () => {
       authorizeInFrontend.mockRejectedValueOnce("string boom");
       const restoreStdin = stubStdin(
@@ -759,4 +836,118 @@ describe("auth-helper", () => {
       }
     });
   });
+
+  describe("helperErrorEvent", () => {
+    it("omits the classification of an unclassified failure", () => {
+      expect(helperErrorEvent(new Error("no AS metadata"))).toEqual({
+        event: "error",
+        message: "no AS metadata",
+      });
+    });
+
+    it("carries a classified failure's exit code and envelope, minus message", () => {
+      const refusal = new FetchTargetError(
+        "refused",
+        "http://169.254.169.254/x",
+      );
+      expect(helperErrorEvent(refusal)).toEqual({
+        event: "error",
+        message: "refused",
+        exitCode: EXIT_CODES.FETCH_REFUSED,
+        envelope: { code: "fetch_refused", url: "http://169.254.169.254/x" },
+      });
+    });
+  });
+
+  describe("relayedHelperError", () => {
+    const message = "Sign-in helper failed: x";
+    type RelayEvent = Parameters<typeof relayedHelperError>[0];
+
+    it.each<[string, unknown]>([
+      ["no exit code", { event: "error", message: "x" }],
+      [
+        "exit code OK",
+        { event: "error", message: "x", exitCode: 0, envelope: { code: "ok" } },
+      ],
+      [
+        "an unknown exit code",
+        { event: "error", message: "x", exitCode: 99, envelope: { code: "z" } },
+      ],
+      ["no envelope", { event: "error", message: "x", exitCode: 12 }],
+      [
+        "a null envelope",
+        { event: "error", message: "x", exitCode: 12, envelope: null },
+      ],
+      [
+        "a non-string code",
+        { event: "error", message: "x", exitCode: 12, envelope: { code: 12 } },
+      ],
+    ])("returns undefined for %s", (_label, event) => {
+      // Decoded wire input: the shape is exactly what is under test.
+      expect(relayedHelperError(event as RelayEvent, message)).toBeUndefined();
+    });
+
+    it("keeps well-typed fields, sanitizes strings, and drops the rest", () => {
+      const error = relayedHelperError(
+        {
+          event: "error",
+          message: "x",
+          exitCode: EXIT_CODES.AUTH_REQUIRED,
+          envelope: {
+            code: "oauth_error",
+            status: 400,
+            url: "https://as.example/token\u001b[2J",
+            oauthError: "invalid_grant",
+            oauthErrorDescription: "expired",
+            method: "tools/list",
+            retryAfter: "5",
+            retryAfterMs: 5000,
+            cause: "token endpoint said no\u001b[2J",
+          },
+        },
+        message,
+      );
+      expect(error?.exitCode).toBe(EXIT_CODES.AUTH_REQUIRED);
+      expect(error?.message).toBe(message);
+      expect(error?.envelope).toEqual({
+        code: "oauth_error",
+        status: 400,
+        url: "https://as.example/token\u241b[2J",
+        oauthError: "invalid_grant",
+        oauthErrorDescription: "expired",
+        method: "tools/list",
+        retryAfter: "5",
+        retryAfterMs: 5000,
+      });
+      // The cause goes back onto the error, where classifyError reads it.
+      expect(error?.cause).toBe("token endpoint said no\u241b[2J");
+    });
+
+    it("round-trips a wrapped refusal to the envelope the TTY path prints", () => {
+      const original = new TypeError("fetch failed", {
+        cause: new FetchTargetError(
+          "Refusing to fetch from a link-local or metadata address (169.254.169.254)",
+          "http://169.254.169.254/.well-known/oauth-authorization-server",
+        ),
+      });
+      // Through the wire, exactly as the parent decodes it.
+      const event = JSON.parse(
+        JSON.stringify(helperErrorEvent(original)),
+      ) as RelayEvent;
+      const relayed = relayedHelperError(event, event.message);
+      expect(classifyError(relayed)).toEqual(classifyError(original));
+    });
+
+    it("drops mistyped optional fields rather than coercing them", () => {
+      const event: unknown = {
+        event: "error",
+        message: "x",
+        exitCode: EXIT_CODES.FETCH_REFUSED,
+        envelope: { code: "fetch_refused", status: "500", url: 42 },
+      };
+      // Decoded wire input: the mistyped fields are what is under test.
+      const error = relayedHelperError(event as RelayEvent, message);
+      expect(error?.envelope).toEqual({ code: "fetch_refused" });
+    });
+  });
 });

clients/mcpdo/__tests__/ema-login-helper.test.ts

@@ -270,10 +270,14 @@ describe("ema-login-helper", () => {
 
     it("emits an error event and rethrows when EMA is not configured", async () => {
       await expect(runEmaLoginHelper()).rejects.toThrow(/not configured/i);
+      // The failure is pre-classified (usage), and that classification rides
+      // on the event so the parent reports it instead of auth_required (#2962).
       expect(events()).toEqual([
         {
           event: "error",
           message: expect.stringContaining("not configured") as string,
+          exitCode: 1,
+          envelope: { code: "usage" },
         },
       ]);
     });

clients/mcpdo/src/connection/auth-helper.ts

@@ -8,8 +8,10 @@ import type {
   MCPServerConfig,
 } from "@inspector/core/mcp/types.js";
 import {
+  classifyError,
   CliExitCodeError,
   EXIT_CODES,
+  type ErrorEnvelope,
 } from "@inspector/core/cli/error-handler.js";
 import {
   writeFetchRefusalWarning,
@@ -40,6 +42,13 @@ import { sanitizeText } from "./sanitize.js";
  * must tell the user travels on the stdout protocol too: a destination-guard
  * refusal (#2846) arrives as a `fetch_refused` event, which the parent prints
  * as its own `Warning:` line before the URL or the error (#2905).
+ *
+ * A flow failure is classified **in the helper**, where the original error
+ * object still exists, and its exit code and envelope fields ride on the
+ * `error` event (#2962). The parent cannot classify the relayed text itself —
+ * a fetch-guard refusal is recognised by its error type, not its wording — so
+ * without this every pre-URL failure came out as `auth_required` / exit 3
+ * while the TTY path reported the same failure as, say, `fetch_refused` / 12.
  */
 
 /** Hidden subcommand name (see registerAuthCommands in mcp.ts). */
@@ -56,7 +65,88 @@ export type AuthHelperEvent =
   | { event: "auth_url"; url: string }
   | { event: "fetch_refused"; url: string; reason: string }
   | { event: "done" }
-  | { event: "error"; message: string };
+  | {
+      event: "error";
+      message: string;
+      /**
+       * The failure's classification, computed by the helper with
+       * `classifyError` (#2962). Present only when the failure classified as
+       * something more specific than the catch-all `error`; absent, the
+       * parent falls back to `auth_required`.
+       */
+      exitCode?: number;
+      envelope?: Partial<ErrorEnvelope>;
+    };
+
+/**
+ * Build the `error` event a helper emits for a failed flow, carrying the
+ * failure's classification when it has one (#2962). The catch-all `error`
+ * classification is left off: the parent's `auth_required` fallback is the
+ * better reading of an unclassified sign-in failure, and it is what the
+ * event meant before classification was relayed.
+ */
+export function helperErrorEvent(error: unknown): AuthHelperEvent {
+  const message = error instanceof Error ? error.message : String(error);
+  const { exitCode, envelope } = classifyError(error);
+  if (envelope.code === "error") return { event: "error", message };
+  // `message` travels once, at the top level; the envelope carries the rest.
+  const fields: Partial<ErrorEnvelope> = { ...envelope };
+  delete fields.message;
+  return { event: "error", message, exitCode, envelope: fields };
+}
+
+/** Every exit code a relayed classification may name — never `OK`. */
+const RELAYABLE_EXIT_CODES: ReadonlySet<number> = new Set(
+  Object.values(EXIT_CODES).filter((code) => code !== EXIT_CODES.OK),
+);
+
+/**
+ * Rebuild the classified error a helper relayed (#2962), or `undefined` when
+ * the event carries no usable classification. Every field is type-checked
+ * rather than trusted, and every string is sanitized: it is server-derived
+ * text on its way to a terminal, like the message.
+ */
+export function relayedHelperError(
+  event: Extract<AuthHelperEvent, { event: "error" }>,
+  message: string,
+): CliExitCodeError | undefined {
+  const { exitCode, envelope } = event;
+  if (
+    typeof exitCode !== "number" ||
+    !RELAYABLE_EXIT_CODES.has(exitCode) ||
+    typeof envelope !== "object" ||
+    envelope === null ||
+    typeof envelope.code !== "string"
+  ) {
+    return undefined;
+  }
+  const text = (value: unknown) =>
+    typeof value === "string" ? sanitizeText(value) : undefined;
+  const number = (value: unknown) =>
+    typeof value === "number" && Number.isFinite(value) ? value : undefined;
+  const fields: Partial<ErrorEnvelope> = {
+    code: sanitizeText(envelope.code),
+    status: number(envelope.status),
+    url: text(envelope.url),
+    method: text(envelope.method),
+    retryAfter: text(envelope.retryAfter),
+    retryAfterMs: number(envelope.retryAfterMs),
+    oauthError: text(envelope.oauthError),
+    oauthErrorDescription: text(envelope.oauthErrorDescription),
+  };
+  // Drop absent fields rather than carrying `undefined` keys into the
+  // envelope, matching every other optional envelope field.
+  for (const key of Object.keys(fields) as (keyof ErrorEnvelope)[]) {
+    if (fields[key] === undefined) delete fields[key];
+  }
+  const relayed = new CliExitCodeError(exitCode, message, fields);
+  // `classifyError` reads a cause off the error itself, never off its
+  // envelope, so the relayed one goes back where it came from — otherwise a
+  // wrapped refusal (`fetch failed` caused by the guard) loses its reason.
+  const cause = text(envelope.cause);
+  if (cause !== undefined) relayed.cause = cause;
+  return relayed;
+}
 
 /**
  * Pending sign-in marker, one per flow key (a server URL, or an
@@ -240,10 +330,7 @@ export async function runAuthHelper(): Promise<void> {
     });
     emit({ event: "done" });
   } catch (error) {
-    emit({
-      event: "error",
-      message: error instanceof Error ? error.message : String(error),
-    });
+    emit(helperErrorEvent(error));
     throw error;
   } finally {
     if (markerPath !== undefined) removeOwnPendingAuthMarker(markerPath);
@@ -494,7 +581,12 @@ async function spawnAuthHelperForUrl(
             clearTimeout(timer);
             // The helper relays server-derived text; strip C0/C1 controls
             // before it reaches a terminal via the error envelope.
-            fail(`Sign-in helper failed: ${sanitizeText(event.message)}`);
+            const message = `Sign-in helper failed: ${sanitizeText(event.message)}`;
+            // Keep the helper's classification when it relayed one (#2962);
+            // `auth_required` is only the fallback for an unclassified one.
+            const relayed = relayedHelperError(event, message);
+            if (relayed !== undefined) reject(relayed);
+            else fail(message);
             return;
           }
         }