Share the readers that refuse wrong tool arguments

app/MindWork AI Studio/Tools/ToolCallingSystem/ToolArgumentReader.cs

@@ -0,0 +1,191 @@
+using System.Diagnostics.CodeAnalysis;
+using System.Text.Json;
+
+namespace AIStudio.Tools.ToolCallingSystem;
+
+/// <summary>
+/// Reads the arguments a model passes to a tool, and refuses 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. The model reads
+/// the refusal and tries again, so every refusal says what arrived, what would have been right,
+/// and, for an optional argument, that leaving it out is always an option. A model which believes
+/// the argument has to be there otherwise keeps trying placeholders, and every attempt costs one of
+/// the tool calls an answer may make.<br/><br/>
+/// A null counts the same as leaving an argument out: with a strict schema, a model has to pass
+/// every argument and passes null for one it does not want to set.
+/// </remarks>
+internal static class ToolArgumentReader
+{
+    /// <summary>
+    /// How much of a wrongly passed argument a refusal repeats back to the model.
+    /// </summary>
+    /// <remarks>
+    /// Enough for a GUID in quotes. The model sent the value itself, so repeating all of it back
+    /// only costs tokens.
+    /// </remarks>
+    private const int MAX_ARGUMENT_ECHO_LENGTH = 40;
+
+    /// <summary>
+    /// Reads a string argument the model always has to pass.
+    /// </summary>
+    /// <param name="arguments">The arguments the model passed.</param>
+    /// <param name="propertyName">The argument.</param>
+    /// <returns>The value, trimmed and never empty.</returns>
+    /// <exception cref="ArgumentException">The argument is missing, no string, or empty.</exception>
+    public static string ReadRequiredString(JsonElement arguments, string propertyName)
+    {
+        if (!TryGetArgument(arguments, propertyName, out var value))
+            throw new ArgumentException($"Missing required argument '{propertyName}'.");
+
+        var text = ReadString(propertyName, value, whenLeftOut: null);
+        if (string.IsNullOrWhiteSpace(text))
+            throw InvalidArgument(propertyName, value, "a non-empty string", whenLeftOut: null);
+
+        return text;
+    }
+
+    /// <summary>
+    /// Reads an optional string argument.
+    /// </summary>
+    /// <param name="arguments">The arguments the model passed.</param>
+    /// <param name="propertyName">The argument.</param>
+    /// <param name="whenLeftOut">What happens without the argument, completing "Leave it out ...".</param>
+    /// <returns>The value, trimmed, or null when the model left the argument out.</returns>
+    /// <exception cref="ArgumentException">The argument is no string.</exception>
+    public static string? ReadOptionalString(JsonElement arguments, string propertyName, string whenLeftOut)
+    {
+        if (!TryGetArgument(arguments, propertyName, out var value))
+            return null;
+
+        return ReadString(propertyName, value, whenLeftOut);
+    }
+
+    /// <summary>
+    /// Reads an optional argument which has to be a positive integer.
+    /// </summary>
+    /// <param name="arguments">The arguments the model passed.</param>
+    /// <param name="propertyName">The argument.</param>
+    /// <param name="whenLeftOut">What happens without the argument, completing "Leave it out ...".</param>
+    /// <returns>The value, or null when the model left the argument out.</returns>
+    /// <exception cref="ArgumentException">The argument is no positive integer.</exception>
+    public static int? ReadOptionalPositiveInt(JsonElement arguments, string propertyName, string whenLeftOut)
+    {
+        if (!TryGetArgument(arguments, propertyName, out var value))
+            return null;
+
+        if (value.ValueKind is not JsonValueKind.Number || !value.TryGetInt32(out var intValue) || intValue <= 0)
+            throw InvalidArgument(propertyName, value, "a positive integer", whenLeftOut);
+
+        return intValue;
+    }
+
+    /// <summary>
+    /// Reads an optional argument which has to be one of the values the tool offers.
+    /// </summary>
+    /// <remarks>
+    /// The values are compared exactly, because the schema offers them exactly so.
+    /// </remarks>
+    /// <param name="arguments">The arguments the model passed.</param>
+    /// <param name="propertyName">The argument.</param>
+    /// <param name="allowedValues">The values the tool offers.</param>
+    /// <param name="whenLeftOut">What happens without the argument, completing "Leave it out ...".</param>
+    /// <returns>The value, or null when the model left the argument out.</returns>
+    /// <exception cref="ArgumentException">The argument is none of the offered values.</exception>
+    public static string? ReadOptionalChoice(JsonElement arguments, string propertyName, IReadOnlyCollection<string> allowedValues, string whenLeftOut)
+    {
+        if (!TryGetArgument(arguments, propertyName, out var value))
+            return null;
+
+        if (!TryReadChoice(value, allowedValues, out var choice))
+            throw InvalidArgument(propertyName, value, $"one of {string.Join(", ", allowedValues)}", whenLeftOut);
+
+        return choice;
+    }
+
+    /// <summary>
+    /// Reads an optional argument which has to be a list of values the tool offers.
+    /// </summary>
+    /// <remarks>
+    /// The values are compared exactly, because the schema offers them exactly so. An empty list is
+    /// refused rather than read as leaving the argument out: it asks for none of the values, and
+    /// what leaving it out does instead is for the refusal to say. A value the model names twice
+    /// counts once.
+    /// </remarks>
+    /// <param name="arguments">The arguments the model passed.</param>
+    /// <param name="propertyName">The argument.</param>
+    /// <param name="allowedValues">The values the tool offers.</param>
+    /// <param name="whenLeftOut">What happens without the argument, completing "Leave it out ...".</param>
+    /// <returns>The values in the order the model named them, or null when it left the argument out.</returns>
+    /// <exception cref="ArgumentException">The argument is no list, an empty one, or holds a value the tool does not offer.</exception>
+    public static IReadOnlyList<string>? ReadOptionalChoices(JsonElement arguments, string propertyName, IReadOnlyCollection<string> allowedValues, string whenLeftOut)
+    {
+        if (!TryGetArgument(arguments, propertyName, out var value))
+            return null;
+
+        var offeredValues = string.Join(", ", allowedValues);
+        if (value.ValueKind is not JsonValueKind.Array || value.GetArrayLength() == 0)
+            throw InvalidArgument(propertyName, value, $"a list of one or more of {offeredValues}", whenLeftOut);
+
+        var choices = new List<string>(value.GetArrayLength());
+        foreach (var item in value.EnumerateArray())
+        {
+            if (!TryReadChoice(item, allowedValues, out var choice))
+                throw InvalidListValue(propertyName, item, $"one of {offeredValues}", whenLeftOut);
+
+            if (!choices.Contains(choice, StringComparer.Ordinal))
+                choices.Add(choice);
+        }
+
+        return choices;
+    }
+
+    /// <summary>
+    /// Looks up an argument, treating null the same as leaving it out.
+    /// </summary>
+    private static bool TryGetArgument(JsonElement arguments, string propertyName, out JsonElement value) =>
+        arguments.TryGetProperty(propertyName, out value) && value.ValueKind is not JsonValueKind.Null;
+
+    private static string ReadString(string propertyName, JsonElement value, string? whenLeftOut)
+    {
+        if (value.ValueKind is not JsonValueKind.String)
+            throw InvalidArgument(propertyName, value, "a string", whenLeftOut);
+
+        return value.GetString()?.Trim() ?? string.Empty;
+    }
+
+    private static bool TryReadChoice(JsonElement value, IReadOnlyCollection<string> allowedValues, [NotNullWhen(true)] out string? choice)
+    {
+        choice = value.ValueKind is JsonValueKind.String ? value.GetString()?.Trim() : null;
+        return choice is not null && allowedValues.Contains(choice, StringComparer.Ordinal);
+    }
+
+    /// <summary>
+    /// Builds the refusal of an argument the model passed wrongly.
+    /// </summary>
+    /// <param name="propertyName">The argument.</param>
+    /// <param name="value">What the model passed, as it arrived.</param>
+    /// <param name="expectation">What the argument must be, completing "must be ...".</param>
+    /// <param name="whenLeftOut">What happens without the argument, completing "Leave it out ...", or null for a required one.</param>
+    private static ArgumentException InvalidArgument(string propertyName, JsonElement value, string expectation, string? whenLeftOut) =>
+        Refusal($"Argument '{propertyName}' must be {expectation}, but was {Echo(value)}.", whenLeftOut);
+
+    /// <summary>
+    /// Builds the refusal of a list the model passed with a wrong value in it.
+    /// </summary>
+    /// <remarks>
+    /// Only the wrong value is repeated back, not the whole list: the model has to find out which
+    /// of its values the tool means.
+    /// </remarks>
+    private static ArgumentException InvalidListValue(string propertyName, JsonElement item, string expectation, string whenLeftOut) =>
+        Refusal($"Every value of argument '{propertyName}' must be {expectation}, but one was {Echo(item)}.", whenLeftOut);
+
+    private static ArgumentException Refusal(string message, string? whenLeftOut) => new(whenLeftOut is null ? message : $"{message} Leave it out {whenLeftOut}.");
+
+    private static string Echo(JsonElement value)
+    {
+        var receivedValue = value.GetRawText();
+        return receivedValue.Length > MAX_ARGUMENT_ECHO_LENGTH ? $"{receivedValue[..MAX_ARGUMENT_ECHO_LENGTH]}..." : receivedValue;
+    }
+}

app/MindWork AI Studio/Tools/ToolCallingSystem/ToolCallingImplementations/ReadWebPageTool.cs

@@ -129,7 +129,7 @@ public sealed class ReadWebPageTool(WebPageRetrievalService webPageRetrievalServ
 
     public async Task<ToolExecutionResult> ExecuteAsync(JsonElement arguments, ToolExecutionContext context, CancellationToken token = default)
     {
-        var urlText = ReadRequiredString(arguments, URL_ARGUMENT);
+        var urlText = ToolArgumentReader.ReadRequiredString(arguments, URL_ARGUMENT);
         if (!Uri.TryCreate(urlText, UriKind.Absolute, out var url) || url is not { Scheme: "http" or "https" })
             throw new ArgumentException("Argument 'url' must be a valid HTTP or HTTPS URL.");
 
@@ -337,18 +337,6 @@ public sealed class ReadWebPageTool(WebPageRetrievalService webPageRetrievalServ
         .Split(['\r', '\n', ',', ';'], StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries)
         .Where(x => !string.IsNullOrWhiteSpace(x)) ?? [];
 
-    private static string ReadRequiredString(JsonElement arguments, string propertyName)
-    {
-        if (!arguments.TryGetProperty(propertyName, out var value) || value.ValueKind is not JsonValueKind.String)
-            throw new ArgumentException($"Missing required argument '{propertyName}'.");
-
-        var text = value.GetString()?.Trim() ?? string.Empty;
-        if (string.IsNullOrWhiteSpace(text))
-            throw new ArgumentException($"Missing required argument '{propertyName}'.");
-
-        return text;
-    }
-
     private static string FormatUrlForLog(Uri url)
     {
         var builder = new UriBuilder(url)

app/MindWork AI Studio/Tools/ToolCallingSystem/ToolCallingImplementations/WebSearch/WebSearchTool.cs

@@ -111,11 +111,6 @@ public sealed class WebSearchTool(IEnumerable<IWebSearchBackend> backends, WebPa
     /// </remarks>
     private static readonly string[] TIME_RANGES = [TIME_RANGE_DAY, TIME_RANGE_WEEK, TIME_RANGE_MONTH, TIME_RANGE_YEAR];
 
-    /// <summary>
-    /// How much of a wrongly passed argument an error message repeats back to the model.
-    /// </summary>
-    private const int MAX_ARGUMENT_ECHO_LENGTH = 40;
-
     public string ImplementationKey => ToolSelectionRules.WEB_SEARCH_TOOL_ID;
 
     /// <inheritdoc />
@@ -826,95 +821,27 @@ public sealed class WebSearchTool(IEnumerable<IWebSearchBackend> backends, WebPa
     /// <summary>
     /// Reads the search query, the one argument the model always has to pass.
     /// </summary>
-    internal static string ReadQuery(JsonElement arguments)
+    internal static string ReadQuery(JsonElement arguments) => ToolArgumentReader.ReadRequiredString(arguments, QUERY_ARGUMENT);
-    {
-        var query = ReadOptionalString(arguments, QUERY_ARGUMENT, whenLeftOut: null);
-        if (string.IsNullOrWhiteSpace(query))
-            throw new ArgumentException($"Missing required argument '{QUERY_ARGUMENT}'.");
-
-        return query;
-    }
 
     /// <summary>
     /// Reads the language tag the model asked for, or null for the configured language.
     /// </summary>
-    internal static string? ReadLanguage(JsonElement arguments) => ReadOptionalString(arguments, LANGUAGE_ARGUMENT, "to use the configured language");
+    internal static string? ReadLanguage(JsonElement arguments) => ToolArgumentReader.ReadOptionalString(arguments, LANGUAGE_ARGUMENT, "to use the configured language");
 
     /// <summary>
     /// Reads the time range the model asked for, or null for no restriction.
     /// </summary>
-    internal static string? ReadTimeRange(JsonElement arguments)
+    internal static string? ReadTimeRange(JsonElement arguments) => ToolArgumentReader.ReadOptionalChoice(arguments, TIME_RANGE_ARGUMENT, TIME_RANGES, "to search without a time restriction");
-    {
-        if (!TryGetArgument(arguments, TIME_RANGE_ARGUMENT, out var value))
-            return null;
-
-        var timeRange = value.ValueKind is JsonValueKind.String ? value.GetString()?.Trim() : null;
-        if (timeRange is null || !TIME_RANGES.Contains(timeRange, StringComparer.Ordinal))
-            throw InvalidArgument(TIME_RANGE_ARGUMENT, value, $"one of {string.Join(", ", TIME_RANGES)}", "to search without a time restriction");
-
-        return timeRange;
-    }
 
     /// <summary>
     /// Reads the result page the model asked for, or null for the first one.
     /// </summary>
-    internal static int? ReadPage(JsonElement arguments) => ReadOptionalPositiveInt(arguments, PAGE_ARGUMENT, "to get the first page");
+    internal static int? ReadPage(JsonElement arguments) => ToolArgumentReader.ReadOptionalPositiveInt(arguments, PAGE_ARGUMENT, "to get the first page");
 
     /// <summary>
     /// Reads how many results the model asked for, or null for the configured number.
     /// </summary>
-    internal static int? ReadLimit(JsonElement arguments) => ReadOptionalPositiveInt(arguments, LIMIT_ARGUMENT, "to get as many results as configured");
+    internal static int? ReadLimit(JsonElement arguments) => ToolArgumentReader.ReadOptionalPositiveInt(arguments, LIMIT_ARGUMENT, "to get as many results as configured");
-
-    /// <summary>
-    /// Looks up an argument, treating null the same as leaving it out.
-    /// </summary>
-    private static bool TryGetArgument(JsonElement arguments, string propertyName, out JsonElement value) =>
-        arguments.TryGetProperty(propertyName, out value) && value.ValueKind is not JsonValueKind.Null;
-
-    private static string? ReadOptionalString(JsonElement arguments, string propertyName, string? whenLeftOut)
-    {
-        if (!TryGetArgument(arguments, propertyName, out var value))
-            return null;
-
-        if (value.ValueKind is not JsonValueKind.String)
-            throw InvalidArgument(propertyName, value, "a string", whenLeftOut);
-
-        return value.GetString()?.Trim();
-    }
-
-    private static int? ReadOptionalPositiveInt(JsonElement arguments, string propertyName, string whenLeftOut)
-    {
-        if (!TryGetArgument(arguments, propertyName, out var value))
-            return null;
-
-        if (value.ValueKind is not JsonValueKind.Number || !value.TryGetInt32(out var intValue) || intValue <= 0)
-            throw InvalidArgument(propertyName, value, "a positive integer", whenLeftOut);
-
-        return intValue;
-    }
-
-    /// <summary>
-    /// Builds the error a model gets for an argument it passed wrongly.
-    /// </summary>
-    /// <remarks>
-    /// The model reads this and tries again, so it says what arrived, what would have been right,
-    /// and, for an optional argument, that leaving it out is always an option. A model which
-    /// believes the argument has to be there otherwise keeps trying placeholders, and every attempt
-    /// costs one of the tool calls an answer may make.
-    /// </remarks>
-    /// <param name="propertyName">The argument.</param>
-    /// <param name="value">What the model passed, as it arrived.</param>
-    /// <param name="expectation">What the argument must be, completing "must be ...".</param>
-    /// <param name="whenLeftOut">What happens without the argument, completing "Leave it out ...", or null for a required one.</param>
-    private static ArgumentException InvalidArgument(string propertyName, JsonElement value, string expectation, string? whenLeftOut)
-    {
-        var receivedValue = value.GetRawText();
-        if (receivedValue.Length > MAX_ARGUMENT_ECHO_LENGTH)
-            receivedValue = $"{receivedValue[..MAX_ARGUMENT_ECHO_LENGTH]}...";
-
-        var message = $"Argument '{propertyName}' must be {expectation}, but was {receivedValue}.";
-        return new ArgumentException(whenLeftOut is null ? message : $"{message} Leave it out {whenLeftOut}.");
-    }
 
     private static string FormatQueryForLog(string query)
     {

app/Tests/Tools/ToolCalling/ToolArgumentReaderTests.cs

@@ -0,0 +1,97 @@
+using System.Text.Json;
+
+using AIStudio.Tools.ToolCallingSystem;
+
+namespace AIStudio.Tests.Tools.ToolCalling;
+
+/// <summary>
+/// Checks the readers every tool shares, where the web search tests do not already.
+/// </summary>
+/// <remarks>
+/// The web search tests cover strings, positive integers, and a single choice through the
+/// arguments of that tool. What is left are a required string which arrives empty and a list of
+/// choices, which the web search does not have: a data source a model names has to be one the
+/// tool offered, and the refusal has to say which ones those are.
+/// </remarks>
+[TestFixture]
+public sealed class ToolArgumentReaderTests
+{
+    private static readonly string[] OFFERED = ["alpha", "beta", "gamma"];
+
+    private const string WHEN_LEFT_OUT = "to search all of them";
+
+    [TestCase("""{"query":""}""")]
+    [TestCase("""{"query":"   "}""")]
+    public void AnEmptyRequiredStringIsRefusedAsEmpty(string json)
+    {
+        var message = Refusal(() => ToolArgumentReader.ReadRequiredString(Arguments(json), "query"));
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(message, Does.Contain("'query'").And.Contain("a non-empty string"), "The argument arrived, so calling it missing would make the model look for a typo in the name.");
+            Assert.That(message, Does.Not.Contain("Leave it out"));
+        });
+    }
+
+    [TestCase("""{}""")]
+    [TestCase("""{"ids":null}""")]
+    public void AListLeftOutIsNotSet(string json)
+    {
+        Assert.That(ToolArgumentReader.ReadOptionalChoices(Arguments(json), "ids", OFFERED, WHEN_LEFT_OUT), Is.Null);
+    }
+
+    [Test]
+    public void OfferedValuesComeThroughOnceEachInTheirOrder()
+    {
+        var choices = ToolArgumentReader.ReadOptionalChoices(Arguments("""{"ids":["gamma"," alpha ","gamma"]}"""), "ids", OFFERED, WHEN_LEFT_OUT);
+        Assert.That(choices, Is.EqualTo(new[] { "gamma", "alpha" }));
+    }
+
+    [TestCase("[]")]
+    [TestCase("\"alpha\"")]
+    [TestCase("5")]
+    public void AnEmptyListOrNoListIsRefusedWithTheValuesThatWouldDo(string value)
+    {
+        var message = Refusal(() => ToolArgumentReader.ReadOptionalChoices(Arguments($$"""{"ids":{{value}}}"""), "ids", OFFERED, WHEN_LEFT_OUT));
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(message, Does.Contain("'ids'").And.Contain("a list of one or more of alpha, beta, gamma"));
+            Assert.That(message, Does.Contain($"but was {value}."));
+            Assert.That(message, Does.Contain($"Leave it out {WHEN_LEFT_OUT}."), "An empty list asks for nothing; what leaving it out does is the way the model wanted.");
+        });
+    }
+
+    [TestCase("\"delta\"")]
+    [TestCase("\"Alpha\"")]
+    [TestCase("5")]
+    [TestCase("null")]
+    public void AValueNotOfferedIsRefusedOnItsOwn(string value)
+    {
+        var message = Refusal(() => ToolArgumentReader.ReadOptionalChoices(Arguments($$"""{"ids":["alpha",{{value}}]}"""), "ids", OFFERED, WHEN_LEFT_OUT));
+
+        Assert.Multiple(() =>
+        {
+            Assert.That(message, Does.Contain("'ids'").And.Contain("one of alpha, beta, gamma"));
+            Assert.That(message, Does.Contain($"but one was {value}."), "The model has to find out which of its values the tool means.");
+            Assert.That(message, Does.Not.Contain("\"alpha\""), "Only the wrong value comes back, not the whole list the model sent.");
+            Assert.That(message, Does.Contain("Leave it out"));
+        });
+    }
+
+    [Test]
+    public void AGuidIsRepeatedBackWhole()
+    {
+        var id = Guid.NewGuid().ToString();
+        var message = Refusal(() => ToolArgumentReader.ReadOptionalChoices(Arguments($$"""{"ids":["{{id}}"]}"""), "ids", OFFERED, WHEN_LEFT_OUT));
+
+        Assert.That(message, Does.Contain($"but one was \"{id}\"."), "Data sources are named by their GUIDs; a shortened one would leave the model guessing.");
+    }
+
+    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;
+}

documentation/Tools.md

@@ -62,7 +62,7 @@ When a tool returns data that future messages must only send to providers at or
 
 ## Security
 
-Treat model-provided tool arguments as untrusted input. Refuse a wrong one rather than guessing what it meant: a placeholder such as `0` is not a page, and reading it as "no page" does something the model did not ask for. The model reads the refusal and tries again, so the message has to name the argument and the value that arrived, say what would be valid, and, for an optional argument, that leaving it out is always possible. `WebSearchTool` shows the pattern.
+Treat model-provided tool arguments as untrusted input. Refuse a wrong one rather than guessing what it meant: a placeholder such as `0` is not a page, and reading it as "no page" does something the model did not ask for. The model reads the refusal and tries again, so the message has to name the argument and the value that arrived, say what would be valid, and, for an optional argument, that leaving it out is always possible. `ToolArgumentReader` reads strings, positive integers, and values out of a fixed choice, alone or as a list, and words the refusals so; `WebSearchTool` shows how a tool uses it.
 
 For tools that perform network requests: