From cdf4755b8e049ab0782415a1da3d31a51955a0f0 Mon Sep 17 00:00:00 2001 From: James Dumay Date: Sun, 2 Aug 2026 19:17:00 +1000 Subject: [PATCH] The tagged tool-call arguments now accept required and optional fields in any order that is valid json. Required fields are still enforced and duplicate args are forbidden. Tests cover reordered fields, missing required fields, and duplicates. Assisted-by: Codex --- common/chat-auto-parser-generator.cpp | 37 +++++--------- common/chat-peg-parser.cpp | 19 +++++++- common/chat-peg-parser.h | 11 +++++ tests/test-chat.cpp | 69 ++++++++++++++++++++++++++- 4 files changed, 110 insertions(+), 26 deletions(-) diff --git a/common/chat-auto-parser-generator.cpp b/common/chat-auto-parser-generator.cpp index af84ff323daf..4a87226c32db 100644 --- a/common/chat-auto-parser-generator.cpp +++ b/common/chat-auto-parser-generator.cpp @@ -397,12 +397,11 @@ common_peg_parser analyze_tools::build_tool_parser_tag_tagged(parser_build_conte auto schema_info = common_schema_info(); schema_info.resolve_refs(params); - // Build parser for each argument, separating required and optional - std::vector required_parsers; - std::vector optional_parsers; + // Object member order is semantically irrelevant. Build one choice of + // every declared argument and retain required-field metadata for the + // mapper to validate when the complete tool call closes. + std::vector arg_parsers; for (const auto & [param_name, param_schema] : properties.items()) { - bool is_required = required.find(param_name) != required.end(); - auto arg = p.tool_arg(p.tool_arg_open(arguments.name_prefix + p.tool_arg_name(p.literal(param_name)) + arguments.name_suffix) + @@ -415,29 +414,19 @@ common_peg_parser analyze_tools::build_tool_parser_tag_tagged(parser_build_conte p.tool_arg_close(p.literal(arguments.value_suffix))))); auto named_arg = p.rule("tool-" + name + "-arg-" + param_name, arg); - if (is_required) { - required_parsers.push_back(named_arg); - } else { - optional_parsers.push_back(named_arg); - } + arg_parsers.push_back(named_arg); } - // Build required arg sequence in definition order common_peg_parser args_seq = p.eps(); - for (size_t i = 0; i < required_parsers.size(); i++) { - if (i > 0) { - args_seq = args_seq + p.space(); - } - args_seq = args_seq + required_parsers[i]; + for (const auto & required_arg : required) { + args_seq = args_seq + p.tool_required_arg(required_arg); } - - // Build optional args with flexible ordering - if (!optional_parsers.empty()) { - common_peg_parser any_opt = p.choice(); - for (const auto & opt : optional_parsers) { - any_opt |= opt; + if (!arg_parsers.empty()) { + common_peg_parser any_arg = p.choice(); + for (const auto & arg : arg_parsers) { + any_arg |= arg; } - args_seq = args_seq + p.repeat(p.space() + any_opt, 0, -1); + args_seq = args_seq + p.zero_or_more(any_arg + p.space()); } if (!arguments.start.empty()) { @@ -462,7 +451,7 @@ common_peg_parser analyze_tools::build_tool_parser_tag_tagged(parser_build_conte // Only peek for an arg tag when there are required args that must follow. // When all args are optional, the model may emit no arg tags at all (#20650). - auto atomic_peek = (!arguments.name_prefix.empty() && !required_parsers.empty()) ? + auto atomic_peek = (!arguments.name_prefix.empty() && !required.empty()) ? std::optional(p.peek(p.literal(arguments.name_prefix))) : std::nullopt; auto func_parser = build_func_parser(p, name, call_id_section, have_call_id, args_seq, atomic_peek); tool_choice |= p.rule("tool-" + name, func_parser); diff --git a/common/chat-peg-parser.cpp b/common/chat-peg-parser.cpp index f786f5ff2314..b34ffe560f2f 100644 --- a/common/chat-peg-parser.cpp +++ b/common/chat-peg-parser.cpp @@ -300,6 +300,7 @@ void common_chat_peg_mapper::map(const common_peg_ast_node & node) { bool is_arg_name = node.tag == common_chat_peg_builder::TOOL_ARG_NAME; bool is_arg_value = node.tag == common_chat_peg_builder::TOOL_ARG_VALUE; bool is_arg_string_value = node.tag == common_chat_peg_builder::TOOL_ARG_STRING_VALUE; + bool is_required_arg = node.tag.rfind(common_chat_peg_builder::TOOL_REQUIRED_ARG_PREFIX, 0) == 0; if (is_tool_open) { pending_tool_call = common_chat_tool_call(); @@ -307,6 +308,13 @@ void common_chat_peg_mapper::map(const common_peg_ast_node & node) { arg_count = 0; args_buffer.clear(); closing_quote_pending = false; + required_args.clear(); + seen_args.clear(); + } + + if (is_required_arg && current_tool) { + required_args.insert(node.tag.substr(std::char_traits::length( + common_chat_peg_builder::TOOL_REQUIRED_ARG_PREFIX))); } if (is_tool_id && current_tool) { @@ -348,11 +356,15 @@ void common_chat_peg_mapper::map(const common_peg_ast_node & node) { } if (is_arg_name && current_tool) { + const std::string arg_name(trim(node.text)); + if (!seen_args.insert(arg_name).second) { + throw std::runtime_error("Duplicate tool argument: " + arg_name); + } std::string arg_entry; if (arg_count > 0) { arg_entry = ","; } - arg_entry += ordered_json(trim(node.text)).dump() + ":"; + arg_entry += ordered_json(arg_name).dump() + ":"; ++arg_count; auto & target = args_target(); @@ -393,6 +405,11 @@ void common_chat_peg_mapper::map(const common_peg_ast_node & node) { } if (is_tool_close && current_tool) { + for (const auto & required_arg : required_args) { + if (seen_args.find(required_arg) == seen_args.end()) { + throw std::runtime_error("Missing required tool argument: " + required_arg); + } + } // Flush buffer to arguments if tool name was never seen if (current_tool->name.empty() && !args_buffer.empty()) { current_tool->arguments = args_buffer; diff --git a/common/chat-peg-parser.h b/common/chat-peg-parser.h index cd14f2c11750..fb2180874895 100644 --- a/common/chat-peg-parser.h +++ b/common/chat-peg-parser.h @@ -5,6 +5,7 @@ #include #include +#include #include class common_chat_peg_mapper { @@ -26,6 +27,8 @@ class common_chat_peg_mapper { int arg_count = 0; bool closing_quote_pending = false; std::string args_buffer; // Buffer to delay arguments until tool name is known + std::set required_args; + std::set seen_args; // Returns a reference to the active argument destination string. // Before tool_name is known, writes go to args_buffer; after, to current_tool->arguments. @@ -75,6 +78,7 @@ class common_chat_peg_builder : public common_peg_parser_builder { static constexpr const char * TOOL_ARG_NAME = "tool-arg-name"; static constexpr const char * TOOL_ARG_VALUE = "tool-arg-value"; static constexpr const char * TOOL_ARG_STRING_VALUE = "tool-arg-string-value"; // For schema-declared string types + static constexpr const char * TOOL_REQUIRED_ARG_PREFIX = "tool-required-arg:"; // Low-level tag methods (from former common_chat_peg_base_builder) common_peg_parser reasoning_block(const common_peg_parser & p) { return tag(REASONING_BLOCK, p); } @@ -104,6 +108,13 @@ class common_chat_peg_builder : public common_peg_parser_builder { common_peg_parser tool_arg_string_value(const common_peg_parser & p) { return tag(TOOL_ARG_STRING_VALUE, p); } common_peg_parser tool_arg_json_value(const common_peg_parser & p) { return tag(TOOL_ARG_VALUE, p); } + // Attach schema metadata to the AST without consuming model output. The + // mapper uses this to enforce required tagged arguments after accepting + // them in arbitrary object-key order. + common_peg_parser tool_required_arg(const std::string & name) { + return tag(std::string(TOOL_REQUIRED_ARG_PREFIX) + name, eps()); + } + // Return a parser that parses the prefix of a string, up to a given delimiter. common_peg_parser prefix(const std::string & s, const std::string & delimiter = {}); diff --git a/tests/test-chat.cpp b/tests/test-chat.cpp index ef02fdde57ef..dfff0168d1ff 100644 --- a/tests/test-chat.cpp +++ b/tests/test-chat.cpp @@ -1453,6 +1453,8 @@ class peg_tester { const std::string & template_path() const { return template_path_; } + common_chat_templates * templates() const { return tmpls_.get(); } + peg_test_builder test(const std::string & input); }; @@ -4260,6 +4262,15 @@ static void test_template_output_peg_parsers(bool detailed_debug) { { auto tst = peg_tester("models/templates/GLM-4.7-Flash.jinja", detailed_debug); + static const common_chat_tool terminal_tool{ + "terminal", "Run or interact with a terminal command", + R"({"type":"object","properties":{"security_risk":{"type":"string"},"summary":{"type":"string"},"command":{"type":"string"},"is_input":{"type":"boolean"},"timeout":{"type":"integer"},"reset":{"type":"boolean"}},"required":["command","security_risk"]})", + }; + static const common_chat_tool think_tool{ + "think", "Record reasoning", + R"({"type":"object","properties":{"summary":{"type":"string"},"thought":{"type":"string"}},"required":["thought"]})", + }; + // Pure content (no reasoning) tst.test("Hello, world!\nWhat's up?") .enable_thinking(false) @@ -4286,6 +4297,62 @@ static void test_template_output_peg_parsers(bool detailed_debug) { .expect_reconstruction() .run(); + // OpenAI tool arguments are object members: required and optional + // fields may be interleaved in any order. + tst.test( + "terminal" + "commandC-c" + "is_inputtrue" + "security_riskLOW" + "") + .enable_thinking(false) + .tools({ terminal_tool, think_tool }) + .expect_tool_calls({ + { "terminal", R"({"command":"C-c","is_input":true,"security_risk":"LOW"})", {} }, + }) + .run(); + + tst.test( + "think" + "summaryInspect the failure" + "thoughtCheck the process state." + "") + .enable_thinking(false) + .tools({ terminal_tool, think_tool }) + .expect_tool_calls({ + { "think", R"({"summary":"Inspect the failure","thought":"Check the process state."})", {} }, + }) + .run(); + + // Flexible ordering must not weaken the schema's presence and + // uniqueness rules. + common_chat_templates_inputs validation_inputs; + validation_inputs.messages = { message_user }; + validation_inputs.tools = { terminal_tool, think_tool }; + validation_inputs.enable_thinking = false; + auto validation_parser = make_peg_parser(tst.templates(), validation_inputs, detailed_debug); + + try { + validation_parser.parse( + "terminal" + "commandpwd" + "", false); + throw std::runtime_error("Expected missing required tagged argument to fail"); + } catch (const std::exception & e) { + assert_contains(e.what(), "Missing required tool argument: security_risk"); + } + + try { + validation_parser.parse( + "think" + "thoughtone" + "thoughttwo" + "", false); + throw std::runtime_error("Expected duplicate tagged argument to fail"); + } catch (const std::exception & e) { + assert_contains(e.what(), "Duplicate tool argument: thought"); + } + // Tool call with reasoning (forced-open mode) tst.test( "I'm\nthinking" @@ -4397,7 +4464,7 @@ static void test_template_output_peg_parsers(bool detailed_debug) { "Thinking.\n" "" "get_weather" - "cityTokyo" + "countryJapan" "\n"; bool got_runtime_error = false;