From 639bacac2e4a4cbc7d3ed2509bdd7617343d2ccb Mon Sep 17 00:00:00 2001 From: lusoris Date: Thu, 3 Sep 2026 09:19:34 +0200 Subject: [PATCH] Fix qwen3coder tool parser keeping quotes in function and parameter names JIRA/Issue: #4487 The qwen3coder state machine takes the XML tag attribute verbatim between "". When the model emits the attribute quoted (), the quotes end up in the emitted JSON argument key ("\"command\"") and in the tool name, so OpenAI-schema validation on the client rejects the tool call. The same names are the lookup keys into toolsParametersTypeMap, so the quotes also silently disable boolean normalization and string enforcement for every parameter of that call. Add trimSurroundingQuotes() next to the existing tag-parser helpers in io_processing/utils and apply it to both extracted names. Only a clean wrapping pair is treated as a delimiter: the name is left byte-identical unless the same quote character is both its first and its last character and does not occur anywhere in between. Unpaired quotes are deliberately left alone. --- .../qwen3coder/qwen3coder_tool_parser.cpp | 4 + src/llm/io_processing/utils.cpp | 15 + src/llm/io_processing/utils.hpp | 6 + src/test/llm/io_processing_utils_test.cpp | 83 ++++++ .../qwen3coder_output_parser_test.cpp | 256 ++++++++++++++++++ 5 files changed, 364 insertions(+) diff --git a/src/llm/io_processing/qwen3coder/qwen3coder_tool_parser.cpp b/src/llm/io_processing/qwen3coder/qwen3coder_tool_parser.cpp index eb501855a8..f58a47ec6e 100644 --- a/src/llm/io_processing/qwen3coder/qwen3coder_tool_parser.cpp +++ b/src/llm/io_processing/qwen3coder/qwen3coder_tool_parser.cpp @@ -155,6 +155,8 @@ bool Qwen3CoderToolParserImpl::parseUntilStateChange(ToolCalls_t& toolCalls) { case State::InsideFunctionName: { DEFINE_TAG_POSITION_AND_BREAK_IF_NOT_FOUND(Qwen3CoderToolParser::XML_TAG_END); this->currentFunction.name = streamContent.substr(this->lastProcessedPosition, pos - this->lastProcessedPosition); + // Some models quote the tag attribute (); the quotes are delimiters, not part of the name. + trimSurroundingQuotes(this->currentFunction.name); this->lastProcessedPosition = pos + Qwen3CoderToolParser::XML_TAG_END.length(); this->currentState = State::InsideFunction; break; @@ -175,6 +177,8 @@ bool Qwen3CoderToolParserImpl::parseUntilStateChange(ToolCalls_t& toolCalls) { case State::InsideParameterName: { DEFINE_TAG_POSITION_AND_BREAK_IF_NOT_FOUND(Qwen3CoderToolParser::XML_TAG_END); this->currentParameterName = streamContent.substr(this->lastProcessedPosition, pos - this->lastProcessedPosition); + // Some models quote the tag attribute (); the quotes are delimiters, not part of the name. + trimSurroundingQuotes(this->currentParameterName); this->lastProcessedPosition = pos + Qwen3CoderToolParser::XML_TAG_END.length(); this->currentState = State::InsideParameter; break; diff --git a/src/llm/io_processing/utils.cpp b/src/llm/io_processing/utils.cpp index 2100710a27..32a2ad5b38 100644 --- a/src/llm/io_processing/utils.cpp +++ b/src/llm/io_processing/utils.cpp @@ -135,6 +135,21 @@ void trimNewline(std::string& str) { } } +void trimSurroundingQuotes(std::string& str) { + if (str.size() < 2) { + return; + } + const char quote = str.front(); + if ((quote != '"' && quote != '\'') || str.back() != quote) { + return; + } + // A quote in the middle means the character belongs to the value, not to a delimiter pair. + if (str.find(quote, 1) != str.size() - 1) { + return; + } + str = str.substr(1, str.size() - 2); +} + const char* jsonTypeOf(const rapidjson::Value& val) { if (val.IsObject()) return "object"; diff --git a/src/llm/io_processing/utils.hpp b/src/llm/io_processing/utils.hpp index 4568ec0851..ec7d61bbe9 100644 --- a/src/llm/io_processing/utils.hpp +++ b/src/llm/io_processing/utils.hpp @@ -35,6 +35,12 @@ void writeArgumentOfAnyType(const rapidjson::Value& arg, rapidjson::Writer) already yields today. + std::string name = R"("")"; + trimSurroundingQuotes(name); + EXPECT_EQ(name, ""); +} diff --git a/src/test/llm/output_parsers/qwen3coder_output_parser_test.cpp b/src/test/llm/output_parsers/qwen3coder_output_parser_test.cpp index 89527747a2..ac6522bca7 100644 --- a/src/test/llm/output_parsers/qwen3coder_output_parser_test.cpp +++ b/src/test/llm/output_parsers/qwen3coder_output_parser_test.cpp @@ -556,6 +556,249 @@ value2 EXPECT_EQ(parser.getLastProcessedPosition(), input.find("") + std::string("").size()); EXPECT_EQ(content, "\n"); } +// Regression tests for https://github.com/openvinotoolkit/model_server/issues/4487: +// the model sometimes quotes the XML attribute (, ). +// The quotes are delimiters, so they must not end up in the tool name or in the argument +// keys, and the schema-driven type handling keyed on those names must keep working. +// The unquoted path is unchanged; TestJustParserImplUnaryToolCall above is its control. +TEST_F(Qwen3CoderOutputParserTest, TestJustParserImplQuotedParameterName) { + const std::string input = R"( + + + +value1 + + +)"; + auto content = input; + ovms::Qwen3CoderToolParserImpl parser(toolsParametersTypeMap); + auto callsOpt = parser.parseChunk(content); + ASSERT_TRUE(callsOpt.has_value()); + ToolCalls_t& calls = callsOpt.value(); + auto status = parser.removeToolCallsFromContentIfNeeded(content); + EXPECT_TRUE(status.ok()) << status.string(); + ASSERT_EQ(calls.size(), 1) << input; + EXPECT_EQ(calls[0].name, "string_tool"); + EXPECT_EQ(calls[0].arguments, "{\"arg1\":\"value1\"}"); + EXPECT_EQ(parser.getCurrentState(), ovms::Qwen3CoderToolParserImpl::State::Content) << input; + EXPECT_EQ(parser.getLastProcessedPosition(), input.find("") + std::string("").size()); + EXPECT_EQ(content, "\n"); +} +TEST_F(Qwen3CoderOutputParserTest, TestJustParserImplSingleQuotedParameterName) { + const std::string input = R"( + + + +value1 + + +)"; + auto content = input; + ovms::Qwen3CoderToolParserImpl parser(toolsParametersTypeMap); + auto callsOpt = parser.parseChunk(content); + ASSERT_TRUE(callsOpt.has_value()); + ToolCalls_t& calls = callsOpt.value(); + auto status = parser.removeToolCallsFromContentIfNeeded(content); + EXPECT_TRUE(status.ok()) << status.string(); + ASSERT_EQ(calls.size(), 1) << input; + EXPECT_EQ(calls[0].name, "string_tool"); + EXPECT_EQ(calls[0].arguments, "{\"arg1\":\"value1\"}"); + EXPECT_EQ(parser.getCurrentState(), ovms::Qwen3CoderToolParserImpl::State::Content) << input; + EXPECT_EQ(content, "\n"); +} +TEST_F(Qwen3CoderOutputParserTest, TestJustParserImplQuotedFunctionName) { + const std::string input = R"( + + + +value1 + + +)"; + auto content = input; + ovms::Qwen3CoderToolParserImpl parser(toolsParametersTypeMap); + auto callsOpt = parser.parseChunk(content); + ASSERT_TRUE(callsOpt.has_value()); + ToolCalls_t& calls = callsOpt.value(); + auto status = parser.removeToolCallsFromContentIfNeeded(content); + EXPECT_TRUE(status.ok()) << status.string(); + ASSERT_EQ(calls.size(), 1) << input; + EXPECT_EQ(calls[0].name, "string_tool"); + EXPECT_EQ(calls[0].arguments, "{\"arg1\":\"value1\"}"); + EXPECT_EQ(parser.getCurrentState(), ovms::Qwen3CoderToolParserImpl::State::Content) << input; + EXPECT_EQ(content, "\n"); +} +TEST_F(Qwen3CoderOutputParserTest, TestJustParserImplQuotedNamesKeepStringEnforcement) { + // Both names are looked up in toolsParametersTypeMap, so quotes left in place would also + // silently disable type handling: arg1 would stay a number instead of the declared string. + const std::string input = R"( + + + +42 + + +7 + + +)"; + auto content = input; + ovms::Qwen3CoderToolParserImpl parser(toolsParametersTypeMap); + auto callsOpt = parser.parseChunk(content); + ASSERT_TRUE(callsOpt.has_value()); + ToolCalls_t& calls = callsOpt.value(); + auto status = parser.removeToolCallsFromContentIfNeeded(content); + EXPECT_TRUE(status.ok()) << status.string(); + ASSERT_EQ(calls.size(), 1) << input; + EXPECT_EQ(calls[0].name, "string_int_tool"); + EXPECT_EQ(calls[0].arguments, "{\"arg1\":\"42\",\"arg2\":7}"); + EXPECT_EQ(content, "\n"); +} +TEST_F(Qwen3CoderOutputParserTest, TestJustParserImplQuotedParameterNameKeepsBooleanNormalization) { + const std::string input = R"( + + + +True + + +)"; + auto content = input; + ovms::Qwen3CoderToolParserImpl parser(toolsParametersTypeMap); + auto callsOpt = parser.parseChunk(content); + ASSERT_TRUE(callsOpt.has_value()); + ToolCalls_t& calls = callsOpt.value(); + auto status = parser.removeToolCallsFromContentIfNeeded(content); + EXPECT_TRUE(status.ok()) << status.string(); + ASSERT_EQ(calls.size(), 1) << input; + EXPECT_EQ(calls[0].name, "bool_tool"); + EXPECT_EQ(calls[0].arguments, "{\"arg1\":true}"); + EXPECT_EQ(content, "\n"); +} +TEST_F(Qwen3CoderOutputParserTest, TestJustParserImplMixedQuotingOfParameterNames) { + const std::string input = R"( + + + +value1 + + +42 + + +52.32 + + +)"; + auto content = input; + ovms::Qwen3CoderToolParserImpl parser(toolsParametersTypeMap); + auto callsOpt = parser.parseChunk(content); + ASSERT_TRUE(callsOpt.has_value()); + ToolCalls_t& calls = callsOpt.value(); + auto status = parser.removeToolCallsFromContentIfNeeded(content); + EXPECT_TRUE(status.ok()) << status.string(); + ASSERT_EQ(calls.size(), 1) << input; + EXPECT_EQ(calls[0].name, "string_int_float_tool"); + EXPECT_EQ(calls[0].arguments, "{\"arg1\":\"value1\",\"arg2\":42,\"arg3\":52.32}"); + EXPECT_EQ(parser.getCurrentState(), ovms::Qwen3CoderToolParserImpl::State::Content) << input; + EXPECT_EQ(content, "\n"); +} +TEST_F(Qwen3CoderOutputParserTest, TestJustParserImplTwoToolCallsWithQuotedNames) { + const std::string input = R"( + + + +value1 + + + + + + +data + + +25.2 + + +)"; + auto content = input; + ovms::Qwen3CoderToolParserImpl parser(toolsParametersTypeMap); + auto callsOpt = parser.parseChunk(content); + ASSERT_TRUE(callsOpt.has_value()); + ToolCalls_t& calls = callsOpt.value(); + auto status = parser.removeToolCallsFromContentIfNeeded(content); + EXPECT_TRUE(status.ok()) << status.string(); + ASSERT_EQ(calls.size(), 2) << input; + EXPECT_EQ(calls[0].name, "string_tool"); + EXPECT_EQ(calls[0].arguments, "{\"arg1\":\"value1\"}"); + EXPECT_EQ(calls[1].name, "string_float_tool"); + EXPECT_EQ(calls[1].arguments, "{\"arg1\":\"data\",\"arg2\":25.2}"); + EXPECT_EQ(parser.getCurrentState(), ovms::Qwen3CoderToolParserImpl::State::Content) << input; + EXPECT_EQ(parser.getLastProcessedPosition(), input.rfind("") + std::string("").size()) << input; + EXPECT_EQ(content, "\n\n"); +} +TEST_F(Qwen3CoderOutputParserTest, TestJustParserImplParameterNameWithInnerQuoteKept) { + // Only a clean wrapping pair is a delimiter; a quote between the ends belongs to the name. + const std::string input = R"( + + + +value1 + + +)"; + auto content = input; + ovms::Qwen3CoderToolParserImpl parser(toolsParametersTypeMap); + auto callsOpt = parser.parseChunk(content); + ASSERT_TRUE(callsOpt.has_value()); + ToolCalls_t& calls = callsOpt.value(); + auto status = parser.removeToolCallsFromContentIfNeeded(content); + EXPECT_TRUE(status.ok()) << status.string(); + ASSERT_EQ(calls.size(), 1) << input; + EXPECT_EQ(calls[0].name, "string_tool"); + EXPECT_EQ(calls[0].arguments, "{\"arg\\\"1\":\"value1\"}"); + EXPECT_EQ(content, "\n"); +} +TEST_F(Qwen3CoderOutputParserTest, TestJustParserImplParameterNameWithUnmatchedQuoteKept) { + // Deliberately out of scope: a single unpaired quote is not a delimiter pair, and the + // parser cannot tell a dropped closing quote from a name that starts with one. + const std::string input = R"( + + + +value1 + + +)"; + auto content = input; + ovms::Qwen3CoderToolParserImpl parser(toolsParametersTypeMap); + auto callsOpt = parser.parseChunk(content); + ASSERT_TRUE(callsOpt.has_value()); + ToolCalls_t& calls = callsOpt.value(); + auto status = parser.removeToolCallsFromContentIfNeeded(content); + EXPECT_TRUE(status.ok()) << status.string(); + ASSERT_EQ(calls.size(), 1) << input; + EXPECT_EQ(calls[0].name, "string_tool"); + EXPECT_EQ(calls[0].arguments, "{\"\\\"arg1\":\"value1\"}"); + EXPECT_EQ(content, "\n"); +} +TEST_F(Qwen3CoderOutputParserTest, TestJustParserImplStreamStepWithQuotedFunctionName) { + // getCurrentFunctionName() feeds the first streamed delta, so it must already be clean. + ovms::Qwen3CoderToolParserImpl parser(toolsParametersTypeMap); + const std::string input = R"( + + + +value1 +)"; + auto content = input; + auto stepResult = parser.parseChunk(content); + ASSERT_FALSE(stepResult.has_value()); + ASSERT_EQ(parser.getCurrentState(), ovms::Qwen3CoderToolParserImpl::State::InsideParameter); + ASSERT_TRUE(parser.getCurrentFunctionName().has_value()); + EXPECT_EQ(parser.getCurrentFunctionName().value(), "string_tool"); +} TEST_F(Qwen3CoderOutputParserTest, TestJustParserImplStreamStepWithMoreThan1StateChange) { ToolCalls_t calls; ovms::Qwen3CoderToolParserImpl parser(toolsParametersTypeMap); @@ -716,6 +959,19 @@ INSTANTIATE_TEST_SUITE_P( return name; }); +TEST_F(Qwen3CoderOutputParserTest, StreamingQuotedFunctionAndParameterNames) { + // End-to-end streaming counterpart of the parser-level quoted-attribute tests: the quotes + // must not leak into the first delta (function name) nor into the argument keys. + auto nameDelta = outputParser->parseChunk(R"()", {}, true, ov::genai::GenerationFinishReason::NONE); + ASSERT_TRUE(nameDelta.has_value()); + const std::string nameJson = ovms::test::deltaToJson(*nameDelta); + EXPECT_NE(nameJson.find(R"("name":"string_tool")"), std::string::npos) << nameJson; + + auto argumentsDelta = outputParser->parseChunk("value1", {}, true, ov::genai::GenerationFinishReason::NONE); + ASSERT_TRUE(argumentsDelta.has_value()); + const std::string argumentsJson = ovms::test::deltaToJson(*argumentsDelta); + EXPECT_NE(argumentsJson.find(R"("arguments":"{\"arg1\":\"value1\"}")"), std::string::npos) << argumentsJson; +} TEST_F(Qwen3CoderOutputParserTest, StreamingSimpleToolCall) { // since unary reuses streaming we don't need to test for partial tool calls // if we don't get closing tag we don't emit tool call