fix: simplify donut chart schema for OpenAI compatibility#8182
fix: simplify donut chart schema for OpenAI compatibility#8182xr843 wants to merge 2 commits intoblock:mainfrom
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: fb46c429d4
ℹ️ 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".
| let count = data | ||
| .get("data") | ||
| .and_then(|d| d.as_array()) | ||
| .map(|a| a.len()) |
There was a problem hiding this comment.
validate_data_param returns the value of params["data"] (for this tool, an array), but this code looks for a nested "data" field again, so count is always 1. As a result, calls with multiple charts still return the fallback text "donut/pie chart: 1 chart(s)", which is incorrect for clients that rely on result.content rather than the structured payload.
Useful? React with 👍 / 👎.
Replace untagged enums in DonutDataItem and DonutChartData with plain structs and Vec to produce a JSON Schema that OpenAI-compatible endpoints accept for function calling. - DonutDataItem: changed from untagged enum (Number|LabeledValue) to a struct with required label+value fields - DonutChartData: removed untagged enum (Single|Multiple), RenderDonutParams now takes Vec<SingleDonutChart> directly - Removed the now-unnecessary labels field from SingleDonutChart since labels are always part of each DonutDataItem - Updated tool description examples to reflect the new schema - Updated and fixed tests to match the simplified schema Fixes block#8173 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: Tim Ren <[email protected]>
xr843
force-pushed
the
fix/autovisualiser-donut-schema
branch
from
fb46c42 to
03bc9e9
Compare
|
Thanks for the PR — the core idea of simplifying the donut schema to avoid A few things to address before we can move forward:
|
DOsinga
added
the
needs_human
- Fix donut text_fallback: `data` is already the array extracted by
`validate_data_param`, so use `data.as_array()` directly instead of
`data.get("data")` which always fell back to 1.
- Remove `labeled_values_as_single_chart` test that duplicates
`labeled_values_single_chart`.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
Thanks for the detailed review @DOsinga! All three items addressed:
Apologies for the multi-PR approach — lesson learned, will follow the one-PR-at-a-time guideline going forward! |
2 participants
Summary
Fixes #8173
The
autovisualiser__render_donuttool schema was rejected by OpenAI-compatible endpoints because it used#[serde(untagged)]enums, which generateanyOf/oneOfconstructs in JSON Schema that OpenAI's function calling validation does not support.This PR replaces the two problematic untagged enums with simpler, flat types:
DonutDataItem: Changed from an untagged enum (Number(f64)|LabeledValue { label, value }) to a plain struct with requiredlabelandvaluefields. This produces a single object schema instead ofanyOf.DonutChartData: Removed the untagged enum (Single(SingleDonutChart)|Multiple(Vec<SingleDonutChart>)).RenderDonutParams.datanow takesVec<SingleDonutChart>directly — a single chart is represented as a one-element array.labelsfield fromSingleDonutChartsince labels are now always embedded in eachDonutDataItem.The HTML template (
donut_template.html) already handles both object-style values and arrays, so no template changes are needed.Test plan
test_render_donutanddonut_rejects_empty_valuestests passanyOf/oneOffor the donut toolgoose run --provider openai --model gpt-4o-mini --with-builtin autovisualiser --text "Reply with exactly OK"should not return schema validation error🤖 Generated with Claude Code