Skip to content

fix(frontend): keep the last value on a duplicate tool-call parameter #299

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
adubkov wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(frontend): keep the last value on a duplicate tool-call parameter #299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions include/ninfer/types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
34 changes: 23 additions & 11 deletions src/models/qwen3_5/frontend/tool_call_parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,11 @@ class QwenToolRegionParser {
const Contract& contract)
: text_(text), max_name_length_(max_name_length), contract_(contract) {}

FallbackReason parse(std::vector<RawToolCall>& calls) const {
[[nodiscard]] std::uint32_t duplicate_parameters_repaired() const noexcept {
return duplicate_parameters_repaired_;
}

FallbackReason parse(std::vector<RawToolCall>& calls) {
std::size_t pos = 0;
for (;;) {
skip_format_whitespace(text_, pos);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -473,26 +477,32 @@ 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);
if (name_end == std::string_view::npos || name_end == name_begin) {
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;
}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -607,7 +618,7 @@ ParsedToolCallOutput parse_qwen_tool_call_output(const std::string& text,

std::vector<RawToolCall> 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;
Expand All @@ -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<std::uint32_t>(out.tool_calls.size());
out.is_tool_call_response = true;
return out;
Expand Down
15 changes: 14 additions & 1 deletion src/serve/operational_log.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,10 +298,23 @@ std::optional<OperationalRecord> 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("<tool_call>");
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),
};
}

Expand Down
1 change: 1 addition & 0 deletions src/serve/request_log.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)}};
}
Expand Down
28 changes: 23 additions & 5 deletions tests/test_tool_call_parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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();
Expand Down