Repository navigation
feat(provenance): stabilise source identity, refine PDF blocks, extract the Retrieval seam - #111
Merged
Merged
Conversation
reindexDocument() used to delete the old derived index and then call addDocument(), which minted a new documentId, dropped localFilePath and left the old row stuck in 'processing'. Once citations persist a documentId, that silently repoints every historical answer at a different source. Split the indexing path so a document is a stable source identity: addDocument / addDocumentFromFile -> create source -> indexDocument(id) reindexDocument -> clearDerivedIndex(id) -> indexDocument(id) indexDocument() owns only the derived chain (blocks -> chunks -> embeddings -> vectors -> status) and never inserts or deletes documents. clearDerivedIndex() is the symmetric teardown, shared with deleteDocument. Persist the parse structure on the source row (documents.structure) so a re-index rebuilds the same page/paragraph blocks without re-parsing a file that may have changed on disk. Fixes #109
PdfLoader joined every TextItem on a page with a space and emitted one page-level block, so a citation could say "page 12" but never "the paragraph at the bottom of page 12". Add a pure pdfTextLayout module that groups items into lines by baseline and lines into paragraphs by vertical gap and first-line indent, each with its own normalized bbox. PdfLoader builds the canonical content from those paragraphs, so content.slice(startOffset, endOffset) === block.text keeps holding; buildPageBlocks() consumes page.blocks when present and falls back to the old one-block-per-page shape otherwise. Geometry is unit-tested with synthetic items; the existing single-run fixtures still produce one block per page. Fixes #110
Retrieval was inline in KnowledgeService.search(): verify the embedding space, embed the query, query the vector store, then join chunks and documents. A new strategy could not be added without editing the service, and the eval harness (#75) had no stable entry point. Introduce src/main/services/retrieval/: interface Retriever { search(notebookId, query, opts): Promise<RetrievedEvidence[]> } RetrievedEvidence carries provenance as a first-class field (source title/type, page range, ordered block spans), so the citation layer (#69) never queries the database again. DenseRetriever is the current strategy; the embedding-space guard stays in KnowledgeService because it is a precondition, not a strategy concern. Provenance hydration is now batch: resolveChunksProvenance() resolves any number of chunks with one join instead of one query per chunk, avoiding the N+1 that topK=5 would otherwise create. hydrateEvidence() wires the batch queries into RetrievedEvidence[]. KnowledgeService.search() keeps its signature and delegates; SearchResult gains a locator field and is otherwise byte-for-byte the same. Fixes #74
PdfLoader produces per-paragraph bboxes and document_blocks stores them, but ChunkProvenanceBlock never read document_blocks.bbox, so the value was dropped before RetrievedEvidence. #69 could build a citation on the current shape and #72 would then only know the page and blockId, not where on the page the paragraph sits. Select document_blocks.bbox in the provenance join, project it into ChunkProvenanceBlock and assert it survives into RetrievedEvidence.locator.blocks in the unit test, the evidence test and the packaged smoke check. Also drop includeContent from RetrieveOptions: which fields the legacy SearchResult exposes is a mapping concern, not a retrieval strategy one, and DenseRetriever never read it.
The migration only adds documents.structure; documents imported before it have NULL there. The new reindexDocument() clears the derived index first, so re-indexing a pre-upgrade PDF would rebuild flat paragraphs and silently drop the page/bbox provenance it used to have. Recover the structure from localFilePath before clearing anything: re-parse the local copy, keep the result only when the content is byte-identical to the canonical documents.content, and persist it. If recovery fails the old index is still cleared, but we never destroy existing provenance to attempt recovery. Verified in the packaged smoke test by nulling structure on an imported PDF, re-indexing, and asserting the structure is backfilled and the blocks keep their page number.
9 tasks
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
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.
What does this PR do?
Closes the provenance work that must land before structured citations (#69), in the order the roadmap calls for:
reindexDocument()rebuilds the derived index in place instead of importing a second document.RetrievedEvidence, so a citation can point at a paragraph (and highlight it) rather than only a page.Retrieverseam that returnsRetrievedEvidence[]with provenance attached, and provenance hydration becomes batch.Why?
processingforever, then calledaddDocument(), which minted a newdocumentIdand droppedlocalFilePath. Once [Feat] Structured citations through retrieval → prompt → chat_messages.metadata.citations[] #69 persists adocumentIdin a citation, that silently repoints every historical answer at a different source — the exact thing the provenance epic exists to prevent.PdfLoaderjoined everyTextItemon a page into one string, so the data could say "page 12" but never "the paragraph at the bottom of page 12".document_blocks.bboxheld the geometry, but the provenance join never selected it, soRetrievedEvidencecould not support the highlight in [Feat] Citation click → open source at page/block with highlight #72.documents.structure; re-indexing a pre-upgrade PDF would rebuild flat paragraphs and drop the page/bbox provenance it used to have.Related issue
Fixes #109
Fixes #110
Fixes #74
Part of the v1.4 — Trusted Research Loop epic: #82
What changed?
indexDocument(documentId, content, structure, options)(derived chain only, never touchesdocuments) andclearDerivedIndex(documentId).addDocument/addDocumentFromFilecreate the source then index;reindexDocumentclears then indexes the same id.deleteDocumentreuses the same teardown.documents.structureJSON column (migration0016), so a re-index rebuilds the same blocks without re-parsing a file that may have changed on disk.reindexDocument()recoversstructurefromlocalFilePathbefore clearing the old index, and only when the re-parsed content is byte-identical todocuments.content.pdfTextLayoutmodule groupsTextItems into lines and paragraphs, each with its own normalized bbox.buildPageBlocks()consumespage.blocks, falling back to the old shape.ChunkProvenanceBlocknow carriesbbox; the provenance join selectsdocument_blocks.bboxand it reachesRetrievedEvidence.locator.blocks[].Retrieverseam (refactor). Newsrc/main/services/retrieval/:Retriever,RetrievedEvidence,DenseRetriever,hydrateEvidence().KnowledgeService.search()keeps its signature and delegates; the embedding-space guard stays in the service.includeContentwas removed fromRetrieveOptions(it is a legacy mapping concern).resolveChunksProvenance(db, chunkIds)resolves any number of chunks with one join; no N+1 attopK.docs/architecture.mdrecords the provenance chain and the Retrieval seam.How was this tested?
npm test— 125 pass (addedtest/pdfTextLayout.test.tsandtest/retrievalEvidence.test.ts, the latter now asserting the paragraph bbox survives).npm run typecheck— clean.npm run lint— 0 errors (pre-existing warnings, unchanged).npm run check:design— no violations.npm run build— clean.npm run build:unpack && npm run smoke:packaged— 18/18 checks pass, including three new packaged-app checks:re-index rebuilds the derived index in place and keeps the source identity,DenseRetriever returns retrieved evidence with a page/block locator(asserts page, bbox),re-index recovers structure for documents imported before the column existed.Follow-ups (deliberately not in this PR)
KnowledgeService.retrieve()façade over the guard + strategies, withsearch()demoted to a compatibility adapter. Planned with [Feat] Structured citations through retrieval → prompt → chat_messages.metadata.citations[] #69.Checklist
npm run typecheckpasses.npm run buildpasses.Desktop / build changes
npm run build:unpackpasses.npm run smoke:packagedpasses.