Make jsonformat strict schemas OpenAI-valid by requiring all properties - #689
Make jsonformat strict schemas OpenAI-valid by requiring all properties#689PratikDhanave wants to merge 2 commits into
Conversation
56c5742 to
390f45b
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the jsonformat response-format schema generation so that when Strict: true schemas are sent to OpenAI, every object lists all keys in properties within its required array (matching OpenAI strict structured-output requirements). It also preserves local Go-side validation semantics by resolving/validating against a relaxed clone of the schema.
Changes:
- Rewrite inferred JSON Schemas in
jsonformatso every object requires all properties; properties that were previously optional are made nullable. - Relax strict-only requirements for local validation by cloning the schema and dropping nullable properties from
requiredbefore resolving. - Add a test asserting
requiredcovers allpropertiesfor nested objects and array elements under strict mode.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| agent/format/jsonformat/jsonformat.go | Adds strict-schema rewrite (require-all-properties + nullable optional props) and relaxed schema resolution for local validation. |
| agent/format/jsonformat/jsonformat_test.go | Adds coverage to ensure OpenAI strict schemas have required covering all properties for nested/object/array-element schemas. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // requiredKeys returns the "required" and "properties" key sets of the object | ||
| // schema at path (a sequence of property names to descend through). | ||
| func objectSchema(t *testing.T, format agent.ResponseFormat, path ...string) *jsonschema.Schema { |
There was a problem hiding this comment.
Good catch — fixed in b0a8e9a: reworded the comment to describe what objectSchema actually does (returns the object schema reached by descending through path, stepping into array item schemas), dropping the stale requiredKeys/key-set wording.
| // makeNullable marks s as accepting the JSON null value in addition to its | ||
| // existing type(s), so an optional property can be omitted by returning null. |
There was a problem hiding this comment.
Good catch — fixed in b0a8e9a: reworded to clarify that in a strict schema every property stays required, and an originally optional property signals "no value" by being null rather than by being omitted.
This comment has been minimized.
This comment has been minimized.
Parity Review — No Issues FoundThis PR makes Scope: Cross-repo parity: The Local validation: The PR preserves local
✅ Parity verdict: The change aligns Go behaviour with .NET and Python. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
OpenAI strict structured outputs require every key in an object's
"properties" to also appear in "required". The jsonschema-go inference
omits omitempty/omitzero fields from "required", so an inferred schema
such as {properties:{name,age}, required:[name], additionalProperties:false}
is rejected by OpenAI with HTTP 400.
Rewrite the schema stored in the response format so every object requires
all of its properties, expressing optionality via nullable types rather
than omission. This mirrors .NET's
AIJsonSchemaCreateOptions.RequireAllProperties = true.
The strict form is only correct for transmission; local validation of Go
values still omits omitempty fields, so resolvedSchema validates against a
relaxed clone that drops nullable (optional) properties from "required".
This keeps tool-argument and structured-output validation behavior intact.
b0a8e9a to
dae1ad1
Compare
What
jsonformatmarks every response format asStrict: true, and the OpenAI providers forward that schema verbatim withstrict: true. Butjsonschema-goinference only appends a field torequiredwhen it is notomitempty/omitzero, while still adding it topropertiesand settingadditionalProperties: false.So a struct like:
produces
properties: {name, age, email}butrequired: [name, age]. OpenAI strict structured outputs require every key inpropertiesto also appear inrequired, so this schema is rejected with HTTP 400.This change rewrites the stored schema so every object requires all of its properties, expressing optionality via nullable types instead of omission.
Why (parity)
This mirrors .NET's
AIJsonSchemaCreateOptions.RequireAllProperties = true, which the .NET/Python SDKs apply when emitting strict schemas: all properties become required and optional ones are made nullable rather than omitted. Doing the transform once injsonformat(rather than in each provider) keeps every provider that readsResponseFormat.Schemaconsistent.How it stays correct locally
The strict form is only appropriate for what is sent to the model. Locally,
Format.Marshal/Unmarshal/Normalizevalidate Go values (including tool arguments and tool outputs) that legitimately omitomitemptyfields. To avoid breaking that,resolvedSchemavalidates against a relaxed deep-clone that drops the nullable (optional) properties back out ofrequired; the strict schema itself is left untouched for transmission. Model responses under strict mode contain every key, so they still validate.Tests
TestStrictRequiresAllPropertiesin the canonicaljsonformat_test.go: builds aResponseFormatfor a struct withomitemptyfields (including a nested object and an array element) and asserts each object'srequiredcovers every key inproperties. It fails before this change (requiredis missing the optional keys) and passes after.TestNormalizeAppliesDefaults, encoding round-trips) and the repo-wide tool-argument/structured-output tests continue to pass, confirming local validation still honors optionality.