From d6e25cca2d3b3cdf8b3047a7f2c5622c81270fcf Mon Sep 17 00:00:00 2001 From: ZeHuari Date: Fri, 28 Aug 2026 22:23:59 -0400 Subject: [PATCH] oauth: let the debugger's warnings arrive as warnings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `reportCaught` sent PostHog a property named `level`. PostHog error tracking groups, alerts and filters on `$exception_level`, and `captureException(err, props)` merges `props` over the properties it built — so `level` landed as a custom property nothing reads, and every report kept the `error` default whatever the caller declared. Only one caller declares anything else, and it is the loudest source in the project. `oauth_debugger_step` is `warning` on purpose: it reports the server UNDER TEST misbehaving, which is what a debugger is for, and its value is the aggregate trend rather than a page. It alerted as an Inspector crash anyway — 378 events across 97 users in 18 days, more than half of every client `$exception`. Sentry had the level right the whole time, which is why two earlier passes (#3955, #3959) could quiet Sentry message by message and leave PostHog untouched. `level` is kept alongside `$exception_level`: it has been on these events since the sink was written, and dropping it would break any saved filter. Second, smaller: a step that fails writes its bare message and then the same message with the recovery hint appended, and exact comparison saw two strings rather than one refusal — 29 of those 378 were a failure reported twice a millisecond apart. `isSameStepFailure` compares by prefix, in either order, which is narrow enough that an unrelated failure cannot be swallowed: it never begins with the whole text of the one before it. Neither change hides anything. The events still arrive; they stop claiming to be crashes. Co-Authored-By: Claude Opus 5 --- .../src/lib/__tests__/error-reporting.test.ts | 25 +++++++++++++ .../client/src/lib/error-reporting.ts | 16 ++++++++ ...debug-state-machine-step-reporting.test.ts | 37 +++++++++++++++++++ .../lib/oauth/debug-state-machine-adapter.ts | 28 +++++++++++++- 4 files changed, 105 insertions(+), 1 deletion(-) diff --git a/mcpjam-inspector/client/src/lib/__tests__/error-reporting.test.ts b/mcpjam-inspector/client/src/lib/__tests__/error-reporting.test.ts index d436e557cf..56dc3013fe 100644 --- a/mcpjam-inspector/client/src/lib/__tests__/error-reporting.test.ts +++ b/mcpjam-inspector/client/src/lib/__tests__/error-reporting.test.ts @@ -147,6 +147,31 @@ describe("reportCaught", () => { ); }); + // Sentry reads `level`; PostHog error tracking reads `$exception_level`, and + // groups, alerts and filters on it. Sending only `level` left every report at + // PostHog's `error` default, which silently overrode the one caller that asks + // for something quieter. + it("declares the level on the key PostHog actually reads", () => { + reportCaught(new Error("server under test misbehaved"), { + source: "oauth_debugger_step", + level: "warning", + }); + + expect(posthogCaptureException).toHaveBeenCalledWith( + expect.any(Error), + expect.objectContaining({ $exception_level: "warning" }), + ); + }); + + it("still defaults to error when the caller names no level", () => { + reportCaught(new Error("boom"), { source: "unit" }); + + expect(posthogCaptureException).toHaveBeenCalledWith( + expect.any(Error), + expect.objectContaining({ $exception_level: "error" }), + ); + }); + it("reports to Sentry but NOT PostHog on a non-capture surface", async () => { // `capture_exceptions: false` only disables posthog-js's automatic // window.onerror handler — an explicit captureException still sends. A diff --git a/mcpjam-inspector/client/src/lib/error-reporting.ts b/mcpjam-inspector/client/src/lib/error-reporting.ts index 784dcd01a3..1555238dfe 100644 --- a/mcpjam-inspector/client/src/lib/error-reporting.ts +++ b/mcpjam-inspector/client/src/lib/error-reporting.ts @@ -189,6 +189,22 @@ export function reportCaught(error: unknown, options: ReportOptions): void { posthog.captureException(normalized, { source: options.source, level: options.level ?? "error", + // The severity PostHog actually reads. `captureException(err, props)` + // merges `props` OVER the properties it built, and error tracking + // groups, alerts and filters on `$exception_level` — so the plain + // `level` above lands as a custom property nothing looks at, and every + // report kept the `error` default no matter what the caller declared. + // + // That silently overrode the one caller that asks for anything else. + // `oauth_debugger_step` is `warning` on purpose — it reports the + // server UNDER TEST misbehaving, which is what a debugger is for, and + // its value is the aggregate trend rather than a page. It alerted as + // an Inspector crash anyway: 378 events across 97 users in 18 days, + // more than half of every client `$exception` in the project. + // + // `level` is kept alongside it: it has been on these events since the + // sink was written, and dropping it would break any saved filter. + $exception_level: options.level ?? "error", ...(options.extra ?? {}), }); } diff --git a/mcpjam-inspector/client/src/lib/oauth/__tests__/debug-state-machine-step-reporting.test.ts b/mcpjam-inspector/client/src/lib/oauth/__tests__/debug-state-machine-step-reporting.test.ts index dfee280ff7..bf99b3059a 100644 --- a/mcpjam-inspector/client/src/lib/oauth/__tests__/debug-state-machine-step-reporting.test.ts +++ b/mcpjam-inspector/client/src/lib/oauth/__tests__/debug-state-machine-step-reporting.test.ts @@ -163,6 +163,43 @@ describe("OAuth debugger step-failure reporting", () => { expect(reportCaught).toHaveBeenCalledTimes(2); }); + it("reports one failure once when the hint is appended after it", () => { + // The SDK writes the bare message, then the same message with the recovery + // hint (`errorWithFallbackHint`). Two strings, one refusal — and exact + // comparison reported both, a millisecond apart. + const { wrapped } = wrappedUpdateState(); + + wrapped({ error: "Dynamic Client Registration failed (400)." }); + wrapped({ + error: + "Dynamic Client Registration failed (400). Configure a pre-registered client or enable DCR on the authorization server.", + }); + + expect(reportCaught).toHaveBeenCalledTimes(1); + }); + + it("swallows the pair in the other order too", () => { + // Nothing guarantees which of the two lands first, and a guard that only + // works one way round is a guard that works half the time. + const { wrapped } = wrappedUpdateState(); + + wrapped({ error: "Client registration failed: timeout. Configure a pre-registered client." }); + wrapped({ error: "Client registration failed: timeout." }); + + expect(reportCaught).toHaveBeenCalledTimes(1); + }); + + it("does not swallow a different failure that merely follows", () => { + // The narrowness is the point: an unrelated message never begins with the + // whole text of the one before it. + const { wrapped } = wrappedUpdateState(); + + wrapped({ error: "Dynamic Client Registration failed (400)." }); + wrapped({ error: "Authenticated request failed: 401 Unauthorized" }); + + expect(reportCaught).toHaveBeenCalledTimes(2); + }); + it("still forwards every update to the caller's updateState", () => { const { wrapped, updateState } = wrappedUpdateState(); diff --git a/mcpjam-inspector/client/src/lib/oauth/debug-state-machine-adapter.ts b/mcpjam-inspector/client/src/lib/oauth/debug-state-machine-adapter.ts index 653aaa8845..87935ceede 100644 --- a/mcpjam-inspector/client/src/lib/oauth/debug-state-machine-adapter.ts +++ b/mcpjam-inspector/client/src/lib/oauth/debug-state-machine-adapter.ts @@ -272,6 +272,28 @@ const UNREPORTED_STEP_FAILURES = new Set([ AUTHORIZATION_SERVER_METADATA_MISSING_ISSUER, ]); +/** + * One failure, or two? + * + * A step that fails twice over — DCR, say — writes its bare message first and + * then the same message with the recovery hint appended + * (`errorWithFallbackHint` in the SDK is `${error} ${FALLBACK_HINT}`). Exact + * comparison saw two different strings and reported both, one millisecond + * apart, so a single refusal arrived as a pair: 29 of 378 events over 18 days. + * + * Prefix, not equality, and in whichever order they arrive. This is + * deliberately narrow — an unrelated failure never begins with the whole text + * of the one before it, so widening a step's message cannot swallow the next + * step's. + */ +function isSameStepFailure( + error: string, + lastReported: string | undefined +): boolean { + if (lastReported === undefined) return false; + return error.startsWith(lastReported) || lastReported.startsWith(error); +} + /** * Wrap the caller's `updateState` so every NEW step failure is reported. * @@ -309,7 +331,11 @@ function withStepFailureReporting( updateState(updates); return; } - if (typeof error === "string" && error !== "" && error !== lastReportedError) { + if ( + typeof error === "string" && + error !== "" && + !isSameStepFailure(error, lastReportedError) + ) { lastReportedError = error; reportCaught(new Error(sanitizeStepError(error)), { source: "oauth_debugger_step",