[malibio]

Summary

Moves every way a conversation reaches a model into nlp-engine: the ChatInferenceEngine interface and its request/response types, the local llama.cpp bridge, the remote OpenAI-compatible client with its discovery, and ndjson. agent keeps the loop, routing, tools and prompts.

• nlp-engine gains reqwest/futures/async-trait, imports nothing from the workspace, and compiles with default features off.
• One prompt dump (inference/prompt_dump.rs): one writer, one seq counter, the same record shape for both engines (kind, engine, seq, target, sent, sent_len). The per-engine copies are deleted.
• scripts/prompt-dump.ts read a stale turn format no engine wrote; it now reads the real records in timeline order, with tests on a mixed local/remote file.
• Nothing is re-exported from the old locations; agent_types no longer re-exports ChatMessage/Role/ToolCallRaw.

Decisions

• model_manager.rs stays in agent. crate-boundaries.md puts "which engine is loaded, chosen from settings" with the agent, and it owns the digest catalog and downloads.
• LlamaChatInferenceEngine::load takes the pinned digest as an argument (it used to call into model_manager, which would make nlp-engine import from agent). Callers pass expected_sha256_for_path, so the integrity gate is unchanged, including in the daemon's production load.
• ModelFamily, ModelInfo, ModelStatus, ModelBackend, ModelError moved too, since ChatModelSpec and discovery use them. The ModelManager trait stays in agent.
• ToolDefinition and the chat engine's ToolSpec are still two identical types; merging them is not a move, so it is left.

Test plan

• bun run test:changed (rust, scripts, types:check, node-types:check) passes
• cargo check -p nodespace-nlp-engine --no-default-features
• Routing/decision evals and a manual local + remote turn: not run here

🤖 Generated with Claude Code

[malibio]

Review Type

Initial review of PR #3755 (closes #3744): move the inference engines into nlp-engine.

Requirements Check

• ✅ agent contains no inference engine: inference.rs, openai_compat_inference.rs, openai_compat_discovery.rs, ndjson.rs and the remote prompt dump are gone from local_agent/; only the ModelManager trait and loop remain.
• ✅ nlp-engine imports nothing from the workspace: Cargo.toml adds only async-trait (workspace dep), futures, reqwest; no path deps. cargo check -p nodespace-nlp-engine --no-default-features (lib) compiles.
• ✅ One prompt dump: single module inference/prompt_dump.rs, one writer, one static SEQ; per-engine copies deleted.
• ✅ Same record shape for both engines (kind, engine, seq, target, sent, sent_len; response kind, engine, seq, raw_response, raw_response_len), tagged by engine; sent is text for local and body for remote. Covered by prompt_records_have_one_shape_whichever_engine_wrote_them.
• ✅ bun run prompt:dump reads a mixed file in order: renderDump + scripts/prompt-dump.test.ts cover timeline order, --prompt, --tools, --last, --raw.
• ✅ One definition per moved type, no re-exports from the old locations (grep for old module paths finds nothing in code; agent_types only uses from nlp-engine).
• ⚠️ No behaviour change: the digest plumbing is preserved (below) and the author ran test:changed, but the routing/decision evals and the manual local + remote turn are explicitly unchecked in the PR test plan.
• ✅ model_manager.rs left in agent, with the reason in the PR body.
• ✅ test:changed passed (per author).

Findings

Verified, no action

• Integrity gate: LlamaChatInferenceEngine::load now takes expected_sha256: Option<&str>. The daemon's production load (packages/daemon/src/services/local_agent_service.rs:2274) passes model_manager::expected_sha256_for_path(&model_path_str), which is the same basename lookup the old in-load code did (new unit test expected_sha256_for_path_resolves_by_basename). Every other caller (agent golden runner, tools.rs:9576, all agent/tests/it and daemon/tests/it live tests) passes the same helper. No caller passes a literal None.
• seq semantics: both engines draw from one counter; prompt/response join on seq; a response carries its prompt's seq. Correct.

Suggested Improvements

• 🟡 packages/nlp-engine/src/inference/local.rs (load): the digest is an Option<&str>, so a future caller can silently skip the integrity gate by passing None, which is exactly the failure the old self-lookup prevented structurally. Principle: make the safe path the default (fail-safe design). Consider two explicit constructors or an enum (Pinned(&str) / Unpinned) so skipping is a visible choice at the call site. Not blocking: all current callers are correct.
• 🟡 Acceptance criterion "no behaviour changes" is evidenced only by compile/unit tests. The PR lists the routing/decision evals and a manual local + remote turn as not run. Please run one local and one remote turn with NODESPACE_PROMPT_DUMP set (this also exercises the new dump end to end) and tick the box, or note why it is waived.

Nitpicks

• Nit: packages/nlp-engine/src/inference/local.rs:16 imports RequestSizes from super::openai_compat; the local engine now depends on the remote client's module for a shared helper. Moving it to engine.rs keeps the two engines siblings.
• Nit: prompt_dump.rs test disabled_dump_writes_nothing_but_still_numbers_calls only asserts b > a; it does not assert nothing was written, so the name overclaims. Also the remote engine only calls dump_prompt when enabled(), so "still numbers calls" holds only for the local path; harmless (seq is only observable when enabled) but the comment/test name implies a guarantee that is not uniform.
• Nit: ToolDefinition and ToolSpec remain two identical types bridged field by field in local.rs; acknowledged in the PR as out of scope, worth an issue.
• Nit: nodespace-docs/decisions/056-gemma-4-e4b-locked-native-model.md:18 still names packages/agent/src/local_agent/inference.rs. It is a historical ADR so leaving it is defensible; if you want zero stale paths, point it at packages/nlp-engine/src/inference/local.rs.
• Note: cargo check -p nodespace-nlp-engine --no-default-features --tests fails on llama_cpp_2 references in chat/mod.rs tests; those lines are untouched by this PR (pre-existing, not a regression).

Docs

The uncommitted nodespace-docs changes (prompt-dump.md, acp-integration.md, docs-inventory.md) match the new record shape and file paths. They are still uncommitted in the docs checkout: commit/push them with this PR landing.

Recommendation

APPROVE. A clean, reviewable move with the integrity gate preserved at every call site and a coherent merged dump. Only the unrun evals/manual turn should be closed out before merge.

[malibio]

Review verdict: APPROVE (self-approval not permitted by GitHub). Full report posted as a comment. Only the unrun evals and manual local/remote turn should be closed out before merge.

[malibio]

Review response

Addressed

• Test name overclaimed: renamed disabled_dump_writes_nothing_but_still_numbers_calls to numbering_stays_monotonic_with_the_dump_off.
• "No behaviour change" evidence, run with NODESPACE_PROMPT_DUMP set, one process, one file: the live stage1_reformulation_for_start_tracking_albums turn on the locked native GGUF (loaded through expected_sha256_for_path), and live_openai_compat_smoke against a stub OpenAI-compatible server on :11434 (not a real provider). Both passed. The file held 4 records with seq 0 (openai_compat) and 1 (native) from one counter, and bun run prompt:dump rendered them in order. The routing/decision evals were not run: this change touches no routing, loop or prompt code.

Skipped

• Option<&str> digest as an enum: every caller passes the helper and a literal None is visible in review; a two-state type is more surface than a move should add. Revisit if a caller outside the catalog flow appears.
• Move RequestSizes to engine.rs: the coupling is pre-existing (the bridge already imported it from the remote client); this PR is a move only.
• ToolDefinition/ToolSpec duplication: pre-existing, not a move.
• ADR-056 path mention: historical record.