fix(error-classifier): stop reading embedded digits as auth signals - #248
Conversation
Connection failures whose text happens to embed "401" inside a port, duration, hostname, or request id were classified as auth issues. That sends transport and DNS failures down the OAuth promotion path, so a server that is simply unreachable is reported as needing authorization and its definition is promoted to auth: 'oauth'. Word-bound the auth keyword patterns, and let a known status code decide the classification before the message heuristic runs. The repository already locks that precedence for clean messages via the committed code=404 and 405 regressions; this extends it to messages that also contain auth-like text.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 077226d1f7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…auth Word-bounding the keyword patterns dropped OAuth error codes that embed them with underscores, such as unauthorized_client and invalid_token_hint, because underscore is a word character. Only the 401 numeral needs a boundary, so bound it against adjacent digits and leave the keywords as substring matches.
|
Codex review: needs maintainer review before merge. Reviewed August 2, 2026, 12:40 PM ET / 16:40 UTC. ClawSweeper reviewWhat this changesThe branch makes HTTP connection-error classification ignore embedded Merge readinessThis PR addresses a real current-main misclassification and is technically sound after the earlier OAuth keyword regression was fixed. It remains necessary because current Priority: P2 Review scores
Verification
How this fits togetherHTTP MCP server connection failures are classified before runtime transport decides whether to surface an offline/HTTP error or promote a server definition into the OAuth flow. The classifier’s auth result feeds flowchart LR
Error[HTTP server error] --> Classifier[Connection error classifier]
Classifier --> Status[Known status code]
Classifier --> Signals[Status-less message signals]
Status --> Outcome[Auth, HTTP, or offline result]
Signals --> Outcome
Outcome --> Runtime[Runtime transport handling]
Runtime --> OAuth[OAuth promotion or surfaced failure]
Decision needed
Why: The source proves the embedded-digit false-positive bug and the patch preserves keyword-based OAuth errors, but the final precedence for an inherently ambiguous status-less numeric token determines whether runtime launches OAuth recovery or surfaces a transport failure. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Confirm that numeric Do we have a high-confidence way to reproduce the issue? Yes—source-reproducible with high confidence: current main applies raw Is this the best way to solve the issue? Unclear pending maintainer intent: the implementation is the narrowest technical repair and retains status-less OAuth keyword handling, but only maintainers can choose the desired precedence for an otherwise ambiguous standalone numeric AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against e1689c3dec7c. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (9 earlier review cycles; latest 8 shown)
|
The digit-only lookarounds still matched 401 inside identifiers built from letters or underscores, so a status-less message such as "request abc401def failed" or "request_401_id failed" was classified as auth and could promote an HTTP server definition to OAuth. Widen the boundary to alphanumerics and underscore; the OAuth keyword codes stay substring matches so unauthorized_client and invalid_token_hint keep working.
|
Addressed the remaining finding in The lookarounds only excluded neighbouring digits, so a /(?<!\d)401(?!\d)/.test('request abc401def failed') // true
/(?<!\d)401(?!\d)/.test('request_401_id failed') // true
const AUTH_TOKEN_PATTERNS = [/(?<![0-9a-z_])401(?![0-9a-z_])/i, /unauthorized/, /invalid_token/, /forbidden/];Proof the new cases are load-bearingTwo regression cases cover the reported forms. Restoring only the previous pattern and rerunning fails exactly those two and nothing else, which also shows the tighter boundary does not reclassify any existing case: Verification on
|
A rejected token is often reported alongside transport wording, such as an expired-token payload whose description reads "Access token expired; connection timed out". Placing the offline patterns ahead of every auth signal therefore narrowed OAuth promotion well beyond the embedded-digit case this branch targets. Keyword signals (unauthorized, invalid_token, forbidden) are unambiguous, so they now run before the offline check. Only the bare 401 numeral, which is genuinely ambiguous in ports, durations, and request ids, stays subordinate to it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnrQ4wMJsicJUpQsarAHkw
|
Closed the P1 merge risk in code rather than asking for it to be accepted, in The flagged tradeoff was that ordering analyzeConnectionError(new Error('{"error":"invalid_token","error_description":"Access token expired; connection timed out"}'))
// before c3a43f4 -> { kind: 'offline' } // "timed out" wins, no OAuth promotion
analyzeConnectionError(new Error('SSE stream disconnected: Unauthorized, connection closed'))
// before c3a43f4 -> { kind: 'offline' } // "connection closed" winsNeither case involves an embedded digit, so neither is what this PR set out to fix. The splitKeywords are unambiguous auth signals; the bare const KEYWORD_AUTH_PATTERNS = [/unauthorized/, /invalid_token/, /forbidden/];
const NUMERIC_AUTH_PATTERNS = [/(?<![0-9a-z_])401(?![0-9a-z_])/i];So the compatibility change is now confined to messages whose only auth signal is a bare Proof the new cases are load-bearingThree regression cases were added. Run against the previous ordering, exactly two fail and nothing else moves: The third case ( Verification on
|
|
Clearing the runtime caveat from my previous comment: Re-ran the same commands on a clean tree at Same counts as the Node 22 run, so nothing was runtime-dependent. Two notes on top of that:
No new findings on my side. The only open item remains the maintainer decision on the bare- |
|
Landed as 223c512. Maintainer decision on the open precedence question: accepted as implemented. For a status-less message carrying both an offline phrase and a standalone numeric Verification at exact head
Changelog entry (maintainer-owned) follows on main. Thanks @Yigtwxx — exemplary review turnaround, every finding closed in code with before/after proof. |
…ifier 401 fix (#248) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
analyzeConnectionErrorclassifies any message containing the substring401as an auth failure. The check is not word-bounded, so digits that appear inside a port number, a duration, a hostname, or a request id are read as an auth signal. A server that is simply unreachable is then reported as needing authorization, and its definition is promoted toauth: 'oauth'— which hides the real fault the user has to fix.The same condition also lets the message heuristic override a status code that is already known exactly, so
code: 404with a message mentioningunauthorizedclassifies as auth rather than http.Root cause
src/error-classifier.ts:53containsAuthTokenmatched raw substrings ('401','unauthorized','invalid_token','forbidden'), and this auth branch ran before both thehttpbranch and theOFFLINE_PATTERNSbranch. Two consequences:401satisfies the auth branch, even when the same message also matchesOFFLINE_PATTERNS(econnrefused,timed out,getaddrinfo).statusCodecannot win, because the right side of the||is evaluated regardless of it.The intended precedence is already established in this repository.
tests/error-classifier.test.tslocks it with committed regressions —classifies code=404 as http (not auth),classifies code=500 as http,classifies HTTP 405 as transport/http instead of auth— and the history records two earlier fixes in the same direction (Fix 405 misclassified as auth error in error-classifier,fix: detect auth errors from error.code property). Those tests pass today only because their messages contain no auth-like text:'Not Found','Internal Server Error'. The existing offline test passes for the same reason — its port is9000.Why this reaches the OAuth flow
shouldAbortSseFallbackdoes not intercept these errors:isPostAuthConnectErrorandisOAuthFlowErrorboth test for a symbol marker (src/runtime/oauth.ts:81-91) that a raw fetch error does not carry. So the flow continues into the promotion branch:Fix
src/error-classifier.tsAUTH_TOKEN_PATTERNSreplaces the substring checks with word-bounded patterns, so401matches only as a standalone token andforbiddenno longer matches insideforbidden_tool.statusCodedecides first, thenOFFLINE_PATTERNS, then the keyword heuristic for messages that carry no status code.No special cases were added and no new dependency is introduced. Auth detection for messages without a status code is unchanged, and is now covered by tests.
Behavior proof
Measured through the real production helpers (
isUnauthorizedError->maybeEnableOAuth), with no mocks, no injected transports, and no network. The identical script was run againstsrc/error-classifier.tsatorigin/mainand at this branch. The first case is a control: it contains no auth-like digits and must stayofflineon both sides.The only difference between the control case and the second case is the port number.
Tests
Added to
tests/error-classifier.test.ts(10 cases, three groups):transport failures whose text embeds auth-like digits— refused connection on port14012, timeout of4010ms, hostnamemcp-4015.internal, and request id8401f2.known status codes take precedence over message keywords—code: 404with'Not Found: /unauthorized',code: 500with'Internal Server Error (request 401ab3)', and a parsed429with'rate limited for forbidden_tool'.auth detection without a status code is preserved— bare'Unauthorized', standalone'Server replied 401', and'invalid_token'still classify as auth.The new tests are load-bearing. Stashing only
src/error-classifier.tsand re-running the file on the unmodified classifier:The three
auth detection ... is preservedcases pass on both sides by design — they guard the behavior this change must not alter.Follow-up from review feedback
Codex flagged a real regression in the first revision (P2,
src/error-classifier.ts:16): word-bounding the keyword patterns dropped OAuth error codes that embed those keywords with underscores.\bunauthorized\bdoes not matchunauthorized_client— a standard RFC 6749 error code — because underscore is a word character, so a status-less OAuth failure would have fallen through tootherand skipped the auth path.Verified and fixed in
47adc37. Only the numeral needs a boundary, so401is bounded against adjacent digits while the keywords stay substring matches:This keeps the original finding fixed while leaving keyword behavior exactly as it is on
main. Addedit.eachcoverage forunauthorized_client,invalid_token_hint, andinvalid_tokenas status-less OAuth error codes; those three pass onmaintoo, so they lock the behavior this change must not alter.Verification
Run on Windows 11 with Node 24.15.0 and pnpm 10.33.2, at head
47adc37.pnpm exec vitest run tests/error-classifier.test.ts— 26 passed (26)pnpm check— passed (oxfmt --checkon 352 files,oxlint --type-aware --deny-warnings,tsc --noEmit)pnpm test— 808 passed, 48 skipped (856); 128 test files passed, 3 skippedCHANGELOG.mdis untouched, since it is release-owned for this repository's PR flow.One pre-existing unhandled error is reported by the full suite in
tests/runtime-integration.test.ts(aDOMException: ABORT_ERRraised fromStreamableHTTPClientTransport.closeduring teardown). It reproduces on unmodifiedmainat7259c8con this machine — baseline there was 795 passed with the same single error — so it is unrelated to this change. It is the reasonpnpm testexits non-zero locally in both runs.