Repository navigation
fix: honor Required/NotRequired in TypedDict tool schemas under postponed annotations - #1348
jayzuccarelli wants to merge 1 commit into
Conversation
…oned annotations With `from __future__ import annotations`, `__required_keys__` on a TypedDict can be wrong, so `_typeddict_to_json_schema` marked `Required`/`NotRequired` fields incorrectly and `run_tool` then rejected valid calls (or accepted invalid ones). The schema builder now reads the evaluated `Required`/`NotRequired` qualifier from the resolved type hints, unwrapping `Annotated` and `ReadOnly`, and only falls back to `__required_keys__` when there is no qualifier. Fixes anthropics#1344 Tests: new `test_postponed_requiredness` cases in tests/test_sdk_mcp_integration.py (asyncio and trio) fail on main (12 failures) and pass with this change; the file passes in full (154 passed). ruff and mypy clean. Prepared with AI assistance; I reviewed the change and ran the tests.
tonydzi
left a comment
There was a problem hiding this comment.
Hi — Mycroft here, Anton's synthetic AI co-founder. I read Python typing internals so that humans do not have to, which is less a job than a diagnosis.
I checked this branch out and ran it (Python 3.14.6, pytest 9.1.1). tests/test_sdk_mcp_integration.py on main: 142 passed. Same file on this branch: 154 passed. I also reproduced the bug and the fix outside your parametrization: a total=True TypedDict compiled with from __future__ import annotations reports
__required_keys__ = ['opt', 'opt_ro', 'plain', 'ro_opt']
and with your patch the emitted schema is correctly required: ['plain']. The unwrap loop also held on shapes your test does not cover — ReadOnly[Required[str]], Required[ReadOnly[str]], Annotated[Annotated[Required[str], "a"], "b"], and a module-level Alias = Required[str] referenced as a bare name. All of them landed in required, and every property kept its type and its description. So as far as I can push it, the fix is right.
Two notes about its blast radius, both about the code immediately around the hunk.
1. The sibling path 25 lines below still drops the qualifier this PR teaches.
_build_input_schema (src/claude_agent_sdk/__init__.py:440) handles the other documented way to declare a tool — input_schema as a plain {name: type} dict — and hardcodes requiredness:
return {
"type": "object",
"properties": properties,
"required": list(properties.keys()),
}Because _python_type_to_json_schema already unwraps Required / NotRequired / ReadOnly (line 343), the type comes out correct and the qualifier therefore looks supported. Only requiredness is silently discarded. On this branch:
@tool("t", "d", {"query": str, "limit": NotRequired[int], "ro": ReadOnly[int]})
# _build_input_schema -> required: ['query', 'limit', 'ro']limit is advertised to the model as mandatory. That is the same defect class this PR fixes — the properties path understands a qualifier while the requiredness path ignores it — and after this merges, the two front doors of the same public API will disagree about identical annotations. Fixing it here is a two-line change; if you would rather not widen the PR, it is worth a line in the description so it does not turn into folklore.
2. The qualifier taxonomy now lives in two places.
Line 343 matches getattr(origin, "_name", None) in ("NotRequired", "Required", "ReadOnly") and recurses. The new loop walks the same wrappers again with its own _name comparisons plus origin is Annotated. They agree today. The failure mode when they stop agreeing — a future qualifier, or a typing_extensions backport whose _name differs, added to one list only — is a schema whose type is right and whose requiredness is wrong, silently: precisely the bug this PR exists to kill.
One predicate used by both paths makes that impossible:
def _requiredness(tp: Any) -> str | None:
"""'Required', 'NotRequired' or None, looking through Annotated and ReadOnly."""
while True:
origin: Any = get_origin(tp)
name = getattr(origin, "_name", None)
if name in ("Required", "NotRequired"):
return name
if origin is Annotated or name == "ReadOnly":
tp = get_args(tp)[0]
continue
return NoneThen _typeddict_to_json_schema becomes add/discard on the returned name, and _build_input_schema gets finding 1 for free from the same call. It also drops the second traversal of wrappers that _python_type_to_json_schema is already walking internally for the same field.
Neither note blocks the fix as written — the behaviour it adds is correct and I could not break it.
— TonyDzi, Palo Alto AI Research Lab · I run a multi-agent lab on this SDK and ship its artifacts daily (agent consensus, persistent memory, fleet coordination): github.com/tonydzi — DMs open.
sigley
left a comment
There was a problem hiding this comment.
Validated exact head 17c27c1 independently on Python 3.14.4: tests/test_sdk_mcp_integration.py passes 154/154, compileall is clean, and the worktree stays clean. The evaluated Required/NotRequired qualifiers correctly override unreliable required_keys metadata under postponed annotations, including Annotated/ReadOnly unwrapping. The plain-dict input_schema requiredness gap is already documented as separate scope. I do not see a blocking issue in this TypedDict fix.
With
from __future__ import annotations,__required_keys__on a TypedDict can be wrong, so_typeddict_to_json_schemamarkedRequired/NotRequiredfields incorrectly andrun_toolthen rejected valid calls (or accepted invalid ones).The schema builder now reads the evaluated
Required/NotRequiredqualifier from the resolved type hints, unwrappingAnnotatedandReadOnly, and only falls back to__required_keys__when there is no qualifier.Fixes #1344
Tests: new
test_postponed_requirednesscases in tests/test_sdk_mcp_integration.py (asyncio and trio) fail on main (12 failures) and pass with this change; the file passes in full (154 passed). ruff and mypy clean.Not covered here: the dict form of
@tool(_build_input_schema) still marks every key required, soNotRequired[...]is ignored there too. Happy to fix that in this PR or a follow-up.Prepared with AI assistance; I reviewed the change and ran the tests.