This repository was archived by the owner on Jul 8, 2026. It is now read-only.
sc-308975: fix MCP transport error during Codex startup - #162
Merged
mdthorpe-sc merged 2 commits intoMar 2, 2026
Merged
Conversation
📝 WalkthroughWalkthroughAdds OAuth bearer-token verification to the auth provider with a fallback to legacy Shortcut API token verification, updates token exchange/refresh normalization and error mapping, introduces comprehensive tests for verifier and token flows, and threads a sessionToken into session/transport creation in the HTTP server flow. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Provider as OAuth Provider
participant AuthServer as Auth Server
participant ShortcutAPI as Shortcut API
participant ErrorHandler
Client->>Provider: verifyAccessToken(token)
rect rgba(100,200,150,0.5)
Note over Provider,AuthServer: Primary OAuth Path (Bearer)
Provider->>AuthServer: Validate bearer token / get member
alt OAuth Success
AuthServer-->>Provider: Member data
Provider-->>Client: AuthInfo (memberId, mentionName)
else OAuth Fails
AuthServer-->>Provider: OAuth error
end
end
rect rgba(200,150,100,0.5)
Note over Provider,ShortcutAPI: Fallback Legacy Path (Shortcut-Token)
Provider->>ShortcutAPI: getCurrentMemberInfo(legacy token)
alt Legacy Success
ShortcutAPI-->>Provider: Member data
Provider-->>Client: AuthInfo
else Legacy Fails
ShortcutAPI-->>Provider: Error
Provider->>ErrorHandler: Throw InvalidTokenError / mapped error
ErrorHandler-->>Client: Error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/auth/provider.test.ts (1)
94-131: Consider asserting header shape per call in fallback/failure tests.You already assert call count; adding per-call header assertions would make regressions more diagnosable.
Optional test hardening
expect(authInfo.extra).toEqual({ memberId: "member-2", mentionName: "legacy-user", }); expect(mockState.calls.length).toBe(2); + expect(getHeader(mockState.calls[0], "Authorization")).toBe("Bearer legacy-token"); + expect(getHeader(mockState.calls[0], "Shortcut-Token")).toBeUndefined(); + expect(getHeader(mockState.calls[1], "Shortcut-Token")).toBe("legacy-token"); }); @@ await expect(provider.verifyAccessToken("invalid-token")).rejects.toBeInstanceOf( InvalidTokenError, ); expect(mockState.calls.length).toBe(2); + expect(getHeader(mockState.calls[0], "Authorization")).toBe("Bearer invalid-token"); + expect(getHeader(mockState.calls[1], "Shortcut-Token")).toBe("invalid-token"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auth/provider.test.ts` around lines 94 - 131, The tests should assert the exact headers sent on each mock call to make failures clearer: update the fallback test (using mockState.handler/getHeader and createOAuthProvider().verifyAccessToken) to assert that mockState.calls[0] contains an Authorization header with value "Bearer legacy-token" and mockState.calls[1] contains a Shortcut-Token header with value "legacy-token"; likewise update the failure test to assert mockState.calls[0] had Authorization "Bearer invalid-token" and mockState.calls[1] had Shortcut-Token "invalid-token" (use the same mockState.calls array and getHeader helper to inspect each call) so per-call header shape is verified.src/auth/provider.ts (1)
152-160: Useconfig.headersinstead of mutating the axios instance to remove theShortcut-Tokenheader.The current approach accesses private axios internals via type-casting, which is brittle. Since
@shortcut/clientaccepts aconfig.headersparameter that overrides the default headers, pass"Shortcut-Token": undefined(or use an alternative initialization pattern) to prevent the header from being set, rather than deleting it from the instance after construction.Current code (lines 152–160)
// Remove Shortcut-Token header so Shortcut API only sees Bearer auth. // biome-ignore lint/suspicious/noExplicitAny: accessing axios internals const instance = (client as any).instance; if (instance?.defaults?.headers) { delete instance.defaults.headers["Shortcut-Token"]; if (instance.defaults.headers.common) { delete instance.defaults.headers.common["Shortcut-Token"]; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auth/provider.ts` around lines 152 - 160, The code mutates axios internals on the created client to remove the "Shortcut-Token" header; instead remove this block and initialize or reconstruct the Shortcut client using its config.headers override so the header is never set (e.g., when calling the `@shortcut/client` constructor or factory that returns `client`, pass a config.headers object with "Shortcut-Token": undefined or simply omit that key). Locate the block referencing `client`/`instance` and `delete instance.defaults.headers["Shortcut-Token"]` and replace the approach by supplying `config.headers` on client creation (or re-create the client with config.headers) rather than deleting headers from the axios `instance`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/auth/provider.ts`:
- Around line 91-114: The current bare catch around the OAuth verification (the
try that calls createOAuthVerificationClient and
oauthClient.getCurrentMemberInfo) swallows non-auth failures; change it to only
fall back for explicit auth-denial errors (e.g., InvalidTokenError or HTTP
401/403 responses) and rethrow or propagate other errors (network, 5xx,
timeouts). Inspect the thrown error from oauthClient.getCurrentMemberInfo
(status/code/message) and only continue to legacy verification when it
represents an unauthorized/forbidden token; otherwise rethrow the error so
upstream/transport failures are not misclassified; apply the same tightened
catch logic to the second OAuth try/catch block handling lines ~136-141
referencing the same functions.
---
Nitpick comments:
In `@src/auth/provider.test.ts`:
- Around line 94-131: The tests should assert the exact headers sent on each
mock call to make failures clearer: update the fallback test (using
mockState.handler/getHeader and createOAuthProvider().verifyAccessToken) to
assert that mockState.calls[0] contains an Authorization header with value
"Bearer legacy-token" and mockState.calls[1] contains a Shortcut-Token header
with value "legacy-token"; likewise update the failure test to assert
mockState.calls[0] had Authorization "Bearer invalid-token" and
mockState.calls[1] had Shortcut-Token "invalid-token" (use the same
mockState.calls array and getHeader helper to inspect each call) so per-call
header shape is verified.
In `@src/auth/provider.ts`:
- Around line 152-160: The code mutates axios internals on the created client to
remove the "Shortcut-Token" header; instead remove this block and initialize or
reconstruct the Shortcut client using its config.headers override so the header
is never set (e.g., when calling the `@shortcut/client` constructor or factory
that returns `client`, pass a config.headers object with "Shortcut-Token":
undefined or simply omit that key). Locate the block referencing
`client`/`instance` and `delete instance.defaults.headers["Shortcut-Token"]` and
replace the approach by supplying `config.headers` on client creation (or
re-create the client with config.headers) rather than deleting headers from the
axios `instance`.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/auth/provider.ts (1)
157-159:⚠️ Potential issue | 🟠 MajorOnly fallback on auth-denial failures in bearer verification.
Line 157 catches all errors and always falls back to legacy verification. This can misclassify upstream/transport failures as invalid credentials. Restrict fallback to explicit auth failures and rethrow everything else.
Suggested tightening
- } catch { - // Fall through to legacy token verification below. + } catch (error) { + const status = (error as { response?: { status?: number } }).response?.status; + const isAuthFailure = + error instanceof InvalidTokenError || status === 401 || status === 403; + if (!isAuthFailure) { + throw error; + } + // Fall through to legacy token verification below. }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auth/provider.ts` around lines 157 - 159, The current bare catch after the bearer-token verification swallows all errors and always falls back to legacy verification; modify the try/catch around the bearer verification (the block that currently ends with "// Fall through to legacy token verification below.") to only swallow explicit authentication-denial errors (e.g., an AuthError/UnauthorizedError or errors with status 401/invalid-credentials markers) and rethrow any other exceptions (network/transport/internal errors) so they are not misclassified; keep the fallback to legacy verification only for genuine auth-denial conditions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/auth/provider.ts`:
- Around line 157-159: The current bare catch after the bearer-token
verification swallows all errors and always falls back to legacy verification;
modify the try/catch around the bearer verification (the block that currently
ends with "// Fall through to legacy token verification below.") to only swallow
explicit authentication-denial errors (e.g., an AuthError/UnauthorizedError or
errors with status 401/invalid-credentials markers) and rethrow any other
exceptions (network/transport/internal errors) so they are not misclassified;
keep the fallback to legacy verification only for genuine auth-denial
conditions.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/auth/oauth.test.tssrc/auth/provider.test.tssrc/auth/provider.tssrc/server-http.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/auth/provider.test.ts
This was referenced Mar 2, 2026
This was referenced Mar 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Problem
Codex startup against the hosted Shortcut MCP server can fail with a transport handshake error when bearer-token verification relies only on in-memory issued-token cache across restarts / multi-instance routing.
Verification
Summary by CodeRabbit
New Features
Tests