feat(responses): deliver structured output to routed openai-chat models - #985
feat(responses): deliver structured output to routed openai-chat models#985DevMello wants to merge 5 commits into
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughStructured output support preserves Responses ChangesStructured output handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ResponsesAPI
participant ResponsesParser
participant OpenAIChatAdapter
participant UpstreamChatCompletions
ResponsesAPI->>ResponsesParser: Parse text.format
ResponsesParser->>OpenAIChatAdapter: Pass options.textFormat
OpenAIChatAdapter->>UpstreamChatCompletions: Send response_format
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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
🤖 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/server/responses/core.ts`:
- Around line 1667-1669: Clear parsed._structuredOutput in the routed compaction
normalization block alongside the existing parsed.options.textFormat removal,
ensuring the synthetic compaction request is sent as plain prose to the Kiro
adapter. Add a regression test covering Kiro-routed compaction with text.format
and verify it succeeds with a prose summary.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3e283b5a-c413-40ee-bb21-25f779684ab0
📒 Files selected for processing (11)
docs/github-copilot-app.mdsrc/adapters/openai-chat.tssrc/responses/parser.tssrc/server/chat-completions.tssrc/server/responses/core.tssrc/types.tstests/chat-completions-endpoint.test.tstests/kiro-adapter.test.tstests/openai-chat-hardening.test.tstests/responses-compaction-routing.test.tstests/responses-parser.test.ts
💤 Files with no reviewable changes (1)
- src/server/chat-completions.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db284da38a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| delete parsed.options.parallelToolCalls; | ||
| // The compaction turn is a plain prose summary; a surviving structured-output format | ||
| // would force schema-constrained JSON into the synthetic compaction item. | ||
| delete parsed.options.textFormat; |
There was a problem hiding this comment.
When routed compaction is sent to a noncanonical openai-responses provider, deleting only parsed.options.textFormat does not change the payload: that adapter's buildRequest starts from parsed._rawBody and buildRoutedCompactionBody only removes tools/tool_choice/parallel. With --output-schema/text.format, the compaction summarizer still receives text.format and can be forced to emit schema JSON instead of the prose summary that the synthetic compaction item needs. Strip text from the raw compaction body as well.
Useful? React with 👍 / 👎.
| tool calls, prompt cache keys, metadata, and `response_format` on native Responses | ||
| routes. Routed `openai-chat` models reject `response_format` with HTTP 400 because | ||
| their structured-output support is not verified. Other Chat Completions fields, | ||
| including penalties, `n`, and logprobs, are not currently supported. | ||
| routes and routed `openai-chat` models (`json_object` and `json_schema` are | ||
| forwarded as-is; a backend without structured-output support returns its own |
There was a problem hiding this comment.
Because this change exposes response_format support to OpenAI-compatible clients, updating only docs/github-copilot-app.md leaves the hosted docs-site reference/guides without the new behavior; users reading the public docs still have no indication that routed openai-chat can accept structured output. Add the corresponding docs-site/ update, including locales if relevant, alongside this docs change.
AGENTS.md reference: AGENTS.md:L224-L225
Useful? React with 👍 / 👎.
|
✅ Deterministic PR hygiene checks passed. |
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 `@docs-site/src/content/docs/reference/proxy-formats.md`:
- Around line 141-145: Rewrite the structured-output section to distinguish the
two wire contracts: state that response_format is forwarded for routed
openai-chat models, native POST /v1/responses routes preserve text.format, and
text.format is translated to response_format only when /v1/responses routes to
openai-chat. Do not imply that response_format is accepted as the native
Responses request field.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1bcfaa10-d88a-4ceb-93b7-ed1024208ee4
📒 Files selected for processing (5)
docs-site/src/content/docs/reference/proxy-formats.mdsrc/adapters/openai-responses.tssrc/server/responses/core.tstests/responses-compaction-routing.test.tstests/server-kiro-completion-e2e.test.ts
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
This is valuable and the translation shape is correct, but the branch must be rebased before it is mergeable under the current repository gates.
I reviewed head f717017a. Preserving Responses text.format, re-nesting it as Chat Completions response_format, leaving native Responses passthrough on _rawBody, and stripping all structured-output state from routed compaction are the correct boundaries. Letting an incompatible chat backend return its own capability error is also preferable to OpenCodex silently returning unconstrained prose. I found no protocol blocker in the patch itself.
I built a local merge result against current dev (a5b37827) and verified:
- six focused parser/adapter/endpoint/compaction/Kiro suites — 192 passed, 0 failed
bun run typecheck— passed
The blocking issue is branch/gate state: this head is 570 commits behind current dev. Its enforce-target job fails because the old checkout does not contain .github/scripts/pr-quality-state.cjs, and no required exact-head ci run is attached. Please rebase the contributor branch onto current dev (do not merge dev into it), resolve any review-readiness checklist the updated gate adds, and rerun CI on the new exact head. Once that is green, I consider this worth merging.
f717017 to
1ebdd83
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
|
|
|
|
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 `@docs/github-copilot-app.md`:
- Around line 53-55: Update the native Responses wire-field description in the
surrounding documentation to state that direct POST /v1/responses callers must
use text.format, while routed openai-chat requests use response_format. Clarify
that Chat Completions input is converted to Responses text.format rather than
forwarded unchanged.
In `@src/adapters/openai-chat.ts`:
- Around line 827-840: Update the json_schema branch in the response-format
mapping to preserve schema-less formats by removing the schema presence
requirement and conditionally adding the schema field only when defined. Keep
the existing name, description, and strict handling, and add a round-trip
regression test covering json_schema without schema.
In `@tests/responses-compaction-routing.test.ts`:
- Around line 420-421: Remove the duplicate const json declaration in
tests/responses-compaction-routing.test.ts lines 420-421, retaining one response
JSON parse for the test callback. Also remove the duplicate const json
declaration in tests/server-kiro-completion-e2e.test.ts lines 244-245, retaining
one response JSON parse there; separate test callback scopes may keep distinct
json declarations.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 31306e04-6768-44ac-ba3f-d9b44714dd93
📒 Files selected for processing (14)
docs-site/src/content/docs/reference/proxy-formats.mddocs/github-copilot-app.mdsrc/adapters/openai-chat.tssrc/adapters/openai-responses.tssrc/responses/parser.tssrc/server/chat-completions.tssrc/server/responses/core.tssrc/types.tstests/chat-completions-endpoint.test.tstests/kiro-adapter.test.tstests/openai-chat-hardening.test.tstests/responses-compaction-routing.test.tstests/responses-parser.test.tstests/server-kiro-completion-e2e.test.ts
💤 Files with no reviewable changes (1)
- src/server/chat-completions.ts
| routes and routed `openai-chat` models (`json_object` and `json_schema` are | ||
| forwarded as-is; a backend without structured-output support returns its own | ||
| error). Other Chat Completions fields, including penalties, `n`, and logprobs, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Correct the native Responses wire-field description.
response_format is not forwarded as-is to a native Responses provider. Chat Completions input is converted to Responses text.format. Direct POST /v1/responses callers must send text.format.
State that routed openai-chat uses response_format, while native Responses uses text.format. The current wording can cause clients to send an unsupported field to /v1/responses.
Proposed fix
- tool calls, prompt cache keys, metadata, and `response_format` on native Responses
- routes and routed `openai-chat` models (`json_object` and `json_schema` are
- forwarded as-is; a backend without structured-output support returns its own
- error). Other Chat Completions fields, including penalties, `n`, and logprobs,
+ tool calls, prompt cache keys, metadata, and structured output. Chat Completions
+ `response_format` is translated to Responses `text.format` for native Responses
+ providers and forwarded as `response_format` to routed `openai-chat` providers.
+ A backend without structured-output support returns its own error. Other Chat
+ Completions fields, including penalties, `n`, and logprobs,🤖 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/github-copilot-app.md` around lines 53 - 55, Update the native Responses
wire-field description in the surrounding documentation to state that direct
POST /v1/responses callers must use text.format, while routed openai-chat
requests use response_format. Clarify that Chat Completions input is converted
to Responses text.format rather than forwarded unchanged.
| const textFormat = parsed.options.textFormat; | ||
| if (textFormat?.type === "json_object") { | ||
| body.response_format = { type: "json_object" }; | ||
| } else if (textFormat?.type === "json_schema" && textFormat.schema !== undefined) { | ||
| body.response_format = { | ||
| type: "json_schema", | ||
| json_schema: { | ||
| name: textFormat.name ?? "response", | ||
| ...(textFormat.description !== undefined ? { description: textFormat.description } : {}), | ||
| schema: textFormat.schema, | ||
| ...(textFormat.strict !== undefined ? { strict: textFormat.strict } : {}), | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve schema-less json_schema formats.
Line 830 drops response_format when textFormat.schema is absent. src/chat/inbound.ts:175-196 accepts that Chat Completions shape and maps it to text.format, so this is not the claimed inverse mapping. The routed request then silently loses structured-output handling and can return prose instead of the upstream capability or validation error.
Remove the schema !== undefined condition. Include schema only when it exists. Add a round-trip regression test for json_schema without schema.
Proposed fix
- } else if (textFormat?.type === "json_schema" && textFormat.schema !== undefined) {
+ } else if (textFormat?.type === "json_schema") {
body.response_format = {
type: "json_schema",
json_schema: {
name: textFormat.name ?? "response",
...(textFormat.description !== undefined ? { description: textFormat.description } : {}),
- schema: textFormat.schema,
+ ...(textFormat.schema !== undefined ? { schema: textFormat.schema } : {}),
...(textFormat.strict !== undefined ? { strict: textFormat.strict } : {}),
},
};📝 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.
| const textFormat = parsed.options.textFormat; | |
| if (textFormat?.type === "json_object") { | |
| body.response_format = { type: "json_object" }; | |
| } else if (textFormat?.type === "json_schema" && textFormat.schema !== undefined) { | |
| body.response_format = { | |
| type: "json_schema", | |
| json_schema: { | |
| name: textFormat.name ?? "response", | |
| ...(textFormat.description !== undefined ? { description: textFormat.description } : {}), | |
| schema: textFormat.schema, | |
| ...(textFormat.strict !== undefined ? { strict: textFormat.strict } : {}), | |
| }, | |
| }; | |
| } | |
| const textFormat = parsed.options.textFormat; | |
| if (textFormat?.type === "json_object") { | |
| body.response_format = { type: "json_object" }; | |
| } else if (textFormat?.type === "json_schema") { | |
| body.response_format = { | |
| type: "json_schema", | |
| json_schema: { | |
| name: textFormat.name ?? "response", | |
| ...(textFormat.description !== undefined ? { description: textFormat.description } : {}), | |
| ...(textFormat.schema !== undefined ? { schema: textFormat.schema } : {}), | |
| ...(textFormat.strict !== undefined ? { strict: textFormat.strict } : {}), | |
| }, | |
| }; | |
| } |
🤖 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/adapters/openai-chat.ts` around lines 827 - 840, Update the json_schema
branch in the response-format mapping to preserve schema-less formats by
removing the schema presence requirement and conditionally adding the schema
field only when defined. Keep the existing name, description, and strict
handling, and add a round-trip regression test covering json_schema without
schema.
| const json = await res.json() as { output?: Array<{ type?: string }> }; | ||
| const compactionItems = (json.output ?? []).filter(item => item.type === "compaction"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicate const json declarations.
Each test callback declares const json twice in the same lexical scope. TypeScript rejects both files before Bun can run the tests.
tests/responses-compaction-routing.test.ts#L420-L421: keep oneconst json = await res.json()declaration.tests/server-kiro-completion-e2e.test.ts#L244-L245: keep oneconst json = await response.json()declaration.
Based on learnings, repeated const names are valid only in separate test callback scopes.
📍 Affects 2 files
tests/responses-compaction-routing.test.ts#L420-L421(this comment)tests/server-kiro-completion-e2e.test.ts#L244-L245
🤖 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/responses-compaction-routing.test.ts` around lines 420 - 421, Remove
the duplicate const json declaration in
tests/responses-compaction-routing.test.ts lines 420-421, retaining one response
JSON parse for the test callback. Also remove the duplicate const json
declaration in tests/server-kiro-completion-e2e.test.ts lines 244-245, retaining
one response JSON parse there; separate test callback scopes may keep distinct
json declarations.
Source: Learnings
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Confirmed that the branch is now rebased onto current dev, so the old freshness-only blocker is resolved. The current head is not merge-ready yet because the latest review found substantive exact-head issues:
- Preserve a
json_schemaformat even when its optionalschemamember is absent; conditionally includeschemainstead of dropping the whole format. Add the schema-less round-trip regression. - Remove the duplicate
const jsondeclarations in bothtests/responses-compaction-routing.test.tsandtests/server-kiro-completion-e2e.test.ts; these are same-scope parse/type failures, not style nits. - Make both docs distinguish the native Responses
text.formatcontract from routed Chat Completionsresponse_format, including that Chat Completions input is converted rather than forwarded as a native Responses field.
The underlying translation boundary remains valuable and correctly placed. Keeping changes requested until those findings and exact-head CI are green.
|
Thank you — this is landing as #1137 in the stacked campaign. Most of your commits were cherry-picked with your authorship intact; the one correction I made is co-authored with you. Your architecture was right and I kept it as designed: carry the parsed format on One correction. The Your #978 landed in the same PR — see the comment there. Verified: 129 pass / 0 fail across the openai-chat, parser, chat-completions and google suites; full Leaving this PR open for you. |
3 participants
Summary
text.formatwas reduced to a boolean and discarded, so structured output (Codex--output-schema) silently never reached routed models, and the chat surface returned HTTP 400 forresponse_formaton routed openai-chat models.response_format, the exact inverse of the existing inbound mapping. The 400 guard is removed, routed compaction strips the format so summaries stay prose, and the native Responses passthrough is untouched.Verification
bun run typecheck, the lint gate,bun run test, andbun run privacy:scanChecklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.