fix(responses): stop reshaping reasoning items that carry encrypted_content - #2229
fix(responses): stop reshaping reasoning items that carry encrypted_content#2229olddonkey wants to merge 2 commits into
Conversation
…ontent Codex replays the reasoning item it received in the next request's input, and a backend that issued `encrypted_content` verifies what comes back. The content-to-summary channel rewrite deletes `content` and substitutes a synthesized `summary`, so the client stored and replayed an item the issuer had never sent, and every later turn failed with "Could not decrypt the provided encrypted_content. Ensure the value is the unmodified encrypted_content from a previous response." No route change is needed to reach this: it fires on the second turn of a fresh session. The rewrite's replay round trip was verified against DeepSeek, which is `statelessResponses` and issues no blob — its reasoning replay goes through the proxy-side cache instead. Providers that do issue a blob joined the same route later through `preserveReasoningContentModels`, a flag whose own purpose is Chat-wire prompt-cache replay, and the verified premise did not follow them. Only the stored item is exempt. The `reasoning_text` delta events carry no blob and still route to the summary channel, so the expandable trace Codex renders for the live turn is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
✅ Deterministic PR hygiene checks passed. |
|
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 (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe reasoning summary rewrite preserves items with non-empty ChangesReasoning preservation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change preserves encrypted reasoning payloads while retaining existing behavior for other reasoning flows, with targeted tests, type checking, and privacy checks passing; no actionable merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This PR stays in draft until every box above is ticked. |
리뷰 · 우선순위 74 / 80구멍은 응답 레그임. 지금 주석이 DeepSeek 전제임 ( 이 PR이 가드를 아이템에 둠. 테스트가 남은 구멍은 본문이 인정함. 모델 스위치/계정 스위치/콤보 로테이션/ #2228이랑 원인 다름. 저건 컴팩션 blob 목적지. 이건 reasoning 아이템 리셰이프. draft고 체크리스트 0/4. hygiene 통과. 지금 HEAD 해결방안: CI 그린이면 #2228이랑 같이 이 댓글은 grok-bot이 작성했습니다 |
|
Correction: the motivating claim in the description does not hold for Grok. I deployed this branch locally and probed the live route, and the result contradicts what I wrote. Grok emits summary-channel reasoning natively. A streaming turn produces
Replaying the reasoning item verbatim works today, with the blob intact, on both wire modes:
So this branch does not fix What the change is still worth. The invariant is real and independently justified: an item carrying an integrity-checked blob must not be reshaped, and the rewrite's own header records that its replay round trip was verified against DeepSeek — which is Where the Grok symptom actually points. The remaining suspect is a blob minted by a different backend and replayed to Grok — the same provenance gap #2228 closes for compaction blobs, which has no counterpart for |
|
Part of #2240 — hardening: do not reshape a reasoning item carrying opaque provider state. That issue tracks the whole 2.28.0 Grok regression; this PR is one layer of it, so it deliberately does not carry a closing keyword. The failures are sequential — each one is only reachable once the previous is fixed — so the issue should stay open until every linked PR lands. |
…vation guard The guard is sound, but its comments claimed it fixed Grok's `Could not decrypt the provided encrypted_content` failure. Live bisection disproved that: Grok emits summary-channel reasoning natively, so `reasoningItemToSummaryShape` returns early and this rewrite never fires on that route. The real cause was `"content": null` on the replayed reasoning item, fixed separately. A false causal claim in a comment is worse than none — the next reader trusts it. The rule is restated on its own terms: an item carrying opaque provider state should not have its stored shape changed unless that backend has an explicit replay contract, which is why DeepSeek was safe and why the Kimi/GLM/NeuralWatt routes now on `preserveReasoningContentModels` are the ones this actually guards. Comments and prose only; no behaviour change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Maintainer status on exact head 3d11f6f: the encrypted_content preservation guard remains a valid hardening layer, but the PR is still draft with a 0/4 readiness checklist and is 24 commits behind dev. Please synchronize with current dev, complete the exact-head CI/readiness gate, and then request final review. This should not close #2240 by itself. |
|
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. |
Summary
Codex replays the reasoning item it received in the next request's
input, and a backend that issuedencrypted_contentverifies what comes back. The content-to-summary channel rewrite deletescontentand substitutes a synthesizedsummary, so the client stores and replays an item the issuer never sent:No route change is needed to reach this — it fires on the second turn of a fresh session.
The rewrite is correct where it was designed and verified. Its own header records the premise: "Codex echoes the reasoning item it received back into the next request's input. DeepSeek's Responses API accepts summary-shaped reasoning input items (verified live), so the rewrite round-trips." DeepSeek is
statelessResponsesand issues no blob — its reasoning replay runs through the proxy-side cache instead, which is why the round trip held. Providers that do issue a blob joined the same route later throughpreserveReasoningContentModels, a flag whose own purpose is Chat-wire prompt-cache replay, and the verified premise did not follow them.registry.tscurrently lists xAI, Kimi, GLM, NeuralWatt and others there.The guard is therefore on the item, not on a provider list: any reasoning item carrying a non-empty
encrypted_contentis returned exactly as the upstream sent it. DeepSeek is unaffected by construction.Only the stored item is exempt. The
response.reasoning_text.delta/.doneevents carry no blob and still route to the summary channel, so the expandable trace Codex renders for the live turn is unchanged — this is not a rollback of #45.Scope
This is one of two root causes behind Grok breaking on the native Responses route. The sibling
Could not decode the compaction blobfailure has a different cause and is #2228; the two do not overlap in files.What this PR deliberately does not touch:
stripItemIdsWhenUnstoredstill strips the reasoning item'sid. codex-rs strips ids from every item understore: false(core/src/client.rs:918-925) and OpenAI accepts that, so stripping is the contract rather than a corruption.sanitizeReasoningInputContentstill blanks replayedcontentfor providers withoutpreserveResponsesReasoningContent. Sending raw reasoning text back upstream has token and privacy consequences, andcontent: []alongside the blob matches what codex-rs itself sends.previous_response_idexpansion) is unguarded for native blobs. Those are real gaps but need per-conversation provenance state, which is a separate design.If a live canary still reports the same error after this lands, the remaining suspects are that list — in that order.
Verification
src/fails the 3 new integrity cases (output_item.done,response.completedoutput, and the non-streaming document rewrite).bun test tests/responses-reasoning-summary-rewrite.test.ts— 19 pass, 0 fail.bun testacross the reasoning/deepseek/xai/responses/passthrough suites — 893 pass, 0 fail.bun run typecheck— clean.bun run privacy:scan— passed.bun test(no runner) hangs on this tree with high CPU and no progress, matching the observation recorded in fix(xai): restore default Grok 4.5/4.6 Responses requests #2217 — usebun run test.Checklist
structure/04_transports-and-sidecars.md, under Reasoning display parity.)🤖 Generated with Claude Code
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
Gate
This branch's change is comment/doc-only, so it was gated as part of the integrated series rather than in isolation.
bun run teston the branch that stacks all of these fixes — 13773 pass, 10 skip, 1 fail across 867 files.The single failure is
tests/key-login-live-update.test.ts> "notify after key login pushes the merged row and keeps modelCosts on live and disk". It is pre-existing and unrelated: it reproduces byte-identically on every branch in this series, including ones that never touch CLI code. Every gate in this series lands on exactly that one failure.(Plain
bun testwith no arguments hangs on this tree with high CPU and no progress — usebun run test.)