Fix Gemma 4 12B tool-call routing: chat template mismatch causes Python-prose output instead of structured tool calls

[malibio]

Closes #1346

[malibio]

Review address summary

✅ Addressed (4/5 recommendations)

🔴 Finding 1 — 12B never recommended: Added RAM_THRESHOLD_MEDIUM = 16 GB constant and implemented three-tier Gemma 4 selection: E4B (< 16 GB) → 12B (16–32 GB) → 31B (≥ 32 GB). Updated recommended_model_id_for docstring and two tests that were asserting only E4B or 31B as valid outcomes.

🟡 Finding 2 — Silent chat_format=0 degradation: Added tracing::warn! when chat_format == 0 (CONTENT_ONLY) with tools present, so a template detection failure is visible in production logs without requiring a debug build.

🟢 Finding 3 — Stale docstring: Updated recommended_model_id_for doc to describe the three-tier logic (done as part of Finding 1 fix).

🟢 Finding 4 — Log level: Downgraded the chat-format diagnostic log from info! to debug!. It fires on every ReAct iteration with the same values; info! was production log noise.

⏭️ Skipped (1/5)

🟢 Finding 5 — min_memory_gb:16 tightness for 12B: The 12B footprint is ~10 GB (7.4 GB weights + 2.5 GB Q8_0 KV at 32K), leaving ~6 GB on a 16 GB device. This is comparable to E4B (~10 GB F16 KV) and Ministral 8B (both also min_memory_gb: 16). The 16 GB floor is consistent with the existing catalog policy; raising it would make 12B inaccessible on the 16–32 GB tier that is its primary target.

[malibio]

Re-Review: PR #1347 — Gemma 4 12B Tool-Call Routing

Review Type: Re-Review (address-review commit: e27c244)

---

Previous Review Summary

• Previous recommendations: 5
• Addressed: 4/5 (Finding 5 deliberately skipped with documented rationale)
• Not addressed: 0
• New issues: 1 (minor — unused variable in Ministral branch, see below)

---

Requirements Check (Issue #1346)

The issue acceptance criteria are runtime-validation items (observe structured tool calls in the eval matrix, non-empty tool_calls_parsed in MLflow traces). This PR correctly scopes itself to the prerequisite infrastructure: catalog entry, diagnostic logging, and recommendation routing. The runtime verification must follow after loading the model.

• ❌ Gemma 4 12B correctly emits [TOOL_CALLS] on tool-requiring turns — not verifiable pre-merge (requires model download and eval matrix run; this PR adds the prerequisite infrastructure)
• ❌ aichat-matrix.ts scenarios 3–8 show real tool calls — runtime validation, post-merge
• ✅ E4B behavior is unchanged — three-tier logic only activates at ≥16 GB; E4B fallback path preserved
• ❌ MLflow traces show non-empty tool_calls_parsed — runtime validation, post-merge

The remaining ❌ items are runtime validation criteria that require the model to be loaded — they cannot be confirmed in a static code review. The PR delivers the infrastructure required for that validation.

---

Code Review Findings (new commit e27c244 only)

No Critical Issues

All four addressed findings are correctly resolved.

Suggested Improvements

Finding 1 — Three-tier threshold: CORRECTLY ADDRESSED

RAM_THRESHOLD_MEDIUM = 16 GB is added and the recommended_model_id_for function now implements the correct three-tier chain: large (≥32 GB) → 31B, medium (≥16 GB) → 12B, else → E4B. Applied to both Gemma4 and Ollama arms.

Finding 2 — chat_format=0 warning: CORRECTLY ADDRESSED

The guard condition tmpl_result.chat_format == 0 && tools.as_ref().is_some_and(|t| !t.is_empty()) is correct. Cross-checking against apply_chat_template: when tools is Some([]), the function passes tools_json: None downstream, but the warn check uses the caller's tools value. An empty-vec caller would pass is_some_and(|t| !t.is_empty()) → false, so no false positive fires. A None caller also skips the warn. No false positives.

The tools.as_ref().map_or(0, |t| t.len()) in the warn message body is logically redundant — by the time execution reaches that line, the guard has already confirmed tools is Some(non-empty), so map_or(0, ...) can never return 0. This is cosmetically inelegant but harmless; the count will always be accurate.

Finding 3 — Stale docstring: CORRECTLY ADDRESSED

recommended_model_id_for docstring updated to describe the three-tier logic with RAM_THRESHOLD_MEDIUM and RAM_THRESHOLD_LARGE references. Both threshold constants also have accurate doc comments.

Finding 4 — Log level: CORRECTLY ADDRESSED

tracing::info! → tracing::debug! for the chat-format diagnostic. Confirmed in the diff and the current file state.

Nitpicks

Nit: model_manager.rs L282 — medium unused in Ministral branch

let medium = total_ram >= RAM_THRESHOLD_MEDIUM; is computed unconditionally before the match, but the Ministral arm never references it. Rust will issue an unused_variable warning unless the compiler determines medium is used in at least one match arm (which it does here via the Gemma4/Ollama arms — so in practice the warning will not fire). This is a code clarity point: hoisting medium above the match implies it applies to all families, when Ministral only has a two-tier policy. Computing it inline within the Gemma4/Ollama arms, or adding a comment on medium scoping, would make the intent clearer. This is non-blocking.

Nit: chat/mod.rs L323 — redundant map_or(0, ...) in warn message

As noted above, tools.as_ref().map_or(0, |t| t.len()) inside the warn body can never produce 0 given the guard condition. Replace with tools.as_ref().unwrap().len() or restructure to avoid the second .as_ref() call entirely. Non-blocking.

---

Assessment

The address-review commit correctly resolves all four actionable findings. The three-tier recommendation logic is sound, boundary conditions are correct (exclusive large → inclusive medium cascade), the warning fires on the right condition, log level is fixed, and tests cover all three valid Gemma 4 recommendation outcomes. No regressions introduced.

The two nits are minor polish items. Neither affects correctness or production behavior.

Recommendation: APPROVE

The remaining acceptance criteria (runtime tool-call validation, eval matrix) require the model to be loaded and are appropriately post-merge work. The infrastructure to enable that validation is correct.

[malibio]

Review address (nit pass)

Addressed (1/2 nits)

Nit 2 — dead fallback in warn body: Refactored the guard to use if let Some(active_tools) = tools.as_ref().filter(|t| !t.is_empty()) so the tool count is bound directly from the slice, eliminating the unreachable default.

Skipped (1/2 nits)

Nit 1 — let medium computed before match: The Ministral arm does not use medium, but moving the computation inside each arm would duplicate the detect_system_ram() call pattern. Current placement mirrors the existing let large = ... immediately above it — consistent style, not a bug.