fix(knowledge): make connector create atomic and stop flattening failures

Review round 1 on #6154.

- Resolve the billing payer before the connector is committed, not after. A
  malformed attribution header rejected post-commit left a live connector behind
  a 500, and a retry created a duplicate plus duplicate sync work. Manual sync
  resolves before writing its audit for the same reason.
- Let the source-config validator carry its own failure class. Collapsing every
  rejection to `validation` flattened the connector PATCH route's 401 (stale
  stored credential) and 409 (missing workspace context) into a 400.
- Add `unauthorized` to OrchestrationErrorCode. It is the class that 401 was
  already expressing on this route, and the v2 vocabulary already had
  UNAUTHORIZED; only the shared union was missing it.
- Report a knowledge base that exists but failed to archive as failed, with the
  reason, rather than as not found. The copilot delete loop folded every
  non-not-found failure into `notFound`, telling the user it was never there.
- Route copilot failures through the same message helper the HTTP surfaces use,
  so an unclassified fault's raw text (a driver's failed SQL) no longer reaches
  the agent verbatim while the UI and public APIs get the generic wording.

Author: TheodoreSpeaks

apps/sim/app/api/knowledge/[id]/connectors/[connectorId]/route.ts

@@ -22,6 +22,7 @@ import {
   type KnowledgeConnectorRow,
   performDeleteKnowledgeConnector,
   performUpdateKnowledgeConnector,
+  type SourceConfigRejection,
 } from '@/lib/knowledge/orchestration'
 import { refreshAccessTokenIfNeeded } from '@/app/api/auth/oauth/utils'
 import { checkKnowledgeBaseAccess, checkKnowledgeBaseWriteAccess } from '@/app/api/knowledge/utils'
@@ -94,28 +95,43 @@ function makeSourceConfigValidator(
   return async (
     connector: KnowledgeConnectorRow,
     sourceConfig: Record<string, unknown>
-  ): Promise<string | null> => {
+  ): Promise<SourceConfigRejection | null> => {
     const connectorConfig = CONNECTOR_REGISTRY[connector.connectorType]
     if (!connectorConfig) {
-      return `Unknown connector type: ${connector.connectorType}`
+      return {
+        message: `Unknown connector type: ${connector.connectorType}`,
+        errorCode: 'validation',
+      }
     }
 
     let accessToken: string | null = null
     if (connectorConfig.auth.mode === 'apiKey') {
       if (!connector.encryptedApiKey) {
-        return 'API key not found. Please reconfigure the connector.'
+        return {
+          message: 'API key not found. Please reconfigure the connector.',
+          errorCode: 'validation',
+        }
       }
       accessToken = (await decryptApiKey(connector.encryptedApiKey)).decrypted
     } else {
       if (!connector.credentialId) {
-        return 'OAuth credential not found. Please reconfigure the connector.'
+        return {
+          message: 'OAuth credential not found. Please reconfigure the connector.',
+          errorCode: 'validation',
+        }
       }
       if (!workspaceId) {
-        return 'Knowledge base is missing workspace context'
+        return {
+          message: 'Knowledge base is missing workspace context',
+          errorCode: 'conflict',
+        }
       }
       const identity = await resolveCredentialTokenIdentity(connector.credentialId, workspaceId)
       if (!identity) {
-        return 'Credential is no longer usable in this workspace. Please reconnect it.'
+        return {
+          message: 'Credential is no longer usable in this workspace. Please reconnect it.',
+          errorCode: 'validation',
+        }
       }
       accessToken = await refreshAccessTokenIfNeeded(
         connector.credentialId,
@@ -126,11 +142,19 @@ function makeSourceConfigValidator(
     }
 
     if (!accessToken) {
-      return 'Failed to refresh access token. Please reconnect your account.'
+      // A stale stored credential, not an unauthenticated caller — but the route
+      // has always answered 401 here, so keep that rather than silently
+      // reclassifying it as part of this refactor.
+      return {
+        message: 'Failed to refresh access token. Please reconnect your account.',
+        errorCode: 'unauthorized',
+      }
     }
 
     const validation = await connectorConfig.validateConfig(accessToken, sourceConfig)
-    return validation.valid ? null : validation.error || 'Invalid source configuration'
+    return validation.valid
+      ? null
+      : { message: validation.error || 'Invalid source configuration', errorCode: 'validation' }
   }
 }
 

apps/sim/app/api/v2/lib/response.ts

@@ -159,6 +159,7 @@ export function decodeCursor<T = Record<string, unknown>>(cursor: string): T | n
 
 const V2_CODE_BY_ORCHESTRATION_ERROR: Record<OrchestrationErrorCode, V2ErrorCode> = {
   validation: 'BAD_REQUEST',
+  unauthorized: 'UNAUTHORIZED',
   forbidden: 'FORBIDDEN',
   not_found: 'NOT_FOUND',
   conflict: 'CONFLICT',

apps/sim/lib/copilot/tools/server/knowledge/knowledge-base.test.ts

@@ -8,13 +8,17 @@ import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
 const {
   mockAssertBillingAttributionSnapshot,
   mockCheckKnowledgeBaseWriteAccess,
+  mockGetKnowledgeBaseById,
   mockPerformCreateKnowledgeConnector,
+  mockPerformDeleteKnowledgeBase,
   mockPerformDeleteKnowledgeConnector,
   mockPerformSyncKnowledgeConnector,
 } = vi.hoisted(() => ({
   mockAssertBillingAttributionSnapshot: vi.fn(),
   mockCheckKnowledgeBaseWriteAccess: vi.fn(),
+  mockGetKnowledgeBaseById: vi.fn(),
   mockPerformCreateKnowledgeConnector: vi.fn(),
+  mockPerformDeleteKnowledgeBase: vi.fn(),
   mockPerformDeleteKnowledgeConnector: vi.fn(),
   mockPerformSyncKnowledgeConnector: vi.fn(),
 }))
@@ -38,8 +42,8 @@ vi.mock('@/lib/knowledge/embeddings', () => ({
 }))
 vi.mock('@/lib/knowledge/orchestration', () => ({
   performCreateKnowledgeBase: vi.fn(),
+  performDeleteKnowledgeBase: mockPerformDeleteKnowledgeBase,
   performCreateKnowledgeConnector: mockPerformCreateKnowledgeConnector,
-  performDeleteKnowledgeBase: vi.fn(),
   performDeleteKnowledgeConnector: mockPerformDeleteKnowledgeConnector,
   performDeleteKnowledgeDocument: vi.fn(),
   performSyncKnowledgeConnector: mockPerformSyncKnowledgeConnector,
@@ -49,7 +53,7 @@ vi.mock('@/lib/knowledge/orchestration', () => ({
   performUploadKnowledgeDocument: vi.fn(),
 }))
 vi.mock('@/lib/knowledge/service', () => ({
-  getKnowledgeBaseById: vi.fn(),
+  getKnowledgeBaseById: mockGetKnowledgeBaseById,
 }))
 vi.mock('@/lib/knowledge/tags/service', () => ({
   createTagDefinition: vi.fn(),
@@ -156,6 +160,53 @@ describe('knowledge base connector Copilot operations', () => {
     expect(mockAssertBillingAttributionSnapshot).toHaveBeenCalledWith(BILLING_ATTRIBUTION)
   })
 
+  it('reports a failed knowledge base delete as failed, not as missing', async () => {
+    mockGetKnowledgeBaseById.mockResolvedValue({
+      id: 'knowledge-base-1',
+      name: 'Paid KB',
+      workspaceId: 'workspace-paid',
+    })
+    mockPerformDeleteKnowledgeBase.mockResolvedValue({
+      success: false,
+      error: 'Knowledge base is locked',
+      errorCode: 'conflict',
+    })
+
+    const result = await knowledgeBaseServerTool.execute(
+      { operation: 'delete', args: { knowledgeBaseId: 'knowledge-base-1' } },
+      CONTEXT
+    )
+
+    // A knowledge base that exists but could not be archived is neither deleted
+    // nor missing — folding it into notFound told the user it was never there.
+    expect(result.data.notFound).toEqual([])
+    expect(result.data.failed).toEqual([
+      { id: 'knowledge-base-1', name: 'Paid KB', reason: 'Knowledge base is locked' },
+    ])
+    expect(result.message).toContain('Knowledge base is locked')
+  })
+
+  it('never relays an unclassified fault to the agent verbatim', async () => {
+    mockGetKnowledgeBaseById.mockResolvedValue({
+      id: 'knowledge-base-1',
+      name: 'Paid KB',
+      workspaceId: 'workspace-paid',
+    })
+    mockPerformDeleteKnowledgeBase.mockResolvedValue({
+      success: false,
+      error: 'select "id" from "knowledge_base" — connection terminated',
+      errorCode: 'internal',
+    })
+
+    const result = await knowledgeBaseServerTool.execute(
+      { operation: 'delete', args: { knowledgeBaseId: 'knowledge-base-1' } },
+      CONTEXT
+    )
+
+    expect(result.data.failed[0].reason).toBe('Failed to delete knowledge base')
+    expect(result.message).not.toContain('connection terminated')
+  })
+
   it('reports that a deleted connector kept its documents, because it did', async () => {
     const result = await knowledgeBaseServerTool.execute(
       { operation: 'delete_connector', args: { connectorId: 'connector-1' } },

apps/sim/lib/copilot/tools/server/knowledge/knowledge-base.ts

@@ -18,6 +18,10 @@ import {
   type BaseServerTool,
   type ServerToolContext,
 } from '@/lib/copilot/tools/server/base-tool'
+import {
+  messageForOrchestrationError,
+  type OrchestrationErrorCode,
+} from '@/lib/core/orchestration/types'
 import { generateSearchEmbedding, recordSearchEmbeddingUsage } from '@/lib/knowledge/embeddings'
 import {
   performCreateKnowledgeBase,
@@ -67,6 +71,20 @@ function requireKnowledgeBillingAttribution(
   return attribution
 }
 
+/**
+ * The message the agent — and therefore the user — is shown for a failed
+ * operation. Mirrors `messageForOrchestrationError` on the HTTP surfaces: a
+ * classified failure is caller-fixable and safe to relay, an unclassified one
+ * carries whatever text the fault happened to have (a driver's failed SQL, say)
+ * and is replaced by the operation's own wording.
+ */
+function agentFacingError(
+  outcome: { error?: string; errorCode?: OrchestrationErrorCode },
+  fallback: string
+): string {
+  return messageForOrchestrationError(outcome, fallback)
+}
+
 type KnowledgeBaseArgs = {
   operation: string
   args?: Record<string, any>
@@ -142,7 +160,10 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg
             chunkingConfig: args.chunkingConfig,
           })
           if (!outcome.success) {
-            return { success: false, message: outcome.error }
+            return {
+              success: false,
+              message: agentFacingError(outcome, 'Failed to create knowledge base'),
+            }
           }
 
           const newKnowledgeBase = outcome.knowledgeBase
@@ -446,7 +467,10 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg
             updates,
           })
           if (!outcome.success) {
-            return { success: false, message: outcome.error }
+            return {
+              success: false,
+              message: agentFacingError(outcome, 'Failed to update knowledge base'),
+            }
           }
 
           const updatedKb = outcome.knowledgeBase
@@ -476,6 +500,10 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg
 
           const deleted: Array<{ id: string; name: string }> = []
           const notFound: string[] = []
+          // A knowledge base that exists but could not be archived is neither
+          // deleted nor missing. Folding it into `notFound` told the user it was
+          // never there instead of why the delete failed.
+          const failed: Array<{ id: string; name: string; reason: string }> = []
 
           for (const kbId of kbIds) {
             const writeAccess = await checkKnowledgeBaseWriteAccess(kbId, context.userId)
@@ -501,19 +529,33 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg
               },
             })
             if (!outcome.success) {
-              notFound.push(kbId)
+              if (outcome.errorCode === 'not_found') {
+                notFound.push(kbId)
+              } else {
+                failed.push({
+                  id: kbId,
+                  name: kbToDelete.name,
+                  reason: agentFacingError(outcome, 'Failed to delete knowledge base'),
+                })
+              }
               continue
             }
             deleted.push({ id: kbId, name: kbToDelete.name })
           }
 
+          const deleteSummary = [
+            deleted.length > 0 ? `Deleted: ${deleted.map((d) => d.name).join(', ')}` : null,
+            failed.length > 0
+              ? `Failed: ${failed.map((f) => `${f.name} (${f.reason})`).join(', ')}`
+              : null,
+          ]
+            .filter(Boolean)
+            .join('. ')
+
           return {
             success: deleted.length > 0,
-            message:
-              deleted.length > 0
-                ? `Deleted: ${deleted.map((d) => d.name).join(', ')}`
-                : 'No knowledge bases found',
-            data: { deleted, notFound },
+            message: deleteSummary || 'No knowledge bases found',
+            data: { deleted, notFound, failed },
           }
         }
 
@@ -611,7 +653,10 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg
             updates: updateData,
           })
           if (!outcome.success) {
-            return { success: false, message: outcome.error }
+            return {
+              success: false,
+              message: agentFacingError(outcome, 'Failed to update document'),
+            }
           }
 
           return {
@@ -929,7 +974,7 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg
                 ?.accessToken ?? null,
           })
           if (!outcome.success) {
-            return { success: false, message: outcome.error }
+            return { success: false, message: agentFacingError(outcome, 'Failed to add connector') }
           }
 
           const connector = outcome.connector
@@ -982,7 +1027,10 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg
             updates,
           })
           if (!outcome.success) {
-            return { success: false, message: outcome.error }
+            return {
+              success: false,
+              message: agentFacingError(outcome, 'Failed to update connector'),
+            }
           }
 
           return {
@@ -1019,7 +1067,10 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg
             connectorId: args.connectorId,
           })
           if (!outcome.success) {
-            return { success: false, message: outcome.error }
+            return {
+              success: false,
+              message: agentFacingError(outcome, 'Failed to delete connector'),
+            }
           }
 
           // Report what the delete actually did. The documents are kept — this
@@ -1079,7 +1130,10 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg
             resolveBillingAttribution: async () => billingAttribution,
           })
           if (!outcome.success) {
-            return { success: false, message: outcome.error }
+            return {
+              success: false,
+              message: agentFacingError(outcome, 'Failed to sync connector'),
+            }
           }
 
           return {

apps/sim/lib/core/orchestration/types.ts

@@ -1,5 +1,11 @@
 export type OrchestrationErrorCode =
   | 'validation'
+  /**
+   * The credentials this operation depends on are no longer usable — a stored
+   * third-party token that will not refresh, not an unauthenticated caller.
+   * Distinct from `forbidden`, which is the caller lacking permission.
+   */
+  | 'unauthorized'
   | 'not_found'
   | 'forbidden'
   | 'conflict'
@@ -14,6 +20,7 @@ export type OrchestrationErrorCode =
  */
 export function statusForOrchestrationError(code: OrchestrationErrorCode | undefined): number {
   if (code === 'validation') return 400
+  if (code === 'unauthorized') return 401
   if (code === 'forbidden') return 403
   if (code === 'not_found') return 404
   if (code === 'conflict') return 409