Expose Gemma 4 26B-A4B as an optional model tier, park 12B, fix per-model RAM gating

[malibio]

Summary

• Re-evaluated Gemma 4 12B (both ggml-org and Unsloth GGUF sources) end-to-end on adequate hardware via the agent-matrix eval. The upstream peg-gemma4 parser-loop bug (Upgrade llama-cpp-sys-2 when Gemma 4 12B peg-gemma4 parser fixes land upstream #1348) is confirmed fixed in code, but both sources still produce malformed tool-call JSON on complex nested payloads (create_schema) — a genuine model-capability limitation at Q4_K_M, not a GGUF-source defect. 12B remains parked; min_memory_gb corrected from 16 to 24 on both catalog entries per Upgrade llama-cpp-sys-2 when Gemma 4 12B peg-gemma4 parser fixes land upstream #1348's OOM evidence.
• Added and exposed Gemma 4 26B-A4B (MoE, 3.8B active params) as an optional high-RAM tier: scores below E4B overall (12.3/16 vs 13.7/16 mean across 3 reps) but never exhibits 12B's JSON-corruption failure mode — supports an active-parameter-footprint explanation for 12B's failure. Not the default; recommended_model_id() is unchanged.
• Fixed a latent bug in both frontend RAM-gating surfaces (model-manager.svelte, ai-chat-model-selector.svelte): both compared system RAM against a single hardcoded 16GB constant instead of each model's own min_memory_gb. Exposing 26B-A4B's real 32GB floor would have hit this immediately. Both now gate per-model.
• Added NODESPACE_PROMPT_DUMP instrumentation for capturing full LLM conversations during debugging: nlp_engine::chat::prompt_dump wired into ChatEngine::generate_blocking (the single chokepoint every native-path caller — Stage-1 routing, Stage-2 ReAct turns, resolve_query, the routing probe — passes through), and agent::local_agent::openai_compat_prompt_dump for the separate OpenAI-compatible HTTP path. Replaces the prior Stage-2-only dump mechanism, which never covered Stage-1 routing.

Full investigation writeup in ADR-056's addendum (nodespace-docs, pushed separately — that repo has no PR flow).

Related issues filed during this investigation

• Redesign agent-matrix eval scenarios around NodeSpace's actual dev-workflow use case #1977 — agent-matrix eval scenarios don't reflect NodeSpace's actual use case
• Reconsider the nlp-engine/agent crate boundary for LLM connectivity code #1978 — nlp-engine/agent crate boundary for LLM connectivity code
• AI chat nodes can render empty in the UI after a WatchNodes reconnect, despite intact database content #1979 — AI chat nodes can render empty in the UI after a WatchNodes reconnect
• Give Stage-1 routing a bounded schema/entity-type signal so it stops treating known types as ambiguous #1980 — Stage-1 routing lacks schema/entity-type context, over-clarifies

Test plan

• bun run test (frontend, 180 files / 4292 tests) — clean
• cargo test --release -p nodespace-agent -p nodespace-nlp-engine — 123/123 passing
• cargo clippy -p nodespace-agent -p nodespace-nlp-engine -p nodespace-app --all-targets -D warnings — clean
• Pre-push gate (test:all + cargo build --bin nodespaced + test:e2e) — passed
• Manual verification via live daemon + Tauri app: confirmed both E4B and 26B-A4B render correctly in Settings and the AI Chat model selector on a 48GB machine; confirmed NODESPACE_PROMPT_DUMP captures full Stage-1 and Stage-2 conversations end-to-end

Closes #1956.

🤖 Generated with Claude Code

https://claude.ai/code/session_015CwAKk1YU2zyLNzW74mYvH

[malibio]

Automated pragmatic-code-review verdict: REQUEST CHANGES (posted as a comment — GitHub blocks self-approval/self-request-changes on your own PR).

Pragmatic Code Review — PR #1981 (Initial Review)

Reviewed: issue-1956-gemma-12b → main at commit 4a68acaf

Summary

This PR delivers on issue #1956's re-evaluation mandate and, in the process, surfaces and fixes two real latent bugs (stale min_memory_gb: 16 catalog values, and a flat-16GB frontend RAM gate that ignored per-model requirements). The investigation's conclusions are backed by the actual diff, not just PR-description prose: the exposed-model set, the new catalog entry's metadata, and the frontend gating logic all check out. The prompt-dump split into two parallel, env-var-compatible modules is architecturally justified — nlp-engine's Cargo.toml confirms it has zero internal workspace path dependencies, so the openai_compat engine genuinely cannot reuse it — and the hot per-token loop in ChatEngine::generate_blocking correctly gates the string-accumulation cost behind the dump-enabled check.

Net positive overall, with one live logical inconsistency that this PR's own min_memory_gb fix introduces, which should be resolved before merge.

Scope note: confirmed the diff does not bundle in anything belonging to the separately-filed follow-ups (#1977 eval scenarios, #1978 crate boundaries, #1979 WatchNodes reconnect, #1980 Stage-1 routing context) — it's limited to the 12B re-evaluation, the 26B-A4B addition, the RAM-gating fix, and the prompt-dump refactor, consistent with the PR's own explanation of scope.

Findings

🔴 Critical

packages/agent/src/local_agent/model_manager.rs:249 vs. :151,190 — RAM_THRESHOLD_MEDIUM (16GB) now contradicts the corrected min_memory_gb: 24 for both 12B entries.

This PR correctly raises GEMMA_4_12B and GEMMA_4_12B_UNSLOTH's min_memory_gb from 16 → 24 (per #1348's GPU-OOM evidence), but leaves const RAM_THRESHOLD_MEDIUM: u64 = 16 * 1024 * 1024 * 1024; unchanged. recommended_model_id_for(ModelFamily::Gemma4) (public fn, line 374) still recommends gemma-4-12b-q4km for any machine ≥16GB — including 16–23GB machines that, per this PR's own corrected value, don't meet the model's memory floor. Before this PR the two numbers agreed (16 == 16); this PR's own correctness fix breaks that agreement without updating the threshold to match.

Mitigating factor confirmed in the diff: the actually-used default-selection path, recommended_model_id(), explicitly bypasses this function for Gemma4 with a comment warning future readers not to "simplify" it back, specifically because 12B/31B are parked. So the bug is not reachable through the real default-selection flow today. But recommended_model_id_for is a pub fn with its own direct unit test (recommended_model_id_for_family_returns_family_match) that checks family matching only, not the threshold value — so nothing catches the drift, and any future caller of this public function (or a refactor that "simplifies" the bypass away, which the comment itself anticipates as a risk) inherits a self-contradictory contract.

Fix: raise RAM_THRESHOLD_MEDIUM to 24GB to match the corrected min_memory_gb, or, if this function's tiering logic is meant to be superseded by the parked status, remove/clearly gate it rather than leave a stale threshold live in a public API.

🟡 Improvements

• packages/nlp-engine/src/chat/mod.rs:625-627 — prompt_dump::enabled() is called once per generated token inside the hot sampling loop. Each call does an std::env::var() lookup even when dumping is disabled (the common case) — the string-accumulation cost is correctly gated, but the check itself isn't hoisted, even though the same loop already computes dump_seq once before entry (~line 556). Hoist to let dump_enabled = prompt_dump::enabled(); before the loop and reuse the bool. Minor in absolute cost, but avoidable and the pattern to follow is already adjacent in the same function.

• packages/nlp-engine/src/chat/prompt_dump.rs records carry no "engine" discriminator, while openai_compat_prompt_dump.rs's records do ("engine":"openai_compat"). The openai_compat module's own doc comment frames the two as symmetric ("same record shape... one continuous timeline"), but a merged log can only positively identify openai_compat records — native/nlp-engine records are identifiable only by field absence. Add a positive "engine": "native" (or similar) to the nlp-engine side to match the claimed symmetry.

• packages/nlp-engine/src/chat/prompt_dump.rs:11 — doc comment still references agent::local_agent::prompt_dump, the module this PR deletes and replaces with agent::local_agent::openai_compat_prompt_dump. Stale cross-reference; update to the new path.

• packages/desktop-app/src/tests/components/model-ram-gating.test.ts — reimplements the gating predicate as a local mirror of the .svelte logic rather than exercising the component's actual $derived expressions, so it would keep passing even if the real logic drifted (e.g., an off-by-one, or a flat constant creeping back in). This follows a documented pre-existing project pattern (per the test file's own comment) so it's not new scope creep from this PR, but it's the primary regression guard for the exact bug just fixed here, so it's worth flagging.

🟢 Nits

• Nit: chat_models.rs references ADR-056 for the E4B lock, while model_manager.rs comments reference ADR-046 for 12B/31B parking status, in adjacent code. Worth a quick sanity check that these two ADR numbers aren't transposed, since they sit side by side and a swap would be easy to miss.
• Nit: openai_compat_prompt_dump.rs:19-22 doc-comment phrasing ("Shared... only in name, not value") is a little awkward — the two SEQ counters don't actually share a name either, each is defined independently. "Parallel, independently-numbered counters, disambiguated by the engine field" reads clearer.

Verification performed against the diff (not taken on faith from the PR description)

• min_memory_gb: confirmed it really was 16 for both 12B entries pre-PR, and the diff changes both to 24 — issue #1956's body claim that the unsloth entry "already declares" 24 was stale; this PR's fix is real and directionally correct.
• EXPOSED_GGUF_MODEL_IDS: confirmed exact set is ["gemma-4-e4b-q4km", "gemma-4-26b-a4b-q8"]; neither 12B id present.
• 26B-A4B catalog entry: has a dedicated test asserting family, quantization, size, URL, and min_memory_gb; catalog-count assertion updated 9→10.
• Both prompt_dump modules capture pre-truncation/pre-normalization text, enabled()/dump_path() short-circuit correctly as a no-op when unset, dumping only happens when the user explicitly sets NODESPACE_PROMPT_DUMP (no default/predictable path), and the two AtomicU64 SEQ counters are confirmed independent (not shared).
• nlp-engine leaf-crate claim confirmed via Cargo.toml — no internal nodespace-* path dependencies, so the separate openai_compat dump module is architecturally necessary, not duplicated effort.
• Old Stage-2-only agent::local_agent::prompt_dump cleanly removed; no dangling references to deleted items found repo-wide.
• No lint suppressions, no new unwrap()/expect() outside test code, no raw console.log in touched .svelte files, no any types in the new TS test file.

Recommendation: REQUEST CHANGES

The single Critical finding is small in scope — likely a one-line constant fix (or removing/gating the stale function) plus a test assertion — but it's a real, self-contradictory public API left behind by this PR's own correctness fix, not a pre-existing issue merely left unaddressed. Everything else (Improvements/Nits) would not by itself block merge. Recommend resolving the RAM_THRESHOLD_MEDIUM/min_memory_gb mismatch before merge; the rest can be addressed in this PR or deferred at the author's discretion.

[malibio]

Addressed review feedback

🔴 Critical — fixed. RAM_THRESHOLD_MEDIUM (16GB, shared with Ministral) was being reused as the Gemma4 tiering threshold too, so this PR's own min_memory_gb: 16 → 24 correction for both 12B entries left recommended_model_id_for(Gemma4) recommending 12B to 16–23GB machines below its own corrected floor. Added a separate RAM_THRESHOLD_GEMMA4_MEDIUM (24GB) instead of overloading the shared constant, and a new test (ram_thresholds_agree_with_the_catalog_entries_they_gate) that asserts every RAM threshold constant matches the min_memory_gb of the catalog entry it gates.

Bonus finding from that new test: it also caught a second, pre-existing mismatch it wasn't written to look for — RAM_THRESHOLD_LARGE (32GB) never matched GEMMA_4_31B.min_memory_gb (24GB), predating this PR entirely. Since 31B is parked/unevaluated (unlike 26B-A4B, which this PR actually validated end-to-end), the Gemma4 "large" recommendation tier now points at GEMMA_4_26B_A4B instead of GEMMA_4_31B — RAM_THRESHOLD_LARGE already equals GEMMA_4_26B_A4B.min_memory_gb (32GB) exactly, so no threshold value changed, just which model it recommends. 31B is no longer reachable from any recommendation path, matching both 12B entries' parked status.

🟡 Improvements — all addressed:

• Hoisted prompt_dump::enabled() out of the per-token hot loop in ChatEngine::generate_blocking into a single bool checked once per generation.
• Added an "engine": "native" discriminator to nlp_engine::chat::prompt_dump's JSON records, matching openai_compat_prompt_dump's "engine": "openai_compat", so a merged dump file can positively identify both instead of only one by field presence.
• Fixed a stale doc-comment cross-reference to the deleted agent::local_agent::prompt_dump module.

🟢 Nits:

• Reworded the awkward "shared only in name, not value" doc comment on the two independent SEQ counters.
• Checked the ADR-046/ADR-056 reference concern — confirmed correct as written (046 = original 12B/31B parking, 056 = E4B default lock), not actually transposed despite sitting side-by-side. No change needed.

Skipped: restructuring model-ram-gating.test.ts to exercise the .svelte components directly rather than mirroring their gating predicate. This is a documented pre-existing project convention (per the test file's own comment), not something introduced by this PR, and reworking that pattern is disproportionate scope for this review pass.

Re-Review Decision

Decision: RE-REVIEW NEEDED

Rationale: the critical fix touches a public function (recommended_model_id_for) with real behavioral changes beyond the minimal fix the original review anticipated — specifically the additional 31B→26B-A4B recommendation swap, which the original review never saw or evaluated (it was discovered by the new regression test after the initial review). That's a second architectural decision layered onto the critical fix, not a mechanical patch, so it warrants a fresh look rather than being waved through on the strength of the first review's approval-in-spirit of "the critical finding is small in scope."

[malibio]

Automated re-review of the delta commit 4a68acaf..0381ba7f in response to the prior REQUEST CHANGES review.

Re-Review: PR #1981 (delta 4a68aca..0381ba7)

Previous Review Summary

• Prior verdict: REQUEST CHANGES at 4a68acaf
• Findings: 1 Critical (RAM_THRESHOLD_MEDIUM left at 16GB after min_memory_gb corrected to 24 for both 12B entries, so recommended_model_id_for(Gemma4) recommended 12B to machines below its own corrected floor), 4 Improvement (hoist prompt_dump::enabled() out of the per-token hot loop; missing "engine" discriminator on native prompt-dump records; stale doc-comment reference to a deleted module; model-ram-gating.test.ts mirrors component logic instead of testing it directly), 2 Nit (possible ADR-046/ADR-056 mix-up; awkward SEQ doc comment wording)

Addressed / Not Addressed

• [Fixed, with a caveat] Critical: RAM threshold drift — new RAM_THRESHOLD_GEMMA4_MEDIUM (24GB) constant added and correctly wired into recommended_model_id_for's Gemma4 branch; verified by a new regression test that passes. See below for a leftover stale-comment issue this same fix introduced elsewhere.
• [Fixed] Improvement: prompt_dump::enabled() hot-loop call — hoisted to dump_enabled, computed once before the loop, single use site inside it. Verified no stale duplicate call remains.
• [Fixed] Improvement: missing "engine" discriminator — "engine": "native" added to both dump_prompt and dump_response JSON records in nlp-engine/src/chat/prompt_dump.rs.
• [Fixed] Improvement: stale doc-comment module reference — prompt_dump.rs's module doc now correctly points at agent::local_agent::openai_compat_prompt_dump.
• [Deferred, reasonably] Improvement: model-ram-gating.test.ts — confirmed untouched by this delta (no diff between 4a68acaf and 0381ba7f); deferred as a pre-existing project convention, disproportionate scope for this pass. Reasonable call.
• [Confirmed non-issue] Nit: ADR-046/ADR-056 — investigated, references are correct as written (046 = original 12B/31B parking, 056 = E4B default lock); no code change needed, correctly left alone.
• [Fixed] Nit: SEQ doc comment reworded, reads clearly now.

Findings

Critical Issues

None. The core threshold/catalog drift is fixed and verified by a passing test.

Suggested Improvements

• packages/agent/src/local_agent/model_manager.rs, recommended_model_id() doc comment and its adjacent inline comment (around what is now lines 406-419 in the delta commit) — both still describe recommended_model_id_for's Gemma4 large tier as "the 12B and 31B tiers" / "that RAM-tiers into 12B/31B." After this delta, recommended_model_id_for(Gemma4)'s large branch returns GEMMA_4_26B_A4B.id, not GEMMA_4_31B.id — 31B is no longer reachable from that function at all. This is now a factually stale comment sitting two functions away from the fix, describing behavior the function no longer has. It's exactly the class of drift this whole review cycle exists to catch: the doc comment directly above recommended_model_id_for itself (and the one on RAM_THRESHOLD_LARGE, flagged below) were updated correctly, but this third, slightly more distant comment on the neighboring function was missed. The substantive guard the comment protects (never let E4B stop being the pinned default) is still correct and still enforced by the code — this is a documentation-only miss, not a behavioral one — but given the meta-point of this review round, it should be fixed too: replace "12B and 31B tiers" with "12B and 26B-A4B tiers" (or similar) in both the doc comment and the inline comment.

• packages/agent/src/local_agent/model_manager.rs, RAM_THRESHOLD_LARGE's own doc comment (directly above const RAM_THRESHOLD_LARGE: u64 = 32 * 1024 * 1024 * 1024;) — still reads "RAM threshold (in bytes) at or above which the large recommended model (Gemma 4 31B) is selected instead of the mid-tier one." This was not updated as part of the 31B→26B-A4B swap, even though the doc comment on recommended_model_id_for itself (a few lines below) was correctly updated to say 26B-A4B. Same fix as above: reword to reference GEMMA_4_26B_A4B instead of GEMMA_4_31B.

Neither of these is a functional bug — GEMMA_4_31B remains correctly listed in CATALOG (still downloadable/selectable directly by id, which is intended; it's parked from recommendation, not deleted), and no other call site in the codebase (including model-store.svelte.ts, which references gemma-4-31b-q4km directly by id and is untouched by this delta) assumes it's still the "large" recommendation tier. But leaving two stale comments in the same function/module that this review round was specifically about undercuts the fix's own stated rationale.

Nitpicks

None beyond the items above (both promoted to Improvement given the review's explicit focus on comment/behavior consistency, rather than filed as Nits).

Verification performed against the actual diff/code (not taken on faith)

• Confirmed via git diff 4a68acaf..0381ba7f --stat that exactly the 4 claimed files changed, no scope creep.
• Confirmed RAM_THRESHOLD_GEMMA4_MEDIUM (24GB) is correctly used in recommended_model_id_for's gemma4_medium branch, not the old shared RAM_THRESHOLD_MEDIUM.
• Confirmed catalog values: MINISTRAL_8B.min_memory_gb == 16, GEMMA_4_12B.min_memory_gb == 24, GEMMA_4_26B_A4B.min_memory_gb == 32, all matching their respective threshold constants exactly.
• Confirmed GEMMA_4_31B.min_memory_gb == 24 (not 32) — verifies the described pre-existing mismatch with the old RAM_THRESHOLD_LARGE (32GB) was real, and is now moot since 31B is no longer gated by that threshold.
• Built and ran the new test directly: cargo test -p nodespace-agent ram_thresholds_agree_with_the_catalog_entries_they_gate — passes. Also ran the full model_manager test module (37 tests) — all pass, including the pre-existing recommended_model_id_for_family_returns_family_match test, confirming no regression.
• Confirmed the u8 -> u64 cast in the new test's byte-conversion (min_memory_gb as u64 * 1024 * 1024 * 1024) casts before multiplying (correct operator precedence), so no truncation/overflow risk given min_memory_gb's max value of 255.
• Confirmed recommended_model_id_for has no external call sites outside model_manager.rs itself (only its own doc comments/tests and recommended_model_id()'s guard comment reference it) — nothing else in the codebase could be silently broken by the 31B→26B-A4B swap.
• Confirmed dump_enabled in nlp-engine/src/chat/mod.rs is computed exactly once before the token-generation loop and used at exactly one site inside it; no stale duplicate prompt_dump::enabled() calls remain in the hot path.
• Confirmed "engine": "native" was added to both JSON record shapes built in nlp-engine/src/chat/prompt_dump.rs (dump_prompt and dump_response), matching the existing "engine": "openai_compat" in the sibling module — full symmetry, not partial.
• Confirmed model-ram-gating.test.ts does not appear in git diff 4a68acaf..0381ba7f's file list — untouched, as claimed.
• No lint suppressions, no new unwrap()/expect(), no unrelated scope creep in the delta.

Recommendation: APPROVE

The Critical finding from the prior review is genuinely fixed and verified by a passing, well-targeted regression test that also caught a second, independent pre-existing bug — a good outcome from writing the test as a general invariant rather than a narrow one-off assertion. All four Improvements and both Nits from the prior review were handled appropriately (fixed, or reasonably deferred/confirmed-non-issue with stated rationale). The two remaining stale-comment spots identified above are real and worth a quick follow-up fix, but they are non-functional (comment-only, not a behavioral bug) and don't rise to blocking severity — they're the kind of thing reasonable to fix in a fast follow-up rather than another review round, especially given this PR has already been through one full REQUEST CHANGES cycle for comment/threshold drift and the substance is sound.

[malibio]

Addressed re-review feedback

Fixed the one gap the re-review found: two stale doc comments left behind by the 31B→26B-A4B swap in the previous commit.

• RAM_THRESHOLD_LARGE's own doc comment still said "Gemma 4 31B" — corrected to describe the current 26B-A4B gating.
• recommended_model_id()'s doc/inline comments still described "12B and 31B tiers" as parked — reworded to precisely state that only 12B remains parked in the ladder (26B-A4B is validated and exposed), while noting the function still bypasses the whole ladder wholesale for its single fixed default rather than tiering partially.

No functional change — confirmed via the full nodespace-agent test suite (467/467 passing) and clippy (clean).

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: pure doc-comment correction, zero logic/behavior change, verified by an identical full test suite pass. The substantive delta (the RAM threshold fix and the 31B→26B-A4B recommendation swap) was already re-reviewed and approved in the prior round — this commit only closes the one loose end that re-review flagged.

Ready for merge, pending your go-ahead.