fix(v2): stop a third-party tool description from 500ing MCP discovery

`v2McpToolInputSchema` declared `description: z.string().optional()` inside a
`.catchall(z.unknown())` object, and a declared key beats the catchall. The MCP
SDK's own `ToolSchema.inputSchema` does not declare `description` at all, so any
value — including the JSON `null` a Python server emits for an absent one —
passes its validation and reaches Sim unchecked. The builder's outbound `.parse()`
then threw, and the discovery error policy correctly declines to classify a
Sim-side schema defect, so the endpoint that completes MCP onboarding answered a
bare 500. The key is dropped and left to the catchall; `type`, `properties`, and
`required` stay pinned because the SDK enforces those at least as tightly.

Also in the v2 resources family:

- The single-resource query schemas for MCP servers, skills, custom tools, and
  secrets are now `.strict()`, matching every list in the same family. A mistyped
  flag was silently ignored behind a 200.
- `openapi/resources.ts` re-derived `RESOURCE_ERRORS` and
  `RESOURCE_CONFLICT_ERRORS` inline in 21 of 22 operations. They now import the
  shared constants; the generated spec is unchanged, which is the point.
- The internal MCP refresh route stamped `updatedAt` alongside `lastToolsRefresh`.
  `updatedAt` means "configuration last changed" and is a public keyset sort, so
  a refresh moved rows out from under an in-flight page. `updateServerStatus`
  already held that invariant; the route now matches it.
- The discovery cooldown is a typed `McpServerCooldownError` rather than a
  substring search for `cooldown`. `McpConnectionError` interpolates the server's
  display name into its message, so a server named after the word was reported as
  a transient cooldown when its connection had genuinely failed.

Author: waleedlatif1

apps/docs/openapi-v2-resources.json

@@ -3019,10 +3019,6 @@
                   "type": "string",
                   "description": "Name of a required argument."
                 }
-              },
-              "description": {
-                "description": "Description of the argument object.",
-                "type": "string"
               }
             },
             "required": ["type"],

apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts

@@ -98,6 +98,35 @@ describe('MCP server refresh route', () => {
     )
   })
 
+  /**
+   * `updatedAt` means "when the server's configuration last changed" and is one
+   * of the public list's keyset sorts, so a refresh must not stamp it. The
+   * service's discovery status write already holds that invariant; this route
+   * writes the same row from the UI's refresh button, and stamping it here moves
+   * the row to the head of `sortBy=updatedAt` under an in-flight v2 page, which
+   * duplicates some servers across pages and skips others. Liveness is published
+   * through `lastToolsRefresh`, `lastConnected`, and `lastError`.
+   */
+  it('records the refresh without stamping updatedAt', async () => {
+    mockDiscoverServerTools.mockResolvedValueOnce([])
+
+    const request = new Request('http://localhost/api/mcp/servers/server-1/refresh', {
+      method: 'POST',
+    }) as NextRequest
+    await POST(request, { params: Promise.resolve({ id: 'server-1' }) })
+
+    const refreshWrites = dbChainMockFns.set.mock.calls.filter(
+      ([values]) => (values as Record<string, unknown>)?.lastToolsRefresh !== undefined
+    )
+    expect(refreshWrites.length).toBeGreaterThan(0)
+    for (const [values] of refreshWrites) {
+      expect(
+        (values as Record<string, unknown>).updatedAt,
+        'the refresh route stamped updatedAt, corrupting the updatedAt keyset page'
+      ).toBeUndefined()
+    }
+  })
+
   it('reports the discovery failure when status persistence leaves a stale connected row', async () => {
     const reflectedSecret = 'Bearer reflected-static-token'
     mockDiscoverServerTools.mockRejectedValueOnce(

apps/sim/app/api/mcp/servers/[id]/refresh/route.ts

@@ -229,11 +229,20 @@ export const POST = withRouteHandler(
 
         const now = new Date()
 
+        /**
+         * Deliberately leaves `updatedAt` alone, matching the invariant
+         * `McpService.updateServerStatus` holds: `updatedAt` means "when the
+         * server's configuration last changed", and it is one of the public
+         * list's keyset sorts. A refresh stamping it moves the row to the head
+         * of `sortBy=updatedAt` under an in-flight page, so a caller walking the
+         * list while anyone presses this button sees servers duplicated across
+         * pages and others skipped. Refresh liveness is already published
+         * through `lastToolsRefresh`, `lastConnected`, and `lastError`.
+         */
         const [refreshedServer] = await db
           .update(mcpServers)
           .set({
             lastToolsRefresh: now,
-            updatedAt: now,
           })
           .where(
             and(

apps/sim/app/api/v2/credentials/route.test.ts

@@ -134,6 +134,32 @@ describe('GET /api/v2/credentials', () => {
     expect(JSON.stringify(body)).not.toContain('createdBy')
   })
 
+  /**
+   * The projection is an explicit field-by-field copy, which is what makes a
+   * column added to the credential table later inert here: a field nobody wrote
+   * into `toV2Credential` is simply never read. The outbound response `.parse()`
+   * strips whatever survives, so a leak needs two independent mistakes. This
+   * pins the pairing against a row carrying a column the projection has never
+   * heard of.
+   */
+  it('withholds a credential column the projection was never taught to publish', async () => {
+    mocks.execute.mockResolvedValueOnce({
+      credentials: [{ ...credential, encryptedFutureSecret: 'MUST_NOT_LEAK_EITHER' }],
+      nextCursorKeys: null,
+      sortBy: 'createdAt',
+      sortOrder: 'desc',
+    })
+
+    const response = await GET(
+      new NextRequest(`http://localhost:3000/api/v2/credentials?workspaceId=${WORKSPACE_ID}`)
+    )
+    const body = await response.json()
+
+    expect(response.status).toBe(200)
+    expect(JSON.stringify(body)).not.toContain('encryptedFutureSecret')
+    expect(JSON.stringify(body)).not.toContain('MUST_NOT_LEAK_EITHER')
+  })
+
   it('hides repository errors that may contain secret details', async () => {
     mocks.execute.mockRejectedValueOnce(new Error('encryptedServiceAccountKey failed'))
 

apps/sim/app/api/v2/custom-tools/[id]/route.test.ts

@@ -130,6 +130,25 @@ describe('/api/v2/custom-tools/[id]', () => {
     })
   })
 
+  /**
+   * Every list in this family rejects a query param it does not implement, so
+   * the single-resource reads must too. A caller who mistypes a flag otherwise
+   * gets a 200 that silently ignored it, which reads as confirmation the flag
+   * exists and does nothing.
+   */
+  it('rejects a query param it does not implement', async () => {
+    const response = await GET(
+      new NextRequest(
+        `http://localhost:3000/api/v2/custom-tools/${tool.id}?workspaceId=${WORKSPACE_ID}&includeCodes=true`,
+        { method: 'GET', headers: { 'x-api-key': 'key' } }
+      ),
+      context
+    )
+
+    expect(response.status).toBe(400)
+    expect(mocks.get).not.toHaveBeenCalled()
+  })
+
   it('updates a custom tool through its semantic update operation', async () => {
     const response = await PATCH(
       request('PATCH', { workspaceId: WORKSPACE_ID, code: 'return 2' }),

apps/sim/app/api/v2/mcp-servers/[id]/route.test.ts

@@ -121,6 +121,25 @@ describe('/api/v2/mcp-servers/[id]', () => {
     })
   })
 
+  /**
+   * Every list in this family rejects a query param it does not implement, so
+   * the single-resource reads must too. A caller who mistypes a flag otherwise
+   * gets a 200 that silently ignored it, which reads as confirmation the flag
+   * exists and does nothing.
+   */
+  it('rejects a query param it does not implement', async () => {
+    const response = await GET(
+      new NextRequest(
+        `http://localhost:3000/api/v2/mcp-servers/${server.id}?workspaceId=${WORKSPACE_ID}&includeTools=true`,
+        { method: 'GET', headers: { 'x-api-key': 'key' } }
+      ),
+      context
+    )
+
+    expect(response.status).toBe(400)
+    expect(mocks.get).not.toHaveBeenCalled()
+  })
+
   it('updates an MCP server through the strict semantic update operation', async () => {
     const response = await PATCH(
       request('PATCH', { workspaceId: WORKSPACE_ID, name: 'New docs' }),

apps/sim/app/api/v2/mcp-servers/[id]/tools/route.test.ts

@@ -36,7 +36,11 @@ vi.mock('@/lib/mcp/application/use-cases', () => ({
 }))
 
 import { WorkspaceApiKeyAuthorizationError } from '@/lib/core/application'
-import { McpConnectionError, McpOauthAuthorizationRequiredError } from '@/lib/mcp/types'
+import {
+  McpConnectionError,
+  McpOauthAuthorizationRequiredError,
+  McpServerCooldownError,
+} from '@/lib/mcp/types'
 import { GET } from '@/app/api/v2/mcp-servers/[id]/tools/route'
 
 const WORKSPACE_ID = 'workspace-1'
@@ -173,6 +177,63 @@ describe('/api/v2/mcp-servers/[id]/tools', () => {
     expect(body.error.code).toBe('INTERNAL_ERROR')
   })
 
+  /**
+   * `inputSchema` below the `object` wrapper is authored by the third-party
+   * server, and the MCP SDK's own `ToolSchema` does not declare `description`
+   * there — its `.catchall(z.unknown())` lets any value through, so a server
+   * serializing an absent description as JSON `null` (what a Python `None`
+   * produces) reaches Sim unvalidated. Declaring the key more tightly than the
+   * upstream schema does made the builder's outbound `.parse()` throw, and
+   * discovery answered a bare 500 for a payload the protocol permits.
+   */
+  it('publishes a tool whose server reported a non-string inputSchema description', async () => {
+    mocks.discover.mockResolvedValueOnce({
+      tools: [
+        {
+          ...TOOL,
+          inputSchema: { type: 'object' as const, description: null, properties: {} },
+        },
+      ],
+    })
+
+    const response = await GET(request(`workspaceId=${WORKSPACE_ID}`), { ...context })
+    const body = await response.json()
+
+    expect(response.status).toBe(200)
+    expect(body.data[0].inputSchema).toEqual({
+      type: 'object',
+      description: null,
+      properties: {},
+    })
+  })
+
+  /**
+   * The 503 wording used to be selected by searching the error message for
+   * `cooldown`. `McpConnectionError` interpolates the server's display name into
+   * that message, so a server the caller happened to name after the word
+   * borrowed the negative-cache wording and told them to wait out a cooldown
+   * that was never entered.
+   */
+  it('does not read cooldown wording out of a server display name', async () => {
+    mocks.discover.mockRejectedValueOnce(new McpConnectionError('ECONNREFUSED', 'Cooldown Docs'))
+
+    const response = await GET(request(`workspaceId=${WORKSPACE_ID}`), { ...context })
+    const body = await response.json()
+
+    expect(response.status).toBe(503)
+    expect(body.error.message).toBe('The MCP server could not be reached')
+  })
+
+  it('reports a server inside the discovery cooldown with its own wording', async () => {
+    mocks.discover.mockRejectedValueOnce(new McpServerCooldownError(SERVER_ID))
+
+    const response = await GET(request(`workspaceId=${WORKSPACE_ID}`), { ...context })
+    const body = await response.json()
+
+    expect(response.status).toBe(503)
+    expect(body.error.message).toBe('The MCP server recently failed and is in cooldown')
+  })
+
   it('rejects a workspace API key, which cannot supply the caller`s OAuth grant', async () => {
     mocks.discover.mockRejectedValueOnce(new WorkspaceApiKeyAuthorizationError())
 

apps/sim/app/api/v2/mcp-servers/utils.ts

@@ -6,7 +6,11 @@ import { createV2ResourceConcealmentPolicy, type V2ErrorPolicy } from '@/lib/api
 import { isTimeoutError } from '@/lib/core/execution-limits'
 import { projectMcpHeaders } from '@/lib/mcp/projection'
 import type { McpServerRow } from '@/lib/mcp/queries'
-import { McpConnectionError, McpOauthAuthorizationRequiredError } from '@/lib/mcp/types'
+import {
+  McpConnectionError,
+  McpOauthAuthorizationRequiredError,
+  McpServerCooldownError,
+} from '@/lib/mcp/types'
 import { v2Error } from '@/app/api/v2/lib/response'
 
 /**
@@ -49,10 +53,15 @@ export const MCP_SERVER_REAUTHORIZATION_REQUIRED = 'MCP_SERVER_REAUTHORIZATION_R
  *
  * Every branch returns a constant, so an upstream message — which may quote a
  * hostname, a token endpoint, or a stack — never reaches the caller.
+ *
+ * Selection is typed for the same reason classification is. The cooldown branch
+ * used to search the message for `cooldown`, but `McpConnectionError`
+ * interpolates the server's display name into its message, so a server a caller
+ * named after the word was told to wait out a cooldown it was never in.
  */
 function unreachableServerMessage(error: unknown): string {
   if (isTimeoutError(error)) return 'The MCP server took too long to respond'
-  if (error instanceof McpConnectionError && error.message.toLowerCase().includes('cooldown')) {
+  if (error instanceof McpServerCooldownError) {
     return 'The MCP server recently failed and is in cooldown'
   }
   return 'The MCP server could not be reached'

apps/sim/app/api/v2/secrets/[name]/route.test.ts

@@ -157,6 +157,25 @@ describe('/api/v2/secrets/[name]', () => {
     })
   })
 
+  /**
+   * The secrets list rejects a query param it does not implement, so the delete
+   * must too. A caller who mistypes `scope` otherwise gets a 400 for the missing
+   * required param — but a caller who adds a param that does not exist at all
+   * would have had it silently ignored.
+   */
+  it('rejects a query param it does not implement', async () => {
+    const response = await DELETE(
+      new NextRequest(
+        `http://localhost:3000/api/v2/secrets/${SECRET_NAME}?workspaceId=${WORKSPACE_ID}&scope=workspace&scopes=personal`,
+        { method: 'DELETE', headers: { 'x-api-key': 'key' } }
+      ),
+      context
+    )
+
+    expect(response.status).toBe(400)
+    expect(mocks.remove).not.toHaveBeenCalled()
+  })
+
   it('renders typed application errors without leaking raw errors', async () => {
     mocks.remove.mockRejectedValueOnce(new OrchestrationError('not_found', 'stored detail'))
 

apps/sim/app/api/v2/skills/[id]/route.test.ts

@@ -124,6 +124,25 @@ describe('/api/v2/skills/[id]', () => {
     })
   })
 
+  /**
+   * Every list in this family rejects a query param it does not implement, so
+   * the single-resource reads must too. A caller who mistypes a flag otherwise
+   * gets a 200 that silently ignored it, which reads as confirmation the flag
+   * exists and does nothing.
+   */
+  it('rejects a query param it does not implement', async () => {
+    const response = await GET(
+      new NextRequest(
+        `http://localhost:3000/api/v2/skills/${skill.id}?workspaceId=${WORKSPACE_ID}&includeContents=true`,
+        { method: 'GET', headers: { 'x-api-key': 'key' } }
+      ),
+      context
+    )
+
+    expect(response.status).toBe(400)
+    expect(mocks.get).not.toHaveBeenCalled()
+  })
+
   it('updates a skill and emits only surface analytics', async () => {
     const response = await PATCH(
       request('PATCH', { workspaceId: WORKSPACE_ID, content: '# Updated' }),