diff --git a/.env.example b/.env.example index ddadd234..a90dded7 100644 --- a/.env.example +++ b/.env.example @@ -32,6 +32,13 @@ NEARAI_AUTH_URL=https://private.near.ai # LLM_BASE_URL=http://localhost:1234/v1 # LLM_API_KEY=sk-... # optional for local servers +# === OpenRouter (via OpenAI-compatible) === +# LLM_MODEL=anthropic/claude-sonnet-4 +# LLM_BACKEND=openai_compatible +# LLM_BASE_URL=https://openrouter.ai/api/v1 +# LLM_API_KEY=sk-or-... + + # Channel Configuration # CLI is always enabled diff --git a/benchmarks/Cargo.toml b/benchmarks/Cargo.toml index 0ad3ef35..acaeb64d 100644 --- a/benchmarks/Cargo.toml +++ b/benchmarks/Cargo.toml @@ -5,6 +5,7 @@ edition = "2024" rust-version = "1.85" description = "Benchmarking harness for IronClaw agent" license = "MIT OR Apache-2.0" +publish = false [[bin]] name = "ironclaw-bench" diff --git a/benchmarks/src/adapters/gaia.rs b/benchmarks/src/adapters/gaia.rs index 0d2ea168..5142babf 100644 --- a/benchmarks/src/adapters/gaia.rs +++ b/benchmarks/src/adapters/gaia.rs @@ -20,8 +20,6 @@ struct GaiaEntry { level: Option, #[serde(alias = "file_name", default)] file_name: Option, - #[serde(alias = "Annotator Metadata", default)] - annotator_metadata: Option, } /// GAIA benchmark suite. diff --git a/benchmarks/src/channel.rs b/benchmarks/src/channel.rs index 0525be11..83131bb8 100644 --- a/benchmarks/src/channel.rs +++ b/benchmarks/src/channel.rs @@ -64,11 +64,6 @@ impl BenchChannel { pub fn capture(&self) -> Arc> { Arc::clone(&self.capture) } - - /// Get a clone of the message sender for injecting follow-up messages. - pub fn sender(&self) -> mpsc::Sender { - self.msg_tx.clone() - } } #[async_trait] diff --git a/benchmarks/src/error.rs b/benchmarks/src/error.rs index 1eb27f22..06646c32 100644 --- a/benchmarks/src/error.rs +++ b/benchmarks/src/error.rs @@ -14,9 +14,6 @@ pub enum BenchError { #[error("Task {task_id} failed: {reason}")] TaskFailed { task_id: String, reason: String }, - #[error("Timeout after {seconds}s for task {task_id}")] - Timeout { task_id: String, seconds: u64 }, - #[error("Scoring error for task {task_id}: {reason}")] Scoring { task_id: String, reason: String }, @@ -31,10 +28,4 @@ pub enum BenchError { #[error("Agent error: {0}")] Agent(#[from] ironclaw::Error), - - #[error("Results directory error: {0}")] - ResultsDir(String), - - #[error("Resume failed: no completed tasks found in {path}")] - ResumeEmpty { path: PathBuf }, } diff --git a/benchmarks/src/instrumented_llm.rs b/benchmarks/src/instrumented_llm.rs index d3f041d9..165261a8 100644 --- a/benchmarks/src/instrumented_llm.rs +++ b/benchmarks/src/instrumented_llm.rs @@ -15,6 +15,7 @@ use ironclaw::llm::{ /// Recorded metrics from a single LLM call. #[derive(Debug, Clone)] +#[allow(dead_code)] pub struct LlmCallRecord { pub input_tokens: u32, pub output_tokens: u32, diff --git a/benchmarks/src/main.rs b/benchmarks/src/main.rs index 3d188d55..94a6e331 100644 --- a/benchmarks/src/main.rs +++ b/benchmarks/src/main.rs @@ -179,7 +179,6 @@ async fn main() -> anyhow::Result<()> { let session = ironclaw::llm::create_session_manager(ironclaw::llm::SessionConfig { auth_base_url: ironclaw_config.llm.nearai.auth_base_url.clone(), session_path: ironclaw_config.llm.nearai.session_path.clone(), - ..Default::default() }) .await; session.ensure_authenticated().await?; diff --git a/benchmarks/src/results.rs b/benchmarks/src/results.rs index e859f6b0..2013ed26 100644 --- a/benchmarks/src/results.rs +++ b/benchmarks/src/results.rs @@ -1,7 +1,6 @@ use std::collections::HashSet; use std::io::{BufRead, Write}; use std::path::{Path, PathBuf}; -use std::time::Duration; use chrono::{DateTime, Utc}; use uuid::Uuid; @@ -23,12 +22,6 @@ pub struct Trace { pub hit_timeout: bool, } -impl Trace { - pub fn wall_time(&self) -> Duration { - Duration::from_millis(self.wall_time_ms) - } -} - #[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] pub struct TraceToolCall { pub name: String, @@ -73,6 +66,7 @@ pub struct RunResult { impl RunResult { /// Build aggregate from individual task results. + #[allow(clippy::too_many_arguments)] pub fn from_tasks( run_id: Uuid, suite_id: &str, @@ -113,10 +107,6 @@ impl RunResult { finished_at: Utc::now(), } } - - pub fn total_wall_time(&self) -> Duration { - Duration::from_millis(self.total_wall_time_ms) - } } /// Append a single task result as one JSON line to the JSONL file. diff --git a/benchmarks/src/suite.rs b/benchmarks/src/suite.rs index 05874f4c..2ef663f9 100644 --- a/benchmarks/src/suite.rs +++ b/benchmarks/src/suite.rs @@ -44,6 +44,7 @@ pub enum ResourceType { /// What the agent produced for scoring. #[derive(Debug, Clone)] +#[allow(dead_code)] pub struct TaskSubmission { pub response: String, pub conversation: Vec, @@ -108,6 +109,7 @@ impl BenchScore { /// Each suite (GAIA, Tau-bench, custom, etc.) implements this trait /// to provide task loading, scoring, and optional lifecycle hooks. #[async_trait] +#[allow(dead_code)] pub trait BenchSuite: Send + Sync { /// Human-readable name (e.g., "GAIA Validation"). fn name(&self) -> &str; diff --git a/src/bootstrap.rs b/src/bootstrap.rs index bf7d0223..3641c790 100644 --- a/src/bootstrap.rs +++ b/src/bootstrap.rs @@ -98,7 +98,10 @@ pub fn save_bootstrap_env(vars: &[(&str, &str)]) -> std::io::Result<()> { } let mut content = String::new(); for (key, value) in vars { - content.push_str(&format!("{}=\"{}\"\n", key, value)); + // Escape backslashes and double quotes to prevent env var injection + // (e.g. a value containing `"\nINJECTED="x` would break out of quotes). + let escaped = value.replace('\\', "\\\\").replace('"', "\\\""); + content.push_str(&format!("{}=\"{}\"\n", key, escaped)); } std::fs::write(&path, content) } @@ -323,6 +326,34 @@ mod tests { assert!(content.contains("DATABASE_URL=postgres://test")); } + #[test] + fn test_save_bootstrap_env_escapes_quotes() { + let dir = tempdir().unwrap(); + let env_path = dir.path().join(".env"); + + // A malicious URL attempting to inject a second env var + let malicious = r#"http://evil.com" +INJECTED="pwned"#; + let mut content = String::new(); + let escaped = malicious.replace('\\', "\\\\").replace('"', "\\\""); + content.push_str(&format!("LLM_BASE_URL=\"{}\"\n", escaped)); + std::fs::write(&env_path, &content).unwrap(); + + let parsed: Vec<(String, String)> = dotenvy::from_path_iter(&env_path) + .unwrap() + .filter_map(|r| r.ok()) + .collect(); + + // Must parse as exactly one variable, not two + assert_eq!(parsed.len(), 1, "injection must not create extra vars"); + assert_eq!(parsed[0].0, "LLM_BASE_URL"); + // The value should contain the original malicious content (unescaped by dotenvy) + assert!( + parsed[0].1.contains("INJECTED"), + "value should contain the literal injection attempt, not execute it" + ); + } + #[test] fn test_ironclaw_env_path() { let path = ironclaw_env_path(); diff --git a/src/config.rs b/src/config.rs index 6f5d5dc6..a1e0dd01 100644 --- a/src/config.rs +++ b/src/config.rs @@ -692,7 +692,7 @@ impl LlmConfig { LlmBackend::NearAi }; - // Always resolve NEAR AI config (used as fallback and for embeddings) + // Resolve NEAR AI config only when backend is NearAi (or when explicitly configured) let nearai_api_key = optional_env("NEARAI_API_KEY")?.map(SecretString::from); let api_mode = if let Some(mode_str) = optional_env("NEARAI_API_MODE")? { @@ -786,7 +786,9 @@ impl LlmConfig { hint: "Set LLM_BASE_URL when LLM_BACKEND=openai_compatible".to_string(), })?; let api_key = optional_env("LLM_API_KEY")?.map(SecretString::from); - let model = optional_env("LLM_MODEL")?.unwrap_or_else(|| "default".to_string()); + let model = optional_env("LLM_MODEL")? + .or_else(|| settings.selected_model.clone()) + .unwrap_or_else(|| "default".to_string()); Some(OpenAiCompatibleConfig { base_url, api_key, @@ -862,7 +864,7 @@ impl EmbeddingsConfig { key: "EMBEDDING_ENABLED".to_string(), message: format!("must be 'true' or 'false': {e}"), })? - .unwrap_or_else(|| settings.embeddings.enabled || openai_api_key.is_some()); + .unwrap_or(settings.embeddings.enabled); Ok(Self { enabled, @@ -1859,3 +1861,165 @@ where .transpose() .map(|opt| opt.unwrap_or(default)) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::settings::{EmbeddingsSettings, Settings}; + use std::sync::Mutex; + + /// Serializes env-mutating tests to prevent parallel races. + static ENV_MUTEX: Mutex<()> = Mutex::new(()); + + /// Clear all embedding-related env vars. + fn clear_embedding_env() { + // SAFETY: Only called under ENV_MUTEX in tests. No other threads + // observe these vars while the lock is held. + unsafe { + std::env::remove_var("EMBEDDING_ENABLED"); + std::env::remove_var("EMBEDDING_PROVIDER"); + std::env::remove_var("EMBEDDING_MODEL"); + std::env::remove_var("OPENAI_API_KEY"); + } + } + + /// Clear all openai-compatible-related env vars. + fn clear_openai_compatible_env() { + // SAFETY: Only called under ENV_MUTEX in tests. + unsafe { + std::env::remove_var("LLM_BACKEND"); + std::env::remove_var("LLM_BASE_URL"); + std::env::remove_var("LLM_MODEL"); + } + } + + #[test] + fn embeddings_disabled_not_overridden_by_openai_key() { + let _guard = ENV_MUTEX.lock().expect("env mutex poisoned"); + + clear_embedding_env(); + // SAFETY: Under ENV_MUTEX, no concurrent env access. + unsafe { + std::env::set_var("OPENAI_API_KEY", "sk-test-key-for-issue-129"); + } + + let settings = Settings { + embeddings: EmbeddingsSettings { + enabled: false, + ..Default::default() + }, + ..Default::default() + }; + + let config = EmbeddingsConfig::resolve(&settings).expect("resolve should succeed"); + assert!( + !config.enabled, + "embeddings should remain disabled when settings.embeddings.enabled=false, \ + even when OPENAI_API_KEY is set (issue #129)" + ); + + // SAFETY: Under ENV_MUTEX. + unsafe { + std::env::remove_var("OPENAI_API_KEY"); + } + } + + #[test] + fn embeddings_enabled_from_settings() { + let _guard = ENV_MUTEX.lock().expect("env mutex poisoned"); + clear_embedding_env(); + + let settings = Settings { + embeddings: EmbeddingsSettings { + enabled: true, + ..Default::default() + }, + ..Default::default() + }; + + let config = EmbeddingsConfig::resolve(&settings).expect("resolve should succeed"); + assert!( + config.enabled, + "embeddings should be enabled when settings say so" + ); + } + + #[test] + fn embeddings_env_override_takes_precedence() { + let _guard = ENV_MUTEX.lock().expect("env mutex poisoned"); + + clear_embedding_env(); + // SAFETY: Under ENV_MUTEX. + unsafe { + std::env::set_var("EMBEDDING_ENABLED", "true"); + } + + let settings = Settings { + embeddings: EmbeddingsSettings { + enabled: false, + ..Default::default() + }, + ..Default::default() + }; + + let config = EmbeddingsConfig::resolve(&settings).expect("resolve should succeed"); + assert!( + config.enabled, + "EMBEDDING_ENABLED=true env var should override settings" + ); + + // SAFETY: Under ENV_MUTEX. + unsafe { + std::env::remove_var("EMBEDDING_ENABLED"); + } + } + + #[test] + fn openai_compatible_uses_selected_model_when_llm_model_unset() { + let _guard = ENV_MUTEX.lock().expect("env mutex poisoned"); + clear_openai_compatible_env(); + + let settings = Settings { + llm_backend: Some("openai_compatible".to_string()), + openai_compatible_base_url: Some("https://openrouter.ai/api/v1".to_string()), + selected_model: Some("openai/gpt-5.1-codex".to_string()), + ..Default::default() + }; + + let cfg = LlmConfig::resolve(&settings).expect("resolve should succeed"); + let compat = cfg + .openai_compatible + .expect("openai-compatible config should be present"); + + assert_eq!(compat.model, "openai/gpt-5.1-codex"); + } + + #[test] + fn openai_compatible_llm_model_env_overrides_selected_model() { + let _guard = ENV_MUTEX.lock().expect("env mutex poisoned"); + clear_openai_compatible_env(); + // SAFETY: Under ENV_MUTEX. + unsafe { + std::env::set_var("LLM_MODEL", "openai/gpt-5-codex"); + } + + let settings = Settings { + llm_backend: Some("openai_compatible".to_string()), + openai_compatible_base_url: Some("https://openrouter.ai/api/v1".to_string()), + selected_model: Some("openai/gpt-5.1-codex".to_string()), + ..Default::default() + }; + + let cfg = LlmConfig::resolve(&settings).expect("resolve should succeed"); + let compat = cfg + .openai_compatible + .expect("openai-compatible config should be present"); + + assert_eq!(compat.model, "openai/gpt-5-codex"); + + // SAFETY: Under ENV_MUTEX. + unsafe { + std::env::remove_var("LLM_MODEL"); + } + } +} diff --git a/src/llm/failover.rs b/src/llm/failover.rs index c53d725f..a9cb9ed2 100644 --- a/src/llm/failover.rs +++ b/src/llm/failover.rs @@ -105,8 +105,9 @@ impl ProviderCooldown { /// Activate cooldown at the given timestamp. fn activate_cooldown(&self, now_nanos: u64) { + // Ensure 0 remains a safe "not in cooldown" sentinel. self.cooldown_activated_nanos - .store(now_nanos, Ordering::Relaxed); + .store(now_nanos.max(1), Ordering::Relaxed); } /// Reset failure count and clear cooldown (called on success). @@ -215,7 +216,10 @@ impl FailoverProvider { .cooldown_activated_nanos .load(Ordering::Relaxed) }) - .expect("providers list is non-empty"); + .ok_or_else(|| LlmError::RequestFailed { + provider: "failover".to_string(), + reason: "FailoverProvider requires at least one provider".to_string(), + })?; tracing::info!( provider = %self.providers[oldest].model_name(), "All providers in cooldown, trying oldest-cooled provider" @@ -265,9 +269,10 @@ impl FailoverProvider { } } - // SAFETY: `available` is non-empty (guaranteed above), so at least one - // iteration ran and `last_error` is `Some`. - Err(last_error.expect("available providers list is non-empty")) + Err(last_error.unwrap_or_else(|| LlmError::RequestFailed { + provider: "failover".to_string(), + reason: "Invariant violated in FailoverProvider: providers were exhausted but no last_error was recorded (this branch should be unreachable; possible causes: no provider attempts were made or `available` was unexpectedly empty).".to_string(), + })) } } @@ -1041,6 +1046,15 @@ mod tests { assert!(result.is_err()); } + // Test: activate_cooldown(0) still activates cooldown (sentinel collision fix). + #[test] + fn cooldown_at_nanos_zero_still_activates() { + let cd = ProviderCooldown::new(); + cd.activate_cooldown(0); + assert!(cd.is_in_cooldown(0, 1000)); + assert_eq!(cd.cooldown_activated_nanos.load(Ordering::Relaxed), 1); + } + // Test: set_model propagates to all providers and active_model_name reflects change. #[test] fn set_model_propagates_to_all_providers() { diff --git a/src/llm/mod.rs b/src/llm/mod.rs index ef91f8e2..21d841f2 100644 --- a/src/llm/mod.rs +++ b/src/llm/mod.rs @@ -209,9 +209,11 @@ fn create_openai_compatible_provider(config: &LlmConfig) -> Result anyhow::Result<()> { }; let session = create_session_manager(session_config).await; - // Session-based auth is only needed for NEAR AI backend without an API key. - // ChatCompletions mode with an API key skips session auth entirely. - if config.llm.backend == ironclaw::config::LlmBackend::NearAi - && config.llm.nearai.api_key.is_none() - { - session.ensure_authenticated().await?; - } - // Initialize tracing let env_filter = EnvFilter::try_from_default_env() .unwrap_or_else(|_| EnvFilter::new("ironclaw=info,tower_http=warn")); @@ -574,6 +566,15 @@ async fn main() -> anyhow::Result<()> { } } + // Session-based auth is only needed for NEAR AI backend without an API key. + // Do this after DB-backed config reload so provider selection from onboarding + // is respected (e.g. OpenAI/OpenAI-compatible should not trigger NEAR auth). + if config.llm.backend == ironclaw::config::LlmBackend::NearAi + && config.llm.nearai.api_key.is_none() + { + session.ensure_authenticated().await?; + } + // Start managed tunnel if configured and no static URL is already set. // // The tunnel process runs in the background, exposing the local gateway diff --git a/src/settings.rs b/src/settings.rs index e8d1242f..0f12dcec 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -1054,6 +1054,37 @@ mod tests { ); } + #[test] + fn test_openai_compatible_db_map_round_trip() { + let settings = Settings { + llm_backend: Some("openai_compatible".to_string()), + openai_compatible_base_url: Some("http://my-vllm:8000/v1".to_string()), + embeddings: EmbeddingsSettings { + enabled: false, + ..Default::default() + }, + ..Default::default() + }; + + let map = settings.to_db_map(); + let restored = Settings::from_db_map(&map); + + assert_eq!( + restored.llm_backend, + Some("openai_compatible".to_string()), + "llm_backend must survive DB round-trip" + ); + assert_eq!( + restored.openai_compatible_base_url, + Some("http://my-vllm:8000/v1".to_string()), + "openai_compatible_base_url must survive DB round-trip" + ); + assert!( + !restored.embeddings.enabled, + "embeddings.enabled=false must survive DB round-trip" + ); + } + #[test] fn toml_round_trip() { let dir = tempfile::tempdir().unwrap(); @@ -1124,8 +1155,6 @@ mod tests { let mut toml_overlay = Settings::default(); toml_overlay.agent.name = "from-toml".to_string(); - // heartbeat.interval_secs stays at default (1800) in the overlay, - // so the base value (600) should be preserved. base.merge_from(&toml_overlay); @@ -1142,7 +1171,6 @@ mod tests { let overlay = Settings::default(); base.merge_from(&overlay); - // All base values preserved since overlay is entirely default assert_eq!(base.agent.name, "custom-name"); assert!(base.heartbeat.enabled); } diff --git a/src/setup/README.md b/src/setup/README.md index 5c6516ea..dda642b3 100644 --- a/src/setup/README.md +++ b/src/setup/README.md @@ -299,16 +299,26 @@ Contains only the settings needed BEFORE database connection. Written by ```env DATABASE_BACKEND="libsql" LIBSQL_PATH="/Users/name/.ironclaw/ironclaw.db" +LLM_BACKEND="openai_compatible" +LLM_BASE_URL="http://my-vllm:8000/v1" ``` -Or for PostgreSQL: +Or for PostgreSQL + NEAR AI: ```env DATABASE_BACKEND="postgres" DATABASE_URL="postgres://user:pass@localhost/ironclaw" +LLM_BACKEND="nearai" +``` + +Or for Ollama: +```env +LLM_BACKEND="ollama" +OLLAMA_BASE_URL="http://localhost:11434" ``` **Why separate?** Chicken-and-egg: you need `DATABASE_BACKEND` to know -which database to connect to, so it can't be stored in the database. +which database to connect to, and `LLM_BACKEND` to know whether to +attempt NEAR AI session auth -- neither can be stored in the database. **Layer 2: Database settings table** (everything else) @@ -339,6 +349,9 @@ Final step of the wizard: - DATABASE_URL (if postgres) - LIBSQL_PATH (if libsql) - LIBSQL_URL (if turso sync) + - LLM_BACKEND (always, when set) + - LLM_BASE_URL (if openai_compatible) + - OLLAMA_BASE_URL (if ollama) 4. Print configuration summary ``` diff --git a/src/setup/wizard.rs b/src/setup/wizard.rs index 2625537e..4988abaf 100644 --- a/src/setup/wizard.rs +++ b/src/setup/wizard.rs @@ -1530,6 +1530,18 @@ impl SetupWizard { env_vars.push(("LIBSQL_URL", url.clone())); } + // LLM bootstrap vars: same chicken-and-egg problem as DATABASE_BACKEND. + // Config::from_env() needs the backend before the DB is connected. + if let Some(ref backend) = self.settings.llm_backend { + env_vars.push(("LLM_BACKEND", backend.clone())); + } + if let Some(ref url) = self.settings.openai_compatible_base_url { + env_vars.push(("LLM_BASE_URL", url.clone())); + } + if let Some(ref url) = self.settings.ollama_base_url { + env_vars.push(("OLLAMA_BASE_URL", url.clone())); + } + if !env_vars.is_empty() { let pairs: Vec<(&str, &str)> = env_vars.iter().map(|(k, v)| (*k, v.as_str())).collect(); @@ -1764,8 +1776,10 @@ async fn fetch_anthropic_models(cached_key: Option<&str>) -> Vec<(String, String /// Returns `(model_id, display_label)` pairs. Falls back to static defaults on error. async fn fetch_openai_models(cached_key: Option<&str>) -> Vec<(String, String)> { let static_defaults = vec![ + ("gpt-5".into(), "GPT-5 (flagship)".into()), + ("gpt-5-mini".into(), "GPT-5 Mini (fast)".into()), + ("gpt-4.1".into(), "GPT-4.1".into()), ("gpt-4o".into(), "GPT-4o".into()), - ("gpt-4o-mini".into(), "GPT-4o Mini (fast)".into()), ("o3".into(), "o3 (reasoning)".into()), ]; @@ -1800,19 +1814,12 @@ async fn fetch_openai_models(cached_key: Option<&str>) -> Vec<(String, String)> data: Vec, } - // Prefixes that indicate chat-relevant models - let chat_prefixes = ["gpt-4", "gpt-3.5", "o1", "o3", "o4", "chatgpt"]; - match resp.json::().await { Ok(body) => { let mut models: Vec<(String, String)> = body .data .into_iter() - .filter(|m| { - chat_prefixes.iter().any(|p| m.id.starts_with(p)) - && !m.id.contains("realtime") - && !m.id.contains("audio") - }) + .filter(|m| is_openai_chat_model(&m.id)) .map(|m| { let label = m.id.clone(); (m.id, label) @@ -1821,13 +1828,74 @@ async fn fetch_openai_models(cached_key: Option<&str>) -> Vec<(String, String)> if models.is_empty() { return static_defaults; } - models.sort_by(|a, b| a.0.cmp(&b.0)); + sort_openai_models(&mut models); models } Err(_) => static_defaults, } } +fn is_openai_chat_model(model_id: &str) -> bool { + let id = model_id.to_ascii_lowercase(); + + let is_chat_family = id.starts_with("gpt-") + || id.starts_with("chatgpt-") + || id.starts_with("o1") + || id.starts_with("o3") + || id.starts_with("o4") + || id.starts_with("o5"); + + let is_non_chat_variant = id.contains("realtime") + || id.contains("audio") + || id.contains("transcribe") + || id.contains("tts") + || id.contains("embedding") + || id.contains("moderation") + || id.contains("image"); + + is_chat_family && !is_non_chat_variant +} + +fn openai_model_priority(model_id: &str) -> usize { + let id = model_id.to_ascii_lowercase(); + + const EXACT_PRIORITY: &[&str] = &[ + "gpt-5", + "gpt-5-mini", + "gpt-5-nano", + "o3", + "o4-mini", + "o1", + "gpt-4.1", + "gpt-4.1-mini", + "gpt-4o", + "gpt-4o-mini", + ]; + if let Some(pos) = EXACT_PRIORITY.iter().position(|m| id == *m) { + return pos; + } + + const PREFIX_PRIORITY: &[&str] = &[ + "gpt-5-", "o3-", "o4-", "o1-", "gpt-4.1-", "gpt-4o-", "gpt-3.5-", "chatgpt-", + ]; + if let Some(pos) = PREFIX_PRIORITY + .iter() + .position(|prefix| id.starts_with(prefix)) + { + return EXACT_PRIORITY.len() + pos; + } + + EXACT_PRIORITY.len() + PREFIX_PRIORITY.len() + 1 +} + +fn sort_openai_models(models: &mut [(String, String)]) { + models.sort_by(|a, b| { + openai_model_priority(&a.0) + .cmp(&openai_model_priority(&b.0)) + .then_with(|| a.0.cmp(&b.0)) + }); +} + /// Fetch installed models from a local Ollama instance. /// /// Returns `(model_name, display_label)` pairs. Falls back to static defaults on error. @@ -2158,12 +2226,49 @@ mod tests { let _guard = EnvGuard::clear("OPENAI_API_KEY"); let models = fetch_openai_models(None).await; assert!(!models.is_empty()); + assert_eq!(models[0].0, "gpt-5"); assert!( models.iter().any(|(id, _)| id.contains("gpt")), "static defaults should include a GPT model" ); } + #[test] + fn test_is_openai_chat_model_includes_gpt5_and_filters_non_chat_variants() { + assert!(is_openai_chat_model("gpt-5")); + assert!(is_openai_chat_model("gpt-5-mini-2026-01-01")); + assert!(is_openai_chat_model("o3-2025-04-16")); + assert!(!is_openai_chat_model("chatgpt-image-latest")); + assert!(!is_openai_chat_model("gpt-4o-realtime-preview")); + assert!(!is_openai_chat_model("gpt-4o-mini-transcribe")); + assert!(!is_openai_chat_model("text-embedding-3-large")); + } + + #[test] + fn test_sort_openai_models_prioritizes_best_models_first() { + let mut models = vec![ + ("gpt-4o-mini".to_string(), "gpt-4o-mini".to_string()), + ("gpt-5-mini".to_string(), "gpt-5-mini".to_string()), + ("o3".to_string(), "o3".to_string()), + ("gpt-4.1".to_string(), "gpt-4.1".to_string()), + ("gpt-5".to_string(), "gpt-5".to_string()), + ]; + + sort_openai_models(&mut models); + + let ordered: Vec = models.into_iter().map(|(id, _)| id).collect(); + assert_eq!( + ordered, + vec![ + "gpt-5".to_string(), + "gpt-5-mini".to_string(), + "o3".to_string(), + "gpt-4.1".to_string(), + "gpt-4o-mini".to_string(), + ] + ); + } + #[tokio::test] async fn test_fetch_ollama_models_unreachable_fallback() { // Point at a port nothing listens on diff --git a/src/tools/builtin/http.rs b/src/tools/builtin/http.rs index 1f37480c..8b4593c2 100644 --- a/src/tools/builtin/http.rs +++ b/src/tools/builtin/http.rs @@ -106,6 +106,46 @@ fn is_disallowed_ip(ip: &IpAddr) -> bool { } } +fn parse_headers_param( + headers: Option<&serde_json::Value>, +) -> Result, ToolError> { + match headers { + None => Ok(Vec::new()), + Some(serde_json::Value::Object(map)) => { + let mut out = Vec::with_capacity(map.len()); + for (k, v) in map { + let value = v.as_str().ok_or_else(|| { + ToolError::InvalidParameters(format!("header '{}' must have a string value", k)) + })?; + out.push((k.clone(), value.to_string())); + } + Ok(out) + } + Some(serde_json::Value::Array(items)) => { + let mut out = Vec::with_capacity(items.len()); + for (idx, item) in items.iter().enumerate() { + let obj = item.as_object().ok_or_else(|| { + ToolError::InvalidParameters(format!( + "headers[{}] must be an object with 'name' and 'value'", + idx + )) + })?; + let name = obj.get("name").and_then(|v| v.as_str()).ok_or_else(|| { + ToolError::InvalidParameters(format!("headers[{}].name must be a string", idx)) + })?; + let value = obj.get("value").and_then(|v| v.as_str()).ok_or_else(|| { + ToolError::InvalidParameters(format!("headers[{}].value must be a string", idx)) + })?; + out.push((name.to_string(), value.to_string())); + } + Ok(out) + } + Some(_) => Err(ToolError::InvalidParameters( + "'headers' must be an object or an array of {name, value}".to_string(), + )), + } +} + impl Default for HttpTool { fn default() -> Self { Self::new() @@ -136,12 +176,21 @@ impl Tool for HttpTool { "description": "The URL to request" }, "headers": { - "type": "object", - "additionalProperties": { "type": "string" }, - "description": "HTTP headers to include" + "type": "array", + "description": "Optional headers as a list of {name, value} objects", + "items": { + "type": "object", + "properties": { + "name": { "type": "string" }, + "value": { "type": "string" } + }, + "required": ["name", "value"], + "additionalProperties": false + } }, "body": { - "description": "Request body (for POST/PUT/PATCH)" + "type": "string", + "description": "Request body. Use plain text or serialized JSON." }, "timeout_secs": { "type": "integer", @@ -165,14 +214,7 @@ impl Tool for HttpTool { let parsed_url = validate_url(url)?; // Parse headers - let headers: HashMap = params - .get("headers") - .and_then(|v| serde_json::from_value(v.clone()).ok()) - .unwrap_or_default(); - let headers_vec: Vec<(String, String)> = headers - .iter() - .map(|(k, v)| (k.clone(), v.clone())) - .collect(); + let headers_vec = parse_headers_param(params.get("headers"))?; // Build request let mut request = match method.to_uppercase().as_str() { @@ -190,16 +232,31 @@ impl Tool for HttpTool { }; // Add headers - for (key, value) in headers { - request = request.header(&key, &value); + for (key, value) in &headers_vec { + request = request.header(key.as_str(), value.as_str()); } // Add body if present let body_bytes = if let Some(body) = params.get("body") { - let bytes = serde_json::to_vec(body) - .map_err(|e| ToolError::InvalidParameters(format!("invalid body JSON: {}", e)))?; - request = request.json(body); - Some(bytes) + if let Some(body_str) = body.as_str() { + if let Ok(json_body) = serde_json::from_str::(body_str) { + let bytes = serde_json::to_vec(&json_body).map_err(|e| { + ToolError::InvalidParameters(format!("invalid body JSON: {}", e)) + })?; + request = request.json(&json_body); + Some(bytes) + } else { + let bytes = body_str.as_bytes().to_vec(); + request = request.body(body_str.to_string()); + Some(bytes) + } + } else { + let bytes = serde_json::to_vec(body).map_err(|e| { + ToolError::InvalidParameters(format!("invalid body JSON: {}", e)) + })?; + request = request.json(body); + Some(bytes) + } } else { None }; @@ -304,6 +361,20 @@ impl Tool for HttpTool { mod tests { use super::*; + #[test] + fn test_http_tool_schema_body_has_type() { + let tool = HttpTool::new(); + let schema = tool.parameters_schema(); + assert_eq!(schema["properties"]["body"]["type"], "string"); + } + + #[test] + fn test_http_tool_schema_headers_is_array() { + let tool = HttpTool::new(); + let schema = tool.parameters_schema(); + assert_eq!(schema["properties"]["headers"]["type"], "array"); + } + #[test] fn test_validate_url_rejects_http() { let err = validate_url("http://example.com").unwrap_err(); @@ -363,4 +434,30 @@ mod tests { // MAX_RESPONSE_SIZE should be 5 MB to prevent OOM while allowing typical API responses. assert_eq!(MAX_RESPONSE_SIZE, 5 * 1024 * 1024); } + + #[test] + fn test_parse_headers_param_accepts_object_legacy_shape() { + let headers = serde_json::json!({"Authorization": "Bearer token"}); + let parsed = parse_headers_param(Some(&headers)).unwrap(); + assert_eq!( + parsed, + vec![("Authorization".to_string(), "Bearer token".to_string())] + ); + } + + #[test] + fn test_parse_headers_param_accepts_array_shape() { + let headers = serde_json::json!([ + {"name": "Authorization", "value": "Bearer token"}, + {"name": "X-Test", "value": "1"} + ]); + let parsed = parse_headers_param(Some(&headers)).unwrap(); + assert_eq!( + parsed, + vec![ + ("Authorization".to_string(), "Bearer token".to_string()), + ("X-Test".to_string(), "1".to_string()) + ] + ); + } } diff --git a/src/tools/builtin/json.rs b/src/tools/builtin/json.rs index 5350dbb7..f6d91a0d 100644 --- a/src/tools/builtin/json.rs +++ b/src/tools/builtin/json.rs @@ -28,7 +28,8 @@ impl Tool for JsonTool { "description": "The JSON operation to perform" }, "data": { - "description": "The JSON data to operate on (string for parse, object otherwise)" + "type": "string", + "description": "JSON input string. For query/stringify/validate, pass serialized JSON." }, "path": { "type": "string", @@ -64,7 +65,8 @@ impl Tool for JsonTool { parsed } "stringify" => { - let json_str = serde_json::to_string_pretty(data).map_err(|e| { + let value = parse_json_input(data)?; + let json_str = serde_json::to_string_pretty(&value).map_err(|e| { ToolError::ExecutionFailed(format!("failed to stringify: {}", e)) })?; @@ -75,14 +77,14 @@ impl Tool for JsonTool { ToolError::InvalidParameters("missing 'path' parameter for query".to_string()) })?; - query_json(data, path)? + let value = parse_json_input(data)?; + query_json(&value, path)? } "validate" => { - let is_valid = if let Some(s) = data.as_str() { - serde_json::from_str::(s).is_ok() - } else { - true // Already a valid JSON value - }; + let is_valid = data + .as_str() + .map(|s| serde_json::from_str::(s).is_ok()) + .unwrap_or(false); serde_json::json!({ "valid": is_valid }) } @@ -102,6 +104,14 @@ impl Tool for JsonTool { } } +fn parse_json_input(data: &serde_json::Value) -> Result { + let json_str = data + .as_str() + .ok_or_else(|| ToolError::InvalidParameters("'data' must be a JSON string".to_string()))?; + serde_json::from_str(json_str) + .map_err(|e| ToolError::InvalidParameters(format!("invalid JSON input: {}", e))) +} + /// Simple JSONPath-like query implementation. fn query_json(data: &serde_json::Value, path: &str) -> Result { let mut current = data; @@ -144,6 +154,13 @@ fn query_json(data: &serde_json::Value, path: &str) -> Result