feat(server): parse <function_calls> and <invoke> XML tool emissions - #623
feat(server): parse <function_calls> and <invoke> XML tool emissions#623dpavlin wants to merge 1 commit into
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
28614dc to
33d5cfd
Compare
|
@cubic-dev-ai review |
@dpavlin I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
33d5cfd to
a4543aa
Compare
|
@cubic-dev-ai review |
@dpavlin I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
a4543aa to
83a200f
Compare
|
@cubic-dev-ai review |
@dpavlin I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
83a200f to
b1bc46f
Compare
|
@cubic-dev-ai review |
@dpavlin I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
b1bc46f to
4a71527
Compare
|
@cubic-dev-ai review |
@dpavlin I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
4a71527 to
3c2f27a
Compare
|
@cubic-dev-ai review |
@dpavlin I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
3c2f27a to
6bb855a
Compare
|
@cubic-dev-ai review |
@dpavlin I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
6bb855a to
b193ceb
Compare
@dpavlin I have started the AI code review. It will take a few minutes to complete. |
9048e73 to
8792f75
Compare
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/src/server/tool_parser.cpp">
<violation number="1" location="server/src/server/tool_parser.cpp:104">
P2: When an `<invoke>` opener contains more than 25 whitespace characters or arrives with `name` split after the 32-byte window, the emitter flushes part of the opener as assistant text before `is_invoke_open_at()` recognizes it. Preserve potential `<invoke` prefixes dynamically, or impose and enforce a matching maximum whitespace bound instead of relying on this fixed holdback.</violation>
<violation number="2" location="server/src/server/tool_parser.cpp:700">
P2: When an invoke argument is a schema-typed string whose JSON value is `"null"`, this new coercion path changes it to JSON null and sends the tool the wrong argument. Apply the null sentinel only when the schema permits null; preserve JSON strings for string-typed properties.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| return false; | ||
| } | ||
|
|
||
| static constexpr size_t INVOKE_OPEN_HOLDBACK = 32; |
There was a problem hiding this comment.
P2: When an <invoke> opener contains more than 25 whitespace characters or arrives with name split after the 32-byte window, the emitter flushes part of the opener as assistant text before is_invoke_open_at() recognizes it. Preserve potential <invoke prefixes dynamically, or impose and enforce a matching maximum whitespace bound instead of relying on this fixed holdback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/server/tool_parser.cpp, line 104:
<comment>When an `<invoke>` opener contains more than 25 whitespace characters or arrives with `name` split after the 32-byte window, the emitter flushes part of the opener as assistant text before `is_invoke_open_at()` recognizes it. Preserve potential `<invoke` prefixes dynamically, or impose and enforce a matching maximum whitespace bound instead of relying on this fixed holdback.</comment>
<file context>
@@ -100,12 +101,39 @@ static bool declared_tool_open_at(const std::string & text, size_t pos,
return false;
}
+static constexpr size_t INVOKE_OPEN_HOLDBACK = 32;
+
+static bool is_invoke_open_at(const std::string & text, size_t idx) {
</file context>
8792f75 to
b81a6e6
Compare
|
@cubic-dev-ai review |
@dpavlin I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 existing issue remains and 1 new issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/src/server/tool_parser.cpp">
<violation number="1" location="server/src/server/tool_parser.cpp:120">
P2: In reasoning (and content) mode, `find_tool_syntax_start` now treats any literal `<function_calls>` prefix or `<invoke name|tool =` sequence as a tool opener. If reasoning/quoted prose mentions these forms for a declared tool name, the buffer is diverted to TOOL_BUFFER and, when the call is incomplete or unparseable, `emit_finish` suppresses the whole buffered text instead of leaking it. That silently deletes legitimate model output that merely quoted the XML syntax. Consider gating interception on the full `<invoke` block being parseable as an allowed call (e.g. require a closing `</invoke>`/`</function_calls>` and valid args), or fall back to emitting the buffered text as content when the tool parse fails, rather than dropping it.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
| if (text.compare(idx, sizeof(TOOL_OPEN) - 1, TOOL_OPEN) == 0 || | ||
| text.compare(idx, sizeof(FUNCTION_CALL_OPEN) - 1, FUNCTION_CALL_OPEN) == 0 || | ||
| text.compare(idx, sizeof(FUNCTION_CALLS_OPEN) - 1, FUNCTION_CALLS_OPEN) == 0 || | ||
| is_invoke_open_at(text, idx) || |
There was a problem hiding this comment.
P2: In reasoning (and content) mode, find_tool_syntax_start now treats any literal <function_calls> prefix or <invoke name|tool = sequence as a tool opener. If reasoning/quoted prose mentions these forms for a declared tool name, the buffer is diverted to TOOL_BUFFER and, when the call is incomplete or unparseable, emit_finish suppresses the whole buffered text instead of leaking it. That silently deletes legitimate model output that merely quoted the XML syntax. Consider gating interception on the full <invoke block being parseable as an allowed call (e.g. require a closing </invoke>/</function_calls> and valid args), or fall back to emitting the buffered text as content when the tool parse fails, rather than dropping it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/server/tool_parser.cpp, line 120:
<comment>In reasoning (and content) mode, `find_tool_syntax_start` now treats any literal `<function_calls>` prefix or `<invoke name|tool =` sequence as a tool opener. If reasoning/quoted prose mentions these forms for a declared tool name, the buffer is diverted to TOOL_BUFFER and, when the call is incomplete or unparseable, `emit_finish` suppresses the whole buffered text instead of leaking it. That silently deletes legitimate model output that merely quoted the XML syntax. Consider gating interception on the full `<invoke` block being parseable as an allowed call (e.g. require a closing `</invoke>`/`</function_calls>` and valid args), or fall back to emitting the buffered text as content when the tool parse fails, rather than dropping it.</comment>
<file context>
@@ -100,12 +101,23 @@ static bool declared_tool_open_at(const std::string & text, size_t pos,
if (text.compare(idx, sizeof(TOOL_OPEN) - 1, TOOL_OPEN) == 0 ||
text.compare(idx, sizeof(FUNCTION_CALL_OPEN) - 1, FUNCTION_CALL_OPEN) == 0 ||
+ text.compare(idx, sizeof(FUNCTION_CALLS_OPEN) - 1, FUNCTION_CALLS_OPEN) == 0 ||
+ is_invoke_open_at(text, idx) ||
text.compare(idx, sizeof(FUNCTION_OPEN) - 1, FUNCTION_OPEN) == 0 ||
text.compare(idx, sizeof(BARE_FUNCTION_OPEN) - 1, BARE_FUNCTION_OPEN) == 0 ||
</file context>
b81a6e6 to
9fd860c
Compare
|
@cubic-dev-ai review |
@dpavlin I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 existing issue remains and 1 new issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/src/server/sse_emitter.cpp">
<violation number="1" location="server/src/server/sse_emitter.cpp:632">
P2: When a tool block appears inside `<think>` and more reasoning follows the block before `</think>`, this path exposes that reasoning as visible assistant content. Split `parsed.cleaned_text` at `</think>` and route the prefix through the reasoning emission path before emitting only the suffix as content.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
9fd860c to
ba8cd44
Compare
|
@cubic-dev-ai review |
@dpavlin I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 4 files
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
ba8cd44 to
592b299
Compare
|
@cubic-dev-ai review |
@dpavlin I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 5 files
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
592b299 to
8bb661f
Compare
|
@cubic-dev-ai review |
@dpavlin I have started the AI code review. It will take a few minutes to complete. |
|
@cubic-dev-ai please report all issues, including existing ones |
@dpavlin I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 existing issues remain and 3 new issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/src/server/sse_emitter.cpp">
<violation number="1" location="server/src/server/sse_emitter.cpp:19">
P3: This newly added `trim_ws` is unused and duplicates the parser's existing helper, adding dead code without affecting XML parsing. Remove this helper.</violation>
</file>
<file name="server/test/test_server_unit.cpp">
<violation number="1" location="server/test/test_server_unit.cpp:884">
P3: The new invoke-parsing tests never exercise the `<param=...>` / `<parameter=...>` equals-attribute forms or the `<parameter name="...">` variant inside `<invoke>`, even though the PR claims these formats are supported and `re_invoke_param_xml` implements them. Every added invoke test uses only `<param name="...">`. Add a case for `<invoke name="read"><param=path>foo.go</param></invoke>` and at least one `<parameter name=...>` / `<parameter=...>` so the claimed formats are actually verified rather than only the name-attribute path.</violation>
</file>
<file name="server/src/server/tool_parser.cpp">
<violation number="1" location="server/src/server/tool_parser.cpp:250">
P2: This change to the shared convert_param_value helper alters `"null"` handling for all existing tool-call patterns, not just the new <invoke> parser. Previously the literal string "null" always became JSON null (checked first, before the props lookup); now it only becomes null when the field is nullable, otherwise the 4-char string is kept. For numeric or unlisted fields, "null" now stays a string (and integer coercion fails), changing output for existing JSON/XML parsers beyond the intended invoke feature. Scope the new behavior to the invoke path, or gate it explicitly, to avoid silent regressions for other clients.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Re-trigger cubic
| allows_null = true; | ||
| } | ||
|
|
||
| if (val == "null") { |
There was a problem hiding this comment.
P2: This change to the shared convert_param_value helper alters "null" handling for all existing tool-call patterns, not just the new parser. Previously the literal string "null" always became JSON null (checked first, before the props lookup); now it only becomes null when the field is nullable, otherwise the 4-char string is kept. For numeric or unlisted fields, "null" now stays a string (and integer coercion fails), changing output for existing JSON/XML parsers beyond the intended invoke feature. Scope the new behavior to the invoke path, or gate it explicitly, to avoid silent regressions for other clients.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/server/tool_parser.cpp, line 250:
<comment>This change to the shared convert_param_value helper alters `"null"` handling for all existing tool-call patterns, not just the new <invoke> parser. Previously the literal string "null" always became JSON null (checked first, before the props lookup); now it only becomes null when the field is nullable, otherwise the 4-char string is kept. For numeric or unlisted fields, "null" now stays a string (and integer coercion fails), changing output for existing JSON/XML parsers beyond the intended invoke feature. Scope the new behavior to the invoke path, or gate it explicitly, to avoid silent regressions for other clients.</comment>
<file context>
@@ -151,55 +166,90 @@ size_t tool_syntax_holdback(const json & tools) {
+ allows_null = true;
+ }
+
+ if (val == "null") {
+ return allows_null ? json(nullptr) : json(val);
+ }
</file context>
| static constexpr size_t THINK_OPEN_LEN = 7; | ||
| static constexpr size_t THINK_CLOSE_LEN = 8; | ||
|
|
||
| static std::string trim_ws(const std::string & v) { |
There was a problem hiding this comment.
P3: This newly added trim_ws is unused and duplicates the parser's existing helper, adding dead code without affecting XML parsing. Remove this helper.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/server/sse_emitter.cpp, line 19:
<comment>This newly added `trim_ws` is unused and duplicates the parser's existing helper, adding dead code without affecting XML parsing. Remove this helper.</comment>
<file context>
@@ -16,6 +16,14 @@ static const char THINK_CLOSE[] = "</think>";
static constexpr size_t THINK_OPEN_LEN = 7;
static constexpr size_t THINK_CLOSE_LEN = 8;
+static std::string trim_ws(const std::string & v) {
+ const char * ws = " \t\r\n";
+ const size_t a = v.find_first_not_of(ws);
</file context>
| " Let me read the remaining key files I haven't seen yet to fully understand the codebase."); | ||
| } | ||
|
|
||
| TEST_CASE(ServerUnitFixture, test_parse_bare_invoke_xml) { |
There was a problem hiding this comment.
P3: The new invoke-parsing tests never exercise the <param=...> / <parameter=...> equals-attribute forms or the <parameter name="..."> variant inside <invoke>, even though the PR claims these formats are supported and re_invoke_param_xml implements them. Every added invoke test uses only <param name="...">. Add a case for <invoke name="read"><param=path>foo.go</param></invoke> and at least one <parameter name=...> / <parameter=...> so the claimed formats are actually verified rather than only the name-attribute path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/test/test_server_unit.cpp, line 884:
<comment>The new invoke-parsing tests never exercise the `<param=...>` / `<parameter=...>` equals-attribute forms or the `<parameter name="...">` variant inside `<invoke>`, even though the PR claims these formats are supported and `re_invoke_param_xml` implements them. Every added invoke test uses only `<param name="...">`. Add a case for `<invoke name="read"><param=path>foo.go</param></invoke>` and at least one `<parameter name=...>` / `<parameter=...>` so the claimed formats are actually verified rather than only the name-attribute path.</comment>
<file context>
@@ -774,6 +809,395 @@ TEST_CASE(ServerUnitFixture, test_parse_space_function_tool_xml_rejects_malforme
+ " Let me read the remaining key files I haven't seen yet to fully understand the codebase.");
+}
+
+TEST_CASE(ServerUnitFixture, test_parse_bare_invoke_xml) {
+ const std::string text =
+ "Reading configuration:\n"
</file context>
Add parser pattern 12 supporting <function_calls><invoke name="..."> <param name="...">...</param></invoke></function_calls> format emitted by DeepSeek-V4. Handles variable/arbitrary tool invocations directly in the text within <function_calls> containers, supports JSON Schema union types (e.g. ["integer", "null"]) while preserving string types and converting null sentinels appropriately, validates required parameters across all invoke body formats, intercepts tool calls during streaming thinking mode, ensures correct thinking block lifecycles via emit_reasoning_delta, and preserves tool buffer entry mode to route unclosed <think> reasoning suffixes back to reasoning delta.
8bb661f to
d5b93f6
Compare
|
this has turned into unmanageable mess. I will try again from scratch. |
1 participant
Summary
Adds support for Claude/Anthropic/Agentic XML tool call emissions using
<function_calls><invoke name="..."><param name="...">...</param></invoke></function_calls>as well as standalone<invoke name="...">blocks.Motivation
When models (such as DeepSeek-V4 or agentic fine-tunes) emit multiple parallel tool calls (e.g. reading multiple files in a batch), they frequently emit
<function_calls><invoke name="read"><param name="path">...</param></invoke>...</function_calls>. Previously, these XML blocks were unparsed and leaked directly into visible assistant text.Changes
tool_parser.cpp):re_function_calls_block()andre_invoke_xml()to extract tool names and parameters from<invoke>tags.<param name="...">...\</param>,<parameter name="...">...\</parameter>,<param=...>, and inline JSON bodies.convert_param_value.<function_calls>...</function_calls>and<invoke>...</invoke>blocks from visible output without stripping surrounding natural language.find_tool_syntax_start/tool_syntax_holdback):<function_calls>and<invokeopeners to streaming holdback so streamed tool calls are properly intercepted before reaching the client.test_server_unit.cpp):test_parse_function_calls_multi_invoke_xml: Verifies multi-file<invoke>parsing and text stripping.test_parse_bare_invoke_xml: Verifies standalone<invoke>with multiple arguments.test_find_tool_syntax_start_invoke: Verifies opener offsets.Verification
-DCMAKE_BUILD_TYPE=RelWithDebInfoon ROCm/HIP.piagent.