Skip to content

Commit edd9a06

Browse files
refactor(knowledge): make lib/knowledge/orchestration the single implementation (#6154)
* refactor(knowledge): make lib/knowledge/orchestration the single implementation Knowledge base create was implemented four times — the internal route, v1, v2, and the copilot tool — and the orchestration around the shared write had drifted. Extract it the same way lib/table/orchestration was: services write, orchestration decides which writes run, guards them, audits them, and returns a transport-neutral failure. Behavior converged, not preserved: - One chunking default (DEFAULT_CHUNKING_CONFIG). The agent defaulted minSize to 1 against the API's 100, so identical input produced differently-chunked knowledge bases depending on who created it. The agent path now chunks at 100. - Every successful mutation is audited inside the orchestration function. The copilot tool called recordAudit zero times, so agent-created knowledge bases, document uploads, updates and deletes left no audit trail at all. - Failures classify by class, not by message text. The knowledge service errors are OrchestrationError subclasses and storage-quota rejections throw a shared StorageLimitExceededError, replacing four separate message greps for "already exists" / "does not have permission" / "storage limit". delete_connector reported the opposite of what happened. It reached the route through an internal HTTP self-call that sent no query string, so the route's keep-documents default always applied while the agent told the user the documents had been removed. The self-call is gone — all four connector operations run in-process — and the orchestration returns the real counts. Also: - OrchestrationErrorCode gains 'payload_too_large' (413 / PAYLOAD_TOO_LARGE). Without it, dropping the storage-limit message match would have regressed the documented 413 on knowledge base create and document upload to a 500. - messageForOrchestrationError renders a route's own wording for an unclassified fault, so a driver's message no longer reaches the client on a 500. - v1 and v2 knowledge base update now forward actorUserId, which the service requires for a workspace move; both omitted it. - The connector DELETE route reads deleteDocuments through parseRequest. Its contract declared z.boolean(), which would have rejected the string a query param actually is. - Drop the 409 from POST /api/v2/knowledge/{id}/documents in the OpenAPI spec. Nothing on the upload path throws a conflict; it was only ever reachable by the message match this change removes. Behavior change worth noting: a v1/v2 PUT carrying only the workspaceId scope field and no actual updates now returns 400 rather than 200 with the unchanged knowledge base. Deliberately deferred: document update remains internal-only. Extracting performUpdateKnowledgeDocument makes exposing it on v1/v2 a contract and a route away, but that is a new public surface rather than part of this consolidation. * 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 45c7d05 commit edd9a06

48 files changed

Lines changed: 3873 additions & 1931 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

apps/docs/openapi-v2-knowledge.json

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -709,9 +709,6 @@
709709
"404": {
710710
"$ref": "#/components/responses/NotFound"
711711
},
712-
"409": {
713-
"$ref": "#/components/responses/Conflict"
714-
},
715712
"413": {
716713
"description": "The uploaded file exceeds the 100 MB limit, or the workspace storage limit has been reached.",
717714
"content": {

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,10 @@ describe('Knowledge Connector By ID API Route', () => {
151151
success: true,
152152
userId: 'user-1',
153153
})
154-
mockCheckWriteAccess.mockResolvedValue({ hasAccess: true })
154+
mockCheckWriteAccess.mockResolvedValue({
155+
hasAccess: true,
156+
knowledgeBase: { workspaceId: 'ws-1', name: 'Test KB' },
157+
})
155158
dbChainMockFns.limit.mockResolvedValueOnce([])
156159

157160
const req = createMockRequest('PATCH', { sourceConfig: { project: 'NEW' } })
@@ -174,7 +177,8 @@ describe('Knowledge Connector By ID API Route', () => {
174177
mockHasWorkspaceLiveSyncAccess.mockResolvedValue(true)
175178

176179
const updatedConnector = { id: 'conn-456', status: 'paused', syncIntervalMinutes: 5 }
177-
dbChainMockFns.limit.mockResolvedValueOnce([updatedConnector])
180+
dbChainMockFns.limit.mockResolvedValueOnce([{ id: 'conn-456', connectorType: 'jira' }])
181+
dbChainMockFns.returning.mockResolvedValueOnce([updatedConnector])
178182

179183
const req = createMockRequest('PATCH', { status: 'paused', syncIntervalMinutes: 5 })
180184
const response = await PATCH(req, { params: mockParams })
@@ -196,6 +200,7 @@ describe('Knowledge Connector By ID API Route', () => {
196200
knowledgeBase: { workspaceId: 'ws-free', name: 'Free KB' },
197201
})
198202
mockHasWorkspaceLiveSyncAccess.mockResolvedValue(false)
203+
dbChainMockFns.limit.mockResolvedValueOnce([{ id: 'conn-456', connectorType: 'jira' }])
199204

200205
const req = createMockRequest('PATCH', { syncIntervalMinutes: 5 })
201206
const response = await PATCH(req, { params: mockParams })

0 commit comments

Comments
 (0)