Client instructions: a standing persona for every answer surface - #497
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. You're on a roll. 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". |
rejojer
added a commit
that referenced
this pull request
Sep 10, 2026
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
added a commit
that referenced
this pull request
Sep 10, 2026
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
added a commit
that referenced
this pull request
Sep 10, 2026
…ions() (#493) * chat(protocol="chat_completions") and the compat note on chat_completions() 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 * Review fixes: type and document the chat_completions protocol value 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 * Review fixes: name the lanes in error text, finish the chat() steer sweep - _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 * Fix managed chat stream overrides in extra_body * Clarify chat completions protocol parameter documentation * chat(): keyword-only after messages, type the protocol value 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 * fix: prevent managed chat extras from overriding document scope * extra_body: one gate for every lane, before any I/O 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
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 605c7c3ab9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
Comment on lines
+1228
to
1230
| system message); the history is text only: tool-role | ||
| turns are rejected on both engines, and message | ||
| fields beyond role/content are dropped. |
There was a problem hiding this comment.
For a plain cloud client, the managed branch preserves every non-system message verbatim (including tool roles, structured content, and extra fields) and lets the endpoint reject unsupported input. Consequently, this public docstring incorrectly promises that tool turns are rejected by both engines and extra fields are dropped; callers can instead receive endpoint 400/422 errors and have extra fields transmitted. Qualify these guarantees as applying only to the own-model engine and describe the managed pass-through behavior.
Useful? React with 👍 / 👎.
PageIndexClient(instructions=...) sets standing guidance for the answering agent — persona, language, format — appended after the managed system prompt wherever an answer is produced: chat() and chat_completions() on both engines, the Responses and Messages protocol lanes, agent_instructions() and the three *_agent_config() bundles. It is a client-level argument like mode=, not a chat-side spelling: it combines with any chat= and never selects own-model chat on its own. Blank configures nothing, as chat(instructions="") does. chat(instructions=) adds to it per call; the prompt order is managed base, client, call, history system rows. One insertion point serves every own-model surface (_base_instructions). The managed cloud chat takes exactly one system message, first: the client's instructions, the call's, and the history's system rows now fold into it, in that order — so chat(instructions=) works on a managed client (refused since #460, although the endpoint has accepted custom instructions since Sep 1), and a system row anywhere in the history no longer 400s. The answer lane's messages contract is the same on both engines: text history only — the endpoint refuses tool rows and structured content itself; the SDK says so first, with the protocol-lane pointer. Live-verified against the production managed chat and the live MCP instructions. Claude-Session: https://claude.ai/code/session_01J8fbpdM5pz2JLjiNY11usy
The managed payload is now the same shape every time — one leading system row when there is any system text, then the role/content history — instead of forwarding the caller's list untouched when nothing folded. That branch let a blank system row sit mid-history and reach the endpoint's "only one system message, first" refusal; blank text now configures nothing, like a blank instructions= does. Claude-Session: https://claude.ai/code/session_01J8fbpdM5pz2JLjiNY11usy
The managed chat_completions lane ran the whole payload through the own-model validator: tool rows, structured content and tuples were refused before the wire, and every field beyond role/content was stripped (tool_calls, name, the endpoint's own citations). The SDK is a thin skin over the cloud. It folds the client's instructions and the history's system/developer rows into the one leading system row the endpoint takes, and sends everything else as given; the endpoint decides what it accepts. Also: - _managed_instructions drops blank system texts, as the managed fold and _anthropic_system already do, so both engines build the same prompt for the same input (and share one prompt-cache key). - An end-to-end test drives chat() on the own-model lane and asserts the persona and the call's system text reach the model; the white-box builder tests alone stayed green with the persona removed from the answer lane. - as_claude_mcp(): the MCP-instructions channel carries the tool guidance only; the client's instructions ride system_prompt. - Comments that restated code or an assertion removed; the chat= combination test asserts chat_model too. Blank instructions configure nothing at the constructor, as chat(instructions="") does. Claude-Session: https://claude.ai/code/session_01TgdXZx63aMwrpaKFTQFFKx
Both lanes now take any iterable of message dicts. chat()'s instructions prepend expanded only lists, so a tuple or generator on the managed lane silently dropped the call's instructions once _require_own_chat no longer refused it; own-model rejected the same shapes outright. list() once at each reader instead. None and other non-iterables fail with Python's own TypeError, as in the OpenAI SDK. _system_text refuses a system row carrying non-text parts instead of keeping the text parts and dropping the rest: the SDK folds that row into its own system text, so it is the reader and must say what it could not read. The endpoint stays the authority on every row the fold leaves in place. Restore the messages docstring sentence 108875b rewrote: tool-role turns are rejected on both engines (the endpoint 400s them), so the managed lane does not take them "verbatim"; rename the test that carried that claim. Cover the blank-text filter, which no test guarded, and drop a truncated comment. Claude-Session: https://claude.ai/code/session_01UBpLu7TJUvrvLvFacs8WYT
…not use A plain managed client is the caller most likely to pass Messages-style blocks as instructions=, and the old message sent it to chat(protocol="messages"), which that same client refuses. Say "pass text", and make the block pointer the exact working call: model= is required on that lane, and it needs a chat_model= client. Claude-Session: https://claude.ai/code/session_018od4o3YBqrny2vzGhrX4q5
…e void The chat_completions protocol lane's guard test still pinned the managed refusal of instructions=, which this branch lifts on purpose; its own managed-fold tests cover what the lane now does. And _anthropic_system lost its doc_id argument when targeting moved to the first user message, so the prompt-order assertion calls it with what it takes.
rejojer
force-pushed
the
feat/client-instructions
branch
from
September 10, 2026 12:03
605c7c3 to
75f93e8
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.
What
PageIndexClient(..., instructions="...")— standing guidance for the answering agent (persona, language, format), appended after the managed system prompt on every surface that produces an answer.Design
instructions, the same word aschat(instructions=). A client-level argument likemode=, not a chat-side spelling: it combines with a stringchat=slot and never selects own-model chat on its own (achat_-prefixed flat argument would have flipped a managed client to own-model chat through the config.yaml default).instructions→chat(instructions=)→ historysystemrows. Stable-before-variable is also the cache order.""/whitespace = unset, aschat(instructions="")._base_instructionsreaches every own-model surface:chat()/chat_completions(),chat(protocol="responses"),chat(protocol="messages")(inside the cached managed block),agent_instructions(),openai_agent_config(),anthropic_runner_config(),claude_agent_config(). Not touched: indexing, the tool surfaces, MCP tool calls, the cloud MCP server's owninitialize.instructions.chat(instructions=)works on a managed client (refused in the SDK since chat(protocol=): the protocol doors move behind the front door #460, although the endpoint has accepted custom instructions since Sep 1), and asystemrow anywhere in the history no longer 400s.Verification
chat()call on the own-model lane (the persona and the call's system text reach the model; red-verified by blanking the persona on that lane alone); the three config bundles; the managed fold (order, streamed door, developer rows) and the verbatim forwarding of the rest of the history (a leading system row kept in place, blank system rows dropped, tool rows / structured content / extra fields untouched).test_cloud_guardsdrops its pin on the old managed refusal.instructions="…French…PINEAPPLE"answers in French; with a per-call addition both layers show; a bridge client'sagent_instructions()ends with the persona after the live MCP guidance.Pending (land with the release)
client.mdxconstructor table row,chat.mdxclient example +instructionsparameter row,agents.mdxone sentence onagent_instructions().https://claude.ai/code/session_01J8fbpdM5pz2JLjiNY11usy
https://claude.ai/code/session_01TgdXZx63aMwrpaKFTQFFKx
Rebased onto 0.2.16's other lanes
Two test pins this branch voids on purpose: the chat_completions protocol lane's guard pinned the managed refusal of
instructions=, which the fold lifts, and_anthropic_systemlost itsdoc_idargument when document targeting moved to the first user message.