[malibio]

Closes #1355

[malibio]

Code Review — PR #1361 (Issue #1355)

Reviewer: Principal Engineer
Verdict: Approve with one important test-design note

This PR correctly identifies that no crate version bump was needed and delivers the right artifacts: a documented Cargo.toml comment and five regression fixture tests. The overall change is a net positive. The findings below are ranked by priority.

---

Findings

[Improvement] regression_gemma4_12b_tool_call_delta_accepted does not test the codebase under review

This is the most substantive finding. Four of the five tests call parse_tool_calls or StreamingToolCallParser::finish — they exercise the actual parser implementation. The Gemma 4 delta test (regression_gemma4_12b_tool_call_delta_accepted) does not. It manually deserializes the delta JSON with serde_json::from_str and asserts on the resulting Value fields. This is testing serde_json, not emit_oai_delta or any other function in this codebase.

The stated regression scenario is real and important: chat_format=0 causes tool calls to arrive as plain content text, so emit_oai_delta never receives a tool_calls array. But what this test actually validates is: "a hardcoded JSON string, when deserialized, has the fields I put into it." That is trivially true by construction and would pass even if emit_oai_delta were deleted.

To properly pin this regression, the test should call emit_oai_delta directly (it is currently fn emit_oai_delta, not pub fn) or it should be promoted to a test in chat/mod.rs where emit_oai_delta is in scope. The minimal fix: make emit_oai_delta pub(crate) and call it in the test, collecting the ChatChunk events and asserting that ToolCallStart and ToolCallArgs events are produced.

File: packages/nlp-engine/src/chat/parser.rs, lines 1262–1277

---

[Improvement] Cargo.toml comment conflates two separate claims

The comment reads:

v0.1.146 ships apply_chat_template_oaicompat with Jinja support required for Gemma 4, and vendors llama.cpp commit e21cdc11 (2026-04-13) — 123 commits past build b8660 (2026-04-03), fixing tool-call failures in Ministral 8B and Gemma 4 12B (issue #1355)

The second claim has two parts that have different evidential status:

1. "vendors llama.cpp commit e21cdc11 (2026-04-13) — 123 commits past build b8660" — this is a factual assertion about what the crate vendors. It should be verifiable via the llama-cpp-sys-2 crate's build.rs or source tarball, but is presented without a source of truth.
2. "fixing tool-call failures" — this is a causal claim backed by the empirical testing documented in the issue. The claim is valid but slightly overstated: the commit count is relative to a tested reference point, not absolute proof that e21cdc11 specifically fixes the failures.

This is not a blocker, but the comment would be more precise as: "and vendors llama.cpp at a commit ≥ build b8660 (2026-04-03), the reference point at which tool-call failures in Ministral 8B and Gemma 4 12B were empirically resolved (issue #1355)."

File: packages/nlp-engine/Cargo.toml, lines 14–16

---

[Improvement] Missing acceptance criteria: tool-calling smoke test and embedding regression

Issue #1355 has two criteria without explicit coverage in this PR:

• "Tool-calling smoke test green for: Gemma 4 E4B/12B, Ministral 3B/8B, Qwen3-8B, Mistral Small 3.2" — no automated tests for this; it requires a live GGUF model. The commit message does not document that this was manually verified.
• "No regression in embedding generation (nomic-embed path)" — similarly requires a live model.

These are inherently manual / environment-dependent criteria, so their absence from the automated test suite is understandable. However the PR description or commit message should explicitly state that these were (or were not yet) manually verified against the real models, so the reviewer and future readers know the current state of those criteria. Right now the commit message is silent on both points.

---

Nit: regression_ministral8b_streaming_broken_json_errors_cleanly duplicates batch behavior

The streaming test feeds the entire input as one feed call, which collapses to the same path as parse_tool_calls (the finish method delegates directly). A more meaningful streaming fixture would feed the broken JSON across multiple feed calls to exercise incremental accumulation. This is a minor quality concern — the test does establish that the streaming path does not panic, which is the stated goal.

File: packages/nlp-engine/src/chat/parser.rs, lines 1240–1250

---

What Is Done Well

• The Cargo.toml comment is clearly structured and preserves the full history of the versioning rationale, which is the right call for a dependency that has a non-obvious constraint chain.
• Tests 1, 2, 3, and 5 (regression_ministral8b_*, regression_gemma4_mistral_sentinel_format_still_works) are well-written, exercise the real parser, include accurate [TOOL_CALLS] format payloads that match the documented Format A and Format C variants, and have clear doc-comments explaining the historical failure.
• The decision not to bump the crate version is documented, which prevents future developers from attempting a redundant bump.
• Format C (Gemma 4 object after sentinel) was already covered by existing tests — the new regression fixture adds a create_schema-specific case with a fields array, which gives additional specificity without being redundant.
• The Cargo.lock confirms llama-cpp-2 = "0.1.146" is actually resolved, so the constraint is live.

---

Acceptance Criteria Coverage

Criterion	Status
Cargo.toml comment updated	Covered
Regression fixtures pinned (Ministral 8B + Gemma 4 12B)	Partially — 4/5 tests exercise the parser; Gemma4 delta test is a structural gap
[TOOL_CALLS] Mistral parser verified post-upgrade	Covered (regression_gemma4_mistral_sentinel_format_still_works)
Tool-calling smoke test across all shipped models	Not documented as verified
No embedding regression	Not documented as verified
bun run quality:fix / tests pass	Assumed green; no CI failure visible

[malibio]

Four of five regression fixtures exercise the actual parser correctly and the Cargo.toml documentation is clear. The Gemma 4 delta test is a structural gap (it tests serde_json, not emit_oai_delta) and should be addressed as a follow-up. The two model-level acceptance criteria (live smoke tests, embedding regression) should be documented as manually verified or tracked as a follow-up. Full review in the comment above.