Skip to content

Commit bc451af

Browse files
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.
1 parent 5fea5f7 commit bc451af

48 files changed

Lines changed: 3665 additions & 1928 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)