docs(openspec): canonical-code signal for Codex WebSocket stale-anchor recovery (#1529) - #1550
Conversation
OpenSpec change (proposal-first, no code yet) redefining the Codex-native WebSocket stale-anchor signal from the nonstandard codex_previous_response_stale classifier to the canonical previous_response_not_found code, sanitized of the raw upstream envelope and the missing response id, so unmodified Codex clients recover. Public /v1/responses keeps stream_incomplete masking. Reconciles responses-api-compat requirements for WebSocket stale-anchor recoverability and masking (1104, 1134, 774, 998). Refs: Soju06#1529 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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: dedc31dc60
ℹ️ 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".
|
|
||
| ## Why | ||
|
|
||
| The requirement `Codex WebSocket stale-anchor failures remain recoverable by a full-context retry` already intends the client to recover on a stable classifier. But standard Codex clients (including the reference pi / pi-ai transport, which retries only on `error.code == "previous_response_not_found"`, and by construction the official Codex client) recover by matching the canonical error code, not by reading a message. Emitting a proxy-specific code silently disables that built-in recovery, turning a recoverable continuity loss into a turn-ending error that needs a manual restart. A client cannot be expected to learn a proxy-specific code, so the fix belongs on the surface that deviated from the canonical contract. |
There was a problem hiding this comment.
When this contract is used by the official Codex client, the central recovery claim remains unverified: design.md explicitly calls the official-client behavior a load-bearing assumption requiring maintainer confirmation, while this paragraph asserts it follows “by construction,” and task 3.1 only checks the proxy-visible payload. If the official transport handles a synthesized response.failed differently from the pi transport, changing the code still terminates the turn, so add an official-client confirmation or client-level regression task before specifying that unmodified Codex clients recover.
AGENTS.md reference: AGENTS.md:L125-L128
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed: the official-client retry mechanism is now confirmed directly from openai/codex source (design.md's "Load-bearing assumption (confirmed from source)" section, commit 6751b54cae32b23786001e2414d749a9916201e1), not left as an unverified "by construction" claim pending your runtime confirmation.
|
|
||
| ## 3. Coverage (follows sign-off) | ||
|
|
||
| - [ ] 3.1 Migrate the ~14 codex-native WebSocket stale-anchor tests from asserting `codex_previous_response_stale` + `"previous_response_not_found" not in payload` to the new contract: `error.code == "previous_response_not_found"` with no `resp_...` id and no raw upstream envelope. |
There was a problem hiding this comment.
For stale-anchor failures received after response.created, the sanitized event still needs its current response ID for event correlation: _rewrite_websocket_continuity_corruption_event deliberately supplies _websocket_downstream_response_id(request_state), and existing coverage distinguishes current resp_prev_nf from stale resp_prev_anchor. Requiring “no resp_... id” would reject both values and can drive the implementation to strip the legitimate current ID; the regression should assert only that the missing previous_response_id is absent.
AGENTS.md reference: AGENTS.md:L129-L132
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed: the masking requirement now distinguishes the raw envelope/stale id from the current response id. _websocket_downstream_response_id continues to supply the current id on the response.failed shape (never the stale anchor), and the proposal wording was corrected to scope that id-preservation guarantee to that shape specifically — the top-level type: error shape used for connect failures carries no response id at all, sanitized or not, so there's nothing to strip there. Test coverage (_assert_previous_response_not_found_error) asserts param is None and the absence of each test's own literal stale-anchor id, not a blanket rejection of any resp_... value.
- Stop asserting the official client recovers "by construction"; state it as a load-bearing assumption to confirm (reference pi/pi-ai is confirmed), and add task 3.4 for official-client confirmation. - Fix the sanitizer test contract: assert only the stale/missing previous_response_id and raw envelope are absent; the current downstream response id is legitimately preserved for event correlation. Refs: Soju06#1529 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks, both P2s addressed in 29ff141:
|
…tale anchors Implements openspec/changes/recover-codex-ws-stale-anchor-with-canonical-code. On the Codex-native /backend-api/codex/responses WebSocket route, a stale previous_response_id anchor was surfaced to the client as a nonstandard codex_previous_response_stale classifier. No unmodified Codex client recognizes that code (confirmed from source for both the reference pi/pi-ai transport and the official codex-rs client), so the turn ended and only a manual restart recovered. Renamed PREVIOUS_RESPONSE_STALE_CODE/MESSAGE to PREVIOUS_RESPONSE_NOT_FOUND_CODE = "previous_response_not_found" / PREVIOUS_RESPONSE_NOT_FOUND_MESSAGE, sanitized of the raw upstream envelope and the missing previous_response_id, with the current downstream response id preserved for correlation. Public /v1/responses keeps stream_incomplete masking; the boundary is enforced by expose_stale_previous_response_classifier, which is reachable as True from exactly one route. Also fixes a second, independent masking layer that the original plan did not anticipate and that static analysis alone did not catch: _wrapped_websocket_error_event unconditionally re-classified any error via is_previous_response_not_found_error and force-replaced it with stream_incomplete, which silently reverted the rename for the connect-failure path only. This was invisible until the corresponding tests were run; previously the already-sanitized code was codex_previous_response_stale, which this independent check never matched, so it went unnoticed. Fixed by gating that re-masking on expose_stale_previous_response_classifier, threaded through its two relevant call sites; its other call sites (malformed JSON, validation errors, missing session, generic AppError) never carry this classification and are unaffected. Tests: renamed the shared assertion helper and added a param-absence check verified safe across all three sanitizer functions. Individually triaged all 16 raw-string-absence assertions across 14 test functions: 5 were redundant with an existing stale-id check and were deleted, 9 had no such check and were replaced with one using that test's own literal anchor id, 5 were correctly left untouched (2 public /v1 tests, 3 transparent-replay-success tests that never surface an error). Strengthened one unit assertion to guard against classifier cross-contamination on an unrelated masking path. Verified: tests/integration/test_proxy_websocket_responses.py (75 passed), tests/unit/test_proxy_utils.py (904 passed), full tests/unit (5059 passed), full tests/integration (1789 passed; 2 pre-existing unrelated failures in test_accounts_api_extended.py confirmed present on the unmodified base branch), Ruff, format, changed-file ty, proxy architecture check, and openspec validate (change + all 48 specs) all pass. Refs: Soju06#1529 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ed shape Review of the just-implemented fix found the proposal's "current downstream response id is preserved" claim was worded to cover both WS error shapes, but the top-level type:error shape has no response-id field at all, sanitized or not, so there is nothing to preserve there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…hor change Re-derived the count directly from the diff against the pre-change file while drafting the PR/issue update: 18 occurrences across 16 test functions (5 deleted, 8 replaced, 5 untouched), not the previously stated 16/14/9. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Reviewed the full impl (this grew well past the docs-only title — worth retitling to fix(proxy) before merge). Verified independently: the CI note: the red here is only "Contributors attribution" (lkraider missing from .all-contributorsrc — needs an all-contributors entry, not a code change), which fails the CI Required aggregator; the two 0-second pytest "failures" belong to cancelled run 30702119159, superseded by run 30702107883 where every shard passed. Two P3s, neither blocking:
Family note: #1558 edits the finalizer region of websocket/mixin.py (~4432-4459), disjoint from the hunks here (~1017, ~4636) — the two compose in either order (shared tests/unit/test_proxy_utils.py may need a trivial rebase for whichever lands second). #1579's spec edits sit at lines 83-127, away from the four requirements this delta modifies, so the delta headers stay valid regardless. |
2 participants
Summary
Implements the direction from #1529: on the Codex-native
/backend-api/codex/responsesWebSocket route, a staleprevious_response_idanchor now surfaces aserror.code = "previous_response_not_found"(raw upstream envelope and missing id still stripped), replacing the proxy-specificcodex_previous_response_staleno unmodified client recognizes. Public/v1/responsesis unchanged (stream_incomplete).This PR was proposal-only when opened; it now includes the implementation, tests, and validation below.
What changed
app/core/errors.py:46-47: renamedPREVIOUS_RESPONSE_STALE_CODE/_MESSAGEtoPREVIOUS_RESPONSE_NOT_FOUND_CODE = "previous_response_not_found"/PREVIOUS_RESPONSE_NOT_FOUND_MESSAGE. Branching logic in_websocket_continuity_error_fields(websocket/helpers.py:1060) is unchanged — it already gated onexpose_stale_previous_response_classifier; this is a value swap._wrapped_websocket_error_event(websocket/helpers.py:1721), used for connect-time"type": "error"frames, unconditionally re-checked the error code and force-replaced anyprevious_response_not_foundmatch withstream_incomplete— even on payloads the caller had already sanitized. Harmless under the old code (codex_previous_response_stalenever matched it); once the sanitized code became canonical, this silently reverted the fix for connect failures. Caught by two failing tests, not static review. Fixed withexpose_stale_previous_response_classifier: bool = False(default preserves prior behavior), threaded through at its 2 call sites downstream of a sanitize call (mixin.py:1020,mixin.py:4640). Traced the other 5 call sites individually:helpers.py:1715(_app_error_to_websocket_event, genericAppError, never setsparam) and 4 client-payload/schema-validation paths inmixin.py(1038, 1046, 1332, 1340) that only ever build hardcodedinvalid_request_error-family codes — none can classify asprevious_response_not_found.expose_stale_previous_response_classifierhas exactly 3 set-sites, all inwebsocket/mixin.py, all set tocodex_session_affinity, which isTruefrom exactly one handler (the Codex-native route). HTTP bridge and public/v1resolve it toFalse/default.Load-bearing assumption — confirmed from source, not pending your test
Confirmed directly from
openai/codex(codex-rs, commit6751b54cae32b23786001e2414d749a9916201e1,main), not just by analogy to pi/pi-ai:codex-rs/codex-api/src/endpoint/responses_websocket.rsclassifiesprevious_response_not_foundasApiError::Retryable.codex-rs/core/src/client.rs'sget_last_response()reads a one-shot channel populated only onOk(ResponseEvent::Completed { .. })— any failed attempt drops the sender unset, so the next retry structurally sends fullrequest.inputwithprevious_response_id: None. Not a code-specific special case.codex-rs/core/src/session/turn.rs's retry loop independently rebuilds the prompt from full local history.DEFAULT_STREAM_MAX_RETRIES = 5); the client's own fallback message:"Previous response was not found. Retrying the full request."This is a structural guarantee, not a heuristic inferred from observed behavior — treating it as sufficient without a separate runtime check.
Validation
openspec validate --strictand--specspass.tests/integration/test_proxy_websocket_responses.py(75 passed),tests/unit/test_proxy_utils.py(904 passed), full unit + integration suites pass.tyon changed files, andscripts/check_proxy_architecture.pypass."previous_response_not_found" not in ...assertions across 16 test functions, individually triaged — 5 redundant with an existing stale-id check (deleted), 8 replaced with a test-specific stale-anchor id check (6 functions, 2 of them with 2 occurrences each), 5 outside this route's scope (public/v1, or success-only paths) left untouched.Non-goals (unchanged)
upstream_unavailableand the suppressed-duplicatestream_incompleteshare the same delivery problem per the issue thread's survey, but are out of scope — follow-up.🤖 Generated with Claude Code