Skip to content

fix: preserve generated chat parser during response parsing - #22

Merged
daavoo merged 5 commits into
mozilla-ai:mainfrom
oglego:fix/tool-call-parser
Sep 7, 2026
Merged

fix: preserve generated chat parser during response parsing#22
daavoo merged 5 commits into
mozilla-ai:mainfrom
oglego:fix/tool-call-parser

Conversation

@oglego

@oglego oglego commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Summary

While going through the examples/context-engineering/README.md example, I noticed tool calling wasn't working as documented:

<tool_call>
{"name": "calculator", "arguments": {"a": 3, "b": 4, "operation": "add"}}
</tool_call>
>

I dug into this and found that there was a complete refactoring of the parser architecture in llama.cpp (PR #18675), which impacts tool calling here - see deps/llama.cpp/docs/autoparser.md for background.

The PEG parser needs to be loaded via params.parser; deps/llama.cpp/tests/test-chat.cpp has examples of this. generation_prompt also needs to be passed through - without it, I think llama.cpp parses the raw response alone instead of generation_prompt + response, which is what the derived grammar actually expects.

Change

The only update needed to resolve this:

syntax.parser.load(params.parser);
syntax.generation_prompt = params.generation_prompt;

Testing

Verified end-to-end with a multi-turn conversation against granite-4.0-micro-Q8_0.gguf - tool calls parse and execute correctly:

> Calculate 3 + 4 
<tool_call>
{"name": "calculator", "arguments": {"a": 3, "b": 4, "operation": "add"}}
</tool_call>
[TOOL EXECUTION] Calling calculator
[TOOL RESULT]
{"result":7.0}
[CONTEXT] Trimmed messages:
[CONTEXT] - [tool]: {"result":49.0}
[CONTEXT] - [assistant]: tool_calls: [calculator({"a": 7, "b": 7, "operation": "multi...
The result of 3 + 4 is 7.
> Now multiple that by 7
<tool_call>
{"name": "calculator", "arguments": {"a": 7.0, "b": 7, "operation": "multiply"}}
</tool_call>
[TOOL EXECUTION] Calling calculator
[TOOL RESULT]
{"result":49.0}
[CONTEXT] Trimmed messages:
[CONTEXT] - [tool]: {"result":7.0}
[CONTEXT] - [assistant]: tool_calls: [calculator({"a": 3, "b": 4, "operation": "add"}...
49
>

Let me know if I am missing something on this, and thanks in advance for the review!

oglego and others added 5 commits September 3, 2026 19:18
Extract the response-parsing step of Model::generate() into a
parse_response() helper so it can be exercised without loading a GGUF,
and cover it with a regression test that renders a real chat template,
feeds a canned <tool_call> response and asserts the tool call is parsed.
The test fails without the parser/generation_prompt fix.

Also, while here:

- skip loading an empty parser definition; common_peg_arena::load()
  throws on an empty string, which is what the legacy (non-jinja)
  template path produces
- cache the deserialized parser arena between turns instead of
  re-parsing the definition on every generation
- wrap parse failures in ModelError; now that a real parser is loaded,
  llama.cpp throws when output does not match the template's format,
  and that previously escaped the agent loop as a bare
  std::runtime_error

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI configures CMAKE_BUILD_TYPE=Release, which defines NDEBUG and turned
every assert()-based assertion in the test suite into a no-op. Throw
instead, so the tests actually guard something on CI.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A hung ChatParserTests held a Windows runner for the full 6h default job
limit, and nothing cancelled superseded runs, so three of them stacked up.

- timeout-minutes on every job, and ctest --timeout 300 plus a per-test
  TIMEOUT of 120s, so a hang fails fast and names the test
- concurrency group with cancel-in-progress
- ctest --output-on-failure so the failure is diagnosable from the log
- put bin/<config> on PATH for Windows tests; multi-config generators put
  DLLs there rather than in bin/
- trace the phases of the parser test on stderr, to identify where it
  stalls on Windows

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@daavoo

daavoo commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

thanks for the catch @oglego 🙏

@daavoo
daavoo merged commit e4536d5 into mozilla-ai:main Sep 7, 2026
5 checks passed
@oglego

oglego commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Happy to help! Thanks for all of the enhancements to it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants