fix(oauth): stop filing DCR refusals as MCPJam bugs

.changeset/dcr-refusal-not-an-mcpjam-bug.md

@@ -0,0 +1,16 @@
+---
+"@mcpjam/sdk": patch
+"@mcpjam/inspector": patch
+---
+
+A registration endpoint answering 4xx is the server under test declining, not an MCPJam defect
+
+The OAuth debugger reported every Dynamic Client Registration failure to Sentry. A 4xx from the registration endpoint is the authorization server declining to mint a client — DCR left unimplemented behind an advertised `registration_endpoint`, an allowlist, an initial access token the client was never given, or metadata the server will not accept. The debugger exists to show that; filing it as a bug against MCPJam is the same noise the client-side origin policy already removes everywhere else.
+
+`isClientRegistrationRefusal` classifies those 4xx messages. 5xx and transport failures are deliberately not refusals: those can be the debug proxy failing, which is ours. 4xx is read as the server's policy knowingly rather than because it always is — RFC 7591 also spends `invalid_client_metadata` on a body the client built wrong, but in practice that code comes back for a valid body the server declines, and the request builder is covered by its own unit tests rather than by this signal.
+
+One rejection also produced two reports. The machines write the bare message, then the same message with the fallback hint appended once no pre-registered client turns up, and the debugger's dedup guard keys on the string. `restatesWithFallbackHint` pairs them by matching the hint exactly rather than by prefix — a prefix rule would have swallowed `Token request failed: 400 Bad Request` followed by the same line carrying `invalid_grant`, which is a more informative failure and not a duplicate.
+
+Both messages still render in the debugger. Only the Sentry issue goes away.
+
+`REGISTRATION_FAILURE_PREFIX` is now exported and read by `trace.ts`, which previously repeated the literal, so a rewording cannot leave the recovered-fallback check matching stale text.

mcpjam-inspector/client/src/lib/oauth/__tests__/debug-state-machine-step-reporting.test.ts

@@ -15,7 +15,10 @@ vi.mock("@mcpjam/sdk/browser", async (importOriginal) => {
   return { ...actual, createOAuthStateMachine };
 });
 
-import { AUTHORIZATION_SERVER_METADATA_MISSING_ISSUER } from "@mcpjam/sdk/browser";
+import {
+  AUTHORIZATION_SERVER_METADATA_MISSING_ISSUER,
+  executeDynamicClientRegistration,
+} from "@mcpjam/sdk/browser";
 
 import { createInspectorOAuthStateMachine } from "../debug-state-machine-adapter";
 
@@ -39,6 +42,41 @@ function wrappedUpdateState(updateState = vi.fn(), currentStep = "metadata") {
   return { wrapped: passed.updateState, updateState };
 }
 
+/**
+ * Replay the pair of messages one failed registration writes: the bare failure,
+ * then the same failure with the fallback hint appended once no pre-registered
+ * client turns up. Taken from the SDK outcome so a rewording there cannot leave
+ * this asserting stale text.
+ */
+async function replayRegistrationFailure(status: number) {
+  const dcr = await executeDynamicClientRegistration({
+    request: {
+      method: "POST",
+      url: "https://auth.example.test/register",
+      headers: {},
+      body: { client_name: "Test Client" },
+    },
+    requestExecutor: async () => ({
+      ok: false,
+      status,
+      statusText: "Registration failed",
+      headers: {},
+      body: { error: "registration_failed" },
+    }),
+  });
+  if (dcr.status === "registered") {
+    throw new Error(`expected a ${status} outcome`);
+  }
+
+  const { wrapped, updateState } = wrappedUpdateState(
+    vi.fn(),
+    "request_client_registration",
+  );
+  wrapped({ error: dcr.error });
+  wrapped({ error: dcr.errorWithFallbackHint });
+  return { updateState };
+}
+
 describe("OAuth debugger step-failure reporting", () => {
   beforeEach(() => {
     reportCaught.mockReset();
@@ -163,6 +201,37 @@ describe("OAuth debugger step-failure reporting", () => {
     expect(reportCaught).toHaveBeenCalledTimes(2);
   });
 
+  it("ignores a registration the authorization server refused", async () => {
+    // A 4xx from the registration endpoint is that server declining to mint a
+    // client, which is what the debugger exists to show — not a defect here.
+    const { updateState } = await replayRegistrationFailure(400);
+
+    expect(reportCaught).not.toHaveBeenCalled();
+    // Silenced for Sentry only — both messages still reach the screen.
+    expect(updateState).toHaveBeenCalledTimes(2);
+  });
+
+  it("reports a registration the server failed to answer exactly once", async () => {
+    // A 5xx can be ours (a broken debug proxy), so it still reports. One
+    // failure writes two strings though — the bare message, then the same
+    // message with the fallback hint appended — and both used to file an issue.
+    await replayRegistrationFailure(503);
+
+    expect(reportCaught).toHaveBeenCalledTimes(1);
+  });
+
+  it("still reports a failure that merely extends the last one", () => {
+    // Only the fallback-hint pair collapses. A retry that comes back with the
+    // reason the first attempt lacked is a more informative failure, not a
+    // duplicate.
+    const { wrapped } = wrappedUpdateState();
+
+    wrapped({ error: "Token request failed: 400 Bad Request" });
+    wrapped({ error: "Token request failed: 400 Bad Request: invalid_grant" });
+
+    expect(reportCaught).toHaveBeenCalledTimes(2);
+  });
+
   it("still forwards every update to the caller's updateState", () => {
     const { wrapped, updateState } = wrappedUpdateState();
 

mcpjam-inspector/client/src/lib/oauth/debug-state-machine-adapter.ts

@@ -3,7 +3,9 @@ import {
   DEFAULT_MCPJAM_CLIENT_ID_METADATA_URL,
   createOAuthStateMachine,
   getBrowserDebugDynamicRegistrationMetadata,
+  isClientRegistrationRefusal,
   isLoopbackOAuthUrl,
+  restatesWithFallbackHint,
   type OAuthFlowState,
   type OAuthProtocolVersion,
   type OAuthRequestExecutor,
@@ -272,6 +274,37 @@ const UNREPORTED_STEP_FAILURES = new Set([
   AUTHORIZATION_SERVER_METADATA_MISSING_ISSUER,
 ]);
 
+/**
+ * Failures the server under test owns, matched on shape rather than by exact
+ * string because the message carries the status code.
+ *
+ * A 4xx from the registration endpoint is that server declining to register a
+ * client — DCR unimplemented behind an advertised endpoint, an allowlist, an
+ * initial access token we were never given. The debugger exists to show that;
+ * reporting it files a bug against MCPJam for another project's policy.
+ */
+function isUnreportedStepFailure(error: string): boolean {
+  return (
+    UNREPORTED_STEP_FAILURES.has(error) || isClientRegistrationRefusal(error)
+  );
+}
+
+/**
+ * Is this message the previous report restated rather than a new failure?
+ *
+ * Plain inequality treats a registration failure's bare message and its
+ * hint-appended twin as two failures, so one rejection filed two issues.
+ */
+function restatesLastReport(
+  error: string,
+  lastReported: string | undefined,
+): boolean {
+  if (lastReported === undefined) return false;
+  return (
+    error === lastReported || restatesWithFallbackHint(error, lastReported)
+  );
+}
+
 /**
  * Wrap the caller's `updateState` so every NEW step failure is reported.
  *
@@ -287,7 +320,7 @@ const UNREPORTED_STEP_FAILURES = new Set([
  *
  * `Warning: `-prefixed messages are skipped entirely — those are advisories the
  * flow recovers from (an optional metadata field the server left out), not step
- * failures. So are the messages in `UNREPORTED_STEP_FAILURES`.
+ * failures. So are the messages `isUnreportedStepFailure` claims.
  */
 function withStepFailureReporting(
   updateState: InspectorOAuthStateMachineConfig["updateState"],
@@ -299,7 +332,7 @@ function withStepFailureReporting(
     const error = updates.error;
     if (
       typeof error === "string" &&
-      (error.startsWith("Warning: ") || UNREPORTED_STEP_FAILURES.has(error))
+      (error.startsWith("Warning: ") || isUnreportedStepFailure(error))
     ) {
       // Not ours to act on: the message is already on screen, and reporting
       // these buries real step failures under server-under-test nits.
@@ -309,7 +342,11 @@ function withStepFailureReporting(
       updateState(updates);
       return;
     }
-    if (typeof error === "string" && error !== "" && error !== lastReportedError) {
+    if (
+      typeof error === "string" &&
+      error !== "" &&
+      !restatesLastReport(error, lastReportedError)
+    ) {
       lastReportedError = error;
       reportCaught(new Error(sanitizeStepError(error)), {
         source: "oauth_debugger_step",

sdk/src/browser.ts

@@ -581,6 +581,8 @@ export {
 export {
   buildDynamicClientRegistrationRequest,
   executeDynamicClientRegistration,
+  isClientRegistrationRefusal,
+  restatesWithFallbackHint,
 } from "./oauth/state-machines/shared/dynamic-client-registration.js";
 export type {
   DynamicClientRegistrationCredentials,

sdk/src/index.ts

@@ -832,6 +832,8 @@ export type {
 export {
   buildDynamicClientRegistrationRequest,
   executeDynamicClientRegistration,
+  isClientRegistrationRefusal,
+  restatesWithFallbackHint,
 } from "./oauth/state-machines/shared/dynamic-client-registration.js";
 export type {
   DynamicClientRegistrationCredentials,

sdk/src/oauth/state-machines/shared/dynamic-client-registration.ts

@@ -212,6 +212,47 @@ function backfillHistory<THistory extends BackfillableHistoryEntry>(
 const FALLBACK_HINT =
   "Configure a pre-registered client or enable DCR on the authorization server.";
 
+export const REGISTRATION_FAILURE_PREFIX = "Dynamic Client Registration failed";
+
+/**
+ * Did the authorization server REFUSE to register, rather than fail to answer?
+ * 5xx and transport failures can be ours, so they are deliberately not refusals.
+ *
+ * 4xx is read as the server's policy knowingly, not because it always is: RFC
+ * 7591 also spends `invalid_client_metadata` on a body the client built wrong.
+ * In practice that code comes back for a valid body the server declines — a
+ * loopback redirect, an auth method it will not issue — and the request builder
+ * is covered by its own unit tests rather than by this signal.
+ *
+ * Parses the messages built below — the machines expose only the string.
+ */
+export function isClientRegistrationRefusal(error: string): boolean {
+  const prefix = `${REGISTRATION_FAILURE_PREFIX} (`;
+  if (!error.startsWith(prefix)) return false;
+  const status = Number.parseInt(error.slice(prefix.length), 10);
+  return status >= 400 && status < 500;
+}
+
+/**
+ * Is `error` the same failure as `previous`, restated with the fallback hint?
+ *
+ * One rejected registration writes both: the bare message, then the message
+ * plus this hint once no pre-registered client turns up. Matching the hint
+ * exactly keeps the pair together without swallowing an unrelated failure that
+ * merely extends the last one (`"…: 400 Bad Request"` → `"…: 400 Bad Request:
+ * invalid_grant"` is two reports, not one).
+ */
+export function restatesWithFallbackHint(
+  error: string,
+  previous: string
+): boolean {
+  return (
+    error !== previous &&
+    error.startsWith(previous) &&
+    error.endsWith(FALLBACK_HINT)
+  );
+}
+
 /**
  * Executes an RFC 7591 registration POST via the caller's executor, back-fills
  * a cloned last history entry with the redacted response, and classifies the
@@ -255,13 +296,13 @@ export async function executeDynamicClientRegistration<
   const httpHistory = backfillHistory(input.httpHistory, redacted);
 
   if (!response.ok) {
-    const registrationError = `Dynamic Client Registration failed (${response.status}).`;
+    const registrationError = `${REGISTRATION_FAILURE_PREFIX} (${response.status}).`;
     return {
       status: "http_error",
       response: redacted,
       httpHistory,
       error: registrationError,
-      fallbackNote: `Dynamic Client Registration failed (${response.status}); using pre-registered client credentials.`,
+      fallbackNote: `${REGISTRATION_FAILURE_PREFIX} (${response.status}); using pre-registered client credentials.`,
       errorWithFallbackHint: `${registrationError} ${FALLBACK_HINT}`,
     };
   }

sdk/src/oauth/state-machines/trace.ts

@@ -1,3 +1,4 @@
+import { REGISTRATION_FAILURE_PREFIX } from "./shared/dynamic-client-registration.js";
 import {
   extractResponseErrorReason,
   toSingleLine,
@@ -255,7 +256,7 @@ function usesRecoveredDynamicClientRegistrationFallback(
   }
 
   return (
-    state.error.startsWith("Dynamic Client Registration failed") ||
+    state.error.startsWith(REGISTRATION_FAILURE_PREFIX) ||
     state.error.startsWith("Client registration failed:")
   );
 }

sdk/tests/oauth/dynamic-client-registration.test.ts

@@ -1,7 +1,9 @@
 import {
   buildDynamicClientRegistrationRequest,
   executeDynamicClientRegistration,
+  isClientRegistrationRefusal,
   redactDynamicClientRegistrationResponse,
+  restatesWithFallbackHint,
 } from "../../src/oauth/state-machines/shared/dynamic-client-registration.js";
 import { createOAuthStateMachine } from "../../src/oauth/state-machines/factory.js";
 import { EMPTY_OAUTH_FLOW_STATE } from "../../src/oauth/state-machines/types.js";
@@ -243,6 +245,100 @@ describe("executeDynamicClientRegistration", () => {
     );
   });
 
+  it.each([400, 401, 403, 404, 422, 499])(
+    "classifies a %i registration as a refusal",
+    async (status) => {
+      const outcome = await executeDynamicClientRegistration({
+        request: buildRequest(),
+        requestExecutor: jest.fn().mockResolvedValue({
+          ok: false,
+          status,
+          statusText: "Rejected",
+          headers: {},
+          body: { error: "invalid_client_metadata" },
+        }),
+        httpHistory: history(),
+      });
+      expect(outcome.status).toBe("http_error");
+      if (outcome.status === "registered") return;
+      // Both strings the machines can put in state, not just the bare one.
+      expect(isClientRegistrationRefusal(outcome.error)).toBe(true);
+      expect(isClientRegistrationRefusal(outcome.errorWithFallbackHint)).toBe(
+        true
+      );
+    }
+  );
+
+  it.each([500, 502, 503])(
+    "does not classify a %i registration as a refusal",
+    async (status) => {
+      const outcome = await executeDynamicClientRegistration({
+        request: buildRequest(),
+        requestExecutor: jest.fn().mockResolvedValue({
+          ok: false,
+          status,
+          statusText: "Server Error",
+          headers: {},
+          body: {},
+        }),
+        httpHistory: history(),
+      });
+      expect(outcome.status).toBe("http_error");
+      if (outcome.status === "registered") return;
+      expect(isClientRegistrationRefusal(outcome.error)).toBe(false);
+      expect(isClientRegistrationRefusal(outcome.errorWithFallbackHint)).toBe(
+        false
+      );
+    }
+  );
+
+  it("does not classify a transport failure as a refusal", async () => {
+    const outcome = await executeDynamicClientRegistration({
+      request: buildRequest(),
+      requestExecutor: jest.fn().mockRejectedValue(new Error("boom")),
+      httpHistory: history(),
+    });
+    expect(outcome.status).toBe("network_error");
+    if (outcome.status === "registered") return;
+    expect(isClientRegistrationRefusal(outcome.error)).toBe(false);
+  });
+
+  it.each(["", "Some other step failed (400).", "Dynamic Client Registration"])(
+    "does not claim the unrelated message %p",
+    (message) => {
+      expect(isClientRegistrationRefusal(message)).toBe(false);
+    }
+  );
+
+  it("pairs a registration failure with its hint-appended twin", async () => {
+    const outcome = await executeDynamicClientRegistration({
+      request: buildRequest(),
+      requestExecutor: jest.fn().mockResolvedValue({
+        ok: false,
+        status: 503,
+        statusText: "Service Unavailable",
+        headers: {},
+        body: {},
+      }),
+      httpHistory: history(),
+    });
+    expect(outcome.status).toBe("http_error");
+    if (outcome.status === "registered") return;
+    expect(
+      restatesWithFallbackHint(outcome.errorWithFallbackHint, outcome.error)
+    ).toBe(true);
+  });
+
+  it.each([
+    ["Token request failed: 400 Bad Request", "invalid_grant"],
+    ["Authenticated request failed: 401", "token expired"],
+  ])(
+    "does not pair %p with a message that merely extends it",
+    (bare, detail) => {
+      expect(restatesWithFallbackHint(`${bare}: ${detail}`, bare)).toBe(false);
+    }
+  );
+
   it("synthesizes a status-0 network_error on executor rejection", async () => {
     const outcome = await executeDynamicClientRegistration({
       request: buildRequest(),