feat(openai): support codex models via the Responses API (#87) - #318
feat(openai): support codex models via the Responses API (#87)#318proyectoauraorg wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDetects Codex models and routes streaming createMessage and non-streaming completePrompt through the OpenAI Responses API; adds Responses-based streaming handler, non-streaming completion helper, Anthropic→Responses message/tool converters, MCP tool handling, and a comprehensive Vitest test suite. ChangesCodex Model Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/api/providers/__tests__/openai-codex-responses.spec.ts (3)
223-225: 💤 Low valueAdd comment to clarify chunk consumption.
The loop consumes the async generator without processing chunks. Adding a brief comment would clarify that this is intentional (e.g.,
// Consume stream to trigger API call).📝 Suggested comment addition
+ // Consume stream to trigger the API call and populate mock call history for await (const _chunk of handler.createMessage("You are helpful", messages, { taskId: "test" })) { - // consume + // (chunks not needed for this test) }🤖 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 `@src/api/providers/__tests__/openai-codex-responses.spec.ts` around lines 223 - 225, The for-await loop that iterates over handler.createMessage("You are helpful", messages, { taskId: "test" }) intentionally discards chunks to trigger the async generator side effects; add a brief inline comment above or inside the loop (e.g., next to the empty body) stating its purpose such as "// Consume stream to trigger API call" so readers know dropping _chunk is deliberate when testing createMessage.
1-466: ⚡ Quick winAdd test coverage for tool schema conversion.
The PR objectives mention "flat tool-schema conversion" as a core feature, but no test verifies that Anthropic-style tool definitions are correctly converted to the Responses API's flat function schema. Consider adding a test that:
- Passes tools in the
createMessageoptions- Verifies the request body's
toolsparameter uses the Responses API formatAs per coding guidelines, use package-local unit tests for serialization and request construction, which would include tool schema conversion.
🤖 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 `@src/api/providers/__tests__/openai-codex-responses.spec.ts` around lines 1 - 466, Add a unit test that calls OpenAiHandler.createMessage with an options.tools array (Anthropic-style tool definitions) and asserts that mockResponsesCreate was invoked and that the captured requestBody.tools contains the flattened Responses API function schema (e.g., each tool has top-level name, description, and parameters instead of nested Anthropic structures); locate createMessage and the mockResponsesCreate usage in the existing spec (tests under OpenAiHandler) and add assertions on mockResponsesCreate.mock.calls[0][0].tools to validate the conversion/serialization of tool definitions.
1-466: ⚡ Quick winAdd test coverage for reasoning stream events.
The PR objectives state support for "streaming (text, reasoning, tool calls, usage)", but the test suite doesn't include tests for reasoning stream events. Consider adding a test that verifies
response.reasoning.delta(or similar event type) is correctly parsed and emitted as reasoning chunks.As per coding guidelines, use package-local unit tests for parsing and state transitions, which would include reasoning event parsing.
🤖 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 `@src/api/providers/__tests__/openai-codex-responses.spec.ts` around lines 1 - 466, Add a unit test to src/api/providers/__tests__/openai-codex-responses.spec.ts that mocks mockResponsesCreate to yield a response.reasoning.delta event (with some delta text) followed by a response.done, then calls OpenAiHandler.createMessage and asserts the async iterator emits a chunk with type "reasoning" (or whatever the handler emits for reasoning events) containing the delta text and that usage is emitted on completion; target the createMessage method on OpenAiHandler and the mockResponsesCreate stream to validate parsing/state transitions for reasoning events.
🤖 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 `@src/api/providers/openai.ts`:
- Around line 840-855: The current event handling block checks for event?.type
=== "response.error" || event?.type === "error" and event?.type ===
"response.failed" but only throws if event.error or event.message exist,
allowing silent ignores; update the logic in the event handler around the event
variable so that whenever event.type is "response.error" or "error" you throw a
new Error(`Responses API error: ${event.error?.message || event.message ||
"Unknown error"}`) unconditionally (move the throw outside the inner
conditional), and similarly when event.type is "response.failed" always throw
new Error(`Response failed: ${event.error?.message || event.message || "Unknown
failure"}`) so the stream fails immediately even if error/message are missing.
---
Nitpick comments:
In `@src/api/providers/__tests__/openai-codex-responses.spec.ts`:
- Around line 223-225: The for-await loop that iterates over
handler.createMessage("You are helpful", messages, { taskId: "test" })
intentionally discards chunks to trigger the async generator side effects; add a
brief inline comment above or inside the loop (e.g., next to the empty body)
stating its purpose such as "// Consume stream to trigger API call" so readers
know dropping _chunk is deliberate when testing createMessage.
- Around line 1-466: Add a unit test that calls OpenAiHandler.createMessage with
an options.tools array (Anthropic-style tool definitions) and asserts that
mockResponsesCreate was invoked and that the captured requestBody.tools contains
the flattened Responses API function schema (e.g., each tool has top-level name,
description, and parameters instead of nested Anthropic structures); locate
createMessage and the mockResponsesCreate usage in the existing spec (tests
under OpenAiHandler) and add assertions on
mockResponsesCreate.mock.calls[0][0].tools to validate the
conversion/serialization of tool definitions.
- Around line 1-466: Add a unit test to
src/api/providers/__tests__/openai-codex-responses.spec.ts that mocks
mockResponsesCreate to yield a response.reasoning.delta event (with some delta
text) followed by a response.done, then calls OpenAiHandler.createMessage and
asserts the async iterator emits a chunk with type "reasoning" (or whatever the
handler emits for reasoning events) containing the delta text and that usage is
emitted on completion; target the createMessage method on OpenAiHandler and the
mockResponsesCreate stream to validate parsing/state transitions for reasoning
events.
🪄 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 Plus
Run ID: 6df82370-c3b6-4783-9507-dc0e91aef4d9
📒 Files selected for processing (2)
src/api/providers/__tests__/openai-codex-responses.spec.tssrc/api/providers/openai.ts
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/api/providers/__tests__/openai-codex-responses.spec.ts`:
- Around line 248-300: Add a new unit test in the same spec that mirrors the
existing "converts tools to the flat Responses API schema" case but supplies an
MCP-named tool (e.g., function.name like "mcp__server__tool_name") and asserts
the constructed request (from mockResponsesCreate.mock.calls[0][0]) sets
tool.strict to false; reuse the same setup: instantiate OpenAiHandler, stub
mockResponsesCreate to yield a response, call handler.createMessage(...) with
the MCP tool, consume the async iterator, then grab requestBody and assert
requestBody.tools[0].strict === false to cover the MCP code path alongside the
existing non-MCP test.
🪄 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 Plus
Run ID: d49ccc2a-09d2-4d9d-8115-6993f5edb813
📒 Files selected for processing (1)
src/api/providers/__tests__/openai-codex-responses.spec.ts
| it("converts tools to the flat Responses API schema (not nested under `function`)", async () => { | ||
| handler = new OpenAiHandler({ | ||
| openAiApiKey: "test-key", | ||
| openAiModelId: "gpt-5.3-codex", | ||
| openAiBaseUrl: "https://test.openai.azure.com/openai/deployments/gpt5.3", | ||
| openAiUseAzure: true, | ||
| }) | ||
|
|
||
| mockResponsesCreate.mockResolvedValue({ | ||
| [Symbol.asyncIterator]: async function* () { | ||
| yield { type: "response.done", response: { usage: { input_tokens: 1, output_tokens: 1 } } } | ||
| }, | ||
| }) | ||
|
|
||
| const messages: Anthropic.Messages.MessageParam[] = [{ role: "user", content: "Hi" }] | ||
| // Chat Completions nested tool shape (what the rest of the handler receives). | ||
| const tools = [ | ||
| { | ||
| type: "function", | ||
| function: { | ||
| name: "calculator", | ||
| description: "Evaluate a math expression", | ||
| parameters: { | ||
| type: "object", | ||
| properties: { expression: { type: "string" } }, | ||
| required: ["expression"], | ||
| }, | ||
| }, | ||
| }, | ||
| ] | ||
|
|
||
| for await (const _chunk of handler.createMessage("You are helpful", messages, { | ||
| taskId: "test", | ||
| tools, | ||
| } as any)) { | ||
| // Consume the stream so responses.create is invoked. | ||
| } | ||
|
|
||
| const requestBody = mockResponsesCreate.mock.calls[0][0] | ||
| expect(Array.isArray(requestBody.tools)).toBe(true) | ||
| expect(requestBody.tools).toHaveLength(1) | ||
|
|
||
| const tool = requestBody.tools[0] | ||
| // Flat structure: fields live at the top level, not under `function`. | ||
| expect(tool.type).toBe("function") | ||
| expect(tool.name).toBe("calculator") | ||
| expect(tool.description).toBe("Evaluate a math expression") | ||
| expect(tool.function).toBeUndefined() | ||
| expect(tool.parameters).toBeDefined() | ||
| expect(tool.parameters.properties).toBeDefined() | ||
| // Non-MCP tools are sent with strict schema validation enabled. | ||
| expect(tool.strict).toBe(true) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add test coverage for MCP tool handling.
The layer description mentions "with MCP handling" and the comment on line 298-299 states "Non-MCP tools are sent with strict schema validation enabled," implying MCP tools have different behavior (likely strict: false).
Add a test case that verifies MCP tools are converted to the flat schema with strict: false to ensure both code paths are covered.
🧪 Example test structure for MCP tools
it("converts MCP tools to flat schema with strict: false", async () => {
handler = new OpenAiHandler({
openAiApiKey: "test-key",
openAiModelId: "gpt-5.3-codex",
openAiUseAzure: true,
})
mockResponsesCreate.mockResolvedValue({
[Symbol.asyncIterator]: async function* () {
yield { type: "response.done", response: { usage: { input_tokens: 1, output_tokens: 1 } } }
},
})
const messages: Anthropic.Messages.MessageParam[] = [{ role: "user", content: "Hi" }]
// MCP tool naming pattern
const tools = [{
type: "function",
function: {
name: "mcp__server__tool_name", // or whatever MCP pattern is used
description: "An MCP tool",
parameters: { type: "object", properties: {}, required: [] },
},
}]
for await (const _chunk of handler.createMessage("System", messages, {
taskId: "test",
tools,
} as any)) {
// consume
}
const requestBody = mockResponsesCreate.mock.calls[0][0]
const tool = requestBody.tools[0]
expect(tool.strict).toBe(false) // MCP tools should NOT use strict validation
})As per coding guidelines: "Use package-local unit tests for pure logic, parsing, state transitions, validation, serialization, request construction, retry decisions, and error handling." This test file should cover both MCP and non-MCP request construction paths.
🤖 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 `@src/api/providers/__tests__/openai-codex-responses.spec.ts` around lines 248
- 300, Add a new unit test in the same spec that mirrors the existing "converts
tools to the flat Responses API schema" case but supplies an MCP-named tool
(e.g., function.name like "mcp__server__tool_name") and asserts the constructed
request (from mockResponsesCreate.mock.calls[0][0]) sets tool.strict to false;
reuse the same setup: instantiate OpenAiHandler, stub mockResponsesCreate to
yield a response, call handler.createMessage(...) with the MCP tool, consume the
async iterator, then grab requestBody and assert requestBody.tools[0].strict ===
false to cover the MCP code path alongside the existing non-MCP test.
There was a problem hiding this comment.
@coderabbitai Fair point — MCP tools do behave differently (they are sent with strict: false to allow flexible schemas). I will add a dedicated test case that:
- Passes an MCP-style tool (with
inputSchemainstead ofparameters) - Asserts the converted output has
strict: false - Verifies the
nameanddescriptionare correctly mapped
This will close the gap between the layer description ("with MCP handling") and the actual test assertions. Will push in the next commit.
There was a problem hiding this comment.
@proyectoauraorg Great plan — using inputSchema instead of parameters for the MCP tool shape is a nice touch that makes the test even more realistic, since that's the actual difference in the wild. The three assertions you listed (flat schema, strict: false, name/description mapping) cover the important contract. Looking forward to the next commit!
(ˆ(oo)ˆ)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/api/providers/__tests__/openai-codex-responses.spec.ts (1)
519-539: ⚡ Quick winAdd the no-details
response.errorcase too.Line 519 covers the
response.failedfallback, but the siblingresponse.error/errorbranch insrc/api/providers/openai.tsis still only exercised with a populated message. A paired unit test here would lock down both fallback paths from this change set.Proposed test addition
it("throws on an error event even when it carries no error/message details", async () => { handler = new OpenAiHandler({ openAiApiKey: "test-key", openAiModelId: "gpt-5.3-codex", openAiUseAzure: true, }) mockResponsesCreate.mockResolvedValue({ [Symbol.asyncIterator]: async function* () { yield { type: "response.failed" } }, }) await expect(async () => { for await (const _chunk of handler.createMessage("System", [{ role: "user", content: "Hello" }], { taskId: "test", })) { // consume } }).rejects.toThrow("Response failed: Unknown failure") }) + + it("throws on response.error even when it carries no error/message details", async () => { + handler = new OpenAiHandler({ + openAiApiKey: "test-key", + openAiModelId: "gpt-5.3-codex", + openAiUseAzure: true, + }) + + mockResponsesCreate.mockResolvedValue({ + [Symbol.asyncIterator]: async function* () { + yield { type: "response.error" } + }, + }) + + await expect(async () => { + for await (const _chunk of handler.createMessage("System", [{ role: "user", content: "Hello" }], { + taskId: "test", + })) { + // consume + } + }).rejects.toThrow("Responses API error: Unknown error") + })As per coding guidelines, "Use package-local unit tests for pure logic, parsing, state transitions, validation, serialization, request construction, retry decisions, and error handling."
🤖 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 `@src/api/providers/__tests__/openai-codex-responses.spec.ts` around lines 519 - 539, Extend the existing test for no-details failure by adding sibling cases that exercise the "response.error" and bare "error" branches: use the same OpenAiHandler and mockResponsesCreate pattern (the async iterator returned by mockResponsesCreate) but yield { type: "response.error" } and a separate test yielding { type: "error" } (both without message fields), then assert that awaiting handler.createMessage(...) rejects with the same "Response failed: Unknown failure" error; reference the OpenAiHandler class, the createMessage method, and mockResponsesCreate when locating where to add these tests.
🤖 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.
Nitpick comments:
In `@src/api/providers/__tests__/openai-codex-responses.spec.ts`:
- Around line 519-539: Extend the existing test for no-details failure by adding
sibling cases that exercise the "response.error" and bare "error" branches: use
the same OpenAiHandler and mockResponsesCreate pattern (the async iterator
returned by mockResponsesCreate) but yield { type: "response.error" } and a
separate test yielding { type: "error" } (both without message fields), then
assert that awaiting handler.createMessage(...) rejects with the same "Response
failed: Unknown failure" error; reference the OpenAiHandler class, the
createMessage method, and mockResponsesCreate when locating where to add these
tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cd030809-66e8-4315-9694-4909ee388700
📒 Files selected for processing (2)
src/api/providers/__tests__/openai-codex-responses.spec.tssrc/api/providers/openai.ts
…g#87) Azure-hosted GPT-5.x codex models (e.g. gpt-5.3-codex) reject the Chat Completions API and require the Responses API, so they could not be used via the OpenAI-compatible provider, which routed everything through chat.completions.create. createMessage/completePrompt now detect codex models and route them through the Responses API (responses.create) with streaming, Anthropic->Responses input conversion, and flat tool-schema conversion. Non-codex models keep using chat.completions unchanged. Ports RooCode PR #11952 (unmerged upstream). Adds openai-codex-responses.spec.ts (13 tests). Existing openai tests unchanged.
…e-Org#87) Error/failed events without error/message fields were silently ignored, risking continued processing of a broken stream. Throw unconditionally on response.error/error/response.failed (with an Unknown error/failure fallback). Adds a test for the no-details case.
b2a9390 to
b2ea01f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/api/providers/openai.ts`:
- Around line 96-99: The Codex path bypasses the non-streaming branch and forces
streaming by calling handleCodexMessage unconditionally; modify the branch that
checks _isCodexModel(modelId) to respect the provider's openAiStreamingEnabled
flag: if openAiStreamingEnabled is true, call/yield*
handleCodexMessage(systemPrompt, messages, metadata) as now, otherwise either
fall through to the existing non-streaming handling code or call
handleCodexMessage with a parameter/configuration to disable streaming so Codex
models use the same non-streaming transport when openAiStreamingEnabled is
false.
- Around line 599-607: The requestBody sets store: false which prevents callers
from using ApiHandlerCreateMessageMetadata.store; remove the hardcoded
store:false and instead set store from the incoming metadata (e.g., use
metadata?.store with the appropriate default true behavior), so the requestBody
creation (the object built around model.id, formattedInput, stream,
instructions, tools, and parallel_tool_calls) respects
ApiHandlerCreateMessageMetadata.store rather than forcing false.
- Around line 380-391: The current loop over response.output returns on the
first content.type === "output_text" and drops subsequent parts; instead, in the
code that extracts text from the Responses API (look for variables response,
outputItem, content and the completePrompt logic), collect each content.text
where content.type === "output_text' into a buffer (array or string builder)
while iterating through all output items and their content, then
join/concatenate all collected pieces and return the combined string after the
loops so the full completion is returned rather than only the first chunk.
🪄 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 Plus
Run ID: 7caed35a-670c-49c7-ada7-c80b5d32a93f
📒 Files selected for processing (2)
src/api/providers/__tests__/openai-codex-responses.spec.tssrc/api/providers/openai.ts
- Fix potential infinite loop when reading truncated output (artifact_id guard) - Add defensive checks for empty/missing tool call fields - Improve error messages with more context for debugging
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)
src/api/providers/openai.ts (1)
953-956:⚠️ Potential issue | 🟠 Major | ⚡ Quick winURL-based images are not handled correctly.
Anthropic's
ImageBlockParam.sourcecan be either base64 ({ type: 'base64', data, media_type }) or URL ({ type: 'url', url }). This code assumes base64, so URL-based images will produce an invalid data URL likedata:undefined;base64,undefined.🐛 Proposed fix to handle both image source types
} else if (block.type === "image") { const image = block as Anthropic.Messages.ImageBlockParam - const imageUrl = `data:${image.source.media_type};base64,${image.source.data}` - content.push({ type: "input_image", image_url: imageUrl }) + if (image.source.type === "base64") { + const imageUrl = `data:${image.source.media_type};base64,${image.source.data}` + content.push({ type: "input_image", image_url: imageUrl }) + } else if (image.source.type === "url") { + content.push({ type: "input_image", image_url: image.source.url }) + } }🤖 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 `@src/api/providers/openai.ts` around lines 953 - 956, The image block handling in the else-if branch (block.type === "image") assumes base64 only and constructs a data URL from image.source.media_type and image.source.data; update the logic in that branch (where image is cast to Anthropic.Messages.ImageBlockParam) to inspect image.source.type: if it is 'base64' build the data URL `data:{media_type};base64,{data}` as before, if it is 'url' set image_url to image.source.url, and otherwise skip/log the block or handle gracefully so you don't produce an invalid data URL for URL-based images.
🤖 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 `@src/api/providers/openai.ts`:
- Around line 953-956: The image block handling in the else-if branch
(block.type === "image") assumes base64 only and constructs a data URL from
image.source.media_type and image.source.data; update the logic in that branch
(where image is cast to Anthropic.Messages.ImageBlockParam) to inspect
image.source.type: if it is 'base64' build the data URL
`data:{media_type};base64,{data}` as before, if it is 'url' set image_url to
image.source.url, and otherwise skip/log the block or handle gracefully so you
don't produce an invalid data URL for URL-based images.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 29d04d32-5056-45a1-9521-120eddccdcc8
📒 Files selected for processing (1)
src/api/providers/openai.ts
📊 Codecov Coverage NoteThe Plan to reach 80%:
I will push these test additions shortly to bring diff coverage above 80%. |
…ants and formatting (Zoo-Code-Org#87) Raises codex Responses-API patch coverage from ~54% to ~94% (target 80%) by exercising the previously untested branches: - non-streaming path: function_call/tool_call, top-level + message text, reasoning summary, usage (incl. cache tokens), object-arg stringify, errors - streaming variants: text.done-only, content_part, reasoning deltas, refusal, output_item.added/done message text, response.completed fallback + usage, legacy choices/usage shape - request body: max_output_tokens when includeMaxTokens is set - conversation formatting: image blocks, array tool_result content, assistant string content
|
Pushed test coverage for the Codex Responses-API path — the only red check was |
|
👋 Friendly ping — this PR has been open for 8 days. Summary: Adds support for Codex models via the OpenAI Responses API (#87).
Would appreciate a review when you have a moment 🙏 |
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Thanks so much for addressing this, sorry it took so long to get around to reviewing.
| } | ||
| } | ||
| } catch (error) { | ||
| throw handleOpenAIError(error, this.providerName) |
There was a problem hiding this comment.
The sibling handlers (openai-native.ts, openai-codex.ts) wrap caught errors in ApiProviderError and call TelemetryService.instance.captureException() before rethrowing. This catch — and the one at line 932 — skips that, so codex failures won't appear in the error dashboard. Worth aligning with the pattern in openai-native.ts:645–648?
| } | ||
| } | ||
| } catch (error) { | ||
| throw handleOpenAIError(error, this.providerName) |
There was a problem hiding this comment.
Same as line 679 — TelemetryService.captureException is missing here for the streaming path too. Errors thrown during streaming won't appear in telemetry.
|
|
||
| let response | ||
| try { | ||
| response = await (this.client as any).responses.create(requestBody) |
There was a problem hiding this comment.
The installed OpenAI SDK (v5.12.2) exposes responses: API.Responses as a typed property directly on the client (client.d.ts:233), so the as any cast isn't needed here or at lines 628/694. Removing them would also let us replace requestBody: any with the typed ResponseCreateParamsNonStreaming/ResponseCreateParamsStreaming and catch field-name typos at compile time.
| const useStreaming = this.options.openAiStreamingEnabled ?? true | ||
|
|
||
| // Build the request body (stream flag added per path below) | ||
| const requestBody: any = { |
There was a problem hiding this comment.
requestBody: any means the compiler won't catch invalid field names or wrong types. OpenAI.Responses.ResponseCreateParamsStreaming (and the non-streaming variant) are both exported from the SDK — worth switching once the as any client cast on line 628/694 is removed.
| * the Chat Completions API and must use the Responses API instead. | ||
| */ | ||
| protected _isCodexModel(modelId: string): boolean { | ||
| return modelId.toLowerCase().includes("codex") |
There was a problem hiding this comment.
This matches any model ID containing the substring "codex" — so my-codex-wrapper or gpt-4-codex-finetune set via the OpenAI-compatible provider would silently route to the Responses API. Could we anchor this to known patterns (e.g. /(^|[-_.])codex([-_.]|$)/i) or add an explicit opt-in flag?
| // Extract text, tool calls, and reasoning from response output | ||
| if (Array.isArray(response?.output)) { | ||
| for (const outputItem of response.output) { | ||
| if (outputItem?.type === "function_call" || outputItem?.type === "tool_call") { |
There was a problem hiding this comment.
ResponseFunctionToolCall in the SDK types type as always 'function_call' — is 'tool_call' ever returned by the Responses API, or is this a defensive alias? Same question for outputItem.tool_call_id (the SDK field is call_id).
| /** | ||
| * Formats an Anthropic message array into the Responses API input format. | ||
| */ | ||
| private _formatConversationForResponsesApi(messages: Anthropic.Messages.MessageParam[]): any[] { |
There was a problem hiding this comment.
src/api/transform/responses-api-input.ts already exports convertToResponsesApiInput for this exact Anthropic→Responses API conversion (used by xai.ts). Is there a reason for a separate private implementation here? One difference I noticed: the shared helper flushes pending user content before appending function_call_output items, which produces different ordering for mixed text+tool_result messages.
| } | ||
|
|
||
| const toolCalls = chunks.filter((c) => c.type === "tool_call") | ||
| expect(toolCalls.length).toBe(1) |
There was a problem hiding this comment.
This test doesn't reach the streamedToolCallIds.has(callId) dedup guard (openai.ts:806) because no arguments delta fires before the output_item.done. Could a test be added that emits both arguments.delta events AND an output_item.done for the same call_id, asserting that the output_item.done is suppressed (no duplicate tool_call chunk)?
|
|
||
| // Third item: function_call | ||
| expect(input[2].type).toBe("function_call") | ||
| expect(input[2].name).toBe("calculator") |
There was a problem hiding this comment.
The function_call item's call_id and arguments aren't asserted here — only type and name. A bug in the sanitizeOpenAiCallId wiring that corrupts the call ID would go undetected, and since input[3]'s call_id isn't checked either, a mismatch between the function_call and its output wouldn't be caught.
|
|
||
| // Fourth item: function_call_output | ||
| expect(input[3].type).toBe("function_call_output") | ||
| expect(input[3].output).toBe("2") |
There was a problem hiding this comment.
The call_id on the function_call_output isn't asserted — this field is how the API pairs a tool result to its originating call. A mismatch (e.g. from the sanitizeOpenAiCallId transform) would silently break tool result attribution.
github-actions
Bot
added
the
awaiting-author
|
This PR has been awaiting author changes for 14 days and will be automatically closed in 7 days. Please address the review comments or leave a comment if you need more time. |
github-actions
Bot
added
the
stale-awaiting-author
|
Closing due to author inactivity after requested changes. Feel free to reopen once the requested changes have been addressed. |
2 participants
Closes #87
Problem
The OpenAI-compatible provider (
OpenAiHandler) routes every request throughchat.completions.create. Azure-hosted GPT-5.x codex models (e.g.gpt-5.3-codex) reject the Chat Completions API ("The chatCompletion operation does not work with the specified model") and require the Responses API, so they can't be used here today. This ports the fix from RooCode PR #11952, which is still unmerged upstream (hence not present in Zoo).Fix
createMessage/completePromptnow detect codex models (_isCodexModel) and route them through the Responses API (responses.create) with streaming (text, reasoning, tool calls, usage), Anthropic→Responses input conversion, and flat tool-schema conversion. Non-codex models keep usingchat.completionsunchanged.Note on tool-call finalization
The codex stream emits
tool_call_partialduring argument deltas; streamed calls are finalized by the consumer's end-of-streamNativeToolCallParser.finalizeRawChunks()(Task.ts), and the completetool_callevent is guarded bystreamedToolCallIdsto avoid duplicates — consistent with how Zoo already consumes both event shapes.Tests
New
openai-codex-responses.spec.ts(13 tests: detection, routing, streaming, tools, errors). Existingopenai.spec.tsunchanged and green.tsc/eslint/prettierand the full openai suite (65 tests) pass locally.Summary by CodeRabbit
New Features
Tests