[malibio]

Summary

Closes #3791.

models_dir() was always NODESPACE_HOME/.nodespace/models. That is right for isolating a database and a registry, but a run that isolates the home (an agent eval, a test harness) then found no models, and the daemon downloaded a second multi-GB copy into the throwaway home.

NODESPACE_MODELS_DIR, when set and not empty, now names the directory outright. Unset or empty resolves under the home exactly as before. A run that wants the models already on the machine sets it to the real directory instead of linking one into the isolated home.

Scope

• Both daemon consumers already go through models_dir() (the chat models in local_agent_service.rs and the embedding model in assembly.rs), so one change covers both.
• Not changed, deliberately: the app's bundled-model copy (bundled_model.rs) reads the user's home by design, and the embedding fallback in nlp-engine is only reached when the daemon passes no path, which it always does.

Tests

• Four unit tests beside the existing home-override ones: a named directory wins over the home, unset resolves under the state directory, an empty value is no override, and a named directory does not need the home to be resolvable.
• bun run test:changed passes (scripts, types, node-types and the Rust workspace).

Follow-up

The eval runbook's setup block links the real models directory into the isolated home. That becomes NODESPACE_MODELS_DIR=$HOME/.nodespace/models once this lands, in the docs repository.

🤖 Generated with Claude Code

[malibio]

Code Review Summary

The change is small, correct and well-scoped. models_dir() is the only place the daemon decides where models live, and the override is applied there. I traced every consumer and found no path that silently ignores it. No blockers. Two test-quality improvements are worth making before merge, and one safety note belongs in the eval runbook.

Acceptance criteria

• Set and non-empty NODESPACE_MODELS_DIR is the models directory, whatever NODESPACE_HOME is (models_dir_from returns it without resolving the state dir).
• Unset or empty resolves to <nodespace_dir>/models exactly as before.
• No daemon consumer computes the models path another way (details below; the only independent computations are dev/test tooling and the app's bundled copy, each with a stated reason).
• Runbook sets the variable: lives in nodespace-docs, known follow-up.

1. Consumer audit

Daemon consumers, both through crate::models_dir():

• assembly.rs::resolve_model_path (embedding model; passes an explicit model_path: Some(..) to EmbeddingConfig, assembly.rs ~811).
• local_agent_service.rs::open_model_manager -> GgufModelManager::with_dir(dir). Everything inside the manager (.partial files, check_disk_space(&self.models_dir, ..), delete, cancel, final rename, record_verified_sha256(final_path) which writes <model>.verified.json beside the file) derives from that one models_dir field. Download, disk-space check and verification all follow the override.
• CLI model commands are gRPC clients of the daemon (packages/cli/src/commands/model.rs), no path computation. The CLI uses nodespace_dir() only for logs/journal/uninstall.
• nlp-engine resolve_model_path fallback (~/.nodespace/models, config.rs ~330) is only reached when model_path is None; the daemon constructs EmbeddingConfig in exactly one place and always passes Some. PR claim confirmed.
• App bundled_model.rs copies the bundled embedding model to dirs::home_dir()/.nodespace/models and deliberately does not read NODESPACE_HOME; it targets the daemon the app registers, which sits under the real home. It does not need the override for the eval use case (the real models dir already holds the model). Edge: an app launched from a shell that exports the variable would register a daemon looking in the override while the app copies to the real home; unlikely, and the "app copies its bundled model on launch" error text would then mislead.
• Not routed through models_dir() and not relevant to the daemon: packages/agent/src/golden/runner.rs::default_model_path and several live_*/routing_latency test helpers hardcode ~/.nodespace/models/.... Dev tools/tests with explicit overrides.

2. Semantics

• Empty = unset: yes, via path_override_from, same filter as NODESPACE_HOME.
• Relative value: accepted as is and resolved against the daemon's working directory. A launchd/spawned daemon's cwd is arbitrary, so a relative value silently points somewhere unexpected and the daemon would create it and download multi-GB models there. NODESPACE_HOME behaves the same, so it is consistent, but the cost here is larger. Document "absolute path" in the doc comment, or reject a non-absolute value.
• Nonexistent dir: GgufModelManager::with_dir runs create_dir_all (parents included), so it is created. A typo yields a fresh empty directory and, on first use, a multi-GB download into it; worth a line in the runbook.
• Value points at a file: create_dir_all fails, open_model_manager returns Err, and the local-agent service degrades to model_manager: None with a logged error (existing, tested path). Embedding: exists() is false, semantic search disabled with a warning. Graceful, though neither message names the variable.
• Read on each call, not cached: consistent with NODESPACE_HOME; both consumers call it once at startup in practice.

3. Safety (isolated daemon pointed at the real models dir)

I found no automatic deletion or rewriting of model files. Removals under the directory are:

• .partial removal on cancel, on download cancel inside perform_download, on SHA mismatch of the .partial, and in delete. All target <catalog filename>.partial and run only in response to an explicit download/cancel/delete.
• delete_model RPC removes the final model file (guarded against the active model). Explicit client action only; no agent tool or startup path calls it.
• Load-time integrity verification (verify_file_sha256_cached) returns an error on mismatch and never deletes the file. There is no startup cleanup, eviction or GC of the models dir.

Residual risks, none blocking:

• An isolated eval daemon shares the directory with the user's live daemon. If both start a download of the same missing model they append to the same .partial; the SHA check then rejects and deletes the partial, never the final model.
• The eval daemon writes .verified.json sidecars into the real directory (cache only).
• Anything in an eval that invokes the delete-model RPC deletes the user's real model. Runbook should say to set the variable only for runs that never delete models.

4. Tests

• The four unit tests cover models_dir_from and path_override_from well, including "does not resolve the state dir when named".
• Gap (Improvement): nothing exercises the env read in models_dir() or the consumer. assembly.rs already has a resolve_with harness that saves and restores env; one test "with the override set and an isolated NODESPACE_HOME, resolve_model_path returns the file under the override" covers the acceptance criterion end to end.
• Gap (Improvement): existing env-sensitive tests do not clear NODESPACE_MODELS_DIR: resolve_with (its VARS list) and the with_env tests in local_agent_service.rs (shared_local_agent_new_survives_a_real_model_manager_init_failure, shared_local_agent_new_opens_the_model_manager_when_home_is_unset) assume models follow NODESPACE_HOME. A shell or eval session that exports the variable, the very point of this feature, will make them fail or pass for the wrong reason (empty_nodespace_home_finds_no_model_even_when_the_user_home_has_one would find the real model). Clear the variable in those harnesses.

5. AGENTS.md compliance

No lint suppression, no any, no compat shim, no node_type literals, no issue numbers in comments. scripts/check-sync-boundary.ts passes with 0 hits. The variable name is generic.

Findings

Critical Issues

• None.

Suggested Improvements

• packages/daemon/src/services/assembly.rs (tests, resolve_with) and packages/daemon/src/services/local_agent_service.rs (with_env tests, ~8228-8290): clear NODESPACE_MODELS_DIR so an exported value does not change their result.
• Add one test through resolve_model_path with the override set and an isolated NODESPACE_HOME.
• packages/daemon/src/lib.rs models_dir doc: state that the value should be an absolute path, or reject a non-absolute value.

Nitpicks

• Nit: home_override_from is now a one-line passthrough to path_override_from; call the shared helper directly and drop the hop.
• Nit: GgufModelManager::with_dir doc still says the daemon passes the models/ directory under its NodeSpace home; it may now be NODESPACE_MODELS_DIR.
• Nit: golden/runner.rs::default_model_path and the live test helpers compute ~/.nodespace/models/... themselves, so the override does not apply to them (dev tooling with explicit overrides, fine to leave).

Recommendation

APPROVE, no blockers. Fold in the test-isolation fix and the consumer-level test before merge; the rest is optional. Merge is the user's call.

[malibio]

Verdict: APPROVE with suggestions, no blockers (full report in the comment above). Fold in before merge: clear NODESPACE_MODELS_DIR in the env-sensitive tests, add one test through resolve_model_path with the override set, and document that the value should be absolute. Not merging; that is the user's call.

[malibio]

Review addressed (commit e2ed82c)

Addressed

• 🟡 Test isolation. The env-sensitive tests (resolve_with in assembly.rs, and the two with_env tests in local_agent_service.rs) now clear NODESPACE_MODELS_DIR, so a shell that exports it cannot decide their result. Verified by running them with NODESPACE_MODELS_DIR=/nonexistent/exported-in-shell exported: all pass.
• 🟡 Consumer-level test. Two tests through resolve_model_path: a named directory is searched under an isolated home, and it replaces the home's directory rather than adding to it.
• 🟡 Relative path / missing directory. models_dir now documents that the value should be an absolute path that exists, and what happens otherwise.
• Nit: removed the one-line home_override_from hop (path_override_from is used directly).
• Nit: GgufModelManager::with_dir no longer describes only the home-relative directory.

Skipped

• Rejecting a relative value: it matches how NODESPACE_HOME behaves, and the embedding path swallows a models_dir() error with .ok()?, so a rejection would silently disable semantic search, which is worse than an obvious wrong directory. Revisit if a relative value ever bites.
• The dev-tooling and live-test helpers that hardcode ~/.nodespace/models: they have explicit overrides and are not the daemon.

Daemon unit tests: 418 pass. quality:fix clean.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: The reviewer approved with no blockers. The follow-ups are tests, a doc comment, and removal of a one-line passthrough. No behavior of models_dir() changed.

🤖 Generated with Claude Code