Fall back rather than fail on non-Encodable response types - #770
Open
eb8680 wants to merge 3 commits into
Open
Conversation
jfeser
requested changes
Sep 2, 2026
jfeser
left a comment
jfeser
left a comment
Contributor
There was a problem hiding this comment.
Per chatgpt:
After this change, tools that take unecodable parameters will appear to be callable using json. Previously, the json encoding mode checked whether parameters are serializable and disabled the tool.
eb8680
added a commit
that referenced
this pull request
Sep 2, 2026
Review feedback on #770. The raise that `_NoEncoding` used to make was doing double duty: besides breaking #763, it was the signal `LexicalToolExtractor.call_assistant` probes for when deciding whether a tool can be offered under the JSON pathway. Removing it left a tool with an unencodable parameter advertised as callable, and every call to it fails to decode -- `InstanceOf` rejects whatever string the model sends -- so the model spends a turn per attempt on a tool that was previously, correctly, withheld. The refusal belongs one layer up rather than back in `Encodable`, which must keep producing something safe to hand to `completion`: a response type with no decoding still makes a request worth sending, because a final-answer tool can answer it, while a tool parameter with no decoding makes the tool useless. `_serialize_name_and_tool` now refuses to advertise such a tool, raising the same `PydanticSchemaGenerationError` that `_pydantic_type_operation` raises and that the probe already catches. Both callers get their pre-#763 behavior back: a lexically-discovered tool is skipped with a warning, an explicitly-passed one fails the request. Only parameters are checked -- a tool that *returns* an unencodable value is still callable, and its result reaches the model as text. Recognizing a refusal is `_UndecodableReturn`'s job, since it is already the type meaning "no direct reply can be decoded". It gains a `__schema_title__` that both refusing schemas carry, so `_is_decodable` can identify one without matching on prose that is free to change. `_is_decodable` asks the generated schema rather than the type. An encoding that supplies its own validation schema does not delegate inward, so a refusal nested in its arguments is never one the model is shown: `write_and_run_body` takes `SkillBody[[int], Interpretation]` and is asked for source, so it decodes whatever the skill returns. Reading the type instead finds that return type and withdraws the very tool the #763 redirect exists to reach. `test_json_mode_skips_unadvertisable_tool` is parametrized over both ways a parameter can fail to name something the model could send -- no schema at all, and a schema no reply satisfies. Only the second fails without this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
OK, it should be fixed |
eb8680
added a commit
that referenced
this pull request
Sep 2, 2026
The encoding registry was designed to be extensible so that a type the library serializes badly, or not at all, can be given an encoding without a library-level fix. That hook was only reachable as `TypeToPydanticType.register` on the internal handler class, so the escape hatch was effectively undiscoverable from the public interface. `register` is now a classmethod on `Encodable`, documented with a doctest that starts from the failure a user actually hits and ends with the type encoding, decoding and schematizing. It takes a `TypeForm` rather than a `type`, since the registry also accepts unions and typing special forms, and returns a decorator that preserves the function it decorates. The internal registrations keep calling `TypeToPydanticType.register`; the library registering into its own handler class is the right layering. The developer-facing pointer in `_NoEncoding`'s error message still names the internal class, deliberately: #770 deletes that block outright, so editing it here would buy nothing but a merge conflict. `Encodable` stays a `TYPE_CHECKING`-only alias, so `@Encodable.register` in typed code needs `# type: ignore[attr-defined]`, as the example shows. That is forced, not incidental: mypy resolves a name as either a generic alias (usable in annotations) or an object with attributes, never both. Replacing the alias with a class of any shape -- plain, generic, Protocol, or a `TypeForm`-typed metaclass `__getitem__` -- makes `register` check but breaks `field: Encodable[T]`, and mypy ignores `__class_getitem__` entirely, so no subscript signature can rescue it. Under that ignore the call is `Any`, so the signature documents rather than checks. Closes #769 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jfeser
pushed a commit
that referenced
this pull request
Sep 2, 2026
The encoding registry was designed to be extensible so that a type the library serializes badly, or not at all, can be given an encoding without a library-level fix. That hook was only reachable as `TypeToPydanticType.register` on the internal handler class, so the escape hatch was effectively undiscoverable from the public interface. `register` is now a classmethod on `Encodable`, documented with a doctest that starts from the failure a user actually hits and ends with the type encoding, decoding and schematizing. It takes a `TypeForm` rather than a `type`, since the registry also accepts unions and typing special forms, and returns a decorator that preserves the function it decorates. The internal registrations keep calling `TypeToPydanticType.register`; the library registering into its own handler class is the right layering. The developer-facing pointer in `_NoEncoding`'s error message still names the internal class, deliberately: #770 deletes that block outright, so editing it here would buy nothing but a merge conflict. `Encodable` stays a `TYPE_CHECKING`-only alias, so `@Encodable.register` in typed code needs `# type: ignore[attr-defined]`, as the example shows. That is forced, not incidental: mypy resolves a name as either a generic alias (usable in annotations) or an object with attributes, never both. Replacing the alias with a class of any shape -- plain, generic, Protocol, or a `TypeForm`-typed metaclass `__getitem__` -- makes `register` check but breaks `field: Encodable[T]`, and mypy ignores `__class_getitem__` entirely, so no subscript signature can rescue it. Under that ignore the call is `Any`, so the signature documents rather than checks. Closes #769 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
`Encodable[T]` carries one obligation: whatever it produces must be safe to hand to `completion`. `_NoEncoding` broke it. For a type with no decoding it raised `PydanticInvalidForJsonSchema` when asked for a *validation* schema, and nothing asks for one until litellm converts the response format -- on the request path, where the exception came back out as `APIConnectionError: OpenAIException`. So any `Skill` returning such a type (`-> Interpretation`, an arbitrary class, or a list/dataclass/tuple holding one) died while assembling its first request, naming neither the skill nor the annotation, even when the call was perfectly answerable: `write_and_run_body` advertises cleanly for these, since a return type reaches the model only through `_best_effort_schema`, which degrades. Refuse the way `_UndecodableReturn` already does for a return type left uninstantiated -- a strict-legal string schema that no reply satisfies, whose description redirects the model to a final-answer tool. Two situations with identical semantics no longer get opposite treatments. That leaves `_NoEncoding` with nothing to be. A mode-conditional schema is exactly what `WithJsonSchema(mode=...)` is, and the fallback already carried one for the serialization side, so the class goes and a second `WithJsonSchema` takes its place. `_UndecodableReturn` is untouched and does not merge with it: it is a *type* substituted into an annotation and checked by identity in `call_assistant`, standing in for a type never determined, so it maps to `Annotated[str, ...]` with no real value behind it. This annotates a real type that must keep serializing real values. The `InstanceOf` already in the chain does the rejecting, preserving the asymmetry that matters: a string from the model fails, a real Python value still validates, so the tool this redirects to can return one. A second symptom falls out. `_pydantic_type_tuple` builds its stand-in schema eagerly, in validation mode, at `Encodable[...]`-construction time, so `Encodable[tuple[int, Interpretation]]` could not previously be constructed at all -- breaking `call_tool` for a tool returning such a tuple in the *send* direction, where every part serializes fine. Nothing raises now, so the branch needed no change. `call_assistant` is unchanged; its `_UndecodableReturn`-and-no-tools check stays the only one there. Two existing tests pinned the raise and now pin the schema. New coverage: the response format converts for `completion` (bare, nested in a list, inside a tuple), a container with an unencodable element still serializes, and a Skill returning one answers end to end through `write_and_run_body`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review feedback on #770. The raise that `_NoEncoding` used to make was doing double duty: besides breaking #763, it was the signal `LexicalToolExtractor.call_assistant` probes for when deciding whether a tool can be offered under the JSON pathway. Removing it left a tool with an unencodable parameter advertised as callable, and every call to it fails to decode -- `InstanceOf` rejects whatever string the model sends -- so the model spends a turn per attempt on a tool that was previously, correctly, withheld. The refusal belongs one layer up rather than back in `Encodable`, which must keep producing something safe to hand to `completion`: a response type with no decoding still makes a request worth sending, because a final-answer tool can answer it, while a tool parameter with no decoding makes the tool useless. `_serialize_name_and_tool` now refuses to advertise such a tool, raising the same `PydanticSchemaGenerationError` that `_pydantic_type_operation` raises and that the probe already catches. Both callers get their pre-#763 behavior back: a lexically-discovered tool is skipped with a warning, an explicitly-passed one fails the request. Only parameters are checked -- a tool that *returns* an unencodable value is still callable, and its result reaches the model as text. Recognizing a refusal is `_UndecodableReturn`'s job, since it is already the type meaning "no direct reply can be decoded". It gains a `__schema_title__` that both refusing schemas carry, so `_is_decodable` can identify one without matching on prose that is free to change. `_is_decodable` asks the generated schema rather than the type. An encoding that supplies its own validation schema does not delegate inward, so a refusal nested in its arguments is never one the model is shown: `write_and_run_body` takes `SkillBody[[int], Interpretation]` and is asked for source, so it decodes whatever the skill returns. Reading the type instead finds that return type and withdraws the very tool the #763 redirect exists to reach. `test_json_mode_skips_unadvertisable_tool` is parametrized over both ways a parameter can fail to name something the model could send -- no schema at all, and a schema no reply satisfies. Only the second fails without this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
eb8680
force-pushed
the
worktree-issue-763-no-encoding-schema
branch
from
September 2, 2026 23:48
13a75ee to
c863012
Compare
eb8680
changed the base branch from
master
to
worktree-issue-761-recursive-schema-refs
September 2, 2026 23:49
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resovles #763
This PR prevents the failure in #763 by falling back to a trivial validation schema.
call_assistantwill still fail for such return types unlesstool_choice="required", but when that is set correctlySkillcalls with arbitrary return types should work as expected.