Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions install.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
60 changes: 60 additions & 0 deletions lib/install-lib.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
14 changes: 11 additions & 3 deletions src/lib/search-fallback.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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");
}
Expand Down
37 changes: 19 additions & 18 deletions src/mcp-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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,
}))
Expand Down Expand Up @@ -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: [
Expand Down
26 changes: 22 additions & 4 deletions tests/mcp-server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
};
Expand All @@ -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.
Expand All @@ -506,12 +509,27 @@ 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.
expect(formatted).toContain('x'.repeat(200) + '...');
// 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');
});
});
6 changes: 6 additions & 0 deletions update.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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)"
}
Expand Down