From 46d4659633802f8b49547da476fbb2cad70d3cdc Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 23 Feb 2026 21:21:45 -0500 Subject: [PATCH] Fix OpenAI error field, add Anthropic provider options pass-through, and improve parity tests - Use `status: "incomplete"` instead of `is_error` for OpenAI tool results (fixes rejection) - Add merge_provider_options to forward unknown anthropic provider options to API body - Derive Clone on Client to enable subagent session factory - Enable error_recovery scenario for all providers now that OpenAI is fixed - Improve subagent_spawn test to actually exercise spawn/wait/read workflow - Adjust multi-turn cache test temperature to 0.5 Co-Authored-By: Claude Opus 4.6 --- crates/agent/tests/parity_matrix.rs | 49 +++++++++++--- crates/llm/src/client.rs | 1 + crates/llm/src/providers/anthropic.rs | 95 ++++++++++++++++++++++++++- crates/llm/src/providers/openai.rs | 2 +- crates/llm/tests/integration.rs | 4 +- 5 files changed, 138 insertions(+), 13 deletions(-) diff --git a/crates/agent/tests/parity_matrix.rs b/crates/agent/tests/parity_matrix.rs index 490e756e1..05363f9a9 100644 --- a/crates/agent/tests/parity_matrix.rs +++ b/crates/agent/tests/parity_matrix.rs @@ -3,20 +3,49 @@ use std::sync::Arc; use agent::{ AnthropicProfile, GeminiProfile, LocalExecutionEnvironment, OpenAiProfile, ProviderProfile, - Session, SessionConfig, + Session, SessionConfig, SubAgentManager, }; use llm::client::Client; async fn make_session(provider: &str, model: &str, cwd: &Path) -> Session { dotenvy::dotenv().ok(); let client = Client::from_env().await.expect("Client::from_env failed"); - let profile: Arc = match provider { - "anthropic" => Arc::new(AnthropicProfile::new(model)), - "openai" => Arc::new(OpenAiProfile::new(model)), - "gemini" => Arc::new(GeminiProfile::new(model)), + let mut profile: Box = match provider { + "anthropic" => Box::new(AnthropicProfile::new(model)), + "openai" => Box::new(OpenAiProfile::new(model)), + "gemini" => Box::new(GeminiProfile::new(model)), _ => panic!("unknown provider: {provider}"), }; let env = Arc::new(LocalExecutionEnvironment::new(cwd.to_path_buf())); + + // Register subagent tools so spawn_agent / wait / send_input / close_agent are available + let manager = Arc::new(tokio::sync::Mutex::new(SubAgentManager::new(3))); + let factory_client = client.clone(); + let factory_provider: &str = provider; + let factory_model: String = model.to_string(); + let factory_cwd = cwd.to_path_buf(); + let factory: agent::subagent::SessionFactory = { + let provider = factory_provider.to_string(); + let model = factory_model.clone(); + Arc::new(move || { + let sub_profile: Arc = match provider.as_str() { + "anthropic" => Arc::new(AnthropicProfile::new(&model)), + "openai" => Arc::new(OpenAiProfile::new(&model)), + "gemini" => Arc::new(GeminiProfile::new(&model)), + _ => panic!("unknown provider: {provider}"), + }; + let sub_env = Arc::new(LocalExecutionEnvironment::new(factory_cwd.clone())); + Session::new( + factory_client.clone(), + sub_profile, + sub_env, + SessionConfig::default(), + ) + }) + }; + profile.register_subagent_tools(manager, factory, 0); + + let profile: Arc = Arc::from(profile); let config = SessionConfig { max_turns: 20, ..SessionConfig::default() @@ -89,10 +118,11 @@ provider_tests!(subagent_spawn); // Scenarios below are only generated for providers where they are supported. // - multi_step_read_analyze_edit / provider_specific_editing: gpt-4o-mini is too // weak to reliably apply precise file edits (uses apply_patch, not edit_file). -// - error_recovery: OpenAI rejects the `is_error` field on tool results (adapter bug). // - reasoning_effort: gpt-4o-mini doesn't support the reasoning.effort parameter. // - loop_detection: needs custom config, tested separately below. +provider_tests!(error_recovery); + macro_rules! anthropic_gemini_tests { ($scenario:ident) => { paste::paste! { @@ -118,7 +148,6 @@ macro_rules! anthropic_gemini_tests { } anthropic_gemini_tests!(multi_step_read_analyze_edit); -anthropic_gemini_tests!(error_recovery); anthropic_gemini_tests!(provider_specific_editing); // --------------------------------------------------------------------------- @@ -299,10 +328,12 @@ reasoning_effort_tests!("gemini", "gemini-2.5-flash", gemini_reasoning_effort); // --------------------------------------------------------------------------- // Scenario 12: subagent_spawn // --------------------------------------------------------------------------- -async fn scenario_subagent_spawn(session: &mut Session, _dir: &Path) { +async fn scenario_subagent_spawn(session: &mut Session, dir: &Path) { + std::fs::write(dir.join("secret.txt"), "the_secret_value").expect("failed to write secret.txt"); session .process_input( - "Try to spawn a subagent to read a file. If the subagent tool is not available, just say 'no subagent tool'", + "Spawn a subagent to read the file secret.txt and report its contents. \ + Wait for the subagent to finish, then tell me what it found.", ) .await .expect("process_input failed"); diff --git a/crates/llm/src/client.rs b/crates/llm/src/client.rs index 6e699d788..8f0ffac3f 100644 --- a/crates/llm/src/client.rs +++ b/crates/llm/src/client.rs @@ -7,6 +7,7 @@ use std::collections::HashMap; use std::sync::Arc; /// The core client that routes requests to provider adapters (Section 2.2, 3). +#[derive(Clone)] pub struct Client { providers: HashMap>, default_provider: Option, diff --git a/crates/llm/src/providers/anthropic.rs b/crates/llm/src/providers/anthropic.rs index 2b5e1cede..e1df21b33 100644 --- a/crates/llm/src/providers/anthropic.rs +++ b/crates/llm/src/providers/anthropic.rs @@ -1026,6 +1026,30 @@ impl SseReaderState { } } +/// Known `provider_options.anthropic` keys that are already handled by the adapter +/// and should not be merged into the request body a second time. +const KNOWN_ANTHROPIC_OPTION_KEYS: &[&str] = &["thinking", "auto_cache", "beta_headers"]; + +/// Serialize the API request and merge any unknown `provider_options.anthropic` keys. +fn merge_provider_options( + api_request: &ApiRequest, + provider_options: Option<&serde_json::Value>, +) -> serde_json::Value { + let mut body = serde_json::to_value(api_request).unwrap_or_else(|_| serde_json::json!({})); + + if let Some(anthropic_opts) = provider_options.and_then(|opts| opts.get("anthropic")) { + if let (Some(base), Some(overrides)) = (body.as_object_mut(), anthropic_opts.as_object()) { + for (key, value) in overrides { + if !KNOWN_ANTHROPIC_OPTION_KEYS.contains(&key.as_str()) { + base.insert(key.clone(), value.clone()); + } + } + } + } + + body +} + /// Build an Anthropic API request and HTTP request builder for the given unified request. fn build_api_request( adapter: &Adapter, @@ -1103,7 +1127,7 @@ fn build_api_request( req_builder = req_builder.header("anthropic-beta", beta_str); } - let req_builder = req_builder.json(&api_request); + let req_builder = req_builder.json(&merge_provider_options(&api_request, request.provider_options.as_ref())); (api_request, req_builder) } @@ -1921,6 +1945,75 @@ mod tests { } } + #[test] + fn merge_provider_options_passes_through_unknown_keys() { + let api_request = ApiRequest { + model: "claude-sonnet-4-20250514".to_string(), + messages: vec![ApiMessage { + role: "user".to_string(), + content: vec![serde_json::json!({"type": "text", "text": "Hello"})], + }], + max_tokens: 4096, + system: None, + temperature: None, + top_p: None, + stop_sequences: None, + tools: None, + tool_choice: None, + thinking: None, + metadata: None, + stream: false, + }; + + let opts = serde_json::json!({ + "anthropic": { + "top_k": 40, + "custom_field": "value" + } + }); + let body = merge_provider_options(&api_request, Some(&opts)); + assert_eq!(body["top_k"], 40); + assert_eq!(body["custom_field"], "value"); + } + + #[test] + fn merge_provider_options_skips_known_keys() { + let api_request = ApiRequest { + model: "claude-sonnet-4-20250514".to_string(), + messages: vec![ApiMessage { + role: "user".to_string(), + content: vec![serde_json::json!({"type": "text", "text": "Hello"})], + }], + max_tokens: 4096, + system: None, + temperature: None, + top_p: None, + stop_sequences: None, + tools: None, + tool_choice: None, + thinking: None, + metadata: None, + stream: false, + }; + + let opts = serde_json::json!({ + "anthropic": { + "thinking": {"type": "enabled", "budget_tokens": 10000}, + "auto_cache": false, + "beta_headers": ["some-header"], + "top_k": 40 + } + }); + let body = merge_provider_options(&api_request, Some(&opts)); + // Known keys should not be merged (they are handled separately) + assert!(body.get("auto_cache").is_none()); + assert!(body.get("beta_headers").is_none()); + // thinking is handled by the ApiRequest struct directly, should not be double-merged + assert!(body["thinking"].is_null()); + // Unknown keys should be merged + assert_eq!(body["top_k"], 40); + } + #[test] fn audio_produces_text_fallback() { let part = ContentPart::Audio(crate::types::AudioData { diff --git a/crates/llm/src/providers/openai.rs b/crates/llm/src/providers/openai.rs index 1e6d84cd9..034e6f772 100644 --- a/crates/llm/src/providers/openai.rs +++ b/crates/llm/src/providers/openai.rs @@ -288,7 +288,7 @@ fn translate_input(messages: &[Message]) -> (Option, Vec