diff --git a/Cargo.lock b/Cargo.lock index e7e59b64..a9668c1c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2453,6 +2453,12 @@ version = "0.1.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c34f04666d835ff5d62e058c3995147c06f42fe86ff053337632bca83e42702d" +[[package]] +name = "env-lock" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2afbd3cff67810192b5bdb76b77feff72775314d41195c1d779bc92e0e83285d" + [[package]] name = "env_filter" version = "0.1.3" @@ -3090,6 +3096,7 @@ dependencies = [ "dashmap", "dirs 5.0.1", "dotenvy", + "env-lock", "etcetera 0.11.0", "fs2", "futures", @@ -3308,6 +3315,7 @@ dependencies = [ "chrono", "clap", "config", + "env-lock", "fs2", "futures", "goose", diff --git a/crates/goose-cli/src/commands/recipe.rs b/crates/goose-cli/src/commands/recipe.rs index 234da56d..6ff112fd 100644 --- a/crates/goose-cli/src/commands/recipe.rs +++ b/crates/goose-cli/src/commands/recipe.rs @@ -45,39 +45,55 @@ pub fn handle_deeplink(recipe_name: &str, params: &[String]) -> Result { } pub fn handle_open(recipe_name: &str, params: &[String]) -> Result<()> { - // Generate the deeplink using the helper function (no printing) - // This reuses all the validation and encoding logic + handle_open_with( + recipe_name, + params, + |url| open::that(url), + &mut std::io::stdout(), + ) +} + +fn handle_open_with( + recipe_name: &str, + params: &[String], + opener: F, + out: &mut W, +) -> Result<()> +where + F: FnOnce(&str) -> std::io::Result<()>, + W: std::io::Write, +{ let params_map = parse_params(params)?; match generate_deeplink(recipe_name, params_map) { - Ok((deeplink_url, recipe)) => { - // Attempt to open the deeplink - match open::that(&deeplink_url) { - Ok(_) => { - println!( - "{} Opened recipe '{}' in Goose Desktop", - style("✓").green().bold(), - recipe.title - ); - Ok(()) - } - Err(err) => { - println!( - "{} Failed to open recipe in Goose Desktop: {}", - style("✗").red().bold(), - err - ); - println!("Generated deeplink: {}", deeplink_url); - println!("You can manually copy and open the URL above, or ensure Goose Desktop is installed."); - Err(anyhow::anyhow!("Failed to open recipe: {}", err)) - } + Ok((deeplink_url, recipe)) => match opener(&deeplink_url) { + Ok(_) => { + writeln!( + out, + "{} Opened recipe '{}' in Goose Desktop", + style("✓").green().bold(), + recipe.title + )?; + Ok(()) } - } + Err(err) => { + writeln!( + out, + "{} Failed to open recipe in Goose Desktop: {}", + style("✗").red().bold(), + err + )?; + writeln!(out, "Generated deeplink: {}", deeplink_url)?; + writeln!(out, "You can manually copy and open the URL above, or ensure Goose Desktop is installed.")?; + Err(anyhow::anyhow!("Failed to open recipe: {}", err)) + } + }, Err(err) => { - println!( + writeln!( + out, "{} Failed to encode recipe: {}", style("✗").red().bold(), err - ); + )?; Err(err) } } @@ -250,47 +266,82 @@ instructions: "Test instructions" assert!(result.is_err()); } + fn run_handle_open( + recipe_path: &str, + params: &[String], + opener_result: std::io::Result<()>, + ) -> (Result<()>, String, String) { + let captured_url = std::cell::RefCell::new(String::new()); + let mut out = Vec::new(); + let result = handle_open_with( + recipe_path, + params, + |url| { + *captured_url.borrow_mut() = url.to_string(); + opener_result + }, + &mut out, + ); + let output = String::from_utf8(out).unwrap(); + (result, captured_url.into_inner(), output) + } + #[test] fn test_handle_open_recipe() { - let temp_dir = TempDir::new().expect("Failed to create temp directory"); + let temp_dir = TempDir::new().unwrap(); let recipe_path = create_test_recipe_file(&temp_dir, "test_recipe.yaml", VALID_RECIPE_CONTENT); - // Test handle_open - should attempt to open but may fail (that's expected in test environment) - // We just want to ensure it doesn't panic and handles the error gracefully - let result = handle_open(&recipe_path, &[]); - // The result may be Ok or Err depending on whether the system can open the URL - // In a test environment, it will likely fail to open, but that's fine - // We're mainly testing that the function doesn't panic and processes the recipe correctly - match result { - Ok(_) => { - // Successfully opened (unlikely in test environment) - } - Err(_) => { - // Failed to open (expected in test environment) - this is fine - } - } + let (expected_url, _) = generate_deeplink(&recipe_path, HashMap::new()).unwrap(); + let (result, captured_url, _) = run_handle_open(&recipe_path, &[], Ok(())); + + assert!(result.is_ok()); + assert_eq!(captured_url, expected_url); } #[test] fn test_handle_open_with_parameters() { - let temp_dir = TempDir::new().expect("Failed to create temp directory"); + let temp_dir = TempDir::new().unwrap(); let recipe_path = create_test_recipe_file(&temp_dir, "test_recipe.yaml", VALID_RECIPE_CONTENT); + let (base_url, _) = generate_deeplink(&recipe_path, HashMap::new()).unwrap(); + let params = vec!["name=Alice".to_string(), "role=developer".to_string()]; - let result = handle_open(&recipe_path, ¶ms); - // The result may be Ok or Err depending on whether the system can open the URL - // In a test environment, it will likely fail to open, but that's fine - // We're mainly testing that the function processes parameters correctly and doesn't panic - match result { - Ok(_) => { - // Successfully opened (unlikely in test environment) - } - Err(_) => { - // Failed to open (expected in test environment) - this is fine - } - } + let (result, captured_url, _) = run_handle_open(&recipe_path, ¶ms, Ok(())); + + assert!(result.is_ok()); + assert!(captured_url.starts_with(&base_url)); + assert!(captured_url.contains("&name=Alice")); + assert!(captured_url.contains("&role=developer")); + } + + #[test] + fn test_handle_open_opener_fails() { + let temp_dir = TempDir::new().unwrap(); + let recipe_path = + create_test_recipe_file(&temp_dir, "test_recipe.yaml", VALID_RECIPE_CONTENT); + + let (expected_url, _) = generate_deeplink(&recipe_path, HashMap::new()).unwrap(); + let opener_err = std::io::Error::new(std::io::ErrorKind::NotFound, "desktop not found"); + let (result, _, output) = run_handle_open(&recipe_path, &[], Err(opener_err)); + + assert!(result.is_err()); + assert!(output.contains("Failed to open recipe in Goose Desktop")); + assert!(output.contains("desktop not found")); + assert!(output.contains(&expected_url)); + } + + #[test] + fn test_handle_open_invalid_recipe() { + let temp_dir = TempDir::new().unwrap(); + let recipe_path = + create_test_recipe_file(&temp_dir, "invalid.yaml", INVALID_RECIPE_CONTENT); + + let (result, _, output) = run_handle_open(&recipe_path, &[], Ok(())); + + assert!(result.is_err()); + assert!(output.contains("Failed to encode recipe")); } #[test] diff --git a/crates/goose-server/Cargo.toml b/crates/goose-server/Cargo.toml index b2a91e05..f990b388 100644 --- a/crates/goose-server/Cargo.toml +++ b/crates/goose-server/Cargo.toml @@ -62,3 +62,4 @@ path = "src/bin/generate_schema.rs" tower = "0.5" async-trait = "0.1.89" tempfile = "3.15.0" +env-lock = "1.0.1" diff --git a/crates/goose-server/src/routes/audio.rs b/crates/goose-server/src/routes/audio.rs index 707f1b5b..73434b0a 100644 --- a/crates/goose-server/src/routes/audio.rs +++ b/crates/goose-server/src/routes/audio.rs @@ -395,6 +395,8 @@ mod tests { #[tokio::test(flavor = "multi_thread")] async fn test_transcribe_endpoint_requires_auth() { + let _guard = env_lock::lock_env([("OPENAI_API_KEY", Some("fake-openai-no-keyring"))]); + let state = AppState::new().await.unwrap(); let app = routes(state); // Test without auth header diff --git a/crates/goose/Cargo.toml b/crates/goose/Cargo.toml index eb7e2b4c..74b892e5 100644 --- a/crates/goose/Cargo.toml +++ b/crates/goose/Cargo.toml @@ -132,6 +132,7 @@ temp-env = "0.3.6" dotenvy = "0.15.7" ctor = "0.2.9" test-case = "3.3" +env-lock = "1.0.1" [[example]] name = "agent" diff --git a/crates/goose/src/providers/base.rs b/crates/goose/src/providers/base.rs index 4a84fff8..cec5734d 100644 --- a/crates/goose/src/providers/base.rs +++ b/crates/goose/src/providers/base.rs @@ -338,6 +338,9 @@ pub trait LeadWorkerProviderTrait { /// Get the currently active model name fn get_active_model(&self) -> String; + + /// Get (lead_turns, failure_threshold, fallback_turns) + fn get_settings(&self) -> (usize, usize, usize); } /// Base trait for AI providers (OpenAI, Anthropic, etc) diff --git a/crates/goose/src/providers/factory.rs b/crates/goose/src/providers/factory.rs index 11aab4dd..4dab2b93 100644 --- a/crates/goose/src/providers/factory.rs +++ b/crates/goose/src/providers/factory.rs @@ -222,14 +222,10 @@ fn create_worker_model_config(default_model: &ModelConfig) -> Result("GOOSE_WORKER_CONTEXT_LIMIT") { - if let Ok(limit) = limit_str.parse::() { - worker_config = worker_config.with_context_limit(Some(limit)); - } - } else if let Ok(limit_str) = global_config.get_param::("GOOSE_CONTEXT_LIMIT") { - if let Ok(limit) = limit_str.parse::() { - worker_config = worker_config.with_context_limit(Some(limit)); - } + if let Ok(limit) = global_config.get_param::("GOOSE_WORKER_CONTEXT_LIMIT") { + worker_config = worker_config.with_context_limit(Some(limit)); + } else if let Ok(limit) = global_config.get_param::("GOOSE_CONTEXT_LIMIT") { + worker_config = worker_config.with_context_limit(Some(limit)); } Ok(worker_config) @@ -238,148 +234,76 @@ fn create_worker_model_config(default_model: &ModelConfig) -> Result)>, - } - - impl EnvVarGuard { - fn new(vars: &[&str]) -> Self { - let saved_vars = vars - .iter() - .map(|&var| (var.to_string(), env::var(var).ok())) - .collect(); - - for &var in vars { - env::remove_var(var); - } - - Self { vars: saved_vars } - } - - fn set(&self, key: &str, value: &str) { - env::set_var(key, value); - } - } - - impl Drop for EnvVarGuard { - fn drop(&mut self) { - for (key, value) in &self.vars { - match value { - Some(val) => env::set_var(key, val), - None => env::remove_var(key), - } - } - } - } + #[test_case::test_case(None, None, None, DEFAULT_LEAD_TURNS, DEFAULT_FAILURE_THRESHOLD, DEFAULT_FALLBACK_TURNS ; "defaults")] + #[test_case::test_case(Some("7"), Some("4"), Some("3"), 7, 4, 3 ; "custom")] #[tokio::test] - async fn test_create_lead_worker_provider() { - // Both API keys needed: openai for worker, anthropic for lead (GOOSE_LEAD_PROVIDER=anthropic) - let _guard = EnvVarGuard::new(&[ - "GOOSE_LEAD_MODEL", - "GOOSE_LEAD_PROVIDER", - "GOOSE_LEAD_TURNS", - "OPENAI_API_KEY", - "ANTHROPIC_API_KEY", + async fn test_create_lead_worker_provider( + lead_turns: Option<&str>, + failure_threshold: Option<&str>, + fallback_turns: Option<&str>, + expected_turns: usize, + expected_failure: usize, + expected_fallback: usize, + ) { + let _guard = env_lock::lock_env([ + ("GOOSE_LEAD_MODEL", Some("gpt-4o")), + ("GOOSE_LEAD_PROVIDER", None), + ("GOOSE_LEAD_TURNS", lead_turns), + ("GOOSE_LEAD_FAILURE_THRESHOLD", failure_threshold), + ("GOOSE_LEAD_FALLBACK_TURNS", fallback_turns), + ("OPENAI_API_KEY", Some("fake-openai-no-keyring")), ]); - _guard.set("OPENAI_API_KEY", "fake-openai-no-keyring"); - _guard.set("ANTHROPIC_API_KEY", "fake-anthropic-no-keyring"); - _guard.set("GOOSE_LEAD_MODEL", "gpt-4o"); - - let gpt4mini_config = ModelConfig::new_or_fail("gpt-4o-mini"); - let result = create("openai", gpt4mini_config.clone()).await; - - match result { - Ok(_) => {} - Err(error) => { - let error_msg = error.to_string(); - assert!(error_msg.contains("OPENAI_API_KEY") || error_msg.contains("secret")); - } - } - - _guard.set("GOOSE_LEAD_PROVIDER", "anthropic"); - _guard.set("GOOSE_LEAD_TURNS", "5"); - - let _result = create("openai", gpt4mini_config).await; - } - - #[tokio::test] - async fn test_lead_model_env_vars_with_defaults() { - let _guard = EnvVarGuard::new(&[ - "GOOSE_LEAD_MODEL", - "GOOSE_LEAD_PROVIDER", - "GOOSE_LEAD_TURNS", - "GOOSE_LEAD_FAILURE_THRESHOLD", - "GOOSE_LEAD_FALLBACK_TURNS", - "OPENAI_API_KEY", - ]); - - _guard.set("OPENAI_API_KEY", "fake-openai-no-keyring"); - _guard.set("GOOSE_LEAD_MODEL", "grok-3"); - - let result = create("openai", ModelConfig::new_or_fail("gpt-4o-mini")).await; - - match result { - Ok(_) => {} - Err(error) => { - let error_msg = error.to_string(); - assert!(error_msg.contains("OPENAI_API_KEY") || error_msg.contains("secret")); - } - } - - _guard.set("GOOSE_LEAD_TURNS", "7"); - _guard.set("GOOSE_LEAD_FAILURE_THRESHOLD", "4"); - _guard.set("GOOSE_LEAD_FALLBACK_TURNS", "3"); - - let _result = create("openai", ModelConfig::new_or_fail("gpt-4o-mini")); + let provider = create("openai", ModelConfig::new_or_fail("gpt-4o-mini")) + .await + .unwrap(); + let lw = provider.as_lead_worker().unwrap(); + let (lead, worker) = lw.get_model_info(); + assert_eq!(lead, "gpt-4o"); + assert_eq!(worker, "gpt-4o-mini"); + assert_eq!( + lw.get_settings(), + (expected_turns, expected_failure, expected_fallback) + ); } #[tokio::test] async fn test_create_regular_provider_without_lead_config() { - let _guard = EnvVarGuard::new(&[ - "GOOSE_LEAD_MODEL", - "GOOSE_LEAD_PROVIDER", - "GOOSE_LEAD_TURNS", - "GOOSE_LEAD_FAILURE_THRESHOLD", - "GOOSE_LEAD_FALLBACK_TURNS", - "OPENAI_API_KEY", + let _guard = env_lock::lock_env([ + ("GOOSE_LEAD_MODEL", None), + ("GOOSE_LEAD_PROVIDER", None), + ("GOOSE_LEAD_TURNS", None), + ("GOOSE_LEAD_FAILURE_THRESHOLD", None), + ("GOOSE_LEAD_FALLBACK_TURNS", None), + ("OPENAI_API_KEY", Some("fake-openai-no-keyring")), ]); - _guard.set("OPENAI_API_KEY", "fake-openai-no-keyring"); - let result = create("openai", ModelConfig::new_or_fail("gpt-4o-mini")).await; - - match result { - Ok(_) => {} - Err(error) => { - let error_msg = error.to_string(); - assert!(error_msg.contains("OPENAI_API_KEY") || error_msg.contains("secret")); - } - } + let provider = create("openai", ModelConfig::new_or_fail("gpt-4o-mini")) + .await + .unwrap(); + assert!(provider.as_lead_worker().is_none()); + assert_eq!(provider.get_model_config().model_name, "gpt-4o-mini"); } - #[test] - fn test_worker_model_preserves_original_context_limit() { - let _guard = EnvVarGuard::new(&[ - "GOOSE_LEAD_MODEL", - "GOOSE_WORKER_CONTEXT_LIMIT", - "GOOSE_CONTEXT_LIMIT", + #[test_case::test_case(None, None, 16_000 ; "no overrides uses default")] + #[test_case::test_case(Some("32000"), None, 32_000 ; "worker limit overrides default")] + #[test_case::test_case(Some("32000"), Some("64000"), 32_000 ; "worker limit takes priority over global")] + fn test_worker_model_context_limit( + worker_limit: Option<&str>, + global_limit: Option<&str>, + expected_limit: usize, + ) { + let _guard = env_lock::lock_env([ + ("GOOSE_WORKER_CONTEXT_LIMIT", worker_limit), + ("GOOSE_CONTEXT_LIMIT", global_limit), ]); - _guard.set("GOOSE_LEAD_MODEL", "gpt-4o"); - let default_model = ModelConfig::new_or_fail("gpt-3.5-turbo").with_context_limit(Some(16_000)); - let _result = create_lead_worker_from_env("openai", &default_model, "gpt-4o"); - - _guard.set("GOOSE_WORKER_CONTEXT_LIMIT", "32000"); - let _result = create_lead_worker_from_env("openai", &default_model, "gpt-4o"); - - _guard.set("GOOSE_CONTEXT_LIMIT", "64000"); - let _result = create_lead_worker_from_env("openai", &default_model, "gpt-4o"); + let result = create_worker_model_config(&default_model).unwrap(); + assert_eq!(result.context_limit, Some(expected_limit)); } #[tokio::test] diff --git a/crates/goose/src/providers/lead_worker.rs b/crates/goose/src/providers/lead_worker.rs index e0786b41..9cc44347 100644 --- a/crates/goose/src/providers/lead_worker.rs +++ b/crates/goose/src/providers/lead_worker.rs @@ -303,6 +303,15 @@ impl LeadWorkerProviderTrait for LeadWorkerProvider { self.lead_provider.get_model_config().model_name }) } + + /// Get (lead_turns, failure_threshold, fallback_turns) + fn get_settings(&self) -> (usize, usize, usize) { + ( + self.lead_turns, + self.max_failures_before_fallback, + self.fallback_turns, + ) + } } #[async_trait]