fix(responses): strip cross-backend reasoning state on a model switch - #2248
fix(responses): strip cross-backend reasoning state on a model switch#2248olddonkey wants to merge 2 commits into
Conversation
…route switch
Switching models mid-conversation broke the next turn. Reproduced end to end
through the proxy: mint a reasoning item on xai/grok-4.6, replay to
openai/gpt-5.6-sol.
replay grok -> grok : OK
replay grok -> SOL : Unknown parameter: 'input[1].status'
... status removed:
replay grok -> SOL : The encrypted content ZvQ+...fBJg could not be verified.
... status and encrypted_content removed:
replay grok -> SOL : OK
Two independent problems. Grok emits an output-only `status` on reasoning items
that OpenAI rejects on input, and a reasoning blob is decodable only by the
backend that minted it, so after a switch the client replays blobs the new
destination cannot read.
This extends the mechanism the repo already uses for opaque provider state
rather than adding a retry: `reasoning-replay-cache` already keeps a bounded,
thread-scoped store and already computes the provider/destination/adapter/model/
credential identity. It now also records which identity served a thread last, and
a request whose identity differs from that record drops `encrypted_content` from
replayed reasoning items before they go out. No record — fresh process, evicted,
expired, no client thread — keeps the blobs rather than discarding valid cached
reasoning on a guess; that leaves a switch spanning a proxy restart uncovered,
which the comment states rather than implies.
`status` is stripped only from items that are not forwarding a blob. An
OpenAI-operated backend binds the blob to the item's exact shape, so removing any
field from an item we still expect it to decode can invalidate it — the same
failure an unconditional `content` strip already produced once on this codebase.
Content blanking predates that invariant and is unchanged; an item carrying both
a native blob and raw content is a known unresolved conflict, noted in place.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The serving-identity record compared `credentialIdentity`, which for OAuth is `accountId + generation` and therefore changes on every token refresh. Six of the eight `bindRouteReasoningReplayScope` call sites are key-rotation or OAuth-refresh rebinds, so an ordinary refresh registered as "the backend changed" and the next turn on that thread dropped a valid blob. Key-pool providers would have paid that repeatedly, and silently — nothing errors, the model just loses cached reasoning. The module already distinguishes the durable dimensions for exactly this reason (lidge-jun#1926: the rotating generation deliberately does not participate). The serving record now compares `providerDestinationDurableIdentity` and `credentialDurableIdentity`, and refuses to record at all when those are missing rather than falling back to the volatile pair: a missed strip costs one degraded turn, a spurious strip is a permanent quality regression. The proxy-owned replay cache keeps its stricter key, which is deliberate. Also documents two behaviours that would otherwise read as bugs: a combo that rotates targets between turns legitimately drops blobs while the SSE model-name rewrite hides the switch from the client, and the image/web-search loops consume the replay scope without rebinding, which is what stops an internal small-model call from poisoning the record for the main conversation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change adds bounded serving-identity tracking for reasoning replay. Confirmed route changes mark requests for encrypted-content removal. Responses passthrough sanitization preserves or removes reasoning fields based on that marker, with regression tests and updated transport documentation. ChangesReasoning replay sanitization
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The replay-state change is narrowly scoped and the reported cross-backend checks succeed, but the PR is not merge-ready because the submitted test run still has one failing test and the required readiness checklist remains incomplete; merge should wait until this is cleared or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant ResponsesRequest
participant responsesCore
participant reasoningReplayCache
participant OcxParsedRequest
participant openaiResponses
ResponsesRequest->>responsesCore: bind reasoning replay scope
responsesCore->>reasoningReplayCache: updateReasoningReplayServingIdentity(scope)
reasoningReplayCache-->>responsesCore: return route change status
responsesCore->>OcxParsedRequest: set _stripReasoningEncryptedContent when changed
OcxParsedRequest->>openaiResponses: sanitize passthrough request
openaiResponses-->>ResponsesRequest: preserve or remove encrypted reasoning fields
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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. |
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This PR stays in draft until every box above is ticked. |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Reviewed exact head 4f9f7288419b3fdcbf1613dfb7a6779bac6b69a8.
The serving-identity record is committed too early. bindRouteReasoningReplayScope() calls updateReasoningReplayServingIdentity() while binding the outbound route, before that route has produced a successful terminal response. The helper both compares and writes the current identity.
That advances the thread incorrectly on a failed switch:
- Route A is the last successful serving identity.
- A request switches to route B, so A's opaque reasoning is stripped, but B fails before a successful terminal response (network/auth/upstream failure).
- The store has already been changed to B.
- The next B retry compares equal and may preserve the client's replayed A blob, causing the same foreign-state failure this PR is intended to prevent.
Please separate comparison from commit. Use the last successfully served identity to decide whether this request must strip foreign opaque state, and record the candidate identity only after a successful terminal response from that route. Add a regression covering A success -> B switch failure -> B retry, asserting that the retry still strips the A blob and that failed attempts do not advance the recorded serving identity.
Once that success-boundary is enforced, the bounded/durable identity direction remains valuable.
|
Superseded by #2254, which carries this change plus the rest of the series as a single review target. These eight PRs had to merge in a strict order, and the later four each carried the whole series as their diff (up to 27 files / +2830), so reviewing them in isolation was not actually possible. #2254 has the same 16 commits with each unit's evidence intact in its message, and the combined test gate. Nothing is dropped — the branch is unchanged and still pushed, so this can be reopened if a split is preferred after all. |
2 participants
Summary
Switching model mid-conversation breaks the next turn. Both failures are cross-backend: the client replays history minted by the previous destination, and the new one rejects it.
Reproduced against live providers through the proxy — mint a reasoning item on
xai/grok-4.6, then replay that history toopenai/gpt-5.6-sol:Two independent problems, in that order.
(a)
statuson a replayed reasoning item. Grok emitsstatus: "completed"; OpenAI rejects the field on input. It is output-only — Grok itself still accepts a replayed item without it, so removing it is safe in both directions.(b) A reasoning blob is only decodable by the backend that minted it. Same class as the compaction-blob provenance problem in #2228, with
encrypted_contenton reasoning items instead.Approach — extend the existing mechanism, not a new one
src/responses/reasoning-replay-cache.tsalready implements a bounded, thread-scoped store (64 entries / 256 KiB / 1 h TTL) and already computes the identity tuplebindRouteReasoningReplayScopeuses. This adds a small parallel record with the same discipline: remember, per client thread, the identity that served the most recent request; on the next request for that thread, if the identity differs, stripencrypted_contentfrom replayed reasoning items before the request goes out.No retry-on-4xx. Deterministic pre-flight only.
The record compares rotation-safe dimensions
The identity tuple includes the credential's OAuth generation, which changes on every token refresh — and six of the eight
bindRouteReasoningReplayScopecall sites are rotation or refresh rebinds. Comparing that tuple would make every key-pool provider look like a destination switch on refresh. The serving record therefore compares only durable dimensions (providerDestinationDurableIdentity,credentialDurableIdentity) and refuses to record when either is missing, rather than falling back to volatile ones. The proxy's own cache keeps its strict key unchanged.The failure directions are deliberately asymmetric: a missed strip costs one degraded turn; a spurious strip is a permanent quality regression.
What is deliberately not covered
No record — fresh process, evicted, expired, or no client thread id — keeps the blobs. Stripping on "unknown" would discard valid cached reasoning after every restart. This is stated in a comment rather than papered over, and it is real: a switch that spans a proxy restart still fails. Closing it needs the upstream's own error signal and is a separate change.
Interaction with other model routing, documented not changed
Prompts carrying images are unaffected: these transforms touch tool containers, reasoning items and compaction items, never message content.
Verification
Live, through the deployed proxy, with real blob-bearing items on both routes (blobs 468–3128 chars):
content: nullCounterfactual, to show the strip is what does the work rather than the upstream having become lenient — the same grok item (726-char blob,
statuspresent) replayed togpt-5.6-sol:That second row is the documented restart gap behaving exactly as designed.
Tests
bun run test— 13742 pass, 10 skip, 1 fail:tests/key-login-live-update.test.ts("notify after key login pushes the merged row and keeps modelCosts"). That failure is pre-existing and unrelated — it reproduces identically on branches that never touch CLI code, and appears on every branch in this series.Part of #2240.
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
Bug Fixes
Tests