Skip to content

Commit af5c138

Browse files
authored
common : fix tool call type detection for nullable and enum schemas (ggml-org#21327)
* common : fix tool call type detection for nullable and enum schemas * common, tests : fix grammar delegation for nullable/enum schemas and add tests Fix enum type inference to scan all enum values (not just index 0) so schemas like {"enum": [0, "celsius"]} correctly detect string type. Fix schema_delegates in peg-parser to handle nullable type arrays (["string", "null"]) and typeless enum schemas in raw mode, allowing the tagged parser to use raw text instead of JSON-formatted strings. Add test cases for Qwen3-Coder (TAG_WITH_TAGGED format): - nullable string ["string", "null"] - nullable string with null first ["null", "string"] - nullable integer ["integer", "null"] - enum without explicit type key
1 parent 277ff5f commit af5c138

3 files changed

Lines changed: 185 additions & 10 deletions

File tree

common/chat-auto-parser-generator.cpp

Lines changed: 55 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -400,12 +400,34 @@ common_peg_parser analyze_tools::build_tool_parser_tag_tagged(parser_build_conte
400400
for (const auto & [param_name, param_schema] : properties.items()) {
401401
bool is_required = required.find(param_name) != required.end();
402402
std::string type = "object";
403-
auto type_obj = param_schema.contains("type") ? param_schema.at("type") : json::object();
404-
if (type_obj.is_string()) {
405-
type_obj.get_to(type);
406-
} else if (type_obj.is_object()) {
407-
if (type_obj.contains("type") && type_obj.at("type").is_string()) {
408-
type_obj.at("type").get_to(type);
403+
if (param_schema.contains("type")) {
404+
const auto & type_obj = param_schema.at("type");
405+
if (type_obj.is_string()) {
406+
type_obj.get_to(type);
407+
} else if (type_obj.is_array()) {
408+
// Handle nullable types like ["string", "null"]
409+
for (const auto & t : type_obj) {
410+
if (t.is_string() && t.get<std::string>() != "null") {
411+
type = t.get<std::string>();
412+
break;
413+
}
414+
}
415+
} else if (type_obj.is_object()) {
416+
if (type_obj.contains("type") && type_obj.at("type").is_string()) {
417+
type_obj.at("type").get_to(type);
418+
}
419+
}
420+
}
421+
// Infer string type from enum values when type is unspecified
422+
if (type == "object" && param_schema.contains("enum")) {
423+
const auto & enum_vals = param_schema.at("enum");
424+
if (enum_vals.is_array()) {
425+
for (const auto & v : enum_vals) {
426+
if (v.is_string()) {
427+
type = "string";
428+
break;
429+
}
430+
}
409431
}
410432
}
411433

@@ -574,9 +596,33 @@ common_peg_parser analyze_tools::build_tool_parser_tag_gemma4_dict(parser_build_
574596
std::vector<arg_entry> arg_entries;
575597

576598
for (const auto & [param_name, param_schema] : properties.items()) {
577-
std::string type = "object";
578-
auto type_v = param_schema.contains("type") ? param_schema.at("type") : json::object();
579-
if (type_v.is_string()) type_v.get_to(type);
599+
std::string type = "object";
600+
if (param_schema.contains("type")) {
601+
const auto & type_v = param_schema.at("type");
602+
if (type_v.is_string()) {
603+
type_v.get_to(type);
604+
} else if (type_v.is_array()) {
605+
// Handle nullable types like ["string", "null"]
606+
for (const auto & t : type_v) {
607+
if (t.is_string() && t.get<std::string>() != "null") {
608+
type = t.get<std::string>();
609+
break;
610+
}
611+
}
612+
}
613+
}
614+
// Infer string type from enum values when type is unspecified
615+
if (type == "object" && param_schema.contains("enum")) {
616+
const auto & enum_vals = param_schema.at("enum");
617+
if (enum_vals.is_array()) {
618+
for (const auto & v : enum_vals) {
619+
if (v.is_string()) {
620+
type = "string";
621+
break;
622+
}
623+
}
624+
}
625+
}
580626

581627
common_peg_parser value_parser = p.eps();
582628
if (type == "string") {

common/peg-parser.cpp

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1561,7 +1561,23 @@ void common_peg_arena::build_grammar(const common_grammar_builder & builder, boo
15611561
if (!s.schema) {
15621562
return true;
15631563
}
1564-
if (s.raw && s.schema->contains("type") && s.schema->at("type").is_string() && s.schema->at("type") == "string") {
1564+
if (s.raw && s.schema->contains("type")) {
1565+
const auto & type_val = s.schema->at("type");
1566+
if (type_val.is_string() && type_val == "string") {
1567+
return true;
1568+
}
1569+
// Handle nullable types like ["string", "null"] - delegate when the
1570+
// non-null type is string, since the tagged format uses raw text
1571+
if (type_val.is_array()) {
1572+
for (const auto & t : type_val) {
1573+
if (t.is_string() && t.get<std::string>() != "null") {
1574+
return t.get<std::string>() == "string";
1575+
}
1576+
}
1577+
}
1578+
}
1579+
// Delegate for enum schemas in raw mode - enum values are literal strings
1580+
if (s.raw && !s.schema->contains("type") && s.schema->contains("enum")) {
15651581
return true;
15661582
}
15671583
return false;

tests/test-chat.cpp

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -657,6 +657,66 @@ static common_chat_tool imaginary_number_tool{
657657
})",
658658
};
659659

660+
static common_chat_tool nullable_string_tool{
661+
/* .name = */ "set_nullable_str",
662+
/* .description = */ "Set a nullable string value",
663+
/* .parameters = */ R"({
664+
"type": "object",
665+
"properties": {
666+
"name": {
667+
"type": ["string", "null"],
668+
"description": "A nullable string"
669+
}
670+
},
671+
"required": ["name"]
672+
})",
673+
};
674+
675+
static common_chat_tool nullable_string_null_first_tool{
676+
/* .name = */ "set_nullable_str_nf",
677+
/* .description = */ "Set a nullable string value with null first in type array",
678+
/* .parameters = */ R"({
679+
"type": "object",
680+
"properties": {
681+
"name": {
682+
"type": ["null", "string"],
683+
"description": "A nullable string with null first"
684+
}
685+
},
686+
"required": ["name"]
687+
})",
688+
};
689+
690+
static common_chat_tool nullable_int_tool{
691+
/* .name = */ "set_nullable_int",
692+
/* .description = */ "Set a nullable integer value",
693+
/* .parameters = */ R"({
694+
"type": "object",
695+
"properties": {
696+
"count": {
697+
"type": ["integer", "null"],
698+
"description": "A nullable integer"
699+
}
700+
},
701+
"required": ["count"]
702+
})",
703+
};
704+
705+
static common_chat_tool enum_no_type_tool{
706+
/* .name = */ "set_unit",
707+
/* .description = */ "Set a temperature unit",
708+
/* .parameters = */ R"({
709+
"type": "object",
710+
"properties": {
711+
"unit": {
712+
"enum": ["celsius", "fahrenheit"],
713+
"description": "Temperature unit"
714+
}
715+
},
716+
"required": ["unit"]
717+
})",
718+
};
719+
660720
static common_chat_tool string_param_tool{
661721
/* .name = */ "string_param",
662722
/* .description = */ "Tool with string parameter for testing",
@@ -2200,6 +2260,7 @@ static void test_template_output_peg_parsers(bool detailed_debug) {
22002260
}
22012261
})
22022262
.run();
2263+
22032264
}
22042265

22052266
{
@@ -2383,6 +2444,58 @@ static void test_template_output_peg_parsers(bool detailed_debug) {
23832444
})
23842445
.expect_reconstruction()
23852446
.run();
2447+
2448+
// nullable string type ["string", "null"]
2449+
tst.test(
2450+
"<tool_call>\n"
2451+
"<function=set_nullable_str>\n"
2452+
"<parameter=name>\nhello world\n</parameter>\n"
2453+
"</function>\n"
2454+
"</tool_call>")
2455+
.tools({ nullable_string_tool })
2456+
.expect_tool_calls({
2457+
{ "set_nullable_str", R"({"name": "hello world"})", {} },
2458+
})
2459+
.run();
2460+
2461+
// nullable string with null first in type array ["null", "string"]
2462+
tst.test(
2463+
"<tool_call>\n"
2464+
"<function=set_nullable_str_nf>\n"
2465+
"<parameter=name>\nhello world\n</parameter>\n"
2466+
"</function>\n"
2467+
"</tool_call>")
2468+
.tools({ nullable_string_null_first_tool })
2469+
.expect_tool_calls({
2470+
{ "set_nullable_str_nf", R"({"name": "hello world"})", {} },
2471+
})
2472+
.run();
2473+
2474+
// nullable integer type ["integer", "null"] - should use JSON value path, not string
2475+
tst.test(
2476+
"<tool_call>\n"
2477+
"<function=set_nullable_int>\n"
2478+
"<parameter=count>\n42\n</parameter>\n"
2479+
"</function>\n"
2480+
"</tool_call>")
2481+
.tools({ nullable_int_tool })
2482+
.expect_tool_calls({
2483+
{ "set_nullable_int", R"({"count": 42})", {} },
2484+
})
2485+
.run();
2486+
2487+
// enum without explicit type key - should infer string from enum values
2488+
tst.test(
2489+
"<tool_call>\n"
2490+
"<function=set_unit>\n"
2491+
"<parameter=unit>\ncelsius\n</parameter>\n"
2492+
"</function>\n"
2493+
"</tool_call>")
2494+
.tools({ enum_no_type_tool })
2495+
.expect_tool_calls({
2496+
{ "set_unit", R"({"unit": "celsius"})", {} },
2497+
})
2498+
.run();
23862499
}
23872500
{
23882501
auto tst = peg_tester("models/templates/deepseek-ai-DeepSeek-V3.1.jinja", detailed_debug);

0 commit comments

Comments
 (0)