[mstomar125]

Part of #2781. This PR fixes the bug the issue describes. The bundled resources/models/ question needs a design decision, so it stays open on the issue; my findings are in a comment there.

Summary

• Daemon (packages/daemon/src/services/assembly.rs, resolve_model_path): the default model path now comes from crate::nodespace_dir(), not dirs::home_dir(). The model therefore follows NODESPACE_HOME, like the database, the registry and daemon.toml already do. Before this change, a daemon isolated with NODESPACE_HOME still loaded the user's real ~/.nodespace/models model and started GPU work. NODESPACED_MODEL_PATH still overrides the path.
• nlp-engine (packages/nlp-engine/src/config.rs, EmbeddingConfig::resolve_model_path): an explicit model_path that doesn't exist now returns NotFound, and EmbeddingService::initialize reports it as ModelLoadError instead of falling into stub mode. Before, it fell back silently to ~/.nodespace/models. The daemon is the only caller that passes an explicit path. The home lookup still applies when no path is given, which is how the standalone live tests use it.
• Callers that relied on the old behaviour:
  • The isolation recipe and the skill-index timeout hint in scripts/eval/preflight.ts now name NODESPACED_MODEL_PATH.
  • .claude/skills/run-app/SKILL.md now says the isolated daemon starts with no embedding model and puts nothing on the GPU, and shows how to opt in.
• Test comment (local_agent_service.rs): a comment claimed that no other unit test in the crate reads $HOME. That was already loose and is now false, so it now relies on nextest running each test in its own process.

This reverses an earlier, deliberate choice. The old doc comment said model resolution read the real home on purpose, so that isolated eval daemons could share the model store. Sharing is now opt-in through NODESPACED_MODEL_PATH. An isolated eval daemon started without that variable has no embedding model, and preflight's skill-index wait times out. Its error now says what to set.

Tests

New tests:

Test	Setup	Expected
daemon services::assembly::tests::empty_nodespace_home_finds_no_model_even_when_the_user_home_has_one	Empty NODESPACE_HOME; HOME holds a placeholder model (stands in for the real ~/.nodespace/models)	None
daemon services::assembly::tests::model_under_nodespace_home_is_chosen_over_the_user_home	Placeholder model under both homes	The NODESPACE_HOME path
nlp-engine config::tests::test_missing_explicit_model_path_does_not_fall_back_to_home	Explicit path missing; HOME holds a placeholder model	NotFound
nlp-engine config::tests::test_existing_explicit_model_path_is_chosen	Explicit path exists	That path
nlp-engine embedding::tests::initialize_fails_when_an_explicit_model_path_is_missing (added in address-review)	Explicit path missing	ModelLoadError, not stub mode

Every test uses a tiny placeholder file and checks path resolution only. Nothing loads a model.

Mutation checks (each mutation applied, the tests run, then the file restored):

Mutation	Result
D1: daemon default reverted to dirs::home_dir()	Both daemon tests fail
D2: daemon models subdirectory misspelt	model_under_nodespace_home… fails
N1: nlp-engine fallback to the home directory restored	test_missing_explicit… fails
N2: nlp-engine exists() check negated	test_existing_explicit… fails (both tests fail)
E1: explicit-path error arm in initialize() removed	initialize_fails_when_an_explicit_model_path_is_missing fails

Other checks:

• Crate-scoped nextest for nodespace-daemon and nodespace-nlp-engine, run with HOME and NODESPACE_HOME set to scratch directories and model-loading files excluded (golden_*, live_terminal_summary, scenario6_update_node_properties): 576 passed, 21 skipped (round 2, after address-review).
• cargo clippy -p nodespace-daemon -p nodespace-nlp-engine --all-targets -- -D warnings -D clippy::unused-async: clean.
• cargo fmt --check: clean.
• bun run scripts/check-pro-boundary.ts: clean.
• scripts/eval/preflight.test.ts: 22 passed.

Out of scope (follow-ups)

• SettingsServiceImpl::with_default_path (settings_service.rs:527) still builds the settings daemon.toml path from $HOME and ignores NODESPACE_HOME. This is the same class of bug: an isolated daemon reads and writes the real settings file, which holds API keys. Meanwhile the local agent reads $NODESPACE_HOME/.nodespace/daemon.toml.
• The chat-model directory (packages/agent/src/local_agent/model_manager.rs:788) comes from HOME. That is the agent lane.

Documentation

• Daemon protocol: protocol stack says all daemon state paths resolve under nodespace_dir() and follow NODESPACE_HOME. This PR brings the embedding model in line with that.
• Daemon CLI lifecycle says NODESPACE_HOME redirects every daemon state path.
• Needs an update: the Setup recipe in development/agent-eval.md should add NODESPACED_MODEL_PATH=$HOME/.nodespace/models/nomic-embed-text-v1.5.Q8_0.gguf to the isolated daemon command. That docs change is not pushed from here. A ready-to-apply patch for it and for the daemon-cli-protocol.md state-path list is in the address-review comment; push it to nodespace-docs main with the merge.

🤖 Generated with Claude Code

[mstomar125]

Pragmatic Code Review — PR #3601

Review Type: Initial Review

Requirements Check (issue #2781, Part A only; Part B is out of scope on purpose):

• ✅ The daemon resolves the model through the existing home helper. resolve_model_path now builds the path from crate::nodespace_dir(), and the PR adds no second NODESPACE_HOME resolver. Production behaviour doesn't change: no shipped profile sets NODESPACE_HOME (core or Pro), so the path is still ~/.nodespace/models/.... The Pro daemon calls core's build_shared_services, so it gets the fix through the pin bump.
• ✅ The same assumption was checked in nlp-engine config.rs. An explicit model_path that doesn't exist used to fall back silently to ~/.nodespace/models. It now returns NotFound. The daemon is the only caller that passes an explicit path. The others (live tests, core integration tests) all use EmbeddingConfig::default().
• ✅ Test: an empty NODESPACE_HOME finds no model, and the real ~/.nodespace/models is never consulted. HOME points at a temp home with a placeholder model, so if the code read the user's home the test would see it and fail, and the real directory is never touched.
• ✅ Test: a model under NODESPACE_HOME is chosen over the one under HOME.
• ✅ The tests use placeholder files only and load no model. They exercise path resolution only.
• ✅ Both behaviours are mutation-checked (D1/D2 for the daemon, N1/N2 for nlp-engine), and the checks are the right ones. D1 (going back to dirs::home_dir()) fails both daemon tests because HOME is redirected to a home that has a model.

Design assessment

The layering choice is right. nlp-engine sits below the daemon, so it can't call nodespace_dir(). Giving it its own copy of the NODESPACE_HOME logic would create two places that decide the home directory, and they would drift apart. Making an explicit model_path authoritative means the crate that owns the home concept decides the path, and the library never swaps in another file. The home lookup that remains is only reached by callers that pass no path (standalone live tests), so it can't undo daemon isolation.

The reversal of the earlier "read the real home on purpose" rule costs less than that old comment implies. Most of the shared store it worried about ("9 GGUFs totalling 39 GB") is chat models. Those still resolve from $HOME (packages/agent/src/local_agent/model_manager.rs:791, default_models_dir), so eval daemons keep sharing them. Only the ~146 MB embedding file becomes opt-in, and nothing re-downloads it in any case. The behaviour now matches what nodespace-docs already promises ("All daemon state paths … resolve under nodespace_dir()"). It also fixes a real problem: isolated daemons were starting GPU work nobody asked for.

The tests change process-global env. That's acceptable here: nodespace-daemon and nodespace-nlp-engine run under nextest, one process per test (package.json rust:test:workspace, scripts/test-gate.ts). The same pattern is already used in main.rs, tray.rs and local_agent_service.rs, and each change is limited to one synchronous call before the variables are restored.

Code Review Findings

1. 🟡 Important: the documented eval setup breaks when this merges, and the docs change is deferred
📁 nodespace-docs development/agent-eval.md (Setup recipe); architecture/daemon-cli-protocol.md (§ protocol stack, the state-path / override paragraph)

• Problem: after this PR, the isolated daemon in the documented Setup recipe (NODESPACE_HOME=/tmp/nodespaced-test … nodespaced) has no embedding model. The skills never get embedded, so preflight's skill-index wait times out after 120 s and every run fails until someone adds a variable that no doc mentions.
• NODESPACED_MODEL_PATH isn't mentioned anywhere in nodespace-docs today. The PR now makes it the supported way to opt in.
• This PR also reverses a deliberate choice that was made specifically for the eval workflow. The people who use that workflow should see the new contract in the docs, not only in a preflight error message.
• Fix: open the docs PR before this merges and link it here, so both land together.
  • Add the variable to the Setup recipe in agent-eval.md.
  • In daemon-cli-protocol.md, add the embedding model to the list of nodespace_dir()-relative state paths, and NODESPACED_MODEL_PATH next to NODESPACED_SOCKET / NODESPACED_DB_PATH as the narrower overrides.
• Principle: update the docs in the same change as the contract, and keep a change that spans two repos atomic.

2. 🟢 Suggestion: "authoritative" NotFound is swallowed into stub mode, and the new doc comment says otherwise
📁 packages/nlp-engine/src/config.rs:298-326, packages/nlp-engine/src/embedding.rs:470-479, packages/daemon/src/services/assembly.rs:826-848

• Problem: the new doc comment says an isolated daemon whose model has gone missing "gets no model rather than the real user's". That's not what happens.
  • EmbeddingService::initialize() catches any resolve_model_path error, sets initialized = true and returns Ok(()), which is stub mode.
  • The daemon then publishes that service, logs "Shared embedding model loaded — semantic search now available", and the processor writes zero vectors.
• When it can happen: only if the file disappears between the daemon's exists() check and the spawn_blocking load, a very small window. Before this PR the same stub result happened whenever the home fallback also found nothing, so this is not a practical regression.
• Fix:
  • Make initialize() return an error when an explicit model_path is missing. Then the daemon's existing load_failed path handles it, and that completes the "explicit is authoritative" intent in about three lines.
  • Or reword the comment and track the change as a follow-up.
  • Either way, the comment also says the daemon's path is "resolved under its own NodeSpace home", but it can equally come from NODESPACED_MODEL_PATH. A lower-level crate describing its caller this specifically will go stale. "Callers that own a home directory resolve the path and pass it; this crate never substitutes another file" says the same thing without depending on the daemon.
• Principle: fail loudly, and make the doc comment match what the code actually does.

3. 🟢 Nit: ambiguous wording in the skill-index hint
📁 scripts/eval/preflight.ts:439-441
"An isolated daemon loads no embedding model unless NODESPACED_MODEL_PATH names one: its log then says "NLP model not found"" can be read as: when the variable is set, the log says not found. Suggested: "…unless NODESPACED_MODEL_PATH names one; without it the daemon log says "NLP model not found"."

4. 🟢 Nit: run-app skill now half-overstates what NODESPACE_HOME relocates
📁 .claude/skills/run-app/SKILL.md:32-33
The skill says NODESPACE_HOME relocates "database, registry, models". After this PR that's true for the embedding model, but chat GGUFs still come from $HOME (model_manager.rs:791). The PR body already scopes the chat-model fix out, which is right. Since this PR is rewriting the skill's model guidance anyway, a short "(embedding model; chat models still load from $HOME)" would stop someone using the skill being surprised.

What's good

• The commit does one thing, and the PR body states the reversal of earlier behaviour openly instead of hiding it.
• The test design makes the pre-fix behaviour show up without touching the real ~/.nodespace: HOME points at a placeholder model, and the tests check which path is returned, not whether a model loads.
• The callers that relied on the old behaviour are covered:
  • The eval preflight recipe and its timeout hint now name NODESPACED_MODEL_PATH.
  • The run-app skill says how to opt in.
  • The test harnesses that set NODESPACE_HOME don't need the embedding model. test-support/src/lib.rs and e2e/daemon-harness.ts run no semantic-search assertions, and the chat-model tests resolve through $HOME. Those harnesses also stop loading a model onto the GPU, which is a small win.
• The local_agent_service.rs comment fix replaces a claim that was no longer true with the guarantee that actually holds (nextest process isolation).

Recommendation

APPROVE. This is a clear improvement: it fixes a real isolation bug, with the right layering and tests that guard against regressions. Please open finding 1's docs PR before merging so the eval recipe doesn't break. Findings 2-4 are optional.

🤖 Generated with Claude Code

[mstomar125]

Automated pragmatic-code-review: APPROVE (posted as a comment — GitHub blocks self-approval). Full findings in the review comment above.

[mstomar125]

Address review: response to the pragmatic review

All four findings are addressed in bc06eb9, except that the docs patch for finding 1 is prepared but not yet pushed (see below).

✅ 2 🟢: a missing explicit model_path no longer falls into stub mode

• packages/nlp-engine/src/embedding.rs: initialize() now returns EmbeddingError::ModelLoadError when an explicit model_path doesn't resolve.
  • The daemon's load task already turns that error into load_failed, logs "Failed to load NLP model — semantic search disabled", and publishes no service.
  • Stub mode stays for callers that give no path.
  • The daemon is the only caller that passes an explicit path.
• config.rs: the doc comment no longer describes the daemon. It now says: "A caller that owns a home directory resolves the path itself and passes it; this crate never substitutes the user's copy for it."
• New test: embedding::tests::initialize_fails_when_an_explicit_model_path_is_missing. Resolution fails before any backend work, so no model is loaded.
  • Mutation E1 (drop the new error arm): the test fails. The other tests still pass.

✅ 3 🟢 Nit: preflight hint wording

scripts/eval/preflight.ts now reads "…unless NODESPACED_MODEL_PATH names one; without it the daemon log says "NLP model not found"."

✅ 4 🟢 Nit: run-app skill

.claude/skills/run-app/SKILL.md now reads "database, registry, embedding model … Chat models still load from $HOME."

📝 1 🟡: docs prepared, not pushed from this branch

nodespace-docs takes commits straight to main, and the established practice is to push a doc change just before bun run merge. The same applies to node-types.md. If the daemon-cli-protocol.md change were pushed now, it would be wrong for as long as this PR is unmerged. The patch below applies cleanly to nodespace-docs main (b18edf0, checked with git apply --check). Whoever merges this PR should push it with the merge.

The agent-eval.md half is correct with or without this PR, because NODESPACED_MODEL_PATH has always overridden the path. It can go in any time.

nodespace-docs patch (agent-eval.md Setup recipe + daemon-cli-protocol.md state paths and overrides)

diff -ru a/architecture/daemon-cli-protocol.md b/architecture/daemon-cli-protocol.md
--- a/architecture/daemon-cli-protocol.md	2026-10-05 12:47:42
+++ b/architecture/daemon-cli-protocol.md	2026-10-05 12:47:42
@@ -55,7 +55,7 @@
         The active path is resolved by `daemon_setup::daemon_socket_relative()` and honored by both the GUI (`grpc_client::resolve_socket_path`) and the node watcher. The `NODESPACED_SOCKET` env var overrides all variants.
     - **Named Pipe:** `\\.\pipe\nodespace-daemon` (Windows)
 
-    All daemon state paths — database, socket, and the multi-database registry (ADR-053) — ultimately resolve under `nodespace_daemon::nodespace_dir()`: `$NODESPACE_HOME/.nodespace` if the `NODESPACE_HOME` env var is set, else `~/.nodespace`. `NODESPACED_SOCKET` and `NODESPACED_DB_PATH` are narrower, single-path overrides layered on top of that root. Both the community daemon and the Pro daemon (`nodespaced-pro`) honor `NODESPACE_HOME` identically — this is what makes it possible to run an isolated daemon instance (e.g. for testing) without touching a real user's live database, socket, or registry.
+    All daemon state paths — database, socket, the multi-database registry (ADR-053) and the embedding model (`models/`) — ultimately resolve under `nodespace_daemon::nodespace_dir()`: `$NODESPACE_HOME/.nodespace` if the `NODESPACE_HOME` env var is set, else `~/.nodespace`. `NODESPACED_SOCKET`, `NODESPACED_DB_PATH` and `NODESPACED_MODEL_PATH` are narrower, single-path overrides layered on top of that root. Both the community daemon and the Pro daemon (`nodespaced-pro`) honor `NODESPACE_HOME` identically — this is what makes it possible to run an isolated daemon instance (e.g. for testing) without touching a real user's live database, socket, or registry.
 
     There is no TCP listener and no HTTP server. The daemon binds a Unix domain
     socket on Unix and a Named Pipe on Windows, and nothing else — there is no
diff -ru a/development/agent-eval.md b/development/agent-eval.md
--- a/development/agent-eval.md	2026-10-05 12:47:42
+++ b/development/agent-eval.md	2026-10-05 12:47:42
@@ -97,6 +97,9 @@
 > `NODESPACED_DB_PATH` alone leaves the daemon **serving the real user
 > database** — an eval run then writes its schemas and chat nodes into live
 > data. `NODESPACE_HOME` redirects the registry and the database together.
+> It moves the embedding model too, so the isolated daemon has none of its
+> own: `NODESPACED_MODEL_PATH` names your copy. Without it the seeded skills
+> are never embedded and the runner's skill-index wait times out.
 > Confirm before running turns: the daemon logs `served_db_path=` at startup,
 > and it must point inside your temp directory.
 
@@ -105,6 +108,7 @@
 
 rm -rf /tmp/nodespaced-test && mkdir -p /tmp/nodespaced-test
 NODESPACE_HOME=/tmp/nodespaced-test \
+NODESPACED_MODEL_PATH=$HOME/.nodespace/models/nomic-embed-text-v1.5.Q8_0.gguf \
 NODESPACED_SOCKET=/tmp/nodespaced-test/daemon.sock \
 RUST_LOG=debug \
   target/release/nodespaced > /tmp/nodespaced-test/daemon.log 2>&1 &

Verification (round 2, in the machine slot)

• Crate-scoped nextest, nodespace-daemon + nodespace-nlp-engine. HOME and NODESPACE_HOME pointed at scratch directories, and the model-loading files were excluded. Result: 576 passed, 21 skipped.
• cargo clippy -p nodespace-daemon -p nodespace-nlp-engine --all-targets -- -D warnings -D clippy::unused-async: clean.
• cargo fmt --check: clean.
• check-pro-boundary: clean.
• scripts/eval/preflight.test.ts: 22 passed.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale:

• The only logic change is the one-arm change the reviewer specified for finding 2.
• It is covered by a new test, and the mutation check confirms the test catches its removal.
• Its one caller, the daemon's model-load task, already handles that error path through load_failed, the same path integrity failures take.
• Findings 3 and 4 change wording only. Finding 1 is a docs patch outside this repository.

🤖 Generated with Claude Code