diff --git a/Cargo.lock b/Cargo.lock index 9154a20d..be7ae819 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -451,7 +451,7 @@ checksum = "c3e64b0cc0439b12df2fa678eae89a1c56a529fd067a9115f7827f1fffd22b32" [[package]] name = "cli-sub-agent" -version = "0.1.107" +version = "0.1.108" dependencies = [ "anyhow", "chrono", @@ -610,7 +610,7 @@ dependencies = [ [[package]] name = "csa-acp" -version = "0.1.107" +version = "0.1.108" dependencies = [ "agent-client-protocol", "anyhow", @@ -629,7 +629,7 @@ dependencies = [ [[package]] name = "csa-config" -version = "0.1.107" +version = "0.1.108" dependencies = [ "anyhow", "chrono", @@ -645,7 +645,7 @@ dependencies = [ [[package]] name = "csa-core" -version = "0.1.107" +version = "0.1.108" dependencies = [ "agent-teams", "chrono", @@ -659,7 +659,7 @@ dependencies = [ [[package]] name = "csa-executor" -version = "0.1.107" +version = "0.1.108" dependencies = [ "agent-teams", "anyhow", @@ -683,7 +683,7 @@ dependencies = [ [[package]] name = "csa-hooks" -version = "0.1.107" +version = "0.1.108" dependencies = [ "anyhow", "chrono", @@ -698,7 +698,7 @@ dependencies = [ [[package]] name = "csa-lock" -version = "0.1.107" +version = "0.1.108" dependencies = [ "anyhow", "chrono", @@ -710,7 +710,7 @@ dependencies = [ [[package]] name = "csa-mcp-hub" -version = "0.1.107" +version = "0.1.108" dependencies = [ "anyhow", "axum", @@ -732,7 +732,7 @@ dependencies = [ [[package]] name = "csa-memory" -version = "0.1.107" +version = "0.1.108" dependencies = [ "anyhow", "async-trait", @@ -750,7 +750,7 @@ dependencies = [ [[package]] name = "csa-process" -version = "0.1.107" +version = "0.1.108" dependencies = [ "anyhow", "csa-core", @@ -767,7 +767,7 @@ dependencies = [ [[package]] name = "csa-resource" -version = "0.1.107" +version = "0.1.108" dependencies = [ "anyhow", "csa-core", @@ -782,7 +782,7 @@ dependencies = [ [[package]] name = "csa-scheduler" -version = "0.1.107" +version = "0.1.108" dependencies = [ "anyhow", "chrono", @@ -800,7 +800,7 @@ dependencies = [ [[package]] name = "csa-session" -version = "0.1.107" +version = "0.1.108" dependencies = [ "anyhow", "chrono", @@ -821,7 +821,7 @@ dependencies = [ [[package]] name = "csa-todo" -version = "0.1.107" +version = "0.1.108" dependencies = [ "anyhow", "chrono", @@ -4089,7 +4089,7 @@ dependencies = [ [[package]] name = "weave" -version = "0.1.107" +version = "0.1.108" dependencies = [ "anyhow", "clap", diff --git a/Cargo.toml b/Cargo.toml index cdc406f3..92cc24bf 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,7 +3,7 @@ members = ["crates/*"] resolver = "2" [workspace.package] -version = "0.1.107" +version = "0.1.108" edition = "2024" rust-version = "1.85" license = "Apache-2.0" diff --git a/crates/cli-sub-agent/src/debate_cmd.rs b/crates/cli-sub-agent/src/debate_cmd.rs index 5f1b5c35..64d20b03 100644 --- a/crates/cli-sub-agent/src/debate_cmd.rs +++ b/crates/cli-sub-agent/src/debate_cmd.rs @@ -131,6 +131,16 @@ pub(crate) async fn handle_debate( Cognitive diversity is degraded." ); } + // Model precedence: CLI --model > project config debate.model > global config debate.model. + // When tier is also set, build_executor applies model override after tier spec construction. + let debate_model = args.model.clone().or_else(|| { + config + .as_ref() + .and_then(|c| c.debate.as_ref()) + .and_then(|d| d.model.clone()) + .or_else(|| global_config.debate.model.clone()) + }); + // Thinking precedence: CLI > config debate.thinking > tier model_spec thinking. let thinking = resolve_debate_thinking( args.thinking.as_deref(), @@ -145,7 +155,7 @@ pub(crate) async fn handle_debate( let executor = crate::pipeline::build_and_validate_executor( &tool, tier_model_spec.as_deref(), - args.model.as_deref(), + debate_model.as_deref(), thinking.as_deref(), crate::pipeline::ConfigRefs { project: config.as_ref(), diff --git a/crates/cli-sub-agent/src/review_cmd.rs b/crates/cli-sub-agent/src/review_cmd.rs index c945355e..a357df01 100644 --- a/crates/cli-sub-agent/src/review_cmd.rs +++ b/crates/cli-sub-agent/src/review_cmd.rs @@ -165,6 +165,16 @@ pub(crate) async fn handle_review(args: ReviewArgs, current_depth: u32) -> Resul args.force_override_user_config, )?; + // Resolve model: CLI --model > project config review.model > global config review.model. + // When tier is also set, build_executor applies model override after tier spec construction. + let review_model = args.model.clone().or_else(|| { + config + .as_ref() + .and_then(|c| c.review.as_ref()) + .and_then(|r| r.model.clone()) + .or_else(|| global_config.review.model.clone()) + }); + // Resolve thinking: CLI > config review.thinking > tier model_spec thinking. // Tier thinking is embedded in model_spec and applied via build_and_validate_executor. let review_thinking = resolve_review_thinking( @@ -187,7 +197,7 @@ pub(crate) async fn handle_review(args: ReviewArgs, current_depth: u32) -> Resul tool, prompt.clone(), args.session.clone(), - args.model.clone(), + review_model.clone(), tier_model_spec.clone(), review_thinking.clone(), format!( @@ -251,7 +261,7 @@ pub(crate) async fn handle_review(args: ReviewArgs, current_depth: u32) -> Resul tool, fix_prompt, Some(session_id.clone()), - args.model.clone(), + review_model.clone(), tier_model_spec.clone(), review_thinking.clone(), format!("fix round {round}/{max_rounds}"), @@ -349,7 +359,7 @@ pub(crate) async fn handle_review(args: ReviewArgs, current_depth: u32) -> Resul for (reviewer_index, reviewer_tool) in reviewer_tools.into_iter().enumerate() { let reviewer_prompt = build_multi_reviewer_instruction(&prompt, reviewer_index + 1, reviewer_tool); - let reviewer_model = args.model.clone(); + let reviewer_model = review_model.clone(); let reviewer_project_root = project_root.clone(); let reviewer_config = config.clone(); let reviewer_global = global_config.clone(); diff --git a/crates/cli-sub-agent/src/run_helpers.rs b/crates/cli-sub-agent/src/run_helpers.rs index 4a25b8dc..fc22c8a7 100644 --- a/crates/cli-sub-agent/src/run_helpers.rs +++ b/crates/cli-sub-agent/src/run_helpers.rs @@ -148,9 +148,12 @@ pub(crate) fn build_executor( Executor::from_tool_name(tool, final_model, budget) }; - // When model_spec is present, the thinking budget comes from the spec. - // An explicit `thinking` argument must override it (CLI > tier spec). + // When model_spec is present, the model and thinking come from the spec. + // Explicit arguments must override them (CLI/config > tier spec). if model_spec.is_some() { + if let Some(explicit_model) = model { + executor.override_model(explicit_model.to_string()); + } if let Some(explicit_thinking) = thinking { let budget = ThinkingBudget::parse(explicit_thinking)?; executor.override_thinking_budget(budget); diff --git a/crates/cli-sub-agent/src/run_helpers_tests.rs b/crates/cli-sub-agent/src/run_helpers_tests.rs index fdb97da7..24d535e5 100644 --- a/crates/cli-sub-agent/src/run_helpers_tests.rs +++ b/crates/cli-sub-agent/src/run_helpers_tests.rs @@ -456,12 +456,12 @@ fn build_executor_model_spec_overrides_both() { execution: Default::default(), }; - // When explicit thinking is provided alongside model_spec, it overrides - // the spec's embedded thinking budget (CLI > tier spec). + // When explicit model and thinking are provided alongside model_spec, + // they override the spec's embedded values (CLI/config > tier spec). let exec = build_executor( &ToolName::Codex, Some("codex/openai/gpt-5.3-codex/xhigh"), - Some("ignored-model"), + Some("explicit-model"), Some("high"), Some(&config), true, @@ -469,8 +469,12 @@ fn build_executor_model_spec_overrides_both() { .unwrap(); let debug = format!("{:?}", exec); assert!( - debug.contains("gpt-5.3-codex"), - "model_spec model missing: {debug}" + debug.contains("explicit-model"), + "explicit model should override model_spec model: {debug}" + ); + assert!( + !debug.contains("gpt-5.3-codex"), + "model_spec model should be overridden by explicit model: {debug}" ); assert!( debug.contains("High"), diff --git a/crates/csa-config/src/global.rs b/crates/csa-config/src/global.rs index 2bac9064..29755807 100644 --- a/crates/csa-config/src/global.rs +++ b/crates/csa-config/src/global.rs @@ -112,6 +112,12 @@ pub struct ReviewConfig { /// over `tool` when both are set. The tier must exist in `[tiers]`. #[serde(default, skip_serializing_if = "Option::is_none")] pub tier: Option, + /// Default model for `csa review`. Overrides the tool's own default model + /// selection (e.g., gemini-cli model steering) without requiring a tier. + /// + /// Priority: CLI `--model` > this field > tier model_spec > tool default. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub model: Option, /// Default thinking budget for `csa review` (`low`, `medium`, `high`, `xhigh`). /// /// `csa review --thinking ` (when supported) overrides this. @@ -151,6 +157,7 @@ impl Default for ReviewConfig { tool: default_review_tool(), gate_mode: GateMode::default(), tier: None, + model: None, thinking: None, gate_command: None, gate_timeout_secs: default_gate_timeout_secs(), @@ -164,6 +171,7 @@ impl ReviewConfig { self.tool == default_review_tool() && self.gate_mode == GateMode::Monitor && self.tier.is_none() + && self.model.is_none() && self.thinking.is_none() && self.gate_command.is_none() && self.gate_timeout_secs == default_gate_timeout_secs() @@ -191,6 +199,12 @@ pub struct DebateConfig { /// `csa debate --timeout ` overrides this per invocation. #[serde(default = "default_debate_timeout_seconds")] pub timeout_seconds: u64, + /// Default model for `csa debate`. Overrides the tool's own default model + /// selection (e.g., gemini-cli model steering) without requiring a tier. + /// + /// Priority: CLI `--model` > this field > tier model_spec > tool default. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub model: Option, /// Default thinking budget for `csa debate` (`low`, `medium`, `high`, `xhigh`). /// /// `csa debate --thinking ` overrides this per invocation. @@ -230,6 +244,7 @@ impl Default for DebateConfig { Self { tool: default_debate_tool(), timeout_seconds: default_debate_timeout_seconds(), + model: None, thinking: None, same_model_fallback: true, tier: None, @@ -237,6 +252,18 @@ impl Default for DebateConfig { } } +impl DebateConfig { + /// Returns true when all fields match defaults (per rust/016 serde-default rule). + pub fn is_default(&self) -> bool { + self.tool == default_debate_tool() + && self.timeout_seconds == default_debate_timeout_seconds() + && self.model.is_none() + && self.thinking.is_none() + && self.same_model_fallback + && self.tier.is_none() + } +} + /// Configuration for fallback behavior when external services are unavailable. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct FallbackConfig { @@ -708,6 +735,10 @@ fn resolve_auto_tool(section: &str, parent_tool: Option<&str>) -> Result #[path = "global_tests.rs"] mod tests; +#[cfg(test)] +#[path = "global_tests_heterogeneous.rs"] +mod tests_heterogeneous; + #[cfg(test)] #[path = "global_tests_priority.rs"] mod tests_priority; diff --git a/crates/csa-config/src/global_tests.rs b/crates/csa-config/src/global_tests.rs index 920d199a..ba155d8e 100644 --- a/crates/csa-config/src/global_tests.rs +++ b/crates/csa-config/src/global_tests.rs @@ -228,6 +228,7 @@ fn test_gate_mode_serde_roundtrip_all_variants() { tool: "codex".to_string(), gate_mode: gate_mode.clone(), tier: None, + model: None, thinking: None, gate_command: None, gate_timeout_secs: ReviewConfig::default_gate_timeout(), @@ -359,400 +360,68 @@ thinking = "high" } #[test] -fn test_parse_debate_config_same_model_fallback_disabled() { +fn test_parse_review_config_with_model() { let toml_str = r#" -[debate] -tool = "auto" -same_model_fallback = false -"#; - let config: GlobalConfig = toml::from_str(toml_str).unwrap(); - assert!(!config.debate.same_model_fallback); -} - -#[test] -fn test_slots_dir() { - // Should not fail on supported platforms - let dir = GlobalConfig::slots_dir(); - assert!(dir.is_ok()); - let path = dir.unwrap(); - assert!(path.to_string_lossy().contains("slots")); -} - -#[test] -fn test_select_heterogeneous_tool_claude_to_others() { - let parent = ToolName::ClaudeCode; - let available = vec![ - ToolName::ClaudeCode, - ToolName::GeminiCli, - ToolName::Codex, - ToolName::Opencode, - ]; - let result = select_heterogeneous_tool(&parent, &available); - assert!(result.is_some()); - let tool = result.unwrap(); - assert_ne!(tool.model_family(), parent.model_family()); -} - -#[test] -fn test_select_heterogeneous_tool_gemini_to_others() { - let parent = ToolName::GeminiCli; - let available = vec![ToolName::GeminiCli, ToolName::Codex, ToolName::ClaudeCode]; - let result = select_heterogeneous_tool(&parent, &available); - assert!(result.is_some()); - let tool = result.unwrap(); - assert_ne!(tool.model_family(), parent.model_family()); -} - -#[test] -fn test_select_heterogeneous_tool_none_when_all_same_family() { - let parent = ToolName::ClaudeCode; - let available = vec![ToolName::ClaudeCode]; // Only same family - let result = select_heterogeneous_tool(&parent, &available); - assert!(result.is_none()); -} - -#[test] -fn test_select_heterogeneous_tool_empty_available() { - let parent = ToolName::ClaudeCode; - let available = vec![]; - let result = select_heterogeneous_tool(&parent, &available); - assert!(result.is_none()); -} - -#[test] -fn test_fallback_config_default() { - let config = GlobalConfig::default(); - assert_eq!(config.fallback.cloud_review_exhausted, "ask-user"); -} - -#[test] -fn test_fallback_config_auto_local() { - let toml_str = r#" -[fallback] -cloud_review_exhausted = "auto-local" -"#; - let config: GlobalConfig = toml::from_str(toml_str).unwrap(); - assert_eq!(config.fallback.cloud_review_exhausted, "auto-local"); -} - -#[test] -fn test_fallback_config_missing_uses_default() { - let toml_str = r#" -[defaults] -max_concurrent = 3 +[review] +tool = "gemini-cli" +model = "gemini-3.1-pro-preview" +thinking = "xhigh" "#; let config: GlobalConfig = toml::from_str(toml_str).unwrap(); - assert_eq!(config.fallback.cloud_review_exhausted, "ask-user"); -} - -#[test] -fn test_all_known_tools() { - let tools = all_known_tools(); - assert_eq!(tools.len(), 4); - assert!(tools.contains(&ToolName::GeminiCli)); - assert!(tools.contains(&ToolName::Opencode)); - assert!(tools.contains(&ToolName::Codex)); - assert!(tools.contains(&ToolName::ClaudeCode)); -} - -#[test] -fn test_heterogeneous_counterpart_claude_to_codex() { - assert_eq!(heterogeneous_counterpart("claude-code"), Some("codex")); -} - -#[test] -fn test_heterogeneous_counterpart_codex_to_claude() { - assert_eq!(heterogeneous_counterpart("codex"), Some("claude-code")); -} - -#[test] -fn test_heterogeneous_counterpart_gemini_returns_none() { - assert_eq!(heterogeneous_counterpart("gemini-cli"), None); -} - -#[test] -fn test_heterogeneous_counterpart_opencode_returns_none() { - assert_eq!(heterogeneous_counterpart("opencode"), None); -} - -#[test] -fn test_heterogeneous_counterpart_unknown_returns_none() { - assert_eq!(heterogeneous_counterpart("unknown-tool"), None); - assert_eq!(heterogeneous_counterpart(""), None); -} - -#[test] -fn test_all_tool_slots_includes_extra_config_tools() { - let mut config = GlobalConfig::default(); - config.tools.insert( - "custom-tool".to_string(), - GlobalToolConfig { - max_concurrent: Some(7), - env: HashMap::new(), - ..Default::default() - }, - ); - - let slots = config.all_tool_slots(); - // 4 static tools + 1 custom = 5 - assert_eq!(slots.len(), 5); - let custom = slots.iter().find(|(t, _)| *t == "custom-tool").unwrap(); - assert_eq!(custom.1, 7); -} - -#[test] -fn test_all_tool_slots_default_config_has_four_tools() { - let config = GlobalConfig::default(); - let slots = config.all_tool_slots(); - assert_eq!(slots.len(), 4); - - let names: Vec<&str> = slots.iter().map(|(n, _)| *n).collect(); - assert!(names.contains(&"gemini-cli")); - assert!(names.contains(&"opencode")); - assert!(names.contains(&"codex")); - assert!(names.contains(&"claude-code")); - - // All should have default concurrency - for (_, count) in &slots { - assert_eq!(*count, 3); - } -} - -#[test] -fn test_env_vars_multiple_keys() { - let mut config = GlobalConfig::default(); - let mut env = HashMap::new(); - env.insert("API_KEY".to_string(), "key-1".to_string()); - env.insert("SECRET".to_string(), "secret-1".to_string()); - config.tools.insert( - "codex".to_string(), - GlobalToolConfig { - max_concurrent: None, - env, - ..Default::default() - }, + assert_eq!( + config.review.model.as_deref(), + Some("gemini-3.1-pro-preview") ); - - let vars = config.env_vars("codex").unwrap(); - assert_eq!(vars.len(), 2); - assert_eq!(vars.get("API_KEY").unwrap(), "key-1"); - assert_eq!(vars.get("SECRET").unwrap(), "secret-1"); -} - -#[test] -fn test_env_vars_nonexistent_tool_returns_none() { - let config = GlobalConfig::default(); - assert!(config.env_vars("totally-unknown").is_none()); + assert!(!config.review.is_default()); } #[test] -fn test_max_concurrent_with_custom_default() { - let mut config = GlobalConfig::default(); - config.defaults.max_concurrent = 10; - - // All tools without overrides should use the custom default - assert_eq!(config.max_concurrent("gemini-cli"), 10); - assert_eq!(config.max_concurrent("codex"), 10); - assert_eq!(config.max_concurrent("unknown"), 10); - - // Tool-specific override still wins - config.tools.insert( - "codex".to_string(), - GlobalToolConfig { - max_concurrent: Some(2), - env: HashMap::new(), - ..Default::default() - }, - ); - assert_eq!(config.max_concurrent("codex"), 2); - assert_eq!(config.max_concurrent("gemini-cli"), 10); // still default +fn test_review_config_model_none_is_default() { + let config = ReviewConfig::default(); + assert!(config.model.is_none()); + assert!(config.is_default()); } #[test] -fn test_max_concurrent_tool_with_none_uses_default() { - let mut config = GlobalConfig::default(); - config.tools.insert( - "codex".to_string(), - GlobalToolConfig { - max_concurrent: None, // explicitly None - env: HashMap::new(), - ..Default::default() - }, +fn test_review_config_model_skipped_when_none() { + let config = ReviewConfig::default(); + let toml_str = toml::to_string(&config).unwrap(); + assert!( + !toml_str.contains("model"), + "None model should be omitted from TOML output: {toml_str}" ); - assert_eq!(config.max_concurrent("codex"), 3); // falls back to default -} - -#[test] -fn test_resolve_debate_tool_explicit_override() { - let mut config = GlobalConfig::default(); - config.debate.tool = "opencode".to_string(); - // When explicitly set, should return the explicit value regardless of parent - let tool = config.resolve_debate_tool(Some("anything")).unwrap(); - assert_eq!(tool, "opencode"); - let tool = config.resolve_debate_tool(None).unwrap(); - assert_eq!(tool, "opencode"); } #[test] -fn test_resolve_review_tool_explicit_ignores_parent() { - let mut config = GlobalConfig::default(); - config.review.tool = "gemini-cli".to_string(); - let tool = config.resolve_review_tool(None).unwrap(); - assert_eq!(tool, "gemini-cli"); -} - -#[test] -fn test_parse_toml_with_all_sections() { +fn test_parse_debate_config_with_model() { let toml_str = r#" -[defaults] -max_concurrent = 2 - -[tools.codex] -max_concurrent = 4 -[tools.codex.env] -OPENAI_API_KEY = "sk-test" - -[review] -tool = "codex" - [debate] -tool = "claude-code" - -[fallback] -cloud_review_exhausted = "auto-local" - -[todo] -show_command = "bat -l md" -diff_command = "delta" -"#; - let config: GlobalConfig = toml::from_str(toml_str).unwrap(); - assert_eq!(config.defaults.max_concurrent, 2); - assert_eq!(config.max_concurrent("codex"), 4); - assert_eq!(config.max_concurrent("gemini-cli"), 2); // falls to default - assert_eq!(config.review.tool, "codex"); - assert_eq!(config.debate.tool, "claude-code"); - assert_eq!(config.debate.timeout_seconds, 1800); - assert_eq!(config.debate.thinking, None); - assert_eq!(config.fallback.cloud_review_exhausted, "auto-local"); - assert_eq!(config.todo.show_command.as_deref(), Some("bat -l md")); - assert_eq!(config.todo.diff_command.as_deref(), Some("delta")); -} - -#[test] -fn test_parse_empty_toml() { - let config: GlobalConfig = toml::from_str("").unwrap(); - assert_eq!(config.defaults.max_concurrent, 3); - assert!(config.defaults.tool.is_none()); - assert!(config.tools.is_empty()); - assert_eq!(config.review.tool, "auto"); - assert_eq!(config.debate.tool, "auto"); - assert_eq!(config.debate.timeout_seconds, 1800); - assert_eq!(config.debate.thinking, None); - assert_eq!(config.fallback.cloud_review_exhausted, "ask-user"); -} - -#[test] -fn test_defaults_config_deserialization_with_tool() { - let toml_str = r#" -[defaults] -max_concurrent = 4 -tool = "codex" +tool = "gemini-cli" +model = "gemini-3.1-pro-preview" +thinking = "xhigh" "#; let config: GlobalConfig = toml::from_str(toml_str).unwrap(); - assert_eq!(config.defaults.max_concurrent, 4); - assert_eq!(config.defaults.tool.as_deref(), Some("codex")); -} - -#[test] -fn test_defaults_config_deserialization_without_tool() { - let toml_str = r#" -[defaults] -max_concurrent = 4 -"#; - let config: GlobalConfig = toml::from_str(toml_str).unwrap(); - assert_eq!(config.defaults.max_concurrent, 4); - assert!(config.defaults.tool.is_none()); -} - -#[test] -fn test_resolve_auto_tool_error_includes_section_name() { - let config = GlobalConfig::default(); - let err = config.resolve_review_tool(Some("gemini-cli")).unwrap_err(); - assert!(err.to_string().contains("review")); - - let err = config.resolve_debate_tool(Some("gemini-cli")).unwrap_err(); - assert!(err.to_string().contains("debate")); -} - -#[test] -fn test_todo_display_config_default() { - let config = GlobalConfig::default(); - assert!(config.todo.show_command.is_none()); - assert!(config.todo.diff_command.is_none()); -} - -#[test] -fn test_state_base_dir_returns_ok() { - let dir = GlobalConfig::state_base_dir(); - assert!(dir.is_ok()); - let path = dir.unwrap(); - let path_str = path.to_string_lossy(); - assert!( - path_str.contains("cli-sub-agent") || path_str.contains("csa"), - "unexpected state dir path: {path_str}" + assert_eq!( + config.debate.model.as_deref(), + Some("gemini-3.1-pro-preview") ); -} - -// ── Task 5: ExecutionConfig in GlobalConfig ───────────────────────── - -#[test] -fn test_global_execution_config_default() { - let config = GlobalConfig::default(); - assert!(config.execution.is_default()); - assert_eq!(config.execution.min_timeout_seconds, 1800); + assert!(!config.debate.is_default()); } #[test] -fn test_global_execution_config_parses_from_toml() { - let toml_str = r#" -[execution] -min_timeout_seconds = 2400 -"#; - let config: GlobalConfig = toml::from_str(toml_str).unwrap(); - assert_eq!(config.execution.min_timeout_seconds, 2400); - assert!(!config.execution.is_default()); -} - -#[test] -fn test_global_execution_config_empty_toml_uses_default() { - let config: GlobalConfig = toml::from_str("").unwrap(); - assert_eq!(config.execution.min_timeout_seconds, 1800); - assert!(config.execution.is_default()); +fn test_debate_config_model_none_is_default() { + let config = DebateConfig::default(); + assert!(config.model.is_none()); + assert!(config.is_default()); } #[test] -fn test_global_execution_config_coexists_with_other_sections() { +fn test_parse_debate_config_same_model_fallback_disabled() { let toml_str = r#" -[defaults] -max_concurrent = 5 - -[execution] -min_timeout_seconds = 3600 - -[review] -tool = "codex" +[debate] +tool = "auto" +same_model_fallback = false "#; let config: GlobalConfig = toml::from_str(toml_str).unwrap(); - assert_eq!(config.defaults.max_concurrent, 5); - assert_eq!(config.execution.min_timeout_seconds, 3600); - assert_eq!(config.review.tool, "codex"); -} - -#[test] -fn test_global_default_template_mentions_execution() { - let template = GlobalConfig::default_template(); - assert!( - template.contains("min_timeout_seconds"), - "Default template should mention min_timeout_seconds" - ); + assert!(!config.debate.same_model_fallback); } diff --git a/crates/csa-config/src/global_tests_heterogeneous.rs b/crates/csa-config/src/global_tests_heterogeneous.rs new file mode 100644 index 00000000..b9487bac --- /dev/null +++ b/crates/csa-config/src/global_tests_heterogeneous.rs @@ -0,0 +1,381 @@ +use super::*; +use std::collections::HashMap; + +#[test] +fn test_select_heterogeneous_tool_claude_to_others() { + let parent = ToolName::ClaudeCode; + let available = vec![ + ToolName::ClaudeCode, + ToolName::GeminiCli, + ToolName::Codex, + ToolName::Opencode, + ]; + let result = select_heterogeneous_tool(&parent, &available); + assert!(result.is_some()); + let tool = result.unwrap(); + assert_ne!(tool.model_family(), parent.model_family()); +} + +#[test] +fn test_select_heterogeneous_tool_gemini_to_others() { + let parent = ToolName::GeminiCli; + let available = vec![ToolName::GeminiCli, ToolName::Codex, ToolName::ClaudeCode]; + let result = select_heterogeneous_tool(&parent, &available); + assert!(result.is_some()); + let tool = result.unwrap(); + assert_ne!(tool.model_family(), parent.model_family()); +} + +#[test] +fn test_select_heterogeneous_tool_none_when_all_same_family() { + let parent = ToolName::ClaudeCode; + let available = vec![ToolName::ClaudeCode]; // Only same family + let result = select_heterogeneous_tool(&parent, &available); + assert!(result.is_none()); +} + +#[test] +fn test_select_heterogeneous_tool_empty_available() { + let parent = ToolName::ClaudeCode; + let available = vec![]; + let result = select_heterogeneous_tool(&parent, &available); + assert!(result.is_none()); +} + +#[test] +fn test_fallback_config_default() { + let config = GlobalConfig::default(); + assert_eq!(config.fallback.cloud_review_exhausted, "ask-user"); +} + +#[test] +fn test_fallback_config_auto_local() { + let toml_str = r#" +[fallback] +cloud_review_exhausted = "auto-local" +"#; + let config: GlobalConfig = toml::from_str(toml_str).unwrap(); + assert_eq!(config.fallback.cloud_review_exhausted, "auto-local"); +} + +#[test] +fn test_fallback_config_missing_uses_default() { + let toml_str = r#" +[defaults] +max_concurrent = 3 +"#; + let config: GlobalConfig = toml::from_str(toml_str).unwrap(); + assert_eq!(config.fallback.cloud_review_exhausted, "ask-user"); +} + +#[test] +fn test_all_known_tools() { + let tools = all_known_tools(); + assert_eq!(tools.len(), 4); + assert!(tools.contains(&ToolName::GeminiCli)); + assert!(tools.contains(&ToolName::Opencode)); + assert!(tools.contains(&ToolName::Codex)); + assert!(tools.contains(&ToolName::ClaudeCode)); +} + +#[test] +fn test_heterogeneous_counterpart_claude_to_codex() { + assert_eq!(heterogeneous_counterpart("claude-code"), Some("codex")); +} + +#[test] +fn test_heterogeneous_counterpart_codex_to_claude() { + assert_eq!(heterogeneous_counterpart("codex"), Some("claude-code")); +} + +#[test] +fn test_heterogeneous_counterpart_gemini_returns_none() { + assert_eq!(heterogeneous_counterpart("gemini-cli"), None); +} + +#[test] +fn test_heterogeneous_counterpart_opencode_returns_none() { + assert_eq!(heterogeneous_counterpart("opencode"), None); +} + +#[test] +fn test_heterogeneous_counterpart_unknown_returns_none() { + assert_eq!(heterogeneous_counterpart("unknown-tool"), None); + assert_eq!(heterogeneous_counterpart(""), None); +} + +#[test] +fn test_all_tool_slots_includes_extra_config_tools() { + let mut config = GlobalConfig::default(); + config.tools.insert( + "custom-tool".to_string(), + GlobalToolConfig { + max_concurrent: Some(7), + env: HashMap::new(), + ..Default::default() + }, + ); + + let slots = config.all_tool_slots(); + // 4 static tools + 1 custom = 5 + assert_eq!(slots.len(), 5); + let custom = slots.iter().find(|(t, _)| *t == "custom-tool").unwrap(); + assert_eq!(custom.1, 7); +} + +#[test] +fn test_all_tool_slots_default_config_has_four_tools() { + let config = GlobalConfig::default(); + let slots = config.all_tool_slots(); + assert_eq!(slots.len(), 4); + + let names: Vec<&str> = slots.iter().map(|(n, _)| *n).collect(); + assert!(names.contains(&"gemini-cli")); + assert!(names.contains(&"opencode")); + assert!(names.contains(&"codex")); + assert!(names.contains(&"claude-code")); + + // All should have default concurrency + for (_, count) in &slots { + assert_eq!(*count, 3); + } +} + +#[test] +fn test_env_vars_multiple_keys() { + let mut config = GlobalConfig::default(); + let mut env = HashMap::new(); + env.insert("API_KEY".to_string(), "key-1".to_string()); + env.insert("SECRET".to_string(), "secret-1".to_string()); + config.tools.insert( + "codex".to_string(), + GlobalToolConfig { + max_concurrent: None, + env, + ..Default::default() + }, + ); + + let vars = config.env_vars("codex").unwrap(); + assert_eq!(vars.len(), 2); + assert_eq!(vars.get("API_KEY").unwrap(), "key-1"); + assert_eq!(vars.get("SECRET").unwrap(), "secret-1"); +} + +#[test] +fn test_env_vars_nonexistent_tool_returns_none() { + let config = GlobalConfig::default(); + assert!(config.env_vars("totally-unknown").is_none()); +} + +#[test] +fn test_max_concurrent_with_custom_default() { + let mut config = GlobalConfig::default(); + config.defaults.max_concurrent = 10; + + // All tools without overrides should use the custom default + assert_eq!(config.max_concurrent("gemini-cli"), 10); + assert_eq!(config.max_concurrent("codex"), 10); + assert_eq!(config.max_concurrent("unknown"), 10); + + // Tool-specific override still wins + config.tools.insert( + "codex".to_string(), + GlobalToolConfig { + max_concurrent: Some(2), + env: HashMap::new(), + ..Default::default() + }, + ); + assert_eq!(config.max_concurrent("codex"), 2); + assert_eq!(config.max_concurrent("gemini-cli"), 10); // still default +} + +#[test] +fn test_max_concurrent_tool_with_none_uses_default() { + let mut config = GlobalConfig::default(); + config.tools.insert( + "codex".to_string(), + GlobalToolConfig { + max_concurrent: None, // explicitly None + env: HashMap::new(), + ..Default::default() + }, + ); + assert_eq!(config.max_concurrent("codex"), 3); // falls back to default +} + +#[test] +fn test_resolve_debate_tool_explicit_override() { + let mut config = GlobalConfig::default(); + config.debate.tool = "opencode".to_string(); + // When explicitly set, should return the explicit value regardless of parent + let tool = config.resolve_debate_tool(Some("anything")).unwrap(); + assert_eq!(tool, "opencode"); + let tool = config.resolve_debate_tool(None).unwrap(); + assert_eq!(tool, "opencode"); +} + +#[test] +fn test_resolve_review_tool_explicit_ignores_parent() { + let mut config = GlobalConfig::default(); + config.review.tool = "gemini-cli".to_string(); + let tool = config.resolve_review_tool(None).unwrap(); + assert_eq!(tool, "gemini-cli"); +} + +#[test] +fn test_parse_toml_with_all_sections() { + let toml_str = r#" +[defaults] +max_concurrent = 2 + +[tools.codex] +max_concurrent = 4 +[tools.codex.env] +OPENAI_API_KEY = "sk-test" + +[review] +tool = "codex" + +[debate] +tool = "claude-code" + +[fallback] +cloud_review_exhausted = "auto-local" + +[todo] +show_command = "bat -l md" +diff_command = "delta" +"#; + let config: GlobalConfig = toml::from_str(toml_str).unwrap(); + assert_eq!(config.defaults.max_concurrent, 2); + assert_eq!(config.max_concurrent("codex"), 4); + assert_eq!(config.max_concurrent("gemini-cli"), 2); // falls to default + assert_eq!(config.review.tool, "codex"); + assert_eq!(config.debate.tool, "claude-code"); + assert_eq!(config.debate.timeout_seconds, 1800); + assert_eq!(config.debate.thinking, None); + assert_eq!(config.fallback.cloud_review_exhausted, "auto-local"); + assert_eq!(config.todo.show_command.as_deref(), Some("bat -l md")); + assert_eq!(config.todo.diff_command.as_deref(), Some("delta")); +} + +#[test] +fn test_parse_empty_toml() { + let config: GlobalConfig = toml::from_str("").unwrap(); + assert_eq!(config.defaults.max_concurrent, 3); + assert!(config.defaults.tool.is_none()); + assert!(config.tools.is_empty()); + assert_eq!(config.review.tool, "auto"); + assert_eq!(config.debate.tool, "auto"); + assert_eq!(config.debate.timeout_seconds, 1800); + assert_eq!(config.debate.thinking, None); + assert_eq!(config.fallback.cloud_review_exhausted, "ask-user"); +} + +#[test] +fn test_defaults_config_deserialization_with_tool() { + let toml_str = r#" +[defaults] +max_concurrent = 4 +tool = "codex" +"#; + let config: GlobalConfig = toml::from_str(toml_str).unwrap(); + assert_eq!(config.defaults.max_concurrent, 4); + assert_eq!(config.defaults.tool.as_deref(), Some("codex")); +} + +#[test] +fn test_defaults_config_deserialization_without_tool() { + let toml_str = r#" +[defaults] +max_concurrent = 4 +"#; + let config: GlobalConfig = toml::from_str(toml_str).unwrap(); + assert_eq!(config.defaults.max_concurrent, 4); + assert!(config.defaults.tool.is_none()); +} + +#[test] +fn test_resolve_auto_tool_error_includes_section_name() { + let config = GlobalConfig::default(); + let err = config.resolve_review_tool(Some("gemini-cli")).unwrap_err(); + assert!(err.to_string().contains("review")); + + let err = config.resolve_debate_tool(Some("gemini-cli")).unwrap_err(); + assert!(err.to_string().contains("debate")); +} + +#[test] +fn test_todo_display_config_default() { + let config = GlobalConfig::default(); + assert!(config.todo.show_command.is_none()); + assert!(config.todo.diff_command.is_none()); +} + +#[test] +fn test_state_base_dir_returns_ok() { + let dir = GlobalConfig::state_base_dir(); + assert!(dir.is_ok()); + let path = dir.unwrap(); + let path_str = path.to_string_lossy(); + assert!( + path_str.contains("cli-sub-agent") || path_str.contains("csa"), + "unexpected state dir path: {path_str}" + ); +} + +// ── Task 5: ExecutionConfig in GlobalConfig ───────────────────────── + +#[test] +fn test_global_execution_config_default() { + let config = GlobalConfig::default(); + assert!(config.execution.is_default()); + assert_eq!(config.execution.min_timeout_seconds, 1800); +} + +#[test] +fn test_global_execution_config_parses_from_toml() { + let toml_str = r#" +[execution] +min_timeout_seconds = 2400 +"#; + let config: GlobalConfig = toml::from_str(toml_str).unwrap(); + assert_eq!(config.execution.min_timeout_seconds, 2400); + assert!(!config.execution.is_default()); +} + +#[test] +fn test_global_execution_config_empty_toml_uses_default() { + let config: GlobalConfig = toml::from_str("").unwrap(); + assert_eq!(config.execution.min_timeout_seconds, 1800); + assert!(config.execution.is_default()); +} + +#[test] +fn test_global_execution_config_coexists_with_other_sections() { + let toml_str = r#" +[defaults] +max_concurrent = 5 + +[execution] +min_timeout_seconds = 3600 + +[review] +tool = "codex" +"#; + let config: GlobalConfig = toml::from_str(toml_str).unwrap(); + assert_eq!(config.defaults.max_concurrent, 5); + assert_eq!(config.execution.min_timeout_seconds, 3600); + assert_eq!(config.review.tool, "codex"); +} + +#[test] +fn test_global_default_template_mentions_execution() { + let template = GlobalConfig::default_template(); + assert!( + template.contains("min_timeout_seconds"), + "Default template should mention min_timeout_seconds" + ); +} diff --git a/crates/csa-executor/src/executor.rs b/crates/csa-executor/src/executor.rs index 0067fb1b..d5e6eaaa 100644 --- a/crates/csa-executor/src/executor.rs +++ b/crates/csa-executor/src/executor.rs @@ -255,10 +255,7 @@ impl Executor { } } - /// Override the thinking budget for this executor. - /// - /// Used by thinking lock: when a tool has `thinking_lock` configured, - /// the locked value replaces whatever thinking budget was set. + /// Override the thinking budget (thinking_lock replaces whatever was set). pub fn override_thinking_budget(&mut self, budget: ThinkingBudget) { match self { Self::GeminiCli { @@ -278,6 +275,18 @@ impl Executor { } } + /// Override the model (CLI `--model` / config `[review].model` > tier model_spec). + pub fn override_model(&mut self, model: String) { + match self { + Self::GeminiCli { model_override, .. } + | Self::Opencode { model_override, .. } + | Self::Codex { model_override, .. } + | Self::ClaudeCode { model_override, .. } => { + *model_override = Some(model); + } + } + } + /// Apply restrictions by modifying the prompt to include restriction instructions. /// Returns the modified prompt. /// diff --git a/crates/csa-executor/src/executor_build_cmd_tests_part2.rs b/crates/csa-executor/src/executor_build_cmd_tests_part2.rs index 93481b26..64d61d83 100644 --- a/crates/csa-executor/src/executor_build_cmd_tests_part2.rs +++ b/crates/csa-executor/src/executor_build_cmd_tests_part2.rs @@ -119,6 +119,48 @@ fn test_build_command_prompt_with_special_characters() { ); } +#[test] +fn test_override_model_replaces_existing() { + let mut exec = Executor::GeminiCli { + model_override: Some("gemini-3-flash-preview".to_string()), + thinking_budget: None, + }; + exec.override_model("gemini-3.1-pro-preview".to_string()); + let debug = format!("{:?}", exec); + assert!( + debug.contains("gemini-3.1-pro-preview"), + "override_model should replace existing model: {debug}" + ); + assert!( + !debug.contains("flash"), + "original model should be gone: {debug}" + ); +} + +#[test] +fn test_override_model_sets_none_to_some() { + let mut exec = Executor::GeminiCli { + model_override: None, + thinking_budget: None, + }; + exec.override_model("gemini-3.1-pro-preview".to_string()); + let session = make_test_session(); + let (cmd, _) = exec.build_command("test", None, &session, None); + let args: Vec<_> = cmd + .as_std() + .get_args() + .map(|a| a.to_string_lossy().to_string()) + .collect(); + assert!( + args.contains(&"-m".to_string()), + "Should have -m flag after override_model: {args:?}" + ); + assert!( + args.contains(&"gemini-3.1-pro-preview".to_string()), + "Should have the overridden model value: {args:?}" + ); +} + #[test] fn test_build_command_no_model_override_omits_model_flag() { let exec = Executor::ClaudeCode { diff --git a/weave.lock b/weave.lock index cbaffa68..e0e316a0 100644 --- a/weave.lock +++ b/weave.lock @@ -1,6 +1,6 @@ [versions] -csa = "0.1.106" -weave = "0.1.106" +csa = "0.1.107" +weave = "0.1.107" last_migrated_at = "2026-03-08T12:08:01.820964091Z" [migrations]