fix(responses): bind reasoning replay to route identity - #1505
Conversation
|
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 (5)
📝 WalkthroughWalkthroughReasoning replay caching now uses structured scopes bound to provider, destination, adapter, model, and credential identities. Request routing rebinds scopes after authentication and transport changes. Bridges and adapters propagate the structured scope, with tests covering isolation and failover behavior. ChangesIdentity-bound reasoning replay
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ResponsesCore
participant ProviderRoute
participant ResponsesBridge
participant ReasoningReplayCache
Client->>ResponsesCore: submit request
ResponsesCore->>ProviderRoute: resolve provider and credential
ProviderRoute-->>ResponsesCore: return route identity
ResponsesCore->>ReasoningReplayCache: bind replay scope
ResponsesCore->>ResponsesBridge: pass structured replay scope
ResponsesBridge->>ReasoningReplayCache: store or retrieve reasoning
ProviderRoute->>ResponsesCore: rotate credential or transport
ResponsesCore->>ReasoningReplayCache: rebind scope before retry
Possibly related PRs
Suggested labels: 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 |
github-actions
Bot
added
the
intake: hygiene-blocked
|
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request has been marked Ready for Review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/reasoning-replay-identity.test.ts`:
- Around line 69-82: Import bindReasoningReplayScope and add coverage in the
identity replay tests for binding a valid holder with undefined: verify
holder.current is cleared, then confirm subsequent writes do not overwrite
reasoning stored under the previous identity. Exercise the delete scope.current
behavior while preserving the existing fail-closed assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: acd6db84-973f-4ddb-ab68-1e78ac7af7a6
📒 Files selected for processing (18)
src/adapters/openai-chat.tssrc/bridge.tssrc/images/loop.tssrc/oauth/index.tssrc/responses/reasoning-replay-cache.tssrc/server/responses/core.tssrc/types.tssrc/web-search/loop.tsstructure/04_transports-and-sidecars.mdtests/bridge-raw-reasoning-hidden.test.tstests/bridge-reasoning-replay-batch.test.tstests/deepseek-reasoning-replay-gaps.test.tstests/images/loop-reasoning-replay.test.tstests/reasoning-replay-identity.test.tstests/reasoning-replay-robustness.test.tstests/reasoning-replay-scope-source.test.tstests/server-key-failover-e2e.test.tstests/web-search.test.ts
2eab82c to
ca953dd
Compare
github-actions
Bot
removed
the
intake: hygiene-blocked
ca953dd to
199b7d6
Compare
|
Maintainer review on exact head The underlying cross-route replay problem is real, and this head applies the retention boundary at the right identity level: client thread, provider name, opaque destination identity, wire adapter, final model, and physical credential identity. API keys, OAuth slot/generation material, account ids, authorization headers, and configured URL paths are represented only by process-local HMACs and are neither logged nor persisted. Incomplete identities clear the mutable holder and fail closed, while key rotation, OAuth refresh, and Codex alternate-account selection rebind before the next replay read/write. I also checked the recovery branches. API-key 429 failover and Codex alternate-account retry rebind to the replacement credential. OAuth refresh changes the generation-bound namespace. Anthropic's separate multi-account pool currently cannot provide the same stable credential-generation tuple, so it remains deliberately unscoped/fail-closed rather than reusing another account's reasoning, consistent with the documented limitation. Exact-head validation under a 2-CPU/8-GB cap:
I found no additional code blocker in the reviewed surface. I am leaving this as a draft and not approving or merging until the author completes the local-CI/ready checklist and exact-head required CI is available. |
199b7d6 to
4d725bd
Compare
|
The prior maintainer review covered a different head SHA. I will review the current head before any approval decision. 🧠 Learnings used
|
|
I will review the current head. The prior maintainer review covered a different head SHA. 🧠 Learnings used
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/responses/core.ts`:
- Around line 281-286: Update the forward branch around
reasoningReplayCredentialIdentity so pool and main-pool contexts derive Codex
identity from accountId plus writerGeneration, while direct forward contexts use
the effective forwarded credential headers. Pass the effective
chatgpt-account-id as an override, keeping all credential material inside
reasoningReplayCredentialIdentity so only its HMAC output becomes the replay
key. Add regression coverage for newer generations, differing forwarded
Authorization values with one account ID, and alternate pool accounts sharing a
ChatGPT account ID.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8564192a-d523-4b17-b85b-9c72db242785
📒 Files selected for processing (5)
src/responses/reasoning-replay-cache.tssrc/server/responses/core.tsstructure/04_transports-and-sidecars.mdtests/reasoning-replay-identity.test.tstests/reasoning-replay-scope-source.test.ts
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Review on current head 4d725bd2f4b495a31abb46e195f4950c190038b2.
There is one blocking credential-isolation bug in the new replay scope. In the forward-auth branch, pool/main-pool identity is derived from chatgptAccountId only, while direct-forward identity ignores the effective forwarded Authorization value and hashes provider.headers instead. That means a token generation change, or two physical pool credentials that share one ChatGPT account id, can keep the same replay namespace. Under a same-thread/call-id collision, raw reasoning from the previous physical credential can therefore still be reused.
Please bind pool/main-pool replay identity to the physical slot and generation, e.g. accountId plus writerGeneration, and bind direct-forward identity to the effective forwarded credential headers including Authorization and chatgpt-account-id. Keep the raw material inside the existing HMAC helper. Add regressions for generation rotation, distinct pool accounts sharing a ChatGPT account id, and distinct forwarded bearer tokens sharing an account id.
Separately, this head is now 10 commits behind current dev@70d2e175 after #1464 merged, and the exact-head Cross-platform CI / React Doctor runs are action_required with zero jobs. Rebase onto current dev and obtain an executed exact-head CI signal after fixing the credential identity.
4d725bd to
93bf9a3
Compare
|
I will review the current head. This uses the exact-head readiness-gate requirement from the repository guidance. 🧠 Learnings used
|
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Re-review on current head 93bf9a376eb607a5167f35c475f5a7e5bef2e037.
The prior P1 credential-isolation blocker is fixed. Pool/main-pool replay identity is now bound to the effective bearer plus the physical accountId, credential generation, and writerGeneration; direct-forward identity uses the effective forwarded Authorization and chatgpt-account-id. The raw credential material stays inside the process-local HMAC identity. The focused regressions cover distinct forwarded bearers under one ChatGPT account, alternate pool slots sharing one ChatGPT account, and generation/writer-generation rotation. The server key-failover E2E also verifies that a 429 rotation cannot replay the previous physical key's raw reasoning.
I also checked the rebinding points: initial route binding uses the resolved effective forward headers/auth context, pool retry binds before rebuilding the retry request, and key/OAuth rotations replace or clear the shared mutable holder before subsequent replay reads/writes. I did not find a new code-level credential-boundary blocker.
The rebase gate is now satisfied: this head is one commit ahead of and zero commits behind current dev@d667367. The remaining blocker is exact-head CI: Cross-platform CI and React Doctor are both action_required with zero jobs executed. Please obtain an executed exact-head CI signal, then request re-review.
Wibias
added
the
maintainer-sponsored
|
The prior review references a different head SHA. I will review the current head before an approval decision. 🧠 Learnings used
|
|
Thank you @luvs01 for pushing this through and for addressing the credential-isolation review feedback. This is useful because raw reasoning replay is now scoped to the exact active route and physical credential identity instead of only the client thread. Provider/model/destination changes, key rotation, OAuth generation changes, and Codex pool failover now move to a different replay namespace, which prevents reasoning from one credential or upstream target from being reused by another. The process-local HMAC identities also keep raw tokens, API keys, account-slot IDs, generations, and upstream destinations out of replay keys while still preserving same-target continuation behavior. |
3 participants
Summary
Root cause and impact
#1474 removed the process-wide fallback and required an explicit conversation scope. That prevents unrelated unscoped requests from sharing replay entries, but a single conversation can still move between providers or physical credentials during combo, pool, OAuth, image, or web-search recovery. Provider-generated call IDs are not globally unique, so a later target could reuse the same thread and call ID and receive the earlier target's raw reasoning.
This follow-up keeps replay process-local and bounded, but keys each entry to the active route and physical credential identity. Same-generation, same-target continuation remains compatible. A provider, model, destination, account/key, or OAuth token-generation change receives the existing safe placeholder or cache-miss behavior instead of another target's reasoning.
The final patch uses the existing OAuth access snapshot (
accountIdplus credentialgeneration) and does not change the OAuth authentication module's public snapshot surface.Follow-up to #1474.
Verification
tsc --noEmitpassed on both Bun versionsbun run privacy:scanpassedgit diff --checkpassedgit diff --check— passedFocused affected-surface tests, dual-runtime typecheck, privacy, diff, and independent review satisfy the local CI gate. Unrelated environment/runtime timing is recorded separately and is not attributed to this change; the same GitHub CI workload is not duplicated locally.
Checklist
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
Documentation
Tests