Skip to content

Commit 1ba0215

Browse files
TheodoreSpeaksTheodoreSpeaks
committed
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.
1 parent bc451af commit 1ba0215

8 files changed

Lines changed: 235 additions & 30 deletions

File tree

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

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import {
2222
type KnowledgeConnectorRow,
2323
performDeleteKnowledgeConnector,
2424
performUpdateKnowledgeConnector,
25+
type SourceConfigRejection,
2526
} from '@/lib/knowledge/orchestration'
2627
import { refreshAccessTokenIfNeeded } from '@/app/api/auth/oauth/utils'
2728
import { checkKnowledgeBaseAccess, checkKnowledgeBaseWriteAccess } from '@/app/api/knowledge/utils'
@@ -94,28 +95,43 @@ function makeSourceConfigValidator(
9495
return async (
9596
connector: KnowledgeConnectorRow,
9697
sourceConfig: Record<string, unknown>
97-
): Promise<string | null> => {
98+
): Promise<SourceConfigRejection | null> => {
9899
const connectorConfig = CONNECTOR_REGISTRY[connector.connectorType]
99100
if (!connectorConfig) {
100-
return `Unknown connector type: ${connector.connectorType}`
101+
return {
102+
message: `Unknown connector type: ${connector.connectorType}`,
103+
errorCode: 'validation',
104+
}
101105
}
102106

103107
let accessToken: string | null = null
104108
if (connectorConfig.auth.mode === 'apiKey') {
105109
if (!connector.encryptedApiKey) {
106-
return 'API key not found. Please reconfigure the connector.'
110+
return {
111+
message: 'API key not found. Please reconfigure the connector.',
112+
errorCode: 'validation',
113+
}
107114
}
108115
accessToken = (await decryptApiKey(connector.encryptedApiKey)).decrypted
109116
} else {
110117
if (!connector.credentialId) {
111-
return 'OAuth credential not found. Please reconfigure the connector.'
118+
return {
119+
message: 'OAuth credential not found. Please reconfigure the connector.',
120+
errorCode: 'validation',
121+
}
112122
}
113123
if (!workspaceId) {
114-
return 'Knowledge base is missing workspace context'
124+
return {
125+
message: 'Knowledge base is missing workspace context',
126+
errorCode: 'conflict',
127+
}
115128
}
116129
const identity = await resolveCredentialTokenIdentity(connector.credentialId, workspaceId)
117130
if (!identity) {
118-
return 'Credential is no longer usable in this workspace. Please reconnect it.'
131+
return {
132+
message: 'Credential is no longer usable in this workspace. Please reconnect it.',
133+
errorCode: 'validation',
134+
}
119135
}
120136
accessToken = await refreshAccessTokenIfNeeded(
121137
connector.credentialId,
@@ -126,11 +142,19 @@ function makeSourceConfigValidator(
126142
}
127143

128144
if (!accessToken) {
129-
return 'Failed to refresh access token. Please reconnect your account.'
145+
// A stale stored credential, not an unauthenticated caller — but the route
146+
// has always answered 401 here, so keep that rather than silently
147+
// reclassifying it as part of this refactor.
148+
return {
149+
message: 'Failed to refresh access token. Please reconnect your account.',
150+
errorCode: 'unauthorized',
151+
}
130152
}
131153

132154
const validation = await connectorConfig.validateConfig(accessToken, sourceConfig)
133-
return validation.valid ? null : validation.error || 'Invalid source configuration'
155+
return validation.valid
156+
? null
157+
: { message: validation.error || 'Invalid source configuration', errorCode: 'validation' }
134158
}
135159
}
136160

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,7 @@ export function decodeCursor<T = Record<string, unknown>>(cursor: string): T | n
159159

160160
const V2_CODE_BY_ORCHESTRATION_ERROR: Record<OrchestrationErrorCode, V2ErrorCode> = {
161161
validation: 'BAD_REQUEST',
162+
unauthorized: 'UNAUTHORIZED',
162163
forbidden: 'FORBIDDEN',
163164
not_found: 'NOT_FOUND',
164165
conflict: 'CONFLICT',

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

Lines changed: 53 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,17 @@ import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
88
const {
99
mockAssertBillingAttributionSnapshot,
1010
mockCheckKnowledgeBaseWriteAccess,
11+
mockGetKnowledgeBaseById,
1112
mockPerformCreateKnowledgeConnector,
13+
mockPerformDeleteKnowledgeBase,
1214
mockPerformDeleteKnowledgeConnector,
1315
mockPerformSyncKnowledgeConnector,
1416
} = vi.hoisted(() => ({
1517
mockAssertBillingAttributionSnapshot: vi.fn(),
1618
mockCheckKnowledgeBaseWriteAccess: vi.fn(),
19+
mockGetKnowledgeBaseById: vi.fn(),
1720
mockPerformCreateKnowledgeConnector: vi.fn(),
21+
mockPerformDeleteKnowledgeBase: vi.fn(),
1822
mockPerformDeleteKnowledgeConnector: vi.fn(),
1923
mockPerformSyncKnowledgeConnector: vi.fn(),
2024
}))
@@ -38,8 +42,8 @@ vi.mock('@/lib/knowledge/embeddings', () => ({
3842
}))
3943
vi.mock('@/lib/knowledge/orchestration', () => ({
4044
performCreateKnowledgeBase: vi.fn(),
45+
performDeleteKnowledgeBase: mockPerformDeleteKnowledgeBase,
4146
performCreateKnowledgeConnector: mockPerformCreateKnowledgeConnector,
42-
performDeleteKnowledgeBase: vi.fn(),
4347
performDeleteKnowledgeConnector: mockPerformDeleteKnowledgeConnector,
4448
performDeleteKnowledgeDocument: vi.fn(),
4549
performSyncKnowledgeConnector: mockPerformSyncKnowledgeConnector,
@@ -49,7 +53,7 @@ vi.mock('@/lib/knowledge/orchestration', () => ({
4953
performUploadKnowledgeDocument: vi.fn(),
5054
}))
5155
vi.mock('@/lib/knowledge/service', () => ({
52-
getKnowledgeBaseById: vi.fn(),
56+
getKnowledgeBaseById: mockGetKnowledgeBaseById,
5357
}))
5458
vi.mock('@/lib/knowledge/tags/service', () => ({
5559
createTagDefinition: vi.fn(),
@@ -156,6 +160,53 @@ describe('knowledge base connector Copilot operations', () => {
156160
expect(mockAssertBillingAttributionSnapshot).toHaveBeenCalledWith(BILLING_ATTRIBUTION)
157161
})
158162

163+
it('reports a failed knowledge base delete as failed, not as missing', async () => {
164+
mockGetKnowledgeBaseById.mockResolvedValue({
165+
id: 'knowledge-base-1',
166+
name: 'Paid KB',
167+
workspaceId: 'workspace-paid',
168+
})
169+
mockPerformDeleteKnowledgeBase.mockResolvedValue({
170+
success: false,
171+
error: 'Knowledge base is locked',
172+
errorCode: 'conflict',
173+
})
174+
175+
const result = await knowledgeBaseServerTool.execute(
176+
{ operation: 'delete', args: { knowledgeBaseId: 'knowledge-base-1' } },
177+
CONTEXT
178+
)
179+
180+
// A knowledge base that exists but could not be archived is neither deleted
181+
// nor missing — folding it into notFound told the user it was never there.
182+
expect(result.data.notFound).toEqual([])
183+
expect(result.data.failed).toEqual([
184+
{ id: 'knowledge-base-1', name: 'Paid KB', reason: 'Knowledge base is locked' },
185+
])
186+
expect(result.message).toContain('Knowledge base is locked')
187+
})
188+
189+
it('never relays an unclassified fault to the agent verbatim', async () => {
190+
mockGetKnowledgeBaseById.mockResolvedValue({
191+
id: 'knowledge-base-1',
192+
name: 'Paid KB',
193+
workspaceId: 'workspace-paid',
194+
})
195+
mockPerformDeleteKnowledgeBase.mockResolvedValue({
196+
success: false,
197+
error: 'select "id" from "knowledge_base" — connection terminated',
198+
errorCode: 'internal',
199+
})
200+
201+
const result = await knowledgeBaseServerTool.execute(
202+
{ operation: 'delete', args: { knowledgeBaseId: 'knowledge-base-1' } },
203+
CONTEXT
204+
)
205+
206+
expect(result.data.failed[0].reason).toBe('Failed to delete knowledge base')
207+
expect(result.message).not.toContain('connection terminated')
208+
})
209+
159210
it('reports that a deleted connector kept its documents, because it did', async () => {
160211
const result = await knowledgeBaseServerTool.execute(
161212
{ operation: 'delete_connector', args: { connectorId: 'connector-1' } },

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

Lines changed: 67 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,10 @@ import {
1818
type BaseServerTool,
1919
type ServerToolContext,
2020
} from '@/lib/copilot/tools/server/base-tool'
21+
import {
22+
messageForOrchestrationError,
23+
type OrchestrationErrorCode,
24+
} from '@/lib/core/orchestration/types'
2125
import { generateSearchEmbedding, recordSearchEmbeddingUsage } from '@/lib/knowledge/embeddings'
2226
import {
2327
performCreateKnowledgeBase,
@@ -67,6 +71,20 @@ function requireKnowledgeBillingAttribution(
6771
return attribution
6872
}
6973

74+
/**
75+
* The message the agent — and therefore the user — is shown for a failed
76+
* operation. Mirrors `messageForOrchestrationError` on the HTTP surfaces: a
77+
* classified failure is caller-fixable and safe to relay, an unclassified one
78+
* carries whatever text the fault happened to have (a driver's failed SQL, say)
79+
* and is replaced by the operation's own wording.
80+
*/
81+
function agentFacingError(
82+
outcome: { error?: string; errorCode?: OrchestrationErrorCode },
83+
fallback: string
84+
): string {
85+
return messageForOrchestrationError(outcome, fallback)
86+
}
87+
7088
type KnowledgeBaseArgs = {
7189
operation: string
7290
args?: Record<string, any>
@@ -142,7 +160,10 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg
142160
chunkingConfig: args.chunkingConfig,
143161
})
144162
if (!outcome.success) {
145-
return { success: false, message: outcome.error }
163+
return {
164+
success: false,
165+
message: agentFacingError(outcome, 'Failed to create knowledge base'),
166+
}
146167
}
147168

148169
const newKnowledgeBase = outcome.knowledgeBase
@@ -446,7 +467,10 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg
446467
updates,
447468
})
448469
if (!outcome.success) {
449-
return { success: false, message: outcome.error }
470+
return {
471+
success: false,
472+
message: agentFacingError(outcome, 'Failed to update knowledge base'),
473+
}
450474
}
451475

452476
const updatedKb = outcome.knowledgeBase
@@ -476,6 +500,10 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg
476500

477501
const deleted: Array<{ id: string; name: string }> = []
478502
const notFound: string[] = []
503+
// A knowledge base that exists but could not be archived is neither
504+
// deleted nor missing. Folding it into `notFound` told the user it was
505+
// never there instead of why the delete failed.
506+
const failed: Array<{ id: string; name: string; reason: string }> = []
479507

480508
for (const kbId of kbIds) {
481509
const writeAccess = await checkKnowledgeBaseWriteAccess(kbId, context.userId)
@@ -501,19 +529,33 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg
501529
},
502530
})
503531
if (!outcome.success) {
504-
notFound.push(kbId)
532+
if (outcome.errorCode === 'not_found') {
533+
notFound.push(kbId)
534+
} else {
535+
failed.push({
536+
id: kbId,
537+
name: kbToDelete.name,
538+
reason: agentFacingError(outcome, 'Failed to delete knowledge base'),
539+
})
540+
}
505541
continue
506542
}
507543
deleted.push({ id: kbId, name: kbToDelete.name })
508544
}
509545

546+
const deleteSummary = [
547+
deleted.length > 0 ? `Deleted: ${deleted.map((d) => d.name).join(', ')}` : null,
548+
failed.length > 0
549+
? `Failed: ${failed.map((f) => `${f.name} (${f.reason})`).join(', ')}`
550+
: null,
551+
]
552+
.filter(Boolean)
553+
.join('. ')
554+
510555
return {
511556
success: deleted.length > 0,
512-
message:
513-
deleted.length > 0
514-
? `Deleted: ${deleted.map((d) => d.name).join(', ')}`
515-
: 'No knowledge bases found',
516-
data: { deleted, notFound },
557+
message: deleteSummary || 'No knowledge bases found',
558+
data: { deleted, notFound, failed },
517559
}
518560
}
519561

@@ -611,7 +653,10 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg
611653
updates: updateData,
612654
})
613655
if (!outcome.success) {
614-
return { success: false, message: outcome.error }
656+
return {
657+
success: false,
658+
message: agentFacingError(outcome, 'Failed to update document'),
659+
}
615660
}
616661

617662
return {
@@ -929,7 +974,7 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg
929974
?.accessToken ?? null,
930975
})
931976
if (!outcome.success) {
932-
return { success: false, message: outcome.error }
977+
return { success: false, message: agentFacingError(outcome, 'Failed to add connector') }
933978
}
934979

935980
const connector = outcome.connector
@@ -982,7 +1027,10 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg
9821027
updates,
9831028
})
9841029
if (!outcome.success) {
985-
return { success: false, message: outcome.error }
1030+
return {
1031+
success: false,
1032+
message: agentFacingError(outcome, 'Failed to update connector'),
1033+
}
9861034
}
9871035

9881036
return {
@@ -1019,7 +1067,10 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg
10191067
connectorId: args.connectorId,
10201068
})
10211069
if (!outcome.success) {
1022-
return { success: false, message: outcome.error }
1070+
return {
1071+
success: false,
1072+
message: agentFacingError(outcome, 'Failed to delete connector'),
1073+
}
10231074
}
10241075

10251076
// Report what the delete actually did. The documents are kept — this
@@ -1079,7 +1130,10 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg
10791130
resolveBillingAttribution: async () => billingAttribution,
10801131
})
10811132
if (!outcome.success) {
1082-
return { success: false, message: outcome.error }
1133+
return {
1134+
success: false,
1135+
message: agentFacingError(outcome, 'Failed to sync connector'),
1136+
}
10831137
}
10841138

10851139
return {

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
11
export type OrchestrationErrorCode =
22
| 'validation'
3+
/**
4+
* The credentials this operation depends on are no longer usable — a stored
5+
* third-party token that will not refresh, not an unauthenticated caller.
6+
* Distinct from `forbidden`, which is the caller lacking permission.
7+
*/
8+
| 'unauthorized'
39
| 'not_found'
410
| 'forbidden'
511
| 'conflict'
@@ -14,6 +20,7 @@ export type OrchestrationErrorCode =
1420
*/
1521
export function statusForOrchestrationError(code: OrchestrationErrorCode | undefined): number {
1622
if (code === 'validation') return 400
23+
if (code === 'unauthorized') return 401
1724
if (code === 'forbidden') return 403
1825
if (code === 'not_found') return 404
1926
if (code === 'conflict') return 409

0 commit comments

Comments
 (0)