-
Notifications
You must be signed in to change notification settings - Fork 3.8k
feat(cli): continue oauth for cli MCPs #8758
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
✅ Review Complete Code Review Summary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 6 files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 issues found across 4 files (reviewed changes from recent commits).
Prompt for AI agents (all 3 issues)
Understand the root cause of the following 3 issues and fix them.
<file name="extensions/cli/src/configLoader.ts">
<violation number="1" location="extensions/cli/src/configLoader.ts:211">
Treating CLI `--config` errors like user-assistant failures now masks problems with explicitly requested configs by falling back to the default agent, violating the documented precedence where the CLI flag should fail loudly if unreadable.</violation>
</file>
<file name="packages/config-yaml/src/markdown/agentFiles.ts">
<violation number="1" location="packages/config-yaml/src/markdown/agentFiles.ts:133">
Port detection in the URL branch only accepts bare numbers, so valid URLs such as https://host:4000/path (port plus path) are rejected, preventing configuration of those MCP servers.</violation>
</file>
<file name="extensions/cli/src/services/MCPService.ts">
<violation number="1" location="extensions/cli/src/services/MCPService.ts:563">
Catching the HTTP fallback error inside getConnectedClient hides 401/405 responses from withTokenRefresh, so HTTP-only servers with expired tokens never trigger the OAuth refresh and remain unauthenticated.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 19 files (reviewed changes from recent commits).
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="extensions/cli/src/services/MCPService.ts">
<violation number="1" location="extensions/cli/src/services/MCPService.ts:477">
Rethrowing every `isAuthError` before the HTTP→SSE fallback means 405 responses (which `isAuthError` classifies as auth issues) never reach the SSE transport. SSE-only servers with no explicit `type` now fail to connect unless the user is signed in, because `withTokenRefresh` rethrows when `isAuthenticated()` is false, preventing the SSE fallback from running. Allow the SSE fallback to run for transport capability errors such as 405 so these servers remain reachable.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| const transport = this.constructHttpTransport(serverConfig); | ||
| await client.connect(transport, {}); | ||
| } catch (e) { | ||
| if (isAuthError(e)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rethrowing every isAuthError before the HTTP→SSE fallback means 405 responses (which isAuthError classifies as auth issues) never reach the SSE transport. SSE-only servers with no explicit type now fail to connect unless the user is signed in, because withTokenRefresh rethrows when isAuthenticated() is false, preventing the SSE fallback from running. Allow the SSE fallback to run for transport capability errors such as 405 so these servers remain reachable.
Prompt for AI agents
Address the following comment on extensions/cli/src/services/MCPService.ts at line 477:
<comment>Rethrowing every `isAuthError` before the HTTP→SSE fallback means 405 responses (which `isAuthError` classifies as auth issues) never reach the SSE transport. SSE-only servers with no explicit `type` now fail to connect unless the user is signed in, because `withTokenRefresh` rethrows when `isAuthenticated()` is false, preventing the SSE fallback from running. Allow the SSE fallback to run for transport capability errors such as 405 so these servers remain reachable.</comment>
<file context>
@@ -561,13 +474,9 @@ export class MCPService
- error: getErrorString(e),
- },
- );
+ if (isAuthError(e)) {
+ throw e;
+ }
</file context>
✅ Addressed in 317f6f4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should work unless 405 is what a server throws when it tries to do http and it should be sse. But this is a bit of an edge case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but did this to fix case where someone throws 405 when auth is missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
…allin/mcp-oauth-cli
…allin/mcp-oauth-cli
…allin/mcp-oauth-cli
|
🎉 This PR is included in version 1.34.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.30.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Adds a 401-detection wrapper to MCP service which attempts to get oauth token from hub if signed in
Summary by cubic
Adds automatic OAuth token refresh for CLI MCP HTTP/SSE connections. On 401/405, we fetch a new token from the hub and retry; agent files now support URL-based MCP references.
New Features
Bug Fixes
Written for commit 26bb215. Summary will update automatically on new commits.