fix(responses): normalize SSE terminal tails and policy failures (rebase of #2488) - #2581
Conversation
The review asked for a regression proving a nested cyber_policy is not hidden by a generic outer envelope. Adding it showed the detection already holds: consumeComboFailure classifies on the full display-safe text as well as the extracted code, so the nested case resolves to a non-retryable 400 today. Recording it as a test rather than changing the normalizer, so a future refactor that narrows detection to the first field-bearing candidate fails here instead of silently retrying across a safety boundary.
|
✅ Deterministic PR hygiene checks passed. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (40)
📝 WalkthroughWalkthroughChangesThe PR normalizes Responses terminal handling and Responses terminal and policy handling
Windows and CI lifecycle validation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Provider as Upstream provider
participant Relay as Responses relay
participant Log as Request log
participant Client as Client
Provider->>Relay: SSE terminal, DONE, EOF, or error
Relay->>Relay: Parse terminal and normalize policy failure
Relay->>Client: Terminal event and one DONE frame
Relay->>Log: Record terminal status and error code
Suggested reviewers: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
![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: 2e8fe53c9f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else if (!pendingDone) { | ||
| // Do not expose a sentinel before a Responses terminal. If EOF | ||
| // follows, the synthetic incomplete path owns the one sentinel; | ||
| // if a terminal arrives later, this pending frame is emitted then. | ||
| pendingDone = { block: outboundBlock, delimiter: frame.delimiter }; |
There was a problem hiding this comment.
When a Responses-compatible upstream emits data: [DONE] without a preceding Responses terminal and keeps the response body open, this branch retains the sentinel and emits nothing; both native relay loops then remain blocked on the next reader.read(), so the client, request log, and turn cleanup can hang indefinitely because the EOF synthesis path is never reached. Treat a lone DONE as an incomplete terminal or start a bounded wait before synthesizing that failure.
AGENTS.md reference: src/AGENTS.md:L15-L19
Useful? React with 👍 / 👎.
2 participants
Summary
Rebase of #2488 (by @AiriDea) onto current
dev— 19 commits, no conflicts — plus aregression for the review's nested-envelope finding.
The change normalizes Responses SSE terminal tails and policy failures. Two review
blockers were raised; both were re-checked against the rebased head:
adapterFailureFromEventoverwriting the classified policy code before testing it.Already fixed on the author's branch:
src/bridge.tsnow testsisCyberPolicyCode(error.code) || isCyberPolicyCode(event.code)and forces HTTP 400,so a conflicting-code policy failure can no longer stay 502/retryable.
cyber_policy. I wrote the regression thereview asked for, and it passes on the current code:
consumeComboFailureclassifieson the full display-safe text as well as the extracted code, so a nested policy body
already resolves to a non-retryable 400.
I did not change the normalizer for (2). I had a patch that narrowed candidate
selection to policy-bearing envelopes first, then drove it red to check it was load-bearing
— it was not, so I reverted it and kept only the test. The test still earns its place: it
fails if a future refactor narrows detection to the first field-bearing candidate, which is
exactly the retry-across-a-safety-boundary the review was guarding against.
Closes #2488.
Verification
Checklist
devdevhead (19/19, no conflicts)Summary by CodeRabbit
Bug Fixes
[DONE], malformed terminal data, and incomplete responses.Security
Documentation