fix: resolve model names for feature gating - #4520
Merged
Merged
Conversation
Collaborator
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
18 tasks
|
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. |
TejasGhatte
marked this pull request as ready for review
June 18, 2026 08:31
Contributor
|
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 (25)
🚧 Files skipped from review as they are similar to previous changes (24)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new ChangesCanonical Model Resolution and Context-Aware Conversion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
Contributor
Confidence Score: 5/5Safe to merge. The change is additive and narrows only the capability-gating code path; all wire model fields used for routing and API request bodies are explicitly left unchanged. ResolveCanonicalModel is nil-safe at every level (nil ctx, nil Config, empty ModelName) and falls back to the existing wire model when no alias is present, preserving pre-refactor behavior for all non-aliased deployments. The eight-case test table covers every precedence tier. All 25 changed files follow the same capModel pattern consistently, and the Gemini context-parameter threading is correct across callers and tests. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming Request\nbifrostReq.Model = opaque wire ID] --> B{ctx has\nResolvedAlias?}
B -- No --> C[fallbackModel\n= bifrostReq.Model]
B -- Yes --> D{ra.Config.ModelName\nnon-empty?}
D -- Yes --> E[capModel =\nModelName\ne.g. claude-opus-4-8]
D -- No --> F{ra.Config.ModelID\nnon-empty?}
F -- Yes --> G[capModel = ModelID]
F -- No --> C
C --> H[capModel = wire model\npre-refactor behavior preserved]
E --> I[Capability gating checks\nIsAdaptiveOnlyThinkingModel\nSupportsAdaptiveThinking\nIsFableFamily\nSupportsFastMode\netc.]
G --> I
H --> I
A --> J[Wire model unchanged\nfor routing / API body\n/ token limits]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Incoming Request\nbifrostReq.Model = opaque wire ID] --> B{ctx has\nResolvedAlias?}
B -- No --> C[fallbackModel\n= bifrostReq.Model]
B -- Yes --> D{ra.Config.ModelName\nnon-empty?}
D -- Yes --> E[capModel =\nModelName\ne.g. claude-opus-4-8]
D -- No --> F{ra.Config.ModelID\nnon-empty?}
F -- Yes --> G[capModel = ModelID]
F -- No --> C
C --> H[capModel = wire model\npre-refactor behavior preserved]
E --> I[Capability gating checks\nIsAdaptiveOnlyThinkingModel\nSupportsAdaptiveThinking\nIsFableFamily\nSupportsFastMode\netc.]
G --> I
H --> I
A --> J[Wire model unchanged\nfor routing / API body\n/ token limits]
Reviews (6): Last reviewed commit: "fix: resolve model names for feature gat..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
TejasGhatte
force-pushed
the
06-18-fix_resolve_model_names_for_feature_gating
branch
2 times, most recently
from
June 18, 2026 09:47
9ffe79f to
063a1ef
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/providers/gemini/gemini_test.go (1)
356-383:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert converter errors before dereferencing
result.Line 356 and Line 398 capture
errfromToGeminiChatCompletionRequest(...), but the test continues usingresultwithout first assertingrequire.NoError(t, err).Suggested fix
result, err := gemini.ToGeminiChatCompletionRequest(nil, &schemas.BifrostChatRequest{ ... }) +require.NoError(t, err) +require.NotNil(t, result) ... result, err := gemini.ToGeminiChatCompletionRequest(nil, &schemas.BifrostChatRequest{ ... }) +require.NoError(t, err) +require.NotNil(t, result)As per coding guidelines,
**/*.goshould follow explicit error handling practices.Also applies to: 398-414
🤖 Prompt for 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. In `@core/providers/gemini/gemini_test.go` around lines 356 - 383, The test code captures the error returned from the ToGeminiChatCompletionRequest function calls but does not assert that the error is nil before dereferencing and using the result variable. Add require.NoError(t, err) assertions immediately after each call to ToGeminiChatCompletionRequest (at both line 356 and line 398) to properly handle and validate the returned error before proceeding with result usage.Source: Coding guidelines
🧹 Nitpick comments (2)
core/providers/gemini/gemini_test.go (1)
1146-1147: ⚡ Quick winAdd at least one non-
nilcontext test for canonical model gating.These updated call sites pass
nilcontext only, so the new context-based canonical-model path is still untested in this suite. Please add one table case that builds a*schemas.BifrostContextwith alias model metadata and verifies capability gating uses the canonical model, not the wire/deployment model.As per coding guidelines, behavior changes should keep deterministic coverage; this PR stack specifically introduces context-aware canonical model resolution for gating.
Also applies to: 1471-1472, 1740-1741, 1848-1849, 2691-2692, 2980-2981, 3523-3524, 3623-3624
🤖 Prompt for 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. In `@core/providers/gemini/gemini_test.go` around lines 1146 - 1147, The test cases for gemini.ToGeminiChatCompletionRequest only pass nil context, leaving the context-based canonical model path untested. Add at least one new test case to the test table that creates a *schemas.BifrostContext with alias model metadata in tt.input and passes it (non-nil) to the gemini.ToGeminiChatCompletionRequest function call, then verify the result uses the canonical model for capability gating rather than the wire or deployment model. Apply this same pattern to all the mentioned call sites (lines 1146-1147, 1471-1472, 1740-1741, 1848-1849, 2691-2692, 2980-2981, 3523-3524, 3623-3624) to ensure context-aware canonical model resolution is properly covered.Source: Coding guidelines
core/providers/openai/responses_test.go (1)
187-187: ⚡ Quick winAdd at least one non-nil
ctxcanonical-model test path.All updated call sites pass
nilcontext, so the new alias-drivenResolveCanonicalModel(ctx, ...)branch remains untested here. Please add one focused case where context resolves to a different canonical model and assert gating follows the resolved model, not the raw wire model.
As per coding guidelines, Go behavior changes should keep deterministic test coverage for the changed path.Also applies to: 246-246, 281-281, 425-425, 546-546, 1681-1681, 1753-1753, 1879-1879
🤖 Prompt for 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. In `@core/providers/openai/responses_test.go` at line 187, The test cases in the file only call ToOpenAIResponsesRequest with nil context, leaving the ResolveCanonicalModel(ctx, ...) code path untested. Add at least one new test case (in addition to the existing tests at lines 187, 246, 281, 425, 546, 1681, 1753, and 1879) where a non-nil context is passed to ToOpenAIResponsesRequest, configure that context to resolve to a different canonical model than the raw wire model, and then assert that the gating behavior follows the resolved canonical model from the context rather than the wire model. This ensures deterministic test coverage for the new context-driven canonical model resolution path.Source: Coding guidelines
🤖 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 `@core/internal/llmtests/passthrough_api.go`:
- Line 90: In the call to gemini.ToGeminiChatCompletionRequest on line 90,
replace the `nil` argument (first parameter) with the constructed `ctx`
variable. This ensures that context-aware canonical model resolution is properly
performed during the test path instead of being skipped, which allows the test
to catch potential alias-gating regressions.
In `@core/providers/gemini/responses.go`:
- Around line 91-92: The capModel variable assigned from
schemas.ResolveCanonicalModel is not normalized, which causes it to retain
provider prefixes like "google/" that should be stripped before capability
checks. After the line where capModel is assigned, apply the same normalization
logic used for the Gemini request model above (strip the "google/" prefix and
trim whitespace) to ensure capModel is in the correct format for the
supportsThinkingConfig, isGemini3Plus, and multimodal tool-output gating checks.
---
Outside diff comments:
In `@core/providers/gemini/gemini_test.go`:
- Around line 356-383: The test code captures the error returned from the
ToGeminiChatCompletionRequest function calls but does not assert that the error
is nil before dereferencing and using the result variable. Add
require.NoError(t, err) assertions immediately after each call to
ToGeminiChatCompletionRequest (at both line 356 and line 398) to properly handle
and validate the returned error before proceeding with result usage.
---
Nitpick comments:
In `@core/providers/gemini/gemini_test.go`:
- Around line 1146-1147: The test cases for gemini.ToGeminiChatCompletionRequest
only pass nil context, leaving the context-based canonical model path untested.
Add at least one new test case to the test table that creates a
*schemas.BifrostContext with alias model metadata in tt.input and passes it
(non-nil) to the gemini.ToGeminiChatCompletionRequest function call, then verify
the result uses the canonical model for capability gating rather than the wire
or deployment model. Apply this same pattern to all the mentioned call sites
(lines 1146-1147, 1471-1472, 1740-1741, 1848-1849, 2691-2692, 2980-2981,
3523-3524, 3623-3624) to ensure context-aware canonical model resolution is
properly covered.
In `@core/providers/openai/responses_test.go`:
- Line 187: The test cases in the file only call ToOpenAIResponsesRequest with
nil context, leaving the ResolveCanonicalModel(ctx, ...) code path untested. Add
at least one new test case (in addition to the existing tests at lines 187, 246,
281, 425, 546, 1681, 1753, and 1879) where a non-nil context is passed to
ToOpenAIResponsesRequest, configure that context to resolve to a different
canonical model than the raw wire model, and then assert that the gating
behavior follows the resolved canonical model from the context rather than the
wire model. This ensures deterministic test coverage for the new context-driven
canonical model resolution path.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro Plus
Run ID: 1e482125-4c12-49d3-ac7f-fb9bd46f9d73
📒 Files selected for processing (25)
core/internal/llmtests/passthrough_api.gocore/providers/anthropic/anthropic.gocore/providers/anthropic/chat.gocore/providers/anthropic/requestbuilder.gocore/providers/anthropic/responses.gocore/providers/anthropic/utils.gocore/providers/azure/azure.gocore/providers/bedrock/mantle.gocore/providers/bedrock/responses.gocore/providers/bedrock/utils.gocore/providers/gemini/chat.gocore/providers/gemini/gemini.gocore/providers/gemini/gemini_test.gocore/providers/gemini/responses.gocore/providers/openai/advisor_filter_test.gocore/providers/openai/chat.gocore/providers/openai/chat_test.gocore/providers/openai/openai.gocore/providers/openai/responses.gocore/providers/openai/responses_marshal_test.gocore/providers/openai/responses_test.gocore/providers/vertex/utils.gocore/providers/vertex/vertex.gocore/schemas/account.gocore/schemas/account_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- core/providers/vertex/utils.go
- core/providers/anthropic/anthropic.go
- core/providers/anthropic/utils.go
- core/schemas/account.go
- core/providers/anthropic/chat.go
- core/providers/anthropic/requestbuilder.go
- core/schemas/account_test.go
- core/providers/bedrock/utils.go
- core/providers/bedrock/responses.go
- core/providers/anthropic/responses.go
TejasGhatte
force-pushed
the
06-18-fix_resolve_model_names_for_feature_gating
branch
from
June 18, 2026 10:04
063a1ef to
a46d43b
Compare
TejasGhatte
force-pushed
the
06-18-fix_resolve_model_names_for_feature_gating
branch
from
June 18, 2026 10:30
a46d43b to
1adaf17
Compare
This was referenced Jun 18, 2026
TejasGhatte
force-pushed
the
06-18-fix_model_id_in_bedrock_batch_request
branch
from
June 18, 2026 14:06
25b266a to
3af716d
Compare
TejasGhatte
force-pushed
the
06-18-fix_resolve_model_names_for_feature_gating
branch
from
June 18, 2026 14:06
1adaf17 to
dd531f5
Compare
akshaydeo
previously approved these changes
Jun 19, 2026
Contributor
Merge activity
|
akshaydeo
changed the base branch from
06-18-fix_model_id_in_bedrock_batch_request
to
graphite-base/4520
June 19, 2026 06:43
akshaydeo
dismissed stale reviews from coderabbitai[bot] and themself
June 19, 2026 06:43
The base branch was changed.
18 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
3 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
Capability and version gating checks across the Anthropic and Bedrock providers were running against the raw wire model ID, which for deployments like Claude-on-Azure is an opaque deployment name rather than the real model string (e.g.
claude-opus-4-8). This caused feature checks (IsAdaptiveOnlyThinkingModel,SupportsAdaptiveThinking,IsFableFamily,SupportsFastMode, etc.) to silently fall through to incorrect behavior for aliased models.This PR introduces
ResolveCanonicalModelto walk the alias hierarchy and return the correct model string for capability gating, then threads it through every affected call site.Changes
ResolveCanonicalModel(ctx, fallbackModel)tocore/schemas/account.go. It walksResolvedAlias.Config.ModelName→ResolvedAlias.Config.ModelID→fallbackModel, returning the first non-empty tier. This preserves pre-existing behavior when no alias is resolved.anthropic/chat.go,anthropic/responses.go,anthropic/requestbuilder.go,anthropic/anthropic.go,anthropic/utils.go,bedrock/responses.go, andbedrock/utils.gonow resolve acapModelonce per function viaResolveCanonicalModeland pass that to the feature-check functions instead of the rawrequest.Model/bifrostReq.Model.bifrostReq.Model) is intentionally left unchanged everywhere it is used for routing, token limits, or the actual API request body — only the capability gating path is affected.Type of change
Affected areas
How to test
go test ./...To validate the fix end-to-end, configure an Azure-hosted Claude deployment where the deployment name differs from the canonical model string (e.g. deployment ID
my-opus-deployment,ModelName: claude-opus-4-8). Send a request withreasoning.max_tokensset and confirm that adaptive thinking is correctly applied rather than falling back tobudget_tokensmode. Repeat withtemperatureand verify it is stripped for Fable/Mythos family models resolved through an alias.Breaking changes
Security considerations
None. This change only affects how model strings are resolved for feature gating; no auth, secrets, or PII are involved.
Checklist
docs/contributing/README.mdand followed the guidelines