From d3a44d2166a0e47d04e0014bd17e208927ec5e0f Mon Sep 17 00:00:00 2001 From: adubkov Date: Mon, 21 Sep 2026 04:06:23 -0700 Subject: [PATCH] Keep last value on duplicate tool-call parameter A repeated made parse_parameter return DuplicateParameter, so the decoder restored the whole tool call verbatim as assistant content. An OpenAI-compatible client sees prose with finish_reason "stop" and cannot tell it apart from the model choosing not to call a tool, so the call is silently dropped. Keep the last occurrence instead, as JSON object syntax would, and count the repair in ToolCallParseDiagnostics so it stays observable. Also log the offending markup with the fallback reason: "malformed structure" names the verdict but not the bytes that earned it, which makes the remaining fallbacks undiagnosable after the request is gone. Measured on a local deployment serving a Qwen3.5 artifact: 220 requests with a parsed tool call, 11 fallbacks to text, 3 of them duplicate parameter. --- include/ninfer/types.h | 11 +++--- .../qwen3_5/frontend/tool_call_parser.cpp | 34 +++++++++++++------ src/serve/operational_log.cpp | 15 +++++++- src/serve/request_log.cpp | 1 + tests/test_tool_call_parser.cpp | 28 ++++++++++++--- 5 files changed, 67 insertions(+), 22 deletions(-) diff --git a/include/ninfer/types.h b/include/ninfer/types.h index a2e8b75481..b7294229f2 100644 --- a/include/ninfer/types.h +++ b/include/ninfer/types.h @@ -328,11 +328,12 @@ tool_call_parse_fallback_reason_name(ToolCallParseFallbackReason reason) noexcep } struct ToolCallParseDiagnostics { - bool marker_seen = false; - std::uint32_t structured_call_count = 0; - std::uint32_t empty_arguments_omitted = 0; - std::uint32_t schema_mismatch_arguments = 0; - ToolCallParseFallbackReason fallback_reason = ToolCallParseFallbackReason::None; + bool marker_seen = false; + std::uint32_t structured_call_count = 0; + std::uint32_t empty_arguments_omitted = 0; + std::uint32_t schema_mismatch_arguments = 0; + std::uint32_t duplicate_parameters_repaired = 0; + ToolCallParseFallbackReason fallback_reason = ToolCallParseFallbackReason::None; [[nodiscard]] friend constexpr bool operator==(const ToolCallParseDiagnostics&, const ToolCallParseDiagnostics&) noexcept = default; diff --git a/src/models/qwen3_5/frontend/tool_call_parser.cpp b/src/models/qwen3_5/frontend/tool_call_parser.cpp index 124281d417..a1826ccefc 100644 --- a/src/models/qwen3_5/frontend/tool_call_parser.cpp +++ b/src/models/qwen3_5/frontend/tool_call_parser.cpp @@ -413,7 +413,11 @@ class QwenToolRegionParser { const Contract& contract) : text_(text), max_name_length_(max_name_length), contract_(contract) {} - FallbackReason parse(std::vector& calls) const { + [[nodiscard]] std::uint32_t duplicate_parameters_repaired() const noexcept { + return duplicate_parameters_repaired_; + } + + FallbackReason parse(std::vector& calls) { std::size_t pos = 0; for (;;) { skip_format_whitespace(text_, pos); @@ -439,7 +443,7 @@ class QwenToolRegionParser { return true; } - FallbackReason parse_tool_call(std::size_t& pos, RawToolCall& call) const { + FallbackReason parse_tool_call(std::size_t& pos, RawToolCall& call) { if (!consume(pos, kToolOpen)) { return FallbackReason::MalformedStructure; } skip_format_whitespace(text_, pos); const FallbackReason failure = parse_function(pos, call); @@ -448,7 +452,7 @@ class QwenToolRegionParser { return consume(pos, kToolClose) ? FallbackReason::None : FallbackReason::MalformedStructure; } - FallbackReason parse_function(std::size_t& pos, RawToolCall& call) const { + FallbackReason parse_function(std::size_t& pos, RawToolCall& call) { if (!consume(pos, kFunctionOpen)) { return FallbackReason::MalformedStructure; } const std::size_t name_begin = pos; const std::size_t name_end = text_.find('>', name_begin); @@ -473,7 +477,7 @@ class QwenToolRegionParser { } } - FallbackReason parse_parameter(std::size_t& pos, RawToolCall& call) const { + FallbackReason parse_parameter(std::size_t& pos, RawToolCall& call) { if (!consume(pos, kParamOpen)) { return FallbackReason::MalformedStructure; } const std::size_t name_begin = pos; const std::size_t name_end = text_.find('>', name_begin); @@ -481,18 +485,24 @@ class QwenToolRegionParser { return FallbackReason::MalformedStructure; } const std::string_view name = text_.substr(name_begin, name_end - name_begin); - if (std::any_of(call.parameters.begin(), call.parameters.end(), - [&](const RawParameter& existing) { return existing.name == name; })) { - return FallbackReason::DuplicateParameter; - } + const auto existing = std::find_if(call.parameters.begin(), call.parameters.end(), + [&](const RawParameter& p) { return p.name == name; }); const std::size_t value_begin = name_end + 1; std::size_t value_end = 0; if (!find_parameter_close(value_begin, value_end)) { return FallbackReason::MalformedStructure; } - call.parameters.push_back(RawParameter{ - .name = name, .value = text_.substr(value_begin, value_end - value_begin)}); + const std::string_view value = text_.substr(value_begin, value_end - value_begin); + + // Last occurrence wins, as it would in JSON object syntax, rather than discarding + // an otherwise well-formed call. + if (existing != call.parameters.end()) { + existing->value = value; + ++duplicate_parameters_repaired_; + } else { + call.parameters.push_back(RawParameter{.name = name, .value = value}); + } pos = value_end + kParamClose.size(); return FallbackReason::None; } @@ -538,6 +548,7 @@ class QwenToolRegionParser { std::string_view text_; std::size_t max_name_length_; const Contract& contract_; + std::uint32_t duplicate_parameters_repaired_ = 0; }; GeneratedToolCall normalize_raw_tool_call(const RawToolCall& raw, const Contract& contract, @@ -607,7 +618,7 @@ ParsedToolCallOutput parse_qwen_tool_call_output(const std::string& text, std::vector raw_calls; const std::string_view tool_region = std::string_view(text).substr(first); - const QwenToolRegionParser parser(tool_region, max_tool_name_length, contract); + QwenToolRegionParser parser(tool_region, max_tool_name_length, contract); const FallbackReason failure = parser.parse(raw_calls); if (failure != FallbackReason::None) { out.diagnostics.fallback_reason = failure; @@ -619,6 +630,7 @@ ParsedToolCallOutput parse_qwen_tool_call_output(const std::string& text, out.tool_calls.push_back(normalize_raw_tool_call(raw, contract, out.diagnostics)); } + out.diagnostics.duplicate_parameters_repaired = parser.duplicate_parameters_repaired(); out.diagnostics.structured_call_count = static_cast(out.tool_calls.size()); out.is_tool_call_response = true; return out; diff --git a/src/serve/operational_log.cpp b/src/serve/operational_log.cpp index 75b7d2655a..59b2816eba 100644 --- a/src/serve/operational_log.cpp +++ b/src/serve/operational_log.cpp @@ -298,10 +298,23 @@ std::optional render_tool_call_fallback(const RequestLogConte reason == ninfer::ToolCallParseFallbackReason::None) { return std::nullopt; } + // The reason alone names the verdict, not the markup that earned it. + constexpr std::size_t kMarkupSnippetBytes = 240; + std::string snippet; + if (const std::size_t marker = outcome.text.find(""); + marker != std::string::npos) { + snippet = outcome.text.substr(marker, kMarkupSnippetBytes); + if (outcome.text.size() - marker > kMarkupSnippetBytes) { snippet += "..."; } + for (char& byte : snippet) { + if (byte == '\n' || byte == '\r' || byte == '\t') { byte = ' '; } + } + } + return OperationalRecord{ .severity = OperationalSeverity::Warning, .message = "req#" + std::to_string(context.id) + " tool markup returned as text | " + - pretty_code(ninfer::tool_call_parse_fallback_reason_name(reason)), + pretty_code(ninfer::tool_call_parse_fallback_reason_name(reason)) + + (snippet.empty() ? std::string{} : " | " + snippet), }; } diff --git a/src/serve/request_log.cpp b/src/serve/request_log.cpp index 0b3ece5795..5b4124a553 100644 --- a/src/serve/request_log.cpp +++ b/src/serve/request_log.cpp @@ -89,6 +89,7 @@ Json tool_call_parse_json(const ninfer::ToolCallParseDiagnostics& diagnostics) { {"structured_call_count", diagnostics.structured_call_count}, {"empty_arguments_omitted", diagnostics.empty_arguments_omitted}, {"schema_mismatch_arguments", diagnostics.schema_mismatch_arguments}, + {"duplicate_parameters_repaired", diagnostics.duplicate_parameters_repaired}, {"fallback_reason", ninfer::tool_call_parse_fallback_reason_name(diagnostics.fallback_reason)}}; } diff --git a/tests/test_tool_call_parser.cpp b/tests/test_tool_call_parser.cpp index 06c47f9b40..ad6231edd5 100644 --- a/tests/test_tool_call_parser.cpp +++ b/tests/test_tool_call_parser.cpp @@ -563,11 +563,6 @@ int test_strict_structure_and_active_tool_set() { ninfer::ToolCallParseFallbackReason::MalformedStructure, "missing parameter close was repaired"); - const std::string duplicate = tool_call("configure", {{"value", "first"}, {"value", "second"}}); - failures += - check_rejected(duplicate, contract, ninfer::ToolCallParseFallbackReason::DuplicateParameter, - "duplicate parameter was silently overwritten"); - const std::string unknown_tool = tool_call("other", {{"value", "x"}}); failures += check_rejected(unknown_tool, contract, ninfer::ToolCallParseFallbackReason::UndeclaredTool, @@ -735,8 +730,31 @@ int test_incremental_embedded_parameter_markup() { } // namespace +int test_duplicate_parameter_keeps_last_value() { + int failures = 0; + const fi::ToolCallOutputContract contract = + contract_for("configure", Json{{"value", Json{{"type", "string"}}}}); + const std::string duplicate = tool_call("configure", {{"value", "first"}, {"value", "second"}}); + const auto parsed = fi::parse_qwen_tool_call_output(duplicate, 64, contract); + + failures += check(parsed.is_tool_call_response, "duplicate parameter still fell back to text"); + failures += check(parsed.content.empty(), "duplicate parameter left prose behind"); + failures += check(parsed.tool_calls.size() == 1, "duplicate parameter did not yield one call"); + if (parsed.tool_calls.size() == 1) { + failures += check(parsed.tool_calls.front().arguments_json == R"({"value":"second"})", + "duplicate parameter did not keep the last value"); + } + failures += check(parsed.diagnostics.fallback_reason == + ninfer::ToolCallParseFallbackReason::None, + "duplicate parameter still reported a fallback reason"); + failures += check(parsed.diagnostics.duplicate_parameters_repaired == 1, + "duplicate parameter repair was not recorded in diagnostics"); + return failures; +} + int main() { int failures = 0; + failures += test_duplicate_parameter_keeps_last_value(); failures += test_basic_legacy_parsing(); failures += test_multiple_calls(); failures += test_declared_strings_preserve_text();