feat(rag): Knowledge Engine overhaul — chunking, source persistence, re-index, hybrid retrieval and retrieval evals - #234
Merged
Conversation
… reset-link poisoning Three pre-pen-test hardening fixes on the authentication and secrets path. * bootstrap: refuse to start in production when validateConfig reports errors. The validator already required a >=32-char JWT_SECRET and an independent PROVIDER_ENCRYPTION_SECRET, but the results were only logged, so a deployment missing them booted on the shipped development defaults — a session token anyone could forge and credentials encrypted under a publicly known key. Development still only warns. * crypto: production no longer falls back to JWT_SECRET when encrypting provider credentials, so the signing key and the at-rest encryption key can never be the same value. Decryption still tries both secrets, so existing payloads stay readable and are re-encrypted on their next save. * auth: build the password-reset link from the configured app URL instead of the request Origin/Host headers. The endpoint is unauthenticated, so those headers let an attacker have a genuine reset token mailed to a victim pointing at a host the attacker controls. Verified: tsc --noEmit clean; auth/crypto/config suites pass (7 files, 73 tests).
findMcpServerById is scoped only by the tenant database, and the by-id handlers passed the URL id straight to it. Any member of one project could therefore address a server belonging to another project in the same tenant: read its full config, PATCH its exposure to public, delete it, or — worst — POST /mcp/:id/execute to run its tools with the upstream credentials that server stores. Adds serverInProjectScope(), which resolves the server and returns null when it belongs to another project, and applies it to GET /mcp/:id, GET /mcp/:id/logs, PATCH /mcp/:id, DELETE /mcp/:id, POST /mcp/:id/refresh-tools and POST /mcp/:id/execute. Out-of-scope ids get the same 404 as missing ones, so the endpoint is not an existence oracle. Owners and admins keep tenant-wide reach, which resolveProjectContext already grants them, and servers stored without a projectId stay tenant-wide, matching how findMcpServerByKey already treats them — so no in-project or admin flow changes. Verified: tsc --noEmit clean; MCP suites pass (6 files, 33 tests).
The same defect the MCP server routes had repeats across the product: a handler takes an id from the URL and resolves it with a lookup scoped only by tenant database, never comparing the object's projectId to the caller's resolved project. Projects are a real authorization boundary — resolveProjectContext restricts a non-privileged member to the projects they belong to — so every one of these routes let a member of one project reach another project's data. Closed here, each with the same guard the mcp.ts fix introduced: * agents — get/update/delete/versions/publish/conversations/chat. Chat and conversations also let the caller drive another project's agent. * tools — get/update/delete/logs, and execute, which issued the outbound call with the victim project's stored upstream credentials and returned the response to the caller. * rag — document get, and also delete and re-ingest, which resolve the document by id alone while scoping only the module, so a member could destroy or overwrite another project's document. Module usage logs (the questions asked against a knowledge base) are now behind the same module resolution the sibling routes already use. * memory — item delete and update, and bulk delete, which ignored the store's project entirely. * guardrails — get/update/delete/evaluations, plus the word-list routes, whose edits change what every guardrail referencing the list blocks. * ocr-jobs — all nine by-id routes, including item read and export. * pii — policy get/update/delete. In every case an out-of-scope id returns exactly what a missing id already returned, so none of these routes becomes an existence oracle. Owners and admins keep the tenant-wide reach resolveProjectContext already grants them, and rows stored without a projectId stay reachable so nothing predating project stamping breaks. Several handlers that previously used requireSessionContext now resolve project context, which is what their sibling list/create routes already did. The guards live in the plugins. The service and database layers still resolve these objects by id alone, so a future caller can reintroduce the same hole; pushing projectId down into those lookups is the durable fix. Verified: tsc --noEmit clean; full suite identical to the pristine tree — 3244 passed, 0 failures, same 5 pre-existing MongoDB-dependent files. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KUtq8Ck52nZDTzh8uTTUfJ
Azure AI Search document keys only allow letters, digits, underscore, dash, and equal sign. Knowledge Engine vector ids use the "module:documentId:chunkIndex" pattern, so every upsert was rejected with 'Invalid document key' and repeated failures tripped the vector-upsert circuit breaker. Unsafe ids are now stored under a marked URL-safe Base64 form (_b64_ prefix) and transparently decoded on query/list, so callers keep their original ids end-to-end. Already-safe ids pass through unchanged, and deletes encode with the same rule so stored documents are targeted correctly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014gQKi8FTcqmRzQTchuKg3s
Vector queries accepted a `filter` that each driver interpreted differently:
Chroma read it as a `where` clause, Elasticsearch as query DSL, Atlas as a
$vectorSearch filter, Milvus stringified it into an expression it cannot parse,
and nine drivers — including Azure AI Search — ignored it outright. Callers
that filtered (Knowledge Engine queries, memory scope isolation) silently got
unfiltered results.
Introduce one provider-neutral filter DSL, parsed and validated once in the
service layer, then translated per driver into the store's native syntax:
{ source: "crawler", depth: { $lte: 2 }, lang: { $in: ["tr", "en"] } }
Field operators $eq/$ne/$gt/$gte/$lt/$lte/$in/$nin/$exists compose with
$and/$or/$not; `$raw` passes a provider-native filter through unchanged.
Filters are always pushed down, never applied afterwards in memory: a driver
declares the operators it can express via a `vector.filterOperators`
capability, and a filter it cannot honour is rejected with 400 rather than
dropped. topK therefore always counts matching documents.
Two stores needed schema changes to filter at all, and both stay backwards
compatible — pre-existing indexes keep serving unfiltered queries and reject
filtered ones with a message pointing at reindexing:
- Azure AI Search cannot filter inside its metadata JSON blob, so new indexes
carry flattened `metadata_kv`/`metadata_keys` collections. That covers the
equality family; range operators are not expressible over a string collection
and are declared unsupported.
- Milvus can only filter a JSON-typed column; new collections type
`metadata_json` as JSON instead of VarChar.
Knowledge Engine modules gain `defaultFilter`, ANDed into every query so
several sources can share one vector index, and `filterableFields`, which
restricts what callers may filter on and is advertised to agents and MCP
clients as the `filter` argument's documentation.
The filter reaches the store through every surface: client and dashboard APIs,
the Knowledge Engine query and playground, the agent `knowledge_search` tool,
the Knowledge Base MCP tool, and the OpenAPI contract.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014gQKi8FTcqmRzQTchuKg3s
The vector index detail page aggregates `vector_query_logs` for daily query volume, latency, average score, filtered-query share, and topK distribution, but nothing ever wrote to that collection — the panel had been empty since it shipped, and `filterApplied` was read by the aggregation while no code set it. `queryVectorIndex` now records one row per search through the database abstraction (`createVectorQueryLog`, implemented for both MongoDB and SQLite), carrying the fields the aggregation matches on: `indexKey` from the index record, `topK`, match count, latency, mean similarity score, whether a metadata filter was pushed down, and the caller attribution resolved from the request context. The write happens after the search returns and its failures are logged and swallowed: analytics must never turn a successful query into an error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014gQKi8FTcqmRzQTchuKg3s
…p usage Two gaps left over from recording vector query logs. The analytics endpoints reached past the database abstraction into a raw MongoDB client, so the panel returned 503 on SQLite tenants and the aggregation pipeline was duplicated between the Fastify plugin and the legacy route. The aggregation moves behind `aggregateVectorQueryStats`, implemented for both backends, with the day-filling and rounding the panel expects shared in `getVectorIndexQueryStats`. Both endpoints now call that one path, and the index key the logs are scoped by is resolved within the caller's project instead of tenant-wide. Vector searches were also absent from the usage rollup: they now record a `vector` service event keyed by index key, carrying match count as a unit, with failed searches counted as errors so the failure rate stays visible. Query embeddings remain billed through the models service, so no tokens are attributed here. Usage accounting and the analytics row are written together and stay best-effort — neither can turn a successful search into an error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014gQKi8FTcqmRzQTchuKg3s
… integration Snapshot only — taken so the azure-search-key-encoding and pen-test-prep branches can be merged without risking the uncommitted tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ywd' into feat/knowledge-engine-overhaul
…nches
Brings three lines of work onto one base so the Knowledge Engine overhaul can
build on top of all of them:
- origin/claude/azure-search-key-encoding-naka6b (canonical vector filter
language, vector query logs + analytics through the DB abstraction,
Azure AI Search key encoding)
- origin/claude/console-pen-test-prep-36fywd (project-scoping the by-id
routes, production config fail-closed, separate encryption key)
- the uncommitted local tree (chunker separator fix, upload de-dup,
defaultTopK/minScore, countRagQueryLogs, MCP composite/tool-name/custom-key,
vector migrations, provider score fixes)
The first two merged clean. The local tree duplicated three of the branch's
features with weaker implementations, so conflicts were resolved per feature
rather than per side:
- Azure document keys: kept the branch's prefix-marked encodeVectorId /
decodeVectorId and dropped the local round-trip-guess encodeKey/decodeKey.
Kept the local fix that searchText must stay undefined — passing '*' puts
Azure into hybrid/RRF scoring and collapses every similarity to ~0.016.
- vector_query_logs: kept the branch's writer, which goes through the DB
abstraction (both backends, with usage attribution), and dropped the local
raw-Mongo writer. Kept the local insight by moving that writer off the
query's critical path with fireAndForget.
- index analytics: kept the branch's move into the DB abstraction, and folded
the local live "Vectors" KPI into getVectorIndexQueryStats, which already
resolves the index key.
Also preserved from the local tree: the query-time dimension guard, the
metric-aware Postgres score expression (now over the branch's SQL-side
filtering), the systemDefault returnDistance/returnMetadata fix without which
every score was 0, and the Milvus id escaping.
Test fixtures updated, not weakened: six vector-service query fixtures now
carry a correctly sized vector because the dimension guard rejects mismatches,
and three RAG route tests mock shapeRagQueryResponse instead of leaving it
undefined.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… and offsets
Adds src/lib/services/rag/chunking.ts and moves every strategy onto a shared
splitter/packer pair, then extends the schema the chunks are stored under.
- 'token' now counts real tokens with gpt-tokenizer (already a dependency,
previously unused). It used to split on whitespace and ignore `encoding`
entirely while the edit form offered three of them.
- chunkSize is a hard cap. The trailing '' separator was inert, so text with
no usable boundary (CJK, base64, a wide markdown table) was emitted whole
and could overflow the embedding model.
- chunkOverlap >= chunkSize used to spin the token splitter forever and was
reachable from the UI. Rejected on write, clamped for existing modules.
- New strategies: markdown (never crosses a heading, carries the breadcrumb),
sentence, and semantic (cuts on embedding distance).
- Optional contextual headers: one LLM-written sentence per chunk situating
it in its document. Degrades to an unprefixed chunk on failure.
- Chunks now record charStart/charEnd/headingPath/tokenCount, which is what
makes small-to-big parent windows resolvable without duplicating text.
- Documents carry an optional per-document chunkConfig override plus the
columns the stored source and re-index will use.
ingest and re-ingest now share indexDocumentContent instead of carrying
near-verbatim copies. That closes two bugs in the copies: caller metadata was
spread OVER the reserved _documentId/_fileName keys, and a run that failed
after upserting left vectors nothing could delete, because deleteRagDocument
reconstructs ids from a chunkCount only written on success.
29 new chunking tests; the pipeline had none, so regressions shipped silently.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…and retrieval evals
Completes the Knowledge Engine overhaul on top of the new chunker.
Storage and lifecycle
- Every ingest now stores the extracted text (inline under 250k chars, in the
file bucket above it) and the original bytes when a bucket is configured.
Re-ingest resolves original -> stored text -> the old lossy chunk join, so a
re-index no longer rebuilds a document from its own overlapping chunks.
- Uploads de-duplicate on a content hash; an explicit force flag overrides it.
- Deleting a module cascades to its documents, chunks, stored sources and
vectors. It used to delete one row and orphan everything else.
Re-index
- Changing chunkConfig or the embedding model marks the module and enqueues a
resumable run (checkpointed per document, cancellable across nodes, resumed
at boot). Queries keep serving the old vectors until it finishes.
- A same-dimension embedding model swap is now flagged; nothing errored before
and retrieval silently returned meaningless neighbours.
Retrieval
- Optional hybrid dense+keyword search per provider, behind a capability flag,
with fused scores normalised back onto the similarity scale.
- minScore and the score histogram are thresholded on the SIMILARITY scale
(denseScore/vectorScore), never on a reranker's or RRF's own scale — a
module's calibrated 0.5 must not silently change meaning when hybrid is on.
- isolateByModule ANDs the module key into every query, defaulting on only
when the vector index is actually shared, so an externally populated index
still returns results.
- Small-to-big parent windows resolved from the stored source at query time.
Insight
- Query logs record preFilterMatchCount, topScore, avgScore, minScoreApplied
and hybrid, which is what separates "nothing was retrieved" from "the
threshold discarded everything". New zero-result feed, score histogram and
retention.
- Evaluations gain a rag target and three scorers judged by embedding
similarity rather than string overlap: context-recall, context-precision
and groundedness.
Also fixes, found while wiring the above: rag evaluation targets were silently
dropped on SQLite; every new module retrieval field was silently dropped on
SQLite; chunkConfig was never validated on any write; and the document detail
endpoints would have returned the whole stored source on the wire.
3665 tests green, tsc clean, EE overlay app tree clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each finding here survived two independent reviewers instructed to refute it.
Data loss
- Deleting a module deleted documents and chunks by module key alone,
tenant-wide. Module keys are unique only within a project, so removing
"docs" in one project destroyed the corpus of "docs" in every other project
of the tenant. The bulk teardown is now taken only when no sibling module
shares the key; otherwise rows go one document at a time.
- The same function enumerated documents project-scoped but deleted them
unscoped, so every document ingested through /client/v1 (which writes no
projectId) lost its row while its vectors and bucket object survived.
Retrieval returning the wrong thing
- The SaaS default vector provider re-wrapped a translated compound filter,
emitting an $and bound to an object instead of an array. With isolation on
by default that broke any filtered query with a ValidationException.
- The same provider unwrapped metadata as typed scalars while the SDK returns
plain JSON, so every key read back null — breaking content hydration,
citations, parent windows and isolation itself.
- Azure AI Search threw on any filter against an index created before this
branch, so the isolation guard the system adds by itself killed every query
on existing indexes. An explicit filter still fails loudly; the system's own
guard degrades visibly instead.
- minScore was being compared against the pre-rerank similarity on reranked
modules, and against two different scales inside one hybrid result set. It
now always means the score the module's own pipeline produced.
- A re-index dropped every user and crawler metadata key from the corpus,
which is exactly what defaultFilter and filterableFields match on.
- Re-indexing a document that predates source persistence stored the lossy
chunk join as its canonical source, making the corruption permanent.
- Query-log analytics read the rows of a same-key module in another project,
including the verbatim end-user query text.
Chunking
- A stored chunkConfig with no chunkOverlap carried the whole previous chunk
forward on every flush, so chunks grew without bound past chunkSize.
undefined loses every numeric comparison, so no guard caught it.
- Semantic chunking dropped whitespace-only atoms, breaking the contiguity the
offsets depend on, so a parent window could cut off the matched chunk.
- topScore was recorded after the min-score filter, so every query the
threshold emptied logged 0 — in the panel that exists to explain them.
Operations
- A production config failure threw into a caller that swallowed it, leaving
the process alive and permanently unready: liveness kept passing, the
rollout never stalled, and three replicas served 503 while looking healthy.
- Re-index runs were claimed only in process memory. With no Redis the queue
is per-pod, so after a rolling restart all three replicas resumed the same
run and re-embedded the same documents concurrently.
Dashboard
- Editing a module 400d whenever the description was empty, because the PATCH
whitelist rejected the null the form sends to clear it.
- The playground crashed the whole page on a module set to "Text only".
- The edit form promised a background re-index and never started one.
- Saving a legacy module silently flipped isolation on, and saving before the
provider list loaded silently turned hybrid off.
3721 tests green (56 new regression tests), tsc clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A second review pass audited the previous commit. Three of its fixes were
wrong, two of them worse than what they replaced.
- Re-index refused to run at all. The cross-replica claim was written as a
TypeScript interface and a test double and implemented on neither provider,
so the job's own guard rejected every run. claimRagReindexRun,
touchRagReindexRunClaim and releaseRagReindexRunClaim now exist on both
backends: one findOneAndUpdate on MongoDB, one UPDATE ... WHERE on SQLite,
with the claim fields, their columns and their tenant migration. Parity
tests pin that two workers cannot both win, that a silent owner is
reclaimable and a live one is not, and that a finished run is never claimed.
- Refusing to store a chunk reconstruction removed the document's last copy.
Re-ingest deletes a document's vectors and chunk rows before rebuilding it,
and for a document with no stored source those rows ARE its text. Keeping
the reconstruction only in a local variable meant one embedding timeout, or
a pod killed mid-document, destroyed the document permanently — with a
failure counter as the only trace. The text is stored again; what is
withheld is its HASH, which is what de-duplication and change detection
compare against, so a re-upload of the real original can never be mistaken
for a duplicate of the join.
- A 409 from the re-index endpoint was reported as success. It means a run
started for the PREVIOUS configuration is still in flight, so nothing is
rebuilding what was just saved. The run now records the configuration it
rebuilds against and refuses to clear reindexRequired for a configuration it
did not rebuild, and the form says so instead of showing a tick.
Also: clearing an optional module field did nothing on SQLite while the API
answered 200, because the service turns the API's null into an explicit
undefined and the guards there skipped undefined. Presence now decides, matching
MongoDB, with a parity test.
Not a regression, checked against origin/main rather than assumed: minScore has
always been compared against the post-rerank score (main, ragService.ts:680 and
:667). rankedScoreOf restores that; the similarityOf introduced earlier on this
branch was the deviation.
3733 tests green, tsc clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A third audit found that the previous commit's durability fix did not actually persist anything. For text under the inline cap, storeDocumentSource only builds an in-memory object — it performs no write — and the source fields reached the document row only in the success branch, after indexing had already deleted the document's vectors and chunk rows. For a document with no previously stored source those rows ARE its text, so any failure in between still destroyed it. The source row fields are now written before the destructive steps. The same audit found that dropping the `sourceIsTrustworthy` guard in that commit also un-gated discardDocumentSource, so a chunk-join re-ingest deleted the document's real stored source object and replaced it with the lossy join. A reconstruction is what we fall back to when the real source could not be READ — very often a transient failure — so it must never delete the object it failed to read. The guard is back, on the discard alone. SQLite's updateRagDocument still used `!== undefined` guards, so the deliberate `sourceHash: undefined` that keeps a reconstruction from becoming canonical was silently skipped there, leaving the stale hash of the true original next to the join. Presence decides now, matching MongoDB. The tests for this no longer assert which CALL carries the source; they assert the source write happens before the first vector delete, which is the property that actually matters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…resolves `next build` failed with "Can't resolve 'gpt-tokenizer/encoding'": webpack resolves requires statically, and the template literal made it try to resolve the encoding DIRECTORY rather than the three modules underneath it. tsc, vitest and eslint all pass on the dynamic form because they run on the real Node resolver, so nothing local caught it — only the production build does. The three specifiers are written out; loading stays lazy and memoised. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Knowledge Engine overhaul, plus the two pending
claude/*branches folded in.What is in here
gpt-tokenizer(thetokenstrategy used to split on whitespace and ignoreencodingentirely),chunkSizeas a hard cap, and newmarkdown/sentence/semanticstrategies.chunkOverlap >= chunkSizeused to spin the token splitter forever and was reachable from the UI.ragtarget and three scorers judged by embedding similarity.claude/azure-search-key-encoding-naka6b(canonical vector filter language, vector query logs through the DB abstraction) andclaude/console-pen-test-prep-36fywd(project-scoping by-id routes, production config fail-closed, separate encryption key). Conflicts were resolved per feature, not per side — see the merge commit.Verification
tscclean · 3733 tests ·eslint0 errors · EE overlaysrc/tree clean.package.jsonandpackage-lock.jsonare byte-identical tomain, so no lockfile regeneration is needed before tagging.Known issues, deliberately shipped
Four review rounds ran; every round's fixes were themselves audited, which is how the last three criticals were caught. What remains is documented and accepted for this release:
-onprem: production now exits on a config validation error, and the EE chart ships an emptyPROVIDER_ENCRYPTION_SECRET.Rollback note:
:productionis a mutable tag pulled withAlways, sorollout undocannot roll this back. The current digest issha256:44e31c06316e4c7babbb88963d29f019b5a32e017d047c11ed8ad1b705d7c55b.🤖 Generated with Claude Code
https://claude.ai/code/session_01KG71cnyfK5GGwbXb25pxsd