fix(mcp): recover cleanly from OAuth failures#5595
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Service and APIs: Discovery failures are persisted in Client and queries: Settings UI: Shared helpers drive tool-count labels ( Reviewed by Cursor Bugbot for commit c0bd4e4. Bugbot is set up for automated code reviews on this repo. Configure here. |
Greptile SummaryThis PR improves MCP OAuth recovery and refresh feedback. The main changes are:
Confidence Score: 4/5The changed MCP failure paths need fixes before merging.
apps/sim/lib/mcp/service.ts and apps/sim/lib/mcp/client.ts
|
| Filename | Overview |
|---|---|
| apps/sim/lib/mcp/service.ts | Adds MCP status persistence and OAuth-pending handling, but the shared auth-error helper can hide non-OAuth 401 credential failures. |
| apps/sim/lib/mcp/client.ts | Adds connect timeout handling and structured diagnostics, but the new error logging can expose truncated secret fragments. |
| apps/sim/hooks/queries/mcp.ts | Stops auto-discovery for disconnected OAuth servers while keeping non-OAuth discovery behavior. |
| apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx | Uses helper functions for MCP refresh action state and server tool labels. |
| apps/sim/app/workspace/[workspaceId]/settings/components/settings-header/settings-header.tsx | Adds optional error-tone rendering for settings header actions. |
Reviews (1): Last reviewed commit: "fix(mcp): improve OAuth failure recovery" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function isOauthAuthorizationError(error: unknown): boolean { | ||
| return error instanceof McpOauthAuthorizationRequiredError || error instanceof UnauthorizedError | ||
| } |
There was a problem hiding this comment.
When a headers-auth MCP server returns 401, the SDK can surface UnauthorizedError, but this helper now routes that through the OAuth-pending path. That path clears lastError, skips the failure counter, and avoids the failure cache, so disconnected non-OAuth servers can be retried repeatedly while the UI has no persisted credential error to show.
| ...describedError, | ||
| message: sanitizeForLogging(describedError.message, 500), | ||
| causeChain: describedError.causeChain?.map((cause) => sanitizeForLogging(cause, 500)), | ||
| stack: | ||
| error instanceof Error && error.stack | ||
| ? sanitizeForLogging(error.stack.split('\n').slice(1).join('\n'), 2000) | ||
| : undefined, |
There was a problem hiding this comment.
These new diagnostics log external MCP error messages, cause chains, and stacks after sanitizeForLogging, but that helper truncates before redacting. If a server error echoes a long or unterminated secret-bearing fragment, the redaction patterns may not match the truncated text and a secret prefix can be written to structured logs.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c0bd4e4. Configure here.
| updatedAt: now, | ||
| }) | ||
| .where(eq(mcpServers.id, serverId)) | ||
| .returning() |
There was a problem hiding this comment.
Refresh masks discovery failures
Medium Severity
When discoverServerTools throws, the handler still builds the HTTP response from the post-refresh database row. If persistence was skipped (for example the new discoveryStartedAt guard or a failed status write) while the row still shows connected, the API returns a successful connected refresh with a cleared error even though discovery failed and tool sync was skipped.
Reviewed by Cursor Bugbot for commit c0bd4e4. Configure here.
1 participant
Summary
This PR makes MCP authentication and refresh failures recover cleanly instead of leaving Workspace Settings in repeated discovery loops or generic error states. Abandoned OAuth servers now wait for an explicit reconnect, failed refreshes expose a retryable Failed state, and persisted connection errors reach the list and detail views.
Dual-auth servers can now be added with a configured static
Authorization: Bearer ...header: the connection test tries that credential before optional OAuth discovery. Supplemental non-auth headers still follow OAuth discovery, preserving OAuth servers that need routing or metadata headers.Connection attempts honor the configured timeout, and diagnostics record timing, phase, outcome, auth type, and header names without header values. Connection-test failures now use allowlisted messages so an upstream server cannot reflect a configured credential into API responses or logs. OAuth-pending servers remain disconnected without negative caching; other failures promote to error after repeated attempts and reset after successful discovery.
Type of Change
Testing
git diff --checkpassed for the changed files.providers/meta/index.ts; the changed paths compile through the focused Vitest runs.Review focus: static Bearer precedence, preservation of OAuth-plus-supplemental-header behavior, and the allowlisted connection-error boundary.
Checklist
Post-Deploy Monitoring & Validation
McpClient,McpService,authorization_required,timeout, andOAuth authorization pendingduring the first 24 hours.Screenshots/Videos
Not applicable; this changes connection behavior and failure handling without altering layout.