From a6ea5e28ba11584fde3b6599d1a5f4cb3c9aec94 Mon Sep 17 00:00:00 2001 From: Ryder Freeman Date: Mon, 9 Mar 2026 20:57:06 -0700 Subject: [PATCH] fix(config): enforce allow_edit_existing_files restriction in tool selection (#388) Wire up the existing but unused `infer_task_edit_requirement()` heuristic to the tool selection pipeline. When a prompt indicates editing intent (fix, implement, refactor, etc.), tools with `allow_edit_existing_files = false` are now filtered out during tier-based selection. Three changes: - `resolve_tool_and_model()` accepts `needs_edit` and forwards it to `resolve_tier_tool_rotated()` (was hardcoded `false`) - New `resolve_tier_tool_filtered()` replaces the unfiltered fallback, `resolve_tier_tool()` now delegates to it with `needs_edit=false` - `run_cmd_execute` calls `infer_task_edit_requirement()` and passes the result through the strategy resolution chain Closes #388 Co-Authored-By: Claude Opus 4.6 --- Cargo.lock | 30 ++--- Cargo.toml | 2 +- .../cli-sub-agent/src/claude_sub_agent_cmd.rs | 1 + crates/cli-sub-agent/src/mcp_server.rs | 1 + crates/cli-sub-agent/src/run_cmd_execute.rs | 2 + .../src/run_cmd_tool_selection.rs | 14 +++ crates/cli-sub-agent/src/run_helpers.rs | 11 +- crates/cli-sub-agent/src/run_helpers_tests.rs | 7 +- crates/csa-config/src/config.rs | 30 +++-- crates/csa-config/src/config_tests_tier.rs | 115 ++++++++++++++++++ crates/csa-scheduler/src/rotation.rs | 100 +++++++++++++++ 11 files changed, 281 insertions(+), 32 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index be7ae819..9ec3c232 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -451,7 +451,7 @@ checksum = "c3e64b0cc0439b12df2fa678eae89a1c56a529fd067a9115f7827f1fffd22b32" [[package]] name = "cli-sub-agent" -version = "0.1.108" +version = "0.1.109" dependencies = [ "anyhow", "chrono", @@ -610,7 +610,7 @@ dependencies = [ [[package]] name = "csa-acp" -version = "0.1.108" +version = "0.1.109" dependencies = [ "agent-client-protocol", "anyhow", @@ -629,7 +629,7 @@ dependencies = [ [[package]] name = "csa-config" -version = "0.1.108" +version = "0.1.109" dependencies = [ "anyhow", "chrono", @@ -645,7 +645,7 @@ dependencies = [ [[package]] name = "csa-core" -version = "0.1.108" +version = "0.1.109" dependencies = [ "agent-teams", "chrono", @@ -659,7 +659,7 @@ dependencies = [ [[package]] name = "csa-executor" -version = "0.1.108" +version = "0.1.109" dependencies = [ "agent-teams", "anyhow", @@ -683,7 +683,7 @@ dependencies = [ [[package]] name = "csa-hooks" -version = "0.1.108" +version = "0.1.109" dependencies = [ "anyhow", "chrono", @@ -698,7 +698,7 @@ dependencies = [ [[package]] name = "csa-lock" -version = "0.1.108" +version = "0.1.109" dependencies = [ "anyhow", "chrono", @@ -710,7 +710,7 @@ dependencies = [ [[package]] name = "csa-mcp-hub" -version = "0.1.108" +version = "0.1.109" dependencies = [ "anyhow", "axum", @@ -732,7 +732,7 @@ dependencies = [ [[package]] name = "csa-memory" -version = "0.1.108" +version = "0.1.109" dependencies = [ "anyhow", "async-trait", @@ -750,7 +750,7 @@ dependencies = [ [[package]] name = "csa-process" -version = "0.1.108" +version = "0.1.109" dependencies = [ "anyhow", "csa-core", @@ -767,7 +767,7 @@ dependencies = [ [[package]] name = "csa-resource" -version = "0.1.108" +version = "0.1.109" dependencies = [ "anyhow", "csa-core", @@ -782,7 +782,7 @@ dependencies = [ [[package]] name = "csa-scheduler" -version = "0.1.108" +version = "0.1.109" dependencies = [ "anyhow", "chrono", @@ -800,7 +800,7 @@ dependencies = [ [[package]] name = "csa-session" -version = "0.1.108" +version = "0.1.109" dependencies = [ "anyhow", "chrono", @@ -821,7 +821,7 @@ dependencies = [ [[package]] name = "csa-todo" -version = "0.1.108" +version = "0.1.109" dependencies = [ "anyhow", "chrono", @@ -4089,7 +4089,7 @@ dependencies = [ [[package]] name = "weave" -version = "0.1.108" +version = "0.1.109" dependencies = [ "anyhow", "clap", diff --git a/Cargo.toml b/Cargo.toml index 92cc24bf..c9b35d64 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,7 +3,7 @@ members = ["crates/*"] resolver = "2" [workspace.package] -version = "0.1.108" +version = "0.1.109" edition = "2024" rust-version = "1.85" license = "Apache-2.0" diff --git a/crates/cli-sub-agent/src/claude_sub_agent_cmd.rs b/crates/cli-sub-agent/src/claude_sub_agent_cmd.rs index e5739148..00baf306 100644 --- a/crates/cli-sub-agent/src/claude_sub_agent_cmd.rs +++ b/crates/cli-sub-agent/src/claude_sub_agent_cmd.rs @@ -43,6 +43,7 @@ pub(crate) async fn handle_claude_sub_agent( &project_root, false, // claude-sub-agent does not support --force false, // claude-sub-agent does not support --force-override-user-config + false, // sub-agent dispatch always uses explicit tool )?; // 8. Build executor and validate tool diff --git a/crates/cli-sub-agent/src/mcp_server.rs b/crates/cli-sub-agent/src/mcp_server.rs index e9be20e9..d256b8fa 100644 --- a/crates/cli-sub-agent/src/mcp_server.rs +++ b/crates/cli-sub-agent/src/mcp_server.rs @@ -488,6 +488,7 @@ async fn handle_run_tool(args: Value) -> Result { &project_root, false, // MCP server does not support --force false, // MCP server does not support --force-override-user-config + false, // MCP tool dispatch always uses explicit tool )?; // Build executor diff --git a/crates/cli-sub-agent/src/run_cmd_execute.rs b/crates/cli-sub-agent/src/run_cmd_execute.rs index 199983b5..d4be95c4 100644 --- a/crates/cli-sub-agent/src/run_cmd_execute.rs +++ b/crates/cli-sub-agent/src/run_cmd_execute.rs @@ -172,6 +172,7 @@ pub(crate) async fn handle_run( }; let run_timeout_seconds = resolve_run_timeout_seconds(timeout, skill.as_deref()); let run_started_at = Instant::now(); + let needs_edit = crate::run_helpers::infer_task_edit_requirement(&prompt_text).unwrap_or(false); let strategy_result = resolve_tool_by_strategy( &strategy, model_spec.as_deref(), @@ -181,6 +182,7 @@ pub(crate) async fn handle_run( &project_root, force, force_override_user_config, + needs_edit, )?; let heterogeneous_runtime_fallback_candidates = strategy_result.runtime_fallback_candidates; let resolved_model_spec = strategy_result.model_spec; diff --git a/crates/cli-sub-agent/src/run_cmd_tool_selection.rs b/crates/cli-sub-agent/src/run_cmd_tool_selection.rs index 35343981..61ba4117 100644 --- a/crates/cli-sub-agent/src/run_cmd_tool_selection.rs +++ b/crates/cli-sub-agent/src/run_cmd_tool_selection.rs @@ -133,6 +133,7 @@ pub(crate) fn resolve_tool_by_strategy( project_root: &Path, force: bool, force_override_user_config: bool, + needs_edit: bool, ) -> Result { match strategy { ToolSelectionStrategy::Explicit(t) => { @@ -144,6 +145,7 @@ pub(crate) fn resolve_tool_by_strategy( project_root, force, force_override_user_config, + needs_edit, )?; Ok(StrategyResolution { tool, @@ -161,6 +163,7 @@ pub(crate) fn resolve_tool_by_strategy( project_root, force, force_override_user_config, + needs_edit, )?; Ok(StrategyResolution { tool, @@ -177,6 +180,7 @@ pub(crate) fn resolve_tool_by_strategy( project_root, force, force_override_user_config, + needs_edit, ), ToolSelectionStrategy::HeterogeneousStrict => { let res = resolve_heterogeneous_strict( @@ -187,6 +191,7 @@ pub(crate) fn resolve_tool_by_strategy( project_root, force, force_override_user_config, + needs_edit, )?; Ok(StrategyResolution { tool: res.0, @@ -216,6 +221,7 @@ fn collect_enabled_tools( } } +#[allow(clippy::too_many_arguments)] fn resolve_heterogeneous_preferred( model_spec: Option<&str>, model: Option<&str>, @@ -224,6 +230,7 @@ fn resolve_heterogeneous_preferred( project_root: &Path, force: bool, force_override_user_config: bool, + needs_edit: bool, ) -> Result { let detected_parent_tool = detect_parent_tool(); let parent_tool_name = resolve_tool(detected_parent_tool, global_config); @@ -245,6 +252,7 @@ fn resolve_heterogeneous_preferred( project_root, force, force_override_user_config, + needs_edit, )?; Ok(StrategyResolution { tool: t, @@ -267,6 +275,7 @@ fn resolve_heterogeneous_preferred( project_root, force, force_override_user_config, + needs_edit, )?; Ok(StrategyResolution { tool: t, @@ -288,6 +297,7 @@ fn resolve_heterogeneous_preferred( project_root, force, force_override_user_config, + needs_edit, )?; Ok(StrategyResolution { tool: t, @@ -298,6 +308,7 @@ fn resolve_heterogeneous_preferred( } } +#[allow(clippy::too_many_arguments)] fn resolve_heterogeneous_strict( model_spec: Option<&str>, model: Option<&str>, @@ -306,6 +317,7 @@ fn resolve_heterogeneous_strict( project_root: &Path, force: bool, force_override_user_config: bool, + needs_edit: bool, ) -> Result<(ToolName, Option, Option)> { let detected_parent_tool = detect_parent_tool(); let parent_tool_name = resolve_tool(detected_parent_tool, global_config); @@ -323,6 +335,7 @@ fn resolve_heterogeneous_strict( project_root, force, force_override_user_config, + needs_edit, ), None => { anyhow::bail!( @@ -346,6 +359,7 @@ fn resolve_heterogeneous_strict( project_root, force, force_override_user_config, + needs_edit, ) } } diff --git a/crates/cli-sub-agent/src/run_helpers.rs b/crates/cli-sub-agent/src/run_helpers.rs index fc22c8a7..cdc20dcb 100644 --- a/crates/cli-sub-agent/src/run_helpers.rs +++ b/crates/cli-sub-agent/src/run_helpers.rs @@ -17,6 +17,8 @@ use csa_session::TokenUsage; /// - model: optional model string (from CLI, with alias resolution applied) /// /// When tool is None, uses tier-based round-robin selection. +/// `needs_edit`: when true, filters out tools with `allow_edit_existing_files = false`. +#[allow(clippy::too_many_arguments)] pub(crate) fn resolve_tool_and_model( tool: Option, model_spec: Option<&str>, @@ -25,6 +27,7 @@ pub(crate) fn resolve_tool_and_model( project_root: &Path, force: bool, force_override_user_config: bool, + needs_edit: bool, ) -> Result<(ToolName, Option, Option)> { // Case 1: model_spec provided → parse it to get tool if let Some(spec) = model_spec { @@ -84,13 +87,15 @@ pub(crate) fn resolve_tool_and_model( if let Some(cfg) = config { // Try round-robin rotation first (needs project root to persist state) if let Ok(Some((tool_name_str, tier_model_spec))) = - csa_scheduler::resolve_tier_tool_rotated(cfg, "default", project_root, false) + csa_scheduler::resolve_tier_tool_rotated(cfg, "default", project_root, needs_edit) { let tool_name = parse_tool_name(&tool_name_str)?; return Ok((tool_name, Some(tier_model_spec), None)); } - // Fallback: original non-rotating selection - if let Some((tool_name_str, tier_model_spec)) = cfg.resolve_tier_tool("default") { + // Fallback: original non-rotating selection (also respects edit restrictions) + if let Some((tool_name_str, tier_model_spec)) = + cfg.resolve_tier_tool_filtered("default", needs_edit) + { let tool_name = parse_tool_name(&tool_name_str)?; return Ok((tool_name, Some(tier_model_spec), None)); } diff --git a/crates/cli-sub-agent/src/run_helpers_tests.rs b/crates/cli-sub-agent/src/run_helpers_tests.rs index 24d535e5..522ba644 100644 --- a/crates/cli-sub-agent/src/run_helpers_tests.rs +++ b/crates/cli-sub-agent/src/run_helpers_tests.rs @@ -549,6 +549,7 @@ fn resolve_tool_and_model_disabled_tool_explicit_errors() { std::path::Path::new("/tmp"), true, // force tier bypass false, // no override + false, // needs_edit ); assert!(result.is_err()); let msg = result.unwrap_err().to_string(); @@ -594,8 +595,9 @@ fn resolve_tool_and_model_disabled_tool_with_override_succeeds() { None, Some(&config), std::path::Path::new("/tmp"), - true, // force tier bypass - true, // override enabled + true, // force tier bypass + true, // override enabled + false, // needs_edit ); assert!(result.is_ok()); let (tool, _, _) = result.unwrap(); @@ -643,6 +645,7 @@ fn resolve_tool_and_model_disabled_tool_model_spec_errors() { std::path::Path::new("/tmp"), true, false, + false, // needs_edit ); assert!(result.is_err()); let msg = result.unwrap_err().to_string(); diff --git a/crates/csa-config/src/config.rs b/crates/csa-config/src/config.rs index beae0cd1..7f560923 100644 --- a/crates/csa-config/src/config.rs +++ b/crates/csa-config/src/config.rs @@ -636,13 +636,23 @@ impl ProjectConfig { /// Falls back to tier3 if task_type not found in tier_mapping. /// Returns None if no enabled tools found. pub fn resolve_tier_tool(&self, task_type: &str) -> Option<(String, String)> { - // 1. Look up task_type in tier_mapping to get tier name + self.resolve_tier_tool_filtered(task_type, false) + } + + /// Resolve tier-based tool selection with edit restriction filtering. + /// + /// When `needs_edit` is true, skips tools whose + /// `restrictions.allow_edit_existing_files` is `false`. + pub fn resolve_tier_tool_filtered( + &self, + task_type: &str, + needs_edit: bool, + ) -> Option<(String, String)> { let tier_name = self .tier_mapping .get(task_type) .map(String::as_str) .or_else(|| { - // Fallback: try to find tier3 or tier-3-* if self.tiers.contains_key("tier3") { Some("tier3") } else { @@ -653,23 +663,21 @@ impl ProjectConfig { } })?; - // 2. Find that tier in tiers map let tier = self.tiers.get(tier_name)?; - // 3. Iterate through tier's models (format: tool/provider/model/thinking_budget) for model_spec_str in &tier.models { - // Parse model spec to extract tool name let parts: Vec<&str> = model_spec_str.splitn(4, '/').collect(); if parts.len() != 4 { - continue; // Invalid format, skip + continue; } - let tool_name = parts[0]; - - // 4. Check if this tool is enabled - if self.is_tool_enabled(tool_name) { - return Some((tool_name.to_string(), model_spec_str.clone())); + if !self.is_tool_enabled(tool_name) { + continue; + } + if needs_edit && !self.can_tool_edit_existing(tool_name) { + continue; } + return Some((tool_name.to_string(), model_spec_str.clone())); } None diff --git a/crates/csa-config/src/config_tests_tier.rs b/crates/csa-config/src/config_tests_tier.rs index 931d6123..96905836 100644 --- a/crates/csa-config/src/config_tests_tier.rs +++ b/crates/csa-config/src/config_tests_tier.rs @@ -385,3 +385,118 @@ fn enabled_tier_models_returns_empty_when_all_tools_disabled() { assert!(config.enabled_tier_models("tier-1").is_empty()); } + +// ── resolve_tier_tool_filtered tests ─────────────────────────────── + +#[test] +fn filtered_skips_restricted_tool_when_needs_edit() { + let mut tools = HashMap::new(); + tools.insert( + "gemini-cli".to_string(), + ToolConfig { + restrictions: Some(ToolRestrictions { + allow_edit_existing_files: false, + }), + ..Default::default() + }, + ); + tools.insert("codex".to_string(), ToolConfig::default()); + + let mut tiers = HashMap::new(); + tiers.insert( + "tier3".to_string(), + TierConfig { + description: "test".to_string(), + models: vec![ + "gemini-cli/google/gemini-2.5-pro/xhigh".to_string(), + "codex/openai/o4-mini/0".to_string(), + ], + token_budget: None, + max_turns: None, + }, + ); + + let mut tier_mapping = HashMap::new(); + tier_mapping.insert("default".to_string(), "tier3".to_string()); + + let config = ProjectConfig { + schema_version: CURRENT_SCHEMA_VERSION, + project: ProjectMeta::default(), + resources: ResourcesConfig::default(), + acp: Default::default(), + tools, + review: None, + debate: None, + tiers, + tier_mapping, + aliases: HashMap::new(), + tool_aliases: HashMap::new(), + preferences: None, + session: Default::default(), + memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), + }; + + // needs_edit=true → should skip gemini-cli, select codex + let result = config.resolve_tier_tool_filtered("default", true); + assert!(result.is_some()); + let (tool, _) = result.unwrap(); + assert_eq!(tool, "codex"); + + // needs_edit=false → should select gemini-cli (first enabled) + let result = config.resolve_tier_tool_filtered("default", false); + assert!(result.is_some()); + let (tool, _) = result.unwrap(); + assert_eq!(tool, "gemini-cli"); +} + +#[test] +fn filtered_returns_none_when_all_restricted_and_needs_edit() { + let mut tools = HashMap::new(); + tools.insert( + "gemini-cli".to_string(), + ToolConfig { + restrictions: Some(ToolRestrictions { + allow_edit_existing_files: false, + }), + ..Default::default() + }, + ); + + let mut tiers = HashMap::new(); + tiers.insert( + "tier3".to_string(), + TierConfig { + description: "test".to_string(), + models: vec!["gemini-cli/google/gemini-2.5-pro/xhigh".to_string()], + token_budget: None, + max_turns: None, + }, + ); + + let mut tier_mapping = HashMap::new(); + tier_mapping.insert("default".to_string(), "tier3".to_string()); + + let config = ProjectConfig { + schema_version: CURRENT_SCHEMA_VERSION, + project: ProjectMeta::default(), + resources: ResourcesConfig::default(), + acp: Default::default(), + tools, + review: None, + debate: None, + tiers, + tier_mapping, + aliases: HashMap::new(), + tool_aliases: HashMap::new(), + preferences: None, + session: Default::default(), + memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), + }; + + let result = config.resolve_tier_tool_filtered("default", true); + assert!(result.is_none()); +} diff --git a/crates/csa-scheduler/src/rotation.rs b/crates/csa-scheduler/src/rotation.rs index 173d91d7..94c61a21 100644 --- a/crates/csa-scheduler/src/rotation.rs +++ b/crates/csa-scheduler/src/rotation.rs @@ -602,4 +602,104 @@ mod tests { let state = RotationState::default(); assert!(state.tiers.is_empty()); } + + fn make_config_with_restrictions( + models: Vec<&str>, + restricted_tools: Vec<&str>, + ) -> ProjectConfig { + use csa_config::ToolRestrictions; + + let mut tools = HashMap::new(); + for tool in restricted_tools { + tools.insert( + tool.to_string(), + ToolConfig { + restrictions: Some(ToolRestrictions { + allow_edit_existing_files: false, + }), + ..Default::default() + }, + ); + } + + let mut tiers = HashMap::new(); + tiers.insert( + "tier3".to_string(), + TierConfig { + description: "test tier".to_string(), + models: models.iter().map(|s| s.to_string()).collect(), + token_budget: None, + max_turns: None, + }, + ); + + let mut tier_mapping = HashMap::new(); + tier_mapping.insert("default".to_string(), "tier3".to_string()); + + ProjectConfig { + schema_version: 1, + project: ProjectMeta { + name: "test".to_string(), + created_at: Utc::now(), + max_recursion_depth: 5, + }, + resources: Default::default(), + acp: Default::default(), + tools, + review: None, + debate: None, + tiers, + tier_mapping, + aliases: HashMap::new(), + tool_aliases: HashMap::new(), + preferences: None, + session: Default::default(), + memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), + } + } + + #[test] + fn test_rotated_skips_restricted_tool_when_needs_edit() { + let temp = tempdir().unwrap(); + let config = make_config_with_restrictions( + vec![ + "gemini-cli/google/gemini-2.5-pro/0", + "codex/openai/o4-mini/0", + ], + vec!["gemini-cli"], + ); + + // needs_edit=true → skip gemini-cli (restricted), pick codex + let result = resolve_tier_tool_rotated(&config, "default", temp.path(), true) + .unwrap() + .unwrap(); + assert_eq!(result.0, "codex", "Should skip restricted gemini-cli"); + + // needs_edit=false → normal rotation (picks gemini-cli) + let temp2 = tempdir().unwrap(); + let result = resolve_tier_tool_rotated(&config, "default", temp2.path(), false) + .unwrap() + .unwrap(); + assert_eq!( + result.0, "codex", + "Without restriction, normal round-robin from index 1" + ); + } + + #[test] + fn test_rotated_returns_none_when_all_restricted_and_needs_edit() { + let temp = tempdir().unwrap(); + let config = make_config_with_restrictions( + vec!["gemini-cli/google/gemini-2.5-pro/0"], + vec!["gemini-cli"], + ); + + let result = resolve_tier_tool_rotated(&config, "default", temp.path(), true).unwrap(); + assert!( + result.is_none(), + "Should return None when only tool is restricted and needs_edit" + ); + } }