fix(proxy): normalize nullable Responses output fields#1220
Conversation
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
📝 WalkthroughWalkthrough在代理非流式 /v1/responses 路径新增响应输出归一化模块并在 ResponseFixer 非流式分支集成，修复包含 null 的字段为 SDK 兼容的空数组/空字符串/字符串化参数，同时添加单元测试与一处 dispatch 注释说明 bypass 行为。 变更详情Responses API 输出字段归一化
审查总览本次变更为 审查工作量评估🎯 3 (中等复杂度) | ⏱️ ~25 分钟 可能相关的 PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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: 1993f4d52c
ℹ️ 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".
| const normalizedResponse = session.getEndpointPolicy().bypassResponseRectifier | ||
| ? fixedResponse | ||
| : await normalizeResponseOutput(session, fixedResponse); |
There was a problem hiding this comment.
When the global enableResponseFixer setting is false, ResponseFixer.process() returns the original response, but this newly added call still rewrites successful /v1/responses JSON because it is gated only by bypassResponseRectifier. In deployments that disable the response fixer for compatibility or debugging, nullable fields will still be mutated before the client receives them; gate normalizeResponseOutput on the same cached fixer setting or run it from within ResponseFixer.process().
Useful? React with 👍 / 👎.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| const result = normalizeResponseOutputPayload(parsed); | ||
| if (!result.applied) return response; |
There was a problem hiding this comment.
[Medium] [TEST-MISSING-CRITICAL] Missing test for applied: false optimization path
Why this is a problem: This early-return path ensures compliant Responses API payloads bypass unnecessary JSON.stringify + new Response() overhead. Without a test, a future refactor could accidentally remove this optimization, impacting every non-streaming Responses API call.
Suggested fix: Add a test in tests/unit/proxy/response-output-normalizer.test.ts:
it("returns the original response when no normalization is needed", async () => {
const response = new Response(
JSON.stringify({
id: "resp_test",
object: "response",
status: "completed",
output: [
{ id: "msg_1", type: "message", role: "assistant", status: "completed", content: [] },
],
}),
{ status: 200, headers: { "content-type": "application/json" } }
);
const normalized = await normalizeResponseOutput(createResponseSession(), response);
expect(normalized).toBe(response);
});
There was a problem hiding this comment.
Code Review Summary
The PR correctly normalizes nullable fields in OpenAI Responses API non-streaming payloads. The core logic is sound and the main happy paths are well-tested. One edge-case test gap in normalizeResponseOutput prevents the new module from fully meeting the project's 80% coverage threshold for defensive early-return paths.
PR Size: M
- Lines changed: 353
- Files changed: 3
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 1 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Medium Priority Issues (Should Fix)
[TEST-MISSING-CRITICAL] Missing test for applied: false optimization path
src/app/v1/_lib/proxy/response-output-normalizer.ts:174 -- normalizeResponseOutput returns the original response when normalizeResponseOutputPayload finds no fixes. Without a test, this early-return optimization could be accidentally removed, causing all compliant Responses API payloads to be unnecessarily re-stringified and re-wrapped in a new Response object.
Additionally, the following defensive early-return branches in normalizeResponseOutput are also untested and worth covering:
- Non-2xx status codes (
response.status < 200 || response.status >= 300) - Non-JSON content types (
\!isJsonContentType(contentType)) - JSON parse failure (
JSON.parsethrows)
Suggested tests:
it("returns the original response when no normalization is needed", async () => {
const response = new Response(
JSON.stringify({
id: "resp_test",
object: "response",
status: "completed",
output: [
{ id: "msg_1", type: "message", role: "assistant", status: "completed", content: [] },
],
}),
{ status: 200, headers: { "content-type": "application/json" } }
);
const normalized = await normalizeResponseOutput(createResponseSession(), response);
expect(normalized).toBe(response);
});
it("skips normalization for non-2xx responses", async () => {
const response = new Response('{"object":"response"}', { status: 500, headers: { "content-type": "application/json" } });
const normalized = await normalizeResponseOutput(createResponseSession(), response);
expect(normalized).toBe(response);
});
it("skips normalization for non-JSON content types", async () => {
const response = new Response("text", { status: 200, headers: { "content-type": "text/plain" } });
const normalized = await normalizeResponseOutput(createResponseSession(), response);
expect(normalized).toBe(response);
});
it("returns the original response when JSON parsing fails", async () => {
const response = new Response("not json", { status: 200, headers: { "content-type": "application/json" } });
const normalized = await normalizeResponseOutput(createResponseSession(), response);
expect(normalized).toBe(response);
});Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean (silent JSON.parse falls follow existing codebase pattern)
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate for critical paths, edge case tests recommended
- Code clarity - Good
Automated review by Claude AI
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function cleanResponseHeaders(headers: Headers): Headers { | ||
| const cleaned = new Headers(headers); | ||
| cleaned.delete("transfer-encoding"); | ||
| cleaned.delete("content-length"); | ||
| return cleaned; | ||
| } |
There was a problem hiding this comment.
cleanResponseHeaders only removes transfer-encoding and content-length. If any middleware in the proxy chain preserves a content-encoding: gzip (or br/deflate) header on a body that was already decompressed by the HTTP layer, the client would try to decompress plain JSON and fail. Adding a content-encoding delete here makes the header set consistent with the actual body.
| function cleanResponseHeaders(headers: Headers): Headers { | |
| const cleaned = new Headers(headers); | |
| cleaned.delete("transfer-encoding"); | |
| cleaned.delete("content-length"); | |
| return cleaned; | |
| } | |
| function cleanResponseHeaders(headers: Headers): Headers { | |
| const cleaned = new Headers(headers); | |
| cleaned.delete("transfer-encoding"); | |
| cleaned.delete("content-length"); | |
| cleaned.delete("content-encoding"); | |
| return cleaned; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-output-normalizer.ts
Line: 20-25
Comment:
The re-serialized response body is always plain UTF-8 JSON, but `cleanResponseHeaders` only removes `transfer-encoding` and `content-length`. If any middleware in the proxy chain preserves a `content-encoding: gzip` (or `br`/`deflate`) header on a body that was already decompressed by the HTTP layer, the client would try to decompress plain JSON and fail. Adding a `content-encoding` delete here makes the header set consistent with the actual body.
```suggestion
function cleanResponseHeaders(headers: Headers): Headers {
const cleaned = new Headers(headers);
cleaned.delete("transfer-encoding");
cleaned.delete("content-length");
cleaned.delete("content-encoding");
return cleaned;
}
```
How can I resolve this? If you propose a fix, please make it concise.| if (!isSSE) { | ||
| return await ProxyResponseHandler.handleNonStream(session, fixedResponse); | ||
| const normalizedResponse = session.getEndpointPolicy().bypassResponseRectifier | ||
| ? fixedResponse | ||
| : await normalizeResponseOutput(session, fixedResponse); | ||
| return await ProxyResponseHandler.handleNonStream(session, normalizedResponse); | ||
| } |
There was a problem hiding this comment.
bypassResponseRectifier now silently controls two independent transforms — the response fixer (a few lines above) and this new output normalizer. A future developer reading only this block may not realise the flag has dual responsibility and either sets it incorrectly or strips the normalizer call when refactoring rectifier logic. A short inline comment captures the intent.
| if (!isSSE) { | |
| return await ProxyResponseHandler.handleNonStream(session, fixedResponse); | |
| const normalizedResponse = session.getEndpointPolicy().bypassResponseRectifier | |
| ? fixedResponse | |
| : await normalizeResponseOutput(session, fixedResponse); | |
| return await ProxyResponseHandler.handleNonStream(session, normalizedResponse); | |
| } | |
| if (!isSSE) { | |
| // bypassResponseRectifier also gates output normalization so that | |
| // raw-passthrough endpoints (e.g. /responses/compact) are untouched. | |
| const normalizedResponse = session.getEndpointPolicy().bypassResponseRectifier | |
| ? fixedResponse | |
| : await normalizeResponseOutput(session, fixedResponse); | |
| return await ProxyResponseHandler.handleNonStream(session, normalizedResponse); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/response-handler.ts
Line: 899-904
Comment:
`bypassResponseRectifier` now silently controls two independent transforms — the response fixer (a few lines above) and this new output normalizer. A future developer reading only this block may not realise the flag has dual responsibility and either sets it incorrectly or strips the normalizer call when refactoring rectifier logic. A short inline comment captures the intent.
```suggestion
if (!isSSE) {
// bypassResponseRectifier also gates output normalization so that
// raw-passthrough endpoints (e.g. /responses/compact) are untouched.
const normalizedResponse = session.getEndpointPolicy().bypassResponseRectifier
? fixedResponse
: await normalizeResponseOutput(session, fixedResponse);
return await ProxyResponseHandler.handleNonStream(session, normalizedResponse);
}
```
How can I resolve this? If you propose a fix, please make it concise.![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/app/v1/_lib/proxy/response-output-normalizer.ts`:
- Around line 16-18: The isJsonContentType function currently does
case-sensitive checks; normalize the input first (e.g., const ct = (contentType
|| "").toLowerCase()) and then perform includes("application/json") ||
includes("+json") to correctly detect JSON regardless of header casing; update
the isJsonContentType function to use this normalized ct variable so calls like
isJsonContentType("Application/JSON") return true.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b2124c7-cdcb-4358-98a9-d622e13b3469
📒 Files selected for processing (3)
src/app/v1/_lib/proxy/response-handler.tssrc/app/v1/_lib/proxy/response-output-normalizer.tstests/unit/proxy/response-output-normalizer.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/v1/_lib/proxy/response-fixer/index.ts (1)
7-7: ⚡ Quick win请改为使用
@/路径别名导入。这里新增的是相对路径导入，建议改为
@/别名路径以符合仓库约定。As per coding guidelines
**/*.{ts,tsx,js,jsx}: Use path alias@/to map to ./src/ for imports.🤖 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/app/v1/_lib/proxy/response-fixer/index.ts` at line 7, 当前在 src/app/v1/_lib/proxy/response-fixer/index.ts 中用相对路径导入 normalizeResponseOutput；请将 import { normalizeResponseOutput } from "../response-output-normalizer" 替换为使用仓库约定的别名导入，如 import { normalizeResponseOutput } from "`@/app/v1/_lib/proxy/response-output-normalizer`"（保持符号名 normalizeResponseOutput 不变）。
🤖 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/app/v1/_lib/proxy/response-fixer/index.ts`:
- Line 7: 当前在 src/app/v1/_lib/proxy/response-fixer/index.ts 中用相对路径导入
normalizeResponseOutput；请将 import { normalizeResponseOutput } from
"../response-output-normalizer" 替换为使用仓库约定的别名导入，如 import {
normalizeResponseOutput } from
"`@/app/v1/_lib/proxy/response-output-normalizer`"（保持符号名 normalizeResponseOutput
不变）。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e611435-a95c-4ebe-b015-91174b8128e4
📒 Files selected for processing (5)
src/app/v1/_lib/proxy/response-fixer/index.tssrc/app/v1/_lib/proxy/response-fixer/response-fixer.test.tssrc/app/v1/_lib/proxy/response-handler.tssrc/app/v1/_lib/proxy/response-output-normalizer.tstests/unit/proxy/response-output-normalizer.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/app/v1/_lib/proxy/response-handler.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/v1/_lib/proxy/response-output-normalizer.ts
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
* chore: 将 Codex 测试模型升级到 GPT-5.5 (#1221) Co-authored-by: tesgth032 <tesgth032@users.noreply.github.com> * feat(anthropic): detect actual model from thinking signature in stream (#1223) * feat(anthropic): detect model from thinking signature in stream Use the thinking signature protobuf embedded in Anthropic stream content_block_delta events to extract the actual model name. This is more accurate than the message_start plain text when thinking is on. Three-state detection: - signature found → use model from signature - no signature with thinking on → fall back to plain model, warn via UI - no signature without thinking → plain model (normal) Add a UI badge and i18n labels for five locales when thinking was enabled but no signature arrived. * fix(proxy): drop claude- prefix gate and validate thinking signature model Remove the requestedModel prefix check so the detection works with model-redirect, Bedrock, and Vertex providers. Validate signature-decoded model names for length (≤128 chars) and presence of "claude" to reject spoofed payloads. Also fix several related issues: - Accept both standard and URL-safe base64 alphabets, and correct the length validation to only reject remainder 1. - Show the no-signature badge in the UI even when no model field is present. - Return fallback_no_thinking for empty streams to avoid false no-signature alerts. - Record the truthful thinkingEnabled value in audit logs instead of a source-derived heuristic. - Fix SSE chunk joining in tests to ensure proper event boundary blank lines. - Correct full-width punctuation in zh-CN and zh-TW tooltip strings. * fix(proxy): strip base64 padding and use @/ imports - Strip trailing padding from base64 input before checking invalid length, so that padded strings like "xxxxx=" are correctly rejected. - Convert relative imports to @/ path aliases for consistency. - Update test fixture to encode protobuf length fields with varint. * fix(proxy): skip thinking-signature detection for non-Anthropic models Restores a guard on the requested model family (claude- or anthropic/ prefix) so that providers using the Anthropic-compatible API path but serving non-Anthropic models (e.g. GLM) are not incorrectly classified. Without this gate, those providers could produce a fallback_no_signature_with_thinking source even though they never emit thinking signatures. Drops the requirement that the signature-decoded model name contain "claude", leaving only the length check (<= 128 chars). This allows future Anthropic model families that may drop the "claude-" prefix. Fixes import order in response-handler.ts after lint:fix. * fix(proxy): normalize nullable Responses output fields (#1220) * fix(proxy): 规范化 Responses 输出空值 * fix(proxy): 完善 Responses 输出归一化边界 * fix: return full self user list shape (#1213) * fix: return full self user list shape * fix: target self user display loading * chore: remove unreachable self user guard * 优化公开状态页 Redis 聚合与轮询性能 (#1211) * 优化公开状态页 Redis 聚合与轮询性能 * 修复公开状态页 rollup 边界问题 * 完善公开状态页 rollup 可靠性 * 完善公开状态页轮询测试清理 * 完善公开状态页轮询测试清理 --------- Co-authored-by: tesgth032 <tesgth032@users.noreply.github.com> --------- Co-authored-by: tesgth032 <tesgth032@hotmail.com> Co-authored-by: tesgth032 <tesgth032@users.noreply.github.com> Co-authored-by: puyujian <46592377+puyujian@users.noreply.github.com> Co-authored-by: Clouder <Clouder0@outlook.com>
2 participants
Summary
Normalize nullable fields in non-streaming OpenAI Responses payloads before returning them to clients, ensuring compatibility with the official
openai-pythonSDK parser.Fixes #1219
Problem
When using CCH as an OpenAI-compatible proxy for
/v1/responses, the raw HTTP response can be successful (HTTP 200,status: completed), but clients using the officialopenai-pythonResponses API parser fail with:This occurs because some upstream providers return
nullfor fields where the OpenAI Responses schema expects arrays, strings, or objects. The proxy passes thesenullvalues through unchanged, causing the SDK parser to crash before the caller can readresponse.output_text.Solution
Add a response output normalizer (
response-output-normalizer.ts) that runs on non-SSE responses after the existingResponseFixerpipeline, before the response is returned to the client.Normalization rules applied:
outputnull[]message.contentnull[]text(output_text)null""annotationsnull[]logprobsnull[]function_call.argumentsnullor object"{}"or JSON stringtoolsnull[]summary(reasoning)null[]Design decisions:
originalFormat === "response"only (OpenAI Responses API format)bypassResponseRectifieris enabled on the endpoint policyusage: null(no normalization needed)transfer-encodingandcontent-lengthheaders after re-serializationThis is the output-side counterpart to the existing request-input rectifier added in #888.
Changes
src/app/v1/_lib/proxy/response-output-normalizer.tssrc/app/v1/_lib/proxy/response-handler.tshandleNonStream()tests/unit/proxy/response-output-normalizer.test.tsTesting
normalizeResponseOutputPayload(3 tests) +normalizeResponseOutput(2 tests)bun run typecheck)bun run lint)bun run build)openai==2.24.0local parser smoke testRelated
Description enhanced by Claude AI
Greptile Summary
This PR adds a
ResponseOutputNormalizerthat convertsnullResponses API fields (output,content,text,annotations,logprobs,tools,summary,function.arguments) to their SDK-expected types (empty array, empty string, or serialized JSON), fixingTypeError: 'NoneType' object is not iterablecrashes in theopenai-pythonparser when upstream providers return nulls.response-output-normalizer.ts: clones and parses the fixed response body, applies targeted null→default coercions, re-serializes, and stripscontent-encoding/content-length/transfer-encodingheaders; gracefully falls back to the original response on parse failure.ResponseFixer.processNonStream: normalizer runs as the final step after the encoding and JSON fixers, scoped tooriginalFormat === \"response\"non-SSE 2xx responses.Confidence Score: 5/5
Safe to merge; the normalizer is additive, scoped to Responses-format non-streaming responses, and falls back to the original response on any error.
The logic is correct for all cases reviewed: null-to-default coercions, format/status/content-type guards, graceful fallback on JSON parse failure, and correct header cleanup. The one design note — that the normalizer is also skipped when enableResponseFixer is false — is reflected in an explicit test and is an intentional architectural choice, not a defect on the changed path.
No files require special attention, though the coupling between enableResponseFixer and the normalizer in response-fixer/index.ts is worth a second look if the normalizer is ever meant to operate independently.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Client participant Handler as ProxyResponseHandler participant Fixer as ResponseFixer participant Normalizer as ResponseOutputNormalizer Client->>Handler: "non-streaming request (originalFormat=response)" Handler->>Handler: bypassResponseRectifier? alt "bypass = true" Handler-->>Client: raw upstream response (null fields preserved) else "bypass = false" Handler->>Fixer: process(session, response) Fixer->>Fixer: enableResponseFixer? alt fixer disabled Fixer-->>Handler: raw response (normalizer also skipped) else fixer enabled Fixer->>Fixer: EncodingFixer + JsonFixer Fixer->>Normalizer: normalizeResponseOutput(session, fixedResponse) Normalizer->>Normalizer: "originalFormat === response?" Normalizer->>Normalizer: 2xx + JSON content-type? Normalizer->>Normalizer: clone + parse body Normalizer->>Normalizer: normalizeResponseOutputPayload() alt null fields found Normalizer-->>Fixer: "new Response (nulls to array/string/{})" else no fixes needed Normalizer-->>Fixer: original response (identity) end Fixer-->>Handler: normalizedResponse end Handler-->>Client: normalized response endPrompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix(proxy): 完善 Responses 输出归一化边界" | Re-trigger Greptile