fix(mothership): keep untagged mcp discovery side-effect free and the catalog resilient

apps/sim/lib/mcp/application/use-cases.ts

@@ -143,6 +143,8 @@ export interface DiscoverMcpServerToolsInput {
   refresh?: boolean
   signal?: AbortSignal
   requireComplete?: boolean
+  /** Discovery nobody asked for: reads the server's shared health state but never writes it. */
+  bestEffort?: boolean
 }
 
 /**
@@ -192,14 +194,18 @@ export const discoverMcpServerToolsUseCase = defineAuthorizedWorkspaceUseCase({
     })
     const refresh = input.refresh ? 'skip-cache' : 'cache-aside'
     const tools =
-      input.signal || input.requireComplete
+      input.signal || input.requireComplete || input.bestEffort
         ? await mcpService.discoverServerTools(
             userId,
             context.server.id,
             context.workspaceId,
             refresh,
             undefined,
-            { signal: input.signal, requireComplete: input.requireComplete }
+            {
+              signal: input.signal,
+              requireComplete: input.requireComplete,
+              bestEffort: input.bestEffort,
+            }
           )
         : await mcpService.discoverServerTools(
             userId,

apps/sim/lib/mcp/oauth/index.ts

@@ -18,6 +18,7 @@ export {
   clearTokens,
   clearVerifier,
   getOrCreateOauthRow,
+  hasDecryptableOauthTokens,
   loadOauthRow,
   loadOauthRowByState,
   type McpOauthRow,

apps/sim/lib/mcp/oauth/provider.test.ts

@@ -0,0 +1,94 @@
+import { auth } from '@modelcontextprotocol/sdk/client/auth.js'
+import { dbChainMockFns, encryptionMock, resetDbChainMock } from '@sim/testing'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+vi.mock('@/lib/core/security/encryption', () => encryptionMock)
+
+import { SimMcpOauthProvider } from '@/lib/mcp/oauth/provider'
+import type { McpOauthRow } from '@/lib/mcp/oauth/storage'
+
+const SERVER_URL = 'https://mcp.example.com/mcp'
+const ISSUER = 'https://auth.example.com'
+
+function grantRow(): McpOauthRow {
+  return {
+    id: 'oauth-row-1',
+    mcpServerId: 'server-1',
+    userId: 'authorizer-1',
+    workspaceId: 'workspace-1',
+    organizationId: null,
+    clientInformation: { client_id: 'client-1', issuer: ISSUER },
+    tokens: { access_token: 'expired', token_type: 'bearer', refresh_token: 'refresh-1' },
+    codeVerifier: null,
+    state: null,
+    stateCreatedAt: null,
+    updatedAt: new Date(),
+  }
+}
+
+/** An authorization server whose token endpoint answers every refresh with `token`. */
+function authorizationServer(token: { status: number; body: Record<string, unknown> }) {
+  return vi.fn(async (input: string | URL) => {
+    const url = String(input)
+    const json = (status: number, body: unknown) =>
+      new Response(JSON.stringify(body), {
+        status,
+        headers: { 'content-type': 'application/json' },
+      })
+    if (url.startsWith('https://mcp.example.com/.well-known/oauth-protected-resource'))
+      return json(200, { resource: SERVER_URL, authorization_servers: [ISSUER] })
+    if (url.startsWith(`${ISSUER}/.well-known/`))
+      return json(200, {
+        issuer: ISSUER,
+        authorization_endpoint: `${ISSUER}/authorize`,
+        token_endpoint: `${ISSUER}/token`,
+        response_types_supported: ['code'],
+        code_challenge_methods_supported: ['S256'],
+      })
+    if (url === `${ISSUER}/token`) return json(token.status, token.body)
+    return json(404, {})
+  })
+}
+
+/**
+ * Best-effort discovery connects with the workspace's one shared grant. A refresh that fails
+ * there must leave the grant as it was: the SDK otherwise clears the tokens and starts a new
+ * authorization flow, disconnecting the server for every member.
+ */
+describe('SimMcpOauthProvider preserving the shared grant', () => {
+  beforeEach(() => {
+    resetDbChainMock()
+  })
+
+  it('writes nothing to the OAuth row when a refresh is rejected', async () => {
+    const provider = new SimMcpOauthProvider({ row: grantRow(), preserveGrant: true })
+
+    await expect(
+      auth(provider, {
+        serverUrl: SERVER_URL,
+        fetchFn: authorizationServer({ status: 400, body: { error: 'invalid_grant' } }),
+      })
+    ).rejects.toThrow('MCP OAuth grant left unchanged')
+
+    expect(dbChainMockFns.update).not.toHaveBeenCalled()
+    expect(dbChainMockFns.insert).not.toHaveBeenCalled()
+    expect(provider.tokens()?.refresh_token).toBe('refresh-1')
+  })
+
+  it('persists a successful refresh, which keeps the grant valid for everyone', async () => {
+    const provider = new SimMcpOauthProvider({ row: grantRow(), preserveGrant: true })
+
+    await expect(
+      auth(provider, {
+        serverUrl: SERVER_URL,
+        fetchFn: authorizationServer({
+          status: 200,
+          body: { access_token: 'fresh', token_type: 'bearer', refresh_token: 'refresh-2' },
+        }),
+      })
+    ).resolves.toBe('AUTHORIZED')
+
+    expect(dbChainMockFns.update).toHaveBeenCalledTimes(1)
+    expect(provider.tokens()?.access_token).toBe('fresh')
+  })
+})

apps/sim/lib/mcp/oauth/provider.ts

@@ -71,17 +71,37 @@ interface SimMcpOauthProviderInit {
    * Dynamic Client Registration and uses these for the auth/token exchange.
    */
   preregistered?: PreregisteredClient
+  /**
+   * The grant belongs to every member and this caller did not ask to reauthorize it: a
+   * refreshed token still persists, but clearing the grant, registering a client or starting
+   * an authorization flow fails with {@link McpOauthGrantPreservedError} before any write.
+   */
+  preserveGrant?: boolean
+}
+
+/** A refresh failed where reauthorizing the shared grant is left to an explicit reconnect. */
+class McpOauthGrantPreservedError extends Error {
+  constructor(step: string) {
+    super(`MCP OAuth grant left unchanged: ${step} requires an explicit reconnect`)
+    this.name = 'McpOauthGrantPreservedError'
+  }
 }
 
 export class SimMcpOauthProvider implements OAuthClientProvider {
   private row: McpOauthRow
   private readonly scope?: string
   private readonly preregistered?: PreregisteredClient
+  private readonly preserveGrant: boolean
 
-  constructor({ row, scope, preregistered }: SimMcpOauthProviderInit) {
+  constructor({ row, scope, preregistered, preserveGrant = false }: SimMcpOauthProviderInit) {
     this.row = row
     this.scope = preregistered?.scope ?? scope
     this.preregistered = preregistered
+    this.preserveGrant = preserveGrant
+  }
+
+  private assertMayChangeGrant(step: string): void {
+    if (this.preserveGrant) throw new McpOauthGrantPreservedError(step)
   }
 
   /** Deployment registrations may restrict consent even when discovery advertises more tools. */
@@ -108,6 +128,7 @@ export class SimMcpOauthProvider implements OAuthClientProvider {
   }
 
   async state(): Promise<string> {
+    this.assertMayChangeGrant('authorization')
     const state = generateId()
     await saveState(this.row.id, state, 'provider.state')
     return state
@@ -132,6 +153,7 @@ export class SimMcpOauthProvider implements OAuthClientProvider {
 
   async saveClientInformation(info: OAuthClientInformationMixed): Promise<void> {
     if (this.preregistered) return
+    this.assertMayChangeGrant('client registration')
     await saveClientInformationDb(this.row.id, info)
     this.row.clientInformation = info
   }
@@ -150,6 +172,7 @@ export class SimMcpOauthProvider implements OAuthClientProvider {
   }
 
   async saveCodeVerifier(codeVerifier: string): Promise<void> {
+    this.assertMayChangeGrant('authorization')
     await saveCodeVerifierDb(this.row.id, codeVerifier)
     this.row.codeVerifier = codeVerifier
   }
@@ -164,6 +187,7 @@ export class SimMcpOauthProvider implements OAuthClientProvider {
   async invalidateCredentials(
     scope: 'all' | 'client' | 'tokens' | 'verifier' | 'discovery'
   ): Promise<void> {
+    if (scope !== 'discovery') this.assertMayChangeGrant(`invalidating ${scope} credentials`)
     if (scope === 'all' || scope === 'client') {
       await clearClient(this.row.id)
       this.row.clientInformation = null

apps/sim/lib/mcp/oauth/storage.test.ts

@@ -14,7 +14,12 @@ afterAll(resetRedisConfigMock)
 
 vi.mock('@/lib/core/security/encryption', () => encryptionMock)
 
-import { getOrCreateOauthRow, loadOauthRow, withMcpOauthRefreshLock } from './storage'
+import {
+  getOrCreateOauthRow,
+  hasDecryptableOauthTokens,
+  loadOauthRow,
+  withMcpOauthRefreshLock,
+} from './storage'
 
 describe('MCP OAuth storage', () => {
   beforeEach(() => {
@@ -77,6 +82,21 @@ describe('MCP OAuth storage', () => {
     expect(row.userId).toBe('authorizer-1')
     expect(dbChainMockFns.insert).not.toHaveBeenCalled()
   })
+
+  /** The row is the workspace's one grant; a presence check must never be what erases it. */
+  it('reports undecryptable tokens as unusable without clearing the shared row', async () => {
+    dbChainMockFns.limit.mockResolvedValueOnce([{ tokens: 'ciphertext-from-a-rotated-key' }])
+    encryptionMockFns.mockDecryptSecret.mockRejectedValueOnce(new Error('bad decrypt'))
+
+    expect(await hasDecryptableOauthTokens('server-1')).toBe(false)
+    expect(dbChainMockFns.update).not.toHaveBeenCalled()
+    expect(dbChainMockFns.set).not.toHaveBeenCalled()
+
+    dbChainMockFns.limit.mockResolvedValueOnce([{ tokens: 'ciphertext' }])
+    expect(await hasDecryptableOauthTokens('server-1')).toBe(true)
+    dbChainMockFns.limit.mockResolvedValueOnce([{ tokens: null }])
+    expect(await hasDecryptableOauthTokens('server-1')).toBe(false)
+  })
 })
 
 describe('withMcpOauthRefreshLock', () => {

apps/sim/lib/mcp/oauth/storage.ts

@@ -156,6 +156,26 @@ export async function loadOauthRow(params: { mcpServerId: string }): Promise<Mcp
   return mapOauthRow(row)
 }
 
+/**
+ * Whether the server's OAuth row holds tokens that decrypt, read without writing: unlike
+ * {@link loadOauthRow}, an undecryptable column is reported, never cleared, so a caller that
+ * only asks whether to try a connection cannot erase the grant every member shares.
+ */
+export async function hasDecryptableOauthTokens(mcpServerId: string): Promise<boolean> {
+  const [row] = await db
+    .select({ tokens: mcpServerOauth.tokens })
+    .from(mcpServerOauth)
+    .where(eq(mcpServerOauth.mcpServerId, mcpServerId))
+    .limit(1)
+  if (!row?.tokens) return false
+  try {
+    await decryptSecret(row.tokens)
+    return true
+  } catch {
+    return false
+  }
+}
+
 export async function setOauthRowUser(rowId: string, userId: string): Promise<void> {
   await db
     .update(mcpServerOauth)

apps/sim/lib/mcp/resolve-config.test.ts

@@ -1,6 +1,6 @@
 import { environmentUtilsMockFns } from '@sim/testing/mocks/environment-utils.mock'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
-import { resolveMcpConfigEnvVars } from '@/lib/mcp/resolve-config'
+import { findMissingMcpConfigEnvVars, resolveMcpConfigEnvVars } from '@/lib/mcp/resolve-config'
 
 const mockGetEffectiveEnvironmentSnapshot =
   environmentUtilsMockFns.mockGetEffectiveEnvironmentSnapshot
@@ -171,3 +171,63 @@ describe('resolveMcpConfigEnvVars secret provenance', () => {
     ])
   })
 })
+
+describe('findMissingMcpConfigEnvVars', () => {
+  it('names the variables this requester has no value for, without reading env for literal configs', async () => {
+    mockGetEffectiveEnvironmentSnapshot.mockReset()
+    mockGetEffectiveEnvironmentSnapshot.mockResolvedValue({
+      personalEncrypted: {},
+      workspaceEncrypted: {},
+      personalDecrypted: { MCP_HOST: 'api.example.com' },
+      workspaceDecrypted: {},
+      conflicts: [],
+      decryptionFailures: [],
+    })
+
+    expect(await findMissingMcpConfigEnvVars(BASE_CONFIG, 'user-b', 'workspace-1')).toEqual([
+      'MCP_TOKEN',
+    ])
+    expect(
+      await findMissingMcpConfigEnvVars(
+        { ...BASE_CONFIG, url: 'https://api.example.com/mcp', headers: {} },
+        'user-b',
+        'workspace-1'
+      )
+    ).toEqual([])
+    expect(mockGetEffectiveEnvironmentSnapshot).toHaveBeenCalledTimes(1)
+  })
+
+  it('counts a variable that failed to decrypt as missing', async () => {
+    mockGetEffectiveEnvironmentSnapshot.mockResolvedValue({
+      personalEncrypted: { MCP_HOST: 'host-encrypted', MCP_TOKEN: 'token-encrypted' },
+      workspaceEncrypted: {},
+      personalDecrypted: { MCP_HOST: 'api.example.com', MCP_TOKEN: '' },
+      workspaceDecrypted: {},
+      conflicts: [],
+      decryptionFailures: ['MCP_TOKEN'],
+    })
+
+    expect(await findMissingMcpConfigEnvVars(BASE_CONFIG, 'user-b', 'workspace-1')).toEqual([
+      'MCP_TOKEN',
+    ])
+  })
+
+  it('counts an intentionally empty value as present', async () => {
+    mockGetEffectiveEnvironmentSnapshot.mockResolvedValue({
+      personalEncrypted: { MCP_PATH_PREFIX: 'prefix-encrypted' },
+      workspaceEncrypted: {},
+      personalDecrypted: { MCP_PATH_PREFIX: '' },
+      workspaceDecrypted: {},
+      conflicts: [],
+      decryptionFailures: [],
+    })
+
+    expect(
+      await findMissingMcpConfigEnvVars(
+        { ...BASE_CONFIG, url: 'https://example.com{{MCP_PATH_PREFIX}}/mcp', headers: {} },
+        'user-b',
+        'workspace-1'
+      )
+    ).toEqual([])
+  })
+})

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

@@ -6,13 +6,14 @@
 
 import { createLogger } from '@sim/logger'
 import { getErrorMessage } from '@sim/utils/errors'
+import { omit } from '@sim/utils/object'
 import {
   type EnvironmentResolutionSnapshot,
   getEffectiveEnvironmentSnapshot,
 } from '@/lib/environment/utils'
 import type { McpServerConfig } from '@/lib/mcp/types'
 import { recordSecretUsage } from '@/lib/secrets/usage/record'
-import { resolveEnvVarReferences } from '@/executor/utils/reference-validation'
+import { createEnvVarPattern, resolveEnvVarReferences } from '@/executor/utils/reference-validation'
 import {
   createIncompleteResolvedSecretTraceRegistry,
   createResolvedSecretTraceRegistry,
@@ -149,3 +150,36 @@ export async function resolveMcpConfigEnvVars(
     resolvedSecretTraceProvenance,
   }
 }
+
+/**
+ * The variables `config` references (url, headers) that `userId` has no value for in this
+ * workspace. Read-only: it records no secret usage, so it can check whether a requester can
+ * authenticate to a server before anything connects.
+ */
+export async function findMissingMcpConfigEnvVars(
+  config: McpServerConfig,
+  userId: string,
+  workspaceId?: string
+): Promise<string[]> {
+  const values = [config.url, ...Object.values(config.headers ?? {})].filter(
+    (value): value is string => typeof value === 'string'
+  )
+  if (!values.some((value) => createEnvVarPattern().test(value))) return []
+  const env = await getEffectiveEnvironmentSnapshot(userId, workspaceId)
+  const merged = { ...env.personalDecrypted, ...env.workspaceDecrypted }
+  /**
+   * A value that failed to decrypt arrives as '' and is named in `decryptionFailures`; an
+   * intentionally empty one is only ''. The list does not say which slice failed, so a key
+   * counts as failed only when its effective value is also empty. The one ambiguity left, an
+   * intentionally empty workspace value over a personal one that failed, counts as missing,
+   * so best-effort discovery skips rather than connects.
+   */
+  const failed = new Set(env.decryptionFailures)
+  const envVars = omit(
+    merged,
+    Object.keys(merged).filter((key) => failed.has(key) && merged[key] === '')
+  )
+  const missingKeys: string[] = []
+  for (const value of values) resolveEnvVarReferences(value, envVars, { missingKeys })
+  return [...new Set(missingKeys)]
+}