diff --git a/src/approval/mod.rs b/src/approval/mod.rs index bffae728a..6a2056648 100644 --- a/src/approval/mod.rs +++ b/src/approval/mod.rs @@ -562,4 +562,50 @@ mod tests { let parsed: ApprovalRequest = serde_json::from_str(&json).unwrap(); assert_eq!(parsed.tool_name, "shell"); } + + // ── Regression: #4247 default approved tools in channels ── + + #[test] + fn non_interactive_allows_default_auto_approve_tools() { + let config = AutonomyConfig::default(); + let mgr = ApprovalManager::for_non_interactive(&config); + + for tool in &config.auto_approve { + assert!( + !mgr.needs_approval(tool), + "default auto_approve tool '{tool}' should not need approval in non-interactive mode" + ); + } + } + + #[test] + fn non_interactive_denies_unknown_tools() { + let config = AutonomyConfig::default(); + let mgr = ApprovalManager::for_non_interactive(&config); + assert!( + mgr.needs_approval("some_unknown_tool"), + "unknown tool should need approval" + ); + } + + #[test] + fn non_interactive_weather_is_auto_approved() { + let config = AutonomyConfig::default(); + let mgr = ApprovalManager::for_non_interactive(&config); + assert!( + !mgr.needs_approval("weather"), + "weather tool must not need approval — it is in the default auto_approve list" + ); + } + + #[test] + fn always_ask_overrides_auto_approve() { + let mut config = AutonomyConfig::default(); + config.always_ask = vec!["weather".into()]; + let mgr = ApprovalManager::for_non_interactive(&config); + assert!( + mgr.needs_approval("weather"), + "always_ask must override auto_approve" + ); + } } diff --git a/src/config/schema.rs b/src/config/schema.rs index da2b67637..207b95ea4 100644 --- a/src/config/schema.rs +++ b/src/config/schema.rs @@ -4267,6 +4267,19 @@ fn default_always_ask() -> Vec { vec![] } +impl AutonomyConfig { + /// Merge the built-in default `auto_approve` entries into the current + /// list, preserving any user-supplied additions. + pub fn ensure_default_auto_approve(&mut self) { + let defaults = default_auto_approve(); + for entry in defaults { + if !self.auto_approve.iter().any(|existing| existing == &entry) { + self.auto_approve.push(entry); + } + } + } +} + fn is_valid_env_var_name(name: &str) -> bool { let mut chars = name.chars(); match chars.next() { @@ -7544,6 +7557,19 @@ impl Config { let mut config: Config = toml::from_str(&contents).context("Failed to deserialize config file")?; + // Ensure the built-in default auto_approve entries are always + // present. When a user specifies `auto_approve` in their TOML + // (e.g. to add a custom tool), serde replaces the default list + // instead of merging. This caused default-safe tools like + // `weather` or `calculator` to lose their auto-approve status + // and get silently denied in non-interactive channel runs. + // See #4247. + // + // Users who want to require approval for a default tool can + // add it to `always_ask`, which takes precedence over + // `auto_approve` in the approval decision (see approval/mod.rs). + config.autonomy.ensure_default_auto_approve(); + // Detect unknown/ignored config keys for diagnostic warnings. // This second pass uses serde_ignored but discards the parsed // result — only the ignored-path list is kept. @@ -9560,7 +9586,9 @@ mod tests { merged.push(']'); } merged.push('\n'); - toml::from_str(&merged).unwrap() + let mut config: Config = toml::from_str(&merged).unwrap(); + config.autonomy.ensure_default_auto_approve(); + config } #[test] @@ -10046,6 +10074,109 @@ auto_approve = ["file_read", "memory_recall", "http_request"] ); } + /// Regression test for #4247: when a user provides a custom auto_approve + /// list, the built-in defaults must still be present. + #[test] + async fn auto_approve_merges_user_entries_with_defaults() { + let raw = r#" +default_temperature = 0.7 + +[autonomy] +auto_approve = ["my_custom_tool", "another_tool"] +"#; + let parsed = parse_test_config(raw); + // User entries are preserved + assert!( + parsed + .autonomy + .auto_approve + .contains(&"my_custom_tool".to_string()), + "user-supplied tool must remain in auto_approve" + ); + assert!( + parsed + .autonomy + .auto_approve + .contains(&"another_tool".to_string()), + "user-supplied tool must remain in auto_approve" + ); + // Defaults are merged in + for default_tool in &[ + "file_read", + "memory_recall", + "weather", + "calculator", + "web_fetch", + ] { + assert!( + parsed.autonomy.auto_approve.contains(&default_tool.to_string()), + "default tool '{default_tool}' must be present in auto_approve even when user provides custom list" + ); + } + } + + /// Regression test: empty auto_approve still gets defaults merged. + #[test] + async fn auto_approve_empty_list_gets_defaults() { + let raw = r#" +default_temperature = 0.7 + +[autonomy] +auto_approve = [] +"#; + let parsed = parse_test_config(raw); + let defaults = default_auto_approve(); + for tool in &defaults { + assert!( + parsed.autonomy.auto_approve.contains(tool), + "default tool '{tool}' must be present even when user sets auto_approve = []" + ); + } + } + + /// When no autonomy section is provided, defaults are applied normally. + #[test] + async fn auto_approve_defaults_when_no_autonomy_section() { + let raw = r#" +default_temperature = 0.7 +"#; + let parsed = parse_test_config(raw); + let defaults = default_auto_approve(); + for tool in &defaults { + assert!( + parsed.autonomy.auto_approve.contains(tool), + "default tool '{tool}' must be present when no [autonomy] section" + ); + } + } + + /// Duplicates are not introduced when ensure_default_auto_approve runs + /// on a list that already contains the defaults. + #[test] + async fn auto_approve_no_duplicates() { + let raw = r#" +default_temperature = 0.7 + +[autonomy] +auto_approve = ["weather", "file_read"] +"#; + let parsed = parse_test_config(raw); + let weather_count = parsed + .autonomy + .auto_approve + .iter() + .filter(|t| *t == "weather") + .count(); + assert_eq!(weather_count, 1, "weather must not be duplicated"); + let file_read_count = parsed + .autonomy + .auto_approve + .iter() + .filter(|t| *t == "file_read") + .count(); + assert_eq!(file_read_count, 1, "file_read must not be duplicated"); + } + #[test] async fn provider_timeout_secs_parses_from_toml() { let raw = r#"