Repository navigation
fix(vanilla_python): add Chat Completions API fallback - #253
Conversation
…ses servers The vanilla_python agent exclusively used the OpenAI Responses API, which causes HTTP 400 errors when the upstream model server (e.g., vLLM) does not implement that endpoint. This adds automatic detection and fallback to the Chat Completions API on the first call, caching the result so subsequent calls skip the probe. No changes needed to the agentic loop since tool calling uses text-pattern matching (ReAct style) in both API modes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe agent now extracts text from both OpenAI response formats and adaptively selects the Responses API or Chat Completions, caching successful capability checks and falling back on HTTP 400/404 errors. Tests cover extraction, fallback, error propagation, and cached selection. ChangesOpenAI API adaptation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AIAgent
participant OpenAIClient
participant ResponsesAPI
participant ChatCompletionsAPI
AIAgent->>OpenAIClient: call responses.create
OpenAIClient->>ResponsesAPI: submit request
ResponsesAPI-->>AIAgent: response or APIStatusError
AIAgent->>ChatCompletionsAPI: fallback on HTTP 400/404
ChatCompletionsAPI-->>AIAgent: chat completion result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
andrewdonheiser
changed the title
vLLM partially supports the Responses API — simple single-turn requests succeed, but multi-turn conversations fail with HTTP 400. The previous guard `self._use_responses_api is True` prevented fallback when the API had previously succeeded, causing the 400 to propagate as an error. Remove the guard so any 400/404 from the Responses API permanently switches to Chat Completions, regardless of prior success. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…llback tests Rename _responses_create to _llm_create to reflect its dual-API role, fix ruff format issue, update stale _execute docstring, and add unit tests covering the Responses→Chat Completions fallback (400/404), cache behavior, and response parsing for both API formats. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agents/vanilla_python/templates/openai_responses_agent/src/openai_responses_agent/agent.py (1)
286-311: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid caching the fallback on 400
A 400 from
responses.createcan mean a malformed request or invalid payload, not just an unsupported endpoint. Setting_use_responses_api = Falseon any 400 permanently downgrades later turns for the rest of the agent’s lifetime. Restrict the cached fallback to clear unsupported-endpoint failures like 404, and treat 400 as a per-call fallback without flipping the cache.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agents/vanilla_python/templates/openai_responses_agent/src/openai_responses_agent/agent.py` around lines 286 - 311, Update the API error handling in _llm_create so only a 404 sets _use_responses_api = False; for a 400, fall back to _chat_completions_create for the current call without changing the cached API preference. Preserve re-raising all other status codes and keep the existing fallback logging behavior appropriate to whether the downgrade is cached.
🧹 Nitpick comments (1)
agents/vanilla_python/templates/openai_responses_agent/tests/test_agent.py (1)
60-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test for the "prior success then later 400/404" transition.
The PR objective explicitly states the fallback should permanently switch "including cases where earlier requests succeeded," but no test sets
_use_responses_api = Truefirst and then verifies a subsequent 400/404 flips it toFalse. Current tests only cover starting fromNone. Add a test seedingagent._use_responses_api = Truebefore triggering the 400/404 exception.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agents/vanilla_python/templates/openai_responses_agent/tests/test_agent.py` around lines 60 - 93, Add coverage for the prior-success transition in the fallback tests around _llm_create: seed agent._use_responses_api = True before triggering an APIStatusError for both 400 and 404 cases, then assert the chat completions fallback response is returned and _use_responses_api becomes False. Preserve the existing assertions for the fallback call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@agents/vanilla_python/templates/openai_responses_agent/src/openai_responses_agent/agent.py`:
- Around line 286-311: Update the API error handling in _llm_create so only a
404 sets _use_responses_api = False; for a 400, fall back to
_chat_completions_create for the current call without changing the cached API
preference. Preserve re-raising all other status codes and keep the existing
fallback logging behavior appropriate to whether the downgrade is cached.
---
Nitpick comments:
In `@agents/vanilla_python/templates/openai_responses_agent/tests/test_agent.py`:
- Around line 60-93: Add coverage for the prior-success transition in the
fallback tests around _llm_create: seed agent._use_responses_api = True before
triggering an APIStatusError for both 400 and 404 cases, then assert the chat
completions fallback response is returned and _use_responses_api becomes False.
Preserve the existing assertions for the fallback call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: d6d1674a-7ccc-4279-9f04-511d15f089ae
📒 Files selected for processing (2)
agents/vanilla_python/templates/openai_responses_agent/src/openai_responses_agent/agent.pyagents/vanilla_python/templates/openai_responses_agent/tests/test_agent.py
shricharan-ks
left a comment
shricharan-ks
left a comment
There was a problem hiding this comment.
Code Review — PR #253 (medium effort, 6 angles, verified)
This PR adds automatic fallback from the Responses API to Chat Completions API when the upstream model server (e.g. vLLM) returns HTTP 400/404. It handles both response formats in _get_output_text_from_response(), caches the API detection result per-agent-instance, and adds comprehensive unit tests. 2 files changed, 192 additions, 7 deletions.
Findings (6 survived verification, most severe first)
1. 🔴 agent.py:241 — if temp != 0 silently drops temperature=0, API defaults to 1.0 (CONFIRMED)
if temp != 0:
kwargs["temperature"] = tempThe constructor defaults temperature=0 for deterministic output. But 0 is falsy in Python, so if temp != 0: evaluates to False — temperature is never sent to the API. Both OpenAI's Chat Completions and Responses APIs default to temperature=1.0 when omitted. Every request runs non-deterministically instead of the intended deterministic mode.
This is pre-existing in the Responses API path (line 283) but the PR propagates it to the new _chat_completions_create. Fix: if temp is not None: or always include "temperature": temp in kwargs.
2. 🟡 agent.py:195 — API detection cache resets every HTTP request (CONFIRMED)
_AIAgentAdapter.run() creates a new AIAgent per HTTP request. Since _use_responses_api is an instance variable initialized to None, the cache never persists across requests. Against a vLLM backend that doesn't support the Responses API, every request probes → fails → retries via Chat Completions, doubling first-turn latency and flooding logs with fallback warnings.
The cache works within a single query() call's multi-turn ReAct loop (2-5 turns), but not across HTTP requests. Consider lifting the detection state to _AIAgentAdapter (process-lifetime) or a module-level variable.
3. 🟡 test_agent.py — No test for the multi-turn fallback scenario the PR fixes (CONFIRMED)
The PR description says: "Fix: Always fall back on 400/404, even if the Responses API previously succeeded (vLLM partially supports Responses API — simple requests succeed but multi-turn conversations fail)."
All 5 tests exercise single, isolated _llm_create() calls. No test calls _llm_create twice where the first succeeds (→ True) and the second gets 400 and must transition to False. This is the exact scenario motivating commit d525731a and is the one most likely to regress.
Missing test: mock responses.create to succeed on call 1, raise APIStatusError(400) on call 2, verify call 2 falls back to chat.completions.create.
4. 🟡 agent.py:293 — HTTP 400 is too broad for "endpoint not supported" (PLAUSIBLE)
The except block treats any 400 as "server doesn't support Responses API" and permanently sets _use_responses_api = False. But HTTP 400 also covers legitimate request errors (too-long context, malformed content, unsupported parameter). A single bad request on a server that does support the Responses API would permanently disable it.
Consider checking the error body/message for endpoint-not-found indicators, or limiting the permanent flip to 404 only.
5. 🟡 agent.py:137 — choices[0].message accessed without guard (PLAUSIBLE)
choices = getattr(response, "choices", None)
if choices:
return getattr(choices[0].message, "content", None) or ""The Responses API path uses defensive getattr() throughout, but the new Chat Completions path directly accesses .message on choices[0]. An OpenAI-compatible server returning a non-standard choices schema (e.g., streaming delta instead of message) would raise an unhandled AttributeError.
6. 🟡 agent.py:233/273 — Parameter resolution duplicated across methods (PLAUSIBLE)
_chat_completions_create and _llm_create both have identical 3-line parameter resolution (msg_list = messages if..., temp = ..., model_id = ...) and the same if temp != 0 guard. Adding a parameter to one without the other would cause silent behavioral divergence between the two API paths.
What looks good
- Fallback design: The try/except structure correctly catches 400/404 and falls back, even after a previous Responses API success — the key fix for vLLM's partial support
- Response format handling:
_get_output_text_from_responsecorrectly dispatches between Chat Completions (choices) and Responses API (output) formats - Test coverage: 5 well-structured unit tests covering success caching, 400/404 fallback, error propagation, and cached fast-path
- Zero changes to
query(): The agentic loop is correctly insulated from the API abstraction change - Clean typing modernization:
Optional[str]→str | None,Dict→dictthroughout
Summary
Finding #1 (temperature=0) is the most impactful — affects every request. It's pre-existing but worth fixing in this PR since _chat_completions_create is new code. Finding #2 (cache scope) is a design improvement worth considering. Finding #3 (missing test) is easy to add and covers the exact scenario that motivated the fix.
- Fix temperature=0 bug: `if temp != 0` → `if temp is not None` so temperature=0 is actually sent to the API (was silently dropped, causing non-deterministic responses) - Differentiate 400 vs 404 in fallback caching: only 404 permanently disables Responses API; 400 falls back per-call without caching (400 can indicate a legitimate request error, not just unsupported) - Add tests for multi-turn fallback (prior success → later failure) and temperature=0 being passed through Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Review Response —
|
| # | Finding | Fix |
|---|---|---|
| 1 | 🔴 if temp != 0 silently drops temperature=0 (API defaults to 1.0) |
Changed to if temp is not None: in both _chat_completions_create and _llm_create |
| 2 | 🟡 HTTP 400 too broad for permanent cache flip | Only 404 now permanently sets _use_responses_api = False. On 400, falls back for the current call only without caching |
| 3 | 🟡 No test for multi-turn fallback (success → later failure) | Added test_fallback_on_400_after_prior_success, test_fallback_on_404_after_prior_success, and test_temperature_zero_is_passed |
Not addressed (follow-up)
- Cache scope resets per HTTP request (ench: cleanup for main README and react Agent readme,. update of the requirements #2):
_AIAgentAdapter.run()creates a newAIAgentper request, so the detection cache only persists within onequery()call's multi-turn loop. Lifting the cache to process-lifetime scope is a design improvement worth a separate PR. choices[0].messagewithout guard (fix: update README.md & dependencies #5): Low risk — Chat Completions always returns.messagein non-streaming mode; we never use streaming in_llm_create.- Parameter duplication (ench: RAG agent readme update and requirements #6): Minor DRY concern, not in scope of this fix PR.
All 12 unit tests pass, lint and formatting clean.
tarun-etikala
left a comment
tarun-etikala
left a comment
There was a problem hiding this comment.
Clean PR — fallback logic is correct, the 400-vs-404 distinction is well-reasoned for vLLM's partial Responses API support, and test coverage is thorough.
Two suggestions (non-blocking):
-
API detection cache doesn't persist across HTTP requests —
_AIAgentAdapter.run()creates a newAIAgentper request, so_use_responses_apiresets toNoneeach time. On a vLLM backend, every request pays for a wasted 404 probe before falling back. Consider lifting the flag to_AIAgentAdapterand syncing it back after eachquery()call. The log message "permanently falling back" is also misleading since the state is lost on the next request. -
Minor test style —
test_temperature_zero_is_passedusescall_kwargs[1]["temperature"];call_kwargs.kwargs["temperature"]is slightly more readable.
Summary
_get_output_text_from_response()The agent uses ReAct-style text patterns for tool calling (not structured API tool calling), so the fallback only needs to handle message/response format differences. The agentic loop (
query()) required zero changes.Jira
Test plan
Test results (behavioral tests against vLLM on OpenShift)
Cluster: agen-e2e-rhoai2 (ROSA HCP), namespace:
adonheis-testingModel: qwen2-5-7b-instruct via vLLM
Failures (pre-existing, not regressions)
search_priceand skipssearch_reviewson comparison queries. Not related to API fallback.Unknown action: Nonewhen model responds to "Hello" — the ReAct regex matchesAction: Nonein the output. Pre-existing agent bug in action parsing.🤖 Generated with Claude Code