iframe-proxy

waleedlatif1 · 2026-10-21T02:48:37Z

Summary

Follow-up to #9263 (discover every workspace MCP server without tagging) and #9262 (BM25 ranking), fixing four audit findings.

Best-effort discovery wrote shared state. A server's status, failure count, cooldown and OAuth row are shared by the workspace, but credentials resolve per requester. Broad catalog calls now reach every usable server, so a member without a personal {{VAR}} made discovery fail. That stamped the server failing and set a 120s workspace-wide cooldown, which broke another member's tagged or targeted discovery and call re-resolution (McpServerCooldownError). For an OAuth server nobody connected, it inserted an empty OAuth row and marked the server disconnected for everyone.
- McpService.discoverServerTools has a bestEffort mode, used only for untagged broad discovery (through discoverMcpServerToolsUseCase from buildMcpToolSchemas). It only reads shared health state. On failure it writes no status, failure count, cooldown or cache eviction. On success it only warms the tool cache. It has its own in-flight key.
- Before reading the cache or connecting, it skips a server the requester can't authenticate to: a referenced variable they have no value for (new findMissingMcpConfigEnvVars, which records no secret usage), or an OAuth server with no authorized tokens (loadOauthRow, which never creates a row). So another member's cached tools are never served to a requester who lacks the credentials to call them.
- Tagged and targeted discovery keep today's health bookkeeping.
listUsableMcpServerIds errors failed the whole catalog, built-ins included, which undid fix(mothership): keep integration discovery working when a tagged mcp server fails or lists no tools #9260. Broad requests now log the error and keep built-ins and tagged servers. Targeted MCP requests (service: mcp:<id> / exact MCP tool id) still fail with the error.
BM25 recall regression. A query with no content terms after tokenization (all, any, what can I use, a non-ASCII query) returned total: 0, so the agent could conclude there are no integrations. It now returns the unranked compact listing, as an empty query does. Exact-ID ranking is unchanged.
A tagged server that's no longer usable (disabled, deleted, Credential Group, other workspace) was dropped from broad requests without a log. It's logged again.

Type of Change

Testing

Regression tests. With the source changes reverted and only the new tests kept, 9 of them fail:

lib/mcp/service.test.ts › McpService best-effort discovery:
- a failing best-effort discovery leaves status and cooldown alone, so another member's targeted discovery still connects
- a requester missing a variable is skipped without connecting, even when another member left the tools cached, and that member is still served
- an unauthorized OAuth server is skipped without getOrCreateOauthRow or a pending stamp
- a successful best-effort discovery warms the cache without stamping status
lib/mcp/resolve-config.test.ts › findMissingMcpConfigEnvVars
catalog.test.ts:
- lists every operation, compactly, for a query with no content terms
- keeps built-ins and tagged servers when the workspace server list fails
- the untagged-discovery wiring tests now assert bestEffort: true

With the fix, all 8 touched or adjacent suites pass: 128 tests across service, service-pool, resolve-config, use-cases, execute-tool, mcp-tools, catalog and the catalog route. bun run type-check in apps/sim is clean.

Reviewers: the OAuth row is one per server (loadOauthRow keys on mcpServerId), so "can authenticate" for OAuth means that row holds tokens.

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)

… catalog resilient Broad agent and plan catalog queries now discover every usable workspace MCP server (#9263), but a server's status, failure count, cooldown and OAuth row are shared by the workspace while its credentials resolve per requester. A member without a personal {{VAR}} the server references made discovery fail, which stamped the server failing and put it in a workspace-wide cooldown, so another member's tagged or targeted discovery and call re-resolution failed with McpServerCooldownError. For an OAuth server the requester never connected, discovery inserted an empty OAuth row and marked the server disconnected for everyone. - Best-effort (untagged, broad) discovery is a McpService mode that only reads shared health state: on failure it writes no status, failure count, cooldown or cache eviction, and on success only warms the tool cache. - Before reading the cache or connecting, it skips a server the requester cannot authenticate to: a referenced variable they have no value for (findMissingMcpConfigEnvVars, which records no secret usage), or an OAuth server with no authorized tokens (read, never created). Another member's cached tools are not served to them. - Tagged and targeted discovery keep today's health bookkeeping. - A failure listing the workspace's servers no longer fails the whole catalog: broad requests log it and keep built-ins and tagged servers; targeted MCP requests still fail with it. - A query with no content terms after tokenization (stop words only, or a script the tokenizer does not split) returns the unranked listing, as an empty query does, instead of total 0. Exact-ID ranking is unchanged. - A tagged server that is no longer usable is logged when dropped.

vercel · 2026-10-21T02:48:43Z

cubic-dev-ai

2 issues found across 8 files

Confidence score: 3/5

In service.ts, loadOauthRow clears and updates the shared OAuth row when decryption fails, so broad best-effort discovery can mutate OAuth state despite being documented as side-effect-free. Keep this path read-only on decrypt failure.
In resolve-config.ts, decrypt failures become "", but the availability check only rejects undefined, so discovery can attempt a connection with unusable credentials. Treat failed decryption as unavailable.

Prompt for AI agents (unresolved issues)


Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/sim/lib/mcp/service.ts">

<violation number="1" location="apps/sim/lib/mcp/service.ts:1329">
P2: This best-effort path is documented as side-effect-free, but `loadOauthRow` clears and updates the shared OAuth row when token decryption fails. A key-rotation or corrupted-token encounter during broad discovery can therefore erase the stored grant and force reauthorization for every workspace member; use a non-mutating token-presence read for this gate.</violation>
</file>

<file name="apps/sim/lib/mcp/resolve-config.ts">

<violation number="1" location="apps/sim/lib/mcp/resolve-config.ts:168">
P2: A failed secret decryption is represented as an empty string, but this helper treats that key as available because the resolver only checks for `undefined`. Best-effort discovery can therefore connect with an unusable credential instead of skipping the server; exclude failed effective keys before resolving.</violation>
</file>

_{Reply with feedback, questions, or to request a fix.

Turn on auto-fix | Re-trigger cubic}

greptile-apps · 2026-10-21T02:52:58Z

Confidence Score: 5/5

[Medium impact] The changes appear safe to merge.

Summary

This PR keeps broad MCP discovery from changing shared server health and makes catalog results more resilient.

Broad MCP discovery no longer disturbs shared server state.
Broad catalogs keep built-in tools when MCP server listing fails.
Searches with no content words show the compact integration list.

The earlier empty-value finding is fixed. No new actionable issues were found in the changes since the previous review.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Catalog request] --> B{Server explicitly requested?}
  B -->|Yes| C[Tracked discovery]
  B -->|No| D[Best-effort discovery]
  D --> E{Requester has credentials?}
  E -->|No| F[Skip server]
  E -->|Yes| G[Read cache or connect]
  G --> H[Keep shared health unchanged]
  G --> I[Preserve OAuth grant]
  C --> J[Keep existing health bookkeeping]

_{Reviews (3) · Last reviewed commit: "fix(mothership): keep the shared mcp oau..." · Reviewed by Greptile}

…redential reads The best-effort gate read the OAuth row with loadOauthRow, which clears a column it cannot decrypt, so a broad catalog query after a key rotation erased the one grant every member shares. hasDecryptableOauthTokens reads the tokens column and tries to decrypt it without writing anything. findMissingMcpConfigEnvVars took a variable that failed to decrypt (which the environment snapshot reports as '') as present, so best-effort discovery connected with an empty credential instead of skipping. An empty effective value now counts as missing.

waleedlatif1 · 2026-10-21T03:03:06Z

@greptile

waleedlatif1 · 2026-10-21T03:03:07Z

@cubic-dev-ai review this PR

cubic-dev-ai · 2026-10-21T03:03:13Z

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

cubic-dev-ai

All reported issues were addressed across 12 files

_{Reply with feedback, questions, or to request a fix.

Turn on auto-fix | Re-trigger cubic}

…t refresh failures Best-effort discovery could still run the SDK's OAuth recovery while listing tools. A rejected refresh made it clear the tokens (and, for a rejected client, the registration) and start a new authorization flow, which disconnected the shared grant for every member. Its OAuth clients now use a SimMcpOauthProvider with preserveGrant, which refuses to invalidate credentials, register a client or start authorization before any write, so a failed refresh skips the server. A successful refresh still persists. It keeps the grant valid for everyone and must be saved where the server rotates refresh tokens. These clients are pooled apart from tracked ones. findMissingMcpConfigEnvVars counted every empty value as missing, but an empty variable is valid. Only a key the snapshot names in decryptionFailures whose effective value is empty now counts as missing.

waleedlatif1 · 2026-10-21T03:17:42Z

@greptile

waleedlatif1 · 2026-10-21T03:17:44Z

@cubic-dev-ai review this PR

cubic-dev-ai · 2026-10-21T03:17:49Z

cubic-dev-ai

No issues found across 14 files

Confidence score: 5/5

Automated review surfaced no issues in the provided summaries.
No files require special attention.

_{Turn on auto-fix | Re-trigger cubic}

cubic-dev-ai Bot reviewed Oct 21, 2026

View reviewed changes

Comment thread apps/sim/lib/mcp/service.ts Outdated

Comment thread apps/sim/lib/mcp/resolve-config.ts Outdated

vercel Bot temporarily deployed to Preview October 21, 2026 03:02 Inactive

greptile-apps Bot reviewed Oct 21, 2026

View reviewed changes

Comment thread apps/sim/lib/mcp/resolve-config.ts

cubic-dev-ai Bot reviewed Oct 21, 2026

View reviewed changes

Comment thread apps/sim/lib/mcp/service.ts

vercel Bot temporarily deployed to Preview October 21, 2026 03:17 Inactive

cubic-dev-ai Bot reviewed Oct 21, 2026

View reviewed changes

waleedlatif1 merged commit 96ab7c8 into staging Oct 21, 2026
54 checks passed

waleedlatif1 mentioned this pull request Oct 21, 2026

v0.9.30: mship improvements, mcp tool and skill discovery, docs refinements #9270

Merged

waleedlatif1 deleted the fix/untagged-mcp-discovery-side-effects branch October 21, 2026 06:52

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

waleedlatif1 commented Oct 21, 2026

Summary

Type of Change

Testing

Checklist

Uh oh!

vercel Bot commented Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

greptile-apps Bot commented Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 5/5

Summary

Diagram

Uh oh!

waleedlatif1 commented Oct 21, 2026

Uh oh!

waleedlatif1 commented Oct 21, 2026

Uh oh!

cubic-dev-ai Bot commented Oct 21, 2026

Uh oh!

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

waleedlatif1 commented Oct 21, 2026

Uh oh!

waleedlatif1 commented Oct 21, 2026

Uh oh!

cubic-dev-ai Bot commented Oct 21, 2026

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This branch was previously deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

vercel Bot commented Oct 21, 2026 •

edited

Loading

cubic-dev-ai Bot left a comment •

edited

Loading

greptile-apps Bot commented Oct 21, 2026 •

edited

Loading

cubic-dev-ai Bot left a comment •

edited

Loading