Fixed tool calling with Groq and OpenRouter (#1003)

app/Tests/Provider/ToolCalling/OpenAIStrictToolSchemaTests.cs

@@ -0,0 +1,107 @@
+using System.Text.Json.Nodes;
+
+using AIStudio.Provider.OpenAI;
+using AIStudio.Tools.ToolCallingSystem;
+
+namespace AIStudio.Tests.Provider.ToolCalling;
+
+/// <summary>
+/// Checks how a tool's parameter schema is translated into the form OpenAI's strict mode requires.
+/// </summary>
+/// <remarks>
+/// Strict mode wants every argument required and says "may be left out" by allowing null instead.
+/// The translation has to keep what the schema means: an argument the tool can do without stays
+/// one the model can leave empty, and an argument the tool needs stays exactly as it was.
+/// </remarks>
+[TestFixture]
+public sealed class OpenAIStrictToolSchemaTests
+{
+    [Test]
+    public void AnOptionalArgumentMayBeNullInstead()
+    {
+        var properties = Converted(ToolParameterSchemaBuilder.Create()
+            .RequiredString("query", "The search query.")
+            .OptionalString("language", "The language.")
+            .OptionalInteger("page", "The page."))["properties"]!;
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(Types(properties["language"]!), Is.EqualTo(new[] { "string", "null" }));
+            Assert.That(Types(properties["page"]!), Is.EqualTo(new[] { "integer", "null" }));
+        });
+    }
+
+    [Test]
+    public void AnOptionalChoiceOffersNullAmongItsValues()
+    {
+        var timeRange = Converted(ToolParameterSchemaBuilder.Create()
+            .RequiredString("query", "The search query.")
+            .OptionalEnum("time_range", "The time range.", "day", "month", "year"))["properties"]!["time_range"]!;
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(Types(timeRange), Is.EqualTo(new[] { "string", "null" }));
+            Assert.That(timeRange["enum"]!.AsArray().Select(value => value?.GetValue<string>()), Is.EqualTo(new[] { null, "day", "month", "year" }));
+        });
+    }
+
+    [Test]
+    public void ARequiredArgumentStaysAsItIs()
+    {
+        var query = Converted(ToolParameterSchemaBuilder.Create()
+            .RequiredString("query", "The search query.")
+            .OptionalString("language", "The language."))["properties"]!["query"]!;
+
+        Assert.That(query["type"]!.GetValue<string>(), Is.EqualTo("string"));
+    }
+
+    [Test]
+    public void EveryArgumentIsRequiredInTheOrderOfTheProperties()
+    {
+        //
+        // The order has to be stable across requests, because prompt caching depends on it. The
+        // optional argument comes first here, so an order taken from the old required list would
+        // show.
+        //
+        var schema = Converted(ToolParameterSchemaBuilder.Create()
+            .OptionalString("language", "The language.")
+            .RequiredString("query", "The search query."));
+
+        Assert.That(schema["required"]!.AsArray().Select(name => name!.GetValue<string>()), Is.EqualTo(new[] { "language", "query" }));
+    }
+
+    [Test]
+    public void ExtraArgumentsStayRefused()
+    {
+        var schema = Converted(ToolParameterSchemaBuilder.Create()
+            .RequiredString("query", "The search query.")
+            .OptionalString("language", "The language."));
+
+        Assert.That(schema["additionalProperties"]!.GetValue<bool>(), Is.False);
+    }
+
+    [Test]
+    public void ASchemaWithoutOptionalArgumentsIsLeftUntouched()
+    {
+        var parameters = ToolParameterSchemaBuilder.Create()
+            .RequiredString("url", "The address of the page.")
+            .Build();
+
+        var converted = OpenAIStrictToolSchema.FromToolParameters(parameters);
+        Assert.That(JsonNode.DeepEquals(JsonNode.Parse(converted.GetRawText()), JsonNode.Parse(parameters.GetRawText())), Is.True);
+    }
+
+    /// <summary>
+    /// Builds the schema and returns it the way strict mode receives it.
+    /// </summary>
+    private static JsonNode Converted(ToolParameterSchemaBuilder builder) => JsonNode.Parse(OpenAIStrictToolSchema.FromToolParameters(builder.Build()).GetRawText())!;
+
+    /// <summary>
+    /// Reads the types a property allows, whether it names one or several.
+    /// </summary>
+    private static string[] Types(JsonNode property) => property["type"] switch
+    {
+        JsonArray types => types.Select(type => type!.GetValue<string>()).ToArray(),
+        var type => [type!.GetValue<string>()],
+    };
+}

app/Tests/Provider/ToolCalling/ProviderToolAdaptersTests.cs

@@ -0,0 +1,85 @@
+using System.Text.Json;
+using System.Text.Json.Nodes;
+
+using AIStudio.Provider;
+using AIStudio.Tools.ToolCallingSystem;
+
+namespace AIStudio.Tests.Provider.ToolCalling;
+
+/// <summary>
+/// Checks when a tool reaches a Chat Completions host in strict mode, and when as written.
+/// </summary>
+/// <remarks>
+/// Strict mode only holds where the host binds the model's calls to the schema. Anywhere else, the
+/// model just reads a schema which calls every argument required: Groq then rejects each call that
+/// leaves one out, and other models fill the gap with a placeholder the tool has to refuse. Such a
+/// host therefore gets the schema the way the tool wrote it.
+/// </remarks>
+[TestFixture]
+public sealed class ProviderToolAdaptersTests
+{
+    [Test]
+    public void AHostWhichDoesNotEnforceStrictModeGetsTheSchemaAsWritten()
+    {
+        var definition = WebSearchLike();
+        var function = Sent(definition, hostEnforcesStrict: false);
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(function["strict"]!.GetValue<bool>(), Is.False);
+            Assert.That(JsonNode.DeepEquals(function["parameters"], JsonNode.Parse(definition.Function.Parameters.GetRawText())), Is.True);
+        });
+    }
+
+    [Test]
+    public void AHostWhichEnforcesStrictModeGetsTheConvertedSchema()
+    {
+        var function = Sent(WebSearchLike(), hostEnforcesStrict: true);
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(function["strict"]!.GetValue<bool>(), Is.True);
+            Assert.That(function["parameters"]!["required"]!.AsArray().Select(name => name!.GetValue<string>()), Is.EqualTo(new[] { "query", "page" }));
+        });
+    }
+
+    [Test]
+    public void AToolWhichOptsOutStaysOutOfStrictModeEverywhere()
+    {
+        var definition = WebSearchLike(isStrict: false);
+        var function = Sent(definition, hostEnforcesStrict: true);
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(function["strict"]!.GetValue<bool>(), Is.False);
+            Assert.That(JsonNode.DeepEquals(function["parameters"], JsonNode.Parse(definition.Function.Parameters.GetRawText())), Is.True);
+        });
+    }
+
+    /// <summary>
+    /// A tool with one required and one optional argument, the shape the web search has.
+    /// </summary>
+    private static ToolDefinition WebSearchLike(bool isStrict = true) => new()
+    {
+        Id = "web_search",
+        Function = new()
+        {
+            Name = "web_search",
+            DescriptionForLLM = "Search the web.",
+            Strict = isStrict,
+            Parameters = ToolParameterSchemaBuilder.Create()
+                .RequiredString("query", "The search query.")
+                .OptionalInteger("page", "The page, starting at 1.")
+                .Build(),
+        },
+    };
+
+    /// <summary>
+    /// Returns the function object of the tool as it goes over the wire.
+    /// </summary>
+    private static JsonNode Sent(ToolDefinition definition, bool hostEnforcesStrict)
+    {
+        var tool = ProviderToolAdapters.ToChatCompletionTool(definition, hostEnforcesStrict);
+        return JsonNode.Parse(JsonSerializer.Serialize(tool, ProviderJsonOptions.OPTIONS))!["function"]!;
+    }
+}

app/Tests/Tools/ToolCalling/WebSearchToolArgumentTests.cs

@@ -0,0 +1,170 @@
+using System.Text.Json;
+
+using AIStudio.Tools.ToolCallingSystem.ToolCallingImplementations.WebSearch;
+
+namespace AIStudio.Tests.Tools.ToolCalling;
+
+/// <summary>
+/// Checks how the web search reads the arguments a model passes, and what it says about wrong ones.
+/// </summary>
+/// <remarks>
+/// A wrong argument is refused rather than guessed at: a placeholder such as 0 is not a page, and
+/// quietly reading it as "no page" would do something the model did not ask for. What makes the
+/// refusal work is its message. The model reads it and tries again, so it has to say what arrived,
+/// what would have been right, and that an optional argument may simply be left out. A model
+/// which believes it has to pass one otherwise keeps trying placeholders, and every attempt costs
+/// one of the tool calls an answer may make.
+/// </remarks>
+[TestFixture]
+public sealed class WebSearchToolArgumentTests
+{
+    [Test]
+    public void AnArgumentLeftOutIsNotSet()
+    {
+        var arguments = Arguments("""{"query":"weather"}""");
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(WebSearchTool.ReadLanguage(arguments), Is.Null);
+            Assert.That(WebSearchTool.ReadTimeRange(arguments), Is.Null);
+            Assert.That(WebSearchTool.ReadPage(arguments), Is.Null);
+            Assert.That(WebSearchTool.ReadLimit(arguments), Is.Null);
+        });
+    }
+
+    [Test]
+    public void ANullArgumentIsTheSameAsOneLeftOut()
+    {
+        var arguments = Arguments("""{"query":"weather","language":null,"time_range":null,"page":null,"limit":null}""");
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(WebSearchTool.ReadLanguage(arguments), Is.Null);
+            Assert.That(WebSearchTool.ReadTimeRange(arguments), Is.Null);
+            Assert.That(WebSearchTool.ReadPage(arguments), Is.Null);
+            Assert.That(WebSearchTool.ReadLimit(arguments), Is.Null);
+        });
+    }
+
+    [Test]
+    public void ValidArgumentsComeThrough()
+    {
+        var arguments = Arguments("""{"query":"  weather  ","language":"de-DE","time_range":"day","page":2,"limit":7}""");
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(WebSearchTool.ReadQuery(arguments), Is.EqualTo("weather"));
+            Assert.That(WebSearchTool.ReadLanguage(arguments), Is.EqualTo("de-DE"));
+            Assert.That(WebSearchTool.ReadTimeRange(arguments), Is.EqualTo("day"));
+            Assert.That(WebSearchTool.ReadPage(arguments), Is.EqualTo(2));
+            Assert.That(WebSearchTool.ReadLimit(arguments), Is.EqualTo(7));
+        });
+    }
+
+    [TestCase("0")]
+    [TestCase("-1")]
+    [TestCase("2.5")]
+    [TestCase("\"5\"")]
+    [TestCase("true")]
+    public void AWrongPageIsRefusedWithWhatArrived(string value)
+    {
+        var message = Refusal(() => WebSearchTool.ReadPage(Arguments($$"""{"query":"weather","page":{{value}}}""")));
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(message, Does.Contain("'page'").And.Contain("a positive integer"));
+            Assert.That(message, Does.Contain($"but was {value}."), "Without the value, the model cannot tell which of its arguments the tool means.");
+            Assert.That(message, Does.Contain("Leave it out"), "The way out a model needs when it believes the argument is required.");
+        });
+    }
+
+    [TestCase("0")]
+    [TestCase("-1")]
+    [TestCase("\"5\"")]
+    public void AWrongLimitIsRefusedWithWhatArrived(string value)
+    {
+        var message = Refusal(() => WebSearchTool.ReadLimit(Arguments($$"""{"query":"weather","limit":{{value}}}""")));
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(message, Does.Contain("'limit'").And.Contain("a positive integer"));
+            Assert.That(message, Does.Contain($"but was {value}."));
+            Assert.That(message, Does.Contain("Leave it out"));
+        });
+    }
+
+    [TestCase("day")]
+    [TestCase("week")]
+    [TestCase("month")]
+    [TestCase("year")]
+    public void EveryOfferedTimeRangeIsAccepted(string timeRange)
+    {
+        //
+        // The week is the one a model asks for when the user says "this week". Both services with
+        // a time filter understand it, and refusing it only made the model try it again and again.
+        //
+        Assert.That(WebSearchTool.ReadTimeRange(Arguments($$"""{"query":"weather","time_range":"{{timeRange}}"}""")), Is.EqualTo(timeRange));
+    }
+
+    [TestCase("\"\"")]
+    [TestCase("\"Day\"")]
+    [TestCase("\"decade\"")]
+    [TestCase("5")]
+    public void AWrongTimeRangeIsRefusedWithTheValuesThatWouldDo(string value)
+    {
+        var message = Refusal(() => WebSearchTool.ReadTimeRange(Arguments($$"""{"query":"weather","time_range":{{value}}}""")));
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(message, Does.Contain("'time_range'").And.Contain("one of day, week, month, year"));
+            Assert.That(message, Does.Contain($"but was {value}."));
+            Assert.That(message, Does.Contain("Leave it out"));
+        });
+    }
+
+    [Test]
+    public void ALanguageWhichIsNoStringIsRefused()
+    {
+        var message = Refusal(() => WebSearchTool.ReadLanguage(Arguments("""{"query":"weather","language":5}""")));
+        Assert.That(message, Does.Contain("'language'").And.Contain("but was 5.").And.Contain("Leave it out"));
+    }
+
+    [Test]
+    public void AMissingQueryIsRefused()
+    {
+        var message = Refusal(() => WebSearchTool.ReadQuery(Arguments("""{"page":1}""")));
+        Assert.That(message, Does.Contain("Missing required argument 'query'"));
+    }
+
+    [Test]
+    public void AQueryWhichIsNoStringIsRefusedWithoutOfferingToLeaveItOut()
+    {
+        var message = Refusal(() => WebSearchTool.ReadQuery(Arguments("""{"query":42}""")));
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(message, Does.Contain("'query'").And.Contain("but was 42."));
+            Assert.That(message, Does.Not.Contain("Leave it out"), "The query is required, so leaving it out is no way out.");
+        });
+    }
+
+    [Test]
+    public void ALongValueIsShortenedInTheMessage()
+    {
+        var longValue = new string('x', 500);
+        var message = Refusal(() => WebSearchTool.ReadTimeRange(Arguments($$"""{"query":"weather","time_range":"{{longValue}}"}""")));
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(message, Does.Not.Contain(longValue), "The model sent the value itself; repeating all of it back only costs tokens.");
+            Assert.That(message, Does.Contain("..."));
+        });
+    }
+
+    private static JsonElement Arguments(string json) => JsonSerializer.Deserialize<JsonElement>(json);
+
+    /// <summary>
+    /// Runs a reader which has to refuse its argument and returns what it said.
+    /// </summary>
+    private static string Refusal(TestDelegate read) => Assert.Throws<ArgumentException>(read)!.Message;
+}