feat(responses): provider opt-in sparse lifecycle snapshot repair (#893) - #1025
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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: 45eb5b834c
ℹ️ 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".
| }; | ||
|
|
||
| function jsonBlock(event: Record<string, unknown>): string { | ||
| return `data: ${JSON.stringify(event)}`; |
There was a problem hiding this comment.
When an upstream Responses stream uses named SSE events, every repaired snapshot or injected lifecycle frame produced through this helper is emitted as only data: .... The proxy’s own canonical bridge frames Responses streams with event: response.*, so EventSource-style clients that listen for response.output_text.delta, response.completed, etc. will stop receiving the repaired events and will see default message events instead whenever responsesSnapshotRepair changes a block. Please include the matching event: line for injected blocks and preserve the original event line when rewriting existing blocks.
Useful? React with 👍 / 👎.
| const itemId = typeof event.item_id === "string" ? event.item_id : undefined; | ||
|
|
||
| // --- item lifecycle tracking + field backfills ------------------------- | ||
| if (type === "response.output_item.added" && isPlainObject(event.item)) { |
There was a problem hiding this comment.
If an opt-in gateway sends response.output_item.added with a missing or non-object item, this guard skips the event instead of marking the stream tainted. The tracker then has no open/completed items, so a later response.completed without output falls through to the empty-output reconstruction path and emits output: [], hiding that upstream actually announced an unparseable item. Please fail closed for malformed output_item.added events rather than treating them as no output.
Useful? React with 👍 / 👎.
| nextEvent = { ...nextEvent, logprobs: [] }; | ||
| changed = true; | ||
| } | ||
| if (type === "response.output_text.delta" && typeof event.delta === "string" && outputIndex !== undefined) { |
There was a problem hiding this comment.
When a tracked message is open but an output_text.delta omits output_index (even with a matching item_id), this condition ignores the text instead of tainting or correlating by id. At response.completed, the repair then injects closing events and reconstructs terminal output from the still-empty open.text, so clients can commit an empty message while the original delta carried the answer. Please fail closed on uncorrelatable text deltas or use item_id correlation before synthesizing closures.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
|
|
||
| if (isCompletedTerminal && openItems.size > 0) { |
There was a problem hiding this comment.
When response.completed carries an explicit output: [] while an item is still open, this block still injects output_text.done, content_part.done, and output_item.done before the later reconstruction check decides not to modify the terminal array. That lets clients commit a message the gateway explicitly excluded from terminal output, so the explicit array is not actually authoritative. Please check the terminal snapshot before synthesizing closures and fail closed when it contradicts the open items.
Useful? React with 👍 / 👎.
| content: [{ type: "output_text", text: open.text, annotations: [] }], | ||
| }, "completed"); | ||
| } | ||
| return repairOutputItem({ ...open.item, status: "completed" }, "completed"); |
There was a problem hiding this comment.
When an opt-in gateway leaves a reasoning item open and streams only response.reasoning_summary_text.delta or response.reasoning_text.delta, the synthetic output_item.done for non-message items is built from the original output_item.added item. Those reasoning deltas are never accumulated, so terminal reconstruction emits a reasoning item with empty summary/content, losing visible reasoning and replay material. Please track reasoning deltas for the synthesized item or fail closed instead of fabricating an empty reasoning item.
Useful? React with 👍 / 👎.
45eb5b8 to
681f2e4
Compare
a8e10eb to
2ac083a
Compare
Responses-compatible gateways can omit canonical lifecycle fields and closing events, leaving Codex clients stuck on an uncommitted turn. - block-level SSE rewrite contract (zero-or-more blocks out per event) in the pull relay and the eager relay - responses-snapshot-repair: field backfills (adopted from PR #928 with attribution) + lifecycle injection — content_part.added on first text delta for an open item, and output_text.done/content_part.done/ output_item.done before terminal events, with terminal output reconstruction only when the gateway omitted it entirely - fail closed on ambiguous/gapped/malformed/oversized/contradictory shapes; explicit output: [] stays authoritative - provider config responsesSnapshotRepair (boolean), management-plane writes permitted on canonical OpenAI pool/direct seeds - JSON (non-stream) path repairs fields after raw inspection
- tee path rewrites when snapshot repair is on even with no payload rewrites (snapshot-only providers were skipped) - failed/incomplete terminals never fabricate completed output - item_id correlation for content/text events; function_call and other item types reserve their index without tainting message repair - per-item text accumulation cap (taint on overflow) and a dispose() teardown on both relays (EOF/cancel/error release) - unterminated-tail emissions stay separately framed on both relays - terminal output canonical backfill (absent/malformed -> retained or canonical empty); snapshot defaults read from the finalized outbound wire body - server-level activation: opt-in repairs end to end, opt-out unchanged
…aggregate bounds, JSON canonical output (#893) - composeSseBlockRewrites composes child dispose hooks (idempotent); pull-relay disposal is exactly-once across EOF/catch/cancel - output_item.done only closes the tracked item when id AND type match; mismatches fail closed - open-item count + aggregate text bounds; taint releases retained state immediately and stops further accumulation - repairResponsesSnapshotJson canonical output for absent/malformed - passthrough-abort gate assertion updated for the new tee condition
…tion guard, cancel-race rewrite stop (#893) - output_text.done replacement adjusts the aggregate and checks caps; closing an open item refunds its text bytes - retainCompletedItem is a no-op once tainted - pull relay never feeds the rewriter after cancel settles a pending read
#1025 review blocker 2. A text-done event carrying a different item_id for a tracked output_index was silently ignored, so the item stayed open and the completed terminal then synthesized its own output_text.done and output_item.done. That is a double close derived from a stream whose identity model we had already observed to be wrong. output_item.done has always treated an id/type mismatch as fail-closed; this applies the same contract to output_text.done — forward the block untouched, taint, and stop injecting. The prior test asserted the double-close shape as intended behavior and is replaced with the fail-closed contract.
681f2e4 to
62a5ee6
Compare
2ac083a to
1ea1b29
Compare
|
Rebased onto the current stack head, with a review blocker folded in. Blocker: the repair did not fail closed on a contradictory A text-done event carrying a different
|
1 participant
Summary
Stack 03 of the bug-stack campaign (
devlog/_plan/260805_bug_stack_campaign/040), stacked on #1023. Fixes #893.Responses-compatible gateways can return sparse lifecycle snapshots — missing canonical fields and the closing events Codex needs to commit a turn, leaving clients stuck on Thinking.
What ships:
SseBlockRewrite): one upstream event maps to zero-or-more client events — event injection is impossible in the old one-payload model. Implemented in both the pull relay and the eager relay, with an exactly-oncedispose()teardown contract (composed chains dispose every child).responsesSnapshotRepairprovider opt-in (config + management plane, canonical OpenAI pool/direct seeds):content_part.addedon the first text delta for a proven-open item, andoutput_text.done/content_part.done/output_item.donebeforeresponse.completed— so the turn actually commits;outputentirely — explicitoutput: []stays authoritative;response.failed/incompletenever fabricate completed output.Four independent review rounds folded (tee-gate skip, terminal fabrication, retention bounds/disposal, tail framing, identity correlation, wire-shape defaults, cancel race). Equivalence analysis vs #928 in the 040 doc.
Tests
tests/responses-snapshot-repair.test.ts(22): exact issue fixture → full canonical sequence + committed message, explicit-empty authority, byte-identical canonical passthrough, taint paths, identity correlation, failed/incomplete, EOF dispose, function-call streams, tail framing, canonical output.tests/responses-snapshot-repair-server.test.ts(2): opt-in repairs end-to-end through/v1/responses; opt-out relays unchanged.bun run typecheck0 errors;bun run privacy:scanpass.bun run teston Linux (ssh lidge): 8276 pass / 0 fail (baseline 8222).Provider docs
Five locale files document the opt-in.