Repository navigation
fix(responses): refuse a continuation whose task scope does not match - #5130
Conversation
A task-scope mismatch correctly refused to replay another task's state, then stripped previous_response_id and continued with whatever input the request carried. When that input was only a delta, the proxy silently turned a continuation into a fresh conversation and the model answered without the history the client believed it had. Missing or corrupt state already asks the client to replay in full; a mismatch now does the same. The refusal is the existing generic previous_response_not_found response, byte for byte, emitted before any upstream send. It is returned even when the input looks complete, because this process cannot prove that it is; a caller that really has the whole conversation retries without previous_response_id and succeeds. The response reveals neither the stored scope nor whether foreign state exists, and the internal reason stays internal. Same-scope continuation is unchanged, and two absent scopes remain the legacy unscoped cohort that still replays together. This is a deliberate change to the historical fresh-start behavior, recorded in the transport contract and in the client-facing guides.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughTask-scope mismatches now create structured ChangesTask-scoped continuation replay
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
리뷰 · 우선순위 76 / 80이전에는 task scope가 안 맞으면 바꾸는 곳은 세 갈래입니다. 의도적으로 예전 “조용한 신선 시작”을 버립니다. 입력이 완전해 보여도 거부합니다. 프록시가 그 입력이 전체 대화인지 증명할 수 없기 때문입니다. 진짜로 전체를 가진 호출자는 문서도 같이 맞춰 두었습니다. 경로 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
✅ Deterministic PR hygiene checks passed. |
![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: 2e4681f0d1
ℹ️ 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".
…answered The refusal was correct but distinguishable. A continuation this process has never seen records no replay failure and reaches the route-dependent refusals, while a mismatch answered earlier with a different message. That difference is itself the disclosure the issue asks to avoid: a caller could tell "state exists and belongs to another task" from "no state at all". A mismatch now falls through to those same route-dependent refusals, so both answers are byte-identical on any given route, and a destination that genuinely owns the continuation chain keeps behaving as it does today for an unknown id rather than being rejected for lacking local state. The regressions compare the two envelopes on the same route and assert neither reaches the model.
The existing WebSocket coverage exercises expired and missing replay state. A scope mismatch is the third way local replay becomes unusable, and the client has to see the same frame or Codex terminates the task instead of reconnecting with its full input. The case stores state under one task, reconnects as another, asserts the generic refusal frame with no upstream request, and then replays complete input without the id and succeeds.
추가 리뷰 · 우선순위 78 / 80이전 리뷰(76/80, tip 이전 리뷰가 말한 “generic 이번 푸시는 mismatch일 때 그 조기 return을 건너뜁니다. 경고 로그는 그대로(내용·scope 없음) 찍고, 테스트가 그 약속을 잠급니다. issue-702는 mismatch와 expired를 같은 라우트에서 돌린 뒤 status·responseText가 같고 upstream도 둘 다 첫 턴만 있는지 비교합니다. 기대 메시지도 라우트별 문장으로 고쳤습니다. WebSocket은 task-a 상태 위에 task-b로 재접속해 같은 경로 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
…forwarding Letting a scope mismatch fall through to the route-dependent handling was wrong. An id this process cannot resolve is the destination's business, and a destination that owns the continuation chain is entitled to receive it. An id this process CAN resolve, to another task's retained state, is not the same thing: forwarding it continues that task's conversation for a different caller. The refusal now happens before that exception, on both the ordinary and combo paths, and uses the one generic local-unavailable envelope. The paired regression asserts a resolved foreign id is refused with no upstream request while an unknown id on the identical native route is still forwarded.
추가 리뷰 · 우선순위 78 / 80이전 추가 리뷰(78/80, tip 바로 전 커밋( 이번 푸시는 그 fall-through를 접습니다. 테스트가 그 경계를 잠급니다. “mismatch와 만료가 바이트까지 같다”는 비교는 빠졌습니다. 그 자리에 쌍 회귀가 들어갔습니다. 같은 네이티브 라우트에서, 풀린 외국 id는 400이고 upstream은 0회입니다. 한 번도 본 적 없는 id는 200이고 그 id가 그대로 올라갑니다. HTTP mismatch 기대 문구와 combo e2e도 generic “Continuation state is unavailable or corrupt…”로 다시 맞췄습니다. WebSocket 거절과 전체 재전송 복구는 그대로입니다. Codex P2는 아직도 “없는 id와 외국 id를 겉으로 같게 하라”고 합니다. 이번 커밋은 그걸 고의로 버린 겁니다. 네이티브에서 400과 200은 구분이 됩니다. 대신 외국 대화를 넘기지는 않습니다. 둘 다 지킬 수는 없습니다. 경로 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
추가 리뷰 · 우선순위 77 / 80이전 추가 리뷰(78/80, tip 앞 푸시( 테스트가 그 짝을 잠급니다. 같은 native 라우트에서 외국 id는 400이고 upstream은 0입니다. 한 번도 본 적 없는 id는 200으로 전달됩니다. 앞에서 넣었던 “mismatch와 expired의 responseText가 바이트로 같다” 테스트는 지웠습니다. issue-702 HTTP mismatch와 combo e2e 기대 문구도 다시 generic corrupt 문장입니다. WebSocket으로 거부된 뒤 전체 재전송으로 복구하는 케이스는 남아 있습니다. 경로 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
A resolved foreign continuation is still refused unconditionally, before any route-dependent handling, so another task's conversation is never forwarded. What changed is the two remaining local-unavailable answers: the canonical forward and routed-continuation refusals carried their own message text, so which local failure had occurred was readable from the response even though all three mean the same thing to a client. They now use the message the early failures already use. Genuine upstream-owned continuation is untouched: an id with no local state at all still reaches a destination that owns the chain. The added case compares missing and mismatched state on one route and asserts identical status, type, code and message.
…lity check The equality case compared a mismatch against expired state and inferred the corrupt answer from the source string. It now produces each condition for real on one route - a spill deleted, a spill overwritten with bytes that do not parse, and an entry retained under a different task - and asserts byte-identical responses with no upstream request.
Unifying the local-unavailable answers changed the canonical-forward text this assertion matched on. It now matches the single message every local replay failure uses.
A second assertion matched the retired canonical-forward text through /continuation state.*expired/i, which is why unifying the message turned shard four red as well. The expected envelope now lives in one constant that every local-replay assertion in this file uses, so a future change to that contract fails one place rather than several written from memory. Status, error code, upstream-send counts and retention assertions are all unchanged; nothing was relaxed to a truthy check.
|
Maintainer integration into Reviewed head Exact-head hosted run35435034056 succeeded with all applicable producers and aggregate CI. The cancelled control run was rerun separately as 35435033238 attempt2 and succeeded. Current review threads are resolved and no maintainer objection remains. Local suites/tests/typecheck/build were NOT RUN. Static integration against dev |
Summary
previous_response_idand continued with whatever input the request carried. When that input was only a delta, the model answered without the history the client believed it had, and nothing told the client that had happened.previous_response_not_found400 already used for missing or corrupt state, byte for byte, returned before any upstream send. The client's remedy is the same one that path already documents: resend the full conversation withoutprevious_response_id.This deliberately changes the historical fresh-start behavior. That behavior was intentional when task scoping was introduced; this applies the fail-closed replay principle the rest of the continuation path already follows. The change is recorded in the Responses transport contract and in the client-facing integration guides, English and Korean.
Scope map:
src/responses/state.tsscope_mismatchreplay provenance and returns the original body; the fresh-requestWeakSetand its helper are gone.src/server/responses/request-prepare.ts,core-combo.tstests/responses/responses-state.test.ts,tests/server/server-combo-failover-e2e.test.ts,tests/codex-integration/issue-702-expired-replay-state.test.tsstructure/transports/responses.md,docs-siteguides (en, ko)Ratchet-tracked files all moved down or stayed within cap:
src/responses/state.ts1,352 → 1,345 (cap 1,371);tests/responses/responses-state.test.ts3,983 → 3,982 (cap 3,983);tests/server/server-combo-failover-e2e.test.ts4,152 → 4,165 (cap 4,166).Verification
ocxcommand, so no local result is claimed. Exact-head hosted CI on this PR is the executable evidence.expandPreviousResponseInputand of the replay-failure shape, including the WebSocket entrypoint, which reuses the same handler and reframes a non-2xx response; confirmed the refusal reuses the identicalformatErrorResponsecall already used for unavailable state, so the wire bytes are unchanged; confirmed the scoped/unscoped cohort test is absence-versus-presence after trimming, so legacy callers keep replaying; confirmed the combo path returns before the snapshot is built, which is why the oldscopeMismatchguard onproviderContinuationis no longer reachable.git diff --checkis clean; no file-size cap was raised and no test was deleted.Checklist
Summary by CodeRabbit
Bug Fixes
previous_response_not_founderror.previous_response_id.Documentation
Tests