fix(lib): preserve custom tool calls in parse_chat_completion - #3505
fix(lib): preserve custom tool calls in parse_chat_completion#3505PranavMishra28 wants to merge 4 commits into
Conversation
|
@seratch would appreciate your eyes on this when you get a chance. small one: parse_chat_completion() drops GPT-5 custom tool calls on a custom-only turn (the raw completion keeps them, only .parse() loses them). the fix just appends the custom call instead of discarding it, mirroring how the else branch already preserves non-function calls. tests included, targeted at next. happy to adjust if you'd rather handle it a different way. |
317260c to
e67afa8
Compare
`parse_chat_completion` (behind `client.chat.completions.parse()` and the streaming `get_final_completion()`) logged a warning and then *dropped* every `custom`-type tool call, so a supported GPT-5 tool call the model made vanished from the parsed result (`tool_calls` could even come back `None`). The handling was also inconsistent: the trailing `else` branch already preserves any non-function tool call by appending it unchanged; only `custom` was special-cased to discard. Append the custom call the same way (it has no schema to parse `parsed_arguments` against, so it's surfaced as-is) instead of dropping it, and remove the now-inaccurate "Ignoring tool call" warning that fired on every custom call in normal use. Adds tests/lib/chat/test_parse_custom_tool_calls.py covering a custom-only and a mixed function+custom message; both fail before this change. Note: `ParsedChatCompletionMessage.tool_calls` is a generated type narrowed to `list[ParsedFunctionToolCall]`, so a custom call round-trips through attribute access and its own `model_dump()` but its `custom` payload is still dropped when the whole completion is serialized — same limitation the existing `else` branch already has for non-function calls. Fully fixing serialization needs the generated type widened, which is out of scope for a lib-only change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
c11f3c9 to
d0c07c8
Compare
|
@apcha-oai @jbeckwith-oai small correction on my end: this originally targeted The bug: One thing worth deciding before you spend review time: So: do you want this lib-only fix as-is, or should it be handled upstream in the Stainless spec instead? Happy to close this if it is the latter. |
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Blocking: this appends a known custom variant to a list and public field declared as list[ParsedFunctionToolCall]. Pyright reports an argument-type error at this append, and the mismatch causes Pydantic to serialize a custom call as only {id, type}, silently dropping custom.name and custom.input from ParsedChatCompletion.model_dump()/model_dump_json(). The new tests avoid that public path by casting to Any and dumping each element independently. Please widen the parsed tool-call type to a discriminated union of ParsedFunctionToolCall and ChatCompletionMessageCustomToolCall (including the local accumulator and the generated/source-of-truth definition so regeneration preserves it), then add regression assertions against serialization of the whole parsed completion. Also, the stated streaming behavior is not currently covered or functional: ChatCompletionChunk.ChoiceDeltaToolCall only accepts type='function', and validating a custom delta fails before get_final_completion() reaches this parser. Either extend the generated streaming delta union/accumulator and test sync+async final-completion paths, or scope this PR and its claims explicitly to non-streaming parse(). Direct parsing otherwise preserves the in-memory custom object as intended, and CodeQL/diff checks are clean.
Appending the custom tool call was not enough. `ParsedChatCompletionMessage`
narrowed `tool_calls` to `list[ParsedFunctionToolCall]`, and pydantic serializes
by the declared type, so dumping a whole parsed completion emitted
`{"id": ..., "type": "custom"}` with the `custom` payload silently gone. The
result did not even validate back into an equivalent model, so a caller
persisting and replaying a parsed completion lost the call.
`tool_calls` is now `list[ParsedChatCompletionMessageToolCallUnion]`, an
annotated discriminated union mirroring `ChatCompletionMessageToolCallUnion` on
the base class with the function member swapped for its parsed subclass. That
makes the annotation describe what the field actually holds, which is what
pydantic needs, and it lines the parsed message up with the raw one instead of
diverging from it.
Consequences of the wider type, all handled here rather than left to callers:
- The four `assert_never` exhaustiveness sites in the streaming accumulator now
have an explicit `custom` branch. A custom call carries no `parsed_arguments`
and the argument-delta/done events are function-tool specific, so each branch
is a documented no-op, but being explicit keeps exhaustiveness real: a third
tool-call type still trips `assert_never`.
- examples/parsing_tools.py narrows on `type` before reaching for `.function`,
which the raw completion has always required.
This also removes the pyright/mypy error the previous commit introduced at
_completions.py:133, where a custom call was appended to a list annotated
`list[ParsedFunctionToolCall]`.
Tests now assert on the whole-completion dump and on a validate round trip
rather than dumping each tool call on its own, which was the hole that let the
serialization loss through. All three fail without this change.
|
pushed a second commit, and it corrects two things i said earlier on this PR. the first fix wasn't complete. appending the custom call makes it show up on attribute access, but
i also said pyright was clean on the changed files. it wasn't: the first commit left an error at the wider type has two consequences and i handled both here instead of leaving them for callers. the four the tests were the reason i missed this. they dumped each tool call individually, which sidesteps the exact bug. they now assert on the whole-completion dump plus a validate round trip, and all three fail on the previous commit. verification: ruff check and format clean, pyright clean on all six files, mypy one error below @apcha-oai @jbeckwith-oai still happy to split the type change out if you'd rather keep this to |
The streaming half of the earlier claim was wrong. `ChoiceDeltaToolCall.type` is
`Optional[Literal["function"]]`, so a streamed custom tool call fails chunk
validation before `get_final_completion()` reaches `parse_chat_completion` at
all:
Input should be 'function' [type=literal_error, input_value='custom']
Extending that path means widening the generated chunk delta types plus the
streaming accumulator, which is a separate change and depends on the wire shape
the API actually emits for streamed custom calls.
Records the boundary in code rather than only in prose:
test_streaming_deltas_cannot_carry_a_custom_tool_call_yet asserts the validation
error, so when the generated types gain a custom member the test fails and points
at the streaming half instead of it being found in the field. The branch comment
in _completions.py now says non-streaming only and references that test.
|
@jbeckwith-oai this is a good review and you caught the thing I got wrong. all four of the parse-side asks are in on the union: on source of truth: on serialization coverage: the tests now dump the whole parsed completion, plus a on streaming, you are right and my description was wrong. verified it rather than taking it on faith: the chunk never validates, so happy to do the streaming half as a follow-up if you can tell me the delta shape to target. state: ruff check and format clean, pyright clean on all six files, mypy 37 vs 38 on base with no new errors (diffed the full list, not eyeballed), |
What
parse_chat_completion()— the helper behindclient.chat.completions.parse()— logs a warning and then drops everycustom-type tool call:So a supported GPT-5 custom tool call that the model emits disappears from the parsed result —
message.tool_callsomits it, and for a custom-only turn it comes backNone. The raw (unparsed)ChatCompletioncarries the call correctly; only.parse()loses it.The handling is also internally inconsistent: the trailing
elsebranch already preserves any non-function tool call by appending it unchanged — onlycustomis special-cased to discard.Fix
Two parts, because appending the call turned out not to be enough.
1. Stop dropping it. Append the custom tool call, mirroring the
elsebranch. Custom calls legitimately don't getparsed_arguments(no schema to parse their free-form input against), so they're surfaced as-is. The inaccurate"Ignoring tool call"warning, which fired on every custom call in normal use, is removed.2. Make the annotation describe what the field holds.
ParsedChatCompletionMessage.tool_callswas narrowed tolist[ParsedFunctionToolCall], and pydantic serializes by the declared type, so dumping a whole parsed completion emitted the call with its payload silently gone:That doesn't even validate back into an equivalent model, so a caller persisting and replaying a parsed completion loses the call.
tool_callsis nowlist[ParsedChatCompletionMessageToolCallUnion], an annotated discriminated union mirroringChatCompletionMessageToolCallUnionon the base class with the function member swapped for its parsed subclass. Function calls still resolve toParsedFunctionToolCallwithparsed_argumentsintact; the parsed message now lines up with the raw one instead of diverging from it.Consequences of the wider type, handled here rather than pushed onto callers:
assert_neverexhaustiveness sites in the streaming accumulator get an explicitcustombranch. A custom call carries noparsed_arguments, and the argument-delta/done events are function-tool specific, so each branch is a documented no-op — but being explicit keeps exhaustiveness real, so a future third tool-call type still tripsassert_never.examples/parsing_tools.pynarrows ontypebefore reaching for.function, which the raw completion has always required.Tests
tests/lib/chat/test_parse_custom_tool_calls.py— pure unit tests, no mock server or live API: a custom-only message, a mixed function+custom message, a dump/validate round trip, and the streaming-boundary test above. The first three assert on the whole-completion dump rather than dumping each tool call on its own, and all three fail without this change.Scope: non-streaming
parse()onlyA streamed custom tool call cannot reach this parser at all.
ChoiceDeltaToolCall.typeisOptional[Literal["function"]], so the chunk fails validation beforeget_final_completion()gets here:Extending that path means widening the generated chunk delta types plus the streaming accumulator, and it depends on the wire shape the API actually emits for streamed custom calls, which I can't determine from outside. So this PR is scoped to non-streaming
.parse(), and the boundary is pinned bytest_streaming_deltas_cannot_carry_a_custom_tool_call_yetrather than left in prose: when the generated types gain a custom member that test fails and points at the streaming half. The branch comment in_completions.pysays the same.Corrections to my earlier description of this PR
Two things I claimed that were wrong, both found by executing rather than reading:
CONTRIBUTING.mdsays modifications are persisted through generation, and it is the actual fix — so it's in scope and it's here. I also understated the symptom: the payload isn't just dropped on the whole-completion path, the emitted object is structurally invalid.pyright --strictwas clean on the changed files. It was not: the first commit left an error at_completions.py:133, appending a custom call to a list annotatedlist[ParsedFunctionToolCall]. That's fixed here, and both checkers are now clean.get_final_completion()path as well. It does not, for the reason in the scope section above.Verification
ruff checkandruff format --checkclean on every file touched.pyrightclean on all six.mypyreports one fewer error thanmain(the:133one above); no new errors — checked by diffing the full error list against the base rather than eyeballing it.tests/lib/chat/36 passed. The remainingtests/lib/failures are Bedrock/Azure and are identical to the base (31 in both, missing optional deps locally).Rebased onto
mainand retargeted there (this PR originally targetednext;mainis where external fixes land — 36 of the last 40 merges).Developed with Claude Code; reviewed and tested by Pranav before marking ready for review.