fix(api): close unique-violation, revival, orphan-write, and env-rename gaps

Author: TheodoreSpeaks

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

@@ -276,6 +276,25 @@ describe('PATCH /api/v2/credentials/[id]', () => {
     expect((await res.json()).error.code).toBe('SERVICE_UNAVAILABLE')
   })
 
+  it('rejects a displayName rename on an env credential instead of dropping it', async () => {
+    mockGetWorkspaceCredential.mockResolvedValue(
+      buildRow({ type: 'env_workspace', envKey: 'STRIPE_API_KEY', displayName: 'STRIPE_API_KEY' })
+    )
+    const res = await callPatch({ workspaceId: WORKSPACE_ID, displayName: 'Renamed' })
+
+    expect(res.status).toBe(400)
+    expect((await res.json()).error.message).toContain('envKey')
+    expect(mockPerformUpdateCredential).not.toHaveBeenCalled()
+  })
+
+  it('still allows a description change on an env credential', async () => {
+    mockGetWorkspaceCredential.mockResolvedValue(buildRow({ type: 'env_workspace' }))
+    const res = await callPatch({ workspaceId: WORKSPACE_ID, description: 'note' })
+
+    expect(res.status).toBe(200)
+    expect(mockPerformUpdateCredential).toHaveBeenCalled()
+  })
+
   it('rotates a secret without echoing it back', async () => {
     const res = await callPatch({ workspaceId: WORKSPACE_ID, apiToken: 'brand-new-token' })
     const body = await res.json()

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

@@ -118,6 +118,22 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Rout
     const actor = await getCredentialActorContext(id, userId)
     if (!actor.member && !actor.isAdmin) return v2Error('NOT_FOUND', 'Credential not found')
 
+    /**
+     * An env credential's display name IS its `envKey` — the lib only applies
+     * `displayName` to `oauth` and `service_account`, so accepting it here would
+     * either drop the rename silently (when sent alongside `description`) or
+     * fail with an unrelated environment-editor message (when sent alone).
+     */
+    if (
+      changes.displayName !== undefined &&
+      (existing.type === 'env_workspace' || existing.type === 'env_personal')
+    ) {
+      return v2Error(
+        'BAD_REQUEST',
+        'displayName cannot be set on an environment credential — its name is its envKey. Delete it and create one under the new key.'
+      )
+    }
+
     const result = await performUpdateCredential({ ...changes, credentialId: id, userId, request })
 
     if (!result.success) {

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

@@ -14,14 +14,14 @@ const {
   mockGetWorkspaceCustomTool,
   mockGetWorkspaceCustomToolByTitle,
   mockDeleteWorkspaceCustomTool,
-  mockUpsertCustomTools,
+  mockUpdateWorkspaceCustomTool,
 } = vi.hoisted(() => ({
   mockCheckRateLimit: vi.fn(),
   mockResolveWorkspaceAccess: vi.fn(),
   mockGetWorkspaceCustomTool: vi.fn(),
   mockGetWorkspaceCustomToolByTitle: vi.fn(),
   mockDeleteWorkspaceCustomTool: vi.fn(),
-  mockUpsertCustomTools: vi.fn(),
+  mockUpdateWorkspaceCustomTool: vi.fn(),
 }))
 
 vi.mock('@/app/api/v1/middleware', () => ({
@@ -33,7 +33,7 @@ vi.mock('@/lib/workflows/custom-tools/operations', () => ({
   getWorkspaceCustomTool: mockGetWorkspaceCustomTool,
   getWorkspaceCustomToolByTitle: mockGetWorkspaceCustomToolByTitle,
   deleteWorkspaceCustomTool: mockDeleteWorkspaceCustomTool,
-  upsertCustomTools: mockUpsertCustomTools,
+  updateWorkspaceCustomTool: mockUpdateWorkspaceCustomTool,
 }))
 
 vi.mock('@/app/api/v2/lib/gate', () => ({
@@ -175,7 +175,7 @@ describe('PATCH /api/v2/custom-tools/[id]', () => {
     mockResolveWorkspaceAccess.mockResolvedValue(null)
     mockGetWorkspaceCustomTool.mockResolvedValue(buildTool())
     mockGetWorkspaceCustomToolByTitle.mockResolvedValue(null)
-    mockUpsertCustomTools.mockResolvedValue([buildTool()])
+    mockUpdateWorkspaceCustomTool.mockResolvedValue(buildTool())
   })
 
   it('returns 404 when the v2 API surface flag is off', async () => {
@@ -186,20 +186,20 @@ describe('PATCH /api/v2/custom-tools/[id]', () => {
     const res = await callPatch({ workspaceId: 'workspace-1', code: 'return 1' })
 
     expect(res.status).toBe(404)
-    expect(mockUpsertCustomTools).not.toHaveBeenCalled()
+    expect(mockUpdateWorkspaceCustomTool).not.toHaveBeenCalled()
   })
 
   it('400s when no field to change is supplied', async () => {
     const res = await callPatch({ workspaceId: 'workspace-1' })
     expect(res.status).toBe(400)
-    expect(mockUpsertCustomTools).not.toHaveBeenCalled()
+    expect(mockUpdateWorkspaceCustomTool).not.toHaveBeenCalled()
   })
 
   it('surfaces an access-denied failure in the v2 error envelope', async () => {
     mockResolveWorkspaceAccess.mockResolvedValue(ACCESS_DENIED)
     const res = await callPatch({ workspaceId: 'workspace-1', code: 'return 1' })
     expect(res.status).toBe(403)
-    expect(mockUpsertCustomTools).not.toHaveBeenCalled()
+    expect(mockUpdateWorkspaceCustomTool).not.toHaveBeenCalled()
   })
 
   it('returns the rate-limit response when denied', async () => {
@@ -213,7 +213,7 @@ describe('PATCH /api/v2/custom-tools/[id]', () => {
     mockGetWorkspaceCustomTool.mockResolvedValue(null)
     const res = await callPatch({ workspaceId: 'workspace-1', code: 'return 1' })
     expect(res.status).toBe(404)
-    expect(mockUpsertCustomTools).not.toHaveBeenCalled()
+    expect(mockUpdateWorkspaceCustomTool).not.toHaveBeenCalled()
   })
 
   it('409s when renaming onto an existing title', async () => {
@@ -223,27 +223,29 @@ describe('PATCH /api/v2/custom-tools/[id]', () => {
 
     expect(res.status).toBe(409)
     expect((await res.json()).error.code).toBe('CONFLICT')
-    expect(mockUpsertCustomTools).not.toHaveBeenCalled()
+    expect(mockUpdateWorkspaceCustomTool).not.toHaveBeenCalled()
   })
 
   it('merges the partial body against the stored tool', async () => {
     const res = await callPatch({ workspaceId: 'workspace-1', code: 'return 2' })
 
     expect(res.status).toBe(200)
-    expect(mockUpsertCustomTools).toHaveBeenCalledWith(
-      expect.objectContaining({
-        workspaceId: 'workspace-1',
-        userId: 'user-1',
-        tools: [
-          {
-            id: 'tool_abc123',
-            title: 'lookup_order',
-            schema: TOOL_SCHEMA,
-            code: 'return 2',
-          },
-        ],
-      })
-    )
+    expect(mockUpdateWorkspaceCustomTool).toHaveBeenCalledWith({
+      workspaceId: 'workspace-1',
+      toolId: 'tool_abc123',
+      title: 'lookup_order',
+      schema: TOOL_SCHEMA,
+      code: 'return 2',
+    })
+  })
+
+  it('404s rather than orphaning a tool deleted between the read and the write', async () => {
+    mockUpdateWorkspaceCustomTool.mockResolvedValue(null)
+
+    const res = await callPatch({ workspaceId: 'workspace-1', code: 'return 2' })
+
+    expect(res.status).toBe(404)
+    expect((await res.json()).error.code).toBe('NOT_FOUND')
   })
 })
 

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

@@ -14,7 +14,7 @@ import {
   deleteWorkspaceCustomTool,
   getWorkspaceCustomTool,
   getWorkspaceCustomToolByTitle,
-  upsertCustomTools,
+  updateWorkspaceCustomTool,
 } from '@/lib/workflows/custom-tools/operations'
 import { checkRateLimit, resolveWorkspaceAccess } from '@/app/api/v1/middleware'
 import { toV2CustomTool, v2CustomToolWriteError } from '@/app/api/v2/custom-tools/utils'
@@ -114,21 +114,13 @@ export const PATCH = withRouteHandler(async (request: NextRequest, context: Rout
       }
     }
 
-    await upsertCustomTools({
-      tools: [
-        {
-          id,
-          title: title ?? current.title,
-          schema: schema ?? current.schema,
-          code: code ?? current.code,
-        },
-      ],
+    const updated = await updateWorkspaceCustomTool({
       workspaceId,
-      userId,
-      requestId,
+      toolId: id,
+      title: title ?? current.title,
+      schema: schema ?? current.schema,
+      code: code ?? current.code,
     })
-
-    const updated = await getWorkspaceCustomTool({ workspaceId, toolId: id })
     if (!updated) return v2Error('NOT_FOUND', 'Custom tool not found')
 
     recordAudit({

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

@@ -14,14 +14,14 @@ const {
   mockResolveWorkspaceAccess,
   mockListWorkspaceMcpServers,
   mockGetWorkspaceMcpServer,
-  mockMcpServerIdExists,
+  mockGetMcpServerIdState,
   mockPerformCreateMcpServer,
 } = vi.hoisted(() => ({
   mockCheckRateLimit: vi.fn(),
   mockResolveWorkspaceAccess: vi.fn(),
   mockListWorkspaceMcpServers: vi.fn(),
   mockGetWorkspaceMcpServer: vi.fn(),
-  mockMcpServerIdExists: vi.fn(),
+  mockGetMcpServerIdState: vi.fn(),
   mockPerformCreateMcpServer: vi.fn(),
 }))
 
@@ -33,7 +33,7 @@ vi.mock('@/app/api/v1/middleware', () => ({
 vi.mock('@/lib/mcp/queries', () => ({
   listWorkspaceMcpServers: mockListWorkspaceMcpServers,
   getWorkspaceMcpServer: mockGetWorkspaceMcpServer,
-  mcpServerIdExists: mockMcpServerIdExists,
+  getMcpServerIdState: mockGetMcpServerIdState,
 }))
 
 vi.mock('@/lib/mcp/orchestration', () => ({
@@ -204,7 +204,7 @@ describe('POST /api/v2/mcp-servers', () => {
     vi.clearAllMocks()
     mockCheckRateLimit.mockResolvedValue(RATE_LIMIT_OK)
     mockResolveWorkspaceAccess.mockResolvedValue(null)
-    mockMcpServerIdExists.mockResolvedValue(false)
+    mockGetMcpServerIdState.mockResolvedValue(null)
     mockPerformCreateMcpServer.mockResolvedValue({
       success: true,
       serverId: 'mcp-abc12345',
@@ -269,7 +269,7 @@ describe('POST /api/v2/mcp-servers', () => {
   })
 
   it('409s on a duplicate URL without letting the lib upsert', async () => {
-    mockMcpServerIdExists.mockResolvedValue(true)
+    mockGetMcpServerIdState.mockResolvedValue({ deleted: false })
 
     const res = await callCreate(VALID_BODY)
 
@@ -291,6 +291,20 @@ describe('POST /api/v2/mcp-servers', () => {
     expect((await res.json()).error.code).toBe('CONFLICT')
   })
 
+  it('revives a soft-deleted URL instead of stranding it behind a 409', async () => {
+    mockGetMcpServerIdState.mockResolvedValue({ deleted: true })
+    mockPerformCreateMcpServer.mockResolvedValue({
+      success: true,
+      serverId: 'mcp-abc12345',
+      updated: true,
+    })
+
+    const res = await callCreate(VALID_BODY)
+
+    expect(res.status).toBe(201)
+    expect(mockPerformCreateMcpServer).toHaveBeenCalled()
+  })
+
   it('creates the server and returns 201 with the public shape', async () => {
     const res = await callCreate({ ...VALID_BODY, headers: { Authorization: 'Bearer tok' } })
     const body = await res.json()

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

@@ -10,9 +10,9 @@ import { generateRequestId } from '@/lib/core/utils/request'
 import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
 import { performCreateMcpServer } from '@/lib/mcp/orchestration'
 import {
+  getMcpServerIdState,
   getWorkspaceMcpServer,
   listWorkspaceMcpServers,
-  mcpServerIdExists,
 } from '@/lib/mcp/queries'
 import { generateMcpServerId } from '@/lib/mcp/utils'
 import { checkRateLimit, resolveWorkspaceAccess } from '@/app/api/v1/middleware'
@@ -106,14 +106,21 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
      * same URL silently overwrites the first. The internal surface and the
      * copilot rely on that; a public create must not, so the collision is
      * detected here, before the lib is given a chance to clobber the row.
+     *
+     * Only a *live* row is a conflict. A soft-deleted one is revived by the lib
+     * rather than inserted alongside, and reporting it as a duplicate would
+     * strand that URL for good: the detail routes resolve live rows only, so it
+     * could be neither fetched, patched, nor re-created.
      */
     const serverId = generateMcpServerId(workspaceId, body.url)
-    if (await mcpServerIdExists({ workspaceId, serverId })) {
+    const idState = await getMcpServerIdState({ workspaceId, serverId })
+    if (idState && !idState.deleted) {
       return v2Error(
         'CONFLICT',
         'An MCP server with this URL already exists in this workspace. Update it with PATCH /api/v2/mcp-servers/{id}.'
       )
     }
+    const revivingSoftDeleted = idState?.deleted === true
 
     const result = await performCreateMcpServer({
       workspaceId,
@@ -138,8 +145,12 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
       return v2McpOrchestrationError(result.errorCode, result.error ?? 'Failed to register server')
     }
 
-    // A concurrent create won the id race and the lib upserted onto it.
-    if (result.updated) {
+    /**
+     * `updated` means the lib wrote onto an existing row. Reviving the
+     * soft-deleted row we already saw is the intended outcome; otherwise a
+     * concurrent create won the id race between the check above and the write.
+     */
+    if (result.updated && !revivingSoftDeleted) {
       return v2Error('CONFLICT', 'An MCP server with this URL already exists in this workspace.')
     }
 

apps/sim/lib/mcp/queries.ts

@@ -41,19 +41,23 @@ export async function getWorkspaceMcpServer(params: {
 }
 
 /**
- * Whether a row already occupies the deterministic id derived from a workspace
- * and URL — soft-deleted rows included, because the create path revives rather
- * than inserts alongside them. Lets a caller reject a duplicate registration
- * before the upsert in `performCreateMcpServer` overwrites the existing row.
+ * The state of the row occupying the deterministic id derived from a workspace
+ * and URL, or null when the id is free.
+ *
+ * The soft-deleted case has to be distinguished rather than merged into "taken":
+ * `performCreateMcpServer` revives such a row instead of inserting alongside it,
+ * so reporting it as a duplicate would make a soft-deleted URL permanently
+ * unusable — it cannot be fetched or patched either, since those resolve live
+ * rows only.
  */
-export async function mcpServerIdExists(params: {
+export async function getMcpServerIdState(params: {
   workspaceId: string
   serverId: string
-}): Promise<boolean> {
+}): Promise<{ deleted: boolean } | null> {
   const [row] = await db
-    .select({ id: mcpServers.id })
+    .select({ deletedAt: mcpServers.deletedAt })
     .from(mcpServers)
     .where(and(eq(mcpServers.id, params.serverId), eq(mcpServers.workspaceId, params.workspaceId)))
     .limit(1)
-  return Boolean(row)
+  return row ? { deleted: row.deletedAt !== null } : null
 }

apps/sim/lib/skills/orchestration/skill-lifecycle.ts

@@ -1,7 +1,7 @@
 import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit'
 import type { skill } from '@sim/db/schema'
 import { createLogger } from '@sim/logger'
-import { getErrorMessage } from '@sim/utils/errors'
+import { getErrorMessage, getPostgresErrorCode } from '@sim/utils/errors'
 import type { NextRequest } from 'next/server'
 import type { z } from 'zod'
 import {
@@ -151,9 +151,21 @@ async function resolveEditableSkill(params: {
 /**
  * `upsertSkills` reports name collisions and vanished ids as thrown Errors.
  * Classify them rather than letting every caller re-match the message.
+ *
+ * The `23505` arm covers the race its in-transaction name `SELECT` cannot: two
+ * concurrent creates (or renames) both pass that check, and the loser is rejected
+ * by `skill_workspace_name_unique` as a raw Postgres error whose message matches
+ * nothing here — which would otherwise surface as a 500 for what is a conflict.
  */
 function classifyUpsertError(error: unknown): PerformSkillResult {
   const message = getErrorMessage(error, 'Failed to save skill')
+  if (getPostgresErrorCode(error) === '23505') {
+    return {
+      success: false,
+      error: 'That skill name is unavailable in this workspace',
+      errorCode: 'conflict',
+    }
+  }
   if (message.includes('is unavailable')) {
     return { success: false, error: message, errorCode: 'conflict' }
   }