From f78ec03fe814139d720ec2b4fad90cc0dee7f093 Mon Sep 17 00:00:00 2001 From: Bart de Boer Date: Tue, 26 May 2026 06:37:31 +0000 Subject: [PATCH 1/2] Fix tagged tool calls inside reasoning --- common/chat-auto-parser-generator.cpp | 64 ++++++++++++++++++++- tests/test-chat.cpp | 81 +++++++++++++++++++-------- 2 files changed, 120 insertions(+), 25 deletions(-) diff --git a/common/chat-auto-parser-generator.cpp b/common/chat-auto-parser-generator.cpp index db3a6cc6fe3..9818390abff 100644 --- a/common/chat-auto-parser-generator.cpp +++ b/common/chat-auto-parser-generator.cpp @@ -454,13 +454,24 @@ common_peg_parser analyze_tools::build_tool_parser_tag_tagged(parser_build_conte auto require_tools = inputs.tool_choice == COMMON_CHAT_TOOL_CHOICE_REQUIRED; common_peg_parser tool_calls = p.eps(); + common_peg_parser tool_call_item = p.eps(); if (!format.per_call_start.empty()) { auto wrapped_call = format.per_call_start + p.space() + tool_choice + p.space() + format.per_call_end; + if (format.section_start.empty()) { + // Lazy grammars are activated from the first tool-call marker in the + // generated suffix. The trigger grammar must validate the tool call, + // but it should not require the tool call to be the last segment in + // the completion. Tagged tool calls can appear inside reasoning + // blocks, followed by , content, or more segments that the + // full parser will handle afterwards. + p.trigger_rule("tool-call", wrapped_call + p.rest()); + } + tool_call_item = wrapped_call + p.space(); if (inputs.parallel_tool_calls) { - tool_calls = p.trigger_rule("tool-call", wrapped_call + p.zero_or_more(p.space() + wrapped_call) + p.space()); + tool_calls = wrapped_call + p.zero_or_more(p.space() + wrapped_call) + p.space(); } else { - tool_calls = p.trigger_rule("tool-call", wrapped_call + p.space()); + tool_calls = wrapped_call + p.space(); } if (!format.section_start.empty()) { tool_calls = p.trigger_rule("tool-calls", @@ -486,6 +497,55 @@ common_peg_parser analyze_tools::build_tool_parser_tag_tagged(parser_build_conte std::string trigger_marker = !format.section_start.empty() ? format.section_start : format.per_call_start; auto content_before_tools = trigger_marker.empty() ? p.eps() : p.until(trigger_marker); + + // Treat tag-based reasoning markers as mode switches rather than containers that + // hide tools. Text inside the reasoning markers is emitted as reasoning_content, + // text outside is emitted as content, and valid tool calls can appear in either + // mode. This path applies to tagged-argument tool formats, such as: + // + // + // + // value + // + // + // + // Use a specific tool boundary that includes the function-name prefix when + // possible, so prose such as "I might call later" remains text. + if (ctx.extracting_reasoning && ctx.reasoning && + !trim_whitespace(ctx.reasoning->start).empty() && + !trim_whitespace(ctx.reasoning->end).empty() && + format.section_start.empty() && + !trim_whitespace(format.per_call_start).empty() && + !trim_whitespace(format.per_call_end).empty()) { + const std::string think_start = trim_whitespace(ctx.reasoning->start); + const std::string think_end = trim_whitespace(ctx.reasoning->end); + const std::string tool_start = trim_whitespace(format.per_call_start) + + (function.name_prefix.empty() ? "" : "\n" + function.name_prefix); + + auto in_think_boundary = p.choice({ + p.literal(tool_start), + p.literal(think_end), + }); + auto root_boundary = p.choice({ + p.literal(think_start), + p.literal(think_end), + p.literal(tool_start), + }); + + auto reasoning_chunk = p.reasoning( + p.negate(in_think_boundary) + p.any() + p.until_one_of({ tool_start, think_end })); + auto content_chunk = p.content( + p.negate(root_boundary) + p.any() + p.until_one_of({ think_start, think_end, tool_start })); + + auto stale_think_end = p.literal(think_end) + p.space(); + auto think_block = + p.literal(think_start) + p.space() + + p.zero_or_more(p.choice({ tool_call_item, reasoning_chunk })) + + p.optional(stale_think_end); + + return p.zero_or_more(p.choice({ think_block, tool_call_item, stale_think_end, content_chunk })) + p.end(); + } + return ctx.reasoning_parser + p.optional(p.content(content_before_tools)) + tool_calls + p.end(); } diff --git a/tests/test-chat.cpp b/tests/test-chat.cpp index 30ea2c07213..ed015a8b97d 100644 --- a/tests/test-chat.cpp +++ b/tests/test-chat.cpp @@ -477,6 +477,24 @@ static common_chat_tool python_tool{ "required": ["code"] })", }; +static common_chat_tool write_tool{ + /* .name = */ "write", + /* .description = */ "write a file", + /* .parameters = */ R"({ + "type": "object", + "properties": { + "file": { + "type": "string", + "description": "File path." + }, + "content": { + "type": "string", + "description": "Full replacement file content." + } + }, + "required": ["file", "content"] + })", +}; static common_chat_tool html_tool{ /* .name = */ "html", @@ -1919,7 +1937,8 @@ static void test_template_output_peg_parsers(bool detailed_debug) { .expect_content(R"({"amount": 123.45, "date": "2025-12-03"})") .run(); - // tool call segment in reasoning + // Qwen3.5 can emit tool calls inside . Treat as a + // reasoning/content mode switch, not as a container that hides tools. tst.test( "Let's call a tool: \n" "\n" @@ -1946,22 +1965,45 @@ static void test_template_output_peg_parsers(bool detailed_debug) { .tools({ python_tool }) - .expect_reasoning( - "Let's call a tool: \n" - "\n" - "\n" - "def hello():\n" - " print(\"Not the real call!\")\n" - "\n" - "hello()\n" - "\n" - "\n" - "") + .expect_reasoning("Let's call a tool:") .expect_tool_calls({ + { "python", "{\"code\": \"def hello():\\n print(\\\"Not the real call!\\\")\\n\\nhello()\"}", {} }, { "python", "{\"code\": \"def hello():\\n print(\\\"Hello, world!\\\")\\n\\nhello()\"}", {} }, }) .run(); + tst.test( + "Need to write a debug file.\n" + "\n" + "\n" + "\n" + "/workspace/jsonlfilter/debug_test.go\n" + "\n" + "\n" + "package main\n" + "\n" + "func main() {\n" + " line := `{name:Jette,status:sleepy}`\n" + " fmt.Printf(\"Status: %q\\n\", result[\"status\"])\n" + "}\n" + "\n" + "\n" + "\n" + "\n" + "\n" + "Done.") + .enable_thinking(true) + .reasoning_format(COMMON_REASONING_FORMAT_AUTO) + .tools({ + write_tool + }) + .expect_reasoning("Need to write a debug file.") + .expect_content("Done.") + .expect_tool_calls({ + { "write", R"({"file":"/workspace/jsonlfilter/debug_test.go","content":"package main\n\nfunc main() {\n line := `{name:Jette,status:sleepy}`\n fmt.Printf(\"Status: %q\\n\", result[\"status\"])\n}\n"})", {} }, + }) + .run(); + // No args tool tst.test( "\n" @@ -2417,7 +2459,8 @@ static void test_template_output_peg_parsers(bool detailed_debug) { .expect_content(R"({"amount": 123.45, "date": "2025-12-03"})") .run(); - // tool call segment in reasoning + // Qwen3.5 can emit tool calls inside . Treat as a + // reasoning/content mode switch, not as a container that hides tools. tst.test( "Let's call a tool: \n" "\n" @@ -2445,17 +2488,9 @@ static void test_template_output_peg_parsers(bool detailed_debug) { .tools({ python_tool }) - .expect_reasoning("Let's call a tool: \n" - "\n" - "\n" - "def hello():\n" - " print(\"Not the real call!\")\n" - "\n" - "hello()\n" - "\n" - "\n" - "\n") + .expect_reasoning("Let's call a tool:") .expect_tool_calls({ + { "python", "{\"code\": \"def hello():\\n print(\\\"Not the real call!\\\")\\n\\nhello()\"}", {} }, { "python", "{\"code\": \"def hello():\\n print(\\\"Hello, world!\\\")\\n\\nhello()\"}", {} }, }) .run(); From ce979f0b62e9462385129720baae1e2a95e9a50c Mon Sep 17 00:00:00 2001 From: Bart de Boer Date: Mon, 25 May 2026 21:34:18 +0000 Subject: [PATCH 2/2] Accept required tagged tool args in any order --- common/chat-auto-parser-generator.cpp | 44 +++++++++++++----- tests/test-chat.cpp | 67 +++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 11 deletions(-) diff --git a/common/chat-auto-parser-generator.cpp b/common/chat-auto-parser-generator.cpp index 9818390abff..79c9659a228 100644 --- a/common/chat-auto-parser-generator.cpp +++ b/common/chat-auto-parser-generator.cpp @@ -8,6 +8,8 @@ #include "nlohmann/json.hpp" #include "peg-parser.h" +#include +#include #include #include @@ -405,22 +407,42 @@ common_peg_parser analyze_tools::build_tool_parser_tag_tagged(parser_build_conte } } - // 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]; - } - - // Build optional args with flexible ordering + common_peg_parser optional_args = p.eps(); if (!optional_parsers.empty()) { common_peg_parser any_opt = p.choice(); for (const auto & opt : optional_parsers) { any_opt |= opt; } - args_seq = args_seq + p.repeat(p.space() + any_opt, 0, -1); + optional_args = p.repeat(any_opt + p.space(), 0, -1); + } + + // Build required args in any order. Tagged arguments carry their + // parameter name in-band, so the model should not have to emit required + // arguments in schema/property order. + common_peg_parser args_seq = p.eps(); + constexpr size_t max_required_permutations = 6; + if (required_parsers.empty()) { + args_seq = optional_args; + } else if (required_parsers.size() <= max_required_permutations) { + std::vector order(required_parsers.size()); + std::iota(order.begin(), order.end(), 0); + + common_peg_parser required_orders = p.choice(); + do { + common_peg_parser seq = optional_args; + for (size_t idx : order) { + seq = seq + required_parsers[idx] + p.space() + optional_args; + } + required_orders |= seq; + } while (std::next_permutation(order.begin(), order.end())); + + args_seq = required_orders; + } else { + // Avoid factorial grammar growth for unusually large schemas. + args_seq = optional_args; + for (const auto & required : required_parsers) { + args_seq = args_seq + required + p.space() + optional_args; + } } if (!arguments.start.empty()) { diff --git a/tests/test-chat.cpp b/tests/test-chat.cpp index ed015a8b97d..e2a53dec6c0 100644 --- a/tests/test-chat.cpp +++ b/tests/test-chat.cpp @@ -495,6 +495,32 @@ static common_chat_tool write_tool{ "required": ["file", "content"] })", }; +static common_chat_tool edit_snake_tool{ + /* .name = */ "edit", + /* .description = */ "edit a file", + /* .parameters = */ R"({ + "type": "object", + "properties": { + "file": { + "type": "string", + "description": "File path." + }, + "new_string": { + "type": "string", + "description": "Replacement text." + }, + "old_string": { + "type": "string", + "description": "Exact text to replace." + }, + "replace_all": { + "type": "boolean", + "description": "Replace every occurrence." + } + }, + "required": ["file", "old_string", "new_string"] + })", +}; static common_chat_tool html_tool{ /* .name = */ "html", @@ -1926,6 +1952,47 @@ static void test_template_output_peg_parsers(bool detailed_debug) { }) .run(); + tst.test( + "\n" + "\n" + "\n" + "/workspace/jsonlfilter/test_parse.go\n" + "\n" + "\n" + "package main\n" + "\n" + "import \"testing\"\n" + "\n" + "func TestDebug(t *testing.T) {\n" + "\tline := `{name:Jette,status:sleepy}`\n" + "\tt.Logf(\"Status: %q\\n\", result[\"status\"])\n" + "}\n" + "\n" + "\n" + "package main\n" + "\n" + "import (\n" + "\t\"strings\"\n" + "\t\"testing\"\n" + ")\n" + "\n" + "func TestDebug(t *testing.T) {\n" + "\tline := `{name:Jette,status:sleepy}`\n" + "\tt.Logf(\"Status: %q\\n\", result[\"status\"])\n" + "}\n" + "\n" + "\n" + "") + .enable_thinking(false) + .reasoning_format(COMMON_REASONING_FORMAT_AUTO) + .tools({ + edit_snake_tool + }) + .expect_tool_calls({ + { "edit", R"({"file":"/workspace/jsonlfilter/test_parse.go","old_string":"package main\n\nimport \"testing\"\n\nfunc TestDebug(t *testing.T) {\n\tline := `{name:Jette,status:sleepy}`\n\tt.Logf(\"Status: %q\\n\", result[\"status\"])\n}","new_string":"package main\n\nimport (\n\t\"strings\"\n\t\"testing\"\n)\n\nfunc TestDebug(t *testing.T) {\n\tline := `{name:Jette,status:sleepy}`\n\tt.Logf(\"Status: %q\\n\", result[\"status\"])\n}"})", {} }, + }) + .run(); + tst.test( "I need to output the invoice details in JSON\n" "\n\n"