chat(protocol="chat_completions") and the compat note on chat_completions() - #493
Merged
Conversation
Member
Author
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…ions() chat() gains the third protocol value: the answer lane's own engine with its Chat Completions envelope kept (chunk dicts when streaming, instructions as a leading system row). It is the one protocol the managed cloud chat serves, so that lane opens without a chat model; the own-model knobs still refuse there. chat_completions() stays, unchanged in signature, with a docstring that marks it as kept for existing code and points new code at chat(). Error strings that steered callers to it now name the protocol lane; the cookbook's two cells use chat(). The managed endpoint takes extra_body as its own request fields (temperature, enable_citations), merged last under the skeleton refusal, so new code reaches them without the old door. Claude-Session: https://claude.ai/code/session_015b7YGZ8LvYp2Q3oGNN8Lfc
The protocol overloads name "chat_completions" so the literal narrows to the envelope dict and the chunk-dict iterator; the protocol arg doc lists it and notes the managed chat serves it; the show_process texts no longer promise a transcript the Chat Completions lane has none of. Claude-Session: https://claude.ai/code/session_015b7YGZ8LvYp2Q3oGNN8Lfc
…weep - _split_chat_messages rejected tool rows and structured content with "use chat(protocol=...)", which is the lane the caller just used now that chat_completions is a protocol value; name responses / messages. - submit_query / get_retrieval still steered to chat_completions(), the door this branch demotes to compat; steer to chat() like the rest. - Three docstring claims narrowed to where they hold: the compat note's "everything is chat(protocol=...)" excepts the text-only stream (that is chat(stream=True, show_process=False)); system rows join the managed prompt only with your own chat model, the managed endpoint forwards them verbatim; extra_body is verbatim on Responses, Messages and the managed endpoint, while own-model chat_completions splits it like the answer lane. - Tests: drop the warnings guard around a deprecation that does not exist and the comments restating assertions; the managed-lane skeleton test now calls the lane it is named for. Claude-Session: https://claude.ai/code/session_014bdfnYbWsejWhuphPt1aSv
chat(messages, doc_id, stream, ...) and chat_completions(messages, stream, doc_id, ...) disagree on positions 2 and 3, and the compat note now tells existing callers the two are interchangeable. A positional rewrite bound doc_id=True and stream="pi-1": an unscoped answer, billed, no error. From messages on, chat() takes keyword arguments only, so that rewrite is a TypeError. Nothing in the repo passed chat() a positional after messages. BREAKING: chat(messages, doc_id) and chat(messages, doc_id, stream) by position no longer bind; spell doc_id= and stream=. Also: the implementation and catch-all overload now carry the same Literal as the protocol overloads, so a misspelt protocol fails type-checking instead of only at runtime; _refuse_skeleton names the managed lane's door (a system row), since instructions= is refused there; _require_own_chat's comment no longer claims to be the gate for every protocol. Claude-Session: https://claude.ai/code/session_01DCBitejpuoNpD8cmQ9HR7j
The managed endpoint's refusals of stream and doc_id sat in cloud_api.chat_completions, one call site of four. The other lanes took the same keys and broke worse: an own-model OpenAI backend sent stream:false under an SSE parser and returned an empty answer, and LiteLLM raised a bare KeyError. The merge had no Mapping check either, so a list splatted into the payload as fabricated fields. _refuse_skeleton now owns all of it: a non-dict is refused, and stream / doc_id join the refused keys beside the skeleton. chat() and chat_completions() call it first, so the own-model half no longer runs doc targeting and the MCP initialize fetch before refusing. cloud_api is a plain transport again. The skeleton remedy no longer splits by client type. A system row works on every chat-shaped lane and instructions= becomes one, so the two labels were inverted for chat_completions() callers. Managed chat(instructions=) opens when #497 lands. Tests: non-dicts and the argument keys at the gate, both public doors refusing before any lane is entered, a foreign managed key pinned as forwarded. The keyword-only test builds its client outside pytest.raises and matches the positional message. Claude-Session: https://claude.ai/code/session_01RTqXsk6Y3iGn9iXZHrzpv6
rejojer
force-pushed
the
feat/chat-protocol-chat-completions
branch
from
September 10, 2026 11:35
31bf587 to
853964d
Compare
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
1 participant
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.
Summary
chat()gains a third protocol value,"chat_completions": the answer lane's own engine with its Chat Completions envelope kept (chunk dicts when streaming,instructionsas a leading system row). It is the one protocol the managed cloud chat serves, so that lane opens without a chat model; the own-model knobs still refuse there.chat_completions()stays public with its signature unchanged. Its docstring now marks it as kept for existing code and points new code atchat(), so everything it offers has a route that does not go through the old name. No warning, no__getattr__interception.extra_bodyas its own request fields (temperature,enable_citations), merged into the payload last under the same skeleton refusal as the other lanes.chat_completions()now name the protocol lane; the cookbook's two cells usechat(). Descriptive mentions are untouched.chat()takes keyword arguments aftermessages. Its positional order (doc_id, thenstream) is the reverse ofchat_completions()'s, and with the compat note inviting the rename,chat(msgs, True, "pi-1")bounddoc_id=True: an unscoped, billed answer with no error. Now aTypeError. Nothing in the repo, the docs, or the cookbook passedchat()a positional aftermessages. The implementation and catch-all overload also carry the protocolLiteral, so a misspelt protocol fails type-checking.Verification
agentsblocked insys.modules).https://claude.ai/code/session_015b7YGZ8LvYp2Q3oGNN8Lfc
https://claude.ai/code/session_01DCBitejpuoNpD8cmQ9HR7j