feat: add msgvault eval — retrieval-quality evaluation against relevance judgments - #649
feat: add msgvault eval — retrieval-quality evaluation against relevance judgments#649fmasi wants to merge 32 commits into
msgvault eval — retrieval-quality evaluation against relevance judgments#649Conversation
Adds `msgvault eval`, which scores fts/vector/hybrid retrieval against TREC-style relevance judgments using standard IR metrics (P@10, nDCG@10, R@100, MAP, MRR). Corpus loading (internal/eval/corpus.go) and metric math (internal/eval/metrics.go) are kept free of engine dependencies so scoring can be unit-tested without an archive. Rebased from the original 2026-06 prototype onto current main: the only API drift was hybrid.BuildFilter gaining a dialect rebind parameter, so the call now goes through Engine.BuildFilter like every other caller. Refs kenn-io#367
… double-counting Addresses the review asks on kenn-io#367. Provenance: every run now reports what produced it — topics/qrels paths, corpus size, embedding model and dimension, endpoint, vector backend, generation fingerprint, RRF-k, k-per-signal, subject-boost, indexed vector count and index size on disk — in both the table and the JSON. A retrieval score is not interpretable, or comparable across machines, without them. Operational cost: per-query median and p95 latency sit beside the quality metrics for each mode, so a quality gain cannot hide a latency regression. Thread double-counting (new eval.DedupeKeys): msgvault retrieves messages, but with --doc-key=conversation the judged unit is the thread. A four-message thread filling the top of a ranking is one retrieved thread, not four. Scoring the un-collapsed list counted it four times, inflating precision and driving recall above 1.0. Keys are now collapsed to their best rank. Threaded fixture (internal/eval/testdata/threaded): a small checked-in msgvault-shaped mailbox — real In-Reply-To/References chains, quoted-reply bodies, varied dates, multiple participants — with message-keyed and conversation-keyed qrels. The TREC legal collection is flat, exactly one message per judged document, so it cannot reach this behaviour: the double-counting bug is invisible there and visible here. Tests cover thread collapsing, quoted-reply distractors, and the fixture's own shape so it cannot be silently flattened. Docs: the command help now states which doc-key to use for conversation-keyed imports, and records that topic phrasing is an experimental variable (FTS5 AND semantics make verbose natural-language topics score near zero). Refs kenn-io#367
Neither thread reconstruction nor contextual embedding exists in msgvault
yet, and this change builds neither. What it does is make sure that when
either lands, the eval extends instead of getting rewritten.
DOC-KEY EXTRACTION IS NOW A REGISTRY, NOT AN ENUM. keyOf was a hardcoded
switch between source_message_id and source_conversation_id, with the
same two names repeated in the flag validation and the usage text. The
mapping from a retrieved hit to the judged id now lives in one place
(docKeyFuncs); validation and usage text derive from it. A future
--doc-key=thread backed by an external message-id -> thread-id mapping
is one new entry — a closure over the loaded mapping — with the scoring
core (Evaluate, Aggregate, DedupeKeys) untouched, since it only ever
sees the opaque string keys the extractor returns. The registry contract
is pinned by a test.
TOPICS CARRY AN OPTIONAL QUERY-CATEGORY COLUMN. Whether a question is
answerable from one message ("pointed") or needs synthesizing across
several ("spanning") decides which retrieval levers a benchmark can even
see: a topic set of only pointed questions is structurally blind to
thread-level improvements. LoadTopics accepts an optional third
tab-separated column with that label; two-column files — including every
existing topics file — parse exactly as before, and labeled and
unlabeled lines can mix in one file. When labels are present, both the
table and the JSON output add a per-category metrics breakdown, so a
future thread-level run can show WHERE it wins rather than averaging
the effect away.
The threaded fixture's topics now carry labels on two of three lines
(the third stays two-column, pinning the mixed format) and the fixture
test asserts them.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014LJs3gFyGatPdTtNGDhXQ1
`msgvault eval` opened its own `sql.Open("sqlite3", …)` to the file
`store.Open` had already opened, and a third read-only connection to
vectors.db just to run one COUNT(*). Both are now gone.
- Reuse `mainStore.DB()` for the hybrid engine and the sqlite-vec backend,
the way serve_vector.go does. The raw handle skipped the DSN parameters
store.Open applies (busy_timeout, WAL, the registered driver's
unicode_lower hook) and went around the Store, which CLAUDE.md says every
DB operation must go through.
- Read the vector count through `Backend.DB()`. That accessor exists for
exactly this; the comment claiming the backend's connection must not be
disturbed was wrong.
- Pass `BuildScope` to `hybrid.Config`, as every other caller does.
`ValidateBuildScope` short-circuits to nil on a zero value, so omitting it
silently disabled the index-scope check: an out-of-scope filter would run
against an index with no vectors for that scope and its near-zero hit
count would be scored as genuinely poor retrieval instead of erroring.
- Resolve `[vector.embed.scope]` accounts before deriving the scope or the
generation fingerprint. The fingerprint folds the scope in, so the
unresolved config computed a different one and every query would have
failed as "index stale" on an account-scoped archive.
- Fail clearly on a PostgreSQL archive instead of pointing a sqlite-vec
backend at a PG handle.
- Drop `humanBytes` in favour of the existing `formatSize`, so index size
renders like every other size in the CLI.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014LJs3gFyGatPdTtNGDhXQ1
Both loaders skip lines they cannot parse, which is right for real TREC
files but made a whole-file format mismatch silent. A three-column qrels
file — no iteration column — loaded as an empty-but-valid `Qrels{}` with no
error, and the only downstream signal was "none of the topics had relevance
judgments", which reads like an id mismatch and sends you looking in the
wrong place. A space-separated topics file failed the same way.
`LoadQrels` and `LoadTopics` now return a `LoadStats` alongside the data:
non-blank lines read, lines parsed, lines skipped, with `Lines == Parsed +
Skipped` always holding. The command turns that into an error naming the
likely format problem when nothing parsed, a stderr warning (so `--json`
stays machine-readable) when the skips outnumber the parses, and the counts
are quoted in the "no judgments matched" error so the two failures can be
told apart.
Also fixes the testifylint findings in the fixture tests.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014LJs3gFyGatPdTtNGDhXQ1
…used `--limit`/`-n` had no validation at all, unlike `search`. Worse, the two backends disagreed about what a non-positive value meant: fts fell back to an internal 100 while the vector backend returns nothing for k=0. It is now rejected outright, so the divergence cannot be reached. The metric labels were also fixed at P@10 / nDCG@10 / R@100 regardless of `-n`. A run with `-n 20` cannot have a recall@100 — the number is bounded by the depth, not by retrieval quality — so reporting one invites a comparison against a run that really did look 100 deep. Cutoffs now follow `-n` (`eval.CutoffsForDepth`), clamped only when `-n` is shallower than the standard depth, and the table and JSON name the depth they used. The mode table also gains a `topics` column: the denominators are about to stop being identical across modes, and a mean is not comparable without one. Rendering moves onto an `evalReport` value rather than two long parameter lists. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LJs3gFyGatPdTtNGDhXQ1
…wing failures Three fixes that all come down to the reported number matching what the run did. Collapse-then-truncate ordering. `DedupeKeys` ran *after* the raw message-level `--limit` truncation, so `--doc-key=conversation --limit 100` scored however many distinct threads happened to sit among the first 100 messages — often a quarter of that — while the tool's own docs claim a thread is counted once, at its best-ranked message, up to the requested depth. Retrieval now over-fetches raw hits, collapses, and only then cuts to the requested depth: `eval.OverFetchPlan` asks for limit*4, grows to *16 and *64 while the engine still has more to give, and stops at that documented ceiling. A 1:1 doc-key does not over-fetch at all, so the default path costs nothing extra — which matters in a command that reports latency. One bad topic no longer kills the run. A filter-only topic (`from:alice@example.com`) has no free text to embed, and the resulting "empty query" error from the hybrid engine aborted `runEval` outright, discarding every score already computed for every prior topic and mode. It is now detected before the engine is touched and recorded as a skipped cell. Diagnostics. Hits that failed to hydrate via `GetMessageSummariesByIDs` were dropped with only a code comment — and a vector index pointing at rows the archive no longer has is precisely the staleness this command exists to catch. Those, the skipped cells, depth shortfalls, and the loader's skipped lines are now collected in a `runDiagnostics` and printed under "Diagnostics" in the table, and under `diagnostics` in the JSON. The doc-key registry becomes `newDocKeyRegistry()`, built inside `runEval` after flags are parsed. Its doc comment claimed a future `--doc-key=thread` resolved through an externally loaded mapping file was one entry away; that was not true of a map fixed at program init, whose entries cannot close over a file named by a flag. Now it is. Each entry also declares whether its key collapses, which is what selects the over-fetch above. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LJs3gFyGatPdTtNGDhXQ1
roborev: Combined Review (
|
…te sort `--modes fts` called query.Engine.Search, which has no relevance component at all: it filters, then orders by sent_at DESC. Scoring that as a *ranking* measures the archive's date distribution and reports the result as retrieval quality — and the numbers it produced were not comparable with the vector and hybrid columns beside them, whose BM25 leg is relevance-ranked. Route the mode through Store.SearchMessagesQueryContext instead: the path /api/v1/search?mode=fts serves, ordered by the dialect's subject-weighted BM25 expression over the same messages_fts index the hybrid engine's BM25 leg fuses. Two further divergences close with it — the store path honours search.DeletionScope, so source-deleted messages stop being scored as hits no production search would return, and its from:/to:/cc: filters are the substring matches production does rather than exact-address equality, which used to score a flat zero for a topic real search answers. Retrieval paths return different hit types, so both now project into one evalHit and the --doc-key registry is defined over that alone: a doc-key must mean the same thing whichever engine produced the hit. The regression test seeds a real archive where BM25 and recency disagree, and asserts the eval ranking equals the production ranking and differs from the chronological one; a second test pins the address-filter semantics. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LJs3gFyGatPdTtNGDhXQ1
The over-fetch loop discarded hybrid.ResultMeta and inferred, from the hit count alone, that a short page meant the corpus had run out. For the fused hybrid query that inference is wrong: each signal's candidate pool is capped at k_per_signal, so the engine can hand back fewer hits than asked for while plenty more matching messages exist below the cap. The loop then stopped, recorded no shortfall, and reported a pool-capped ranking as if it were everything retrieval could find — a silently shallow nDCG/recall with nothing in the output to say so. Carry PoolSaturated through fetchResult and split the two cases. Unsaturated and short is still an exhausted corpus and still says nothing, because that is the honest answer. Saturated and short becomes its own counter and its own diagnostic naming the k_per_signal that caused it. Judgment call: the pool is not grown automatically for a retry. k_per_signal is a fusion parameter, not a page size — enlarging it changes which candidates fuse and therefore the ranking, and the run reports it in the provenance block as the setting that produced these numbers. Silently searching at a different one would make the report untrue and the run incomparable with any other. The diagnostic names the setting and says raising it changes the fusion, so the choice stays with whoever reads the numbers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LJs3gFyGatPdTtNGDhXQ1
search.Parse never fails outright. An operator it recognises but cannot read — `before:invalid`, `larger:5X` — is recorded on the query and the filter is simply dropped, leaving a strictly broader query behind. That is a sensible default for an interactive search box, where the user sees the results and fixes the typo; it is silent corruption for a benchmark, where the topic still scores, under its own id, against a question nobody asked. `before:invalid renewal` was evaluated as `renewal`. Every production front door — the search command, /api/v1/search, /cli/search — rejects such a query via Query.Err(). Parse each topic once, before any mode runs it (a malformed filter is a property of the topic, not of the mode, and was being dropped once per mode), check Err, and on failure skip the topic and report its id and the parse error through the existing diagnostics rather than scoring the widened query. The end-to-end test runs the command against a real archive with one clean topic and one malformed one, and asserts the clean topic's ranking and the malformed topic's diagnostic line — pinning both this fix and the FTS path at the call site rather than only in the helpers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LJs3gFyGatPdTtNGDhXQ1
|
Fixed all three, pushed in d145c6b. High (FTS ranking): the eval was reading Medium (PoolSaturated): confirmed it was being dropped. A saturated per-signal pool now reports as a distinct shortfall reason from real corpus exhaustion, naming Medium (topic parse errors): New tests for all three, each verified red first (revert the fix, confirm the new test fails). Full suite green. |
roborev: Combined Review (
|
…r-check lint-ci's testify-helper-check flags any test function with 4+ direct testify package calls (assert.X(t, ...) / require.X(t, ...)) that doesn't route repeated checks through a local `assert := assert.New(t)` / `require := require.New(t)` helper. Convert every flagged function in internal/eval to that house style (already used throughout cmd/msgvault/cmd/*_test.go), dropping the now-redundant t argument on each call. No assertions, expected values, or test logic changed.
`msgvault eval` built `embed.NewClient` unconditionally, so a `vector.embeddings.api_format = "voyage-contextual"` archive — the format kenn-io#589 added for conversation-aware embeddings — was queried with an OpenAI-compatible client: a flat `{"input": [...]}` body posted to `/v1/embeddings` instead of the nested contextual request to `/contextualizedembeddings`, and no `input_type` at all. That matters more here than elsewhere. An eval only ever embeds queries, and the contextual API distinguishes the two roles: documents are embedded with `input_type=document`, queries with `input_type=query`. Scoring a contextual index with vectors from another endpoint and no role measures a protocol mismatch, not retrieval quality — and it is exactly the comparison people have asked this tool for (voyage-context-4 against the OpenAI-compatible path). - Lift the client construction out of `newEmbeddingRuntime` into `newOpenAIEmbedClient` / `newVoyageContextualEmbedClient`, and add `newQueryEmbeddingClient`, which selects between them on `EffectiveAPIFormat()`. Query-only callers cannot use `newEmbeddingRuntime`: it also builds an embed worker and demands a document publisher backend, neither of which a query-time path has or needs. The indexing side keeps using the same two constructors, so the two paths cannot drift on endpoint, model, dimension, limits or retry policy. - `attachVector` calls `Validate()` on the resolved vector config, as serve's precheck does, and selects the client through `newQueryEmbeddingClient` before it opens anything. An `api_format` with no client now fails naming the offending value instead of silently falling back; previously the run got as far as opening vectors.db and died as "vector search not enabled". `Validate()` also subsumes the ad-hoc endpoint/model check it replaces, with a message that names the key. - Report the api format in the run provenance (and in `--json` as `embedding_api_format`). The generation fingerprint already encodes it, but not readably; two runs are only comparable if the protocol behind them is visible next to the model name. The end-to-end test wires the eval command's own vector setup to a config declaring the contextual format and asserts what reaches the wire: the contextual endpoint, `input_type=query`, the nested request shape. Against the old code it fails the way the bug does in production — a flat request to `/v1/embeddings` and a dimension-mismatch error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LJs3gFyGatPdTtNGDhXQ1
|
Confirmed. attachVector always built an OpenAI-compatible client and ignored api_format entirely. Production's newEmbeddingRuntime (serve_vector.go) switches on Embeddings.EffectiveAPIFormat() to pick between the OpenAI-compatible client and NewVoyageClient for voyage-contextual. Pulled that selection logic into a shared constructor both index-side and eval now call, so they can't drift apart. The bigger issue was the query-vs-document role. Voyage's contextual API tags each embed call input_type: document or input_type: query. The old code sent neither, since it never reached the Voyage client at all. Both client types already implement the same EmbedQuery interface, so once the right client gets constructed, the query role comes along for free. Also validates the resolved config now and fails clearly on an unsupported api_format instead of silently falling back. New tests assert the actual wire protocol for both formats: path, input_type, nested vs flat body. |
roborev: Combined Review (
|
…ping it Qrels.RelevantSet returns an empty set for two different facts: a qid the qrels file never mentions, and a qid it judges with every grade at 0. The scoring loop tested len(rel) == 0 and skipped both. Only the first is unscoreable. The second is a real measurement — retrieval looked and found nothing it should have found — and it can only ever score zero, so excluding it removes a zero from every macro average and reports a better run than happened. trec_eval scores such a topic; so must this. Add Qrels.HasJudgments, which answers "does this qid appear at all" without going through the relevant set, and gate the skip on that. A judged-but- all-zero topic now contributes zeros and is counted in topics_evaluated. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LJs3gFyGatPdTtNGDhXQ1
The provenance block exists so a score can never be read without knowing what produced it, but two of its numbers described the tables rather than the retrieval. Corpus size was COUNT(*) over messages and conversations. That counts dedup-hidden duplicates, messages deleted from their source account, and conversations with nothing live left in them — none of which any search this command runs can return. Count the live population instead, reusing store.LiveMessagesWhere (the same predicate Store.SearchMessagesQuery applies for the default active deletion scope) so the two cannot drift, and derive the conversation count from those live messages. IndexedVectors was COUNT(*) over the embeddings table. vectors.db retains a retired generation's rows — vec0 partition-key isolation means retiring does not delete them — and can hold a rebuild in progress, while search reads only the active generation. Scope the count to the generation attachVector already resolves. IndexSizeBytes still reports the whole file, which is the number that should describe the file. Neither is visible on the flat, freshly built TREC corpus this command has been exercised against; both bite on a live archive that has been syncing and re-embedding for a while. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LJs3gFyGatPdTtNGDhXQ1
…pshot Search re-resolves the active generation on every query, but attachVector resolves it once for provenance reporting. A rebuild or activation triggered mid-run can leave the reported vector count stale for topics scored afterward. Documented rather than engineered around: the race window only matters if you reactivate embeddings against the same archive an eval is actively running against, which is narrow enough that a lock or per-query re-check isn't worth the added complexity.
|
Fixed both, pushed in b380968. All-zero qrels: confirmed — a topic id present in the qrels file with every judgment at rel=0 produced the same empty relevant set as a topic id that never appears at all, so both got skipped. Added Qrels.HasJudgments to tell the two apart; a topic judged entirely non-relevant now scores normally (contributing real zeros to the macro average) instead of being silently dropped. Provenance: confirmed too, both halves. Corpus counts now go through the same live-messages predicate search actually uses, so dedup-hidden and source-deleted rows drop out. IndexedVectors is now scoped to the generation_id the run resolved as active, not a whole-table count across every retired generation vectors.db still holds. While fixing that second one we found one more gap in the same vein and decided to document rather than chase: attachVector resolves the active generation once at setup, but the search engine re-resolves it on every query. If someone reactivates embeddings mid-run against the same archive, the reported vector count can go stale for topics scored after the swap. Narrow enough (you'd have to be rebuilding against the same archive you're benchmarking, at the same time) that a lock or per-query check felt like more machinery than the risk warrants. Noted directly in the command help and the field's doc comment instead. |
roborev: Combined Review (
|
`subject:""` is non-empty text that parses cleanly and still carries nothing to search on: the parser drops an empty operator value rather than building a `LIKE '%%'` that matches everything, so Query.Err() is nil and the previous round's parse-error skip lets it through. The fts path then answers the empty query by listing the whole live corpus newest-first, and the topic scores whatever the archive's date distribution hands it — a number that has nothing to do with retrieval, folded straight into the headline mean. Production rejects the identical query (cmd/search.go and the /cli/search handler both test Query.IsEmpty), so reuse that test here and route the topic through the same per-topic skip-and-report path a malformed filter takes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LJs3gFyGatPdTtNGDhXQ1
A topic the qrels file never mentions cannot be scored, and skipping it is right — but one judged topic is enough for the command to print a headline number, so a qrels file matching a fraction of a large topics file reported a mean over a small, self-selected subset and looked like a complete run. Nothing about the scoring changes; the omission just stops being silent. The unscored topic ids are carried in the existing diagnostics block (JSON) and rendered by notes() as "N of M topics had no matching qrels entry", capped by a new eval.FormatIDList so a wholesale qid mismatch does not print a wall of ids over the sentence explaining it. Deliberately no --require-full-coverage flag: partial coverage is normal while judgments are still being gathered, and this command's established answer to "non-fatal but changes what the numbers mean" is a diagnostic, not a hard failure. The JSON field gives a scripted run something to assert on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LJs3gFyGatPdTtNGDhXQ1
A duplicate qid loaded as two independent topics, so every judgment for it was applied once per occurrence: the query got two votes in a macro average that is meant to be one vote per query, and when the repeated lines carried different text the run answered one question while reporting it against the other's judgments. Rejection rather than dedupe-and-count, which is where the existing malformed-line handling stops. The two are different failures: a malformed line contributes nothing, so dropping and counting it leaves the run intact, while a duplicate qid contributes twice and no policy can repair the ambiguity — keeping the first silently discards a distinct question that the file still scores under that id. The qid is the join key to the qrels file; only the file can make it identify one query. The whole file is read before complaining so one error names every offending id (capped by eval.FormatIDList), and the --topics help states the rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LJs3gFyGatPdTtNGDhXQ1
The rankings handed to AveragePrecision and ReciprocalRank are already truncated to -n, so a relevant message below that rank is invisible to them exactly as it is to recall — but they were reported bare, as if they had seen the whole ranking. A run at -n 20 was publishing an "MRR" comparable, on its label alone, with one that retrieved a hundred deep. The computation is unchanged and correct for a truncated list; only the label was wrong. Cutoffs gains Depth — the retrieval depth, as opposed to a cutoff of a metric's own — so the same mechanism that renames P@k/nDCG@k/R@k when -n clamps them now renders MAP@n and MRR@n, in the table header, the per-category header and the JSON keys. The JSON cutoffs block gains map/mrr entries so every metric's depth reads the same way. Depth deliberately stays out of the "depths were clamped" test, which is now Cutoffs.IsStandard(): runs at -n 100 and -n 500 both report the standard P@10/nDCG@10/R@100 and neither is clamped, though their MAP@n differ. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014LJs3gFyGatPdTtNGDhXQ1
The note computed coverage as TopicsLoad.Parsed - len(UnjudgedTopics), which equals scored only when every judged topic also scores. A topic that is judged but parses to no search criteria (or that every mode skips) is neither unjudged nor scored, so that arithmetic overstates coverage and can print a number that contradicts topics_evaluated in the very same report. diag.scored is now set from the same counter runEval already builds topics_evaluated from, and the note reads it directly instead of re-deriving a count from a different pair of fields.
|
Fixed all four, pushed through 97e83a5. Empty parsed queries: confirmed. subject:"" parses without error but to no search criteria, and fts answers that by listing the whole live corpus newest-first. Extended the existing per-topic parse check to also catch Query.IsEmpty(), so it skips and reports instead of silently scoring a date sort. Unjudged topics: added a count and the topic IDs to diagnostics, both table and JSON. Left out the coverage flag you suggested. Partial judgment coverage is normal while a qrels file is still being built, and every other non-fatal condition on this command goes to diagnostics rather than failing the run. A flag would have been the one exception to that. Duplicate topic IDs: rejected outright rather than deduped. A repeated qid scores the same judgments twice, so it double-weights that query in the macro average, and if the two occurrences carry different query text there's no non-arbitrary way to pick a winner. LoadTopics now names every offending id and refuses to load. MAP/MRR labeling: they're now MAP@n/MRR@n, matching what the P@k/R@k fix already did two rounds back. Rankings get truncated to -n before AP and MRR are computed, so a relevant document past that rank is invisible to them the same way it is to recall — the unqualified label was overclaiming what the number covers. One more we caught ourselves reviewing this batch: the new unjudged-topics note computed coverage as Parsed minus unjudged, which only equals what actually got scored when every judged topic also scores. A topic judged but empty-query, or skipped by every mode, is neither unjudged nor scored, so that arithmetic could print a coverage number that contradicts topics_evaluated in the same report. Fixed to read the same counter topics_evaluated is built from. |
roborev: Combined Review (
|
|
Status: every finding through round 5 is fixed and pushed (see the replies above). CI's green apart from test-postgres and test-windows-slow-packages, still running. One more just landed: per-mode corpus provenance under an account-scoped vector generation. It's real, but restructuring the report to carry per-mode corpus counts is a bigger change than anything so far, and it's the sixth round of findings on this PR. Before going further I'd like a steer: keep chasing rounds to zero, or is this a good state to pause and look at as-is? Not trying to shortcut anything real, just aware the CI here runs on your account, not mine, and didn't want to keep spending it without checking that's wanted. |
|
Curious about the round count here, so I looked at how this repo's own merges have handled roborev findings: 45 of the last 48 merged clean, and the 3 that didn't either had a documented reason or, in one case, none — which then broke main. That's enough to keep working through rounds properly rather than stopping. Will keep responding to what comes next. |
Corpus provenance always reported archive-wide live message/conversation counts, even for a pure-vector run scoped to a subset of connected accounts via [vector.embed.scope]. Add VectorMessages/VectorConversations, computed in one query against the embedded-and-scoped population so a transient failure can't desync them, and print them only when they differ from the archive-wide numbers.
|
Fixed, pushed through a10867c. Added VectorMessages and VectorConversations to the provenance block. For vector mode they report the population an account-scoped [vector.embed.scope] generation actually searches. The number comes from intersecting the generation's embedded message ids in vectors.db against main.db's live, scoped, stamped rows. The table only prints the extra line when it differs from the archive-wide corpus, so an unscoped generation, the common case, doesn't get a second line saying the same thing twice. First pass computed messages through the existing Backend.EmbeddedMessageCount, then conversations through a second, separately-erroring query. Reviewing this batch caught the problem. The two calls can desync on a transient failure in just one of them, printing a self-contradictory line ("0 live messages, N conversations") with nothing in the output explaining why. Both counts now come from one query. It reads COUNT(DISTINCT id) and COUNT(DISTINCT conversation_id) off the same row, so they can only ever go stale together. Added a test that deliberately stamps an out-of-scope message into the same generation, to pin down the source_id filter itself rather than let embed_gen alone carry the test for the wrong reason. |
roborev: Combined Review (
|
collectVectorCorpusStats required messages.embed_gen = gen, but Backend.Search reads vectors.db purely by generation_id — a message whose content change reset embed_gen keeps its stale vector searchable until a re-embed actually runs. Drop the embed_gen predicate so counting matches search's own contract instead of undercounting relative to it. fix(query): chunk GetMessageSummariesByIDs to stay under SQLite's parameter limit One id was one bound parameter in the IN-list; eval's dense vector/hybrid modes can over-fetch past SQLite's ~32766-parameter ceiling at a large -n. Chunk into batches of 500 while preserving the caller-order contract other callers rely on for search rank.
|
Fixed both, pushed through c381a40. Corpus provenance: confirmed. SQLite parameter limit: real. Not new to this round, either. New tests for both. One resets an embedded message's |
roborev: Combined Review (
|
The base summary query was chunked to stay under SQLite's bound-parameter limit, but the label-hydration call right after it still passed the full unchunked result set into its own IN-list, leaving the same overflow reachable one call later. Chunk that call the same way.
|
Fixed, pushed through 2026b2c. Confirmed. Chunking the base summary query wasn't the whole fix. The label hydration right after it still passed every result into one IN list. Same overflow, one call later. Wrapped that call in the same 500-id loop, so it now hydrates labels chunk by chunk instead of all at once. Extended the existing chunk test rather than writing a new one. Gave the first message (chunk one) and the last message (chunk two, since the fixture sits at messageSummaryIDChunk + 3) each a label, then checked both come back attached. It's worth being honest about what that proves. 503 ids is nowhere near SQLite's actual 32766-parameter ceiling, so it can't demonstrate the overflow itself. What it does catch is a bug that silently skipped the second label-fetch call, or mis-sliced the chunk boundaries, which is the failure mode this fix could plausibly introduce, even if it isn't the one roborev originally flagged. |
roborev: Combined Review (
|
Rounds 7-8 chunked the same SQLite parameter-limit overflow in internal/query/sqlite.go (the vector/hybrid hydration path); this is the same bug in a separate package, reachable through eval's FTS mode via Store.SearchMessagesQueryContext -> batchPopulateContext -> batchGetRecipients/batchGetLabels. Chunk both into batches of 500 the same way, merging into a shared map per chunk.
|
Fixed, pushed through 21d4d05. Confirmed, and it's the same bug in a different package this time. Rounds 7 and 8 chunked the overflow in Checked whether anything else in that file has the same shape reachable from eval. New test creates enough messages to cross the chunk boundary, puts a recipient and a label on the first message and on the last, and checks both come back hydrated correctly. |
roborev: Combined Review (
|
A binary built with sqlite_vec but not fts5 (or an archive whose FTS5 shadow tables failed to initialize) had Store.SearchMessagesQueryContext silently fall back to a LIKE-based, reverse-chronological scan while the eval report kept labeling the mode "fts" and implying BM25 ranking. requireFTS5ForModes stops the run before the first topic instead. Deliberately does not chase the narrower runtime fallback (searchMessagesQueryNoFTS, triggered when a query that started fine errors mid-run): closing that would mean changing a production API several daemon handlers share, for a rare, already-defensive edge case. Documented instead of built.
|
Fixed, pushed through 66d5db8. Confirmed. The stub only kicks in when sqlite_vec is missing, so a binary built with sqlite_vec but not fts5 gets the real eval command anyway. Checked whether hybrid needs the same guard. It doesn't. Its BM25 leg runs One thing I looked at and didn't fix: |
roborev: Combined Review (
|
…config collectVectorCorpusStats filtered on vecCfg.Embed.Scope.MessageTypes/ SourceIDs directly, but the embed and search paths both go through BuildScope(), which lowercases/trims message types and drops non-positive source ids. A config value like "EMAIL" matched zero rows in the raw version while the archive's own lowercase message_type column matched fine under BuildScope() — silently zeroing a valid corpus count.
|
Fixed, pushed through c86b21f. Confirmed, and it's exactly the mismatch you'd expect. The corpus query filtered on the raw config fields, New test sets |
roborev: Combined Review (
|
# Conflicts: # cmd/msgvault/cmd/serve_vector.go
|
Pushed 665d7f2, merging main back in. Not a review finding, just catching up. Main had drifted about 30 commits since this branch forked, and one of them, the semantic person search feature, touched the same function this PR's own client-sharing fix lives in. Only one file conflicted, Full suite, |
roborev: Combined Review (
|
DedupeKeys dropped a hit whose extracted key was empty instead of keeping it in the ranking. Since MRR/AP/nDCG are rank-position-based, dropping it let every relevant document below it shift up a slot, inflating those metrics by a rank position the run never earned. Give it a key unique to its position instead: still occupies the slot, never resolves to relevant, never collides with another such hit. fix(eval): thread the command's context into schema init and migrations s.InitSchema() and runStartupMigrations(s) both ran on context.Background(), so Ctrl-C during a long schema init or migration on a large archive didn't actually stop it. Use the already-existing context-aware Store methods; added runStartupMigrationsContext as a sibling to the existing background-context helper so the other three callers keep their current behavior unchanged.
|
Fixed both, pushed through 998af0b. Rank inflation: confirmed, and it's a real one. Context propagation: also confirmed. |
roborev: Combined Review (
|
LoadQrels did a plain map assignment per line, so a later line for the same query/document pair silently overwrote an earlier, differently-graded one -- which grade a run scored against depended on nothing but file order, with no diagnostic. Mirrors LoadTopics' existing policy for a repeated qid: read the whole file, then reject naming every conflicting pair in one message. An identical repeat (same grade twice) stays fine.
|
Fixed, pushed through e71ea2a. Confirmed, and it's the same shape as a bug this file already guards against on the topics side. New tests cover both sides: two lines grading "301 docA" as 1 and 0 now fail with both values named in the error, and two identical "301 docA 1" lines still load clean. |
roborev: Combined Review (
|
1 participant
Follow-up to #367.
What this adds
A new
msgvault evalcommand that measures retrieval quality over a set oflabeled queries, so the effect of an indexing, embedding, or fusion change can
be measured rather than guessed.
(
<qid> <iter> <docid> <rel>) and a topics TSV (<qid>\t<query>), bothexternal to the archive. The metric functions in
internal/evalare pure —no I/O, no engine dependencies — and unit-tested in isolation.
fts,vector, and/orhybrid(
--modes), against the local archive.Cutoffs follow
-n: below the standard depths they are clamped to the depththe run actually retrieved and the column headers say so, because "R@100"
from a run that only ever looked 20 deep is a mislabeled number rather than
a hard one.
backend, generation fingerprint, fusion parameters (rrf_k, k_per_signal,
subject_boost), vector count, index size on disk, corpus size, and which
topics/qrels files were used. A quality number is not comparable — or even
interpretable — without these.
metrics, so a quality "improvement" can't hide an operational regression.
--json, plus aDiagnosticssection for thenon-fatal things that used to be invisible: unparseable qrels/topics lines,
hits the vector index returned that no longer resolve to a message row
(index staleness — exactly what this tool exists to catch), and topics a
mode structurally cannot answer.
--doc-key=message|conversationselects whether qrels ids arematched against
source_message_idorsource_conversation_id, sincedifferent judgment sets reference different units (an mbox import keys by
conversation, for example).
A bug this found while being built
msgvault retrieves messages, but with
--doc-key=conversationthe judgedunit is the thread. A four-message thread filling ranks 1–4 is one retrieved
document, not four; scoring the raw list counts the same thread four times,
inflating P@k and pushing recall above 1.0. The ranking is now collapsed so
each thread is counted once, at its best-ranked message (
eval.DedupeKeys,with a regression test that demonstrates the broken numbers on the
un-collapsed list).
Order matters here too, and it is the second half of the same bug: collapsing
after the
-ntruncation caps the result at however many distinct threadshappen to sit inside the first n messages, not n distinct threads. So for a
doc-key coarser than a message the command over-fetches raw hits, collapses,
and only then cuts to the requested depth — growing the pool geometrically
while the engine still has more to give, up to a bounded ceiling
(
eval.OverFetchPlan).-n 100 --doc-key=conversationnow means 100 distinctconversations. Reported latency includes that over-fetch; a 1:1 doc-key does
not over-fetch at all, so the default path is unchanged.
This bug is invisible on flat corpora like TREC Legal (exactly one message per
judged document), which motivated the next item.
A msgvault-shaped test fixture
internal/eval/testdata/threaded/is a small checked-in mailbox with realIn-Reply-To/References chains, quoted replies, multiple participants, varied
dates, multi-message threads plus singletons, and matching topics and qrels at
both message and conversation granularity. Tests guard the fixture's own shape
(so it can't silently be flattened into something that no longer tests
threading), pin the thread-collapsing behaviour above, and cover the
quoted-reply-as-distractor failure mode that flat corpora can't express.
Limitations (deliberate, stated up front)
thread-level scoring against reconstructed threads (conversations as
msgvault's importers already store them, yes; threads recovered from
reply-chain headers, no) and no awareness of contextual/summary-augmented
embeddings. msgvault has neither feature today, so there is nothing to
score — but from separate benchmarking I've done on public Enron data, those
two are the biggest levers for email retrieval quality by a wide margin,
considerably larger than fusion or reranking tweaks. This eval measures what
exists; it doesn't yet measure what matters most.
interactive-task subset usable here has 3 topics. That's enough to prove the
plumbing end-to-end and to sanity-check the metrics, not enough for
statistically meaningful comparisons. Treat absolute numbers from it
accordingly.
semantics, so verbose natural-language topics score near zero on fts while
their keyword reductions score well; dense retrieval can move the other way.
Runs are only comparable across the same topics file (this is documented in
the command help, and the topics path is part of the reported provenance).
Two extension points, and why they're here now
Both exist so that thread-level and context-aware evaluation can be added
later — by anyone — without rewriting the scoring core.
Pluggable doc-key extraction. The scoring core operates on opaque
string ids; the mapping from a retrieved hit to the judged id lives in one
registry (
newDocKeyRegistryincmd/msgvault/cmd/eval.go). It is builtper run rather than at program init, so an entry can close over state that
only exists after flags are parsed — which is what a future
--doc-key=threadbacked by an external message-id → thread-id mappingfile would need. Adding it is one new registry entry; validation, scoring,
and output pick it up unchanged.
Optional query-category column in topics. The topics TSV accepts an
optional third column, a free-form label for the question's shape (e.g.
pointed= answerable from one message,spanning= requires synthesizingacross several). When present, results are additionally broken down per
category (table and JSON); two-column files load exactly as before. The
reason this matters: in my separate benchmarking, whether the topic set
contains spanning questions decided whether thread-level improvements were
visible at all — a benchmark built only from pointed questions is
structurally blind to them. Carrying the label now means the day msgvault
grows a thread-level feature, the eval can already say which question
shapes it helps.
Neither adds a subsystem; together they're a small refactor plus an optional
column, both covered by tests.
Testing
internal/eval: unit tests for all metrics (known values + edge cases),cutoff clamping, qrels/topics parsing (including the category column,
backward compatibility, and the format-mismatch diagnostics), latency
summary, collapse-then-truncate ordering and the over-fetch plan, and the
threaded fixture's integrity.
cmd/msgvault/cmd: doc-key registry contract, and the ranking path against amock engine — depth is filled with distinct keys, no over-fetch for a 1:1
doc-key, the pool grows and then stops, a shortfall is reported, and a
filter-only topic degrades to a skipped cell rather than killing the run.
go build+go testwith-tags "fts5 sqlite_vec"passes.(fts/vector/hybrid, both doc keys, table and JSON output).