[travsr-mcp] resolve the embed hook backend per repo, not machine-wide

[ritikpal1122]

Closes #547.

The fix

inject_embed_hook resolved the embedding backend with active_backend_id() alone, which reads only the machine-wide default from ~/.travsr/embed.toml. It never consulted <repo>/.travsr/embed.toml. When they differ, the MCP server spawns a sidecar for the wrong model, trips the stored/plugin model_id mismatch guard, and silently disables Step 4 (semantic ANN) for the session, including the docs lane.

This is the same mistake fixed for the daemon in #526, so this mirrors that fix rather than inventing one:

fn hook_backend_id(db_path: &Path) -> Option<String> {
    let repo_root = db_path.parent().and_then(|p| p.parent());
    repo_root.and_then(repo_backend_id).or_else(active_backend_id)
}

The parent().parent() derivation was verified, not assumed: inject_embed_hook has one caller (serve_stdio), whose only caller is crates/travsr-cli/src/main.rs:1263, which builds repo_root.join(".travsr/graph.db").

Regression test

Two tests, #547 in their doc comments. Mutation-checked by reverting the helper to active_backend_id():

test tests::hook_backend_id_falls_back_to_global_when_repo_unconfigured ... ok
test tests::hook_backend_id_prefers_repo_config_over_global ... FAILED
  left: Some("global-backend")
 right: Some("repo-backend")

left: Some("global-backend") is itself evidence the HOME isolation works: the test read the faked ~/.travsr/embed.toml, not the developer's real one. Isolation follows the existing repo pattern (tempdir HOME, save/restore, and an ENV_LOCK mutex because HOME is process-global and tests run in parallel).

On consolidating the duplication

hook_backend_id now exists in both travsr-mcp and travsr-daemon. Deliberately left duplicated.

A shared home is legal (travsr-plugin-host already owns both repo_backend_id and active_backend_id, and both crates already depend on it), but it would not have prevented this bug: the buggy code called active_backend_id() directly and would have kept doing so. The failure was that the call site never learned the rule, not that an implementation diverged.

The durable fix is at the source. active_backend_id()'s doc comment already says to use repo_backend_id for per-repo decisions, but nothing enforces it. Making that mechanical (renaming it to something like machine_default_backend_id() so the wrong choice reads wrong at every call site, or exposing a single resolve_backend entry point) would close the class. That is a travsr-plugin-host API change touching every caller, so it belongs in its own issue.

Verification

cargo test -p travsr-mcp --lib          593 passed, 0 failed, 2 ignored
cargo clippy -p travsr-mcp --all-targets  clean
cargo fmt -p travsr-mcp -- --check        clean
cargo check --workspace --all-targets     clean
bash .github/scripts/check-em-dash.sh     OK

Note: cargo test -p travsr-mcp without --lib shows 28 failures in the conformance target, all CARGO_BIN_EXE_travsr is unset. Pre-existing and unrelated - verified by running the same target on the base commit, which fails identically. They shell out to the travsr binary, which is not built when targeting -p travsr-mcp alone.

[raj-rkv]

Reviewed against a worktree of origin/master, diffed at the merge base ef3eac1.

The fix is right, and I confirmed it closes the loop rather than only changing one side. The reason this bug bites is that the spawn path already resolved per-repo. resolve_backend_paths (embed_catalog.rs:1801) is:

let repo_root = db_path.parent().and_then(|p| p.parent())?;
let backend_id = repo_backend_id(repo_root).or_else(active_backend_id)?;

So before this PR the hook resolved active_backend_id() alone while the sidecar spawn resolved repo-then-global, and the two disagreeing is exactly what trips the stored/plugin model_id guard #547 describes. After it, both sides compute the same id from the same input. That is the part worth checking, and it holds.

The rest of the description checks out too:

• travsr-daemon's hook_backend_id (lib.rs:10593) is byte-identical apart from the issue number in its doc, so "same helper" is accurate.
• inject_embed_hook really does have one call site (lib.rs:100, inside serve_stdio), and serve_stdio_global does not inject at all, so nothing else reaches this.
• The mutation check reproduces exactly as reported: reverting the helper fails ..._prefers_repo_config_over_global with left: Some("global-backend") and leaves the fallback test passing.
• The Windows-ignore asymmetry is correct rather than an oversight: the first test never reaches active_backend_id, since the repo file wins before HOME is consulted, so HOME isolation is irrelevant to it and ignoring it would lose real Windows coverage.

Local: rustfmt clean, cargo clippy --workspace --all-targets -- -D warnings clean, cargo test --workspace 2456 passed / 0 failed, auto-merges onto current master. 24 CI checks green.

One finding on the test isolation, in the inline comment. It is a one-line fix and the lock it needs already exists.

[raj-rkv]

This is a second HOME lock in a binary that already has one, and they do not exclude each other.

crates/travsr-mcp/src/seed.rs:740 already declares

pub(crate) static DOCS_ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());

pub(crate) deliberately, and its own doc comment makes the same argument this one does:

Callers must hold [DOCS_ENV_LOCK]: HOME is process-global, and that is the same lock the env knobs are already serialized on.

DocsConfigEnv::new() (seed.rs:6214) sets HOME, and its Drop restores it. Those tests and these are in the same travsr_mcp lib test binary, which runs 595 tests across a thread pool, so two disjoint mutexes mean the two groups can interleave freely.

I proved it rather than reasoned about it. A thread takes DOCS_ENV_LOCK and repoints HOME exactly as DocsConfigEnv::new() does, while ENV_LOCK is held:

PROBE before interference: Some("global-backend")
PROBE docs test acquired DOCS_ENV_LOCK while ENV_LOCK is held
PROBE after interference:  None
PROBE fallback assertion would FAIL

It never blocks, and afterwards hook_backend_id returns None where hook_backend_id_falls_back_to_global_when_repo_unconfigured asserts Some("global-backend"). The reverse direction is just as live: this test restoring HOME mid-flight breaks the docs tests, which is the thing their lock was added to prevent.

Not a product bug, and not one CI has hit yet, but it is a flake that would surface as an unrelated test failing for no visible reason, which is the expensive kind. The fix is to use the lock that already exists:

use crate::seed::DOCS_ENV_LOCK;
let _guard = DOCS_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner());

My probe imported it from lib.rs unchanged, so no visibility change is needed. If the name reads oddly outside the docs tests, promoting it to a crate-level ENV_LOCK used by both places is the same edit and reads better, since it is not really about docs config, it is about HOME.

[raj-rkv]

Two notes on this helper, neither blocking.

The duplication is three copies, not two, and the third already lives in the crate you name as the right long-term home. resolve_backend_paths (embed_catalog.rs:1798-1802) already opens with these exact two lines, already takes a db_path, and is already in travsr-plugin-host:

let repo_root = db_path.parent().and_then(|p| p.parent())?;
let backend_id = repo_backend_id(repo_root).or_else(active_backend_id)?;

I think your argument for not consolidating is right, and for the reason you give: the bug was a call site that never learned the rule, not an implementation that diverged, so a shared home would not have caught it. But it does change the cost of the follow-up you describe. Exposing what is already written there, as a pub fn backend_id_for_db(db_path: &Path), is a much smaller change than renaming active_backend_id across every caller, and it gets most of the same benefit: the per-repo rule becomes something you call rather than something you remember. Worth mentioning in the issue you file.

The parent().parent() verification misses a branch. The description says it was verified rather than assumed, via main.rs:1263 building repo_root.join(".travsr/graph.db"). That is the else arm; the statement is

let db_path = if let Some(p) = db { p } else { repo_root.join(".travsr/graph.db") };

so travsr mcp --stdio --db /somewhere/else.db hands this an arbitrary path and parent().parent() is not a repo root.

The behaviour is fine, which is why this is a note rather than a finding: repo_backend_id finds no embed.toml there and it falls through to active_backend_id(), which is precisely today's behaviour. And resolve_backend_paths makes the identical assumption on the spawn side, so hook and spawn still agree even in that case, which is the invariant that actually matters. Worth a sentence in the doc comment saying a --db outside a repo degrades to the machine default, since the next reader of parent().parent() will ask.

[ritikpal1122]

Both addressed in 4de577a.

The lock. You are right, and the part that settles it is that DOCS_ENV_LOCK's own doc comment already records this exact shape happening once before:

Two of them previously held two independent module-local locks, which serialized each module against itself while still racing the other, a latent flake that only showed up once a third module's tests widened the window.

I reintroduced the thing that comment was written about. Now aliased to the existing lock at the use site:

use crate::seed::DOCS_ENV_LOCK as ENV_LOCK;

I took reuse over promoting it to a crate-level ENV_LOCK. The rename reads better and I agree it is what the lock actually guards, but it touches 34 call sites across seed.rs, query.rs and tools.rs, which is not this PR's business. Left the alias so the name reads correctly here.

The --db branch. Correct, I verified the wrong arm. Documented rather than changed, since the behaviour is right:

parent().parent() assumes db_path is <repo>/.travsr/graph.db, which holds for the default path. An explicit --db <path> outside a repo yields no embed.toml there and falls through to the machine default, which is the pre-existing behaviour and not a regression. resolve_backend_paths on the spawn side makes the same assumption from the same input, so hook and spawn still agree in that case, which is the invariant that matters.

On the third copy. Good catch that resolve_backend_paths already opens with these two lines in the crate I named as the right home. That does make the follow-up much cheaper than the rename I described: exposing pub fn backend_id_for_db(db_path: &Path) from what is already written there gets most of the benefit. I will put that in the issue rather than this PR.