fix: move anthropic raw request body check to core - #4430
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughMoves Anthropic raw-body passthrough flag clearing from the model catalog resolver plugin into a new unexported core helper ChangesAnthropic passthrough flag clearing refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Merge activity
|
Confidence Score: 3/5The routing change addresses the intended alias/governance path but still needs fixes around fallback state and the moved lifecycle expectations before it is safe to merge. The code paths are localized and covered by targeted tests, but the shared context mutation can affect fallback attempts and an existing integration expectation no longer matches the new timing. core/bifrost.go and plugins/modelcatalogresolver/main.go
 T-Rex LogsWhat T-Rex did
Reviews (1): Last reviewed commit: "fix: move anthropic raw request body che..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if baseProvider == schemas.Anthropic || baseProvider == schemas.Vertex || baseProvider == schemas.Azure { | ||
| return | ||
| } | ||
| ctx.SetValue(schemas.BifrostContextKeyUseRawRequestBody, false) | ||
| ctx.SetValue(schemas.BifrostContextKeySendBackRawResponse, false) | ||
| ctx.SetValue(schemas.BifrostContextKeyPassthroughOverridesPresent, false) |
There was a problem hiding this comment.
This helper permanently clears the Anthropic passthrough flags on the shared request context when a Bedrock or other non-native provider is tried. Fallback attempts reuse the same context, and the native-provider branch returns without restoring the original values for Anthropic, Vertex, or Azure. When an Anthropic-integration request first tries Bedrock and then falls back to a native Anthropic-compatible provider, the fallback still sees UseRawRequestBody, SendBackRawResponse, and PassthroughOverridesPresent as false, so native Claude Code/raw passthrough is disabled incorrectly. Preserve the original passthrough intent separately or compute these as per-attempt effective values instead of destructively clearing the shared context.
| @@ -224,19 +224,6 @@ func ResolveProviderFromCatalog(ctx *schemas.BifrostContext, catalog *modelcatal | |||
| if slices.Contains(providers, preferred) { | |||
There was a problem hiding this comment.
Removing the catalog-time clear changes when these flags are visible. The existing Anthropic route test captures UseRawRequestBody, SendBackRawResponse, and PassthroughOverridesPresent inside RequestConverter, before the new requestWorker helper can run, but it still expects all three to already be false when catalog selection picks Bedrock. That test path will now observe the passthrough values set by checkAnthropicPassthrough instead of the worker-cleared values. Update the integration test to assert the new worker-level behavior, or keep a pre-conversion clear if converters still require these flags to be cleared before dispatch.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
tejas ghatte seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
3 participants
Summary
When an Anthropic-integration request is routed to a non-native Anthropic provider (e.g. Bedrock via a key alias), the raw-body passthrough flags must be cleared so the provider performs its own format conversion. Previously, this guard only fired during catalog resolution, meaning it was silently skipped when the provider was selected through a key alias, prefix, or governance rule — causing tool calls to break when Claude was routed to Bedrock outside of catalog resolution.
Changes
clearAnthropicPassthroughForNonNativeProviderincore/bifrost.go, which clearsBifrostContextKeyUseRawRequestBody,BifrostContextKeySendBackRawResponse, andBifrostContextKeyPassthroughOverridesPresentwhenever an Anthropic-integration request resolves to a provider that isn't Anthropic, Vertex, or Azure.requestWorker, after the final provider is resolved, ensuring it fires regardless of how the provider was selected and re-runs correctly on fallback attempts.modelcatalogresolver/main.gosince the new per-attempt call inrequestWorkercovers all routing paths, making the catalog-level check redundant.TestClearAnthropicPassthroughForNonNativeProvidercovering Bedrock, Anthropic, Vertex, Azure, and non-Anthropic integration types to verify the flags are cleared only in the correct case.Type of change
Affected areas
How to test
go test ./core/... ./plugins/modelcatalogresolver/...To validate end-to-end: configure a model with a key alias that routes to Bedrock, send a Claude tool-call request through the Anthropic integration, and confirm the request is correctly converted by Bedrock rather than passed through as a raw Anthropic body.
Screenshots/Recordings
N/A
Breaking changes
Related issues
Security considerations
No security implications. This change only affects internal context flag management for request routing.
Checklist
docs/contributing/README.mdand followed the guidelines