fix(responses): defer the serving-identity commit + passthrough findings (#2264 rebased) - #2273
Merged
Conversation
Three defects found in review of the Grok Responses series, plus one stale comment. All confirmed against the code before fixing. **One gate used the wrong predicate.** Custom-tool lowering was gated on `provider.authMode !== "forward"` while every neighbouring gate uses `!isCanonicalOpenAiForwardProvider`. A noncanonical forward provider therefore skipped `rewriteRoutedCustomToolsForUpstream` but still ran namespace lowering, so a namespace child that was a custom tool got promoted while keeping `type: "custom"` and the gateway rejected it. This repeats the mistake the same series documented elsewhere: forward auth says nothing about which backend answers, because a noncanonical forward provider never receives the caller's credentials. Both sides move together — the adapter's lowering gate and core's converted-name collection — since lowering names without restoring them is worse than not lowering at all. **The OpenAI-operated classifier missed a legitimate base-URL form.** It compared the normalized base URL for exact equality with `https://api.openai.com/v1`, so a provider configured as `baseUrl: "https://api.openai.com"` with `responsesPath: "/v1/responses"` reaches the official endpoint yet was classified as routed. That is not cosmetic: routed classification drops `content: null` from OpenAI-minted encrypted reasoning and degrades native compaction blobs — this series' own regression, in reverse. Both official forms are now accepted, still by exact normalized match so a lookalike host cannot qualify. **Request rebuilds left the namespace alias map stale.** Every recovery rebuild replaces `request` without refreshing the alias map the response path uses to restore private tool names, so a rebuild that changes the lowering decision restores against a stale map. Refreshed from the rebuilt request on every path that replaces it — the pre-existing OAuth-401 and image-413 rebuilds included, since the bug is in the rebuild pattern rather than in one caller. **`_stripReasoningEncryptedContent` is no longer only a route-switch flag.** It is also set when an upstream rejects opaque state of unknown provenance. The comment now names both producers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`updateReasoningReplayServingIdentity` compared and committed in one call, and
`bindRouteReasoningReplayScope` calls it before the request goes out — so the
candidate destination was recorded whether or not that request ever completed.
turn 1 -> A succeeds record = A
turn 2 -> B: A != B, strip A blobs record = B (committed too early)
... this request then fails (rate limit, transport, 5xx)
turn 3 -> retry B: B == B, no strip
but the transcript still carries A-minted blobs -> rejected
The opaque-blob recovery rescues turn 3, so this degraded rather than broke:
one wasted round trip and one turn of degraded reasoning on a path meant to be
deterministic. The record's meaning was the defect — it should mean "this
destination served this thread", and a request that never completed served
nothing.
Split the call in two. `reasoningReplayServingIdentityChanged` compares without
writing; `commitReasoningReplayServingIdentity` records, and runs only at a
successful terminal response. Bounded discipline is unchanged: same LRU/TTL and
byte accounting, same refusal to record without a durable identity dimension,
same fail-soft direction where no record still means keep the blobs.
For bridged transports a terminal means `completed` or `incomplete`. For
streamed passthrough it means a non-error upstream status before relay starts:
waiting for SSE completion would retain request state for the stream's
lifetime, and a later body failure does not undo that the destination accepted
and served the turn. That boundary is stated in the code rather than implied.
The two post-recovery re-records are gone — a successful recovery now reaches
the same terminal commit as any other success.
Regression test: A succeeds, an A->B turn strips and then fails, and the next B
request for the same thread still strips. Verified it fails against the old
code.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e URL isOpenAiOperatedResponsesDestination() matched on the base URL alone, so a provider with baseUrl "https://api.openai.com" and a custom responsesPath such as "/other" was classified as OpenAI-operated even though the adapter posts that request to a non-Responses endpoint. That preserved OpenAI-only null-content and reasoning semantics for a destination that never sees the official Responses API. Resolve the effective endpoint with the adapter's own construction rules — a configured responsesPath is appended verbatim, only the default branch runs the /v1/responses suffix normalization — and require an exact normalized match on https://api.openai.com/v1/responses. The conventional /v1 base and the bare official origin still classify; lookalike hosts still do not. Adds negative regressions for a custom non-Responses path on both official base forms, plus positive coverage for the bare origin default and an explicit /responses path. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zpLCh4eEms6un3VjapRgL
4 tasks
Contributor
|
✅ Deterministic PR hygiene checks passed. |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe PR tightens OpenAI Responses destination detection, enables custom-tool rewriting for noncanonical forward routes, separates replay identity comparison from commit operations, and commits identities only after successful serving across passthrough, bridge, sidecar, run-turn, and adapter paths. ChangesResponses routing and replay serving
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: ✨ Finishing Touches📝 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 |
4 tasks
![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: 1999602f02
ℹ️ 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".
| return formatErrorResponse(502, "upstream_error", undeclaredToolCallMessage(undeclared)); | ||
| } | ||
| } | ||
| commitReasoningReplayServingRoute(); |
There was a problem hiding this comment.
When a streaming client uses a provider configured for bounded JSON, this commits the new serving identity before responsesJsonToSseStream runs. If the upstream response exceeds the synthesized SSE item limit, the conversion returns a local 502 at lines 3823–3830, so the client retains its prior route's transcript even though the cache now records the new route; retrying the new route then keeps the prior route's opaque blobs instead of stripping them. Move the commit past all conversion failure paths, immediately before returning the successfully constructed response.
Useful? React with 👍 / 👎.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Lands PR #2264 by @olddonkey (the four Ingwannu review findings on the landed #2258 series), rebased cleanly onto current dev with all three commits preserved.
Verification
Checklist
Summary by CodeRabbit