Review view: chat(protocol=) - #461
Open
rejojer wants to merge 8 commits into
Open
Conversation
responses() and messages() become _responses()/_messages(): the same engines, reachable as chat(protocol="responses"|"messages") with the protocol's own input and output shapes (transcript items / content blocks in, the envelope or native stream out). The old names are the vendor SDKs' own, and an agent-written client.messages(...) now fails fast with the way in — a runtime-only __getattr__, invisible to the type checker so attribute typos on the client still get flagged. chat() gains the knobs that lost their public home: instructions (appended after the managed prompt — a string on every lane, Messages system blocks with protocol="messages"), max_turns, backend, extra_headers, extra_body. reasoning_effort lands natively on each lane: LiteLLM's kwarg, Responses reasoning.effort, Anthropic output_config.effort. show_process stays the answer lane's view. Six overloads keep the return types narrow for py.typed consumers; the answer lane's contract is unchanged. The managed cloud chat rejects the own-model knobs as before. Claude-Session: https://claude.ai/code/session_017uvMD9eatgdaupLGFUjuKM
…remedies
- Messages lane: through the public door the thinking budget rides
extra_body, so the default max_tokens lift reads it there (extra_body
wins on the wire, so it wins in the lift); the documented
extra_body={"thinking": ...} no longer 400s on max_tokens < budget.
- Responses lane: temperature/top_p/reasoning/max_output_tokens reach the
wire via extra_body only; the envelope now reports what was sent instead
of the never-set locals.
- One _require_own_chat refusal for chat(protocol=...), the doors behind
it, and instructions. The doors' own copies had already drifted from
chat()'s text, and every copy pointed a local client with chat_model
blank at the managed chat it does not have; a door reached directly on
such a client fell through to a raw AttributeError. The check is the
one chat_completions already makes.
- Messages lane treats model="" as unset, like every other model check.
- The protocol refusal for show_process runs before the stream=True hint,
so the first remedy offered is the right one.
- instructions="" configures nothing (no empty system row, cache key
unchanged), matching the protocol lanes.
- Responses validation names messages, the public parameter, not input.
- Stale docstring pointer to messages() fixed.
- A protocol=None, stream: bool overload restores str | ChatStream for a
runtime-variable stream; the catch-all had widened it to a 4-way union.
- Tests: door X refuses like chat(protocol=X) on managed and blank-local
clients; the protocol gate and the non-stream show_process order are
asserted by distinct messages; chat(stream=True) joins the max_turns
matrix; each fix carries a red-verified assertion.
Claude-Session: https://claude.ai/code/session_01M9GdjnuDHHwDKqMzvPWCcj
rejojer
changed the title
… does
The rule chat() follows, written down: chat() names PageIndex's own
parameters plus a subset of LiteLLM's unified vocabulary; anything
vendor-specific rides extra_body under the lane's wire names. Three
layers meet on the wire — the skeleton (managed prompt, conversation,
tools) is the SDK's, the named knobs are translated per lane, and
extra_body is the caller's, merged last so it wins. A key the SDK writes
into a nested object must go in through extra_body too, so the caller's
other keys in that object survive — the SDK merge is shallow.
- Responses lane: reasoning_effort joins the caller's extra_body
"reasoning" object (their keys win) instead of riding a separate
reasoning= that extra_body's object replaced whole on the wire.
Mirrors the Messages lane's output_config. The envelope's
given.get("reasoning") now reports the merged object for free.
- reasoning_effort="" is unset on both protocol lanes, like model=""
and instructions="".
- extra_body docstring: drop the invitation to override the SDK's
system/input — that is the skeleton, and a fixed input breaks the
tool loop; name the wire fields per lane instead of one mixed list.
- messages docstring: every system row joins the managed prompt on the
answer lane, not only a leading one (_split_chat_messages hoists all).
- max_turns docstring: the OpenAI lanes raise at the cap, the Messages
lane returns the truncated run — the divergence was documented only on
the private door.
- Migration message: parameters go by keyword; the doors' sampling and
thinking fields ride extra_body.
- chat_model docstring: responses is no longer a chat surface.
- _split_chat_messages refusals no longer name chat_completions, a
method the chat() caller never typed.
- Tests: the Responses door equivalence takes the extra_body shape;
effort/extra_body collision, key survival and "" pinned on both
protocol lanes; the ModelSettings spy asserts the values reached the
wire, not only the envelope.
Claude-Session: https://claude.ai/code/session_01Hj6t26s7thUjkcho6sn4zE
metadata is a Responses request field the caller sets through extra_body; the envelope hard-coded None. Same source as the other caller-set fields: given.get(). Claude-Session: https://claude.ai/code/session_01Hj6t26s7thUjkcho6sn4zE
…s unset
reasoning_effort="" reached LiteLLM as a literal empty effort on the
answer lane while the protocol lanes already treated it as unset. And
chat_completions' managed-cloud own-model gate, which chat() routes
through, refused model="" / reasoning_effort="" / {} as knobs the caller
never set. Non-numeric knobs now read falsy as unset, as the local lane
always has (model or chat_model, if extra_body, backend or {}); numeric
ones keep `is not None`.
Claude-Session: https://claude.ai/code/session_01BAmVWYKoSnFEydbMjuZjHc
extra_body merges last on every lane, so a caller's system / instructions / input / messages / tools replaced the managed prompt, the conversation or the doc tools wholesale. The run succeeded and answered without tool guidance or document scoping, silently. Those are the SDK's on every lane (the three-layer rule: skeleton, named knobs, caller extras); the door for the prompt is instructions=. Refused at the two seams where extra_body meets the wire, before the Anthropic transport exists on the Messages lane. Claude-Session: https://claude.ai/code/session_01BAmVWYKoSnFEydbMjuZjHc
…eLLM-routed models On the answer lane, extra_body for a non-OpenAI destination becomes ModelSettings.extra_args, the LiteLLM kwargs channel (LiteLLM would plant extra_body as a literal request field, which Anthropic rejects). openai-agents passes ModelSettings' own fields to litellm.acompletion by name beside **extra_args, so a caller's temperature / top_p / max_tokens / penalties / tool_choice in extra_args collided with the explicit keyword and died as a raw TypeError inside the framework. Split by the framework's own contract: keys that are ModelSettings fields ride their field (the caller's value winning over ours), the rest stay LiteLLM kwargs. The field list is the public dataclass, not a hand-kept name list; bad values now fail ModelSettings validation and surface as PageIndexAPIError. response_format is not a ModelSettings field and still has no door on this lane; documented. Claude-Session: https://claude.ai/code/session_01BAmVWYKoSnFEydbMjuZjHc
chat_completions shares the seam that now refuses the skeleton keys; its docstring still promised an unconditional merged-last win. Same sentence as chat(). Claude-Session: https://claude.ai/code/session_01BAmVWYKoSnFEydbMjuZjHc
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.
Review-only view of #460 (chat(protocol=): the protocol doors move behind the front door) plus every follow-up on
feat/chat-protocol, diffed againstreview/base-91b6c36, main frozen at the commit #460 was merged onto.Never merge. The base must never advance; merging would only pollute it. Follow-up review rounds run on this PR; fixes found here push to
feat/chat-protocol(this diff updates) and reachmainthrough their own small PRs.Same pattern as #457 / #458 (v0.2.13) and #427 (v0.2.11).
https://claude.ai/code/session_01M9GdjnuDHHwDKqMzvPWCcj