fix: extra_body refuses the skeleton keys; sampling fields ride ModelSettings on LiteLLM-routed models - #472
Merged
Merged
Conversation
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 (cherry picked from commit 4d18854)
…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 (cherry picked from commit c1ad980)
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 (cherry picked from commit f9072f9)
rejojer
added a commit
that referenced
this pull request
Sep 10, 2026
RATE_LIMITED / USAGE_LIMIT_REACHED (pageindex-chat #472) arrive as a normal tool error inside HTTP 200, already retried server-side; the invoker re-raises them as 429 / 402, the way a post-retry status escapes, so every lane fails fast without a per-lane change. The bridge retries the whole 5xx range, the same range the invoker re-raises; a non-JSON 200 body (a JSONDecodeError is a RequestException too) stays a model-visible envelope, since the server was reached. The MCP stub tests keep the machine's proxy out of 127.0.0.1. Claude-Session: https://claude.ai/code/session_01F7vqmZdnWeKC9SUBytDrdf
rejojer
added a commit
that referenced
this pull request
Sep 10, 2026
) * Tool-path rate limits: retry at the bridge, then fail the run fast A PageIndex cloud 429 (or 5xx) on a tool call used to reach the model as an INTERNAL_ERROR envelope saying "try again": the model re-called once with no wait, then wrote the failure into its answer, and chat() returned normally with no status anywhere. The same 429 before the loop (the doc_id targeting lookup) already propagated raw. - McpBridge mounts a urllib3 Retry: 429/502/503 and connection failures, three attempts, 0/2/4 s apart or as Retry-After says; read timeouts are never replayed (240 s each, and the server may have acted); a Retry-After past a minute is a quota, not a blip, so the backoff runs instead of sleeping it out. Exhausted, the last response falls through to the existing >= 400 branch, so the status_code survives. - _bridge_invoker re-raises 429/5xx alongside 401/403. The frameworks turn a raised tool exception back into model-visible text, so each chat() door gets its own escape: the in-process MCPServer's failure_error_function lets a PageIndex-caused failure propagate and _translate_run_error unwraps it from the framework's wrapper (which also un-flattens the 401 case); the Messages lane runs each turn's tools through the runner's public generate_tool_call_response() and raises before the next model call. - _model_backend_error keeps the provider's status_code. Claude Agent SDK tools cannot fail fast: the SDK MCP server converts handler exceptions into JSON-RPC errors for Claude Code by design. Claude-Session: https://claude.ai/code/session_014S88dcSz7jykegAWyWZk8E * Tool-path fail-fast: cover unreachable servers and all 5xx The bridge retry is now a plain urllib3 Retry: 429 and every 5xx retried three times at the fixed 0/2/4 s backoff, Retry-After ignored. That drops the _Retry subclass, whose get_retry_after raised InvalidHeader on a non-integer header (turning a 429 into "could not reach the server"), honoured a 60 s Retry-After three times over, and let a 413 carrying Retry-After replay. 500 and 504 join the forcelist so the invoker's "what survived the bridge's retries" holds for every status it re-raises. Retry is imported from requests.adapters, the declared dependency. The invoker re-raises transport failures too: once the bridge's own connection retries fail, the model cannot reach the server either, and the envelope only sent it round the retry loop. The handshake error blames the API key only on 401/403: a rate-limited handshake is now a run-terminating error and was telling users to rotate a working key. Docstrings on agent_tools()/build_agent_tools and the Anthropic adapter state the real raise set: 401/403, post-retry 429/5xx, unreachable server. Claude-Session: https://claude.ai/code/session_013xk3xt9KgHNTjsYmFLKxbu * fix: surface Messages tool failures before advancing runner * fix: require urllib3 1.26 for MCP retries * fix: keep the Messages fail-fast quiet and single-path Raise ToolError from the tools chat(protocol="messages") runs instead of the raw PageIndexAPIError: the Anthropic runner log.exception()s any other exception, so every fail-fast printed a 20-line traceback from anthropic's internals before the SDK raised its own error. The lane still records the failure and raises it right after the runner's tool batch, so the ToolError content never reaches the model. Drop the two post-loop _messages_fail_fast calls: the runner executes tools only through the public generate_tool_call_response (0.108.0 through 1.4.0), which checked_tool_response wraps, so they could never fire. Annotate _pageindex_cause for the py.typed package. Claude-Session: https://claude.ai/code/session_01CfYSeq8kM7HjfF79TGbsiT * fix: fail fast on the cloud's account-limit tool errors; one 5xx list RATE_LIMITED / USAGE_LIMIT_REACHED (pageindex-chat #472) arrive as a normal tool error inside HTTP 200, already retried server-side; the invoker re-raises them as 429 / 402, the way a post-retry status escapes, so every lane fails fast without a per-lane change. The bridge retries the whole 5xx range, the same range the invoker re-raises; a non-JSON 200 body (a JSONDecodeError is a RequestException too) stays a model-visible envelope, since the server was reached. The MCP stub tests keep the machine's proxy out of 127.0.0.1. Claude-Session: https://claude.ai/code/session_01F7vqmZdnWeKC9SUBytDrdf * fix: raise the account-limit escape outside the invoker's own try _raise_account_limit fired inside _invoke's try, so its escape depended on 429 and 402 also appearing in the except's re-raise tuple: two lists in one function that had to agree. The check now runs after the try, where the except cannot swallow it, and 402 leaves the tuple (the MCP route never answers HTTP 402; it was there only to let the raise through). The mcp_stub fixture also sets the lowercase no_proxy: requests reads that spelling first, so a machine with no_proxy set still routed the stub requests through its proxy despite NO_PROXY.
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.
Two follow-ups to #460 (
chat(protocol=)), from the second review round on #461. Same commits as feat/chat-protocol 4d18854 / c1ad980, cherry-picked onto main.extra_body refuses the skeleton keys (4d18854)
extra_body merges last on every lane, so a caller's
system/instructions/input/messages/toolsreplaced 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 door for the prompt isinstructions=. Refused at the two seams where extra_body meets the wire, before the Anthropic transport exists on the Messages lane.extra_body sampling fields ride their ModelSettings field on LiteLLM-routed models (c1ad980)
On the answer lane, extra_body for a non-OpenAI destination becomes
ModelSettings.extra_args, the LiteLLM kwargs channel. openai-agents passes ModelSettings' own fields tolitellm.acompletionby name beside**extra_args, so a caller'stemperature/top_p/max_tokens/ penalties /tool_choicecollided 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_formatis not a ModelSettings field and still has no door on this lane; documented.Each fix carries red-verified tests. 476 green on feat/chat-protocol; the same commits are verified green on this base before the PR was opened.
https://claude.ai/code/session_01BAmVWYKoSnFEydbMjuZjHc