Disallow null optional GenAI JSON fields - #454
danielpolimac wants to merge 3 commits into
Conversation
Pull request dashboard statusWaiting on the author · refreshed 2026-09-17 08:01 UTC Respond to 2 review items (e.g. link a commit, explain why not, ask a follow-up): Status above doesn't look right?
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the GenAI JSON schemas (and the non-normative Pydantic models used to generate them) to disallow null values for fields that are intended to be optional (omittable).
Changes:
- Removed
anyOf: [{type: ...}, {type: null}]unions from multiple GenAI JSON schema properties and replaced them with non-nulltypedeclarations. - Updated non-normative Pydantic models to use non-optional types (e.g.,
strinstead ofOptional[str]) for the same fields. - Added a changelog entry describing the behavior change.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| model/gen-ai/gen-ai-tool-definitions.json | Removes nullable unions for description / parameters in tool definitions. |
| model/gen-ai/gen-ai-retrieval-documents.json | Removes nullable unions for retrieval doc id and score. |
| model/gen-ai/gen-ai-output-messages.json | Removes nullable unions across multiple message-part fields (e.g., mime_type, ids, names). |
| model/gen-ai/gen-ai-memory-records.json | Removes nullable unions for memory record id / metadata / score. |
| model/gen-ai/gen-ai-input-messages.json | Removes nullable unions across multiple input message-part fields (e.g., mime_type, ids, names). |
| docs/gen-ai/non-normative/models.py | Aligns Pydantic model field types with the non-nullable schema intent. |
| changelog.d/+.bugfix.2.md | Documents the change in schema nullability. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "default": null, | ||
| "description": "The description of the tool. Since this attribute could be large, it's NOT RECOMMENDED to be populated by default. Instrumentations MAY provide a way to enable populating this property.", | ||
| "title": "Description" | ||
| "title": "Description", | ||
| "type": "string" | ||
| }, | ||
| "parameters": { | ||
| "anyOf": [ | ||
| { | ||
| "$ref": "http://json-schema.org/draft-07/schema#" | ||
| }, | ||
| { | ||
| "type": "null" | ||
| } | ||
| ], | ||
| "default": null, |
There was a problem hiding this comment.
Addressed in 136ff7e. Optional non-nullable fields now omit the generated default: null annotation.
| "default": null, | ||
| "description": "A unique identifier for the document.", | ||
| "title": "Id" | ||
| "title": "Id", | ||
| "type": "string" | ||
| }, | ||
| "score": { | ||
| "anyOf": [ | ||
| { | ||
| "type": "number" | ||
| }, | ||
| { | ||
| "type": "null" | ||
| } | ||
| ], | ||
| "default": null, | ||
| "description": "The relevance score of the document.", | ||
| "title": "Score" | ||
| "title": "Score", | ||
| "type": "number" |
There was a problem hiding this comment.
Addressed in 136ff7e. The generated retrieval-document fields are omittable, non-nullable, and no longer advertise a null default.
| "title": "Id", | ||
| "type": "string" | ||
| }, | ||
| "metadata": { | ||
| "anyOf": [ | ||
| { | ||
| "additionalProperties": true, | ||
| "type": "object" | ||
| }, | ||
| { | ||
| "type": "null" | ||
| } | ||
| ], | ||
| "additionalProperties": true, | ||
| "default": null, | ||
| "description": "Provider-specific metadata associated with the memory record.", | ||
| "title": "Metadata" | ||
| "title": "Metadata", | ||
| "type": "object" | ||
| }, | ||
| "score": { | ||
| "anyOf": [ | ||
| { | ||
| "type": "number" | ||
| }, | ||
| { | ||
| "type": "null" | ||
| } | ||
| ], | ||
| "default": null, | ||
| "description": "The relevance score of the memory record when populated on search results.", | ||
| "title": "Score" | ||
| "title": "Score", | ||
| "type": "number" |
There was a problem hiding this comment.
Addressed in 136ff7e. The generated memory-record fields are omittable, non-nullable, and no longer advertise a null default.
| type: Literal["function"] = Field(description="The type of the tool.") | ||
| description: Optional[str] = Field( | ||
| description: str = Field( | ||
| default=None, | ||
| description=( | ||
| "The description of the tool. " | ||
| "Since this attribute could be large, it's NOT RECOMMENDED to be populated by default. " | ||
| "Instrumentations MAY provide a way to enable populating this property." | ||
| ), | ||
| ) | ||
| parameters: Optional[JsonSchemaDraft7Dict] = Field( | ||
| parameters: JsonSchemaDraft7Dict = Field( | ||
| default=None, | ||
| description=( |
There was a problem hiding this comment.
Addressed in 136ff7e. Added a shared omittable_field helper that preserves the runtime default while omitting default: null from the generated schema, keeping the omittable/non-nullable contract consistent.
| from pydantic_core import core_schema | ||
|
|
||
|
|
||
| def omittable_field(**kwargs: Any): |
There was a problem hiding this comment.
Have you explored MISSING / NotRequired as suggested in #53? That is the part of the problem this helper leaves open: it changes what the schema says without changing what the models do, so the annotation is now false and the round trip is broken.
ToolCallRequestPart(type="tool_call", name="x").model_dump_json()
# {"type":"tool_call","id":null,"name":"x","arguments":null} <- rejected by the new schemaMISSING (pydantic 2.13) covers serialization and deserialization together - omitted on dump, rejected on parse, and the generated schema is the one this PR wants:
from pydantic.experimental.missing_sentinel import MISSING
id: str | MISSING = Field(MISSING, description="Unique identifier for the tool call.")There was a problem hiding this comment.
Addressed in de056bd. The models now use Pydantic MISSING so unavailable fields are omitted during serialization, while explicit null values remain representable where captured.
| type: Literal["function"] = Field(description="The type of the tool.") | ||
| description: Optional[str] = Field( | ||
| default=None, | ||
| description: str = omittable_field( |
There was a problem hiding this comment.
Too strict here. Omission of description/parameters already has a defined meaning in this convention - capture is opt-in, so absent means "not collected". A provider-reported null means something different: the tool has no description / takes no parameters. Banning null collapses the two.
It also pushes work onto every producer: SDK objects carry these as optional and serialize to null. The repo's own scenario does it - reference/scenarios/openai-agents/scenario.py:80 emits "description": t.description, which is None for a tool without a docstring.
Either keep null allowed on these two and document what it means, or state that instrumentations MUST drop null before emitting.
There was a problem hiding this comment.
Addressed in de056bd. Tool descriptions and parameters are now typed as value, null, or MISSING; their descriptions state the distinct capture semantics.
| ) | ||
| name: str = Field(description="Name of the tool.") | ||
| arguments: Any = Field(default=None, description="Arguments for the tool call.") | ||
| arguments: Any = omittable_field(description="Arguments for the tool call.") |
There was a problem hiding this comment.
arguments is Any, so "arguments": null stays valid while "id": null becomes invalid - and this is the field where the distinction actually carries information (arguments not captured vs. the call genuinely had null arguments). Same for response and memory content.
The PR makes omission normative without saying what it means. Spell out, for each of these fields, what absent means and what null means, otherwise consumers still can't rely on either.
There was a problem hiding this comment.
Addressed in de056bd. Arguments uses MISSING, and the model descriptions distinguish omission from captured null for arguments, tool responses, and memory content.
Motivation
Generated GenAI JSON schemas currently accept both a missing optional field and an explicit
null. Consumers cannot distinguish omission from an explicit null value.Changes
Validation
make generate-allmake check-policiesidis accepted.idis rejected.Fixes #53