Repository navigation
fix(oauth): stop reporting DCR refusals as MCPJam errors - #6859
weimar-torres-herrera wants to merge 2 commits into
Conversation
An authorization server that answers the debugger's registration POST with a non-2xx status (no open DCR, an initial access token required, metadata it rejects) was filed in Sentry as an MCPJam error, twice per refusal: the bare message and its fallback-hint twin. The debug proxy returns whatever the server under test answered and its own failures throw instead, so "Dynamic Client Registration failed (N)" is always the server's answer. The SDK now exports isDynamicClientRegistrationHttpError and the adapter adds it to isUnreportedStepFailure; the message stays on screen, and proxy failures keep reporting. Refs: PLB-274
…entry-noise # Conflicts: # mcpjam-inspector/client/src/lib/oauth/debug-state-machine-adapter.ts
GitHub Actions
Failure
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
No actionable comments were generated in the recent review.
 |
|
Found 1 test failure on Blacksmith runners: Failure
|
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light-3x.png)
olartgabo
left a comment
olartgabo
left a comment
There was a problem hiding this comment.
Process
- PLB-274 is In Progress; set it to In review.
- The "I can explain every line" box is unticked.
The rest of the record is complete: approved specs and plan, every criterion tested, and a changeset.
Code
Both debug proxy routes return the target's status inside a 200. A proxy non-2xx surfaces as "Client registration failed" and still reports. So the new match only silences answers from the authorization server. Two optional notes are inline. I'll approve once the process items are fixed.
| * can keep it visible without filing it as an MCPJam defect. | ||
| */ | ||
| export function isDynamicClientRegistrationHttpError(error: string): boolean { | ||
| return error.startsWith(HTTP_ERROR_PREFIX); |
There was a problem hiding this comment.
Optional. The bare prefix also matches Dynamic Client Registration failed (undefined). and (0).. The client builds the status from data.status in the proxy's 200 body, so either form means the proxy contract broke, which is our bug, and those should still report. Matching /^Dynamic Client Registration failed \(\d{3}\)\./ silences only real HTTP answers and still covers the hinted form.
| ); | ||
|
|
||
| it("still reports the debug proxy failing on registration", async () => { | ||
| await replayRegistrationFailure(async () => { |
There was a problem hiding this comment.
Optional. This case throws the proxy error by hand, so it would still pass if createDebugRequestExecutor stopped throwing on a proxy non-2xx, which is the contract this change relies on. wrappedUpdateState() already returns the real execute; passing it with authFetch resolving new Response("", { status: 500 }) would test the real path.
Task, spec and plan
User story
As an on-call engineer watching Sentry and #mcpjam-alerts,
I want the OAuth debugger to stop filing an authorization server's refusal of Dynamic Client Registration as an MCPJam error,
so that INSPECTOR-CLIENT-2N2, -2JB, -2JM and their siblings (125 events from 17 users in the last 30 days, each refusal counted twice) stop alerting on server configuration we cannot fix.
Acceptance criteria
Dynamic Client Registration failed (<status>)., with the hint when no pre-registered client exists, exactly as before.Client registration failed: Backend debug proxy error: …) still reports.invalid_response) still reports.What this does
The debug proxy wraps whatever the server under test answered in an HTTP 200. Its own failures come back as non-2xx, which the client throws as
Backend debug proxy errorandexecuteDynamicClientRegistrationturns intoClient registration failed: …. SoDynamic Client Registration failed (N)only ever follows the authorization server's real answer: registration is closed, it wants an initial access token we were never given, or it rejects our metadata. The SDK now exportsisDynamicClientRegistrationHttpErrornext to the code that writes that message, and the adapter adds it toisUnreportedStepFailure, which already silences a missingissuer,registration_endpointor resource metadata and an unexpected probe status. It matches every status, not only 4xx, because the status is the server's answer;isUnexpectedProbeStatusfollows the same rule. This supersedes #4484, which silenced only 4xx on the assumption that a 5xx could come from the proxy, and builds on its analysis of the doubled report. Failures that still report still arrive as that bare/hinted pair; #5473's finding key already files both in one Sentry issue, so collapsing them stays out of scope.Validation:
it.eachcases (400/401/403/404/500) failed on main withreportCaught"called 2 times".debug-state-machine-step-reporting.test.tsanddebug-state-machine-adapter.test.ts, 52 tests passing after merging main. The new cases run the real SDKexecuteDynamicClientRegistrationand replay both updates the state machines write.sdk/tests/oauth/dynamic-client-registration.test.ts, 75 passing (the message is byte-stable).server/routes/web/__tests__/oauth-proxy-projection.test.ts, 44 passing (pins the proxy contract above).npm run typecheck -w @mcpjam/sdkandnpm run typecheck:client -w @mcpjam/inspectorboth exit 0.npm run slop:check: no rule's count went up. Prettier is clean on the changed hunks. Re-checked on the merge with main (fix(oauth): name the server's certificate when TLS trust fails #6832 addedisTlsTrustFailureto the same function; both predicates are kept).Live check (local, 2026-10-26). I ran the OAuth debugger harness (
/__e2e/oauth-debugger, ports 5373/6473) against a fake authorization server whose/registerrefuses, through the real local debug proxy with an access-link token. Every Sentry envelope was captured in the page and never sent, and abeforeSendEventhook on the page's own Sentry client recorded each event's content. On main, one refusal produced 2 Sentry warnings for each of 400, 403 and 500. On this branch it produced 0, with the message unchanged on screen. The control, a registration endpoint on a closed port so the real proxy itself answered HTTP 500, still produced its 2 reports on both. The same four runs on the merge with main gave the same result.Documentation
Nothing applicable: error reporting only, no user-facing behavior or docs change.
Architecture
Screenshots and recordings
No UI change. The image shows the debugger before and after (same message on screen) and the Sentry report count from each local run.
Deleted
Nothing.
Reused
isUnreportedStepFailureseam indebug-state-machine-adapter.ts, and the pattern of an SDK-exported predicate next to the code that writes the message (isUnexpectedProbeStatus,isAuthenticatedRequestFailure).wrappedUpdateState) and the realexecuteDynamicClientRegistration.Production signal
Sentry should show no
message:"Dynamic Client Registration failed (*"events from the new release, and PostHog error tracking none on hosted. Older desktop builds keep sending them until users update, so 2JB, 2N2, 2JK, 2MF and 2JM get resolved in the first fixed release, not plainly; that stops old builds from reopening them and alerting Slack again. 2FH, 28B and 2C6 predate #5473's grouping and may hold other failures, so their latest events get checked first.Automated decisions
None: no automated decision about users.
Checklist
npm run slop:size: 27).as any, emptycatch,.catch(() => {})or rawconsoleon the server.Refs PLB-274
Need help on this PR? Tag
@codesmith-botwith what you need.