-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix(llm): apply tool defaults across all schema shapes; keep response formats sentinel-free #6444
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 10 commits
2ba5b76
80a6538
80a0346
4fcf683
f414377
1eca873
47625b5
7857497
2b1e042
8d34c09
574d783
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,13 +7,29 @@ | |
| _T = TypeVar("_T") | ||
|
|
||
|
|
||
| def to_strict_json_schema(model: type[BaseModel] | TypeAdapter[Any]) -> dict[str, Any]: | ||
| def to_strict_json_schema( | ||
| model: type[BaseModel] | TypeAdapter[Any], | ||
| *, | ||
| null_sentinel_for_defaults: bool = False, | ||
| ) -> dict[str, Any]: | ||
| """Convert a model's JSON schema to the strict dialect the OpenAI API expects. | ||
|
|
||
| Strict mode doesn't support the ``default`` keyword, so defaults are always | ||
| removed. With ``null_sentinel_for_defaults``, a defaulted field additionally | ||
| accepts ``null`` as a sentinel meaning "use the Python default" — only enable | ||
| this when the decode side resolves the sentinel before validation, as the tool | ||
| pipeline does in ``_prepare_function_arguments``. Without it, the schema stays | ||
| faithful: defaulted fields remain required and non-nullable, and the raw | ||
| payload validates directly with pydantic. | ||
| """ | ||
| if isinstance(model, TypeAdapter): | ||
| schema = model.json_schema() | ||
| else: | ||
| schema = model.model_json_schema() | ||
|
|
||
| return _ensure_strict_json_schema(schema, path=(), root=schema) | ||
| return _ensure_strict_json_schema( | ||
| schema, path=(), root=schema, null_sentinel_for_defaults=null_sentinel_for_defaults | ||
| ) | ||
|
|
||
|
|
||
| # from https://platform.openai.com/docs/guides/function-calling?api-mode=responses&strict-mode=disabled#strict-mode | ||
|
|
@@ -34,6 +50,7 @@ def _ensure_strict_json_schema( | |
| *, | ||
| path: tuple[str, ...], | ||
| root: dict[str, object], | ||
| null_sentinel_for_defaults: bool, | ||
| ) -> dict[str, Any]: | ||
| """Mutates the given JSON schema to ensure it conforms to the `strict` standard | ||
| that the API expects. | ||
|
|
@@ -44,7 +61,12 @@ def _ensure_strict_json_schema( | |
| defs = json_schema.get("$defs") | ||
| if is_dict(defs): | ||
| for def_name, def_schema in defs.items(): | ||
| _ensure_strict_json_schema(def_schema, path=(*path, "$defs", def_name), root=root) | ||
| _ensure_strict_json_schema( | ||
| def_schema, | ||
| path=(*path, "$defs", def_name), | ||
| root=root, | ||
| null_sentinel_for_defaults=null_sentinel_for_defaults, | ||
| ) | ||
|
|
||
| definitions = json_schema.get("definitions") | ||
| if is_dict(definitions): | ||
|
|
@@ -53,6 +75,7 @@ def _ensure_strict_json_schema( | |
| definition_schema, | ||
| path=(*path, "definitions", definition_name), | ||
| root=root, | ||
| null_sentinel_for_defaults=null_sentinel_for_defaults, | ||
| ) | ||
|
|
||
| typ = json_schema.get("type") | ||
|
|
@@ -65,15 +88,25 @@ def _ensure_strict_json_schema( | |
| if is_dict(properties) and properties: | ||
| json_schema["required"] = list(properties.keys()) | ||
| json_schema["properties"] = { | ||
| key: _ensure_strict_json_schema(prop_schema, path=(*path, "properties", key), root=root) | ||
| key: _ensure_strict_json_schema( | ||
| prop_schema, | ||
| path=(*path, "properties", key), | ||
| root=root, | ||
| null_sentinel_for_defaults=null_sentinel_for_defaults, | ||
| ) | ||
| for key, prop_schema in properties.items() | ||
| } | ||
|
|
||
| # arrays | ||
| # { 'type': 'array', 'items': {...} } | ||
| items = json_schema.get("items") | ||
| if is_dict(items): | ||
| json_schema["items"] = _ensure_strict_json_schema(items, path=(*path, "items"), root=root) | ||
| json_schema["items"] = _ensure_strict_json_schema( | ||
| items, | ||
| path=(*path, "items"), | ||
| root=root, | ||
| null_sentinel_for_defaults=null_sentinel_for_defaults, | ||
| ) | ||
|
|
||
| # unions (anyOf / oneOf) | ||
| # Strip empty schema objects ({}) — they are JSON Schema's identity element | ||
|
|
@@ -88,13 +121,23 @@ def _ensure_strict_json_schema( | |
| variants = [v for v in variants if v != {}] | ||
| if len(variants) == 1: | ||
| json_schema.update( | ||
| _ensure_strict_json_schema(variants[0], path=(*path, union_key, "0"), root=root) | ||
| _ensure_strict_json_schema( | ||
| variants[0], | ||
| path=(*path, union_key, "0"), | ||
| root=root, | ||
| null_sentinel_for_defaults=null_sentinel_for_defaults, | ||
| ) | ||
| ) | ||
| json_schema.pop(union_key, None) | ||
| elif len(variants) >= 2: | ||
| json_schema.pop(union_key, None) | ||
| json_schema["anyOf"] = [ | ||
| _ensure_strict_json_schema(variant, path=(*path, "anyOf", str(i)), root=root) | ||
| _ensure_strict_json_schema( | ||
| variant, | ||
| path=(*path, "anyOf", str(i)), | ||
| root=root, | ||
| null_sentinel_for_defaults=null_sentinel_for_defaults, | ||
| ) | ||
| for i, variant in enumerate(variants) | ||
| ] | ||
| else: | ||
|
|
@@ -105,34 +148,39 @@ def _ensure_strict_json_schema( | |
| if is_list(all_of): | ||
| if len(all_of) == 1: | ||
| json_schema.update( | ||
| _ensure_strict_json_schema(all_of[0], path=(*path, "allOf", "0"), root=root) | ||
| _ensure_strict_json_schema( | ||
| all_of[0], | ||
| path=(*path, "allOf", "0"), | ||
| root=root, | ||
| null_sentinel_for_defaults=null_sentinel_for_defaults, | ||
| ) | ||
| ) | ||
| json_schema.pop("allOf") | ||
| else: | ||
| json_schema["allOf"] = [ | ||
| _ensure_strict_json_schema(entry, path=(*path, "allOf", str(i)), root=root) | ||
| _ensure_strict_json_schema( | ||
| entry, | ||
| path=(*path, "allOf", str(i)), | ||
| root=root, | ||
| null_sentinel_for_defaults=null_sentinel_for_defaults, | ||
| ) | ||
| for i, entry in enumerate(all_of) | ||
| ] | ||
|
|
||
| # popped before the default handling below so a wrapped schema doesn't retain them | ||
| json_schema.pop("title", None) | ||
| json_schema.pop("discriminator", None) | ||
|
|
||
| # strict mode doesn't support default | ||
| if "default" in json_schema: | ||
| json_schema.pop("default", None) | ||
|
|
||
| # Treat any parameter with a default value as optional. If the parameter’s type doesn't | ||
| # support None, the default will be used instead. | ||
| t = json_schema.get("type") | ||
| if isinstance(t, str): | ||
| json_schema["type"] = [t, "null"] | ||
|
|
||
| elif isinstance(t, list): | ||
| types = t.copy() | ||
| if "null" not in types: | ||
| types.append("null") | ||
|
|
||
| json_schema["type"] = types | ||
|
Comment on lines
-121
to
-132
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you verify it doesn't break existing behaviour? I think the reasoning behind that is that users using default values on parameters don't want to require the LLMs to generate a value for it
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated in 80a6538. I kept the existing default-as-null schema behavior, so the LLM is still not required to generate a value it cannot know. The new validate_response_format helper recursively replaces null with the Pydantic default before validation, while preserving null for fields whose declared type is actually nullable. The focused response-format and tool suites pass (101 tests).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry this was an LLM comment, after this comment was sent, I added to claude code a manual guard to never autosend comments without human approval. Me and theo chatted offline. The idea that we agreed on is that we will make the schema for defaults be type | null and then if the model gives null then we will put in the default value.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The intent is clear, thanks! My comment was about P.S. I should add such guard as well — Claude has also needlessly commented once on my behalf.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right. This PR was hanging on the assumption that people would call a different function to normalize the nulls out of their structured outputs into the defaults. We no longer do that. |
||
|
|
||
| json_schema.pop("title", None) | ||
| json_schema.pop("discriminator", None) | ||
| # Strict schemas require all fields, but LLMs cannot see or apply Python defaults. | ||
| # For tool schemas, let the field additionally accept null as a sentinel; tool | ||
| # preparation (_prepare_function_arguments) swaps a returned null back for the | ||
| # field's default. Response formats stay faithful: the model must produce a value. | ||
| if null_sentinel_for_defaults: | ||
| _add_null_sentinel(json_schema) | ||
|
|
||
| # we can't use `$ref`s if there are also other properties defined, e.g. | ||
| # `{"$ref": "...", "description": "my description"}` | ||
|
|
@@ -154,7 +202,9 @@ def _ensure_strict_json_schema( | |
| json_schema.pop("$ref") | ||
| # Since the schema expanded from `$ref` might not have `additionalProperties: false` applied, # noqa: E501 | ||
| # we call `_ensure_strict_json_schema` again to fix the inlined schema and ensure it's valid. # noqa: E501 | ||
| return _ensure_strict_json_schema(json_schema, path=path, root=root) | ||
| return _ensure_strict_json_schema( | ||
| json_schema, path=path, root=root, null_sentinel_for_defaults=null_sentinel_for_defaults | ||
| ) | ||
|
|
||
| # simplify nullable unions (“anyOf” or “oneOf”) | ||
| for union_key in ("anyOf", "oneOf"): | ||
|
|
@@ -184,6 +234,43 @@ def _ensure_strict_json_schema( | |
| return json_schema | ||
|
|
||
|
|
||
| def _add_null_sentinel(json_schema: dict[str, Any]) -> None: | ||
| """Make a defaulted field additionally accept ``null``, preserving its original | ||
| constraints. | ||
|
|
||
| Appending ``"null"`` to a bare ``type`` only works when the schema has a direct type | ||
| and no value constraints. Literals (``const``/``enum``), unions (``anyOf``), and | ||
| ``$ref``-ed models need a full null branch instead, otherwise the wire schema either | ||
| contradicts itself or never advertises the sentinel at all. | ||
| """ | ||
| typ = json_schema.get("type") | ||
| if typ == "null" or (is_list(typ) and "null" in typ): | ||
| return # already nullable via type | ||
|
|
||
| for union_key in ("anyOf", "oneOf"): | ||
| variants = json_schema.get(union_key) | ||
| if is_list(variants): | ||
| if not any(variant == {"type": "null"} for variant in variants): | ||
| variants.append({"type": "null"}) | ||
| return | ||
|
|
||
| has_value_constraint = "const" in json_schema or "enum" in json_schema | ||
| if isinstance(typ, str) and not has_value_constraint: | ||
| json_schema["type"] = [typ, "null"] | ||
| return | ||
| if is_list(typ) and not has_value_constraint: | ||
| json_schema["type"] = [*typ, "null"] | ||
| return | ||
|
|
||
| # const / enum / $ref / allOf: wrap so null is a valid instance without violating | ||
| # the original constraints. An empty schema already accepts null — leave it. | ||
| inner = dict(json_schema) | ||
| if not inner: | ||
| return | ||
| json_schema.clear() | ||
| json_schema["anyOf"] = [inner, {"type": "null"}] | ||
|
u9g marked this conversation as resolved.
Outdated
|
||
|
|
||
|
|
||
| def resolve_ref(*, root: dict[str, object], ref: str) -> object: | ||
| if not ref.startswith("#/"): | ||
| raise ValueError(f"Unexpected $ref format {ref!r}; Does not start with #/") | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.