-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(responses,codex): stop charging a send that never happened, and let a reauthenticated account back in #4690
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+691
−62
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,27 +1,52 @@ | ||
| # wp2 — land #4683 | ||
| # wp2 (round 1) — land #4683 | ||
|
|
||
| Closed. The change is on `dev` as `2702911708` and is the last commit of the frozen 2.56.0 | ||
| candidate. | ||
|
|
||
| ## The change | ||
|
|
||
| A Codex client chained by `previous_response_id` sends only the newest turn. When local replay | ||
| state was gone, a destination on a translated wire received that delta alone under a normal 200: | ||
| the conversation was replaced by the one line the user had just typed. Only the canonical ChatGPT | ||
| forward route and stateless Responses destinations failed closed. The fix refuses with | ||
| `previous_response_not_found` for every destination that cannot see the omitted prefix, and raises | ||
| `RESPONSE_TTL_MS` from 1 hour to 24 hours so an ordinary idle gap resumes by expansion instead. | ||
| state was gone, a destination that cannot see the omitted prefix received that delta alone under a | ||
| normal 200: the conversation was replaced by the one line the user had just typed, with nothing in | ||
| the response saying so. Only the canonical ChatGPT forward route and stateless Responses | ||
| destinations failed closed. | ||
|
|
||
| The fix refuses with `previous_response_not_found` for every destination except the native | ||
| Responses passthrough, which forwards the id to a backend that stored the chain. The three wires | ||
| that look stateful do not qualify, and `src/responses/continuation-ownership.ts` records why: devin | ||
| re-sends the whole conversation each turn, cursor reads its `checkpointRef` out of the same expired | ||
| store and otherwise falls back to `full-replay`, and kiro rebuilds `conversationState.history` from | ||
| the turns it was handed. Kiro's former `invalid_request_error` is removed with them, because that | ||
| code ended the task instead of triggering the recovery Codex performs on the structured one. | ||
|
|
||
| ## Rebase note | ||
| `RESPONSE_TTL_MS` moves from 1 hour to 24 hours so an ordinary idle gap resumes by local expansion | ||
| instead of a replay round trip, and `WEBSOCKET_IDLE_TIMEOUT_SECONDS` is documented as coupled to it | ||
| with a test holding the pair together. | ||
|
|
||
| ## Two things this cycle got wrong first | ||
|
|
||
| The branch was opened against `49dcdbf535`, before #4677 split `core.ts`. The gate had moved to | ||
| `src/server/responses/request-prepare.ts`, so the branch was rebuilt on the current `dev` tip and | ||
| the gate ported there rather than rebased through a conflicting delete/split. One rebase, then CI, | ||
| then squash merge. | ||
| `src/server/responses/request-prepare.ts`, so the branch was rebuilt on the `dev` tip and the gate | ||
| ported there rather than rebased through a conflicting split. One rebase, then CI, then squash. | ||
|
|
||
| The first allowlist let kiro, cursor and devin through. A dispatched audit disputed it and was | ||
| right; all three were then verified in source to rebuild the conversation from the request they are | ||
| handed, and the exported set is now empty. | ||
|
|
||
| ## Evidence | ||
|
|
||
| - `bun test tests/codex-integration/issue-702-expired-replay-state.test.ts` — 16 pass / 0 fail on | ||
| the rebased base. The new case was driven red first: with the gate stashed, the expired | ||
| continuation returned 200 carrying the delta only. | ||
| - `bun test tests/responses/responses-core-modules.test.ts` — 9 pass, so the owner-module | ||
| inventory and line ceiling still hold after the port. | ||
| - `bun run structure:check` — passed. | ||
| - Cross-platform CI at the exact head SHA — recorded in the PR. | ||
| - Exact head `d8ef6ee9b889e51e5d3e547d60a537b8fbecfb85`. Cross-platform CI run `34935526979`: | ||
| success on Linux, Windows and macOS. Squash-merged to `dev` as `2702911708`. | ||
| - Two CI-found failures were fixed rather than worked around: the file-size ratchet caught | ||
| `tests/responses/responses-state.test.ts` growing past its cap, and the three added lines were | ||
| removed instead of raising the baseline; `tests/oauth/state-store-sweeper.test.ts` swept at | ||
| `+1h`, which no longer expires a continuation row under 24-hour retention. | ||
| - Focused local files, each passing on the final tree: | ||
| `tests/codex-integration/issue-702-expired-replay-state.test.ts` (20), | ||
| `tests/responses/responses-state.test.ts` (145), `tests/responses/ws-endpoint.test.ts` (27), | ||
| `tests/responses/responses-core-modules.test.ts` (9), | ||
| `tests/oauth/state-store-sweeper.test.ts` (19), | ||
| `tests/ci-workflows/file-size-ratchet.test.ts` (6). | ||
| - The new refusal case was driven red first: with the gate reverted, the expired continuation | ||
| returned 200 carrying the delta only. | ||
| - `bun run structure:check` — passed. No local full suite was run. |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make the corrected 59-commit delta the single release-gate scope.
The documents use conflicting scopes.
000_roadmap.mdcalls the nine commits the frozen range and makes verdicts for those nine the completion criterion.020_regression_audit.mdlater states that the nine commits are only the tail of the 59-commit delta, records nineteen slices covering every commit, and describes criterion 2 as covering1cc89cf88c..2702911708. The full-delta audit evidence exists, but the roadmap wording still allows the gate to be satisfied by a nine-commit review.Define the frozen range as
1cc89cf88c..2702911708, require a verdict for every commit in that range, and label the nine-commit table as the tail or release-focus subset. Update the Round 2 introduction to use the corrected 59-commit scope.🤖 Prompt for AI Agents