feat(copilot): verify managed MCP connections#5596
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview After add (unless
Reviewed by Cursor Bugbot for commit 256e22c. Bugbot is set up for automated code reviews on this repo. Configure here. |
Greptile SummaryThis PR adds live MCP server verification to managed MCP add and edit flows. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (1): Last reviewed commit: "feat(copilot): verify managed MCP connec..." | Re-trigger Greptile |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
j15z
force-pushed
the
feat/mothership-mcp-verification
branch
from
e7abac4 to
c51b45b
Compare
- persist failed verification status before returning - classify OAuth redirect failures as authorization-required - cover status persistence and immediate OAuth retry
- use the shared OAuth-required classifier in aggregate discovery - keep server summaries consistent for redirect-required auth - cover both sibling paths with focused regressions
| const quotaCheck = await checkStorageQuotaForBillingContext( | ||
| storageBillingContext, | ||
| fileSize ?? 0 | ||
| ) |
There was a problem hiding this comment.
Mothership multipart still billed
Medium Severity
Multipart initiate now resolves resolveStorageBillingContext and enforces checkStorageQuotaForBillingContext for every non-exempt context, including mothership. Presigned mothership uploads in this same change set are explicitly unbilled and skip payer storage accounting, so large mothership attachments can unexpectedly consume workspace storage quota while small ones do not.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4b9857f. Configure here.
There was a problem hiding this comment.
Large mothership attachments can unexpectedly consume workspace storage quota while small ones do not.
Needs human: this multipart file appears in the PR only because the head diverges from the force-updated dev history (68 base-only and 62 head-only commits). A normal base merge produced 15 semantic conflicts across unrelated billing, uploads, UI, and Copilot code, so I aborted it rather than change storage behavior outside this MCP PR. Options are to rebase/cherry-pick the three MCP fix commits onto current dev and force-push (recommended), which should remove this upload diff, or address the upload issue separately on dev. Leaving this thread open until the branch history is repaired.
- persist failed verification status before returning - classify OAuth redirect failures as authorization-required - cover status persistence and immediate OAuth retry
- use the shared OAuth-required classifier in aggregate discovery - keep server summaries consistent for redirect-required auth - cover both sibling paths with focused regressions
j15z
force-pushed
the
feat/mothership-mcp-verification
branch
from
4b9857f to
256e22c
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 256e22c. Configure here.
| error: message, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Failed verify poisons discovery cache
Medium Severity
verifyServerConnection probes via discoverServerTools, whose failure path always calls markServerUnhealthy for non-auth errors. A failed add/edit verification therefore writes the 2-minute negative cache even though the failure is already returned inline. Fixing credentials outside an MCP edit (for example adding a missing env var) leaves discovery in cooldown until TTL expiry.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 256e22c. Configure here.
| >, | ||
| oauthCredentialsChanged = false | ||
| ): boolean { | ||
| return ( |
There was a problem hiding this comment.
@Sg312 Does this seem like the correct way to be checking if oauth creds changed?
1 participant
Summary
Mothership can now distinguish “the MCP server record was saved” from “the server is actually usable.” MCP add operations and connection-affecting edits perform a live
tools/listverification and return a structured result to the agent, including tool count, authorization requirements, or a concise failure reason.A failed verification does not delete the newly created server, so the agent can report the credential or connection problem without losing the user’s configuration. Disabled servers and cosmetic-only edits skip the network handshake explicitly instead of producing a false failure or waiting for a timeout.
Validation
manage_mcp_toolsuites passed (18 tests).git diff --checkpassed for the changed files.node_modules, producing unrelated package-version errors. The changed TypeScript paths compiled through the focused Vitest runs.Post-Deploy Monitoring & Validation
verificationobject and compare it with subsequent workflow MCP usage.verified: truewith a tool count; authorization and transport failures are reported explicitly without deleting the server.