From 68c920973548a84cd821e2d4973a0be97c250a31 Mon Sep 17 00:00:00 2001 From: Ed Heltzel <402910+edheltzel@users.noreply.github.com> Date: Wed, 15 Jul 2026 12:11:00 -0400 Subject: [PATCH] fix: provision embedding model in lifecycle + stop rendering RRF scores as percentages (#240) Two regressions found while diagnosing a 23-day silent semantic-search outage. 1. Model provisioning (the root cause of #240) The default embedding model swapped nomic-embed-text/768 -> qwen3-embedding:0.6b/1024 in a72fac2 (#107/#160, 2026-06-22), but no lifecycle script ever pulled it. Every install that updated past #160 silently lost semantic search: hybrid degraded to keyword-only, embeddings stayed at 0, and no surface reported a failure. Adds recall_provision_embedding_model() to lib/install-lib.sh, called from install.sh (self-check) and update.sh (step_verify) -- update.sh being the path that actually regressed. The model name is READ FROM src/lib/embeddings.ts, never hardcoded in bash. A duplicated constant is precisely how the last swap drifted out of sync; hardcoding it here would rebuild the same bug for the next swap. Respects the Ed-locked OPTIONAL invariant (src/lib/embedding-marker.ts:10-13): no Ollama -> informational only, exit 0, never gates the install. 2. Score unit bug The `score` field was polymorphic: raw FTS5 bm25 rank on the FTS-only path (negative, unbounded) and RRF on the fused path (0..~0.033). One formatter rendered both as `(score * 100).toFixed(1) + '%'`, producing an observed "-1121.0%" -- and, once semantic recovered, a meaningless "1.6%" against a 3.3% ceiling. - mcp-server.ts: run the FTS-only list through reciprocalRankFusion so `score` carries one unit everywhere. Same ordering, honest unit. - search-fallback.ts: render `rrf=0.0328`, not `3.3%`. RRF is a relative ranking signal, not a confidence. - mcp-server.ts reused formatHybridResults instead of its hand-copied duplicate, so the bug existed in two places and had to be fixed twice (DRY law). Tests: fixture score 0.83 -> 0.0328 (0.83 was never reachable); regression test asserts no '%' is ever rendered and a negative score cannot print "-1121.0". Verified end-to-end on a live install: model pulled, 14,616 embeddings backfilled, vec index rebuilt, semantic backend knn, and the query that returned "No results found" now returns the decision it was looking for. Refs #240, #241, #226, #107, #160. --- install.sh | 4 +++ lib/install-lib.sh | 60 ++++++++++++++++++++++++++++++++++++++ src/lib/search-fallback.ts | 14 +++++++-- src/mcp-server.ts | 37 +++++++++++------------ tests/mcp-server.test.ts | 26 ++++++++++++++--- update.sh | 6 ++++ 6 files changed, 122 insertions(+), 25 deletions(-) diff --git a/install.sh b/install.sh index 31123c9..0f3ffe4 100755 --- a/install.sh +++ b/install.sh @@ -240,6 +240,10 @@ do_install() { log_info "Running self-check..." local _check_ok=true + # Provision the embedding model before the self-check reports health (#240). + # Optional by design: never flips _check_ok, never aborts the install. + recall_provision_embedding_model + # First: verify every expected symlink is in place. This catches the class # of failure where an inner loop got interrupted between canonical-copy and # symlink creation (we hit this once — canonicals were copied but the diff --git a/lib/install-lib.sh b/lib/install-lib.sh index d21967d..89ecc95 100644 --- a/lib/install-lib.sh +++ b/lib/install-lib.sh @@ -2544,3 +2544,63 @@ recall_link_global() { log_error " ls -la ~/.bun/bin/recall ~/.bun/bin/recall-mcp" return 1 } + +# ─── Embedding model provisioning (#240) ───────────────────────────────────── +# Recall's semantic tier needs a local Ollama embedding model. Before this, +# nothing in the install/update lifecycle provisioned it: the default model was +# swapped (nomic-embed-text/768 → qwen3-embedding:0.6b/1024, #107/#160) and every +# existing install silently lost semantic search — hybrid search degraded to +# keyword-only with no FAIL anywhere. See #240. +# +# 🔒 Respects the Ed-locked OPTIONAL invariant (src/lib/embedding-marker.ts): +# embeddings stay optional. No Ollama → informational, never an error, never a +# non-zero exit. This function reports and repairs; it never gates the install. +# +# DRY: the model name is READ FROM src/lib/embeddings.ts, never hardcoded here. +# Hardcoding it in bash is precisely how the last swap drifted out of sync — a +# duplicated constant is what created #240 in the first place. + +# Echo the embedding model the current build expects. Empty on any failure. +recall_expected_embedding_model() { + bun -e 'import("./src/lib/embeddings.ts").then(m => console.log(m.EMBEDDING_MODEL)).catch(() => process.exit(1))' \ + 2>/dev/null | tr -d '[:space:]' +} + +# Ensure the expected embedding model is present. Best-effort, never fatal. +recall_provision_embedding_model() { + local model + model="$(cd "$RECALL_REPO_DIR" && recall_expected_embedding_model)" + + if [[ -z "$model" ]]; then + log_warn "Could not determine the expected embedding model — skipping semantic-tier check" + return 0 + fi + + if ! command -v ollama >/dev/null 2>&1; then + log_info "Ollama not installed — semantic search stays disabled (optional; keyword search is unaffected)" + log_info " To enable later: install Ollama, then 'ollama pull $model' and 'recall embed backfill'" + return 0 + fi + + if ! curl -sf -m 3 http://localhost:11434/api/tags >/dev/null 2>&1; then + log_warn "Ollama installed but not running — semantic search is degraded to keyword-only" + log_warn " Start it: 'brew services start ollama' (macOS) or 'ollama serve'" + log_warn " Then: 'ollama pull $model' && 'recall embed backfill' && 'recall embed reindex'" + return 0 + fi + + if ollama list 2>/dev/null | awk 'NR>1 {print $1}' | grep -qxF "$model"; then + log_success "Embedding model present: $model" + return 0 + fi + + log_info "Embedding model '$model' missing — pulling it (required for semantic search)..." + if ollama pull "$model" >/dev/null 2>&1; then + log_success "Pulled embedding model: $model" + log_info " Existing records still need embeddings: 'recall embed backfill' && 'recall embed reindex'" + else + log_warn "Failed to pull '$model' — semantic search stays degraded to keyword-only" + log_warn " Retry manually: ollama pull $model" + fi + return 0 +} diff --git a/src/lib/search-fallback.ts b/src/lib/search-fallback.ts index 68e84fe..4d64087 100644 --- a/src/lib/search-fallback.ts +++ b/src/lib/search-fallback.ts @@ -46,8 +46,16 @@ export function shouldFallbackToHybrid( /** * Format hybrid results using the same display shape as memory_hybrid_search: - * `${score}% ${sourceTag} [${table}#${id}] | provenance: ${provenance}` over a + * `rrf=${score} ${sourceTag} [${table}#${id}] | provenance: ${provenance}` over a * 200-char content preview, blocks joined by `\n\n---\n\n`. + * + * `score` is a Reciprocal Rank Fusion score, NOT a percentage (#240). RRF sums + * 1/(60+rank) per list, so it tops out near 0.033 for a rank-1-in-both-lists hit + * and has no upper bound of 1. Rendering it as `${score * 100}%` implied a + * confidence it never carried — and on the FTS-only path, where the field used to + * hold a raw negative bm25 rank, it printed impossibilities like "-1121.0%". + * It is a relative ranking signal: comparable within one result set, meaningless + * as an absolute. Labelled `rrf=` so it can never be misread as confidence. */ export function formatHybridResults(results: HybridSearchResult[]): string { return results @@ -60,11 +68,11 @@ export function formatHybridResults(results: HybridSearchResult[]): string { : "[FTS]"; const preview = r.content.length > 200 ? r.content.slice(0, 200) + "..." : r.content; - const score = (r.score * 100).toFixed(1); + const score = r.score.toFixed(4); // Shared provenanceLabel (ADR-0001): NULL is reported as "unknown", // never guessed. Single-sourced in lib/provenance.ts. const provenance = provenanceLabel(r.provenance); - return `${score}% ${sourceTag} [${r.table}#${r.id}] | ${provenance}\n${preview}`; + return `rrf=${score} ${sourceTag} [${r.table}#${r.id}] | ${provenance}\n${preview}`; }) .join("\n\n---\n\n"); } diff --git a/src/mcp-server.ts b/src/mcp-server.ts index ddf3d2f..cefcfe4 100644 --- a/src/mcp-server.ts +++ b/src/mcp-server.ts @@ -81,6 +81,7 @@ import { notMarkedDuplicateSql } from "./lib/dedup.js"; import { shouldFallbackToHybrid, buildHybridFallbackOutcome, + formatHybridResults, } from "./lib/search-fallback.js"; import { provenanceLabel } from "./lib/provenance.js"; import { detectThreats, summarizeThreats } from "./lib/threat-detect.js"; @@ -345,13 +346,25 @@ export async function hybridSearch( return { results, embeddingsAvailable, semanticBackend }; } - // FTS only fallback + // FTS only fallback. Run the single ranked list through RRF so `score` carries + // the SAME unit as the fused path above (#240). Raw FTS5 bm25 rank is negative + // and unbounded; emitting it into the same `score` field the fused path fills + // with RRF made one field mean two incompatible things, and the shared + // formatter rendered the mix as nonsense (a real "-1121.0%" was observed). + // RRF over one list is just rank-based scoring — same ordering, honest unit. + const ftsOnlyFused = reciprocalRankFusion([ + ftsResults.map((r) => ({ + id: `${r.table === "loa" ? "loa_entries" : r.table}:${r.id}`, + })), + ]); const ftsOnly = ftsResults .map((r) => ({ table: r.table, id: r.id, content: r.content, - score: r.rank || 0, + score: + ftsOnlyFused.get(`${r.table === "loa" ? "loa_entries" : r.table}:${r.id}`) ?? + 0, source: "fts" as const, provenance: r.provenance ?? null, })) @@ -516,22 +529,10 @@ server.tool( semanticBackend, ); - const formatted = results - .map((r) => { - const sourceTag = - r.source === "both" - ? "[FTS+VEC]" - : r.source === "vec" - ? "[VEC]" - : "[FTS]"; - const preview = - r.content.length > 200 - ? r.content.slice(0, 200) + "..." - : r.content; - const score = (r.score * 100).toFixed(1); - return `${score}% ${sourceTag} [${r.table}#${r.id}] | ${provenanceLabel(r.provenance)}\n${preview}`; - }) - .join("\n\n---\n\n"); + // Single-sourced display shape (#240): this was a hand-copied duplicate + // of formatHybridResults, so the score-unit bug existed in two places + // and had to be fixed twice. One definition now. + const formatted = formatHybridResults(results); return { content: [ diff --git a/tests/mcp-server.test.ts b/tests/mcp-server.test.ts index 92858a2..f617afd 100644 --- a/tests/mcp-server.test.ts +++ b/tests/mcp-server.test.ts @@ -457,11 +457,14 @@ describe('hybrid vector-branch provenance (issue #67)', () => { // --------------------------------------------------------------------------- describe('memory_search hybrid fallback (issue #39)', () => { + // score is an RRF score, not a confidence (#240). RRF sums 1/(60+rank) per + // list, so a rank-1-in-both-lists hit tops out near 0.0328 — 0.83 was never a + // value this field could hold. const sample: HybridSearchResult = { table: 'decisions', id: 42, content: 'We standardized on bun over npm for all Recall tooling', - score: 0.83, + score: 0.0328, source: 'vec', provenance: 'user_authored', }; @@ -481,8 +484,8 @@ describe('memory_search hybrid fallback (issue #39)', () => { const outcome = buildHybridFallbackOutcome('which package manager', [sample]); expect(outcome.text).toContain('No exact keyword hits for "which package manager"; showing semantic matches:'); - // Reuses the memory_hybrid_search display shape: score% [TAG] [table#id] | provenance. - expect(outcome.text).toContain('83.0% [VEC] [decisions#42] | provenance: user_authored'); + // Reuses the memory_hybrid_search display shape: rrf=score [TAG] [table#id] | provenance. + expect(outcome.text).toContain('rrf=0.0328 [VEC] [decisions#42] | provenance: user_authored'); expect(outcome.text).toContain('We standardized on bun over npm'); // Metrics: one honest log line, attributed to the fallback path. @@ -506,7 +509,7 @@ describe('memory_search hybrid fallback (issue #39)', () => { { ...sample, id: 7, source: 'both', provenance: null, content: 'x'.repeat(250) }, ]); - expect(formatted).toContain('83.0% [VEC] [decisions#42] | provenance: user_authored'); + expect(formatted).toContain('rrf=0.0328 [VEC] [decisions#42] | provenance: user_authored'); // NULL provenance reports as "unknown", never guessed (ADR-0001). expect(formatted).toContain('[FTS+VEC] [decisions#7] | provenance: unknown'); // Long content is truncated to a 200-char preview with an ellipsis. @@ -514,4 +517,19 @@ describe('memory_search hybrid fallback (issue #39)', () => { // Result blocks are separated by the shared divider. expect(formatted).toContain('\n\n---\n\n'); }); + + // Regression (#240): the score field is never rendered as a percentage. It + // held raw FTS5 bm25 rank on the FTS-only path — negative and unbounded — and + // `(score * 100).toFixed(1) + '%'` printed observed nonsense like "-1121.0%". + test('formatHybridResults never renders a score as a percentage', () => { + const formatted = formatHybridResults([ + { ...sample, score: -11.21 }, + { ...sample, id: 8, score: 0 }, + ]); + + expect(formatted).not.toContain('%'); + expect(formatted).not.toContain('-1121.0'); + expect(formatted).toContain('rrf=-11.2100'); + expect(formatted).toContain('rrf=0.0000'); + }); }); diff --git a/update.sh b/update.sh index b415cd9..85875b3 100755 --- a/update.sh +++ b/update.sh @@ -430,6 +430,12 @@ step_verify() { exit 1 fi + # Provision the embedding model (#240). This is the path that regressed: the + # model default swapped in #107/#160 and update.sh never pulled the new one, + # so every updated install silently lost semantic search. Optional by design — + # never exits non-zero. + recall_provision_embedding_model + "$mem_bin" --version "$mem_bin" stats 2>/dev/null | head -10 || log_warn "recall stats output non-zero (db may be empty)" }