fix: 499 completed Codex responses streams#1248
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:
📝 WalkthroughWalkthrough本 PR 为代理响应流最终化引入 Codex SSE 终止态解析与转发端增量追踪，扩展 DeferredStreamingFinalization 以携带 providerType，并调整 finalize 判定、状态映射、会话清理与熔断落库逻辑；同时补充大量针对 /v1/responses SSE 的单元测试覆盖。 ChangesCodex SSE 完成状态与客户端中止语义处理
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
miraserver
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces logic to handle client aborts gracefully for Codex streams by parsing SSE data to determine if a terminal response.completed event was received before the abort occurred. If completed, the stream is treated as successful, preventing unnecessary error reporting, session clearing, or circuit breaker triggers. Extensive unit tests have been added to cover these scenarios. The feedback suggests removing the clientAborted restriction when detecting the Codex terminal state to also handle other non-normal stream terminations (like timeouts or connection drops) after a successful completion.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![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: e9bb85df4b
ℹ️ 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".
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR correctly addresses the 499-misclassification for Codex Responses SSE streams. The terminal-state detection logic in getCodexResponsesTerminalState is sound — it iterates all parsed SSE events and tracks the last terminal state, correctly handling edge cases like response.failed after response.completed. The providerType capture in deferred streaming metadata prevents stale session state from influencing the finalization decision. Test coverage is thorough with 10 new cases covering all specified scenarios.
PR Size: L
- Lines changed: 670 (652 additions, 18 deletions)
- 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 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
No significant issues identified. Key validation points:
- Operator precedence in the
codexTerminalStateternary is correct —&&binds tighter than?:, so all five conditions must hold beforegetCodexResponsesTerminalStateis invoked. - Last-wins semantics in the switch statement ensures that a terminal failure after
response.completedis not misclassified as success (verified by the "terminal failure follows response.completed" test). shouldDetectFake200now gates onstreamCompletedForFinalizationinstead ofstreamEndedNormally, which is correct — when Codex has emittedresponse.completed, the protocol evidence supports running fake-200 detection.- Comment accuracy: All inline comments were updated to match the new semantics (e.g., "流自然结束" → "流已完成", added Codex bullet point).
parseSSEDatasafety: JSON parse errors within it are caught, and partial/incomplete SSE events gracefully degrade —getCodexResponsesTerminalStatecorrectly skips non-object data.
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Thorough (10 new tests)
- Code clarity - Good
Automated review by Claude AI
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/proxy/response-handler-abort-listener-cleanup.test.ts (1)
268-270: 💤 Low value可选：移除冗余的 mock 清理调用。
第 268 行的
vi.clearAllMocks()已经清理了所有 mock（包括testState.cancelTask和testState.cleanupTask），因此第 269-270 行的mockClear()调用是冗余的。♻️ 建议的简化
beforeEach(() => { testState.asyncTasks = []; vi.clearAllMocks(); - testState.cancelTask.mockClear(); - testState.cleanupTask.mockClear(); vi.restoreAllMocks(); });🤖 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/unit/proxy/response-handler-abort-listener-cleanup.test.ts` around lines 268 - 270, 移除冗余的 mock 清理调用：在测试中已调用 vi.clearAllMocks()（第 268 行），它已清除所有 mocks，因此删除后续对 testState.cancelTask.mockClear() 和 testState.cleanupTask.mockClear() 的显式调用以简化代码并避免重复清理。
🤖 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 `@tests/unit/proxy/response-handler-abort-listener-cleanup.test.ts`:
- Around line 268-270: 移除冗余的 mock 清理调用：在测试中已调用 vi.clearAllMocks()（第 268
行），它已清除所有 mocks，因此删除后续对 testState.cancelTask.mockClear() 和
testState.cleanupTask.mockClear() 的显式调用以简化代码并避免重复清理。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e17588bf-c17d-4bcd-85ae-a1771ce5cef8
📒 Files selected for processing (3)
src/app/v1/_lib/proxy/response-handler.tssrc/app/v1/_lib/proxy/stream-finalization.tstests/unit/proxy/response-handler-abort-listener-cleanup.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/v1/_lib/proxy/stream-finalization.ts
- src/app/v1/_lib/proxy/response-handler.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3062d6a4a
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47f513f6b4
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3cd58c056
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc0b5aa3f9
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1723378be8
ℹ️ 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 codexTerminalState = shouldInspectCodexTerminalState | ||
| ? forwardedCodexTerminalState !== "none" | ||
| ? forwardedCodexTerminalState | ||
| : getCodexResponsesTerminalState(allContent) |
There was a problem hiding this comment.
When a Codex stream aborts after the stats reader has buffered event: response.completed\ndata: ... but before the terminating blank line, the forwarded tracker correctly stays at none, but this fallback parses allContent with parseSSEData, which flushes trailing data as an event even without an SSE boundary. That makes a non-normal/client-aborted partial terminal chunk get recorded as status 200 even though the terminal SSE event was never dispatchable to the client and the upstream stream did not end normally.
Useful? React with 👍 / 👎.
Codex CLI closes the connection immediately after receiving the terminal SSE event (response.completed/incomplete). Previously CCH observed the client abort before the upstream reader finished, so successful requests were logged as 499 CLIENT_ABORTED. Root cause: the internal reader that builds allContent breaks early on clientAbort, so allContent is truncated and misses the terminal event. Fix: track Codex terminal state, usage, service_tier and prompt_cache_key incrementally in a TransformStream before tee(), so the data is captured regardless of abort timing. Changes: - response-handler.ts: add createCodexResponsesTerminalStateTracker (incremental SSE parser, last-event-wins, finalize() for trailing data without boundary newline). tracker is the primary source of truth for Codex deferred streams; allContent is the fallback. codexTerminalNonSuccess (failed/error) -> 502 + recordFailure. codexTerminalFinished (completed/incomplete) + clientAbort -> 200. providerType captured in deferred meta at set-time for failover. - stream-finalization.ts: add optional providerType to DeferredStreamingFinalization; normalize at set-time from meta or session provider. - tests: 30 cases covering full terminal-state matrix (completed/failed/incomplete/error/response.error, abort timing, provider-mismatch, multiline SSE, trailing-\n\n-less event, non-Codex unaffected, prompt_cache_key preservation). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Superseded by #1249: same fix squashed into a single clean commit. This thread accumulated bot-review noise from 8 intermediate iterations — closing to keep the review history readable. |
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
Problem
Codex CLI closes the downstream HTTP connection immediately after receiving the terminal SSE event (
response.completed). CCH observed the client abort before the internal reader finished, so successful requests were logged as499 CLIENT_ABORTEDand counted as provider failures.Root cause: The internal reader that builds
allContentbreaks early onclientAbort. With truncated content, the deferred finalization had no terminal evidence, so it fell through to the client-abort branch.Related: #1083, #1242, #985
Solution
Track Codex terminal state, usage metrics, service tier, and
prompt_cache_keyincrementally in aTransformStreambeforetee()— the data is captured regardless of when the client disconnects.The tracker is the primary source of truth for Codex deferred streams;
allContentis the fallback for non-Codex paths.Terminal state semantics
response.completed,response.incomplete200success (even if client aborted after receiving it)response.failed,response.error,error502+recordFailure(circuit breaker)499 CLIENT_ABORTED/502(original behavior)Changes
response-handler.ts—createCodexResponsesTerminalStateTracker: incremental SSE parser withfinalize()for trailing data without a boundary newline (last-event-wins semantics); tracker-first priority for usage/service_tier/prompt_cache_key in deferred finalization;providerTypeguard uses deferred meta, not current session provider (correct under failover).stream-finalization.ts— add optionalproviderTypetoDeferredStreamingFinalization; normalize at set-time from meta or session provider.\n\n-less event, non-Codex unaffected, prompt_cache_key preservation.Testing
vitest: 30/30 passtypecheck: exit 0biome check: cleanGenerated with Claude Code