Tool parsers fixes for finish with incomplete output structure - #4493
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts Gemma4 and LFM2 streaming tool-call parsers to avoid re-emitting tool arguments during the final STOP flush when the model output is structurally incomplete (e.g., missing end tags/closures), aligning with a more forgiving “emit correct output when possible” convention.
Changes:
- Clear the internal
ToolCallbuffer immediately after emitting the arguments delta in theToolCallEndedstate (prevents duplicate argument emission on final flush). - Add regression tests for both Gemma4 and LFM2 covering missing end-tag scenarios where generation stops mid-structure.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/llm/io_processing/lfm2/lfm2_tool_parser.cpp |
Clears buffered tool-call data after emitting arguments to prevent duplicate emissions on STOP flush. |
src/llm/io_processing/gemma4/gemma4_tool_parser.cpp |
Same buffering fix for Gemma4 tool-call streaming. |
src/test/llm/output_parsers/lfm2_output_parser_test.cpp |
Adds regression test for STOP occurring before tool-call end markers; asserts arguments emitted exactly once. |
src/test/llm/output_parsers/gemma4_output_parser_test.cpp |
Adds regression test for missing end tag with stray `< |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
mzegla
force-pushed
the
parsers_fixes
branch
from
September 1, 2026 08:46
3c79b67 to
65a211c
Compare
przepeck
approved these changes
Sep 1, 2026
mzegla
changed the title
Comment on lines
+287
to
+293
| if (this->currentState == State::Content || | ||
| this->currentState == State::InsideFunctionName) { | ||
| // No usable function name was ever captured -- nothing to recover. Still clear the | ||
| // dangling partial state so it doesn't look like a call is still in flight. | ||
| resetParsingState(); | ||
| return std::nullopt; | ||
| } |
Comment on lines
+217
to
+224
| if (this->currentState == State::Content || | ||
| this->currentState == State::InsideToolCall || | ||
| this->currentState == State::InsideFunctionName) { | ||
| // No usable function name was ever captured -- nothing to recover. Still clear the | ||
| // dangling partial state so it doesn't look like a call is still in flight. | ||
| resetParsingState(); | ||
| return std::nullopt; | ||
| } |
dtrawins
approved these changes
Sep 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Moving towards convention "if we can emit correct output, do it, even if it's not completely in line with expected structure".
This approach is more forgiving if model skips some closures. It also helps if model reaches
max_tokenslimit mid generation and is in general more live-streaming kind of approach.