[malibio]

Closes #1365

[malibio]

Code Review — PR #1367: Gemma 4 12B Runaway Fix

Review Type: Initial Review

---

Requirements Check

From issue #1365 acceptance criteria:

• ✅ AC1 — Root-cause the runaway generation: fixed via is_eog_token() replacing bare EOS comparison, plus augment_gemma4_stops() injecting <end_of_turn> and <turn|> as string-level stops.
• ✅ AC2 — Root-cause and fix OAI parser update error: ffi error -3: addressed via consecutive-error counter with a break at 5 failures, preventing continued generation of garbage tokens and daemon death.
• ⚠️ AC3 — Confirm 12B is offered the full tool set + skill-mediated prompt: the commit message explicitly notes this was fixed by #1366, already merged to main. This PR does not address AC3 directly but the issue is resolved in the base.
• ❌ AC4 — Re-run scenario matrix against 12B end-to-end: no evidence in the PR of a post-fix matrix run. The issue called for an end-to-end validation with no runaway, no parser errors, no daemon death. This should be documented.
• ❌ AC5 — Document verdict: keep E4B vs. ship 12B on trustworthy in-NodeSpace measurement: not present in the PR description or any linked comment.

---

Code Review Findings

🟡 Important — Magic number 3 for CHAT_FORMAT_PEG_GEMMA4 repeated across callers

File: packages/nlp-engine/src/chat/mod.rs, lines 362 and 747

The integer 3 (representing COMMON_CHAT_FORMAT_PEG_GEMMA4) appears three times in this diff with no named constant. The value is correctly documented in comments, but a future llama.cpp upgrade that renumbers format IDs would silently break both the stop augmentation path and the debug branch without a compile-time signal. The function signature accepts i32 rather than a typed enum.

Recommended: introduce a module-level constant const CHAT_FORMAT_PEG_GEMMA4: i32 = 3; alongside the existing chat_format=0 reference pattern. This is a low-cost guard against upstream format ID drift.

---

🟡 Important — OAI parser error recovery is an unconditional circuit breaker, not a parser reset

File: packages/nlp-engine/src/chat/mod.rs, lines 546–559

The 5-consecutive-error threshold is a pragmatic stop-loss, and the rationale (unrecoverable parser state) is sound. However, when generation breaks out of the loop, the finalize path at line 585 (oai_parser.update("", false)) still runs unconditionally. If the parser is truly in an unrecoverable state after 5 consecutive FFI errors, this finalize call may also error or produce malformed output.

The current code silently discards a finalize error via tracing::warn! (line 596) — acceptable for now — but worth a targeted comment noting that the finalize is best-effort after an unrecoverable parser break. This prevents the next reader from being confused by the warn-and-continue at line 596 when they see the error path above it.

---

🟡 Important — augment_gemma4_stops uses string-level matching as a secondary EOG layer

File: packages/nlp-engine/src/chat/mod.rs, lines 527–529

The additional_stops check (line 527) does piece.contains(s.as_str()), which fires if the stop token string appears anywhere in a decoded piece — including as a substring of a user-authored text field. This is the pre-existing behaviour in the codebase, not introduced by this PR, but injecting <turn|> and <end_of_turn> as string stops increases the surface for false-positive stops if a user message or tool result contains those substrings (unlikely but not impossible with adversarial content).

This is a pre-existing design risk and not a blocker for this PR. Worth a // SAFETY comment on line 527 acknowledging the substring match is intentional and why it is acceptable here.

---

🟢 Suggestion — Token cap reduction comment could note per-model context

File: packages/agent/src/local_agent/inference.rs, line 85

The comment is thorough. One addition worth considering: note that 2048 is also the cap chosen because it matches MAX_RESPONSE_TOKENS (defined in agent_loop.rs:47), so the worst-case total per round-trip is bounded symmetrically. This makes it easier for future engineers to reason about the relationship between the two constants without needing to cross-reference both files.

---

🟢 Suggestion — Tests do not assert ordering of injected stops

File: packages/nlp-engine/src/chat/mod.rs, lines 1379–1415

The three tests cover the core behaviour well (inject-missing, no-duplicate, non-Gemma4). One gap: the deduplication test verifies <end_of_turn> is not duplicated when already present, but does not assert that the existing entry is preserved at its original index. If a future refactor accidentally cleared and re-inserted the vector, the test would still pass. A minor defensive assertion like assert_eq!(stops[0], "<end_of_turn>") after augmentation would close this gap.

---

🟢 Nit — Debug log for Gemma 4 stop augmentation is gated on chat_format == 3 at the call site

File: packages/nlp-engine/src/chat/mod.rs, lines 362–367

The inline if tmpl_result.chat_format == 3 debug block is separate from the augment_gemma4_stops call and duplicates the format-check condition. Moving the debug log inside augment_gemma4_stops would eliminate the caller-side duplication and keep the Gemma 4 specific logic in one place. This is purely structural cleanup — no behaviour change.

---

Summary Assessment

This is a well-targeted, minimally-scoped fix. The three root causes from the issue are addressed with proportional solutions: the EOG fix is architecturally correct (delegating to the library's is_eog_token() rather than reimplementing), the stop augmentation is clearly motivated by the specific GGUF metadata deficiency, and the error recovery prevents daemon death. The comment quality is high throughout.

The main gap against the acceptance criteria is the missing evidence of a post-fix end-to-end matrix run (AC4) and the model verdict documentation (AC5). These are operational validation steps, not code defects — but the issue explicitly called for them as merge criteria.

Recommendation: REQUEST CHANGES — on AC4 and AC5 only. The code itself is approvable; block on documented post-fix validation and verdict per the issue's acceptance criteria.

[malibio]

Address-review summary

Addressed (5/5 code findings):

• ✅ CHAT_FORMAT_PEG_GEMMA4 constant — extracted magic 3 into a named const at module scope; all usages (function body + tests) updated
• ✅ Debug log consolidation — moved Gemma 4 additional_stops debug log inside augment_gemma4_stops, removed duplicate chat_format == 3 guard at call site
• ✅ Finalize best-effort comment — clarifies that oai_parser.update("", false) runs after the circuit-breaker exit intentionally (warn-and-continue to still drain the splitter and emit Done)
• ✅ Deduplication test position assertion — added assert_eq!(stops[0], "<end_of_turn>") to catch any future refactor that clears and re-inserts the vector
• ✅ inference.rs comment — notes that 2048 mirrors MAX_RESPONSE_TOKENS (agent_loop.rs) for symmetric per-turn bounding

Skipped:

• ⏭️ piece.contains() intent comment — pre-existing semantics unchanged by this PR; not worth annotating in scope
• ⏭️ AC4/AC5 (scenario matrix + verdict) — these are runtime validation work, not code changes; will be addressed in a follow-up issue comment once 12B is testable end-to-end with the current build