fix: use SDK OAuth provider for remote MCP#19
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSwitches remote MCP transport auth from static bearer headers to an SDK-managed OAuth provider, preserves auth-remediation errors across downstream lifecycle methods, and adds tests for missing credentials, 403 classification, and sending stored tokens. ChangesOAuth Auth Provider Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
ian-pascoe
changed the title
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/downstream.ts`:
- Around line 282-303: The connect() method currently remaps all non-timeout
errors to "SERVER_UNAVAILABLE", which hides CapletsError("AUTH_REQUIRED") thrown
by oauthProvider(); update connect() to detect CapletsError with code
"AUTH_REQUIRED" (or error.code === "AUTH_REQUIRED") and rethrow it unchanged so
the original nextAction/login remediation remains available; for all other
non-timeout errors keep the existing remap to "SERVER_UNAVAILABLE". Ensure
references to oauthProvider, connect, CapletsError and the "SERVER_UNAVAILABLE"
remap are adjusted accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 438615b8-77ed-435b-8dff-504a81c67375
📒 Files selected for processing (3)
.changeset/tidy-otters-brush.mdsrc/downstream.tstest/downstream.test.ts
|
| Filename | Overview |
|---|---|
| src/downstream.ts | Core transport creation refactored: static OAuth headers replaced with FileOAuthProvider; isAuthRemediationError guards added to connect(), refreshTools(), and callTool() to surface AUTH_REQUIRED/AUTH_FAILED without wrapping. |
| test/downstream.test.ts | Three integration tests added covering missing-credential AUTH_REQUIRED, 403-to-AUTH_FAILED classification, and end-to-end Authorization header presence for stored OAuth tokens. |
| .changeset/tidy-otters-brush.md | Patch changeset entry accurately describing the OAuth provider migration. |
Sequence Diagram
sequenceDiagram
participant Caller
participant DownstreamManager
participant oauthProvider
participant FileOAuthProvider
participant SDKTransport
participant RemoteMCP
Caller->>DownstreamManager: listTools(server)
DownstreamManager->>DownstreamManager: connect(server)
DownstreamManager->>oauthProvider: oauthProvider(server)
oauthProvider->>oauthProvider: readTokenBundle(server)
alt No access token AND no refresh token
oauthProvider-->>Caller: throw CapletsError(AUTH_REQUIRED)
else Token present
oauthProvider->>FileOAuthProvider: new FileOAuthProvider(server, ...)
FileOAuthProvider-->>DownstreamManager: authProvider
DownstreamManager->>SDKTransport: "new StreamableHTTPClientTransport({authProvider, fetch: fetchWithOAuthAuthClassification})"
SDKTransport->>FileOAuthProvider: tokens()
FileOAuthProvider-->>SDKTransport: "{access_token, refresh_token, ...}"
SDKTransport->>RemoteMCP: POST /mcp (Authorization: Bearer token)
alt 200 OK
RemoteMCP-->>SDKTransport: initialize result
SDKTransport-->>DownstreamManager: connected
DownstreamManager->>RemoteMCP: tools/list
alt 200 OK
RemoteMCP-->>Caller: Tool[]
else 403 Forbidden
RemoteMCP-->>SDKTransport: 403
SDKTransport->>SDKTransport: fetchWithOAuthAuthClassification intercepts
SDKTransport-->>Caller: throw CapletsError(AUTH_FAILED)
end
else 401 Unauthorized
RemoteMCP-->>SDKTransport: 401
SDKTransport->>FileOAuthProvider: redirectToAuthorization(url)
FileOAuthProvider-->>Caller: throw CapletsError(AUTH_REQUIRED)
end
end
Reviews (3): Last reviewed commit: "fix: match OAuth redirect callback signa..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/downstream.ts`:
- Around line 305-329: In oauthProvider adjust the onRedirect callback passed
into the FileOAuthProvider constructor so it matches the expected signature
(url: URL) => void; replace the current no-arg lambda with one that accepts a
URL parameter (e.g., (url: URL) => { throw new CapletsError(... ) }) so
FileOAuthProvider.redirectToAuthorization can invoke it without a type/signature
mismatch; keep the thrown CapletsError payload the same and continue to use
this.options.authDir and server values as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4bef875f-a2d3-4060-a7d2-f530d1283008
📒 Files selected for processing (2)
src/downstream.tstest/downstream.test.ts
1 participant
Summary
Root Cause
Caplets was manually reading stored OAuth tokens and injecting a static
Authorizationheader into remote MCP transports. That bypassed the MCP SDK's OAuth handling for refresh, resource metadata, and auth challenge/upscope flows.Validation
pnpm vitest run test/downstream.test.ts test/auth.test.tspnpm typecheckpnpm format:checkpnpm lintpnpm schema:checkpnpm buildpnpm testpnpm format:check && pnpm lint && pnpm typecheck && pnpm schema:check && pnpm test && pnpm buildSummary by CodeRabbit
New Features
Bug Fixes
Tests