Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

63 changes: 59 additions & 4 deletions src/models/chat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use super::usage::Usage;
#[derive(Debug, Serialize, Deserialize, Clone, ToSchema)]
pub struct ReasoningConfig {
#[serde(skip_serializing_if = "Option::is_none")]
pub effort: Option<String>, // "low" | "medium" | "high"
pub effort: Option<String>, // model-dependent: "none" | "minimal" | "low" | "medium" | "high" | "xhigh"
#[serde(skip_serializing_if = "Option::is_none")]
pub max_tokens: Option<u32>, // Alternative to effort
#[serde(skip_serializing_if = "Option::is_none")]
Expand All @@ -28,16 +28,24 @@ impl ReasoningConfig {
tracing::warn!("Both effort and max_tokens specified - prioritizing max_tokens");
}

// Only validate effort if max_tokens is not present (since max_tokens takes priority)
// 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 {
Comment on lines +31 to 37

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

if effort.trim().is_empty() {
if self.max_tokens.is_none() {
return Err("Effort cannot be empty string".to_string());
}
} else if self.max_tokens.is_none()
&& !["low", "medium", "high"].contains(&effort.as_str())
&& !["none", "minimal", "low", "medium", "high", "xhigh"].contains(&effort.as_str())
{
return Err("Invalid effort value. Must be 'low', 'medium', or 'high'".to_string());
return Err(
"Invalid effort value. Must be one of: none, minimal, low, medium, high, xhigh"
.to_string(),
);
}
}

Expand Down Expand Up @@ -157,3 +165,50 @@ pub struct ChatCompletionChoice {
#[serde(skip_serializing_if = "Option::is_none")]
pub logprobs: Option<LogProbs>,
}

#[cfg(test)]
mod reasoning_effort_validation_tests {
use super::ReasoningConfig;

fn cfg(effort: &str) -> ReasoningConfig {
ReasoningConfig {
effort: Some(effort.to_string()),
max_tokens: None,
exclude: None,
}
}

#[test]
fn accepts_none() {
// `none` is the default reasoning effort on gpt-5.1+; it must not be
// rejected before reaching the provider.
assert!(cfg("none").validate().is_ok());
}

#[test]
fn accepts_minimal_and_xhigh() {
// `minimal` exists on the original GPT-5 series; `xhigh` on gpt-5.1-codex-max.
assert!(cfg("minimal").validate().is_ok());
assert!(cfg("xhigh").validate().is_ok());
}

#[test]
fn still_accepts_low_medium_high() {
assert!(cfg("low").validate().is_ok());
assert!(cfg("medium").validate().is_ok());
assert!(cfg("high").validate().is_ok());
}

#[test]
fn still_rejects_unknown_value() {
// A genuine typo is still caught locally rather than round-tripping to a 400.
let err = cfg("invalid").validate().unwrap_err();
assert!(err.contains("Invalid effort value"));
}

#[test]
fn still_rejects_empty_value() {
let err = cfg("").validate().unwrap_err();
assert!(err.contains("Effort cannot be empty string"));
}
}
107 changes: 94 additions & 13 deletions src/providers/azure/provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,29 @@ impl AzureProvider {
fn api_version(&self) -> String {
self.config.params.get("api_version").unwrap().clone()
}

/// Whether this provider targets Azure's next-generation v1 API surface
/// (`/openai/v1/...`), selected by setting `api_version` to "preview" or "v1".
/// The v1 API is required for the latest models and parameters (for example
/// `reasoning_effort` on gpt-5.x); dated api-versions keep the legacy
/// deployment-in-path form.
fn uses_v1_api(&self) -> bool {
matches!(self.api_version().as_str(), "preview" | "v1")
}

/// 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(),
)
}
}
Comment on lines +71 to +83

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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 test

Repository: 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" -l

Repository: 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 -80

Repository: 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.

}

#[async_trait]
Expand Down Expand Up @@ -113,15 +136,27 @@ impl Provider for AzureProvider {

let deployment = model_config.params.get("deployment").unwrap();
let api_version = self.api_version();
let url = format!(
"{}/{}/chat/completions?api-version={}",
self.endpoint(),
deployment,
api_version
);

// Convert to Azure-specific request format
let azure_request = AzureChatCompletionRequest::from(payload.clone());
let mut azure_request = AzureChatCompletionRequest::from(payload.clone());

// The v1 API routes by the `model` field in the body rather than a deployment
// in the URL path. Legacy dated api-versions keep the deployment-in-path form.
let url = if self.uses_v1_api() {
azure_request.base.model = deployment.clone();
format!(
"{}/chat/completions?api-version={}",
self.v1_base(),
api_version
)
} else {
format!(
"{}/{}/chat/completions?api-version={}",
self.endpoint(),
deployment,
api_version
)
};

let response = self
.http_client
Expand Down Expand Up @@ -209,12 +244,20 @@ impl Provider for AzureProvider {
let deployment = model_config.params.get("deployment").unwrap();
let api_version = self.api_version();

let url = format!(
"{}/{}/embeddings?api-version={}",
self.endpoint(),
deployment,
api_version
);
// The v1 API routes by the `model` field in the body rather than a deployment
// in the URL path. Legacy dated api-versions keep the deployment-in-path form.
let mut payload = payload;
let url = if self.uses_v1_api() {
payload.model = deployment.clone();
format!("{}/embeddings?api-version={}", self.v1_base(), api_version)
} else {
format!(
"{}/{}/embeddings?api-version={}",
self.endpoint(),
deployment,
api_version
)
};

let response = self
.http_client
Expand Down Expand Up @@ -244,6 +287,44 @@ impl Provider for AzureProvider {
}
}

#[cfg(test)]
mod v1_routing_tests {
use super::*;
use std::collections::HashMap;

fn provider(params: &[(&str, &str)]) -> AzureProvider {
AzureProvider {
config: ProviderConfig {
key: "azure".to_string(),
r#type: ProviderType::Azure,
api_key: "test-key".to_string(),
params: params
.iter()
.map(|(k, v)| (k.to_string(), v.to_string()))
.collect::<HashMap<_, _>>(),
},
http_client: Client::new(),
}
}

#[test]
fn preview_api_version_selects_v1_base() {
let p = provider(&[("resource_name", "my-res"), ("api_version", "preview")]);
assert!(p.uses_v1_api());
assert_eq!(p.v1_base(), "https://my-res.openai.azure.com/openai/v1");
}

#[test]
fn dated_api_version_keeps_legacy_deployment_path() {
let p = provider(&[("resource_name", "my-res"), ("api_version", "2024-10-21")]);
assert!(!p.uses_v1_api());
assert_eq!(
p.endpoint(),
"https://my-res.openai.azure.com/openai/deployments"
);
}
}

#[cfg(test)]
mod reasoning_effort_precedence_tests {
use super::*;
Expand Down
Loading