Skip to content

fix(mcp): keep a legacy server row from blanking the whole server list#5762

Merged
waleedlatif1 merged 2 commits into
stagingfrom
fix/mcp-transport-contract-resilience
Jul 19, 2026
Merged

fix(mcp): keep a legacy server row from blanking the whole server list#5762
waleedlatif1 merged 2 commits into
stagingfrom
fix/mcp-transport-contract-resilience

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

  • The MCP server-list response validated transport/authType/connectionStatus with strict enums, but those are free-text DB columns that can hold legacy values (e.g. transport http/sse from before the streamable-http consolidation). Because the client strict-parses the whole z.array(...) with retry:false, one off-enum row blanked the entire workspace's MCP list ("Response failed contract validation").
  • Found live in prod: 13 active rows (sse/http) across 11 workspaces. Same class as fix(mcp): coerce corrupted consecutiveFailures instead of crashing the whole server list #5593, which only covered consecutiveFailures.
  • Fix: response-side .catch() tolerance on all three strict-enum-over-free-text columns (request bodies stay strict) — transportstreamable-http, authType/connectionStatusundefined. Uses Zod's built-in .catch(), matching the existing pattern in contracts/admin.ts.
  • Stop the workspace-fork copy from propagating legacy transport values (the still-active vector that regenerated bad rows).
  • Bundled two correctness fixes found in the same MCP audit: create/upsert path now resets connection status on a headers→OAuth flip (mirrors the update path, kills a false "connected" state); bulk discovery now drops the positive tool cache on OAuth-pending (mirrors the single-server path, no stale tools after re-auth).
  • Data backfill of the 13 legacy rows handled separately.

Type of Change

  • Bug fix

Testing

  • 149 tests pass across lib/api/contracts, app/api/mcp/servers, lib/mcp, and ee/workspace-forking; typecheck, Biome, and check:api-validation all clean.
  • Runtime-verified: bad response values coerce (no throw), valid values pass through, and create/test request bodies still reject invalid transport.

Follow-up (not in this PR)

  • Optional durable hardening: per-row isolation at the list route (drop+log a malformed row) as a backstop for any future strict field. The three .catch()es close the entire present class, so this is future-proofing only.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

The server-list response validated transport/authType/connectionStatus with
strict enums, but those columns are free text and can hold legacy values
(e.g. transport 'http'/'sse' from before the streamable-http consolidation,
including rows copied verbatim by workspace fork). Since the client strict-
parses the whole array with retry:false, a single off-enum row blanked the
entire workspace's MCP list.

- Tolerate off-enum values in the response via Zod .catch() (request bodies
  stay strict): transport -> streamable-http, authType/connectionStatus -> undefined
- Stop the workspace-fork copy from propagating legacy transport values
- Reset connection status on a headers->OAuth flip in the create/upsert path,
  mirroring the update path (kills a false "connected" state)
- Drop the positive tool cache on OAuth-pending in bulk discovery, matching
  the single-server path (no stale tools after re-auth is required)
@vercel

vercel Bot commented Jul 19, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Jul 19, 2026 9:20am

Request Review

@cursor

cursor Bot commented Jul 19, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes MCP list response shaping, fork copy defaults, and OAuth/auth-type upsert plus discovery cache invalidation; request validation stays strict, but incorrect coercion could mask data issues or briefly affect connection UI state.

Overview
Fixes a production bug where one MCP server row with legacy or invalid enum values in free-text DB columns caused the entire workspace MCP server list API to fail contract validation and render empty.

Response validation now tolerates bad stored values on list/read paths only (create/update/test bodies stay strict): transport coerces to streamable-http, and off-enum authType / connectionStatus become undefined via Zod .catch(), matching the pattern used elsewhere in admin contracts.

Workspace fork copies MCP servers with transport forced to streamable-http so forks no longer propagate legacy http/sse rows.

Also aligns two MCP lifecycle edge cases with existing update/single-server behavior: create/upsert resets connection state and revokes OAuth when auth type flips (e.g. OAuth → headers), and bulk tool discovery deletes the positive tool cache when a server hits OAuth-pending so force-refresh cannot serve stale tools.

Reviewed by Cursor Bugbot for commit 40a5552. Configure here.

Comment thread apps/sim/lib/mcp/orchestration/server-lifecycle.ts Outdated
@greptile-apps

greptile-apps Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a production outage where a single MCP server row with a legacy transport, authType, or connectionStatus value would fail strict Zod enum parsing and blank the entire workspace's MCP server list. The fix applies response-side .catch() coercion on the three offending fields while keeping request-side schemas strict.

  • Contract tolerance (mcp.ts): transport falls back to 'streamable-http', authType and connectionStatus fall back to undefined on off-enum DB values, matching the .catch() pattern already used in contracts/admin.ts.
  • Auth-type flip reset (server-lifecycle.ts): the create/upsert path now detects authTypeChanged and oauthDisabled, resetting connection state and clearing stale errors on an auth-type switch — mirroring the existing performUpdateMcpServer logic.
  • Bulk OAuth-pending cache eviction (service.ts): drops the positive tool cache when markServerOauthPending actually applies the DB update (guarded by statusApplied), preventing stale tools from being served after re-auth is required.
  • Fork normalization (copy-resources.ts): always writes transport: 'streamable-http' into forked rows, closing the propagation vector for new legacy values.

Confidence Score: 5/5

Safe to merge — changes are narrowly scoped Zod coercions and mirrored logic from already-tested update paths, with 149 passing tests covering the new branches.

All three schema tolerances are additive (request schemas stay strict), the auth-type reset logic mirrors the already-correct update path, and the OAuth-pending cache eviction follows the identical guard used by the single-server failure path.

No files require special attention.

Important Files Changed

Filename Overview
apps/sim/lib/api/contracts/mcp.ts Adds .catch() tolerance on transport, authType, and connectionStatus response schemas so off-enum DB values coerce gracefully instead of failing the whole server-list parse. Request-side schemas remain strict.
apps/sim/lib/mcp/orchestration/server-lifecycle.ts Introduces authTypeChanged and oauthDisabled flags in the create/upsert path. An auth-type flip now resets connectionStatus to disconnected and clears lastConnected/lastError, mirroring the existing update path.
apps/sim/lib/mcp/service.ts Bulk discovery oauth-pending branch now drops the positive tool cache when markServerOauthPending applies the DB update (statusApplied guard), preventing stale tools from being served after re-auth is required.
apps/sim/ee/workspace-forking/lib/copy/copy-resources.ts Fork path now explicitly sets transport to 'streamable-http', closing the propagation vector for new legacy transport values.
apps/sim/lib/mcp/orchestration/server-lifecycle.test.ts Adds a test covering the create/upsert OAuth→headers flip, verifying connectionStatus is reset to disconnected and OAuth tokens are revoked.

Reviews (2): Last reviewed commit: "fix(mcp): reset connection status on any..." | Re-trigger Greptile

…/upsert path

The non-OAuth branch optimistically marked the server connected on an
oauth->headers flip, leaving a stale lastError and diverging from
performUpdateMcpServer. Mirror the update path exactly: reset to disconnected
and clear lastError on an auth-type flip or OAuth URL/creds change; only mark
connected for a non-OAuth (re-)registration with unchanged auth. Adds a
create-path regression test.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 40a5552. Configure here.

@waleedlatif1
waleedlatif1 merged commit e337308 into staging Jul 19, 2026
20 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/mcp-transport-contract-resilience branch July 19, 2026 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant