fix(azure): route gpt-5.x reasoning_effort via the v1 API (api-version=preview)#106
Open
doronkopit5 wants to merge 2 commits into
Open
fix(azure): route gpt-5.x reasoning_effort via the v1 API (api-version=preview)#106doronkopit5 wants to merge 2 commits into
doronkopit5 wants to merge 2 commits into
Conversation
…n=preview) The Azure provider used a legacy dated api-version (e.g. 2024-10-21), which predates reasoning_effort support, so Azure silently dropped the parameter and ran reasoning models (gpt-5.4-mini) at full/default effort. Migrate chat_completions and embeddings to Azure's v1 API (/openai/v1/...?api-version=preview), passing the deployment as the body `model`. Opt-in per provider: the v1 path is used only when the provider's api_version is "preview" or "v1", so providers on dated api-versions are unchanged. Adds v1-routing unit tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthrough
ChangesAzure v1 API Routing
Reasoning Effort Expansion
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
🔒 Container Vulnerability Scan (hub-migrations - amd64)Click to expand results |
Contributor
🔒 Container Vulnerability Scan (hub-migrations - arm64)Click to expand results |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/providers/azure/provider.rs`:
- Around line 71-83: The v1_base method in the Azure provider lacks validation
to ensure that the base_url parameter is properly formatted for v1 API mode
(must point to the /openai/v1 root). Add validation logic at configuration
initialization time to verify that when api_version is "preview" or "v1", the
provided base_url ends with "/openai/v1" or is empty (so the default is used).
If validation fails, raise an error with a clear message stating the required
format. Alternatively, if validation is deferred to operators, enhance the
documentation comment for v1_base to explicitly specify the exact format
requirement and consequences of misconfiguration.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: eed9cdf5-ee51-4b54-b69e-bae183d44011
📒 Files selected for processing (1)
src/providers/azure/provider.rs
Comment on lines
+71
to
+83
| /// Base URL for the v1 API. When `base_url` is configured it is used verbatim | ||
| /// (operators point it at the `/openai/v1` root); otherwise it is built from | ||
| /// `resource_name`. | ||
| fn v1_base(&self) -> String { | ||
| if let Some(base_url) = self.config.params.get("base_url") { | ||
| base_url.trim_end_matches('/').to_string() | ||
| } else { | ||
| format!( | ||
| "https://{}.openai.azure.com/openai/v1", | ||
| self.config.params.get("resource_name").unwrap(), | ||
| ) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: find Azure preview/v1 configs that might still use legacy-style base URLs.
# Expected: preview/v1 configs should point to /openai/v1 (or be empty so resource_name-derived URL is used).
rg -n -C2 '"api_version"\s*:\s*"(preview|v1)"'
rg -n -C2 '"base_url"\s*:\s*"'Repository: traceloop/hub
Length of output: 39
🏁 Script executed:
# Find where v1_base() is called and how the result is used
rg -n "v1_base\(\)" src/providers/azure/provider.rs
# Check if base_url is validated during config construction/parsing
rg -n "base_url" src/providers/azure/provider.rs -A 2 -B 2
# Search for test coverage of base_url handling
rg -n "base_url" --type rust | grep -i testRepository: traceloop/hub
Length of output: 4532
🏁 Script executed:
# Find uses_v1_api() definition to understand when v1 mode is triggered
rg -n "fn uses_v1_api" src/providers/azure/provider.rs -A 5
# Check api_version extraction/parsing and validation
rg -n "api_version" src/providers/azure/provider.rs -B 2 -A 2
# Search for any config validation that checks base_url format
rg -n "validate\|check\|ensure" src/providers/azure/ -i
# Look at the broader Azure config structure
fd "azure" --type f src/ | xargs rg "struct.*Config" -lRepository: traceloop/hub
Length of output: 3312
🏁 Script executed:
# Check for config validation logic or documentation about base_url format
rg -n "base_url" src/config/ -A 3 -B 3
# Check if there's any documentation or comments about base_url configuration for Azure
rg -n "base_url" --type md
# Look for where base_url gets validated or checked in the provider chain
rg -n "transform_provider_dto\|AzureProviderConfig" src/ -A 10 -B 2 | head -80Repository: traceloop/hub
Length of output: 192
Document base_url format requirement for v1 API mode.
When using api_version "preview" or "v1", the base_url must point to the /openai/v1 root. The code delegates this validation to operators (as noted in the docstring), but there is no operator-facing validation or error handling if misconfigured. Consider adding a check at config initialization time to catch and report base_url mismatches early, rather than failing silently at request time. Alternatively, enhance the configuration documentation to explicitly state the required base_url format for v1 mode.
🤖 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 `@src/providers/azure/provider.rs` around lines 71 - 83, The v1_base method in
the Azure provider lacks validation to ensure that the base_url parameter is
properly formatted for v1 API mode (must point to the /openai/v1 root). Add
validation logic at configuration initialization time to verify that when
api_version is "preview" or "v1", the provided base_url ends with "/openai/v1"
or is empty (so the default is used). If validation fails, raise an error with a
clear message stating the required format. Alternatively, if validation is
deferred to operators, enhance the documentation comment for v1_base to
explicitly specify the exact format requirement and consequences of
misconfiguration.
Contributor
🔒 Container Vulnerability Scan (hub - amd64)Click to expand results |
Contributor
🔒 Container Vulnerability Scan (hub - arm64)Click to expand results |
…g.effort The nested `reasoning.effort` validator rejected any value outside low/medium/high, so `reasoning.effort="none"` was blocked at the gateway before reaching the provider — even though gpt-5.1+ default to and accept `none`. This was inconsistent with the unvalidated top-level `reasoning_effort` field. Widen the accepted set to none/minimal/low/medium/high/xhigh (the union across current OpenAI reasoning models) and let the provider be the source of truth on per-model support; genuinely unknown values are still rejected locally. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
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/models/chat.rs`:
- Around line 31-37: The validate() method now accepts the expanded effort set
including minimal and xhigh values, but the to_thinking_prompt() method (line 81
onward) still only handles low, medium, and high effort values. Update the
to_thinking_prompt() method to map the newly accepted effort values (minimal and
xhigh) to appropriate conversions for non-OpenAI providers (Anthropic/Bedrock
paths) instead of silently treating them as None. Additionally, ensure the same
expansion is applied at line 43 so that all accepted effort values are properly
handled across both validation and conversion paths.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f62f590d-0d93-491a-99ad-9dce476c8c16
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
src/models/chat.rs
Comment on lines
+31
to
37
| // Only validate effort if max_tokens is not present (since max_tokens takes priority). | ||
| // The accepted set is the union across OpenAI reasoning models — values are | ||
| // model-dependent (`none` on gpt-5.1+, `minimal` on the original GPT-5 series, | ||
| // `xhigh` on gpt-5.1-codex-max), so we accept any of them here and let the upstream | ||
| // provider be the source of truth on per-model support (it returns a clear 400 for a | ||
| // value a given model doesn't accept). | ||
| if let Some(effort) = &self.effort { |
Contributor
There was a problem hiding this comment.
Expanded accepted effort set is not propagated to non-OpenAI conversion path.
After this change, validate() accepts minimal and xhigh, but to_thinking_prompt() (Line 81 onward) still only handles low|medium|high, so newly accepted values are silently treated as no prompt (None). This creates a behavior gap for Anthropic/Bedrock-style paths.
Suggested fix
pub fn to_thinking_prompt(&self) -> Option<String> {
if self.max_tokens.is_some() {
// If max_tokens is specified, use a generic thinking prompt
Some("Think through this step-by-step with detailed reasoning.".to_string())
} else {
match self.effort.as_deref() {
Some(effort) if !effort.trim().is_empty() => match effort {
- "high" => {
+ "high" | "xhigh" => {
Some("Think through this step-by-step with detailed reasoning.".to_string())
}
"medium" => Some("Consider this problem thoughtfully.".to_string()),
- "low" => Some("Think about this briefly.".to_string()),
+ "low" | "minimal" => Some("Think about this briefly.".to_string()),
+ "none" => None,
_ => None,
},
_ => None,
}
}
}Also applies to: 43-43
🤖 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 `@src/models/chat.rs` around lines 31 - 37, The validate() method now accepts
the expanded effort set including minimal and xhigh values, but the
to_thinking_prompt() method (line 81 onward) still only handles low, medium, and
high effort values. Update the to_thinking_prompt() method to map the newly
accepted effort values (minimal and xhigh) to appropriate conversions for
non-OpenAI providers (Anthropic/Bedrock paths) instead of silently treating them
as None. Additionally, ensure the same expansion is applied at line 43 so that
all accepted effort values are properly handled across both validation and
conversion paths.
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
1 participant
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.
Why
gpt-5.4-minievaluations ran full reasoning (~3072 reasoning tokens/call, ~22s) despite callers settingreasoning_effort. Root cause: the Azure provider used a 2024-vintage dated api-version (2024-10-21), which predatesreasoning_effortsupport — Azure silently drops unsupported body params, so reasoning ran at default effort.What
AzureProviderchat_completions+embeddingsto Azure's v1 API (/openai/v1/...?api-version=preview), passing the deployment name as the request bodymodel.api_versionis"preview"/"v1". Providers on dated api-versions keep the legacy deployment-in-path behavior unchanged.uses_v1_api()/v1_base()helpers and v1-routing unit tests.Activation
Set the eval Azure provider's
api_versiontopreview(management API/DB); ensure it hasresource_name(or abase_urlat the/openai/v1root).Test
cargo test— 6/6 azure provider tests pass (2 new v1-routing + 4 existing);cargo fmtclean.🤖 Generated with Claude Code
Summary by CodeRabbit
ReasoningConfigeffort validation to accept additional levels:none,minimal, andxhigh.effortvalidation rules.