fix(streaming): repair sparse Responses snapshots - #928
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:
📝 WalkthroughWalkthroughAdds the opt-in ChangesResponses snapshot repair
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant handleResponses
participant ResponsesGateway
participant SnapshotRepair
Client->>handleResponses: Submit Responses request
handleResponses->>ResponsesGateway: Forward request
ResponsesGateway-->>handleResponses: Return sparse SSE or JSON payload
handleResponses->>SnapshotRepair: Repair when responsesSnapshotRepair is true
SnapshotRepair-->>handleResponses: Return repaired payload
handleResponses-->>Client: Relay client-facing response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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-snapshot-repair.ts`:
- Around line 226-236: Update the retainCompletedItem call in the event
reconstruction branch to measure the serialized item with
Buffer.byteLength(JSON.stringify(item), "utf8") instead of
encoder.encode(...).byteLength, while preserving the existing validation and
retention flow.
🪄 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: 229cfebe-96ef-4cbe-8823-bb7fd0acc702
📒 Files selected for processing (11)
docs-site/src/content/docs/ja/reference/configuration/providers.mddocs-site/src/content/docs/ko/reference/configuration/providers.mddocs-site/src/content/docs/reference/configuration/providers.mddocs-site/src/content/docs/ru/reference/configuration/providers.mddocs-site/src/content/docs/zh-cn/reference/configuration/providers.mdsrc/config.tssrc/server/responses-snapshot-repair.tssrc/server/responses/core.tssrc/types.tstests/config.test.tstests/responses-snapshot-repair.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/responses-snapshot-repair.ts (1)
216-235: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTaint reconstruction when a
response.output_item.doneevent is malformed.
reconstructionTaintedis set only whenevent.itemis a plain object. If a done event has noitemor has a non-objectitem, the handler skips this block and keeps previously retained items usable. A laterresponse.completedevent can then reconstructresponse.outputwithout the missing item.Handle
response.output_item.doneindependently of item validation. SetreconstructionTainted = truewhenevent.item,output_index, or the repaired item type is invalid. Add a regression test for a malformed done event followed by a terminal snapshot.Proposed fix
+ if (type === "response.output_item.done" && !isPlainObject(event.item)) { + reconstructionTainted = true; + } + if ((type === "response.output_item.added" || type === "response.output_item.done") && isPlainObject(event.item)) {🤖 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/server/responses-snapshot-repair.ts` around lines 216 - 235, Update the response event handling around repairOutputItem so response.output_item.done is validated independently of isPlainObject(event.item). For every done event, set reconstructionTainted when event.item is missing or non-object, output_index is not a nonnegative integer, or the repaired item lacks a string type; only retain the completed item when all validations pass. Add a regression test covering a malformed done event followed by response.completed and verify the terminal snapshot does not reconstruct the missing item.
🤖 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.
Outside diff comments:
In `@src/server/responses-snapshot-repair.ts`:
- Around line 216-235: Update the response event handling around
repairOutputItem so response.output_item.done is validated independently of
isPlainObject(event.item). For every done event, set reconstructionTainted when
event.item is missing or non-object, output_index is not a nonnegative integer,
or the repaired item lacks a string type; only retain the completed item when
all validations pass. Add a regression test covering a malformed done event
followed by response.completed and verify the terminal snapshot does not
reconstruct the missing item.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4d0e51f2-116d-42d2-abfc-4ed8de09f243
📒 Files selected for processing (1)
src/server/responses-snapshot-repair.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: f42074fad1
ℹ️ 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".
|
Addressed the incremental CodeRabbit data-integrity finding in fe53d58. Every malformed |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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.
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/auth-cors.ts`:
- Around line 443-445: Update the canonical openai provider key comparison in
the validation logic near the responsesSnapshotRepair type check to exclude
responsesSnapshotRepair, while retaining its boolean validation. Add
management-provider validation coverage for pool and direct modes, ensure the
openai-responses path continues applying the option, and continue rejecting
non-canonical custom authMode "forward" providers.
🪄 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: c67fae7f-6981-464c-ba34-55b1d38b826d
📒 Files selected for processing (12)
docs-site/src/content/docs/ja/reference/configuration/providers.mddocs-site/src/content/docs/ko/reference/configuration/providers.mddocs-site/src/content/docs/reference/configuration/providers.mddocs-site/src/content/docs/ru/reference/configuration/providers.mddocs-site/src/content/docs/zh-cn/reference/configuration/providers.mdsrc/config.tssrc/server/auth-cors.tssrc/server/responses-snapshot-repair.tssrc/types.tstests/config.test.tstests/management-provider-validation.test.tstests/responses-snapshot-repair.test.ts
lidge-jun
left a comment
lidge-jun
left a comment
There was a problem hiding this comment.
This is a much better shape than #894 — the scoping problem that closed that one is genuinely fixed here. Default-off behind route.provider.responsesSnapshotRepair, bounded at 256 items / 8 MiB with the translator budget charged and released, both SSE and JSON covered, and the relay design unit left alone. 825 of the 1,013 added lines are the repair module and its test.
One blocker.
Valid upstream empty output is overwritten
src/server/responses-snapshot-repair.ts:108-110 — hasAuthoritativeOutput requires a non-empty array, so an upstream that explicitly sends response.completed.response.output: [] after emitting done items gets its answer replaced with reconstructed content.
That breaks the contract this repair rests on: fill absences, never overwrite what the gateway actually sent. An empty array is a value, not an absence — a gateway that deliberately returns no output is saying something, and we would be contradicting it. tests/responses-snapshot-repair.test.ts:199-227 currently pins the wrong behavior, expecting reconstruction for [].
The fix is to reconstruct only when output is genuinely absent. If a malformed present value also needs handling, that is worth doing — but as its own documented policy with its own test, not folded into the absence path.
One thing worth adding
The integration test at tests/responses-snapshot-repair.test.ts:362-435 uses a stream containing a full response.output_item.done. Issue #893's reported fixture is more abbreviated than that — added → delta → completed. Running that exact shape produces a canonical terminal snapshot with output: [], which may well be enough for current clients, but nothing here demonstrates it.
Either add the issue's exact stream as a fixture and assert the observable consumer contract, or state in a comment why the canonical empty snapshot is sufficient. Right now the PR fixes a shape adjacent to the reported one, and the gap is not written down.
Verified on your head c08772c: typecheck passes, 172 focused tests pass across snapshot repair, config, and provider management.
Fix the empty-output case and I will take another look.
|
@lidge-jun Addressed the requested changes in
Validation on the new head:
Please take another look when convenient. |
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27a88c1634
ℹ️ 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".
|
Addressed both current GitHub Codex findings in
Current-head validation:
One transparent local-suite note: a current-head @lidge-jun the two automated follow-up findings are now handled without expanding the PR scope. |
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/responses-snapshot-repair.ts (1)
249-257: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject blank output item types before retention.
Line 249 accepts
item.type: ""and whitespace-only values. The code then clears the unfinished index and retains the invalid item. A terminal event can reconstruct this item intoresponse.output, although it has no usable canonical type.Require a non-empty trimmed type before calling
retainCompletedItem. Otherwise, taint reconstruction and emit canonicaloutput: [].Proposed fix
- if (outputIndex !== undefined && typeof item.type === "string") { + if (outputIndex !== undefined + && typeof item.type === "string" + && item.type.trim().length > 0) {🤖 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/server/responses-snapshot-repair.ts` around lines 249 - 257, Update the retention condition in the reconstruction flow around retainCompletedItem to require item.type to be a non-empty, trimmed string, not merely any string. For blank or whitespace-only types, leave the unfinished index intact, set reconstructionTainted, and ensure the existing tainted-output path emits canonical output: [].
🤖 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.
Outside diff comments:
In `@src/server/responses-snapshot-repair.ts`:
- Around line 249-257: Update the retention condition in the reconstruction flow
around retainCompletedItem to require item.type to be a non-empty, trimmed
string, not merely any string. For blank or whitespace-only types, leave the
unfinished index intact, set reconstructionTainted, and ensure the existing
tainted-output path emits canonical output: [].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 85aee37a-8a67-4e80-b954-21ca157eb2bc
📒 Files selected for processing (2)
src/server/responses-snapshot-repair.tstests/responses-snapshot-repair.test.ts
|
Addressed the CodeRabbit outside-diff blank-type finding in The retention gate now requires Regression evidence:
Latest validation remains: focused 151/151 with 619 assertions, management 4/4, typecheck pass, privacy scan pass, and |
|
@coderabbitai review |
|
@codex review |
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
3 participants
Summary
responsesSnapshotRepair;outputonly from bounded, contiguousresponse.output_item.doneindexes;outputvalues, including an empty array;pool/directmanagement writes to persist the opt-in without weakening exact-seed or reserved forward-auth validation;Why
Some OpenAI Responses-compatible gateways return successful but structurally sparse lifecycle payloads. Current Codex clients may then fail to commit or render the final assistant message even though the upstream request completed with HTTP 200.
This is the narrow follow-up requested when #894 was closed. It keeps the accepted core idea while removing the process and relay-policy overlap from that PR.
Scope and safety
The repair is provider-local and explicit opt-in. Providers that do not set
responsesSnapshotRepair: trueretain the existing passthrough behavior.The implementation only backfills absent or structurally invalid canonical fields. Valid upstream values remain authoritative. Terminal reconstruction is suppressed when indexes are malformed, gapped, or exceed the existing retained-item count/byte bounds, and retained bytes are charged to the request translator budget.
For the built-in canonical
openaiprovider, management validation excludes only this provider-local field from exact seed comparison, then applies the existing strict boolean validator. Other extra fields and every noncanonical customauthMode: "forward"provider remain rejected. This also prevents later merged PATCH validation from locking a provider row after the opt-in has been persisted.This PR does not modify
devlog/**,bun-stream-caps,relay-eager,relay.ts, or the Darwin eager-relay gate. Rewrite traffic follows the existing platform decision unchanged.Validation
bun test tests/responses-snapshot-repair.test.ts tests/config.test.ts tests/sse-payload-rewrite.test.ts tests/passthrough-abort.test.ts— 151 pass, 0 failbun test tests/bridge.test.ts tests/openai-responses-passthrough.test.ts— 115 pass, 0 failbun run typecheck— passbun run privacy:scan— passcd docs-site && bun install --frozen-lockfile && bun run build— 216 pages builtbun run teston head27a88c16— 7,603 pass, 8 skip, 7 fail; the same seven failures reproduce on a clean detacheddevworktree because this host resolves*.example.testinto the198.18.0.0/15benchmark rangexhighrequested) — PASS, no P0/P1 and no must-fix findings27a88c16— addressed in82555c0c; current-head re-review requested6261d3dc— completed, no actionable commentsCloses #893.
Supersedes #894 with the narrower scope requested by the maintainer.
Summary by CodeRabbit
New Features
responsesSnapshotRepairprovider setting.Documentation
Tests