Skip to content

feat(documents): add semantic attachment search - #664

Open
salmonumbrella wants to merge 3 commits into
kenn-io:mainfrom
salmonumbrella:feat/document-vectors
Open

feat(documents): add semantic attachment search#664
salmonumbrella wants to merge 3 commits into
kenn-io:mainfrom
salmonumbrella:feat/document-vectors

Conversation

@salmonumbrella

@salmonumbrella salmonumbrella commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Attachment documents can now be searched lexically, semantically, or with deterministic hybrid ranking. Vector generations build behind the active corpus and become visible only after exact coverage is complete, so interrupted or replacement builds never expose a partial index.

Build failures are isolated to the affected extraction. A malformed provider result no longer poisons healthy documents later in the batch, and input-preparation failures follow the same bounded retry and terminal-diagnostic lifecycle as provider failures. A generation with terminal failures is reported as blocked instead of appearing to make silent progress; operators can inspect it, retry failed work, or retire it while the prior active generation remains searchable.

Archives upgraded from older extraction storage must run msgvault documents build --full-rebuild --capabilities <manifest> --yes before document-vector consent or build. Their missing normalized identity is not guessed or backfilled, and newly published extractions must carry the current Docbank identity.

PostgreSQL holds an archive-scoped advisory lock across ledger and backend mutations, fencing daemon and local command writers from deleting a vector another process just republished. SQLite continues to rely on its archive ownership lock. Because document vectors have not shipped, the final table and constraint shapes live directly in the base schemas; there are no upgrade-only repair paths for unreleased layouts.

The document lane consumes Docbank v0.14.0's normalized-document, embedding-plan, retrieval, and egress-identity contracts rather than maintaining parallel implementations. Msgvault persists enough normalized identity to reconstruct the Docbank raw embedding plan, pages vector backends until the scoped candidate bound is satisfied, and preserves attachment, message, person, source-unit, model, generation, rank, score, excerpt, and highlight provenance through fusion. Because scope metadata remains in Msgvault, pageable candidate collection uses exact distance ordering; this favors complete scoped results over approximate nearest-neighbor speed until scope can move into the backend query.

Hosted processing has two independent consent boundaries:

  • Document consent covers Docbank-prepared attachment text sent while building vectors.
  • Query consent covers search text sent only for explicit semantic or hybrid searches. auto remains lexical and causes no query egress.

Both paths reject redirects and revalidate the exact endpoint/model policy before each request. SQLite/sqlite-vec and PostgreSQL/pgvector implement the same generation and paging lifecycle; builds without vector support fail closed only when an embedding operation is requested.

With [vector], [vector.embeddings], and [attachments.documents.index.embeddings] configured, review and record each disclosure separately:

msgvault documents vectors consent --purpose documents --yes
msgvault documents vectors consent --purpose queries --yes
msgvault documents vectors build --limit 100
msgvault documents vectors status
msgvault documents search "signed inspection notes" --mode hybrid --candidate-limit 100

Rebuilds keep the prior generation searchable until atomic activation. Operators can use resume, retry, retire, and status for bounded recovery and cleanup.

This is the document-semantic foundation consumed by #668. It remains separate from the visual attachment lane in #650 and does not mix document vectors with visual vectors or ordinary message-search ranking.

Closes #617

@roborev-ci

roborev-ci Bot commented Aug 21, 2026

Copy link
Copy Markdown

roborev: Combined Review (71bd914)

Code requires changes: one high-severity PostgreSQL schema conflict and three medium-severity consistency/privacy issues must be addressed.

High

  • Incompatible PostgreSQL authority tablesinternal/vector/pgvector/schema.sql:135
    The pgvector backend and main PostgreSQL store both create document_vector_generations in the same schema with incompatible columns (generation_id versus id). Backend migrations and writes consequently reference a nonexistent column, preventing PostgreSQL document-vector initialization.
    Fix: Rename the backend authority table and update its foreign keys, migrations, and queries.

Medium

  • Configured embedding input cap is not enforcedinternal/vector/document/worker.go:401
    Document chunks are sent to the embedding provider without applying vector.embeddings.max_input_chars, although the limit is represented in the generation fingerprint and consent disclosure. Oversized chunks may be rejected and permanently block generation activation.
    Fix: Pass the configured cap into the worker and truncate inputs on rune boundaries, or reject configurations whose cap is below the maximum document chunk size.

  • Inconsistent candidate_limit behaviorinternal/store/document_search.go:357, internal/vector/document/search.go:270
    Direct lexical search ignores an explicit limit and always uses the fixed maximum, while semantic search defaults an omitted limit to 1000 despite the CLI and API advertising 100. Identical requests can therefore produce different rankings and costs depending on consent and runtime path.
    Fix: Define a shared default of 100 and honor validated explicit limits in both paths.

  • Embedding consent survives endpoint changesinternal/vector/document/model.go:68
    The consent fingerprint excludes vector.embeddings.endpoint, so changing from a trusted endpoint to a hosted provider does not require renewed consent before attachment plaintext is sent to the new destination.
    Fix: Retain an endpoint-independent corpus fingerprint if needed for vector reuse, but add an egress-consent fingerprint covering the canonical destination and other privacy-relevant provider settings. Require fresh consent whenever it changes.


Reviewers: 2 done | Synthesis: codex, 13s | Total: 13m45s

@salmonumbrella

Copy link
Copy Markdown
Contributor Author

@wesm can you take this over and finish it off from here? The current branch is pushed as-is. Remaining work is the main-branch conflict plus the outstanding RoboRev and PostgreSQL CI issues.

@wesm

wesm commented Aug 22, 2026

Copy link
Copy Markdown
Member

Yes, I'm on it

@wesm
wesm force-pushed the feat/document-vectors branch from 71bd914 to 0886c23 Compare August 22, 2026 10:13
@roborev-ci

roborev-ci Bot commented Aug 22, 2026

Copy link
Copy Markdown

roborev: Combined Review (0886c23)

Verdict: Three medium-severity issues should be addressed before merging.

Medium

  • Default searches may disclose query text to an external embedding providerinternal/vector/document/search.go:169
    An omitted mode becomes auto, which switches to hybrid search when an active vector generation exists. This sends the complete query to the configured embedding service without explicit user selection, while the consent disclosure only mentions attachment chunks. Default omitted modes to lexical search; require explicit semantic, hybrid, or clearly disclosed auto selection before calling EmbedQuery; and disclose query-text egress.

  • Filtering and deduplication can severely underfill semantic resultsinternal/vector/document/search.go:222
    The backend limit is applied to chunks before stale-token filtering and occurrence deduplication. Chunks from one long document can dominate the candidates, collapse into one occurrence, and prevent lower-ranked documents from being considered. Progressively over-fetch results until the requested number of authoritative unique occurrences is resolved or a separate bounded ceiling is reached.

  • Embedding requests ignore the configured batch sizeinternal/vector/document/worker.go:249
    The worker sends every claimed chunk in one EmbedDocuments call without respecting vector.embeddings.batch_size. Default manual builds can claim 100 chunks while the configured batch default is 32, causing compatible providers to reject oversized requests. Pass the configured batch size to the worker and split requests accordingly while preserving document boundaries and partial-prefix handling.


Reviewers: 2 done | Synthesis: codex, 13s | Total: 18m8s

@wesm
wesm force-pushed the feat/document-vectors branch from 0886c23 to e0b5eb8 Compare August 22, 2026 10:26
@roborev-ci

roborev-ci Bot commented Aug 22, 2026

Copy link
Copy Markdown

roborev: Combined Review (e0b5eb8)

Verdict: Four medium-severity issues require attention before merge.

Medium

  • Undisclosed search-query egresscmd/msgvault/cmd/documents_vector.go:133
    Consent discloses attachment-text egress but not search-query egress. Once vectors activate, the default auto mode sends every document query to the hosted embedding provider. Explicitly disclose query-text egress or require separate opt-in for automatic semantic search.

  • Embedding requests can exceed the configured batch sizeinternal/vector/document/worker.go:249
    The worker sends every claimed chunk in one EmbedDocuments call. Manual builds use --limit (default 100) instead of vector.embeddings.batch_size (default 32), potentially exceeding endpoint limits. Pass the configured batch size into the worker and split provider calls accordingly.

  • Cursor incompatibility during vector initializationinternal/api/document_search.go:275
    Lexical searches made before vector initialization return version-2 store cursors, but subsequent searches use SearchService, whose version-1 decoder rejects them. Use the semantic service’s cursor format for startup fallback, or consistently route explicit lexical searches through the store.

  • Consented attachment text can be replayed to an unconsented redirect destinationcmd/msgvault/cmd/serve_vector.go:268
    Document-vector workers use an ungated messageClient that follows 307/308 redirects and can replay attachment chunks to another origin, despite consent being tied to the configured destination. Construct a dedicated document embedding client that revalidates consent at request time and rejects redirects for every retry and packed request.


Reviewers: 2 done | Synthesis: codex, 13s | Total: 15m41s

@roborev-ci

roborev-ci Bot commented Aug 22, 2026

Copy link
Copy Markdown

roborev: Combined Review (8b10dbd)

Verdict: One high-severity consent bypass and two medium-severity semantic indexing/search correctness issues require changes.

High

  • Document-egress consent can be bypassed through redirectscmd/msgvault/cmd/serve_vector.go:268, cmd/msgvault/cmd/serve_vector.go:309
    • The document lane reuses messageClient without a BeforeRequest gate. Its standard HTTP client follows 307/308 redirects and replays POST bodies containing extracted attachment text to a destination not covered by the recorded egress consent.
    • Fix: Use a dedicated document-embedding client that revalidates exact consent before every request and rejects redirects across retries and Voyage packed requests. Add a behavioral test proving a cross-origin 307/308 never receives document text.

Medium

  • Filtering after the global candidate cutoff produces incomplete semantic resultsinternal/vector/document/search.go:222

    • Semantic search retrieves the global top CandidateLimit vectors before applying source, date, person, and attachment filters. Relevant in-scope vectors below that cutoff are discarded, potentially yielding incomplete or empty filtered searches.
    • Fix: Apply scope constraints during vector retrieval, or iteratively over-fetch/page candidates until enough filtered results are found or all candidates are exhausted.
  • Contextual embeddings use unstable document boundariesinternal/vector/document/worker.go:369

    • A single extraction can be divided across bounded worker runs, with each subset becoming a separate DocumentInput. Embeddings therefore depend on claim timing, batch size, and concurrency.
    • Fix: Claim and embed complete extractions atomically, or persist deterministic, fingerprinted extraction partitions that remain stable across runs.

Reviewers: 2 done | Synthesis: codex, 14s | Total: 14m34s

@roborev-ci

roborev-ci Bot commented Aug 22, 2026

Copy link
Copy Markdown

roborev: Combined Review (c77852a)

Verdict: Four medium-severity issues require attention, including query privacy, incomplete result metadata, truncation accuracy, and semantic-search scalability.

Medium

  • Plaintext queries may be disclosed automatically to hosted embedding providersinternal/vector/document/search.go:169
    Omitted or auto mode becomes hybrid search when vectors are active, causing request.Query to be sent to EmbedQuery. Existing consent covers attachment chunks but does not disclose search-query egress. Default automatic searches to lexical and require explicit semantic or hybrid mode, or explicitly disclose and gate query embedding through consent.

  • Truncated can remain false when results exceed CandidateLimitinternal/vector/document/search.go:122
    The flag only reflects upstream lexical/vector limits, not overflow caused by scoped-occurrence expansion or hybrid fusion of disjoint result sets. Detect overflow, such as by fetching limit+1, and propagate it to Truncated.

  • Semantic results omit person provenanceinternal/store/document_search.go:339
    Person-scoped semantic searches populate live-copy counts but omit PersonProvenance. Hybrid fusion can also discard lexical provenance when a semantic row becomes the result base. Populate provenance when request.Person is set and preserve it during fusion.

  • SQLite semantic searches materialize corpus-sized result setsinternal/vector/sqlitevec/document_vectors.go:163
    Vec0’s k is set to the entire generation size even though no more than 1,000 results are returned, causing excessive sorting and materialization for large corpora. Bound the inner k using the requested candidate count, with limited oversampling if needed for deterministic tie handling.


Reviewers: 2 done | Synthesis: codex, 13s | Total: 21m44s

@roborev-ci

roborev-ci Bot commented Aug 22, 2026

Copy link
Copy Markdown

roborev: Combined Review (4b511ab)

Semantic/hybrid timestamp scanning and vector-command forwarding have medium-severity defects that can break core workflows.

Medium

  • internal/store/document_search.go:318 — SQLite returns the computed COALESCE(...) timestamp as text, which cannot be scanned directly into *time.Time. Semantic and hybrid searches fail for typical messages containing timestamps. Scan using the existing nullableTimestamp helper, then assign the converted value to result.OccurredAt, matching the lexical search path.

  • cmd/msgvault/cmd/documents_vector.go:92 — The new documents vectors commands always open the local writable store instead of using the established daemon/remote forwarding path. They fail when the daemon owns the archive, ignore configured remote endpoints, and are rejected by the daemon CLI allowlist. Forward them through the daemon like existing document commands, permit the nested command in cli_handlers.go, and propagate the configured embedding-provider API-key environment variable when required.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 18m26s

@wesm

wesm commented Aug 22, 2026

Copy link
Copy Markdown
Member

Hey @salmonumbrella — thanks again for the substantial work on #664. During final review we compared the branch with the document-understanding packages that have now landed in Docbank.

We found that this branch necessarily chooses and fingerprints a document embedding recipe and retrieval policy locally. Those are cross-project contracts: if we merge them only in msgvault now, Docbank and msgvault will drift, or we will require a full re-embedding and compatibility transition later.

Our intent is therefore to pause merging #664 while we define and evaluate the storage-neutral EmbeddingPlan and shared retrieval rules in Docbank, then update this PR to consume those contracts. This PR remains the working integration vehicle and the basis of the feature; this is not a rejection or restart.

Msgvault will continue to own attachment/message occurrence mapping, consent enforcement, provenance and filtering, scheduling, publication authority, and its CLI/API/MCP surfaces. The shared Docbank work will cover deterministic semantic preparation, recipe identity, provider request safety, and common retrieval behavior.

This will add some time and reshape parts of the diff. We will link the Docbank work here and coordinate the rebase/refactor instead of asking you to chase implementation-specific review feedback that may shortly be superseded.

@salmonumbrella

Copy link
Copy Markdown
Contributor Author

@wesm I paused implementation and opened kenn-io/docbank#176 so we can align the ownership boundary before either repository hardens a conflicting contract.

It contains the complete provider-neutral design spec and all four implementation plans: evidence/Markdown/catalog, provider runtime and self-hosted Docling, embeddings/retrieval, and operator/consumer surfaces.

The main question is explicit there. Our proposed direction makes Docbank the optional durable authority for a file's sanitized Markdown, normalized evidence, embedding sets, corpus index, and document-candidate retrieval. Msgvault would keep attachment/message occurrence mapping, source visibility, mirroring orchestration, and presentation. Your comment here describes Msgvault retaining publication authority with Docbank providing the shared semantic contracts.

Could you review #176 and say which side of that boundary you intend? We will hold the Docbank foundation implementation until that is settled, then update #664 against the agreed contract instead of creating another re-embedding transition.

@roborev-ci

roborev-ci Bot commented Aug 22, 2026

Copy link
Copy Markdown

roborev: Combined Review (649365f)

Verdict: Two medium-severity issues could break document-vector status and semantic search after target rotation.

Medium

  • internal/api/document_search.go:100 — Production adapter lacks vector-status support

    The production daemon supplies *storeAPIAdapter, which does not implement DocumentVectorStatusStore. As a result, /api/v1/documents/vectors/status returns document_vector_status_unavailable whenever document vectors are enabled.

    Fix: Forward GetDocumentVectorTargetProfileID and GetDocumentVectorOperationsStatus through the adapter, then add a compile-time interface assertion and production-wiring test.

  • internal/vector/document/search.go:167 — Stale generations remain semantically “ready” after target rotation

    Readiness validates the active generation’s fingerprint but does not confirm that its extraction profile is still the current document target. After rotation, occurrence resolution filters out all hits, causing semantic searches to return empty results and hybrid searches to degrade silently to lexical-only results.

    Fix: Resolve the current target before semantic work and return ErrSemanticSearchUnavailable unless it matches generation.TargetExtractionProfileID. Add a behavioral test covering runtime target rotation.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 19m47s

@wesm
wesm force-pushed the feat/document-vectors branch from 649365f to 81af514 Compare August 23, 2026 14:17
@wesm
wesm self-requested a review as a code owner August 23, 2026 14:17
@wesm
wesm force-pushed the feat/document-vectors branch from 81af514 to 68845d3 Compare August 23, 2026 14:24
@roborev-ci

roborev-ci Bot commented Aug 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (68845d3)

Changes need fixes before merge: three medium-severity issues could stall vector generation or reconstruct incompatible embedding inputs.

Medium

  • Legacy extraction targets can stall vector generationcmd/msgvault/cmd/documents_vector.go:50
    Vector consent and generation creation accept existing extraction targets without verifying normalization-v3 compatibility. After an upgrade, legacy extraction rows retain NULL normalization identity fields, so LoadNormalizedDocument cannot reconstruct them. Require a v3-compatible target before consent/build, or trigger re-extraction/backfill. Add an upgrade test with a legacy target and ready extraction.

  • Normalized-document round trips lose chunk metadatainternal/documentindex/worker.go:419, internal/store/document_normalized.go:64
    SyntheticPrefixLen, TableChunk, CodeChunk, and span Synthetic metadata are dropped despite being present in the publication schema. Structured or split chunks may reconstruct differently from their original Docbank evidence, fail validation, or produce incompatible embedding plans. Persist and reconstruct these fields, with coverage for non-default structured and synthetic metadata.

  • Input-preparation failures leave publications pendinginternal/vector/document/worker.go:254
    When input preparation fails, claimed publications are not transitioned through FailDocumentVectorChunk. They remain pending until lease expiry, receive no retry schedule or diagnostic, and repeatedly incur the same delay. For non-cancellation errors, release and fail affected claims with an appropriate local-source classification, preferably isolating failures per extraction.


Reviewers: 2 done | Synthesis: codex, 12s | Total: 22m26s

@roborev-ci

roborev-ci Bot commented Aug 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (1017b22)

The PR has two medium-severity correctness issues affecting upgraded archives and scoped semantic search.

Medium

  • Legacy vectorization can repeatedly fail after upgradeinternal/store/document_normalized.go:21
    Existing extractions retain NULL values for normalization_version, document_family, and unit_kind, but the loader scans them into non-nullable values. Upgraded v2 archives can therefore be approved for vectorization yet repeatedly fail input preparation until the generation is blocked.
    Fix: Reject legacy targets with an actionable re-extraction requirement, or safely migrate their normalized identity before allowing vector consent/build. Validate these fields during publication as well.

  • Scoped semantic search can miss matches beyond the first 1,000 global resultsinternal/vector/pgvector/document_vectors.go:151, internal/vector/pgvector/document_vectors.go:174
    Scoped search pages through globally ranked vectors using OFFSET, but the HNSW query is limited by the configured hnsw.ef_search candidate pool of 1,000. Selective person, source, or attachment filters may therefore falsely report exhaustion while valid scoped matches remain outside that pool.
    Fix: Push scope filtering into the PostgreSQL vector query, or use an exact or iterative scan for deep paging so exhaustion reflects the scoped corpus.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 26m9s

@roborev-ci

roborev-ci Bot commented Aug 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (1792a53)

Semantic search pagination has two medium-severity correctness issues that can produce incomplete, duplicate, or skipped results.

Medium

  • internal/vector/pgvector/document_vectors.go:155 — Scoped semantic search pages through a globally ordered query that may use the approximate HNSW index. Restrictive account/date filters can therefore return incomplete or empty results when eligible candidates fall outside the initial ANN neighborhood. Use an exact scan for exhaustive pagination, or incorporate scope filters into the indexed query.

  • internal/vector/sqlitevec/document_vectors.go:183 — The KNN subquery selects only offset+k plus a small oversampling margin before applying the token tie-breaker. With many equal-distance vectors, the expanding subset can reorder earlier results between pages, causing duplicates or omissions. Apply the complete (distance, token) ordering before pagination, or use a stable keyset cursor enforced by the KNN query.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 10m13s

@roborev-ci

roborev-ci Bot commented Aug 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (34c54cb)

Semantic attachment search has two medium-severity correctness and lifecycle issues; no critical or high-severity findings were reported.

Medium

  • Normalized-document persistence loses structural metadatainternal/documentindex/worker.go:418, internal/store/document_normalized.go:107
    The persistence round trip drops SyntheticPrefixLen, TableChunk, CodeChunk, and each span’s Synthetic flag. Structured documents can therefore be reconstructed differently from their normalized evidence before validation and embedding-plan construction, potentially causing terminal preparation failures that block generation activation.
    Fix: Persist and reload all four fields, and add a round-trip test covering table, code, and synthetic-prefix content with nonzero metadata.

  • Retired-generation cleanup incorrectly depends on current embedding consentcmd/msgvault/cmd/documents_vector_runtime.go:100, cmd/msgvault/cmd/serve_vector_init.go:476
    Cleanup is gated behind resolving and consenting to the current hosted embedding policy, and the scheduler is not registered without current consent. If embeddings are disabled or the policy changes after a generation is retired, its backend vectors can no longer be removed despite the retirement command promising cleanup during subsequent vector operations.
    Fix: Run retired-generation cleanup independently of provider policy and consent, and make cleanup available whenever the document backend contains retired generations.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 18m2s

@roborev-ci

roborev-ci Bot commented Aug 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (4b06e3e)

Review found two medium-severity issues affecting vector status availability and hybrid-result metadata consistency.

Medium

  • internal/api/document_search.go:100 — The production daemon supplies storeAPIAdapter, which does not implement DocumentVectorStatusStore. As a result, the enabled /documents/vectors/status endpoint always returns 503 document_vector_status_unavailable.

    • Fix: Add forwarding methods and a compile-time interface assertion for storeAPIAdapter.
  • internal/vector/document/fusion.go:93 — Hybrid results retain semantic chunk metadata while replacing only the excerpt and highlight offsets with lexical values. If the lanes select different chunks from the same attachment, chunk_key, headings, and unit bounds describe a different chunk from the displayed excerpt.

    • Fix: Keep one lane’s evidence fields coherent, or expose separate lexical and semantic chunk provenance.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 15m8s

@roborev-ci

roborev-ci Bot commented Aug 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (af36a1f)

Semantic document search is security-conscious, but two medium-severity lifecycle issues can prevent vector initialization or silently disable results.

Medium

  • Restored SQLite backups can falsely report complete vector coverageinternal/store/document_vectors.go:1279, cmd/msgvault/cmd/documents_vector_runtime.go:160
    Coverage trusts ready rows in the main database without verifying backend vectors. Because SQLite backups exclude vectors.db, restoring an archive can retain a complete active generation with no vectors. Semantic searches then silently return no results, while scheduled convergence exits without rebuilding.
    Fix: Invalidate document-vector generations/publications during SQLite restore, or reconcile backend token existence before treating coverage as complete.

  • Dead extraction heads can indefinitely block vector initializationinternal/store/document_vector_operations.go:78
    The normalized-identity check examines every extraction head for the target profile, including heads whose final live occurrence was removed. These heads are excluded from vector coverage and full-rebuild targets, so the recommended rebuild can finish while initialization remains blocked until unrelated derivative garbage collection removes them.
    Fix: Restrict the incompatibility check to heads with current live eligible occurrences, or remove dead heads during rebuilding before validating identity.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 17m57s

@roborev-ci

roborev-ci Bot commented Aug 23, 2026

Copy link
Copy Markdown

roborev: Combined Review (feb605f)

Medium-severity issues remain in vector restore and retirement cleanup.

Medium

  • internal/backupapp/restore.go:57 — Overwrite restore invalidates only the main-database ledger, while the excluded vectors.db may retain later generation IDs. If the restored SQLite sequence reuses an ID, this can cause permanent dimension conflicts or leave backend vectors that cannot be enumerated for cleanup. Remove or reset the target document-vector backend during overwrite publication, or reconcile generations while guaranteeing IDs cannot collide.

  • cmd/msgvault/cmd/documents_vector.go:342 — Manual retirement updates ledger state, but cleanup is only available through scheduled execution. With cron and post-sync disabled, retired generations can never have their backend vectors deleted or ledger rows purged, causing repeated rebuilds to accumulate full vector corpora. Add a bounded manual cleanup/resume operation, or have manual vector commands drain the oldest retired generation.


Reviewers: 2 done | Synthesis: codex, 8s | Total: 20m26s

@roborev-ci

roborev-ci Bot commented Aug 24, 2026

Copy link
Copy Markdown

roborev: Combined Review (b9028ac)

Semantic document search is generally sound, but two medium-severity correctness issues remain.

Medium

  • Mutable-table offset paging can skip valid resultsinternal/vector/pgvector/document_vectors.go:147, internal/vector/sqlitevec/document_vectors.go:160
    Semantic backend paging uses numeric offsets against a mutable active-generation table. If obsolete-vector cleanup deletes earlier rows between page queries, the next offset can skip surviving vectors, causing scoped semantic search to miss valid results or report exhaustion prematurely. Use a keyset cursor based on the last (distance, token) pair, or keep the complete paged search within a consistent read snapshot.

  • Restart requirement ignores document embedding consentinternal/api/document_search.go:151
    scheduled_registration_requires_restart checks only query consent, but document embedding consent is also captured at daemon startup in DocumentEmbeddingConsented. If document-only consent is recorded after startup, scheduled builds remain inactive until restart while the endpoint incorrectly reports that no restart is required. Track whether document embedding consent was installed in the running vector runtime and include it in this calculation, or make scheduled jobs dynamically re-read consent.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 15m10s

@roborev-ci

roborev-ci Bot commented Aug 24, 2026

Copy link
Copy Markdown

roborev: Combined Review (26e93e5)

Lexical search pagination is unintentionally capped, breaking clients that need results beyond 1,000 matches.

Medium

  • internal/store/document_search.go:377 — The shared candidate limit now applies to lexical searches, reducing the default reachable result set from 10,000 to 100 and imposing a hard maximum of 1,000. Existing clients can no longer paginate through lexical matches beyond that bound.
    • Fix: Preserve the previous lexical default and maximum; apply the 1,000-candidate bound only to semantic and hybrid retrieval.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 14m55s

@roborev-ci

roborev-ci Bot commented Aug 24, 2026

Copy link
Copy Markdown

roborev: Combined Review (dbd98ef)

The change has two medium-severity issues affecting vector-space correctness and lexical-search performance.

Medium

  • internal/vector/document/model.go:75 — Endpoint changes can silently reuse incompatible embeddings.
    The generation fingerprint identifies the provider only by API format, model name, and dimension. Different OpenAI-compatible endpoints may produce incompatible vectors under the same model name, causing queries from a new vector space to rank an old corpus incorrectly.
    Fix: Include a canonical, non-secret deployment identity—such as an endpoint hash or explicit model revision—in the fingerprint so endpoint changes create a new generation.

  • internal/vector/document/search.go:115 — Lexical searches unnecessarily exhaust up to 10,000 candidates.
    Lexical and auto searches page through the entire candidate set before returning the first result page. Each page recomputes bounded content and filename candidates, and every outer cursor request repeats the process, severely regressing ordinary lexical-search performance.
    Fix: Route effective lexical searches through a single SearchDocuments call and preserve its cursor and ranking metadata; use exhaustive collection only for hybrid fusion.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 15m8s

@wesm
wesm force-pushed the feat/document-vectors branch from dbd98ef to 02c7dbe Compare August 24, 2026 12:49
@roborev-ci

roborev-ci Bot commented Aug 24, 2026

Copy link
Copy Markdown

roborev: Combined Review (02c7dbe)

Medium-severity issues remain in embedding configuration validation and malformed provider-response handling.

Medium

  • cmd/msgvault/cmd/documents_vector.go:47 — The consent path permits vector.enabled = false, causing cfg.Vector.Validate() to skip embedding validation. Invalid endpoint, dimension, or input-limit settings can then reach vectordocument.Fingerprint, where validation failures panic instead of producing a CLI error.

    • Fix: Validate the embedding policy independently of lane enablement before fingerprinting, and preferably make fingerprint construction return an error.
  • internal/vector/document/worker.go:603 — Invalid provider response counts, indices, or dimensions surface as ordinary providerErr outcomes and are classified as transient. Deterministically malformed responses or dimension mismatches can therefore trigger repeated hosted calls until the attempt limit.

    • Fix: Introduce typed or sentinel provider-response validation errors and immediately classify them as invalid_provider_shape or invalid_provider_vector.

Reviewers: 2 done | Synthesis: codex, 9s | Total: 23m47s

Add optional lexical, semantic, and hybrid search over indexed attachment
text, with generation-based activation so incomplete vector builds never
replace the active corpus.

Use Docbank's released normalized-document and embedding contracts as the
shared source of truth. Keep document and query egress consent separate,
reject redirects, and page vector candidates until scoped retrieval is
complete and accurately bounded.

This also:
- shares the document-vector CLI consent command and routing
- stabilizes generation, endpoint, and provider identity
- preserves provenance through bounded hybrid fusion
- updates the Go and Nix dependency closures for Docbank's Go 1.27 requirement
- makes vector builds converge safely
- rejects stale normalized identity and invalid embedding policy
- makes malformed provider responses terminal
- makes semantic paging exhaustive and stable
- decouples retired cleanup from consent
- exposes vector status and coherent hybrid evidence
- keeps restore and cleanup recoverable
- preserves lexical search depth and store-native pagination
- refreshes the generated document-search client

Generated with Codex

Co-authored-by: Wes McKinney <wesmckinn+git@gmail.com>
Co-authored-by: Codex <noreply@openai.com>
@wesm
wesm force-pushed the feat/document-vectors branch from 02c7dbe to 5b7835c Compare August 24, 2026 13:33
@roborev-ci

roborev-ci Bot commented Aug 24, 2026

Copy link
Copy Markdown

roborev: Combined Review (5b7835c)

Code review found two medium-severity issues; no critical or high-severity findings.

Medium

  • Concurrent SQLite document-vector operations are not serializedinternal/store/document_vector_lock.go:19
    The SQLite operation lock is a no-op, while cron and per-account post-sync hooks can invoke document-vector work concurrently in the same daemon. Because the archive ownership lock only excludes other processes, lifecycle transitions, cleanup, and hosted embedding passes can overlap and exceed the configured operation bound.
    Fix: Add process-local serialization for SQLite or guard scheduled jobs with a shared non-overlapping runner.

  • Normalized documents lose embedding metadata during round tripsinternal/store/document_normalized.go:107, internal/documentindex/worker.go:419
    Persistence and reconstruction omit SyntheticPrefixLen, TableChunk, CodeChunk, and span Synthetic. Documents using these fields may produce changed embedding inputs and provenance when reconstructed before BuildEmbeddingPlan.
    Fix: Copy these fields into DocumentPublishedChunk/DocumentPublishedSpan, select and scan their database columns when loading, and add a round-trip test covering nonzero synthetic, table, and code metadata.


Reviewers: 2 done | Synthesis: codex, 9s | Total: 23m42s

Cron and post-sync convergence can run at the same time in one SQLite daemon. The archive ownership lock only excludes other processes, so both passes could mutate the same generation concurrently.

Serialize the existing SQLite operation boundary in process. PostgreSQL retains its schema-wide advisory lock.

Generated with Codex

Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Aug 24, 2026

Copy link
Copy Markdown

roborev: Combined Review (3079e0c)

Medium-severity correctness issues remain in generation consistency and normalized-document reconstruction.

Medium

  • internal/vector/document/search.go:82 — Active generation and document-index revision are validated only before provider/backend work. If activation or reindexing occurs during a request, current authority checks may reject old-generation hits, silently returning an empty or partial first page. Revalidate the active generation, target, fingerprint, and revision after collecting candidates; retry or return a stale-cursor error if they changed.

  • internal/store/document_normalized.go:107, internal/documentindex/worker.go:419 — The normalized-document round trip drops SyntheticPrefixLen, TableChunk, CodeChunk, and span Synthetic metadata, so embedding-plan preparation can receive a reconstruction that differs from the original document. Preserve these fields during chunk publication and in the normalized-document query/scan, and add a round-trip test covering table, code, and synthetic metadata.


Reviewers: 2 done | Synthesis: codex, 10s | Total: 25m17s

Semantic candidate collection spans provider and backend calls. An activation or reindex during that window can apply new authority to old hits and silently return a partial response.

Revalidate the active generation, target, fingerprint, and document revision after collection. Return the existing stale-cursor signal so clients can retry deliberately without automatic duplicate query egress.

Generated with Codex

Co-authored-by: Codex <noreply@openai.com>
@roborev-ci

roborev-ci Bot commented Aug 24, 2026

Copy link
Copy Markdown

roborev: Combined Review (6e34a3d)

Code review found two medium-severity issues: retired-vector cleanup depends unnecessarily on embedding runtime configuration, and normalized document loading causes excessive repeated SQL queries.

Medium

  • cmd/msgvault/cmd/documents_vector_runtime.go:50 — Retired-generation cleanup always initializes the enabled embedding runtime and requires a semantic client. If vector features are disabled or provider configuration becomes invalid, documents vectors resume --generation-id fails even though cleanup only needs the backend, leaving retired vectors unpurged.

    • Fix: Add a backend-only cleanup path that works independently of feature enablement, provider configuration, and semantic clients.
  • internal/store/document_normalized.go:133 — Loading a normalized document performs one span query per chunk. Because the worker reloads the complete document for every bounded embedding batch, large documents can trigger thousands of repeated SQL queries, especially with PostgreSQL.

    • Fix: Fetch all spans for the extraction in one ordered query and group them by chunk key. Ideally, cache the result or limit embedding-plan preparation to claimed chunks.

Reviewers: 2 done | Synthesis: codex, 8s | Total: 21m29s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Add document vectors and semantic retrieval for extracted attachments

2 participants