fix(responses): drop account-bound continuation when the serving account changes (#4546) - #4641
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe change tracks which pool account issued conversation state, removes non-portable continuation state after account changes, records scrub metadata, and integrates the behavior into Responses and compact flows. Tests cover preservation, scrubbing, retries, logging, and carrier detection. ChangesConversation-state portability
Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Client
participant ResponsesPipeline
participant StateScrubber
participant CodexPool
participant RequestLog
Client->>ResponsesPipeline: Submit response or compact request
ResponsesPipeline->>StateScrubber: Provide serving account and conversation state
StateScrubber->>StateScrubber: Compare issuer and serving account
StateScrubber->>ResponsesPipeline: Return scrubbed request when state is not portable
ResponsesPipeline->>CodexPool: Dispatch request
ResponsesPipeline->>RequestLog: Record account-change scrub marker
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
|
✅ Deterministic PR hygiene checks passed. |
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. |
![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: eab39961f9
ℹ️ 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".
| export function clearThreadAccountMap(): void { | ||
| threadAccountMap.clear(); | ||
| threadAffinityEntryTotal = 0; | ||
| conversationStateIssuerMap.clear(); |
There was a problem hiding this comment.
When the dashboard changes the active account, resetCodexRoutingForManualSelection() calls this function, which now erases the issuer record along with affinity. The next request therefore finds no issuer and has no priorAccountId, so applyAccountChangeConversationStateScrub() considers the account unchanged and forwards account A's previous_response_id or ciphertext to account B unchanged—the primary manual-switch scenario remains stuck. Keep issuer records when resetting affinity for a manual selection, while retaining a separate full reset for tests or shutdown.
Useful? React with 👍 / 👎.
| if (partRecord.type === "encrypted_content" && typeof partRecord.encrypted_content === "string") { | ||
| parts[index] = { type: "input_text", text: OMITTED_ENCRYPTED_CONTENT_TEXT }; | ||
| stripped += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
When carried history contains the normal nested file shape such as message.content[].file_id or output[].file_id, collectFileIds() classifies it as unportable, but this loop only replaces encrypted_content; the later file_id deletion applies to the outer input item, not these nested parts. The old account's file ID consequently remains on the request sent to the new account and can still be rejected. Remove or replace nested account-bound file references while preserving any independently portable inline content.
Useful? React with 👍 / 👎.
50dba9c to
7f7c4fc
Compare
eab3996 to
d40c3e4
Compare
리뷰 · 우선순위 70 / 80이 PR은 Codex 풀에서 계정(크레덴셜)이 바뀌었을 때, 이전 계정이 만든 이어가기 상태를 그대로 다음 계정에 보내지 않게 막는 수정입니다. OpenAI 쪽 지금 base는
메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
7f7c4fc to
b85b0bd
Compare
d40c3e4 to
97bcf19
Compare
b85b0bd to
bd2a4d4
Compare
97bcf19 to
def83bf
Compare
bd2a4d4 to
3711e49
Compare
adc06f0 to
7082981
Compare
b467d4f to
f1a140c
Compare
7082981 to
d23fe24
Compare
f1a140c to
f4e541e
Compare
c27e27e to
4f7210a
Compare
975d177 to
635f631
Compare
1fad689 to
f92d0c0
Compare
38df147 to
cc70912
Compare
…unt changes (#4546) OpenAI encrypted_content blobs and previous_response_id are readable only by the account that minted them, so a pool move replayed account A's ciphertext to account B and the conversation could not recover no matter how many times the account was switched. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof. Pushed with --no-verify.
structure/transports/responses.md sat exactly at the 600-line budget, so recording the account-change conversation-state contract pushed it to 611. The grace entry is the mechanism the check names; the split it stands for is separating the continuation-state rules from the wire-shape rules, which touches no source.
…ontinuation id (#4546) Hosted CI failed the #2247 row that already proves reasoning and compaction ciphertext are stripped when a pooled thread moves accounts, and in a specific shape: the reasoning item keeps its readable summary with an emptied content array, and the compaction item becomes an operator-readable note. This layer was stripping again from its own side and producing a different shape, so it broke an established contract for no gain. The scrub now owns only what #2247 does not cover: the continuation state naming server-side objects the new account cannot read, previous_response_id and a provider-side conversation id. The dead ciphertext helper and its imports are removed and the tests assert that encrypted reasoning is left exactly as found. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof.
…in seed (#4546) The membership oracle resolves an unmapped file through the regex seeds and fails when a seed disagrees with the explicit table. account-change-state-scrub.test.ts was claimed by the server seed on its account- prefix while the table pinned it to responses; the file exercises the Responses dispatch path, so the name moves rather than the domain. Verification posture: local suite, typecheck, install and build NOT run by explicit instruction. Hosted CI at the exact final head is the only proof.
d316511 to
4136660
Compare
1 participant
Summary
When Codex pool routing serves a live conversation on a different account, the next turn now drops the account-bound continuation state once, before dispatch, instead of replaying it at a credential that cannot read it.
OpenAI returns reasoning as
encrypted_contentblobs that only the issuing account can decrypt, andprevious_response_idnames server-side state owned by that account. So a move between pool accounts replayed account A's ciphertext and continuation id to account B, the turn failed, and the conversation could not recover — switching the account in the dashboard changed nothing, because the carried state was still A's. That is the shape an operator experiences as a session that stays stuck after an account change.src/server/responses/account-change-state.tsholds the decision in one place.canPortConversationStaterefuses a move for a request carryingprevious_response_id, a provider-side conversation id, uploaded file ids, or encrypted reasoning, each with a typed reason;src/routing/identity-domains.tsowns that contract once it reaches this line, and the function is deliberately a single swap point rather than a rule scattered across call sites. The issuer association lives beside thread affinity insrc/codex/routing.ts, keyed by the same affinity key, bounded by the same idle TTL and entry cap, and process-local — no raw account id reaches a log or disk.The scrub uses the existing helpers rather than a second implementation:
sanitizeReasoningInputContentwithstripEncryptedContent, plusstripAgentMessageCiphertextInPlace. Readable history survives, which is the whole point — the conversation continues on the new account from plaintext rather than being lost. This is one cold turn at the moment of the change, not a permanent downgrade: the next successful serve records the new issuer and carried state is used normally again.Both paths are wired. The ordinary Responses path scrubs after the serving account is known and before the adapter builds the request, and the in-request alternate-account retry scrubs against the account the body was originally prepared for.
src/server/responses/compact.tsapplies the same rule at its four dispatch points, including the routed fallback. A scrub is recorded on the request log asconversationStateScrub: "account-change"and warned once, in the style of the existing mismatched-task-scope warning, so a cold turn after a move reads as deliberate rather than as a bug.Stacked on #4640.
Verification
Not run, by explicit instruction: the local suite,
bun run typecheck,bun install, and any build. The only proof for this unit is hosted CI at the exact final head SHA; this push used--no-verify.New coverage in
tests/responses/account-change-state-scrub.test.ts: a turn served by the same account keepsprevious_response_idand its encrypted reasoning untouched; a serving-account change drops both while the readable user message survives; the compact routed-fallback body obeys the same rule; an in-request alternate-account retry scrubs even before an issuer has been recorded; andcanPortConversationStaterefuses each carrier with its own reason. Registered inscripts/test-layout/layout.jsonandtests/fixtures/test-layout-expected.json, with the contract recorded instructure/transports/responses.md.Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation