fix json parser - #155
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1c1e3f1a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| context.Response.StatusCode = malformed | ||
| ? StatusCodes.Status400BadRequest | ||
| : StatusCodes.Status500InternalServerError; |
There was a problem hiding this comment.
When Kestrel rejects a request body over the configured 500 MB limit, reading the body throws a BadHttpRequestException whose status is 413. The new catch intercepts that exception before Kestrel can handle it, IsMalformedRequest returns false, and these oversized client requests are therefore rewritten as 500 responses. Preserve the exception's 4xx status (and similarly other framework request errors) instead of classifying every non-JSON exception as an internal fault.
Useful? React with 👍 / 👎.
| foreach (var item in enumEl.EnumerateArray()) | ||
| values.Add(item.ValueKind == JsonValueKind.String ? item.GetString() : item.GetRawText()); | ||
| tp.Enum = values; |
There was a problem hiding this comment.
For a valid numeric, boolean, or null enum, this stores only the value's raw spelling in a List<string>, losing its JSON type. The downstream JSON-schema renderers in ChatTemplate.cs serialize that list as strings, so [0, 1] becomes ["0", "1"], while other renderers omit enums for non-string parameter types altogether; the model therefore receives a different constraint from the caller's schema. The analogous conversion in OutputParser.cs has the same issue, so the representation and renderers need to retain typed enum values rather than raw text.
Useful? React with 👍 / 👎.
| string name = item.GetString(); | ||
| first ??= name; | ||
| if (name != "null") | ||
| return name; |
There was a problem hiding this comment.
When a schema uses "type": ["string", "null"], returning only string removes the fact that an explicit JSON null is valid. The required list does not encode nullability—it only controls whether the property must be present—so this is especially wrong for a required nullable property, which is rendered to the model as accepting strings only. Preserve the union/null alternative in the tool representation and prompt renderers rather than discarding it; the duplicate reduction in OutputParser.cs should be corrected as well.
Useful? React with 👍 / 👎.
Engine comparison — TensorSharp vs llama.cpp (PR smoke)No report artifact was produced — the benchmark failed before generating results (see the workflow logs). |
1 participant
No description provided.