feat: /v1/responses endpoint (OpenAI Responses API surface for Codex CLI) - #34
Conversation
…rity; log drops Final-review fix wave: - tools_to_chat drops unknown non-function tool types instead of forwarding them (forwarding 400s the whole turn at a chat provider); dropped_tool_types surfaces them and shim logs a warning. - usage carries input_tokens_details.cached_tokens for context-accounting parity with the chat path. - spec note updated to match.
Surfaced by the live codex-exec acceptance test: Codex v0.142.1 sends an input message with role 'developer' (OpenAI's system-equivalent). Map it to 'system' so non-OpenAI chat providers (which accept only system/user/ assistant/tool) don't reject the turn. Acceptance run also confirmed: real Codex accepts the request translation + Responses SSE (text turn renders + usage read), the function-call round-trip works end-to-end (function_call -> exec_command run -> function_call_output -> follow-up), and the drop-native- tools fix keeps all 8 function tools incl. exec_command while dropping only namespace/web_search.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 44 minutes and 44 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an OpenAI Responses API façade ( ChangesResponses API Shim
Sequence Diagram(s)sequenceDiagram
participant Client as Codex CLI / Client
participant Shim as shim.py /v1/responses
participant ResponsesAPI as responses_api.py
participant Router as LLMRouterHost
Client->>Shim: POST /v1/responses (ResponsesRequest)
Shim->>ResponsesAPI: input_to_messages(input, instructions)
Shim->>ResponsesAPI: tools_to_chat(tools)
Shim->>Router: execute_async(ChatRequest contract)
Router-->>Shim: router result (finish_reason, usage, tool_calls)
alt stream: false
Shim->>ResponsesAPI: result_to_responses_object(result)
Shim-->>Client: Responses JSON object
else stream: true
Shim->>ResponsesAPI: responses_created_event(obj)
Shim-->>Client: SSE: response.created
Router-->>Shim: task completes
Shim->>ResponsesAPI: responses_sse_events(obj)
Shim-->>Client: SSE: output_item.added, text delta/done, response.completed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![chatgpt-codex-connector[bot]](https://avatars.github.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: c94c9e4071
ℹ️ 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".
| yield _emit("response.output_item.done", | ||
| {"output_index": out_index, "item": item}) | ||
|
|
||
| yield _emit("response.completed", {"response": obj}) |
There was a problem hiding this comment.
For public streamed /v1/responses calls through auth_proxy, this response.completed frame is the only terminal frame carrying usage and x_router, but both are nested under response. The proxy's _parse_stream_tail only reads top-level payload["usage"] / payload["x_router"], and its JSON path reads chat-only prompt_tokens / completion_tokens, so Codex/Responses traffic is persisted with zero tokens and missing router metadata/cost even though the shim has the data. This breaks the advertised same metering for the new endpoint; update the proxy parser for Responses frames or emit a proxy-visible final metadata frame.
Useful? React with 👍 / 👎.
| _session_from_header(req, request) | ||
| return await _handle_responses(req) | ||
|
|
||
| @app.post("/{profile_name}/v1/responses") |
There was a problem hiding this comment.
For consumers with allowed_routes configured, /{profile}/v1/responses is not recognized as profile:{profile} before proxying: auth_proxy._requested_route_from only special-cases /{profile}/v1/chat/completions and otherwise uses the body model/name. A Codex client using the documented profiled base URL can therefore be denied with caller_route_not_allowed or checked against the wrong model before this handler runs; add the Responses path to the proxy's route extraction alongside chat completions.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
shim.py (1)
971-976: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate the intentional broad catch for Ruff.
This catch is converting post-commit failures into
response.failed; add the localnoqaso BLE001 does not keep warning on the intended pattern.Proposed fix
- except Exception as exc: + except Exception as exc: # noqa: BLE001 — post-commit SSE must emit response.failed🤖 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 `@shim.py` around lines 971 - 976, The broad exception handler in the response-failure path is intentional, so add a local Ruff suppression for BLE001 on the `except Exception as exc` in the `_responses_failed` flow. Keep the existing conversion to `response.failed` intact, and annotate only this catch block so Ruff stops flagging the deliberate pattern.Source: Linters/SAST tools
tests/test_responses_shim.py (2)
120-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the profiled-route test prove the path profile is used.
Because
_seedgives every route the same response, this test would still pass ifcontract["profile"] = profile_namewere removed. Capturehost.execute_async’s contract or seed distinct behavior so the assertion fails when the path profile is ignored.🤖 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 `@tests/test_responses_shim.py` around lines 120 - 125, The profiled-route test currently does not verify that the path profile is actually applied, because `_seed` makes every route return the same response. Update `test_responses_profiled_route` to either capture and assert the `contract["profile"]` value passed through `host.execute_async`, or seed distinct behavior per profile so the response changes when the path profile is ignored. Use the `test_responses_profiled_route`, `_seed`, and `host.execute_async` symbols to locate the test and make the assertion fail if `contract["profile"] = profile_name` is removed.
69-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
next(...)for single-item lookups flagged by Ruff.This removes temporary lists and clears RUF015.
Proposed fix
- msg = [o for o in body["output"] if o["type"] == "message"][0] + msg = next(o for o in body["output"] if o["type"] == "message") @@ - completed = [d for d in datas if d.get("type") == "response.completed"][0] - msg = [o for o in completed["response"]["output"] if o["type"] == "message"][0] + completed = next(d for d in datas if d.get("type") == "response.completed") + msg = next(o for o in completed["response"]["output"] if o["type"] == "message") @@ - fc = [o for o in r.json()["output"] if o["type"] == "function_call"][0] + fc = next(o for o in r.json()["output"] if o["type"] == "function_call")Also applies to: 99-100, 115-115
🤖 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 `@tests/test_responses_shim.py` at line 69, Replace the Ruff-flagged single-item list comprehensions in the response shim tests with next(...) lookups so you avoid building temporary lists and clear RUF015. Update the message retrieval in the test helper and the other similar assertions in this test module, using the existing body["output"] filtering logic and the relevant msg-style variables to keep the behavior unchanged.Source: Linters/SAST tools
🤖 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.
Inline comments:
In `@docs/superpowers/plans/2026-06-28-v1-responses-shim.md`:
- Around line 319-359: Add the missing dropped_tool_types(tools) helper in the
tool-conversion section alongside tools_to_chat and tool_choice_to_chat so it
can report which incoming tool types were discarded. Then update
_handle_responses to call that helper and emit the warning/logging that surfaces
the dropped tool types, matching the behavior described in the spec and the
final shim. Keep the helper consistent with the existing conversion utilities
and ensure it integrates cleanly with the responses-to-chat tool handling path.
- Around line 253-316: `input_to_messages` is missing the documented `developer`
to `system` role normalization, so update the item-to-message conversion path to
map any `developer` role entry to `system` before appending it to `messages`.
Make this change in the `input_to_messages` loop alongside the existing `role`
handling so requests with `developer` items are emitted as valid `system`
messages.
- Around line 319-338: `tools_to_chat` still preserves unknown/native tool
entries in its else branch, but the intended behavior is to drop anything that
is not a function tool. Update the `tools_to_chat` logic so it only converts and
appends items with `type == "function"` and a `name`, and otherwise skips them
entirely; keep the rest of the function shape the same so it remains the inverse
of `codex_backend._to_responses_tools`.
- Around line 957-984: The _handle_responses flow is missing the dropped-tool
visibility check before building ChatRequest. Update _handle_responses to first
inspect the incoming ResponsesRequest for dropped tools, call
dropped_tool_types, and emit the warning log when any tool types were dropped,
then continue constructing the ChatRequest and contract as before. Use the
existing _handle_responses, dropped_tool_types, and _request_to_contract symbols
to place the warning in the non-streaming and streaming path shared setup.
In `@shim.py`:
- Line 118: The session owner is currently taken from the public
ResponsesRequest.caller field, which can be spoofed by clients. Update
ResponsesRequest handling in shim.py so caller is not trusted from the JSON body
and is instead overwritten from the ingress header before any
ownership/session-meter logic runs. Make the fix in the request
parsing/normalization path and the code that uses caller as the owner so the
session-meter ownership always comes from the ingress-derived value.
- Around line 101-106: The ResponsesRequest model currently allows
previous_response_id to pass through silently, which can lead to stateful
clients getting a misleading 200 without prior context. Update the request
handling around ResponsesRequest in shim.py to explicitly detect
previous_response_id and reject it with a 400 error before processing. Keep the
model permissive for other unknown fields, but fail fast for
previous_response_id so callers know to resend the full input.
In `@tests/test_responses_api.py`:
- Around line 30-43: Add a regression test in test_input_items_plain_messages or
a new test covering a singleton top-level message dict passed directly to
input_to_messages, since the current tests only exercise list inputs and
{type:"message"} wrappers inside lists. Update input_to_messages in
responses_api.py so a standalone dict with role/content is normalized to a
one-item messages list instead of being treated like an empty items path, and
keep the existing flattening/unwrapping behavior intact.
---
Nitpick comments:
In `@shim.py`:
- Around line 971-976: The broad exception handler in the response-failure path
is intentional, so add a local Ruff suppression for BLE001 on the `except
Exception as exc` in the `_responses_failed` flow. Keep the existing conversion
to `response.failed` intact, and annotate only this catch block so Ruff stops
flagging the deliberate pattern.
In `@tests/test_responses_shim.py`:
- Around line 120-125: The profiled-route test currently does not verify that
the path profile is actually applied, because `_seed` makes every route return
the same response. Update `test_responses_profiled_route` to either capture and
assert the `contract["profile"]` value passed through `host.execute_async`, or
seed distinct behavior per profile so the response changes when the path profile
is ignored. Use the `test_responses_profiled_route`, `_seed`, and
`host.execute_async` symbols to locate the test and make the assertion fail if
`contract["profile"] = profile_name` is removed.
- Line 69: Replace the Ruff-flagged single-item list comprehensions in the
response shim tests with next(...) lookups so you avoid building temporary lists
and clear RUF015. Update the message retrieval in the test helper and the other
similar assertions in this test module, using the existing body["output"]
filtering logic and the relevant msg-style variables to keep the behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db986cc7-e852-4640-a7b4-97acae6f5961
📒 Files selected for processing (7)
README.mddocs/superpowers/plans/2026-06-28-v1-responses-shim.mddocs/superpowers/specs/2026-06-28-v1-responses-shim-design.mdresponses_api.pyshim.pytests/test_responses_api.pytests/test_responses_shim.py
| def tools_to_chat(tools: Any) -> "list[dict] | None": | ||
| """Responses tools (FLAT {type:"function", name, ...}) -> chat-completions | ||
| tools (NESTED {type:"function", function:{name,...}}). Inverse of | ||
| codex_backend._to_responses_tools. Unknown/native tool types pass through.""" | ||
| if not tools: | ||
| return None | ||
| out: list[dict] = [] | ||
| for t in tools: | ||
| if not isinstance(t, dict): | ||
| continue | ||
| if t.get("type") == "function" and t.get("name"): | ||
| fn: dict = {"name": t.get("name")} | ||
| if t.get("description") is not None: | ||
| fn["description"] = t["description"] | ||
| if t.get("parameters") is not None: | ||
| fn["parameters"] = t["parameters"] | ||
| out.append({"type": "function", "function": fn}) | ||
| else: | ||
| out.append(t) # native/unknown tool — best-effort pass-through | ||
| return out or None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
tools_to_chat shows outdated pass-through behavior; must drop non-function tools.
The plan's tools_to_chat appends unknown/native tool types unchanged (else: out.append(t)). This contradicts the design spec (line 109-117: "Any other type… → dropped, not forwarded") and the actual implementation which drops them to avoid 400 errors from chat providers. The design spec even notes this was "[Updated from the original 'pass-through' after the final code review.]" — but the implementation plan was never updated. Update to filter and return only type == "function" tools.
def tools_to_chat(tools: Any) -> "list[dict] | None":
- """Responses tools (FLAT {type:"function", name, ...}) -> chat-completions
- tools (NESTED {type:"function", function:{name,...}}). Inverse of
- codex_backend._to_responses_tools. Unknown/native tool types pass through."""
+ """Responses tools (FLAT {type:"function", name, ...}) -> chat-completions
+ tools (NESTED {type:"function", function:{name,...}}). Inverse of
+ codex_backend._to_responses_tools. Non-function tools are dropped."""
if not tools:
return None
out: list[dict] = []
for t in tools:
if not isinstance(t, dict):
continue
- if t.get("type") == "function" and t.get("name"):
+ if t.get("type") == "function" and t.get("name"):
fn: dict = {"name": t.get("name")}
if t.get("description") is not None:
fn["description"] = t["description"]
if t.get("parameters") is not None:
fn["parameters"] = t["parameters"]
out.append({"type": "function", "function": fn})
- else:
- out.append(t) # native/unknown tool — best-effort pass-through
return out or None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def tools_to_chat(tools: Any) -> "list[dict] | None": | |
| """Responses tools (FLAT {type:"function", name, ...}) -> chat-completions | |
| tools (NESTED {type:"function", function:{name,...}}). Inverse of | |
| codex_backend._to_responses_tools. Unknown/native tool types pass through.""" | |
| if not tools: | |
| return None | |
| out: list[dict] = [] | |
| for t in tools: | |
| if not isinstance(t, dict): | |
| continue | |
| if t.get("type") == "function" and t.get("name"): | |
| fn: dict = {"name": t.get("name")} | |
| if t.get("description") is not None: | |
| fn["description"] = t["description"] | |
| if t.get("parameters") is not None: | |
| fn["parameters"] = t["parameters"] | |
| out.append({"type": "function", "function": fn}) | |
| else: | |
| out.append(t) # native/unknown tool — best-effort pass-through | |
| return out or None | |
| def tools_to_chat(tools: Any) -> "list[dict] | None": | |
| """Responses tools (FLAT {type:"function", name, ...}) -> chat-completions | |
| tools (NESTED {type:"function", function:{name,...}}). Inverse of | |
| codex_backend._to_responses_tools. Non-function tools are dropped.""" | |
| if not tools: | |
| return None | |
| out: list[dict] = [] | |
| for t in tools: | |
| if not isinstance(t, dict): | |
| continue | |
| if t.get("type") == "function" and t.get("name"): | |
| fn: dict = {"name": t.get("name")} | |
| if t.get("description") is not None: | |
| fn["description"] = t["description"] | |
| if t.get("parameters") is not None: | |
| fn["parameters"] = t["parameters"] | |
| out.append({"type": "function", "function": fn}) | |
| return out or None |
🤖 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 `@docs/superpowers/plans/2026-06-28-v1-responses-shim.md` around lines 319 -
338, `tools_to_chat` still preserves unknown/native tool entries in its else
branch, but the intended behavior is to drop anything that is not a function
tool. Update the `tools_to_chat` logic so it only converts and appends items
with `type == "function"` and a `name`, and otherwise skips them entirely; keep
the rest of the function shape the same so it remains the inverse of
`codex_backend._to_responses_tools`.
| def tools_to_chat(tools: Any) -> "list[dict] | None": | ||
| """Responses tools (FLAT {type:"function", name, ...}) -> chat-completions | ||
| tools (NESTED {type:"function", function:{name,...}}). Inverse of | ||
| codex_backend._to_responses_tools. Unknown/native tool types pass through.""" | ||
| if not tools: | ||
| return None | ||
| out: list[dict] = [] | ||
| for t in tools: | ||
| if not isinstance(t, dict): | ||
| continue | ||
| if t.get("type") == "function" and t.get("name"): | ||
| fn: dict = {"name": t.get("name")} | ||
| if t.get("description") is not None: | ||
| fn["description"] = t["description"] | ||
| if t.get("parameters") is not None: | ||
| fn["parameters"] = t["parameters"] | ||
| out.append({"type": "function", "function": fn}) | ||
| else: | ||
| out.append(t) # native/unknown tool — best-effort pass-through | ||
| return out or None | ||
|
|
||
|
|
||
| def tool_choice_to_chat(tc: Any) -> Any: | ||
| """Inverse of codex_backend._to_responses_tool_choice. Strings pass through; | ||
| {type:"function", name} -> {type:"function", function:{name}}.""" | ||
| if tc is None or isinstance(tc, str): | ||
| return tc | ||
| if isinstance(tc, dict) and tc.get("type") == "function": | ||
| name = tc.get("name") | ||
| if name is None and isinstance(tc.get("function"), dict): | ||
| name = tc["function"].get("name") | ||
| if name: | ||
| return {"type": "function", "function": {"name": name}} | ||
| return None | ||
|
|
||
|
|
||
| # ---- id helpers (shared by the response-out half, Tasks 2-3) -------------- | ||
|
|
||
| def _new_id(prefix: str) -> str: | ||
| return f"{prefix}_{uuid.uuid4().hex[:24]}" | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Missing dropped_tool_types helper and its usage in _handle_responses.
The design spec references dropped_tool_types(tools) (line 115) to expose what was discarded, and the actual shim.py calls it to log a warning. The implementation plan omits this helper entirely. Add the helper to Task 1 and the logging call to Task 4's _handle_responses so the plan matches the final implementation.
🤖 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 `@docs/superpowers/plans/2026-06-28-v1-responses-shim.md` around lines 319 -
359, Add the missing dropped_tool_types(tools) helper in the tool-conversion
section alongside tools_to_chat and tool_choice_to_chat so it can report which
incoming tool types were discarded. Then update _handle_responses to call that
helper and emit the warning/logging that surfaces the dropped tool types,
matching the behavior described in the spec and the final shim. Keep the helper
consistent with the existing conversion utilities and ensure it integrates
cleanly with the responses-to-chat tool handling path.
| async def _handle_responses(req: ResponsesRequest, profile_name: str | None = None): | ||
| import responses_api as _rapi | ||
| chatreq = ChatRequest( | ||
| model=req.model or "", | ||
| messages=_rapi.input_to_messages(req.input, req.instructions), | ||
| tools=_rapi.tools_to_chat(req.tools), | ||
| tool_choice=_rapi.tool_choice_to_chat(req.tool_choice), | ||
| temperature=req.temperature, | ||
| max_tokens=req.max_output_tokens, | ||
| policy_ir=req.policy_ir, | ||
| session=req.session, | ||
| ) | ||
| contract = _request_to_contract(chatreq, default_profile, default_max_tokens) | ||
| if profile_name is not None: | ||
| contract["profile"] = profile_name | ||
|
|
||
| if not req.stream: | ||
| try: | ||
| result = await host.execute_async(contract) | ||
| except Exception as exc: | ||
| admission = _policy_admission_error(exc) | ||
| if admission is None: | ||
| raise | ||
| return _invalid_policy_response(admission) | ||
| if not result.get("ok"): | ||
| return _openai_error_from_router(result) | ||
| return _responses_object_with_router(result, req) | ||
| return await _handle_responses_stream(contract, req) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
_handle_responses missing dropped-tool warning and dropped_tool_types call.
The plan's _handle_responses jumps straight to building ChatRequest. The actual implementation first checks for dropped tools and logs a warning (mirroring the design spec's requirement to make "a needed-but-dropped tool visible"). Add:
async def _handle_responses(req: ResponsesRequest, profile_name: str | None = None):
import responses_api as _rapi
+ dropped = _rapi.dropped_tool_types(req.tools)
+ if dropped:
+ _log.warning("responses: dropped non-function tool types %s "
+ "(chat providers accept only function tools)", dropped)
chatreq = ChatRequest(...)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def _handle_responses(req: ResponsesRequest, profile_name: str | None = None): | |
| import responses_api as _rapi | |
| chatreq = ChatRequest( | |
| model=req.model or "", | |
| messages=_rapi.input_to_messages(req.input, req.instructions), | |
| tools=_rapi.tools_to_chat(req.tools), | |
| tool_choice=_rapi.tool_choice_to_chat(req.tool_choice), | |
| temperature=req.temperature, | |
| max_tokens=req.max_output_tokens, | |
| policy_ir=req.policy_ir, | |
| session=req.session, | |
| ) | |
| contract = _request_to_contract(chatreq, default_profile, default_max_tokens) | |
| if profile_name is not None: | |
| contract["profile"] = profile_name | |
| if not req.stream: | |
| try: | |
| result = await host.execute_async(contract) | |
| except Exception as exc: | |
| admission = _policy_admission_error(exc) | |
| if admission is None: | |
| raise | |
| return _invalid_policy_response(admission) | |
| if not result.get("ok"): | |
| return _openai_error_from_router(result) | |
| return _responses_object_with_router(result, req) | |
| return await _handle_responses_stream(contract, req) | |
| async def _handle_responses(req: ResponsesRequest, profile_name: str | None = None): | |
| import responses_api as _rapi | |
| dropped = _rapi.dropped_tool_types(req.tools) | |
| if dropped: | |
| _log.warning("responses: dropped non-function tool types %s " | |
| "(chat providers accept only function tools)", dropped) | |
| chatreq = ChatRequest( | |
| model=req.model or "", | |
| messages=_rapi.input_to_messages(req.input, req.instructions), | |
| tools=_rapi.tools_to_chat(req.tools), | |
| tool_choice=_rapi.tool_choice_to_chat(req.tool_choice), | |
| temperature=req.temperature, | |
| max_tokens=req.max_output_tokens, | |
| policy_ir=req.policy_ir, | |
| session=req.session, | |
| ) | |
| contract = _request_to_contract(chatreq, default_profile, default_max_tokens) | |
| if profile_name is not None: | |
| contract["profile"] = profile_name | |
| if not req.stream: | |
| try: | |
| result = await host.execute_async(contract) | |
| except Exception as exc: | |
| admission = _policy_admission_error(exc) | |
| if admission is None: | |
| raise | |
| return _invalid_policy_response(admission) | |
| if not result.get("ok"): | |
| return _openai_error_from_router(result) | |
| return _responses_object_with_router(result, req) | |
| return await _handle_responses_stream(contract, req) |
🤖 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 `@docs/superpowers/plans/2026-06-28-v1-responses-shim.md` around lines 957 -
984, The _handle_responses flow is missing the dropped-tool visibility check
before building ChatRequest. Update _handle_responses to first inspect the
incoming ResponsesRequest for dropped tools, call dropped_tool_types, and emit
the warning log when any tool types were dropped, then continue constructing the
ChatRequest and contract as before. Use the existing _handle_responses,
dropped_tool_types, and _request_to_contract symbols to place the warning in the
non-streaming and streaming path shared setup.
| class ResponsesRequest(BaseModel): | ||
| """Permissive OpenAI /v1/responses body. Unknown fields are kept | ||
| (extra="allow") so Responses params the shim does not read (reasoning, | ||
| include, store, parallel_tool_calls, prompt_cache_key, text, | ||
| previous_response_id, …) never break the request.""" | ||
| model_config = ConfigDict(extra="allow") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject previous_response_id explicitly instead of silently ignoring it.
Line 104 documents previous_response_id as ignored, but without server-side state this can return a 200 response missing prior context. Fail fast with a 400 so stateful clients know to resend full input.
Proposed fix
class ResponsesRequest(BaseModel):
@@
caller: str | None = None
+ previous_response_id: str | None = None
@@
async def _handle_responses(req: ResponsesRequest, profile_name: str | None = None):
import responses_api as _rapi
+ if req.previous_response_id:
+ return JSONResponse(status_code=400, content={"error": {
+ "message": "previous_response_id is not supported by this stateless shim; send full input context",
+ "type": "invalid_request_error",
+ "code": "previous_response_id_unsupported",
+ }})Also applies to: 906-907
🤖 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 `@shim.py` around lines 101 - 106, The ResponsesRequest model currently allows
previous_response_id to pass through silently, which can lead to stateful
clients getting a misleading 200 without prior context. Update the request
handling around ResponsesRequest in shim.py to explicitly detect
previous_response_id and reject it with a 400 error before processing. Keep the
model permissive for other unknown fields, but fail fast for
previous_response_id so callers know to resend the full input.
| temperature: float | None = None | ||
| policy_ir: list | None = None | ||
| session: str | None = None | ||
| caller: str | None = None |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not trust body-supplied caller for session ownership.
ResponsesRequest accepts caller from the public JSON body, and line 902 uses it as the session-meter owner. Overwrite it from the ingress header only; otherwise clients can spoof or squat session ownership.
Proposed fix
def _session_from_header(req: ChatRequest, request: Request) -> None:
@@
- caller = request.headers.get("x-llm-router-caller")
- if caller:
- req.caller = caller
+ # Only the ingress proxy may set caller identity.
+ req.caller = request.headers.get("x-llm-router-caller") or NoneAlso applies to: 895-902
🤖 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 `@shim.py` at line 118, The session owner is currently taken from the public
ResponsesRequest.caller field, which can be spoofed by clients. Update
ResponsesRequest handling in shim.py so caller is not trusted from the JSON body
and is instead overwritten from the ingress header before any
ownership/session-meter logic runs. Make the fix in the request
parsing/normalization path and the code that uses caller as the owner so the
session-meter ownership always comes from the ingress-derived value.
| def test_input_items_plain_messages(): | ||
| items = [{"role": "user", "content": "a"}, {"role": "assistant", "content": "b"}] | ||
| assert ra.input_to_messages(items) == items | ||
|
|
||
|
|
||
| def test_input_content_parts_are_flattened(): | ||
| items = [{"role": "user", "content": [ | ||
| {"type": "input_text", "text": "a"}, {"type": "input_text", "text": "b"}]}] | ||
| assert ra.input_to_messages(items) == [{"role": "user", "content": "ab"}] | ||
|
|
||
|
|
||
| def test_message_type_wrapper_is_unwrapped(): | ||
| items = [{"type": "message", "role": "user", "content": "hi"}] | ||
| assert ra.input_to_messages(items) == [{"role": "user", "content": "hi"}] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add a regression test for singleton message objects.
These cases only cover {role, content} / {type:"message", ...} inside a list. In responses_api.py:57-63, a top-level dict currently takes the items = [] path, so a documented input shape would be silently dropped and this suite would not catch it. Please add a standalone-object case here and fix input_to_messages accordingly.
Suggested regression cases
def test_message_type_wrapper_is_unwrapped():
items = [{"type": "message", "role": "user", "content": "hi"}]
assert ra.input_to_messages(items) == [{"role": "user", "content": "hi"}]
+
+
+def test_top_level_message_wrapper_is_unwrapped():
+ item = {"type": "message", "role": "user", "content": "hi"}
+ assert ra.input_to_messages(item) == [{"role": "user", "content": "hi"}]
+
+
+def test_top_level_role_content_object_becomes_message():
+ item = {"role": "user", "content": "hi"}
+ assert ra.input_to_messages(item) == [{"role": "user", "content": "hi"}]🤖 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 `@tests/test_responses_api.py` around lines 30 - 43, Add a regression test in
test_input_items_plain_messages or a new test covering a singleton top-level
message dict passed directly to input_to_messages, since the current tests only
exercise list inputs and {type:"message"} wrappers inside lists. Update
input_to_messages in responses_api.py so a standalone dict with role/content is
normalized to a one-item messages list instead of being treated like an empty
items path, and keep the existing flattening/unwrapping behavior intact.
… backend The /v1/responses endpoint is a CLIENT-facing OpenAI-compat surface (the Codex CLI drives the router through it). codex_backend.py is a PROVIDER backend (it consumes a ChatGPT subscription, the opposite direction). They are unrelated finalities that merely share the external OpenAI Responses wire format; framing responses_api as "the inbound mirror of codex_backend" coupled two concerns with independent reasons to change. - responses_api.py: reword the docstrings — it implements the OpenAI Responses wire contract as a sibling of /v1/chat/completions, not as the inverse of the provider backend. SSE event names are cited from the public spec. - tests/test_responses_api.py: drop `import codex_backend`. The three symmetry tests generated their inputs from codex_backend internals, so a provider-driven change there could break the client-surface tests. Replace with explicit real Responses-API payloads (what the Codex CLI actually sends) asserted directly. Behaviour unchanged: responses_api has no runtime dependency on codex_backend (never did); this only removes the narrative and test-level coupling. The two keep their own small wire translation so each surface stays free to evolve.
…rfaces The x_router metadata block and the per-session meter fold were copy-pasted verbatim in three places: _router_response_to_openai (chat unary), _final_chunk_parts (chat streaming) and the new _responses_object_with_router (/v1/responses). The /v1/responses PR inherited the existing 2-way duplication and made it 3-way. Extract a single module-level _build_x_router(result, subscription_providers, session, owner) and call it from all three sites. usage stays per-surface (chat prompt_tokens_details vs Responses input_tokens_details differ), so it is NOT folded in. Pure refactor — behaviour-preserving. Full suite 393 passed, 2 skipped before and after.
The PR committed docs/superpowers/{plans,specs}/2026-06-28-* — 1325 lines of an
agent workflow's spec + implementation plan (the plan literally instructs workers
to use the `superpowers:subagent-driven-development` sub-skill and references the
author's local checkout path). These are process scaffolding, not documentation
of the artifact; they will rot unread.
The repo's actual practice is that these docs are NOT committed: the existing
references in config.live.lua and sources/__init__.py point to a
docs/superpowers/specs/2026-06-10-provider-sources-design.md that was never
committed (a dangling ref to the author's local checkout). Committing the
2026-06-28 spec+plan is the anomaly; removing them restores consistency.
The /v1/responses endpoint stays documented for users where it belongs: the
README block already added by this PR. Also drop the one carried-over "mirror of
codex_backend.py" phrase from the README module list, to match the decoupled
framing (the surface is a sibling of /v1/chat/completions, not the provider
backend's inverse).
(Pre-existing: the two dangling config.live.lua / sources refs to the never-
committed 2026-06-10 doc are left untouched — not this PR's concern.)
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)
shim.py (1)
941-963: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSSE
sequence_numbercollides on the streaming failure path.In
gen_running, theresponse.createdevent is emitted with the defaultseq=0(line 943), butresponses_failed_event(...)is also called with its defaultseq=0(lines 952-954 and 958). Two frames sharingsequence_number=0violate the Responses API's strictly-increasing sequence contract and can trip strict clients (e.g. Codex CLI). The success path is fine becauseresponses_sse_eventsstarts atstart_seq=1.🔧 Proposed fix
except Exception as exc: admission = _policy_admission_error(exc) yield _rapi.responses_failed_event( rid, admission or f"responses error: {exc}", - "invalid_policy" if admission else "internal_error") + "invalid_policy" if admission else "internal_error", seq=1) return if not result.get("ok"): err = str(result.get("error") or "router error") - yield _rapi.responses_failed_event(rid, err, str(result.get("error") or "error")) + yield _rapi.responses_failed_event(rid, err, str(result.get("error") or "error"), seq=1) return🤖 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 `@shim.py` around lines 941 - 963, The streaming failure path in gen_running reuses the default sequence_number of 0 for both responses_created_event and responses_failed_event, which breaks the strictly increasing SSE contract. Update the failure branches in gen_running to pass an explicit sequence number after the initial created event, matching the success path behavior used by _responses_object_with_router and _rapi.responses_sse_events, so every emitted frame has a unique increasing sequence_number.
🤖 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 `@shim.py`:
- Around line 941-963: The streaming failure path in gen_running reuses the
default sequence_number of 0 for both responses_created_event and
responses_failed_event, which breaks the strictly increasing SSE contract.
Update the failure branches in gen_running to pass an explicit sequence number
after the initial created event, matching the success path behavior used by
_responses_object_with_router and _rapi.responses_sse_events, so every emitted
frame has a unique increasing sequence_number.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 637850b6-7074-4dd1-a5b1-419083e3a371
📒 Files selected for processing (4)
README.mdresponses_api.pyshim.pytests/test_responses_api.py
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- responses_api.py
On the streaming /v1/responses failure path (gen_running), response.created was emitted with the default sequence_number=0 and then response.failed was also emitted with the default seq=0 — two frames sharing sequence_number=0, which violates the Responses API's strictly-increasing sequence contract and can trip strict clients (the Codex CLI). The success path was already correct because responses_sse_events starts at start_seq=1. Behaviour change (named): on the streaming failure path the response.failed frame now carries sequence_number=1 (it follows response.created at 0). No other path changes — gen_ready and the success branch were already strictly increasing. Add a regression test that deterministically drives gen_running (collapse the early-fail window + an execute_async that suspends before returning a router error) and asserts every numbered SSE frame is strictly increasing. Verified it fails on the pre-fix code (sequence_numbers [0, 0]) and passes after. Found by CodeRabbit on the review-repair revision. Full suite 393->394 passed (the new test), 2 skipped, before and after.
2 participants
What
Adds
POST /v1/responsesandPOST /{profile_name}/v1/responses— an OpenAI Responses API surface for the router, so Responses-only clients (notably the Codex CLI, which speaks onlywire_api="responses") can drive it. Today the router serves only/v1/chat/completions, so Codex gets404 /v1/responsesand can't use it at all.How
The endpoint is the inbound mirror of
codex_backend.py(which already does the outbound chat↔Responses translation). New pure moduleresponses_api.py; thin glue inshim.py.input/instructions→messages(inverse ofcodex_backend._messages_to_input), flattools→ nested,tool_choice,max_output_tokens→max_tokens, model-prefix convention. Runs the samehost.execute_async, same policy admission, same per-session metering, samex_router.responseobject (non-stream) or a faithful Responses SSE event stream (response.created→output_item/output_text/function_call_argumentsevents →response.completed), pseudo-streamed from the complete result with heartbeats. No core changes, nocodex_backend.pychanges.Notable design points (from review + live testing)
type:"namespace"(multi_agent) andtype:"web_search"tools alongside its 8functiontools. A chat-completions provider rejects any non-functiontool, which would 400 the whole turn (killing the shell tool too). The shim drops unknown tool types and logs them, so the text turn — and all function tools incl.exec_command— survive.role:"developer"→system. Codex sends adeveloperinput message (OpenAI's system-equivalent); normalized tosystemfor non-OpenAI provider portability.[DONE]sentinel (the Responses API ends onresponse.completed).previous_response_id/server-side state is unsupported by design (Codex sends full input withstore:false).Tests
tests/test_responses_api.py— 31 unit tests (pure translation, incl. a round-trip symmetry assertion againstcodex_backend._messages_to_input).tests/test_responses_shim.py— 6 integration tests (realLLMRouterHost+ mock provider responses +TestClient).Live acceptance — real Codex v0.142.1 binary
Drove the real
codex execagainst this shim (mock backend, no provider key):developerrole, content-parts), parsed the Responses SSE, rendered the answer, and read usage. ✅function_callevent → ranexec_command(echo codex-tool-ok) → sentfunction_call_outputback → shim translated it to atoolmessage → Codex finished cleanly. ✅Docs
Design + implementation plan under
docs/superpowers/;README.mdupdated.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation