diff --git a/.github/workflows/cmake-multi-platform.yml b/.github/workflows/cmake-multi-platform.yml index 8aa6a23..ccd2f14 100644 --- a/.github/workflows/cmake-multi-platform.yml +++ b/.github/workflows/cmake-multi-platform.yml @@ -14,9 +14,15 @@ on: required: false default: '' +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + jobs: build: runs-on: ${{ matrix.os }} + # Without this a hung test holds a runner for the 6h default + timeout-minutes: 45 strategy: # 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: working-directory: ${{ steps.strings.outputs.build-output-dir }} # 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). # See https://cmake.org/cmake/help/latest/manual/ctest.1.html for more detail - run: ctest --build-config ${{ matrix.build_type }} + run: ctest --build-config ${{ matrix.build_type }} --timeout 300 --output-on-failure - name: Verify examples run # Examples use POSIX headers (unistd.h) and are not available on Windows diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index abb627f..255375d 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -9,6 +9,7 @@ on: jobs: pre-commit: runs-on: ubuntu-latest + timeout-minutes: 15 steps: - uses: actions/checkout@v4 diff --git a/.github/workflows/update-llama-cpp.yml b/.github/workflows/update-llama-cpp.yml index c4261fb..10b4854 100644 --- a/.github/workflows/update-llama-cpp.yml +++ b/.github/workflows/update-llama-cpp.yml @@ -8,6 +8,7 @@ on: jobs: update-submodule: runs-on: ubuntu-latest + timeout-minutes: 30 permissions: contents: write diff --git a/CMakeLists.txt b/CMakeLists.txt index ef46696..d0a8c12 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -166,9 +166,26 @@ if(AGENT_CPP_BUILD_TESTS) target_link_libraries(test_callbacks PRIVATE agent model ${LLAMA_COMMON_TARGET} llama) target_compile_features(test_callbacks PRIVATE cxx_std_17) + add_executable(test_chat_parser tests/test_chat_parser.cpp) + target_include_directories(test_chat_parser PRIVATE + src + tests + ${LLAMA_SOURCE_DIR}/common + ${LLAMA_SOURCE_DIR}/ggml/include + ${LLAMA_SOURCE_DIR}/include + ${LLAMA_SOURCE_DIR}/vendor + ) + # The test parses responses against real chat templates shipped by llama.cpp + target_compile_definitions(test_chat_parser PRIVATE + LLAMA_TEMPLATES_DIR="${LLAMA_SOURCE_DIR}/models/templates" + ) + target_link_libraries(test_chat_parser PRIVATE model ${LLAMA_COMMON_TARGET} llama) + target_compile_features(test_chat_parser PRIVATE cxx_std_17) + add_test(NAME ToolTests COMMAND test_tool) add_test(NAME CallbacksTests COMMAND test_callbacks) add_test(NAME GrammarTests COMMAND test_grammar) + add_test(NAME ChatParserTests COMMAND test_chat_parser) if(AGENT_CPP_BUILD_MCP) add_executable(test_mcp_client tests/test_mcp_client.cpp) @@ -185,9 +202,16 @@ if(AGENT_CPP_BUILD_TESTS) # On Windows, DLLs are placed in the bin/ directory by llama.cpp # We need to add this directory to PATH so tests can find the DLLs + # A hung test must fail rather than hold the runner until the job limit + set_tests_properties(ToolTests CallbacksTests GrammarTests ChatParserTests + PROPERTIES TIMEOUT 120 + ) + if(WIN32) - set_tests_properties(ToolTests CallbacksTests GrammarTests PROPERTIES - ENVIRONMENT "PATH=${CMAKE_BINARY_DIR}/bin\;$ENV{PATH}" + # Multi-config generators (Visual Studio) place DLLs in bin/, + # single-config ones in bin/ - both are on PATH so tests can load them + set_tests_properties(ToolTests CallbacksTests GrammarTests ChatParserTests PROPERTIES + ENVIRONMENT "PATH=${CMAKE_BINARY_DIR}/bin/${CMAKE_BUILD_TYPE}\;${CMAKE_BINARY_DIR}/bin/Release\;${CMAKE_BINARY_DIR}/bin\;$ENV{PATH}" ) endif() diff --git a/src/model.cpp b/src/model.cpp index 6482c03..97f86b3 100644 --- a/src/model.cpp +++ b/src/model.cpp @@ -203,6 +203,54 @@ Model::tokenize(const std::string& prompt) const return prompt_tokens; } +namespace { + +/// Deserializes the PEG parser derived from the chat template. An empty +/// definition yields an empty arena, which common_chat_peg_parse() treats as a +/// pure-content parser. +common_peg_arena +load_parser(const std::string& parser_definition) +{ + common_peg_arena arena; + if (!parser_definition.empty()) { + arena.load(parser_definition); + } + return arena; +} + +common_chat_msg +parse_with_parser(const common_peg_arena& parser, + const common_chat_params& params, + const std::string& response, + std::optional format_override) +{ + common_chat_parser_params syntax; + // Use explicitly configured format, or fall back to auto-detected format. + // Note that the format only selects the mapper; the PEG parser below is + // what actually drives parsing. + syntax.format = format_override.value_or(params.format); + // The parser is derived from the chat template together with the + // generation prompt it was built against, and parsing needs both + // (ggml-org/llama.cpp#18675). Without them tool calls stay as raw text. + syntax.generation_prompt = params.generation_prompt; + syntax.parse_tool_calls = true; + + try { + auto parsed_msg = + common_chat_peg_parse(parser, response, false, syntax); + parsed_msg.role = "assistant"; + return parsed_msg; + } catch (const std::exception& e) { + // llama.cpp throws plain std::runtime_error when the output does not + // match the template's format; surface it as a library error so + // callers can catch it alongside the rest of agent.cpp. + throw ModelError(std::string("failed to parse model response: ") + + e.what()); + } +} + +} + common_chat_msg Model::generate(const std::vector& messages, const std::vector& tools, @@ -226,15 +274,24 @@ Model::generate(const std::vector& messages, std::string response = generate_from_tokens(prompt_tokens, callback); - common_chat_parser_params syntax; - // Use explicitly configured format, or fall back to auto-detected format - syntax.format = config_.chat_format.value_or(params.format); - syntax.parse_tool_calls = true; + // The PEG parser only changes when the rendered template changes, so keep + // the deserialized arena around instead of re-parsing it every turn. + if (parser_source_ != params.parser) { + parser_arena_ = load_parser(params.parser); + parser_source_ = params.parser; + } - auto parsed_msg = common_chat_parse(response, false, syntax); - parsed_msg.role = "assistant"; + return parse_with_parser( + parser_arena_, params, response, config_.chat_format); +} - return parsed_msg; +common_chat_msg +parse_response(const common_chat_params& params, + const std::string& response, + std::optional format_override) +{ + return parse_with_parser( + load_parser(params.parser), params, response, format_override); } std::string diff --git a/src/model.h b/src/model.h index e06ee50..91c11ac 100644 --- a/src/model.h +++ b/src/model.h @@ -43,6 +43,21 @@ struct ModelConfig std::string load_grammar_file(const std::string& grammar_path); +/// Parses a raw model response into a chat message, using the PEG parser and +/// generation prompt derived from the model's chat template. +/// @param params result of common_chat_templates_apply() for the turn that +/// produced @p response +/// @param response the raw text generated by the model +/// @param format_override optional explicit chat format, overriding the +/// auto-detected params.format +/// @throws agent_cpp::ModelError if the response does not match the expected +/// format +common_chat_msg +parse_response( + const common_chat_params& params, + const std::string& response, + std::optional format_override = std::nullopt); + // Forward declaration class Model; @@ -198,6 +213,10 @@ class Model std::vector processed_tokens_; // Track tokens in KV cache int n_past_ = 0; // Track position in KV cache ModelConfig config_; + // Chat parser derived from the template, cached across turns along with + // the definition it was built from + std::string parser_source_; + common_peg_arena parser_arena_; }; } // namespace agent_cpp diff --git a/tests/test_chat_parser.cpp b/tests/test_chat_parser.cpp new file mode 100644 index 0000000..2eed45a --- /dev/null +++ b/tests/test_chat_parser.cpp @@ -0,0 +1,142 @@ +#include "model.h" +#include "test_utils.h" +#include +#include +#include +#include +#include + +#ifndef LLAMA_TEMPLATES_DIR +#error "LLAMA_TEMPLATES_DIR must be defined" +#endif + +namespace { + +// Progress markers on stderr, unbuffered: if a test hangs (as this one did on +// Windows) the ctest timeout output shows exactly which phase it stalled in. +void +trace(const std::string& phase) +{ + std::cerr << "[trace] " << phase << std::endl; +} + +std::string +read_file(const std::string& path) +{ + std::ifstream file(path); + if (!file) { + throw std::runtime_error("failed to open " + path); + } + std::ostringstream buffer; + buffer << file.rdbuf(); + return buffer.str(); +} + +// Mirrors how Model::generate() renders a turn, so the parser under test is +// fed the same common_chat_params the real code path produces. +common_chat_params +apply_template(const std::string& template_name, bool with_tools) +{ + const std::string template_path = + std::string(LLAMA_TEMPLATES_DIR) + "/" + template_name; + + trace("reading " + template_path); + const std::string template_source = read_file(template_path); + + trace("initializing templates"); + auto tmpls = common_chat_templates_init(nullptr, template_source); + + common_chat_msg user_msg; + user_msg.role = "user"; + user_msg.content = "Calculate 3 + 4"; + + common_chat_templates_inputs inputs; + inputs.messages = { user_msg }; + if (with_tools) { + common_chat_tool calculator; + calculator.name = "calculator"; + calculator.description = "Performs arithmetic"; + calculator.parameters = + R"({"type":"object","properties":{"a":{"type":"number"},)" + R"("b":{"type":"number"},"operation":{"type":"string"}},)" + R"("required":["a","b","operation"]})"; + inputs.tools = { calculator }; + } + inputs.tool_choice = COMMON_CHAT_TOOL_CHOICE_AUTO; + inputs.add_generation_prompt = true; + inputs.enable_thinking = false; + + trace("applying template"); + auto params = common_chat_templates_apply(tmpls.get(), inputs); + trace("template applied"); + + return params; +} + +} + +// Regression test for tool calls being returned as raw text instead of being +// parsed. llama.cpp ggml-org/llama.cpp#18675 moved parsing to a PEG parser +// derived from the chat template; parse_response() must forward both the +// derived parser and the generation prompt it was built against, otherwise +// common_chat_parse() falls back to a pure-content parser. +TEST(test_tool_call_is_parsed_from_response) +{ + auto params = apply_template("ibm-granite-granite-4.0.jinja", true); + ASSERT_TRUE(!params.parser.empty()); + + const std::string response = + "\n" + R"({"name": "calculator", "arguments": {"a": 3, "b": 4, "operation": "add"}})" + "\n"; + + auto parsed = agent_cpp::parse_response(params, response); + + ASSERT_EQ(parsed.role, std::string("assistant")); + ASSERT_EQ(parsed.tool_calls.size(), static_cast(1)); + ASSERT_EQ(parsed.tool_calls[0].name, std::string("calculator")); + ASSERT_TRUE(parsed.tool_calls[0].arguments.find("\"operation\"") != + std::string::npos); + ASSERT_TRUE(parsed.content.empty()); +} + +// A plain answer must still come back as content, with no spurious tool calls. +TEST(test_plain_response_is_parsed_as_content) +{ + auto params = apply_template("ibm-granite-granite-4.0.jinja", true); + + auto parsed = + agent_cpp::parse_response(params, "The result of 3 + 4 is 7."); + + ASSERT_TRUE(parsed.tool_calls.empty()); + ASSERT_EQ(parsed.content, std::string("The result of 3 + 4 is 7.")); +} + +// Templates rendered without tools still parse ordinary content. +TEST(test_response_without_tools_is_parsed_as_content) +{ + auto params = apply_template("ibm-granite-granite-4.0.jinja", false); + + auto parsed = agent_cpp::parse_response(params, "Hello!"); + + ASSERT_TRUE(parsed.tool_calls.empty()); + ASSERT_EQ(parsed.content, std::string("Hello!")); +} + +int +main() +{ + std::cout << "\n=== Running Chat Parser Unit Tests ===\n" << std::endl; + + try { + RUN_TEST(test_tool_call_is_parsed_from_response); + RUN_TEST(test_plain_response_is_parsed_as_content); + RUN_TEST(test_response_without_tools_is_parsed_as_content); + + std::cout << "\n=== All tests passed! āœ“ ===\n" << std::endl; + return 0; + } catch (const std::exception& e) { + std::cerr << "\nāœ— TEST FAILED: " << e.what() << std::endl; + return 1; + } +} diff --git a/tests/test_utils.h b/tests/test_utils.h index c8ca95f..239a44c 100644 --- a/tests/test_utils.h +++ b/tests/test_utils.h @@ -1,16 +1,50 @@ #pragma once -#include #include +#include +#include #include // Simple test framework macros - reusable across all test files #define TEST(name) void name() -#define ASSERT_EQ(a, b) assert((a) == (b)) -#define ASSERT_TRUE(x) assert(x) -#define ASSERT_FALSE(x) assert(!(x)) -#define ASSERT_STREQ(a, b) assert(std::string(a) == std::string(b)) +// Assertions must not rely on assert(): CI builds with CMAKE_BUILD_TYPE=Release +// defines NDEBUG, which would turn every check below into a no-op. +#define ASSERT_FAIL(expr_text) \ + do { \ + std::ostringstream oss; \ + oss << "assertion failed: " << (expr_text) << " (" << __FILE__ << ":" \ + << __LINE__ << ")"; \ + throw std::runtime_error(oss.str()); \ + } while (0) + +#define ASSERT_TRUE(x) \ + do { \ + if (!(x)) { \ + ASSERT_FAIL(#x); \ + } \ + } while (0) + +#define ASSERT_FALSE(x) \ + do { \ + if (x) { \ + ASSERT_FAIL("!(" #x ")"); \ + } \ + } while (0) + +#define ASSERT_EQ(a, b) \ + do { \ + if (!((a) == (b))) { \ + ASSERT_FAIL(#a " == " #b); \ + } \ + } while (0) + +#define ASSERT_STREQ(a, b) \ + do { \ + if (std::string(a) != std::string(b)) { \ + ASSERT_FAIL(#a " == " #b); \ + } \ + } while (0) #define RUN_TEST(name) \ do { \