refactor(knowledge): make lib/knowledge/orchestration the single implementation - #6154
Conversation
…ementation
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.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Orchestration layer owns guards, Failure handling uses Defaults & contracts: Copilot fixes: KB delete distinguishes failed vs not found; Behavior change: v1/v2 KB PUT with only Reviewed by Cursor Bugbot for commit 1ba0215. Bugbot is set up for automated code reviews on this repo. Configure here. |
Greptile SummaryThe PR consolidates knowledge-base, document, and connector mutations behind shared orchestration functions used by API versions and the copilot tool.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; both previously reported defects are fixed at the current head.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/knowledge/orchestration/connectors.ts | Centralizes connector create, update, delete, and sync behavior; the previously reported billing-attribution ordering defect is fixed before durable effects. |
| apps/sim/lib/copilot/tools/server/knowledge/knowledge-base.ts | Replaces direct service calls and internal HTTP connector calls with orchestration and correctly separates non-not-found deletion failures. |
| apps/sim/app/api/knowledge/[id]/connectors/route.ts | Delegates connector creation to shared orchestration while preserving route-level authentication and billing attribution. |
| apps/sim/app/api/knowledge/[id]/connectors/[connectorId]/sync/route.ts | Delegates manual connector sync to orchestration with billing attribution resolved before audit and dispatch effects. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
UI[Internal and versioned API routes] --> O[Knowledge orchestration]
Copilot[Copilot knowledge tool] --> O
O --> Guards[Authorization, billing, and validation guards]
Guards --> Services[Knowledge services and database mutations]
Services --> Audit[Audit and telemetry]
Services --> Queue[Document processing and connector sync queues]
Reviews (2): Last reviewed commit: "fix(knowledge): make connector create at..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…ures 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.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1ba0215. Configure here.
1 participant
Summary
lib/knowledge/orchestration/**so the internal route, v1, v2, and the copilot tool share one implementation — same shape aslib/table/orchestration. Services write, orchestration guards/audits/classifies.minSizeto 1 against the API's 100, so identical input chunked differently depending on who created the KB. Agent path now chunks at 100.recordAuditappeared 0 times in the copilot tool, so agent-created KBs, uploads, updates and deletes left no audit trail.OrchestrationErrorsubclasses; storage limits throw a sharedStorageLimitExceededError. Removes four separate greps for"already exists"/"does not have permission"/"storage limit".delete_connectorreporting the opposite of what happened — it reached the route via an internal HTTP self-call with no query string, so the keep-documents default always applied while the agent said documents were removed. Self-call deleted, all four connector ops run in-process, real counts returned.Notes
OrchestrationErrorCodegainspayload_too_large(413). Without it, dropping the storage-limit message match would have regressed the documented 413 on KB create + document upload to a 500.messageForOrchestrationErrorkeeps a driver's message off a 500 response.actorUserId(the service requires it for a workspace move; both omitted it).deleteDocumentsthroughparseRequest; its contract saidz.boolean(), which rejects the string a query param actually is.POST /api/v2/knowledge/{id}/documentsin the OpenAPI spec — nothing on that path throws a conflict.Behavior change: a v1/v2
PUTcarrying only theworkspaceIdscope field and no updates now returns 400 instead of 200 with the unchanged KB.Deferred: document update stays internal-only.
performUpdateKnowledgeDocumentmakes exposing it a contract + route away, but that's new public surface, not consolidation.Inherited CI failure:
check:api-validation:strictfails ondoubleCasts 9 vs baseline 8. Verified identical onimprovement/v2-endpointswith none of this branch's changes — all 9 casts are intools/hosting.ts,contracts/tables.ts,executor/utils/errors.ts, and the logger/SDK packages. Not touched here.Type of Change
Testing
bunx vitest run lib/knowledge app/api/knowledge app/api/v1/knowledge app/api/v2/knowledge lib/copilot— 118 files, 1387 tests passingbun run type-check— cleanbun run lint:check— cleanbun run check:api-validation— passescheck:api-validation:strictnoted aboveChecklist