feat(kb): add production knowledge base ingestion - #5174
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Confidence Score: 4/5Safe to continue W1 work on; none of the new issues are blocking, but three small gaps should be closed before lecturer staging exposure. The core ingestion pipeline — SAS generation, two-phase upload confirmation, row-level locking, optimistic status claiming, and HMAC webhook verification — is well-implemented with race conditions explicitly handled. Three non-blocking gaps were found: the upload SAS is missing the 5-minute startsOn clock-skew backdate that the ingestion SAS already applies; the upload SAS allows HTTP where the ingestion SAS restricts to HTTPS; and the Hatchet task discards the undefined return from dispatchKBIngestion and always succeeds, so onFailure is never triggered in the dispatch-race path, leaving a narrow window where a QUEUED resource with no externalWorkflowRunId is skipped by the monitor. Files Needing Attention: packages/graphql/src/services/knowledge.ts (upload SAS policy), packages/hatchet/src/index.ts (ignored dispatch return value), packages/util/src/publicUrl.ts (IPv6 blanket block)
|
| Filename | Overview |
|---|---|
| packages/graphql/src/services/knowledge.ts | Core knowledge service: requestKbFileUpload generates a blob-scoped SAS without startsOn clock skew protection; confirmKbFileUpload handles idempotency well via upsert + P2002 recovery; row-lock pattern in deleteKb/deleteKbResource is deliberate. |
| packages/hatchet/src/kbIngestion.ts | External Hatchet bridge with recovery, cancellation, and timeout logic; dispatchKBIngestion returns undefined in certain race conditions that the calling Hatchet task treats as success, skipping onFailure signaling; ingestion SAS correctly applies clock-skew startsOn backdate. |
| packages/hatchet/src/index.ts | ingestKBResource task discards the undefined return from dispatchKBIngestion and always returns { success: true }; onFailure handler correctly sends FAILED webhook when all retries are exhausted. |
| packages/graphql/src/services/knowledgeWebhooks.ts | HMAC-SHA256 webhook verification is timing-safe and correctly validates timestamp replay window; updateMany with ingestionAttemptId guard is idempotent by design; always returning 200 for valid-signature requests is intentional. |
| packages/util/src/publicUrl.ts | SSRF guard covers RFC 1918, link-local, loopback, and special-use ranges; blanket IPv6 rejection prevents any IPv6 URL (including legitimate public hosts); does not perform DNS resolution (by design). |
| packages/prisma/src/prisma/schema/knowledge.prisma | Defines KB and KBResource models with two-migration incremental rollout; missing @@unique([blobName]) guard (flagged in previous review) and no uniqueness constraint on externalWorkflowRunId. |
| packages/kb-management/src/components/KnowledgeBaseFileDropzone.tsx | Two-phase upload (SAS request → Azure PUT → confirm) is correctly implemented; uses BlobServiceClient with account URL + blob-scoped SAS, which is a valid but non-standard pattern that works because the SDK propagates the SAS through container/blob navigation. |
| apps/backend-docker/src/app.ts | Webhook endpoint correctly uses express.raw to capture raw body for HMAC verification before any JSON parsing; route is placed before the GraphQL handler and not gated by JWT middleware. |
Sequence Diagram
sequenceDiagram
participant U as Lecturer UI
participant GQL as GraphQL API
participant AZ as Azure Blob
participant H as Hatchet Worker
participant EH as External Hatchet
participant WH as Webhook Handler
Note over U,WH: Phase 1 — Upload
U->>GQL: requestKbFileUpload(kbId, fileName, mimeType, size)
GQL->>AZ: createIfNotExists(container)
GQL-->>U: uploadSasURL, containerName, blobName
U->>AZ: PUT blob (SAS-authenticated)
U->>GQL: confirmKbFileUpload(kbId, blobName, title)
GQL->>AZ: blobClient.exists() + getProperties()
GQL->>GQL: upsert KBResource (ADDED)
GQL-->>U: KBResource status ADDED
Note over U,WH: Phase 2 — Ingestion
U->>GQL: ingestKbResource(id, speedMode)
GQL->>GQL: CAS updateMany status QUEUED
GQL->>H: tasks.ingestKBResource.runNoWait(payload)
GQL-->>U: KBResource status QUEUED
H->>EH: client.runNoWait(workflowName, payload)
H->>GQL: updateMany externalWorkflowRunId
Note over H,WH: Phase 3 — Monitor + Webhook
loop every monitor tick
H->>EH: runs.get_status(runId)
EH-->>H: RUNNING / COMPLETED / FAILED
H->>WH: POST /api/webhooks/kb-ingestion (signed)
WH->>WH: HMAC verify + timestamp check
WH->>GQL: updateMany status where ingestionAttemptId matches
end
alt Hatchet task fails all retries
H->>WH: onFailure POST status FAILED (signed)
WH->>GQL: updateMany status FAILED
end
Reviews (6): Last reviewed commit: "feat(kb): add management and ingestion P..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Round 2 rulings: reusable package (not standalone app), no simulated ingestion, workload-identity blob reads, base v3-ai, allow docx/pptx.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 1509424 | Triggered | Generic Password | d898fad | .devcontainer/docker-compose.yml | View secret |
| 1509424 | Triggered | Generic Password | d898fad | .devrouter.yml | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
| sizeBytes Int | ||
| blobName String | ||
| blobHref String | ||
|
|
||
| status KBResourceStatus @default(UPLOADED) | ||
| statusMessage String? | ||
| ingestedAt DateTime? | ||
|
|
||
| kb KB @relation(fields: [kbId], references: [id], onDelete: Cascade, onUpdate: Cascade) | ||
| kbId String @db.Uuid | ||
|
|
||
| createdAt DateTime @default(now()) | ||
| updatedAt DateTime @updatedAt | ||
|
|
||
| @@index([kbId, status]) | ||
| } |
There was a problem hiding this comment.
blobName
blobName is server-minted as <uuid>.<ext> in requestKbFileUpload, but neither the schema nor the planned confirmKbFileUpload service checks whether a KBResource row already exists for that blob before inserting. A client that retries confirmKbFileUpload after a network timeout (the blob is already present from the first call, so blobClient.exists() passes) will create a second row pointing to the same Azure blob. When either row is later deleted via deleteKbResource, the shared blob is removed from storage while the other row remains — producing a dangling KBResource whose ingestion or re-download will silently fail.
Adding @@unique([blobName]) to KBResource (UUIDs are globally unique, so the column is safe to uniquify without a composite key) gives a DB-level guarantee that the confirmKbFileUpload mutation is idempotent under retries, with no service-layer coordination needed.
Merge W1 into the kb-poc integration branch. Knowledge-graph visualization and model selection remain parked in draft PR #5206 for W9.
|
Too many files changed for review. ( Bypass the limit by tagging |
rschlaefli
changed the title
rschlaefli
changed the title
2 participants
What this adds
This draft PR is the integration line for the knowledge-base workflow into
v3-ai. W1 through W4 are now on the branch:The graph visualization and model-selection work remains parked in draft PR #5206 for W9. PR #5078 is reference material only; its useful behavior is being reimplemented selectively.
How it works
KBChatbotstores the KB-to-chatbot binding. PostgreSQL enforces the one-enabled-KB-per-chatbot rule with a partial unique index.doc_querytool.kb_id,chatbot_id, an opaque session subject,jti,iat, andexp. Participant identity is not included, and the existing participant JWT path is unchanged.KBserver loads only withscope_tokenauthentication and complete KB, chatbot, and session context. Misconfiguration fails closed.KB_doc_querycard is registered under the emitted runtime name. Its EN/DE UI shows generic failures and does not expose upstream error text.Branch coverage
v3-ai69c2a499aorigin/v3-aiReview focus
Verification
Current head:
pnpm run check:allin the Node 24 devcontainer: 25 typecheck tasks and six lint tasks passed; formatting, syncpack, AGENTS, and Prisma schema-sync checks passed.pnpm run build: 22 of 22 production tasks passed.https://manage.klicker.kb-poc.localhost: attach, replacement warning, replace, disconnect, linked chatbot, no-KB warning, EN desktop, and DE mobile.Earlier branch verification:
Failed/Warning:
seed:rawcommand still stops in the pre-existing account seed with PrismaP2002on(provider, providerAccountId). The changed MCP seed path is covered by the dedicated four-case suite and the real-database probe above.1509424is an existing unrelated signal for intentional local test credentials. W4 adds no credential value.Fresh GitHub CI for head
69c2a499apassed: formatting, lint, syncpack, types, GraphQL, build/compile, all eight Playwright shards and their status gate, and both fallback image builds are green. GitGuardian remains failed only on existing unrelated incident1509424, as noted above.Screenshots
W4 chatbot binding
W4 chatbot status
W3 status and cutover
Security / privacy
Blocking before merge
Follow-up after merge
v3-aipath only after the external platform and pilot gates pass.No merge is requested by this update.