[codex] strip codex responses max_output_tokens#399
Merged
Conversation
github-actions
bot
added
area: server
📝 WalkthroughWalkthroughCodex responses normalization was moved from route-layer helpers into a new transformer function Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Proxy as UpstreamEndpoint (proxy)
participant Transformer as normalizeCodexResponsesBodyForProxy
participant Upstream as Upstream API
Client->>Proxy: POST /responses (Codex-style body)
Proxy->>Transformer: normalizeCodexResponsesBodyForProxy(body, 'codex')
Transformer-->>Proxy: normalized body (roles rewritten, token fields removed, store=false)
Proxy->>Proxy: applyConfiguredPayloadRules(normalized body)
Proxy->>Transformer: normalizeCodexResponsesBodyForProxy(bodyAfterRules, 'codex')
Transformer-->>Proxy: final normalized body
Proxy->>Upstream: forward normalized request
Upstream-->>Proxy: response
Proxy-->>Client: proxied response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (3)
src/server/routes/proxy/chat.codex-oauth.test.ts (1)
213-214: Add an explicitmax_tokensassertion to fully lock the Codex strip contract.You now assert
max_output_tokensandmax_completion_tokens; addingmax_tokenshere would cover all three stripped aliases in this path.🧪 Suggested test addition
expect(forwardedBody.max_output_tokens).toBeUndefined(); + expect(forwardedBody.max_tokens).toBeUndefined(); expect(forwardedBody.max_completion_tokens).toBeUndefined();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/routes/proxy/chat.codex-oauth.test.ts` around lines 213 - 214, The test in chat.codex-oauth.test.ts currently asserts that forwardedBody.max_output_tokens and forwardedBody.max_completion_tokens are undefined but misses the max_tokens alias; update the test to also assert that forwardedBody.max_tokens is undefined (add expect(forwardedBody.max_tokens).toBeUndefined(); alongside the existing assertions for max_output_tokens and max_completion_tokens) so the Codex strip contract is fully locked for all three token aliases.src/server/routes/proxy/upstreamEndpoint.test.ts (1)
837-877: Extend Codex stripping assertions to include all token-limit aliases.These updated tests now pin
max_output_tokens, but the production strip helper also removesmax_tokensandmax_completion_tokens. Add both to prevent partial-regression gaps.🧪 Suggested coverage extension
responsesOriginalBody: { model: 'gpt-5.4', input: 'hello codex', stream: false, store: true, @@ temperature: 0.3, top_p: 0.8, + max_completion_tokens: 256, + max_tokens: 128, max_output_tokens: 512, }, @@ expect(request.body.top_p).toBe(0.8); + expect(request.body.max_completion_tokens).toBeUndefined(); + expect(request.body.max_tokens).toBeUndefined(); expect(request.body.max_output_tokens).toBeUndefined();params: { 'text.verbosity': 'low', + max_completion_tokens: 48, + max_tokens: 32, max_output_tokens: 64, store: true, }, @@ expect(request.body.safety_identifier).toBeUndefined(); + expect(request.body.max_completion_tokens).toBeUndefined(); + expect(request.body.max_tokens).toBeUndefined(); expect(request.body.max_output_tokens).toBeUndefined(); expect(request.body.store).toBe(false);Also applies to: 1005-1043
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/routes/proxy/upstreamEndpoint.test.ts` around lines 837 - 877, The test for codex token-limit stripping currently only checks max_output_tokens; update the assertions generated from buildUpstreamEndpointRequest (using the responsesOriginalBody payload) to also assert that request.body.max_tokens and request.body.max_completion_tokens are undefined (in addition to max_output_tokens) so the test mirrors the production strip helper behavior; apply the same addition to the other related test block referenced around lines 1005-1043.src/server/routes/proxy/upstreamEndpoint.ts (1)
972-991: Move Codex request-shaping into shared transformer/proxy-core logic.Lines 972-991 add more Codex protocol-conversion behavior directly in the route adapter path. Consider extracting this Codex normalization chain into shared transformation logic (single helper/module) and calling it here to keep route code thin.
As per coding guidelines
src/server/routes/**/*.ts: “Route files insrc/server/routes/**are adapters... and must not own protocol conversion...”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/routes/proxy/upstreamEndpoint.ts` around lines 972 - 991, The route is performing Codex protocol conversion inline using the chain of functions ensureCodexResponsesStoreFalse, stripCodexUnsupportedResponsesFields, ensureCodexResponsesInstructions, applyCodexResponsesCompatibility and applyConfiguredPayloadRules; extract that chain into a shared transformer (e.g., export a function normalizeCodexResponses(sanitizedResponsesBody, sitePlatform) from the proxy-core/transformer module) that encapsulates the ordering (compatibility -> instructions -> strip -> store-flag -> configured payload rules), update upstreamEndpoint.ts to import and call normalizeCodexResponses to produce body and configuredResponsesBody (preserving the applyConfiguredPayloadRules step and sitePlatform parameter), and add unit tests for the new helper and replace the inline chain with the single helper call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/server/routes/proxy/chat.codex-oauth.test.ts`:
- Around line 213-214: The test in chat.codex-oauth.test.ts currently asserts
that forwardedBody.max_output_tokens and forwardedBody.max_completion_tokens are
undefined but misses the max_tokens alias; update the test to also assert that
forwardedBody.max_tokens is undefined (add
expect(forwardedBody.max_tokens).toBeUndefined(); alongside the existing
assertions for max_output_tokens and max_completion_tokens) so the Codex strip
contract is fully locked for all three token aliases.
In `@src/server/routes/proxy/upstreamEndpoint.test.ts`:
- Around line 837-877: The test for codex token-limit stripping currently only
checks max_output_tokens; update the assertions generated from
buildUpstreamEndpointRequest (using the responsesOriginalBody payload) to also
assert that request.body.max_tokens and request.body.max_completion_tokens are
undefined (in addition to max_output_tokens) so the test mirrors the production
strip helper behavior; apply the same addition to the other related test block
referenced around lines 1005-1043.
In `@src/server/routes/proxy/upstreamEndpoint.ts`:
- Around line 972-991: The route is performing Codex protocol conversion inline
using the chain of functions ensureCodexResponsesStoreFalse,
stripCodexUnsupportedResponsesFields, ensureCodexResponsesInstructions,
applyCodexResponsesCompatibility and applyConfiguredPayloadRules; extract that
chain into a shared transformer (e.g., export a function
normalizeCodexResponses(sanitizedResponsesBody, sitePlatform) from the
proxy-core/transformer module) that encapsulates the ordering (compatibility ->
instructions -> strip -> store-flag -> configured payload rules), update
upstreamEndpoint.ts to import and call normalizeCodexResponses to produce body
and configuredResponsesBody (preserving the applyConfiguredPayloadRules step and
sitePlatform parameter), and add unit tests for the new helper and replace the
inline chain with the single helper call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2900399a-4cd8-4f26-8221-7c4c2b43cebb
📒 Files selected for processing (3)
src/server/routes/proxy/chat.codex-oauth.test.tssrc/server/routes/proxy/upstreamEndpoint.test.tssrc/server/routes/proxy/upstreamEndpoint.ts
github-actions
bot
added
size: M
Owner
Author
|
Addressed the review follow-up in 8c4802f.
Validation rerun:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/server/transformers/openai/responses/codexCompatibility.test.ts (1)
4-45: Add an explicit idempotence case.
normalizeCodexResponsesBodyForProxy()is intentionally called twice insrc/server/routes/proxy/upstreamEndpoint.ts, so anormalize(normalize(body))assertion here would lock that contract at the unit-test layer too.♻️ Suggested test
describe('normalizeCodexResponsesBodyForProxy', () => { it('normalizes codex responses bodies before proxying upstream', () => { const body = normalizeCodexResponsesBodyForProxy({ input: [ { @@ expect(body).toEqual({ input: [ { type: 'message', role: 'developer', content: [{ type: 'input_text', text: 'be precise' }], }, ], instructions: '', store: false, temperature: 0.3, }); }); + + it('is value-idempotent for codex bodies', () => { + const source = { + input: [ + { + type: 'message', + role: 'system', + content: [{ type: 'input_text', text: 'be precise' }], + }, + ], + max_output_tokens: 512, + store: true, + }; + + const once = normalizeCodexResponsesBodyForProxy(source, 'codex'); + const twice = normalizeCodexResponsesBodyForProxy(once, 'codex'); + + expect(twice).toEqual(once); + }); it('leaves non-codex bodies untouched', () => { const source = { input: 'hello', max_output_tokens: 512,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/transformers/openai/responses/codexCompatibility.test.ts` around lines 4 - 45, Add an idempotence unit test in codexCompatibility.test.ts that asserts calling normalizeCodexResponsesBodyForProxy twice yields the same result (e.g., const once = normalizeCodexResponsesBodyForProxy(source, 'codex'); const twice = normalizeCodexResponsesBodyForProxy(once, 'codex'); expect(twice).toEqual(once)); this locks the contract used by src/server/routes/proxy/upstreamEndpoint.ts where normalizeCodexResponsesBodyForProxy is invoked twice and ensures normalizeCodexResponsesBodyForProxy remains stable when re-applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/server/transformers/openai/responses/codexCompatibility.test.ts`:
- Around line 4-45: Add an idempotence unit test in codexCompatibility.test.ts
that asserts calling normalizeCodexResponsesBodyForProxy twice yields the same
result (e.g., const once = normalizeCodexResponsesBodyForProxy(source, 'codex');
const twice = normalizeCodexResponsesBodyForProxy(once, 'codex');
expect(twice).toEqual(once)); this locks the contract used by
src/server/routes/proxy/upstreamEndpoint.ts where
normalizeCodexResponsesBodyForProxy is invoked twice and ensures
normalizeCodexResponsesBodyForProxy remains stable when re-applied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95d5dc73-9cdb-468e-96b1-27bb3c15a95a
📒 Files selected for processing (6)
src/server/routes/proxy/architecture-boundaries.test.tssrc/server/routes/proxy/chat.codex-oauth.test.tssrc/server/routes/proxy/upstreamEndpoint.test.tssrc/server/routes/proxy/upstreamEndpoint.tssrc/server/transformers/openai/responses/codexCompatibility.test.tssrc/server/transformers/openai/responses/codexCompatibility.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/server/routes/proxy/chat.codex-oauth.test.ts
- src/server/routes/proxy/upstreamEndpoint.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes a Codex upstream contract mismatch in the
/v1/messagesand/v1/responsesproxy paths.When a downstream Claude-style
/v1/messagesrequest was bridged into the ChatGPT Codex OAuth upstream/responsesendpoint, Metapi preserved or synthesized amax_output_tokensfield in the outgoing Codex request body. The current Codex upstream rejects that field withUnsupported parameter: max_output_tokens, which surfaced to users as a fast 400 failure even though the downstream request itself was otherwise valid.The root cause was that Codex-specific response shaping still assumed an older native
/responsescontract. We already forcestore: falseand normalize a few Codex-only compatibility details inbuildUpstreamEndpointRequest(...), but token-limit fields were still allowed to pass through that final Codex request-shaping step. That meant the same incompatibility could leak in from OpenAI chat input, Claude/v1/messagesinput, native/v1/responsespayloads, or even configured payload rules.The fix adds a Codex-only cleanup step in the final
/responsesrequest builder somax_output_tokens,max_completion_tokens, andmax_tokensare stripped before the request is sent upstream, and again after payload rules are applied so configuration cannot reintroduce the unsupported field. The associated tests were updated to lock the current contract for all affected entry paths, including/v1/messages -> /responses, OpenAI/chat-derived Codex requests, native Codex responses requests, and payload-rule overrides.Validation was done with focused proxy tests plus the repo drift guardrail:
npm test -- src/server/routes/proxy/upstreamEndpoint.test.ts src/server/routes/proxy/chat.codex-oauth.test.ts src/server/routes/proxy/responses.codex-oauth.test.tsnpm run repo:drift-checkSummary by CodeRabbit
Bug Fixes
New Features
Tests