Skip to content

Commit e4536d5

Browse files
oglegodaavooclaude
authored
fix: preserve generated chat parser during response parsing (#22)
* fix: preserve generated chat parser during response parsing * test: add regression test for chat parser, harden parse failures 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> * style: apply clang-format Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: make assertions work in Release builds 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> * ci: bound job and test runtime, trace parser test phases 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> --------- Co-authored-by: daavoo <daviddelaiglesiacastro@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 499604c commit e4536d5

8 files changed

Lines changed: 299 additions & 15 deletions

File tree

‎.github/workflows/cmake-multi-platform.yml‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,15 @@ on:
1414
required: false
1515
default: ''
1616

17+
concurrency:
18+
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
19+
cancel-in-progress: true
20+
1721
jobs:
1822
build:
1923
runs-on: ${{ matrix.os }}
24+
# Without this a hung test holds a runner for the 6h default
25+
timeout-minutes: 45
2026

2127
strategy:
2228
# Set fail-fast to false to ensure that feedback is delivered for all matrix combinations. Consider changing this to true when your workflow is stable.
@@ -97,7 +103,7 @@ jobs:
97103
working-directory: ${{ steps.strings.outputs.build-output-dir }}
98104
# Execute tests defined by the CMake configuration. Note that --build-config is needed because the default Windows generator is a multi-config generator (Visual Studio generator).
99105
# See https://cmake.org/cmake/help/latest/manual/ctest.1.html for more detail
100-
run: ctest --build-config ${{ matrix.build_type }}
106+
run: ctest --build-config ${{ matrix.build_type }} --timeout 300 --output-on-failure
101107

102108
- name: Verify examples run
103109
# Examples use POSIX headers (unistd.h) and are not available on Windows

‎.github/workflows/lint.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ on:
99
jobs:
1010
pre-commit:
1111
runs-on: ubuntu-latest
12+
timeout-minutes: 15
1213

1314
steps:
1415
- uses: actions/checkout@v4

‎.github/workflows/update-llama-cpp.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ on:
88
jobs:
99
update-submodule:
1010
runs-on: ubuntu-latest
11+
timeout-minutes: 30
1112

1213
permissions:
1314
contents: write

‎CMakeLists.txt‎

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -166,9 +166,26 @@ if(AGENT_CPP_BUILD_TESTS)
166166
target_link_libraries(test_callbacks PRIVATE agent model ${LLAMA_COMMON_TARGET} llama)
167167
target_compile_features(test_callbacks PRIVATE cxx_std_17)
168168

169+
add_executable(test_chat_parser tests/test_chat_parser.cpp)
170+
target_include_directories(test_chat_parser PRIVATE
171+
src
172+
tests
173+
${LLAMA_SOURCE_DIR}/common
174+
${LLAMA_SOURCE_DIR}/ggml/include
175+
${LLAMA_SOURCE_DIR}/include
176+
${LLAMA_SOURCE_DIR}/vendor
177+
)
178+
# The test parses responses against real chat templates shipped by llama.cpp
179+
target_compile_definitions(test_chat_parser PRIVATE
180+
LLAMA_TEMPLATES_DIR="${LLAMA_SOURCE_DIR}/models/templates"
181+
)
182+
target_link_libraries(test_chat_parser PRIVATE model ${LLAMA_COMMON_TARGET} llama)
183+
target_compile_features(test_chat_parser PRIVATE cxx_std_17)
184+
169185
add_test(NAME ToolTests COMMAND test_tool)
170186
add_test(NAME CallbacksTests COMMAND test_callbacks)
171187
add_test(NAME GrammarTests COMMAND test_grammar)
188+
add_test(NAME ChatParserTests COMMAND test_chat_parser)
172189

173190
if(AGENT_CPP_BUILD_MCP)
174191
add_executable(test_mcp_client tests/test_mcp_client.cpp)
@@ -185,9 +202,16 @@ if(AGENT_CPP_BUILD_TESTS)
185202

186203
# On Windows, DLLs are placed in the bin/ directory by llama.cpp
187204
# We need to add this directory to PATH so tests can find the DLLs
205+
# A hung test must fail rather than hold the runner until the job limit
206+
set_tests_properties(ToolTests CallbacksTests GrammarTests ChatParserTests
207+
PROPERTIES TIMEOUT 120
208+
)
209+
188210
if(WIN32)
189-
set_tests_properties(ToolTests CallbacksTests GrammarTests PROPERTIES
190-
ENVIRONMENT "PATH=${CMAKE_BINARY_DIR}/bin\;$ENV{PATH}"
211+
# Multi-config generators (Visual Studio) place DLLs in bin/<config>,
212+
# single-config ones in bin/ - both are on PATH so tests can load them
213+
set_tests_properties(ToolTests CallbacksTests GrammarTests ChatParserTests PROPERTIES
214+
ENVIRONMENT "PATH=${CMAKE_BINARY_DIR}/bin/${CMAKE_BUILD_TYPE}\;${CMAKE_BINARY_DIR}/bin/Release\;${CMAKE_BINARY_DIR}/bin\;$ENV{PATH}"
191215
)
192216
endif()
193217

‎src/model.cpp‎

Lines changed: 64 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,54 @@ Model::tokenize(const std::string& prompt) const
203203
return prompt_tokens;
204204
}
205205

206+
namespace {
207+
208+
/// Deserializes the PEG parser derived from the chat template. An empty
209+
/// definition yields an empty arena, which common_chat_peg_parse() treats as a
210+
/// pure-content parser.
211+
common_peg_arena
212+
load_parser(const std::string& parser_definition)
213+
{
214+
common_peg_arena arena;
215+
if (!parser_definition.empty()) {
216+
arena.load(parser_definition);
217+
}
218+
return arena;
219+
}
220+
221+
common_chat_msg
222+
parse_with_parser(const common_peg_arena& parser,
223+
const common_chat_params& params,
224+
const std::string& response,
225+
std::optional<common_chat_format> format_override)
226+
{
227+
common_chat_parser_params syntax;
228+
// Use explicitly configured format, or fall back to auto-detected format.
229+
// Note that the format only selects the mapper; the PEG parser below is
230+
// what actually drives parsing.
231+
syntax.format = format_override.value_or(params.format);
232+
// The parser is derived from the chat template together with the
233+
// generation prompt it was built against, and parsing needs both
234+
// (ggml-org/llama.cpp#18675). Without them tool calls stay as raw text.
235+
syntax.generation_prompt = params.generation_prompt;
236+
syntax.parse_tool_calls = true;
237+
238+
try {
239+
auto parsed_msg =
240+
common_chat_peg_parse(parser, response, false, syntax);
241+
parsed_msg.role = "assistant";
242+
return parsed_msg;
243+
} catch (const std::exception& e) {
244+
// llama.cpp throws plain std::runtime_error when the output does not
245+
// match the template's format; surface it as a library error so
246+
// callers can catch it alongside the rest of agent.cpp.
247+
throw ModelError(std::string("failed to parse model response: ") +
248+
e.what());
249+
}
250+
}
251+
252+
}
253+
206254
common_chat_msg
207255
Model::generate(const std::vector<common_chat_msg>& messages,
208256
const std::vector<common_chat_tool>& tools,
@@ -226,15 +274,24 @@ Model::generate(const std::vector<common_chat_msg>& messages,
226274

227275
std::string response = generate_from_tokens(prompt_tokens, callback);
228276

229-
common_chat_parser_params syntax;
230-
// Use explicitly configured format, or fall back to auto-detected format
231-
syntax.format = config_.chat_format.value_or(params.format);
232-
syntax.parse_tool_calls = true;
277+
// The PEG parser only changes when the rendered template changes, so keep
278+
// the deserialized arena around instead of re-parsing it every turn.
279+
if (parser_source_ != params.parser) {
280+
parser_arena_ = load_parser(params.parser);
281+
parser_source_ = params.parser;
282+
}
233283

234-
auto parsed_msg = common_chat_parse(response, false, syntax);
235-
parsed_msg.role = "assistant";
284+
return parse_with_parser(
285+
parser_arena_, params, response, config_.chat_format);
286+
}
236287

237-
return parsed_msg;
288+
common_chat_msg
289+
parse_response(const common_chat_params& params,
290+
const std::string& response,
291+
std::optional<common_chat_format> format_override)
292+
{
293+
return parse_with_parser(
294+
load_parser(params.parser), params, response, format_override);
238295
}
239296

240297
std::string

‎src/model.h‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,21 @@ struct ModelConfig
4343
std::string
4444
load_grammar_file(const std::string& grammar_path);
4545

46+
/// Parses a raw model response into a chat message, using the PEG parser and
47+
/// generation prompt derived from the model's chat template.
48+
/// @param params result of common_chat_templates_apply() for the turn that
49+
/// produced @p response
50+
/// @param response the raw text generated by the model
51+
/// @param format_override optional explicit chat format, overriding the
52+
/// auto-detected params.format
53+
/// @throws agent_cpp::ModelError if the response does not match the expected
54+
/// format
55+
common_chat_msg
56+
parse_response(
57+
const common_chat_params& params,
58+
const std::string& response,
59+
std::optional<common_chat_format> format_override = std::nullopt);
60+
4661
// Forward declaration
4762
class Model;
4863

@@ -198,6 +213,10 @@ class Model
198213
std::vector<llama_token> processed_tokens_; // Track tokens in KV cache
199214
int n_past_ = 0; // Track position in KV cache
200215
ModelConfig config_;
216+
// Chat parser derived from the template, cached across turns along with
217+
// the definition it was built from
218+
std::string parser_source_;
219+
common_peg_arena parser_arena_;
201220
};
202221

203222
} // namespace agent_cpp

‎tests/test_chat_parser.cpp‎

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,142 @@
1+
#include "model.h"
2+
#include "test_utils.h"
3+
#include <fstream>
4+
#include <iostream>
5+
#include <sstream>
6+
#include <stdexcept>
7+
#include <string>
8+
9+
#ifndef LLAMA_TEMPLATES_DIR
10+
#error "LLAMA_TEMPLATES_DIR must be defined"
11+
#endif
12+
13+
namespace {
14+
15+
// Progress markers on stderr, unbuffered: if a test hangs (as this one did on
16+
// Windows) the ctest timeout output shows exactly which phase it stalled in.
17+
void
18+
trace(const std::string& phase)
19+
{
20+
std::cerr << "[trace] " << phase << std::endl;
21+
}
22+
23+
std::string
24+
read_file(const std::string& path)
25+
{
26+
std::ifstream file(path);
27+
if (!file) {
28+
throw std::runtime_error("failed to open " + path);
29+
}
30+
std::ostringstream buffer;
31+
buffer << file.rdbuf();
32+
return buffer.str();
33+
}
34+
35+
// Mirrors how Model::generate() renders a turn, so the parser under test is
36+
// fed the same common_chat_params the real code path produces.
37+
common_chat_params
38+
apply_template(const std::string& template_name, bool with_tools)
39+
{
40+
const std::string template_path =
41+
std::string(LLAMA_TEMPLATES_DIR) + "/" + template_name;
42+
43+
trace("reading " + template_path);
44+
const std::string template_source = read_file(template_path);
45+
46+
trace("initializing templates");
47+
auto tmpls = common_chat_templates_init(nullptr, template_source);
48+
49+
common_chat_msg user_msg;
50+
user_msg.role = "user";
51+
user_msg.content = "Calculate 3 + 4";
52+
53+
common_chat_templates_inputs inputs;
54+
inputs.messages = { user_msg };
55+
if (with_tools) {
56+
common_chat_tool calculator;
57+
calculator.name = "calculator";
58+
calculator.description = "Performs arithmetic";
59+
calculator.parameters =
60+
R"({"type":"object","properties":{"a":{"type":"number"},)"
61+
R"("b":{"type":"number"},"operation":{"type":"string"}},)"
62+
R"("required":["a","b","operation"]})";
63+
inputs.tools = { calculator };
64+
}
65+
inputs.tool_choice = COMMON_CHAT_TOOL_CHOICE_AUTO;
66+
inputs.add_generation_prompt = true;
67+
inputs.enable_thinking = false;
68+
69+
trace("applying template");
70+
auto params = common_chat_templates_apply(tmpls.get(), inputs);
71+
trace("template applied");
72+
73+
return params;
74+
}
75+
76+
}
77+
78+
// Regression test for tool calls being returned as raw text instead of being
79+
// parsed. llama.cpp ggml-org/llama.cpp#18675 moved parsing to a PEG parser
80+
// derived from the chat template; parse_response() must forward both the
81+
// derived parser and the generation prompt it was built against, otherwise
82+
// common_chat_parse() falls back to a pure-content parser.
83+
TEST(test_tool_call_is_parsed_from_response)
84+
{
85+
auto params = apply_template("ibm-granite-granite-4.0.jinja", true);
86+
ASSERT_TRUE(!params.parser.empty());
87+
88+
const std::string response =
89+
"<tool_call>\n"
90+
R"({"name": "calculator", "arguments": {"a": 3, "b": 4, "operation": "add"}})"
91+
"\n</tool_call>";
92+
93+
auto parsed = agent_cpp::parse_response(params, response);
94+
95+
ASSERT_EQ(parsed.role, std::string("assistant"));
96+
ASSERT_EQ(parsed.tool_calls.size(), static_cast<size_t>(1));
97+
ASSERT_EQ(parsed.tool_calls[0].name, std::string("calculator"));
98+
ASSERT_TRUE(parsed.tool_calls[0].arguments.find("\"operation\"") !=
99+
std::string::npos);
100+
ASSERT_TRUE(parsed.content.empty());
101+
}
102+
103+
// A plain answer must still come back as content, with no spurious tool calls.
104+
TEST(test_plain_response_is_parsed_as_content)
105+
{
106+
auto params = apply_template("ibm-granite-granite-4.0.jinja", true);
107+
108+
auto parsed =
109+
agent_cpp::parse_response(params, "The result of 3 + 4 is 7.");
110+
111+
ASSERT_TRUE(parsed.tool_calls.empty());
112+
ASSERT_EQ(parsed.content, std::string("The result of 3 + 4 is 7."));
113+
}
114+
115+
// Templates rendered without tools still parse ordinary content.
116+
TEST(test_response_without_tools_is_parsed_as_content)
117+
{
118+
auto params = apply_template("ibm-granite-granite-4.0.jinja", false);
119+
120+
auto parsed = agent_cpp::parse_response(params, "Hello!");
121+
122+
ASSERT_TRUE(parsed.tool_calls.empty());
123+
ASSERT_EQ(parsed.content, std::string("Hello!"));
124+
}
125+
126+
int
127+
main()
128+
{
129+
std::cout << "\n=== Running Chat Parser Unit Tests ===\n" << std::endl;
130+
131+
try {
132+
RUN_TEST(test_tool_call_is_parsed_from_response);
133+
RUN_TEST(test_plain_response_is_parsed_as_content);
134+
RUN_TEST(test_response_without_tools_is_parsed_as_content);
135+
136+
std::cout << "\n=== All tests passed! ✓ ===\n" << std::endl;
137+
return 0;
138+
} catch (const std::exception& e) {
139+
std::cerr << "\n✗ TEST FAILED: " << e.what() << std::endl;
140+
return 1;
141+
}
142+
}

0 commit comments

Comments
 (0)