Skip to content

fix(responses): stop reshaping reasoning items that carry encrypted_content - #2229

Closed
olddonkey wants to merge 2 commits into
lidge-jun:devfrom
olddonkey:fix/xai-reasoning-replay-integrity
Closed

fix(responses): stop reshaping reasoning items that carry encrypted_content#2229
olddonkey wants to merge 2 commits into
lidge-jun:devfrom
olddonkey:fix/xai-reasoning-replay-integrity

Conversation

@olddonkey

@olddonkey olddonkey commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

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 stores and replays an item the issuer never sent:

{"code":"invalid-argument","error":"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 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 statelessResponses and 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 through preserveReasoningContentModels, a flag whose own purpose is Chat-wire prompt-cache replay, and the verified premise did not follow them. registry.ts currently 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_content is returned exactly as the upstream sent it. DeepSeek is unaffected by construction.

Only the stored item is exempt. The response.reasoning_text.delta / .done events 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 blob failure has a different cause and is #2228; the two do not overlap in files.

What this PR deliberately does not touch:

  • stripItemIdsWhenUnstored still strips the reasoning item's id. codex-rs strips ids from every item under store: false (core/src/client.rs:918-925) and OpenAI accepts that, so stripping is the contract rather than a corruption.
  • sanitizeReasoningInputContent still blanks replayed content for providers without preserveResponsesReasoningContent. Sending raw reasoning text back upstream has token and privacy consequences, and content: [] alongside the blob matches what codex-rs itself sends.
  • Identity-scoped replay (model switch, account switch, combo rotation, previous_response_id expansion) 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

  • RED-first: reverting only src/ fails the 3 new integrity cases (output_item.done, response.completed output, and the non-streaming document rewrite).
  • bun test tests/responses-reasoning-summary-rewrite.test.ts — 19 pass, 0 fail.
  • bun test across the reasoning/deepseek/xai/responses/passthrough suites — 893 pass, 0 fail.
  • bun run typecheck — clean.
  • bun run privacy:scan — passed.
  • Note: plain 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 — use bun run test.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. (structure/04_transports-and-sidecars.md, under Reasoning display parity.)
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. The change only stops the proxy from editing an upstream payload; no new persistence, logging, credential handling, or destination. Privacy scan green.

🤖 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

  • Bug Fixes
    • Preserved encrypted reasoning content without modification across streamed and non-streamed responses.
    • Continued routing unencrypted reasoning text updates through the summary channel.
  • Tests
    • Added coverage verifying encrypted payloads remain unchanged in completed responses and streaming events.
    • Confirmed unencrypted reasoning deltas continue using the summary channel.
  • Documentation
    • Documented handling for encrypted reasoning content and text updates.

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 test on 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 test with no arguments hangs on this tree with high CPU and no progress — use bun run test.)

…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>
@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Aug 20, 2026
@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: fc6b3558-5650-4c5c-833d-9cd1fb15c6c5

📥 Commits

Reviewing files that changed from the base of the PR and between 3d11f6f and 10b68d2.

📒 Files selected for processing (3)
  • src/server/responses-reasoning-summary-rewrite.ts
  • structure/04_transports-and-sidecars.md
  • tests/responses-reasoning-summary-rewrite.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The reasoning summary rewrite preserves items with non-empty encrypted_content. Blob-free reasoning deltas continue to use the summary channel. Tests cover streaming and non-streaming rewrites.

Changes

Reasoning preservation

Layer / File(s) Summary
Encrypted reasoning rewrite and validation
src/server/responses-reasoning-summary-rewrite.ts, structure/04-transports-and-sidecars.md, tests/responses-reasoning-summary-rewrite.test.ts
reasoningItemToSummaryShape leaves reasoning items with non-empty encrypted_content unchanged. Blob-free reasoning_text deltas still route to the summary channel. Documentation and tests cover the behavior.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 10b68

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: ingwannu

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: preserving reasoning items that contain encrypted_content without reshaping them.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 unsupported.)
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (0/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 0/4).

Review readiness checklist

  • ⬜ 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.

0/4 boxes ticked.

This PR stays in draft until every box above is ticked.

@github-actions
github-actions Bot marked this pull request as draft August 20, 2026 22:09
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 74 / 80

구멍은 응답 레그임. 지금 dev src/server/responses-reasoning-summary-rewrite.ts reasoningItemToSummaryShape (:35-44)가 type === "reasoning"이면 content를 지우고 합성 summary를 넣음. Codex가 받은 아이템을 다음 input에 그대로 에코함. blob을 민팅한 백엔드가 unmodified 검사함. 리셰이프된 아이템이 오면 Could not decrypt the provided encrypted_content 남. 루트 바꿀 필요 없음. 새 세션 두 번째 턴에서 터짐.

주석이 DeepSeek 전제임 (:17-19). DeepSeek는 statelessResponses고 blob을 안 냄. 프록시 캐시로 리플레이함. 나중에 preserveReasoningContentModels로 blob을 내는 프로바이더가 같은 리라이트 루트에 붙었음. 지금 dev src/providers/registry.ts xAI가 grok-4.5/4.6 OAuth+inbound responses를 openai-responses로 박음 (:1030-1042) + preserveReasoningContentModels (:1062). 기본 OAuth Grok Responses가 이 리라이트를 탐. #45 트레이스 리라이트가 blob 아이템까지 삼킨 거임.

이 PR이 가드를 아이템에 둠. encrypted_content 문자열이 비어 있지 않으면 바이트 그대로 반환. 델타 response.reasoning_text.delta / .done은 blob이 없어서 요약 채널로 그대로 감. 라이브 확장 트레이스는 유지. DeepSeek는 blob이 없어서 영향 없음. stripItemIdsWhenUnstored는 그대로 id를 뗌. sanitizeReasoningInputContentpreserveResponsesReasoningContent 없는 쪽에서 리플레이 content를 비움. 방향 맞음.

테스트가 output_item.done / response.completed / non-streaming document에서 blob 아이템 byte-for-byte, 델타 리라이트는 살아 있는 걸 잠금. RED-first라고 함. src/만 되돌리면 새 3케이스가 깨짐. ㅋㅋ 한 가드가 세 경로를 같이 막음.

남은 구멍은 본문이 인정함. 모델 스위치/계정 스위치/콤보 로테이션/previous_response_id 확장은 native blob 프로비넌스가 없음. 이 PR에서 풀 설계 넣지 말 것. 카나리 후에도 같은 에러면 그 순서임.

#2228이랑 원인 다름. 저건 컴팩션 blob 목적지. 이건 reasoning 아이템 리셰이프. structure/04_transports-and-sidecars.md만 겹침. #2217/#2227이랑 modelWireDefaults.wire 같이 소유하면 안 됨. 이 PR은 그 맵 안 건드림. Chat 기본(#2227)이 돼도 Responses 옵트인에서 같은 리셰이프가 남음. 닫지 말 것. types.ts 스플릿 안 씹힘. rewrite 모듈이랑 테스트/SOT만임. #2188 사이드카, #2190 x_search랑 섞지 말 것.

draft고 체크리스트 0/4. hygiene 통과. 지금 HEAD 03735eca6에서 기본 Grok OAuth 두 번째 턴이 죽을 수 있어서 점수 높음. 2.28 블로커는 아님.

해결방안: CI 그린이면 #2228이랑 같이 dev 머지. 기본 와이어 싸움은 #2217/#2227에서. 프로비넌스 태그는 후속. 스플릿이 rewrite 모듈을 옮기면 리베이스하지 말고 닫고 다시 짜라. 지금은 그 정도 아님.

이 댓글은 grok-bot이 작성했습니다

@olddonkey

Copy link
Copy Markdown
Contributor Author

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 response.reasoning_summary_part.added / response.reasoning_summary_text.delta / .done — no reasoning_text events at all — and the completed item carries encrypted_content plus summary with no content:

turn1 event types: {'response.reasoning_summary_part.added': 1,
                    'response.reasoning_summary_text.delta': 19,
                    'response.reasoning_summary_text.done': 1,
                    'response.reasoning_summary_part.done': 1}
reasoning item keys: ['encrypted_content', 'id', 'status', 'summary', 'type']  blob len: 452

reasoningItemToSummaryShape returns early when there is no reasoning_text content (if (text.length === 0) return item;), so on this route the rewrite this PR guards never fired in the first place. It is listed in preserveReasoningContentModels, which arms routeUsesContentChannelReasoning, but the rewrite is inert because the shape it converts never arrives.

Replaying the reasoning item verbatim works today, with the blob intact, on both wire modes:

  • non-streaming, same model — completed
  • streaming, same model — completed
  • grok-4.5-minted blob replayed to grok-4.6completed

So this branch does not fix Could not decrypt the provided encrypted_content, and cross-model switching inside the Grok family is not the trigger either. I should not have asserted a cause I had only traced statically.

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 statelessResponses and issues no blob. preserveReasoningContentModels now also lists Kimi, GLM and NeuralWatt routes; any of them that emits content-channel reasoning and a blob would hit exactly the failure this guards, and nothing else in the chain would catch it. I would keep it as hardening, retitled honestly, rather than as a fix for the Grok symptom.

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 encrypted_content. The reporter's history was heavy openai/gpt-5.6-sol traffic before switching to Grok, which fits. I could not mint an OpenAI blob through the forward route to prove it end to end, so that stays a hypothesis rather than a diagnosis.

@olddonkey

Copy link
Copy Markdown
Contributor Author

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>
@Ingwannu

Copy link
Copy Markdown
Owner

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.

@olddonkey

Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants