Skip to content

Record which client failed a token exchange and why - #199

Open
robertjamesprior wants to merge 2 commits into
mainfrom
hypeship/oauth-failure-attribution
Open

robertjamesprior wants to merge 2 commits into
mainfrom
hypeship/oauth-failure-attribution

Conversation

@robertjamesprior

@robertjamesprior robertjamesprior commented Sep 17, 2026

Copy link
Copy Markdown

Scope

This instruments the retained legacy TypeScript path. Since #202, new-client discovery points at auth.onkernel.com, so the canonical path is the Go authorization server, which emits kernel.oauth.token_exchange to SigNoz and never writes this PostHog event.

The Go side needs the same two additions and does not have them: oauthas/telemetry.go carries client_type only, with no client id, and no upstream provider code on the metric. That is scope item 1 of KERNEL-2258 and is not folded in here.

So this is deliberately partial coverage. It is still worth landing: the legacy path carries every cached client until they re-register, and it is where the failures in KERNEL-2279 were measured.

Why

oauth_token_exchange records the stage a failure happened at and nothing about who hit it or what the provider said.

  • Which client. oauth_client_type only distinguishes kernel_cli, registered_client and unknown, so every third-party client is one bucket.
  • Why the provider rejected it. src/app/token/route.ts collapsed any non-OK Clerk response into a flat invalid_grant and discarded the body.

Over the 30 days to Sep 16, 1,009 new connections succeeded and 102 failed at the provider exchange, in bursts: 42% of attempts on Sep 9, 0% on most days. Of the 65 distinct source IPs that hit it, 16 were ever seen completing an exchange afterwards.

Codex install issue #118 is one report of this same failure, open since June because nobody could tell from the data which client it was or what Clerk said.

What this adds

  • oauth_client_id. Not a secret and not personal data for a public PKCE client, stable per registered client, already in the request.
  • oauth_provider_status_code and oauth_provider_error_code, the latter normalized against the RFC 6749 section 5.2 code set with an unknown fallback.

The provider's error_description is never recorded.

On what the code does and does not tell you: invalid_grant covers expired, revoked, redirect-mismatched and wrong-client grants, so it narrows a failure rather than identifying its cause. An earlier revision of this PR claimed more than that.

Testing

bun test: 627 pass, 0 fail. Route tests cover the rejection path, the success path, and an unrecognized provider code falling back to unknown without leaking the original string. analytics.test.ts asserts the three values reach PostHog under their exact property names with non-undefined values, since Bun treats extra undefined properties as absent.

🤖 Generated with Claude Code

@vercel

vercel Bot commented Sep 17, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
mcp Ready Ready Preview Sep 18, 2026 7:42pm UTC

@robertjamesprior

Copy link
Copy Markdown
Author

Context, plus a note on priority.

This is the instrument for KERNEL-2279, not a fix. Over the last 30 days in prod, 1,009 first-time connections succeeded and 102 failed at the provider exchange, arriving in bursts: 42% of attempts failed on Sep 9, 37% on Aug 19, 0% on most days. Of the 65 distinct source IPs that hit it, only 16 were ever seen completing an exchange afterwards. The event currently records the stage and nothing else, so there is nothing to investigate with. Codex issue #118 is one report of the same failure and it has been open since June for that reason.

33 lines of non-test code. Two things worth more than a skim: reading the Clerk body on the rejection branch (nothing downstream consumes it, but worth confirming), and whether recording client_id sits inside the privacy line you drew on this event originally.

This should not jump the CUA-TS cutover. It has been failing quietly since mid-August and another week changes nothing.

@masnwilliams masnwilliams left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requesting two narrow fixes before merge:

  1. keep the provider error boundary bounded and make the telemetry claim accurate. invalid_grant covers expired, revoked, redirect-mismatched, and wrong-client grants under RFC 6749, so the current field does not distinguish the causes named by the PR. either add a privacy-safe reason classifier with regression cases for distinct causes, or scope the field and surrounding names/comments to the coarse provider OAuth code it actually records.

  2. cover the final analytics sink. the route tests stop at recordExchange, so they do not prove the three new values reach PostHog under the intended property names. add concrete non-undefined assertions in analytics.test.ts.

one rollout note, not a reason to expand this diff across repos: #202 has now moved new-client discovery to the Go authorization server, so this instruments retained legacy TypeScript traffic rather than the canonical path. please update the PR scope and link the Go telemetry follow-up so this is not treated as complete current-path coverage.

validated at 849c0512d467a80566795eff3d0a9bb8700aee8a, including a synthetic merge with current main: typecheck passed, 621 tests passed, changed-file formatting passed, and git diff --check passed.

Comment thread src/app/token/route.ts Outdated
): Promise<string | undefined> {
try {
const body = (await response.json()) as { error?: unknown };
return typeof body.error === "string" ? body.error.slice(0, 64) : undefined;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

body.error is still an untrusted string. slicing it bounds length, but it does not prevent free text, PII, or high-cardinality values from reaching logs and PostHog. please normalize against a small provider OAuth-code allowlist (with an unknown fallback) rather than recording arbitrary strings. also, invalid_grant itself does not distinguish the expired/revoked/redirect-mismatch cases named above, so the helper comment and test name currently overstate what this value tells us.

Comment thread src/lib/mcp/analytics.ts
oauth_outcome: exchange.outcome,
oauth_error_code: exchange.errorCode,
oauth_provider_status_code: exchange.providerStatusCode,
oauth_provider_error_code: exchange.providerErrorCode,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please extend the existing captureOAuthTokenExchange test with non-undefined clientId, providerStatusCode, and providerErrorCode values and assert these exact PostHog keys. the route tests only verify the object passed into recordExchange, and Bun treats extra undefined properties as absent in the current sink assertion, so a typo or omitted mapping here would still pass.

robertjamesprior and others added 2 commits September 18, 2026 19:40
Token exchange telemetry records that a stage failed and nothing about
who or why, so a failure that has been breaking first-time connections
cannot be attributed to a client or a cause.

Add the OAuth client_id to the event, and on a provider rejection record
the upstream status and the provider's own error code. The code is what
separates an expired authorization code from a redirect mismatch or a
revoked client; the free-text description that accompanies it is not
recorded.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review feedback. The provider's error field was recorded as an
arbitrary bounded-length string, which still lets free text, PII and
unbounded cardinality reach logs and analytics. Normalize it against
the RFC 6749 section 5.2 code set with an `unknown` fallback.

Also correct the claim: `invalid_grant` covers expired, revoked,
redirect-mismatched and wrong-client grants, so the value narrows a
failure rather than identifying its cause. Comments and the test name
said otherwise.

The route tests only proved the object handed to recordExchange, and
Bun treats extra undefined properties as absent, so a mapping typo
would have passed. Assert the three PostHog keys directly.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@robertjamesprior
robertjamesprior force-pushed the hypeship/oauth-failure-attribution branch from 849c051 to e1b2915 Compare September 18, 2026 19:41
@robertjamesprior

Copy link
Copy Markdown
Author

all three addressed.

bounded error code. normalized against the RFC 6749 section 5.2 set with an unknown fallback, so free text, PII and unbounded cardinality cannot reach logs or analytics. added a regression case: an unrecognized code records as unknown and the original string does not appear in the event.

the overclaim. you were right, invalid_grant narrows a failure rather than identifying its cause. corrected in the helper comment, the type doc and the test name, and removed the causal claim from the PR body.

the sink. analytics.test.ts now asserts oauth_client_id, oauth_provider_status_code and oauth_provider_error_code directly with non-undefined values, rather than stopping at recordExchange.

scope. PR body now says this instruments the legacy TypeScript path only, and points at KERNEL-2258 scope item 1 for the Go equivalent. oauthas/telemetry.go carries client_type and no client id, so the canonical path has the same blind spot.

627 tests pass.

one related note: the PostHog alert I built for KERNEL-2279 had the same problem, so it is renamed "legacy TypeScript path" and there is now a SigNoz alert on kernel.oauth.token_exchange covering the Go server.

@robertjamesprior

Copy link
Copy Markdown
Author

correction to something I wrote in the PR body and in KERNEL-2279: I said the Go port already fixed the upstream-reason gap. it does not.

oauthas/clerk.go:280 parses Clerk's error into a typed ProviderOAuthError, but oauthas/token.go:183 matches it with errors.As and never reads it, returning the same flat invalid_grant. providerErr appears twice in that file, on the declaration and the match, and there is no logging at the site. so the cutover does not diagnose #118, and this PR is not redundant with it.

also worth flagging before anyone implements KERNEL-2258 scope item 1: the two fields have different cost on the Go side. the provider error code is bounded to six RFC values plus unknown and is fine as an otel metric attribute. a client id is not, since dynamic registration mints one per install and every attribute combination becomes a time series. that one belongs on a log line or a span, not on kernel.oauth.token_exchange. noted in 2258.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants