From a1c1e3f1a5a833bc148a58b1276d47d1ffd1fbe6 Mon Sep 17 00:00:00 2001 From: Zhongkai Fu Date: Sun, 16 Aug 2026 22:34:08 -0700 Subject: [PATCH] fix json parser --- .../ApiExceptionMiddlewareTests.cs | 225 +++++++++++++ InferenceWeb.Tests/ToolFunctionParseTests.cs | 33 ++ .../ToolFunctionParserBenchmark.cs | 297 +++++++++++++++++ InferenceWeb.Tests/ToolFunctionParserTests.cs | 298 ++++++++++++++++++ TensorSharp.Runtime/OutputParser.cs | 41 ++- .../Logging/ApiExceptionMiddleware.cs | 154 +++++++++ TensorSharp.Server/Program.cs | 6 + .../RequestParsers/ToolFunctionParser.cs | 219 ++++++++++--- 8 files changed, 1233 insertions(+), 40 deletions(-) create mode 100644 InferenceWeb.Tests/ApiExceptionMiddlewareTests.cs create mode 100644 InferenceWeb.Tests/ToolFunctionParserBenchmark.cs create mode 100644 InferenceWeb.Tests/ToolFunctionParserTests.cs create mode 100644 TensorSharp.Server/Logging/ApiExceptionMiddleware.cs diff --git a/InferenceWeb.Tests/ApiExceptionMiddlewareTests.cs b/InferenceWeb.Tests/ApiExceptionMiddlewareTests.cs new file mode 100644 index 00000000..71487ccb --- /dev/null +++ b/InferenceWeb.Tests/ApiExceptionMiddlewareTests.cs @@ -0,0 +1,225 @@ +using System.Text; +using System.Text.Json; +using Microsoft.AspNetCore.Http; +using TensorSharp.Server.Logging; + +namespace InferenceWeb.Tests; + +/// +/// No request may answer with a framework error page. Before this middleware +/// existed, an exception escaping an endpoint hit the Developer Exception Page +/// that WebApplication.CreateBuilder installs ahead of all application +/// middleware, and a client sending Accept: text/html got the throwing +/// file's verbatim source back — which is how issue #142 was reported, with the +/// source of ToolFunctionParser.cs arriving in place of a completion. In +/// Production the same request got a bodiless 500 instead. +/// +public class ApiExceptionMiddlewareTests +{ + private static DefaultHttpContext ContextFor(string path) + { + var ctx = new DefaultHttpContext(); + ctx.Request.Path = path; + ctx.Response.Body = new MemoryStream(); + return ctx; + } + + private static async Task BodyOf(HttpContext ctx) + { + ctx.Response.Body.Seek(0, SeekOrigin.Begin); + using var reader = new StreamReader(ctx.Response.Body, Encoding.UTF8); + return await reader.ReadToEndAsync(); + } + + /// + /// Raise the throw the reporter hit — a legal tool spec whose enum + /// holds integers, read by an accessor that only accepts strings — from + /// inside the pipeline, exactly as a request parser does. It has to be + /// thrown here rather than caught and re-thrown by value: throw ex; + /// resets TargetSite, which is how the middleware tells a caller's + /// unreadable body from a fault of the server's own. + /// + private static Task ThrowJsonKindMismatch() + { + using var doc = JsonDocument.Parse("""{"enum":[1,2,3]}"""); + foreach (var v in doc.RootElement.GetProperty("enum").EnumerateArray()) + _ = v.GetString(); + throw new Xunit.Sdk.XunitException("GetString() on a Number was expected to throw."); + } + + private static Exception JsonKindMismatch() + { + try + { + ThrowJsonKindMismatch().GetAwaiter().GetResult(); + throw new Xunit.Sdk.XunitException("unreachable"); + } + catch (InvalidOperationException ex) + { + return ex; + } + } + + [Fact] + public async Task JsonKindMismatch_OnOpenAiRoute_Becomes400NotAnHtmlPage() + { + var ctx = ContextFor("/v1/chat/completions"); + + await ApiExceptionMiddleware.InvokeAsync(ctx, ThrowJsonKindMismatch); + + Assert.Equal(400, ctx.Response.StatusCode); + string body = await BodyOf(ctx); + using var doc = JsonDocument.Parse(body); + var error = doc.RootElement.GetProperty("error"); + Assert.Equal("invalid_request_error", error.GetProperty("type").GetString()); + // The message describes the caller's own JSON, so it is safe to echo and + // is the only thing that tells them which value the server choked on. + Assert.Contains("Number", error.GetProperty("message").GetString()); + Assert.DoesNotContain(" throw parseFailure); + + Assert.Equal(400, ctx.Response.StatusCode); + using var doc = JsonDocument.Parse(await BodyOf(ctx)); + Assert.Equal("invalid_request_error", doc.RootElement.GetProperty("error").GetProperty("type").GetString()); + } + + [Fact] + public async Task ServerFault_Becomes500AndLeaksNothingAboutTheException() + { + var ctx = ContextFor("/v1/chat/completions"); + var fault = new NullReferenceException("Object reference not set — /home/dev/TensorSharp/Secret.cs:line 42"); + + await ApiExceptionMiddleware.InvokeAsync(ctx, () => throw fault); + + Assert.Equal(500, ctx.Response.StatusCode); + string body = await BodyOf(ctx); + Assert.Equal("internal_error", + JsonDocument.Parse(body).RootElement.GetProperty("error").GetProperty("type").GetString()); + + // A server fault's detail belongs in the log, which RequestLoggingMiddleware + // has already written by the time the exception reaches here. + Assert.DoesNotContain("Secret.cs", body); + Assert.DoesNotContain("NullReferenceException", body); + Assert.DoesNotContain("Object reference", body); + } + + [Fact] + public async Task OnOllamaOrWebUiRoute_UsesTheFlatErrorShape() + { + var ctx = ContextFor("/api/chat"); + + await ApiExceptionMiddleware.InvokeAsync(ctx, ThrowJsonKindMismatch); + + Assert.Equal(400, ctx.Response.StatusCode); + using var doc = JsonDocument.Parse(await BodyOf(ctx)); + Assert.Equal(JsonValueKind.String, doc.RootElement.GetProperty("error").ValueKind); + } + + [Fact] + public async Task CorrelationIdSurvivesTheClearedResponse() + { + // RequestLoggingMiddleware sets this on the way in and documents it as + // present on every response; Response.Clear() would otherwise drop it + // from exactly the responses a user needs to quote. + var ctx = ContextFor("/v1/chat/completions"); + ctx.Response.Headers[RequestLoggingMiddleware.RequestIdHeader] = "abc123"; + + await ApiExceptionMiddleware.InvokeAsync(ctx, ThrowJsonKindMismatch); + + Assert.Equal("abc123", ctx.Response.Headers[RequestLoggingMiddleware.RequestIdHeader]); + } + + [Fact] + public async Task AfterResponseStarted_TheExceptionPropagates() + { + // A streaming completion that fails mid-body has already committed its + // status line; dropping the connection is the only signal SSE has. + var ctx = ContextFor("/v1/chat/completions"); + ctx.Features.Set(new StartedResponseFeature()); + + await Assert.ThrowsAsync(() => + ApiExceptionMiddleware.InvokeAsync(ctx, () => throw new InvalidOperationException("mid-stream"))); + } + + [Fact] + public async Task ClientAbort_IsNotAnsweredAtAll() + { + // The caller hung up before the response started: there is no socket + // left to write an error body to, so the cancellation propagates rather + // than turning into a 500 nobody can read. + var ctx = ContextFor("/v1/chat/completions"); + using var aborted = new CancellationTokenSource(); + await aborted.CancelAsync(); + ctx.RequestAborted = aborted.Token; + + await Assert.ThrowsAnyAsync(() => + ApiExceptionMiddleware.InvokeAsync(ctx, () => Task.FromCanceled(aborted.Token))); + + Assert.Equal(200, ctx.Response.StatusCode); + Assert.Equal(string.Empty, await BodyOf(ctx)); + } + + [Fact] + public async Task CancellationThatIsNotAClientAbort_StillBecomesAnError() + { + // An internal timeout is a server fault, not a hang-up: the connection + // is alive and deserves an answer. + var ctx = ContextFor("/v1/chat/completions"); + using var unrelated = new CancellationTokenSource(); + await unrelated.CancelAsync(); + + await ApiExceptionMiddleware.InvokeAsync(ctx, () => Task.FromCanceled(unrelated.Token)); + + Assert.Equal(500, ctx.Response.StatusCode); + } + + [Fact] + public async Task NoException_PassesThroughUntouched() + { + var ctx = ContextFor("/v1/chat/completions"); + ctx.Response.StatusCode = 200; + + await ApiExceptionMiddleware.InvokeAsync(ctx, () => Task.CompletedTask); + + Assert.Equal(200, ctx.Response.StatusCode); + Assert.Equal(string.Empty, await BodyOf(ctx)); + } + + [Fact] + public void OnlyJsonReadFailuresCountAsAMalformedRequest() + { + Assert.True(ApiExceptionMiddleware.IsMalformedRequest(JsonKindMismatch())); + Assert.False(ApiExceptionMiddleware.IsMalformedRequest(new NullReferenceException())); + // An InvalidOperationException raised by our own code is a server fault, + // not a client one — only System.Text.Json's are attributed to the body. + Assert.False(ApiExceptionMiddleware.IsMalformedRequest(new InvalidOperationException("ours"))); + } + + private sealed class StartedResponseFeature : Microsoft.AspNetCore.Http.Features.IHttpResponseFeature + { + public int StatusCode { get; set; } = 200; + public string ReasonPhrase { get; set; } + public IHeaderDictionary Headers { get; set; } = new HeaderDictionary(); + public Stream Body { get; set; } = new MemoryStream(); + public bool HasStarted => true; + public void OnStarting(System.Func callback, object state) { } + public void OnCompleted(System.Func callback, object state) { } + } +} diff --git a/InferenceWeb.Tests/ToolFunctionParseTests.cs b/InferenceWeb.Tests/ToolFunctionParseTests.cs index 77972c1f..e7f5d245 100644 --- a/InferenceWeb.Tests/ToolFunctionParseTests.cs +++ b/InferenceWeb.Tests/ToolFunctionParseTests.cs @@ -122,6 +122,39 @@ public void ToolWithoutParametersIsStillUsable() Assert.Empty(tools[0].Required); } + /// + /// A tool's parameters is a JSON Schema document, so enum may + /// hold values of any type and type may be a union. Both reach the + /// model as prompt text, so the spelling matters: a boolean has to arrive as + /// JSON's true rather than .NET's True, and a nullable field + /// has to keep a type name the renderers recognise instead of degrading to + /// any. Same rule as the server's tool parser (issue #142). + /// + [Fact] + public void JsonSchemaValuesThatAreNotStringsKeepTheirJsonSpelling() + { + var tools = ToolFunction.ParseList(""" + [{ + "name": "configure", + "parameters": { + "type": "object", + "properties": { + "level": { "type": "integer", "enum": [0, 1, 2] }, + "persist": { "type": "boolean", "enum": [true, false] }, + "scope": { "type": ["string", "null"] }, + "choice": { "type": "string", "enum": ["a", null] } + } + } + }] + """); + + var fn = Assert.Single(tools); + Assert.Equal(new[] { "0", "1", "2" }, fn.Parameters["level"].Enum); + Assert.Equal(new[] { "true", "false" }, fn.Parameters["persist"].Enum); + Assert.Equal("string", fn.Parameters["scope"].Type); + Assert.Equal(new[] { "a", "null" }, fn.Parameters["choice"].Enum); + } + /// /// Every failure mode has to surface as a (or a /// subclass of it, e.g. JsonReaderException) so the CLI's single catch turns diff --git a/InferenceWeb.Tests/ToolFunctionParserBenchmark.cs b/InferenceWeb.Tests/ToolFunctionParserBenchmark.cs new file mode 100644 index 00000000..e83d525b --- /dev/null +++ b/InferenceWeb.Tests/ToolFunctionParserBenchmark.cs @@ -0,0 +1,297 @@ +// Copyright (c) Zhongkai Fu. All rights reserved. +// https://github.com/zhongkaifu/TensorSharp +// +// This file is part of TensorSharp. +// +// TensorSharp is licensed under the BSD-3-Clause license found in the LICENSE file in the root directory of this source tree. +// +// TensorSharp is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the BSD-3-Clause License for more details. +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.Linq; +using System.Text; +using System.Text.Json; +using TensorSharp.Models; +using TensorSharp.Server.RequestParsers; + +namespace InferenceWeb.Tests; + +/// +/// Micro-benchmark for . Implemented as XUnit +/// Facts so it runs under dotnet test alongside the correctness suite; +/// numbers are printed to stdout (look for the [ToolFunctionParser.*] +/// banner). Needs no model, no GPU and no native init. +/// +/// This sits on the synchronous prologue of every chat request that declares +/// tools — OpenAIChatAdapter.ChatCompletionsAsync parses the spec before +/// it takes a queue ticket — so an agent harness that re-sends its whole tool +/// catalogue on every turn pays this cost per request, ahead of any inference. +/// The corpus below is sized for that case: a catalogue the size of a real MCP +/// server's, not a single toy tool. +/// +/// +/// A/Bs the current +/// kind-checked parser against , a verbatim copy +/// of the pre-#142 implementation. That implementation threw on half the specs +/// in this file, so the A/B runs on the all-strings corpus it could still handle +/// — the only input on which the two are comparable — to show that reading +/// before each access costs nothing. +/// +/// +public class ToolFunctionParserBenchmark +{ + // A tool catalogue the size of a working agent harness: 12 tools x 6 + // parameters, each parameter carrying a description and half of them an + // enum. ~14 KB of JSON, comparable to what a filesystem+shell+search MCP + // server advertises. + private const int ToolCount = 12; + private const int ParamsPerTool = 6; + + private static readonly string StringOnlySpec = BuildSpec(mixedKinds: false); + private static readonly string MixedKindSpec = BuildSpec(mixedKinds: true); + + // Both candidates are warmed before either is timed, and the rounds + // alternate between them, so neither is charged for tiered-JIT promotion or + // for a cold ArrayPool — measuring them one after the other made whichever + // ran first look ~3x slower than it is. + private const int Warmup = 500; + private const int Rounds = 6; + private const int ItersPerRound = 1000; + + /// + /// One process-wide warm-up, run once before any measurement. .NET promotes + /// a method to its optimized tier on call count and elapsed time, so a + /// per-benchmark warm-up loop is not enough on its own: whichever benchmark + /// happened to run first still measured tier-0 code and read ~3.5x slower + /// than the very same parser did one call later. + /// + private static readonly bool Warmed = WarmUp(); + + private static bool WarmUp() + { + var sw = Stopwatch.StartNew(); + while (sw.ElapsedMilliseconds < 750) + { + Run(StringOnlySpec, 20, ToolFunctionParser.ParseOpenAI); + Run(StringOnlySpec, 20, LegacyParseOpenAI); + Run(MixedKindSpec, 20, ToolFunctionParser.ParseOpenAI); + } + return true; + } + + [Fact] + public void Parse_KindTolerantVersusLegacyStringOnlyParser() + { + Assert.True(Warmed); + Console.WriteLine($"[ToolFunctionParser] corpus: {ToolCount} tools x {ParamsPerTool} params, " + + $"{Encoding.UTF8.GetByteCount(StringOnlySpec) / 1024.0:F1} KB JSON, " + + $"{Rounds} rounds x {ItersPerRound} parses, best round reported"); + + var candidates = new (string Label, Func> Parse)[] + { + ("legacy (pre-#142, string-only)", LegacyParseOpenAI), + ("current (kind-checked)", ToolFunctionParser.ParseOpenAI), + }; + double[] best = BenchAlternating(StringOnlySpec, candidates); + + double legacy = best[0], current = best[1]; + Console.WriteLine($"[ToolFunctionParser] current / legacy = {current / legacy:F3}x " + + $"({(legacy - current) / legacy * 100:+0.0;-0.0}% faster)"); + + // The A/B is only meaningful if both parsers saw the same catalogue. + using var doc = JsonDocument.Parse(StringOnlySpec); + Assert.Equal(ToolCount, ToolFunctionParser.ParseOpenAI(doc.RootElement).Count); + Assert.Equal(ToolCount, LegacyParseOpenAI(doc.RootElement).Count); + } + + [Fact] + public void Parse_MixedKindSpec_IsNotAPathologicalCase() + { + Assert.True(Warmed); + Console.WriteLine($"[ToolFunctionParser] mixed-kind corpus: " + + $"{Encoding.UTF8.GetByteCount(MixedKindSpec) / 1024.0:F1} KB JSON " + + $"(integer/boolean enums, union types — the shapes that used to throw)"); + + // The same parser over both corpora: the kinds the legacy parser could + // not read must not cost materially more than the ones it could. + var candidates = new (string, Func>)[] + { + ("current (all strings)", ToolFunctionParser.ParseOpenAI), + }; + double strings = BenchAlternating(StringOnlySpec, candidates)[0]; + double mixed = BenchAlternating(MixedKindSpec, + [("current (mixed kinds)", ToolFunctionParser.ParseOpenAI)])[0]; + + Console.WriteLine($"[ToolFunctionParser] mixed / strings = {mixed / strings:F3}x"); + + using var doc = JsonDocument.Parse(MixedKindSpec); + var tools = ToolFunctionParser.ParseOpenAI(doc.RootElement); + Assert.Equal(ToolCount, tools.Count); + Assert.All(tools, t => Assert.Equal(ParamsPerTool, t.Parameters.Count)); + } + + /// + /// Time every candidate over the same corpus, reporting each one's fastest + /// round in microseconds per spec. Returns the results in candidate order. + /// + private static double[] BenchAlternating( + string json, (string Label, Func> Parse)[] candidates) + { + var best = new double[candidates.Length]; + Array.Fill(best, double.MaxValue); + int checksum = 0; + + foreach (var candidate in candidates) + checksum += Run(json, Warmup, candidate.Parse); + + for (int round = 0; round < Rounds; round++) + { + for (int c = 0; c < candidates.Length; c++) + { + var sw = Stopwatch.StartNew(); + checksum += Run(json, ItersPerRound, candidates[c].Parse); + sw.Stop(); + best[c] = Math.Min(best[c], sw.Elapsed.TotalMilliseconds * 1000.0 / ItersPerRound); + } + } + + for (int c = 0; c < candidates.Length; c++) + { + Console.WriteLine($"[ToolFunctionParser] {candidates[c].Label,-32} {best[c],8:F2} us/spec " + + $"{1_000_000.0 / best[c],10:N0} spec/s"); + } + Console.WriteLine($"[ToolFunctionParser] (checksum {checksum})"); + return best; + } + + private static int Run(string json, int iters, Func> parse) + { + // Each iteration re-parses the document too, because that is what the + // request path actually does — timing the parser against a pre-parsed + // JsonDocument would measure a call that never happens in production. + int tools = 0; + for (int i = 0; i < iters; i++) + { + using var doc = JsonDocument.Parse(json); + tools += parse(doc.RootElement).Count; + } + return tools; + } + + /// + /// The parser exactly as it stood before the issue #142 fix + /// (ToolFunctionParser.ParseOpenAI/ParseFunction at commit + /// b353392), kept only as the benchmark's baseline. It throws + /// on any tool spec that puts a + /// non-string where it expected one, so it can only be run on + /// . + /// + private static List LegacyParseOpenAI(JsonElement body) + { + if (!body.TryGetProperty("tools", out var toolsEl) || toolsEl.ValueKind != JsonValueKind.Array) + return null; + + var tools = new List(); + foreach (var toolEl in toolsEl.EnumerateArray()) + { + string type = toolEl.TryGetProperty("type", out var t) ? t.GetString() : "function"; + if (type != "function") continue; + if (!toolEl.TryGetProperty("function", out var fnEl)) continue; + + var tf = new ToolFunction + { + Name = fnEl.TryGetProperty("name", out var n) ? n.GetString() : "", + Description = fnEl.TryGetProperty("description", out var d) ? d.GetString() : "" + }; + + if (fnEl.TryGetProperty("parameters", out var paramsEl)) + { + if (paramsEl.TryGetProperty("properties", out var propsEl) && + propsEl.ValueKind == JsonValueKind.Object) + { + tf.Parameters = new Dictionary(); + foreach (var prop in propsEl.EnumerateObject()) + { + var tp = new ToolParameter + { + Type = prop.Value.TryGetProperty("type", out var pt) ? pt.GetString() : "string", + Description = prop.Value.TryGetProperty("description", out var pd) ? pd.GetString() : null + }; + if (prop.Value.TryGetProperty("enum", out var enumEl) && enumEl.ValueKind == JsonValueKind.Array) + tp.Enum = enumEl.EnumerateArray().Select(e => e.GetString()).ToList(); + tf.Parameters[prop.Name] = tp; + } + } + if (paramsEl.TryGetProperty("required", out var reqEl) && reqEl.ValueKind == JsonValueKind.Array) + tf.Required = reqEl.EnumerateArray().Select(e => e.GetString()).ToList(); + } + + tools.Add(tf); + } + return tools.Count > 0 ? tools : null; + } + + /// + /// Build an OpenAI Chat Completions tools catalogue. With + /// the schemas use the JSON Schema spellings + /// that the legacy parser could not read — integer and boolean enums, and + /// "type": ["…", "null"] for nullable fields — while keeping the same + /// tool and parameter counts so the two corpora stay comparable. + /// + private static string BuildSpec(bool mixedKinds) + { + var tools = new List(ToolCount); + for (int t = 0; t < ToolCount; t++) + { + var properties = new Dictionary(ParamsPerTool); + for (int p = 0; p < ParamsPerTool; p++) + { + var schema = new Dictionary + { + // p % 3 spreads the three type spellings over the parameters; + // the union form is the one a nullable field gets. + ["type"] = !mixedKinds ? "string" + : p % 3 == 0 ? new[] { "string", "null" } + : p % 3 == 1 ? "integer" + : (object)"string", + ["description"] = $"Parameter {p} of tool {t}, described about as fully as a real one is." + }; + + if (p % 2 == 0) + { + schema["enum"] = !mixedKinds ? new object[] { "alpha", "beta", "gamma" } + : p % 3 == 1 ? new object[] { 0, 1, 2, 3 } + : p % 3 == 2 ? new object[] { true, false } + : new object[] { "alpha", "beta", "gamma" }; + } + + properties[$"param_{p}"] = schema; + } + + tools.Add(new + { + type = "function", + function = new + { + name = $"tool_{t}", + description = $"Benchmark tool number {t}, with a description of the length a real tool carries.", + parameters = new Dictionary + { + ["type"] = "object", + ["properties"] = properties, + ["required"] = new[] { "param_0", "param_1" } + } + } + }); + } + + return JsonSerializer.Serialize(new + { + model = "bench", + messages = new[] { new { role = "user", content = "go" } }, + tools + }); + } +} diff --git a/InferenceWeb.Tests/ToolFunctionParserTests.cs b/InferenceWeb.Tests/ToolFunctionParserTests.cs new file mode 100644 index 00000000..ab0a6aae --- /dev/null +++ b/InferenceWeb.Tests/ToolFunctionParserTests.cs @@ -0,0 +1,298 @@ +// Copyright (c) Zhongkai Fu. All rights reserved. +// https://github.com/zhongkaifu/TensorSharp +// +// This file is part of TensorSharp. +// +// TensorSharp is licensed under the BSD-3-Clause license found in the LICENSE file in the root directory of this source tree. +// +// TensorSharp is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the BSD-3-Clause License for more details. +using System; +using System.Text.Json; +using TensorSharp.Server.RequestParsers; + +namespace InferenceWeb.Tests; + +/// +/// reads a tool's parameters, which is a +/// JSON Schema document — and JSON Schema puts non-strings in places a naive +/// reader assumes are strings. Every accessor used to be an unguarded +/// GetString(), so an "enum": [1, 2, 3] or a nullable field's +/// "type": ["string", "null"] threw +/// out of the parser and failed the entire chat request with a 500, even though +/// the spec was valid (https://github.com/zhongkaifu/TensorSharp/issues/142). +/// These tests pin every kind that is legal in a tool spec, on all three +/// protocol entry points. +/// +/// The sibling covers the unrelated public +/// TensorSharp.Runtime.ToolFunction.ParseList, which is the CLI's +/// --tools path. +/// +/// +public class ToolFunctionParserTests +{ + /// + /// The payload from issue #142, reduced: an OpenAI tool spec whose schema is + /// entirely legal but uses an integer enum, a boolean enum, a union type and + /// a no-argument tool. Every one of these threw before the fix. + /// + private const string HarnessToolsBody = """ + { + "model": "qwen35", + "messages": [ { "role": "user", "content": "hi" } ], + "tools": [ + { + "type": "function", + "function": { + "name": "set_verbosity", + "description": "Set the log verbosity.", + "parameters": { + "type": "object", + "properties": { + "level": { "type": "integer", "description": "0-3.", "enum": [0, 1, 2, 3] }, + "scope": { "type": ["string", "null"], "description": "Subsystem, or null for all." }, + "persist": { "type": "boolean", "enum": [true, false] } + }, + "required": ["level"] + } + } + }, + { + "type": "function", + "function": { "name": "now", "description": "Current time.", "parameters": null } + } + ] + } + """; + + private static JsonElement Body(string json) => JsonDocument.Parse(json).RootElement; + + // ---- issue #142 regression -------------------------------------------- + + [Fact] + public void ParseOpenAI_HarnessToolSpec_ParsesInsteadOfThrowing() + { + var tools = ToolFunctionParser.ParseOpenAI(Body(HarnessToolsBody)); + + Assert.Equal(2, tools.Count); + + var verbosity = tools[0]; + Assert.Equal("set_verbosity", verbosity.Name); + Assert.Equal("Set the log verbosity.", verbosity.Description); + Assert.Equal(["level"], verbosity.Required); + + // An integer enum keeps its JSON spelling, so the schema handed to the + // model still says 0-3 rather than "0"-"3" or nothing at all. + Assert.Equal("integer", verbosity.Parameters["level"].Type); + Assert.Equal(["0", "1", "2", "3"], verbosity.Parameters["level"].Enum); + + // "type": ["string", "null"] collapses to the real type; the union's + // nullability is already expressed by "level" being the only required one. + Assert.Equal("string", verbosity.Parameters["scope"].Type); + Assert.Equal("Subsystem, or null for all.", verbosity.Parameters["scope"].Description); + + // JSON casing, not .NET's — JsonElement.ToString() would render "True". + Assert.Equal("boolean", verbosity.Parameters["persist"].Type); + Assert.Equal(["true", "false"], verbosity.Parameters["persist"].Enum); + + // "parameters": null is how a no-argument tool is usually written. + Assert.Equal("now", tools[1].Name); + Assert.Empty(tools[1].Parameters); + Assert.Empty(tools[1].Required); + } + + [Theory] + [InlineData("""{"tools":[{"type":"function","function":{"name":"f","parameters":{"type":"object","properties":{"a":{"type":"string","enum":[1,"b",true,null,{"k":1},[2]]}}}}}]}""")] + [InlineData("""{"tools":[{"type":"function","function":{"name":"f","parameters":{"type":"object","properties":{"a":{"type":["null","integer"]}}}}}]}""")] + [InlineData("""{"tools":[{"type":"function","function":{"name":"f","parameters":{"type":"object","properties":{"a":true}}}}]}""")] + [InlineData("""{"tools":[{"type":"function","function":{"name":"f","parameters":{"type":"object","properties":{"a":{"type":{"const":"x"}}}}}}]}""")] + [InlineData("""{"tools":[{"type":"function","function":{"name":"f","parameters":{"type":"object","required":["a",7,null]}}}]}""")] + [InlineData("""{"tools":[{"type":"function","function":{"name":"f","parameters":{"type":"object","properties":{"a":{"description":42}}}}}]}""")] + [InlineData("""{"tools":[{"type":"function","function":{"name":7,"description":[1]}}]}""")] + [InlineData("""{"tools":[{"type":"function","function":{"name":"f","parameters":[]}}]}""")] + [InlineData("""{"tools":[{"type":"function","function":{"name":"f","parameters":{"properties":"nope","required":"nope"}}}]}""")] + [InlineData("""{"tools":["a string",42,null,[],{"type":"function"}]}""")] + [InlineData("""{"tools":[{"type":7,"function":{"name":"f"}}]}""")] + [InlineData("""{"tools":"not an array"}""")] + [InlineData("""[1,2,3]""")] + public void EveryProtocol_TolerantOfAnyJsonKindInAToolSpec(string json) + { + // No shape of syntactically valid JSON may escape as an exception: the + // parsers run before the request has produced any output, so a throw + // here is a 500 on a request the server could otherwise have served. + Assert.Null(Record.Exception(() => ToolFunctionParser.ParseOpenAI(Body(json)))); + Assert.Null(Record.Exception(() => ToolFunctionParser.ParseOpenAIResponses(Body(json)))); + Assert.Null(Record.Exception(() => ToolFunctionParser.ParseOllama(Body(json)))); + } + + // ---- enum value rendering ---------------------------------------------- + + [Fact] + public void ParseOpenAI_NonStringEnumValues_KeepTheirRawJsonText() + { + const string body = """ + { + "tools": [{ + "type": "function", + "function": { + "name": "f", + "parameters": { + "type": "object", + "properties": { "a": { "type": "string", "enum": ["s", 1, 2.5, true, false, null, {"k":1}, [2]] } } + } + } + }] + } + """; + + var values = ToolFunctionParser.ParseOpenAI(Body(body))[0].Parameters["a"].Enum; + + // A string member is unquoted so it re-serializes as itself; everything + // else is its literal JSON text, which is what the prompt renderers emit + // back into the schema they show the model. + Assert.Equal(["s", "1", "2.5", "true", "false", "null", """{"k":1}""", "[2]"], values); + } + + // ---- shapes that already worked keep working --------------------------- + + [Fact] + public void ParseOpenAI_PlainStringSchema_IsUnchanged() + { + const string body = """ + { + "tools": [{ + "type": "function", + "function": { + "name": "get_current_weather", + "description": "Get the current weather in a given city.", + "parameters": { + "type": "object", + "properties": { + "city": { "type": "string", "description": "The city name." }, + "unit": { "type": "string", "enum": ["celsius", "fahrenheit"] } + }, + "required": ["city"] + } + } + }] + } + """; + + var tool = Assert.Single(ToolFunctionParser.ParseOpenAI(Body(body))); + Assert.Equal("get_current_weather", tool.Name); + Assert.Equal("Get the current weather in a given city.", tool.Description); + Assert.Equal(2, tool.Parameters.Count); + Assert.Equal("string", tool.Parameters["city"].Type); + Assert.Equal("The city name.", tool.Parameters["city"].Description); + Assert.Equal(["celsius", "fahrenheit"], tool.Parameters["unit"].Enum); + Assert.Equal(["city"], tool.Required); + } + + [Fact] + public void ParseOllama_NestsUnderFunctionWithoutRequiringAType() + { + const string body = """ + { + "tools": [{ + "function": { + "name": "get_current_weather", + "parameters": { + "type": "object", + "properties": { "city": { "type": "string" }, "days": { "type": "integer", "enum": [1, 7] } }, + "required": ["city"] + } + } + }] + } + """; + + var tool = Assert.Single(ToolFunctionParser.ParseOllama(Body(body))); + Assert.Equal("get_current_weather", tool.Name); + Assert.Equal(["1", "7"], tool.Parameters["days"].Enum); + Assert.Equal(["city"], tool.Required); + } + + [Fact] + public void ParseOpenAIResponses_FlatShapeStillParses() + { + const string body = """ + { + "tools": [ + { "type": "web_search" }, + { + "type": "function", + "name": "get_weather", + "parameters": { + "type": "object", + "properties": { "city": { "type": "string" } }, + "required": ["city"] + } + } + ] + } + """; + + // The built-in web_search tool is not ours to run and must not become a + // nameless function declaration in the prompt. + var tool = Assert.Single(ToolFunctionParser.ParseOpenAIResponses(Body(body))); + Assert.Equal("get_weather", tool.Name); + Assert.Equal("string", tool.Parameters["city"].Type); + } + + // ---- nothing to parse --------------------------------------------------- + + [Theory] + [InlineData("{}")] + [InlineData("""{"tools":[]}""")] + [InlineData("""{"tools":null}""")] + [InlineData("""{"tools":[{"type":"web_search"}]}""")] + public void NoUsableTools_ReturnsNullSoCallersShortCircuit(string json) + { + Assert.Null(ToolFunctionParser.ParseOpenAI(Body(json))); + Assert.Null(ToolFunctionParser.ParseOpenAIResponses(Body(json))); + } + + [Fact] + public void ParseOpenAI_NonFunctionEntriesAreSkippedButFunctionsSurvive() + { + const string body = """ + { + "tools": [ + { "type": "code_interpreter" }, + { "type": "function", "function": { "name": "kept" } }, + { "function": { "name": "also_kept" } } + ] + } + """; + + var tools = ToolFunctionParser.ParseOpenAI(Body(body)); + Assert.Equal(["kept", "also_kept"], tools.ConvertAll(t => t.Name)); + } + + [Fact] + public void ParseFunction_MissingStringFieldsBecomeEmptyNotNull() + { + // ToolFunction/ToolParameter declare these non-nullable, and the prompt + // renderers call string.IsNullOrEmpty on them; a null would be a latent + // NullReferenceException one refactor away. + const string body = """ + {"tools":[{"type":"function","function":{"name":"f","parameters":{"type":"object","properties":{"a":{}}}}}]} + """; + + var tool = Assert.Single(ToolFunctionParser.ParseOpenAI(Body(body))); + Assert.Equal(string.Empty, tool.Description); + Assert.Equal("string", tool.Parameters["a"].Type); + Assert.Equal(string.Empty, tool.Parameters["a"].Description); + Assert.Empty(tool.Parameters["a"].Enum); + } + + [Fact] + public void ParseFunction_RequiredDropsEntriesThatCannotNameAProperty() + { + const string body = """ + {"tools":[{"type":"function","function":{"name":"f","parameters":{"type":"object","required":["a",7,null,"b"]}}}]} + """; + + Assert.Equal(["a", "b"], Assert.Single(ToolFunctionParser.ParseOpenAI(Body(body))).Required); + } +} diff --git a/TensorSharp.Runtime/OutputParser.cs b/TensorSharp.Runtime/OutputParser.cs index 7df13cd0..7254a74d 100644 --- a/TensorSharp.Runtime/OutputParser.cs +++ b/TensorSharp.Runtime/OutputParser.cs @@ -120,14 +120,20 @@ private static ToolFunction ParseOne(JsonElement entry) continue; // a schema keyword sitting next to "properties" ("type", "$schema", ...) var param = new ToolParameter { - Type = GetString(prop.Value, "type") ?? string.Empty, + Type = ReadSchemaType(prop.Value) ?? string.Empty, Description = GetString(prop.Value, "description") ?? string.Empty, }; if (prop.Value.TryGetProperty("enum", out JsonElement enumValues) && enumValues.ValueKind == JsonValueKind.Array) { + // A string member is stored unquoted, because the renderers + // add the quotes themselves; anything else keeps its raw JSON + // text. GetRawText rather than ToString: ToString renders a + // boolean in .NET's casing ("True", which is not JSON) and + // renders null as an empty string, which reaches the model as + // a meaningless empty choice in the enum. foreach (JsonElement v in enumValues.EnumerateArray()) - param.Enum.Add(v.ValueKind == JsonValueKind.String ? v.GetString() : v.ToString()); + param.Enum.Add(v.ValueKind == JsonValueKind.String ? v.GetString() : v.GetRawText()); } fn.Parameters[prop.Name] = param; } @@ -143,6 +149,37 @@ private static string GetString(JsonElement obj, string name) ? v.GetString() : null; + /// + /// Read a property schema's type. JSON Schema allows a union, and + /// "type": ["string", "null"] is how every schema generator spells + /// a nullable field, while holds a single + /// name that the renderers switch on — an unrecognised one degrades the + /// parameter to any and drops its enum. Keep the first real type + /// and drop the "null" member, whose meaning required + /// already carries. + /// + private static string ReadSchemaType(JsonElement schema) + { + if (!schema.TryGetProperty("type", out JsonElement type)) + return null; + if (type.ValueKind == JsonValueKind.String) + return type.GetString(); + if (type.ValueKind != JsonValueKind.Array) + return null; + + string first = null; + foreach (JsonElement v in type.EnumerateArray()) + { + if (v.ValueKind != JsonValueKind.String) + continue; + string name = v.GetString(); + first ??= name; + if (name != "null") + return name; + } + return first; + } + private static void CollectRequired(JsonElement obj, List into) { if (!obj.TryGetProperty("required", out JsonElement req) || req.ValueKind != JsonValueKind.Array) diff --git a/TensorSharp.Server/Logging/ApiExceptionMiddleware.cs b/TensorSharp.Server/Logging/ApiExceptionMiddleware.cs new file mode 100644 index 00000000..45f9e247 --- /dev/null +++ b/TensorSharp.Server/Logging/ApiExceptionMiddleware.cs @@ -0,0 +1,154 @@ +// Copyright (c) Zhongkai Fu. All rights reserved. +// https://github.com/zhongkaifu/TensorSharp +// +// This file is part of TensorSharp. +// +// TensorSharp is licensed under the BSD-3-Clause license found in the LICENSE file in the root directory of this source tree. +// +// TensorSharp is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the BSD-3-Clause License for more details. + +using System; +using System.Text.Json; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Http; + +namespace TensorSharp.Server.Logging +{ + /// + /// The server's outermost error boundary: an exception that escapes an + /// endpoint becomes a protocol-shaped JSON error instead of whatever the + /// framework would have produced. + /// + /// Without it the server has two bad answers for the same event. + /// WebApplication.CreateBuilder installs the Developer Exception Page + /// ahead of all application middleware, so under + /// ASPNETCORE_ENVIRONMENT=Development an unhandled exception answers a + /// request that sent Accept: text/html with a ~30 KB HTML page + /// containing the throwing file's verbatim source, the full stack, the + /// absolute paths of the server's source tree and an echo of the request + /// headers — which is how issue #142 was reported: a tool spec the parser + /// could not read came back as the source of ToolFunctionParser.cs. + /// In Production the page is not registered at all and the same request gets + /// a bodiless 500, which an OpenAI-compatible client cannot interpret either. + /// + /// + /// Registered as the first application middleware, so it sits below + /// the Developer Exception Page and handles the exception before that page + /// ever sees it, and above + /// , which has already logged the + /// failure with its full detail server-side by the time it arrives here. + /// Nothing about the exception reaches the client. + /// + /// + internal static class ApiExceptionMiddleware + { + /// + /// Assembly prefix identifying a body the server could not read. Matched + /// as a prefix because the runtime attributes the throw to + /// System.Text.Json or, once it has passed through the rethrow + /// helper, to System.Text.Json.Rethrowable. + /// + private const string JsonAssemblyPrefix = "System.Text.Json"; + + public static IApplicationBuilder UseApiExceptionHandling(this IApplicationBuilder app) + { + return app.Use(next => context => InvokeAsync(context, () => next(context))); + } + + internal static async Task InvokeAsync(HttpContext context, Func next) + { + try + { + await next(); + } + catch (Exception ex) when (!context.Response.HasStarted && !IsClientAbort(context, ex)) + { + await WriteErrorAsync(context, ex); + } + // A streaming completion that fails mid-body has already sent its + // status line, so there is nothing left to shape; the exception + // propagates and the connection drops, which is the only signal SSE + // has for a truncated stream. + } + + /// + /// The caller hung up — a browser navigating away, a harness cancelling a + /// generation. There is no longer a socket to answer on, so writing an + /// error body would only throw again; let it propagate and be logged as + /// the abort it is. + /// + private static bool IsClientAbort(HttpContext context, Exception ex) + => ex is OperationCanceledException && context.RequestAborted.IsCancellationRequested; + + /// + /// Is this the caller's error rather than the server's? Two failure + /// modes are attributable to the request body: + /// for text that is not valid JSON, and an + /// raised inside System.Text.Json + /// for valid JSON whose value is of a kind the reader did not expect — + /// GetString() on a number, TryGetProperty on a non-object. + /// An from our own code means + /// something else entirely and stays a 500, so the provenance test is + /// what separates them. + /// + internal static bool IsMalformedRequest(Exception ex) + { + if (ex is JsonException) + return true; + if (ex is not InvalidOperationException) + return false; + + // TargetSite is the more precise of the two and survives a rethrow; + // Source covers the case where the stack was not captured. + string origin = ex.TargetSite?.DeclaringType?.Assembly.GetName().Name ?? ex.Source; + return origin != null && origin.StartsWith(JsonAssemblyPrefix, StringComparison.Ordinal); + } + + private static async Task WriteErrorAsync(HttpContext context, Exception ex) + { + bool malformed = IsMalformedRequest(ex); + + // Response.Clear() drops headers as well as the body, and the + // correlation id RequestLoggingMiddleware promises on every response + // is one of them — put it back so an error is still the response a + // user can quote in a bug report. + context.Response.Headers.TryGetValue(RequestLoggingMiddleware.RequestIdHeader, out var requestId); + context.Response.Clear(); + if (requestId.Count > 0) + context.Response.Headers[RequestLoggingMiddleware.RequestIdHeader] = requestId; + + context.Response.StatusCode = malformed + ? StatusCodes.Status400BadRequest + : StatusCodes.Status500InternalServerError; + + // A malformed-body message describes the caller's own JSON ("the + // target element has type 'Number'"), so it is safe and useful to + // return. Anything else is a server fault whose detail stays in the + // log, where RequestLoggingMiddleware has already written it. + string message = malformed + ? $"Could not read the request body: {ex.Message}" + : "The server failed to handle the request."; + + // OpenAI SDKs parse {error:{message,type}}; the Ollama and Web UI + // clients expect a flat {error:"..."}. Shape by route prefix, the + // same rule PromptOverflowMiddleware uses. + if (context.Request.Path.StartsWithSegments("/v1")) + { + await context.Response.WriteAsJsonAsync(new + { + error = new + { + message, + type = malformed ? "invalid_request_error" : "internal_error" + } + }); + } + else + { + await context.Response.WriteAsJsonAsync(new { error = message }); + } + } + } +} diff --git a/TensorSharp.Server/Program.cs b/TensorSharp.Server/Program.cs index 0d31c0d0..155a4a9d 100644 --- a/TensorSharp.Server/Program.cs +++ b/TensorSharp.Server/Program.cs @@ -242,6 +242,12 @@ StartupBanner.EmitBackendFallback(startupLogger, hostingOptions, configuredBackendInput); +// Outermost application middleware, so it handles an escaping exception before +// the framework's Developer Exception Page can answer with the throwing source +// file. Request logging sits just inside it and has already recorded the +// failure in full by the time it rethrows here, so every API surface fails as +// JSON without losing a single log line. +app.UseApiExceptionHandling(); app.UseTensorSharpRequestLogging(); // Convert a prompt-doesn't-fit-context failure into a 400. After request // logging so the rejection is still traced; before the endpoints so it covers diff --git a/TensorSharp.Server/RequestParsers/ToolFunctionParser.cs b/TensorSharp.Server/RequestParsers/ToolFunctionParser.cs index 0961e7a7..5743fd9a 100644 --- a/TensorSharp.Server/RequestParsers/ToolFunctionParser.cs +++ b/TensorSharp.Server/RequestParsers/ToolFunctionParser.cs @@ -9,7 +9,6 @@ // MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the BSD-3-Clause License for more details. using System.Collections.Generic; -using System.Linq; using System.Text.Json; using TensorSharp.Models; @@ -21,41 +20,59 @@ namespace TensorSharp.Server.RequestParsers /// (the parsing path is identical, only the wrapping differs: Ollama nests /// the function under "function"; OpenAI additionally requires /// "type": "function"). + /// + /// Every read goes through the kind-checked helpers at the bottom of this + /// file rather than 's typed accessors, because + /// those accessors throw when + /// the element is of another kind — and a tool's parameters is a JSON + /// Schema document, in which several fields legally hold non-strings: + /// + /// + /// enum takes values of any type — "enum": [1, 2, 3] on an + /// integer parameter is as valid as a list of strings + /// type is a type name or an array of them — + /// "type": ["string", "null"] is what most schema generators emit for + /// a nullable field + /// parameters is frequently null on a tool that takes no + /// arguments, and a property schema may itself be a boolean + /// + /// + /// Each of those used to escape as an unhandled exception and fail the whole + /// chat request rather than the one field it came from, so a harness sending + /// a perfectly valid OpenAI tool spec got a 500 instead of a completion + /// (https://github.com/zhongkaifu/TensorSharp/issues/142). Nothing here + /// throws: a field of an unexpected kind is skipped, not fatal. + /// /// internal static class ToolFunctionParser { public static List ParseOllama(JsonElement body) { - if (!body.TryGetProperty("tools", out var toolsEl) || toolsEl.ValueKind != JsonValueKind.Array) + if (!TryGetArray(body, "tools", out var toolsEl)) return null; var tools = new List(); foreach (var toolEl in toolsEl.EnumerateArray()) { - if (!toolEl.TryGetProperty("function", out var fnEl)) + if (!TryGetObject(toolEl, "function", out var fnEl)) continue; - var tf = ParseFunction(fnEl); - if (tf != null) - tools.Add(tf); + tools.Add(ParseFunction(fnEl)); } return tools.Count > 0 ? tools : null; } public static List ParseOpenAI(JsonElement body) { - if (!body.TryGetProperty("tools", out var toolsEl) || toolsEl.ValueKind != JsonValueKind.Array) + if (!TryGetArray(body, "tools", out var toolsEl)) return null; var tools = new List(); foreach (var toolEl in toolsEl.EnumerateArray()) { - string type = toolEl.TryGetProperty("type", out var t) ? t.GetString() : "function"; - if (type != "function") continue; - if (!toolEl.TryGetProperty("function", out var fnEl)) continue; + if (!IsFunctionTool(toolEl)) continue; + if (!TryGetObject(toolEl, "function", out var fnEl)) continue; - var tf = ParseFunction(fnEl); - if (tf != null) - tools.Add(tf); + tools.Add(ParseFunction(fnEl)); } return tools.Count > 0 ? tools : null; } @@ -67,53 +84,179 @@ public static List ParseOpenAI(JsonElement body) /// public static List ParseOpenAIResponses(JsonElement body) { - if (!body.TryGetProperty("tools", out var toolsEl) || toolsEl.ValueKind != JsonValueKind.Array) + if (!TryGetArray(body, "tools", out var toolsEl)) return null; var tools = new List(); foreach (var toolEl in toolsEl.EnumerateArray()) { - string type = toolEl.TryGetProperty("type", out var t) ? t.GetString() : "function"; - if (type != "function") continue; + if (!IsFunctionTool(toolEl)) continue; - var tf = ParseFunction(toolEl); - if (tf != null) - tools.Add(tf); + tools.Add(ParseFunction(toolEl)); } return tools.Count > 0 ? tools : null; } + /// + /// An OpenAI tools entry names its kind in type; anything else — + /// the Responses API also carries server-side built-ins such as + /// web_search — is not a function we can offer the model. A + /// missing or null type means "function", which is what every + /// Chat Completions client intends, while a type that is not a + /// string at all is not a function declaration. + /// + private static bool IsFunctionTool(JsonElement toolEl) + { + if (toolEl.ValueKind != JsonValueKind.Object) + return false; + if (!toolEl.TryGetProperty("type", out var typeEl)) + return true; + + return typeEl.ValueKind switch + { + JsonValueKind.Null => true, + JsonValueKind.String => typeEl.GetString() == "function", + _ => false, + }; + } + + /// + /// Parse one function declaration — {name, description, parameters}, + /// where parameters is a JSON Schema object. Never returns null. + /// private static ToolFunction ParseFunction(JsonElement fnEl) { var tf = new ToolFunction { - Name = fnEl.TryGetProperty("name", out var n) ? n.GetString() : "", - Description = fnEl.TryGetProperty("description", out var d) ? d.GetString() : "" + Name = ReadString(fnEl, "name") ?? string.Empty, + Description = ReadString(fnEl, "description") ?? string.Empty }; - if (fnEl.TryGetProperty("parameters", out var paramsEl)) + // "parameters": null on an argument-less tool is common, and so is + // omitting it entirely; both leave the defaults from ToolFunction. + if (!TryGetObject(fnEl, "parameters", out var paramsEl)) + return tf; + + if (TryGetObject(paramsEl, "properties", out var propsEl)) { - if (paramsEl.TryGetProperty("properties", out var propsEl) && - propsEl.ValueKind == JsonValueKind.Object) + tf.Parameters = new Dictionary(); + foreach (var prop in propsEl.EnumerateObject()) + tf.Parameters[prop.Name] = ParseParameter(prop.Value); + } + + if (TryGetArray(paramsEl, "required", out var reqEl)) + { + // "required" lists property names, so a non-string entry names + // nothing and is dropped rather than carried through as a null. + var required = new List(); + foreach (var item in reqEl.EnumerateArray()) { - tf.Parameters = new Dictionary(); - foreach (var prop in propsEl.EnumerateObject()) - { - var tp = new ToolParameter - { - Type = prop.Value.TryGetProperty("type", out var pt) ? pt.GetString() : "string", - Description = prop.Value.TryGetProperty("description", out var pd) ? pd.GetString() : null - }; - if (prop.Value.TryGetProperty("enum", out var enumEl) && enumEl.ValueKind == JsonValueKind.Array) - tp.Enum = enumEl.EnumerateArray().Select(e => e.GetString()).ToList(); - tf.Parameters[prop.Name] = tp; - } + if (item.ValueKind == JsonValueKind.String) + required.Add(item.GetString()); } - if (paramsEl.TryGetProperty("required", out var reqEl) && reqEl.ValueKind == JsonValueKind.Array) - tf.Required = reqEl.EnumerateArray().Select(e => e.GetString()).ToList(); + tf.Required = required; } return tf; } + + /// + /// Parse one property schema. An absent type keeps the historical + /// "string" default; a schema that is not even an object (JSON + /// Schema allows a bare true/false in a property slot) + /// yields that same untyped default. + /// + private static ToolParameter ParseParameter(JsonElement schema) + { + var tp = new ToolParameter + { + Type = ReadSchemaType(schema) ?? "string", + Description = ReadString(schema, "description") ?? string.Empty + }; + + if (TryGetArray(schema, "enum", out var enumEl)) + { + // ToolParameter.Enum is a list of strings, but the schema's values + // need not be. Render a non-string member as its raw JSON text so + // it round-trips exactly when the prompt renderers re-serialize + // the schema: 1 stays 1 and true stays true. JsonElement.ToString() + // is the wrong tool here — it renders booleans in .NET's casing + // ("True") and null as an empty string. + var values = new List(); + foreach (var item in enumEl.EnumerateArray()) + values.Add(item.ValueKind == JsonValueKind.String ? item.GetString() : item.GetRawText()); + tp.Enum = values; + } + + return tp; + } + + /// + /// Read a property schema's type. JSON Schema allows a union — + /// "type": ["string", "null"] is the standard spelling of a + /// nullable field — while holds a single + /// name, because it is re-emitted into the prompt as a JSON Schema string + /// and switched on when rendering Harmony's TypeScript tool namespace. So + /// keep the first real type and drop the "null" member, whose + /// meaning the required list already carries. + /// + private static string ReadSchemaType(JsonElement schema) + { + if (schema.ValueKind != JsonValueKind.Object || !schema.TryGetProperty("type", out var typeEl)) + return null; + + if (typeEl.ValueKind == JsonValueKind.String) + return typeEl.GetString(); + + if (typeEl.ValueKind == JsonValueKind.Array) + { + string first = null; + foreach (var item in typeEl.EnumerateArray()) + { + if (item.ValueKind != JsonValueKind.String) + continue; + string name = item.GetString(); + first ??= name; + if (name != "null") + return name; + } + return first; + } + + return null; + } + + /// Read as a string, or null unless it is one. + private static string ReadString(JsonElement obj, string name) + => obj.ValueKind == JsonValueKind.Object + && obj.TryGetProperty(name, out var value) + && value.ValueKind == JsonValueKind.String + ? value.GetString() + : null; + + private static bool TryGetObject(JsonElement obj, string name, out JsonElement value) + => TryGetOfKind(obj, name, JsonValueKind.Object, out value); + + private static bool TryGetArray(JsonElement obj, string name, out JsonElement value) + => TryGetOfKind(obj, name, JsonValueKind.Array, out value); + + /// + /// is + /// itself only safe on an object — it throws on any other kind — so the + /// container is checked before the member. + /// + private static bool TryGetOfKind(JsonElement obj, string name, JsonValueKind kind, out JsonElement value) + { + value = default; + if (obj.ValueKind != JsonValueKind.Object + || !obj.TryGetProperty(name, out var found) + || found.ValueKind != kind) + { + return false; + } + + value = found; + return true; + } } }