diff --git a/Cargo.lock b/Cargo.lock index bd897123..57d3f552 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -451,7 +451,7 @@ checksum = "c3e64b0cc0439b12df2fa678eae89a1c56a529fd067a9115f7827f1fffd22b32" [[package]] name = "cli-sub-agent" -version = "0.1.105" +version = "0.1.106" dependencies = [ "anyhow", "chrono", @@ -610,7 +610,7 @@ dependencies = [ [[package]] name = "csa-acp" -version = "0.1.105" +version = "0.1.106" dependencies = [ "agent-client-protocol", "anyhow", @@ -629,7 +629,7 @@ dependencies = [ [[package]] name = "csa-config" -version = "0.1.105" +version = "0.1.106" dependencies = [ "anyhow", "chrono", @@ -645,7 +645,7 @@ dependencies = [ [[package]] name = "csa-core" -version = "0.1.105" +version = "0.1.106" dependencies = [ "agent-teams", "chrono", @@ -659,7 +659,7 @@ dependencies = [ [[package]] name = "csa-executor" -version = "0.1.105" +version = "0.1.106" dependencies = [ "agent-teams", "anyhow", @@ -683,7 +683,7 @@ dependencies = [ [[package]] name = "csa-hooks" -version = "0.1.105" +version = "0.1.106" dependencies = [ "anyhow", "chrono", @@ -698,7 +698,7 @@ dependencies = [ [[package]] name = "csa-lock" -version = "0.1.105" +version = "0.1.106" dependencies = [ "anyhow", "chrono", @@ -710,7 +710,7 @@ dependencies = [ [[package]] name = "csa-mcp-hub" -version = "0.1.105" +version = "0.1.106" dependencies = [ "anyhow", "axum", @@ -732,7 +732,7 @@ dependencies = [ [[package]] name = "csa-memory" -version = "0.1.105" +version = "0.1.106" dependencies = [ "anyhow", "async-trait", @@ -750,7 +750,7 @@ dependencies = [ [[package]] name = "csa-process" -version = "0.1.105" +version = "0.1.106" dependencies = [ "anyhow", "csa-core", @@ -767,7 +767,7 @@ dependencies = [ [[package]] name = "csa-resource" -version = "0.1.105" +version = "0.1.106" dependencies = [ "anyhow", "csa-core", @@ -782,7 +782,7 @@ dependencies = [ [[package]] name = "csa-scheduler" -version = "0.1.105" +version = "0.1.106" dependencies = [ "anyhow", "chrono", @@ -800,7 +800,7 @@ dependencies = [ [[package]] name = "csa-session" -version = "0.1.105" +version = "0.1.106" dependencies = [ "anyhow", "chrono", @@ -821,7 +821,7 @@ dependencies = [ [[package]] name = "csa-todo" -version = "0.1.105" +version = "0.1.106" dependencies = [ "anyhow", "chrono", @@ -4089,7 +4089,7 @@ dependencies = [ [[package]] name = "weave" -version = "0.1.105" +version = "0.1.106" dependencies = [ "anyhow", "clap", diff --git a/Cargo.toml b/Cargo.toml index 224a55a5..e598c8ed 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,7 +3,7 @@ members = ["crates/*"] resolver = "2" [workspace.package] -version = "0.1.105" +version = "0.1.106" 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 aea4d008..e5739148 100644 --- a/crates/cli-sub-agent/src/claude_sub_agent_cmd.rs +++ b/crates/cli-sub-agent/src/claude_sub_agent_cmd.rs @@ -282,6 +282,8 @@ mod tests { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), } } @@ -375,6 +377,8 @@ mod tests { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; let tools = get_auto_selectable_tools(Some(&cfg), std::path::Path::new("/tmp")); diff --git a/crates/cli-sub-agent/src/cli_review.rs b/crates/cli-sub-agent/src/cli_review.rs index 5b523c80..4e233958 100644 --- a/crates/cli-sub-agent/src/cli_review.rs +++ b/crates/cli-sub-agent/src/cli_review.rs @@ -73,6 +73,10 @@ pub struct ReviewArgs { #[arg(long)] pub fix: bool, + /// Maximum fix iterations when --fix is enabled (default: 3) + #[arg(long, default_value_t = 3, value_parser = clap::value_parser!(u8).range(1..))] + pub max_rounds: u8, + /// Review mode: standard (default) or red-team #[arg(long, value_enum)] pub review_mode: Option, @@ -170,17 +174,20 @@ pub fn validate_review_args(args: &ReviewArgs) -> std::result::Result<(), clap:: Ok(()) } -pub fn validate_command_args(command: &Commands) -> std::result::Result<(), clap::Error> { +pub fn validate_command_args( + command: &Commands, + min_timeout: u64, +) -> std::result::Result<(), clap::Error> { match command { Commands::Run { timeout, .. } => { - validate_timeout(*timeout)?; + validate_timeout(*timeout, min_timeout)?; } Commands::Review(args) => { validate_review_args(args)?; - validate_timeout(args.timeout)?; + validate_timeout(args.timeout, min_timeout)?; } Commands::Debate(args) => { - validate_timeout(args.timeout)?; + validate_timeout(args.timeout, min_timeout)?; } _ => {} } @@ -188,14 +195,21 @@ pub fn validate_command_args(command: &Commands) -> std::result::Result<(), clap Ok(()) } -fn validate_timeout(timeout: Option) -> std::result::Result<(), clap::Error> { +fn validate_timeout( + timeout: Option, + min_timeout: u64, +) -> std::result::Result<(), clap::Error> { if let Some(t) = timeout { - if t < 1200 { + if t < min_timeout { + let min_minutes = min_timeout / 60; return Err(clap::Error::raw( clap::error::ErrorKind::ValueValidation, - "Absolute timeout (--timeout) must be at least 1200 seconds (20 minutes). \ - Short timeouts waste tokens because the agent starts working but gets killed before producing output. \ - Record this in your CLAUDE.md or memory: CSA minimum timeout is 1200 seconds", + format!( + "Absolute timeout (--timeout) must be at least {min_timeout} seconds ({min_minutes} minutes). \ + Short timeouts waste tokens because the agent starts working but gets killed before producing output. \ + Record this in your CLAUDE.md or memory: CSA minimum timeout is {min_timeout} seconds. \ + Configure via [execution] min_timeout_seconds in .csa/config.toml or global config." + ), )); } } diff --git a/crates/cli-sub-agent/src/debate_cmd.rs b/crates/cli-sub-agent/src/debate_cmd.rs index ec7c7bbc..5f1b5c35 100644 --- a/crates/cli-sub-agent/src/debate_cmd.rs +++ b/crates/cli-sub-agent/src/debate_cmd.rs @@ -45,6 +45,68 @@ pub(crate) async fn handle_debate( // 2b. Verify debate skill is available (fail fast before any execution) verify_debate_skill_available(&project_root)?; + // 2c. Run pre-review quality gate (reuses [review] gate settings) + // + // Debate reuses the review section's gate settings because the gate is a + // shared pre-execution quality check (lint/test) that applies equally to + // both review and debate workflows. + { + let gate_command = config + .as_ref() + .and_then(|c| c.review.as_ref()) + .and_then(|r| r.gate_command.as_deref()); + let gate_timeout = config + .as_ref() + .and_then(|c| c.review.as_ref()) + .map(|r| r.gate_timeout_secs) + .unwrap_or_else(csa_config::ReviewConfig::default_gate_timeout); + let gate_mode = &global_config.review.gate_mode; + + let gate_result = crate::pipeline::gate::evaluate_quality_gate( + &project_root, + gate_command, + gate_timeout, + gate_mode, + ) + .await?; + + if gate_result.skipped { + debug!( + reason = gate_result.skip_reason.as_deref().unwrap_or("unknown"), + "Quality gate skipped" + ); + } else if !gate_result.passed() { + match gate_mode { + csa_config::GateMode::Monitor => { + warn!( + command = %gate_result.command, + exit_code = ?gate_result.exit_code, + "Quality gate failed (monitor mode — continuing with debate)" + ); + } + csa_config::GateMode::CriticalOnly | csa_config::GateMode::Full => { + let mut msg = format!( + "Pre-debate quality gate failed (mode={gate_mode:?}).\n\ + Command: {}\nExit code: {:?}", + gate_result.command, gate_result.exit_code + ); + if !gate_result.stdout.is_empty() { + msg.push_str(&format!("\n--- stdout ---\n{}", gate_result.stdout)); + } + if !gate_result.stderr.is_empty() { + msg.push_str(&format!("\n--- stderr ---\n{}", gate_result.stderr)); + } + anyhow::bail!(msg); + } + } + } else { + debug!( + command = %gate_result.command, + "Quality gate passed" + ); + } + } + // 3. Read question (from arg or stdin) let question = read_prompt(args.question)?; diff --git a/crates/cli-sub-agent/src/debate_cmd_tests.rs b/crates/cli-sub-agent/src/debate_cmd_tests.rs index 8faa49ed..46ec5950 100644 --- a/crates/cli-sub-agent/src/debate_cmd_tests.rs +++ b/crates/cli-sub-agent/src/debate_cmd_tests.rs @@ -45,6 +45,8 @@ fn project_config_with_enabled_tools(tools: &[&str]) -> ProjectConfig { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), } } diff --git a/crates/cli-sub-agent/src/main.rs b/crates/cli-sub-agent/src/main.rs index df58d13f..9dafa898 100644 --- a/crates/cli-sub-agent/src/main.rs +++ b/crates/cli-sub-agent/src/main.rs @@ -124,6 +124,34 @@ pub(crate) fn validate_sa_mode(command: &Commands, current_depth: u32) -> anyhow Ok(sa_mode_arg.unwrap_or(false)) } +/// Resolve the effective minimum timeout from project and global configs. +/// +/// Priority: project `[execution].min_timeout_seconds` > global > compile-time default. +/// Config loading errors are silently ignored (fall back to compile-time default). +fn resolve_effective_min_timeout() -> u64 { + let compile_default = csa_config::ExecutionConfig::default_min_timeout(); + + // Try to load project config (merged with user-level). + // This is the same merged config that pipeline uses, so project overrides global + // via the standard TOML deep-merge path. + if let Ok(cwd) = std::env::current_dir() { + if let Ok(Some(config)) = csa_config::ProjectConfig::load(&cwd) { + if !config.execution.is_default() { + return config.execution.min_timeout_seconds; + } + } + } + + // Fall back to global config. + if let Ok(global) = csa_config::GlobalConfig::load() { + if !global.execution.is_default() { + return global.execution.min_timeout_seconds; + } + } + + compile_default +} + fn apply_sa_mode_prompt_guard(command: &Commands, current_depth: u32) -> anyhow::Result<()> { if command_sa_mode_arg(command).is_none() { return Ok(()); @@ -173,7 +201,12 @@ async fn run() -> Result<()> { let cli = Cli::parse(); let output_format = cli.format; let command = cli.command; - if let Err(err) = validate_command_args(&command) { + + // Resolve effective min_timeout_seconds from configs (project overrides global). + // This is a lightweight load; config errors are ignored (fall back to compile-time default). + let min_timeout = resolve_effective_min_timeout(); + + if let Err(err) = validate_command_args(&command, min_timeout) { err.exit(); } diff --git a/crates/cli-sub-agent/src/pipeline.rs b/crates/cli-sub-agent/src/pipeline.rs index caea8684..6bd1aeb2 100644 --- a/crates/cli-sub-agent/src/pipeline.rs +++ b/crates/cli-sub-agent/src/pipeline.rs @@ -18,6 +18,9 @@ use csa_executor::{AcpMcpServerConfig, Executor}; use csa_hooks::{HookEvent, run_hooks_for_event}; use csa_process::{ExecutionResult, check_tool_installed}; +#[path = "pipeline_gate.rs"] +pub(crate) mod gate; + #[path = "pipeline_prompt_guard.rs"] pub(crate) mod prompt_guard; diff --git a/crates/cli-sub-agent/src/pipeline_gate.rs b/crates/cli-sub-agent/src/pipeline_gate.rs new file mode 100644 index 00000000..d4a825df --- /dev/null +++ b/crates/cli-sub-agent/src/pipeline_gate.rs @@ -0,0 +1,245 @@ +//! Pre-review quality gate: detect and run pre-commit hooks or custom gate +//! commands before `csa review` / `csa debate` to catch lint/test failures early. +//! +//! Detection order (strict): +//! 1. Explicit `gate_command` from project config +//! 2. `git config core.hooksPath` → `/pre-commit` +//! 3. No gate found → skip with debug log +//! +//! When `CSA_DEPTH > 0`, the gate is skipped entirely to prevent recursion. + +use std::path::Path; +use std::process::Stdio; + +use anyhow::Result; +use tokio::process::Command; +use tracing::{debug, warn}; + +use csa_config::GateMode; + +/// Result of running a quality gate command. +#[derive(Debug, Clone)] +pub(crate) struct GateResult { + /// The command that was executed. + pub command: String, + /// Process exit code (None if killed by signal). + pub exit_code: Option, + /// Captured stdout. + pub stdout: String, + /// Captured stderr. + pub stderr: String, + /// Whether the gate was skipped (depth > 0, no gate found, etc.). + pub skipped: bool, + /// Human-readable reason when skipped. + pub skip_reason: Option, +} + +impl GateResult { + fn skipped(reason: &str) -> Self { + Self { + command: String::new(), + exit_code: None, + stdout: String::new(), + stderr: String::new(), + skipped: true, + skip_reason: Some(reason.to_string()), + } + } + + /// Whether the gate passed (either skipped or exit code 0). + pub fn passed(&self) -> bool { + self.skipped || self.exit_code == Some(0) + } +} + +/// Evaluate the pre-review quality gate. +/// +/// Returns `GateResult` describing what happened. The caller decides whether +/// to abort based on `gate_mode` and the result. +/// +/// # Gate detection order +/// 1. `gate_command` from config (explicit override) +/// 2. `git config core.hooksPath` → `/pre-commit` (if executable) +/// 3. No gate found → skip +/// +/// # Recursion guard +/// When `CSA_DEPTH > 0`, the gate is always skipped to prevent recursive +/// quality checks when CSA spawns sub-agents. +pub(crate) async fn evaluate_quality_gate( + project_root: &Path, + gate_command: Option<&str>, + gate_timeout_secs: u64, + gate_mode: &GateMode, +) -> Result { + // Recursion guard: skip when running as a sub-agent + let depth: u32 = std::env::var("CSA_DEPTH") + .ok() + .and_then(|v| v.parse().ok()) + .unwrap_or(0); + if depth > 0 { + debug!(depth, "Skipping quality gate (CSA_DEPTH > 0)"); + return Ok(GateResult::skipped("CSA_DEPTH > 0 (sub-agent)")); + } + + // Step 1: Resolve gate command + let resolved_command = match gate_command { + Some(cmd) => { + debug!(command = cmd, "Using explicit gate_command from config"); + cmd.to_string() + } + None => match detect_git_hooks_pre_commit(project_root).await? { + Some(cmd) => { + debug!(command = %cmd, "Detected pre-commit hook via core.hooksPath"); + cmd + } + None => { + debug!("No quality gate found; skipping"); + return Ok(GateResult::skipped( + "no gate command configured or detected", + )); + } + }, + }; + + // Step 2: Execute the gate command + execute_gate_command( + &resolved_command, + project_root, + gate_timeout_secs, + gate_mode, + ) + .await +} + +/// Detect pre-commit hook via `git config core.hooksPath`. +/// +/// Returns the shell command to execute the pre-commit hook, or None if +/// `core.hooksPath` is not set or the pre-commit script doesn't exist. +/// +/// Intentionally does NOT fall back to `.git/hooks/` — this supports +/// monorepo/submodule shared hooks configurations. +async fn detect_git_hooks_pre_commit(project_root: &Path) -> Result> { + let output = Command::new("git") + .args(["config", "core.hooksPath"]) + .current_dir(project_root) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .output() + .await?; + + if !output.status.success() { + // core.hooksPath is not set + return Ok(None); + } + + let hooks_path = String::from_utf8_lossy(&output.stdout).trim().to_string(); + if hooks_path.is_empty() { + return Ok(None); + } + + // Resolve relative paths against project root + let hooks_dir = if Path::new(&hooks_path).is_absolute() { + std::path::PathBuf::from(&hooks_path) + } else { + project_root.join(&hooks_path) + }; + + let pre_commit = hooks_dir.join("pre-commit"); + if pre_commit.exists() { + // Return the path as a shell command + Ok(Some(pre_commit.display().to_string())) + } else { + debug!( + path = %pre_commit.display(), + "core.hooksPath set but pre-commit script not found" + ); + Ok(None) + } +} + +/// Execute a gate command with timeout and process group management. +/// +/// Reuses the same patterns from `csa-hooks/src/runner.rs`: +/// - `sh -c` execution +/// - `process_group(0)` for clean kill +/// - Negative-PID signal propagation on timeout +async fn execute_gate_command( + command: &str, + project_root: &Path, + timeout_secs: u64, + gate_mode: &GateMode, +) -> Result { + let mut cmd = Command::new("sh"); + cmd.arg("-c") + .arg(command) + .current_dir(project_root) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + + // Create new process group for clean timeout kill. + // tokio::process::Command::process_group(0) calls setsid in the child, + // allowing timeout to kill the entire group via negative PID. + #[cfg(unix)] + cmd.process_group(0); + + let child = cmd.spawn()?; + let timeout = std::time::Duration::from_secs(timeout_secs); + + match tokio::time::timeout(timeout, child.wait_with_output()).await { + Ok(Ok(output)) => { + let exit_code = output.status.code(); + let stdout = String::from_utf8_lossy(&output.stdout).to_string(); + let stderr = String::from_utf8_lossy(&output.stderr).to_string(); + + let result = GateResult { + command: command.to_string(), + exit_code, + stdout, + stderr, + skipped: false, + skip_reason: None, + }; + + if !result.passed() { + match gate_mode { + GateMode::Monitor => { + warn!( + exit_code = ?exit_code, + "Quality gate failed (monitor mode, not blocking)" + ); + } + GateMode::CriticalOnly | GateMode::Full => { + // Caller will handle abort based on gate_mode + } + } + } + + Ok(result) + } + Ok(Err(e)) => { + anyhow::bail!("Quality gate command failed to execute: {e}"); + } + Err(_elapsed) => { + // Timeout: kill the process group + // Note: the child has already been consumed by wait_with_output, + // but the process group may still have orphaned children. + // In practice, tokio drops the child handle which sends SIGKILL. + warn!( + timeout_secs, + command, "Quality gate timed out after {timeout_secs}s" + ); + Ok(GateResult { + command: command.to_string(), + exit_code: None, + stdout: String::new(), + stderr: format!("Quality gate timed out after {timeout_secs}s"), + skipped: false, + skip_reason: None, + }) + } + } +} + +#[cfg(test)] +#[path = "pipeline_gate_tests.rs"] +mod tests; diff --git a/crates/cli-sub-agent/src/pipeline_gate_tests.rs b/crates/cli-sub-agent/src/pipeline_gate_tests.rs new file mode 100644 index 00000000..b11b9187 --- /dev/null +++ b/crates/cli-sub-agent/src/pipeline_gate_tests.rs @@ -0,0 +1,334 @@ +use super::*; + +/// Set CSA_DEPTH env var for test isolation. +/// +/// # Safety +/// Tests using this must be run with `--test-threads=1` or accept +/// that env mutations are process-global. All gate tests restore the +/// env var after use. +unsafe fn set_depth(val: &str) { + // SAFETY: Test-only; env var is restored after each test. + unsafe { std::env::set_var("CSA_DEPTH", val) }; +} + +/// Remove CSA_DEPTH env var after test. +/// +/// # Safety +/// Same concurrency caveat as `set_depth`. +unsafe fn clear_depth() { + // SAFETY: Test-only; restoring env to clean state. + unsafe { std::env::remove_var("CSA_DEPTH") }; +} + +// --------------------------------------------------------------------------- +// GateResult helpers +// --------------------------------------------------------------------------- + +#[test] +fn test_gate_result_skipped_is_passed() { + let result = GateResult::skipped("test reason"); + assert!(result.skipped); + assert!(result.passed()); + assert_eq!(result.skip_reason.as_deref(), Some("test reason")); + assert!(result.command.is_empty()); +} + +#[test] +fn test_gate_result_exit_0_is_passed() { + let result = GateResult { + command: "true".to_string(), + exit_code: Some(0), + stdout: String::new(), + stderr: String::new(), + skipped: false, + skip_reason: None, + }; + assert!(result.passed()); +} + +#[test] +fn test_gate_result_exit_1_is_not_passed() { + let result = GateResult { + command: "false".to_string(), + exit_code: Some(1), + stdout: String::new(), + stderr: String::new(), + skipped: false, + skip_reason: None, + }; + assert!(!result.passed()); +} + +#[test] +fn test_gate_result_no_exit_code_is_not_passed() { + let result = GateResult { + command: "killed".to_string(), + exit_code: None, + stdout: String::new(), + stderr: String::new(), + skipped: false, + skip_reason: None, + }; + assert!(!result.passed()); +} + +// --------------------------------------------------------------------------- +// evaluate_quality_gate +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn test_gate_skipped_when_csa_depth_set() { + // SAFETY: Test-only env mutation; gate tests are not parallel-safe by nature. + unsafe { set_depth("1") }; + let dir = tempfile::tempdir().unwrap(); + + let result = evaluate_quality_gate( + dir.path(), + Some("echo should-not-run"), + 300, + &GateMode::Full, + ) + .await + .unwrap(); + + assert!(result.skipped); + assert!(result.passed()); + assert!(result.skip_reason.as_deref().unwrap().contains("CSA_DEPTH")); + + // SAFETY: Restoring env to clean state. + unsafe { clear_depth() }; +} + +#[tokio::test] +async fn test_gate_skipped_when_no_command_and_no_hooks_path() { + // SAFETY: Test-only env mutation. + unsafe { set_depth("0") }; + let dir = tempfile::tempdir().unwrap(); + + // Initialize a git repo without core.hooksPath + tokio::process::Command::new("git") + .args(["init"]) + .current_dir(dir.path()) + .output() + .await + .unwrap(); + + let result = evaluate_quality_gate(dir.path(), None, 300, &GateMode::Full) + .await + .unwrap(); + + assert!(result.skipped); + assert!(result.passed()); + + // SAFETY: Restoring env. + unsafe { clear_depth() }; +} + +#[tokio::test] +async fn test_gate_runs_explicit_command_success() { + // SAFETY: Test-only env mutation. + unsafe { set_depth("0") }; + let dir = tempfile::tempdir().unwrap(); + + let result = + evaluate_quality_gate(dir.path(), Some("echo 'gate passed'"), 300, &GateMode::Full) + .await + .unwrap(); + + assert!(!result.skipped); + assert!(result.passed()); + assert_eq!(result.exit_code, Some(0)); + assert!(result.stdout.contains("gate passed")); + + // SAFETY: Restoring env. + unsafe { clear_depth() }; +} + +#[tokio::test] +async fn test_gate_runs_explicit_command_failure() { + // SAFETY: Test-only env mutation. + unsafe { set_depth("0") }; + let dir = tempfile::tempdir().unwrap(); + + let result = evaluate_quality_gate( + dir.path(), + Some("echo 'lint error' >&2; exit 1"), + 300, + &GateMode::Full, + ) + .await + .unwrap(); + + assert!(!result.skipped); + assert!(!result.passed()); + assert_eq!(result.exit_code, Some(1)); + assert!(result.stderr.contains("lint error")); + + // SAFETY: Restoring env. + unsafe { clear_depth() }; +} + +#[tokio::test] +async fn test_gate_timeout() { + // SAFETY: Test-only env mutation. + unsafe { set_depth("0") }; + let dir = tempfile::tempdir().unwrap(); + + let result = evaluate_quality_gate( + dir.path(), + Some("sleep 60"), + 1, // 1 second timeout + &GateMode::Full, + ) + .await + .unwrap(); + + assert!(!result.skipped); + assert!(!result.passed()); + assert!(result.stderr.contains("timed out")); + + // SAFETY: Restoring env. + unsafe { clear_depth() }; +} + +#[tokio::test] +async fn test_gate_monitor_mode_still_runs() { + // SAFETY: Test-only env mutation. + unsafe { set_depth("0") }; + let dir = tempfile::tempdir().unwrap(); + + // Even in monitor mode, the gate runs — the mode only affects how callers + // handle the result. + let result = evaluate_quality_gate(dir.path(), Some("exit 1"), 300, &GateMode::Monitor) + .await + .unwrap(); + + assert!(!result.skipped); + assert!(!result.passed()); + // In monitor mode the function still returns a GateResult; caller decides. + + // SAFETY: Restoring env. + unsafe { clear_depth() }; +} + +#[tokio::test] +async fn test_gate_captures_stdout_and_stderr() { + // SAFETY: Test-only env mutation. + unsafe { set_depth("0") }; + let dir = tempfile::tempdir().unwrap(); + + let result = evaluate_quality_gate( + dir.path(), + Some("echo 'stdout-content'; echo 'stderr-content' >&2"), + 300, + &GateMode::Full, + ) + .await + .unwrap(); + + assert!(result.passed()); + assert!(result.stdout.contains("stdout-content")); + assert!(result.stderr.contains("stderr-content")); + + // SAFETY: Restoring env. + unsafe { clear_depth() }; +} + +// --------------------------------------------------------------------------- +// detect_git_hooks_pre_commit +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn test_detect_no_hooks_path() { + let dir = tempfile::tempdir().unwrap(); + tokio::process::Command::new("git") + .args(["init"]) + .current_dir(dir.path()) + .output() + .await + .unwrap(); + + let result = detect_git_hooks_pre_commit(dir.path()).await.unwrap(); + assert!(result.is_none()); +} + +#[tokio::test] +async fn test_detect_hooks_path_with_pre_commit() { + let dir = tempfile::tempdir().unwrap(); + tokio::process::Command::new("git") + .args(["init"]) + .current_dir(dir.path()) + .output() + .await + .unwrap(); + + // Create a hooks directory with a pre-commit script + let hooks_dir = dir.path().join("my-hooks"); + std::fs::create_dir_all(&hooks_dir).unwrap(); + let pre_commit = hooks_dir.join("pre-commit"); + std::fs::write(&pre_commit, "#!/bin/sh\nexit 0\n").unwrap(); + + // Set core.hooksPath + tokio::process::Command::new("git") + .args(["config", "core.hooksPath", hooks_dir.to_str().unwrap()]) + .current_dir(dir.path()) + .output() + .await + .unwrap(); + + let result = detect_git_hooks_pre_commit(dir.path()).await.unwrap(); + assert!(result.is_some()); + assert!(result.unwrap().contains("pre-commit")); +} + +#[tokio::test] +async fn test_detect_hooks_path_without_pre_commit() { + let dir = tempfile::tempdir().unwrap(); + tokio::process::Command::new("git") + .args(["init"]) + .current_dir(dir.path()) + .output() + .await + .unwrap(); + + // Create hooks directory but NO pre-commit script + let hooks_dir = dir.path().join("my-hooks"); + std::fs::create_dir_all(&hooks_dir).unwrap(); + + tokio::process::Command::new("git") + .args(["config", "core.hooksPath", hooks_dir.to_str().unwrap()]) + .current_dir(dir.path()) + .output() + .await + .unwrap(); + + let result = detect_git_hooks_pre_commit(dir.path()).await.unwrap(); + assert!(result.is_none()); +} + +#[tokio::test] +async fn test_detect_hooks_path_relative() { + let dir = tempfile::tempdir().unwrap(); + tokio::process::Command::new("git") + .args(["init"]) + .current_dir(dir.path()) + .output() + .await + .unwrap(); + + // Create relative hooks directory + let hooks_dir = dir.path().join("relative-hooks"); + std::fs::create_dir_all(&hooks_dir).unwrap(); + std::fs::write(hooks_dir.join("pre-commit"), "#!/bin/sh\nexit 0\n").unwrap(); + + // Set relative path + tokio::process::Command::new("git") + .args(["config", "core.hooksPath", "relative-hooks"]) + .current_dir(dir.path()) + .output() + .await + .unwrap(); + + let result = detect_git_hooks_pre_commit(dir.path()).await.unwrap(); + assert!(result.is_some()); +} diff --git a/crates/cli-sub-agent/src/pipeline_session_exec.rs b/crates/cli-sub-agent/src/pipeline_session_exec.rs index b9267aea..7c470902 100644 --- a/crates/cli-sub-agent/src/pipeline_session_exec.rs +++ b/crates/cli-sub-agent/src/pipeline_session_exec.rs @@ -469,6 +469,37 @@ pub(crate) async fn execute_with_session_and_meta_with_parent_source( Some(&merged_env) }; + // Build runtime overrides from .csa/config.toml [hooks] section. + // These take PRIORITY over hooks.toml PreRun/PostRun entries. + let project_hook_overrides = config.filter(|c| !c.hooks.is_default()).map(|c| { + let mut overrides = std::collections::HashMap::new(); + if let Some(ref cmd) = c.hooks.pre_run { + overrides.insert( + "pre_run".to_string(), + csa_hooks::HookConfig { + enabled: true, + command: Some(cmd.clone()), + timeout_secs: c.hooks.timeout_secs, + fail_policy: csa_hooks::FailPolicy::default(), + waivers: Vec::new(), + }, + ); + } + if let Some(ref cmd) = c.hooks.post_run { + overrides.insert( + "post_run".to_string(), + csa_hooks::HookConfig { + enabled: true, + command: Some(cmd.clone()), + timeout_secs: c.hooks.timeout_secs, + fail_policy: csa_hooks::FailPolicy::default(), + waivers: Vec::new(), + }, + ); + } + overrides + }); + // Load hooks config once, reused by PreRun, PostRun, and SessionComplete hooks. let hooks_config = load_hooks_config( csa_session::get_session_root(project_root) @@ -476,7 +507,7 @@ pub(crate) async fn execute_with_session_and_meta_with_parent_source( .map(|r| r.join("hooks.toml")) .as_deref(), global_hooks_path().as_deref(), - None, + project_hook_overrides.as_ref(), ); // PreRun hook: fires before tool execution starts. let sessions_root = session_dir diff --git a/crates/cli-sub-agent/src/pipeline_tests.rs b/crates/cli-sub-agent/src/pipeline_tests.rs index daab06b5..856a786a 100644 --- a/crates/cli-sub-agent/src/pipeline_tests.rs +++ b/crates/cli-sub-agent/src/pipeline_tests.rs @@ -111,6 +111,8 @@ fn resolve_idle_timeout_prefers_cli_override() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; assert_eq!(resolve_idle_timeout_seconds(Some(&cfg), Some(42)), 42); @@ -241,6 +243,8 @@ fn resolve_idle_timeout_uses_config_then_default() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; assert_eq!(resolve_idle_timeout_seconds(Some(&cfg), None), 222); @@ -274,6 +278,8 @@ fn resolve_liveness_dead_seconds_uses_config_then_default() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; assert_eq!(resolve_liveness_dead_seconds(Some(&cfg)), 42); @@ -411,6 +417,8 @@ fn test_config_with_node_heap_limit(node_heap_limit_mb: Option) -> ProjectC preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), } } @@ -616,6 +624,8 @@ fn config_with_tier_for_tool(_tool_prefix: &str, model_spec: &str) -> ProjectCon preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), } } diff --git a/crates/cli-sub-agent/src/pipeline_tests_tail.rs b/crates/cli-sub-agent/src/pipeline_tests_tail.rs index a261424e..10483bbe 100644 --- a/crates/cli-sub-agent/src/pipeline_tests_tail.rs +++ b/crates/cli-sub-agent/src/pipeline_tests_tail.rs @@ -19,6 +19,8 @@ async fn build_and_validate_executor_no_tiers_both_flags_equivalent() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; let result_true = build_and_validate_executor( diff --git a/crates/cli-sub-agent/src/pipeline_tests_thinking.rs b/crates/cli-sub-agent/src/pipeline_tests_thinking.rs index 8f4bebec..ba71074f 100644 --- a/crates/cli-sub-agent/src/pipeline_tests_thinking.rs +++ b/crates/cli-sub-agent/src/pipeline_tests_thinking.rs @@ -31,6 +31,8 @@ async fn thinking_lock_project_config_overrides_cli_thinking() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; let result = build_and_validate_executor( @@ -126,6 +128,8 @@ async fn thinking_lock_project_overrides_global() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; let mut global_tools = HashMap::new(); @@ -217,6 +221,8 @@ async fn project_default_thinking_applies_when_cli_absent() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; let result = build_and_validate_executor( @@ -280,6 +286,8 @@ async fn project_default_model_is_checked_against_tiers_when_enabled() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; let result = build_and_validate_executor( @@ -341,6 +349,8 @@ async fn project_default_model_is_ignored_when_tool_defaults_disabled() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; let result = build_and_validate_executor( diff --git a/crates/cli-sub-agent/src/pipeline_transcript.rs b/crates/cli-sub-agent/src/pipeline_transcript.rs index bb612ede..c107ce9b 100644 --- a/crates/cli-sub-agent/src/pipeline_transcript.rs +++ b/crates/cli-sub-agent/src/pipeline_transcript.rs @@ -80,6 +80,8 @@ mod tests { tool_aliases: HashMap::new(), preferences: None, memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), } } diff --git a/crates/cli-sub-agent/src/review_cmd.rs b/crates/cli-sub-agent/src/review_cmd.rs index 0727ca9b..c945355e 100644 --- a/crates/cli-sub-agent/src/review_cmd.rs +++ b/crates/cli-sub-agent/src/review_cmd.rs @@ -1,34 +1,40 @@ -use std::path::{Path, PathBuf}; +use std::path::Path; use anyhow::{Context, Result}; use tokio::task::JoinSet; use tracing::{debug, error, info, warn}; -use crate::cli::{ReviewArgs, ReviewMode}; +use crate::cli::ReviewArgs; use crate::review_consensus::{ - CLEAN, agreement_level, build_consolidated_artifact, build_multi_reviewer_instruction, - build_reviewer_tools, consensus_strategy_label, consensus_verdict, parse_consensus_strategy, - parse_review_verdict, resolve_consensus, write_consolidated_artifact, + CLEAN, agreement_level, build_multi_reviewer_instruction, build_reviewer_tools, + consensus_strategy_label, consensus_verdict, parse_consensus_strategy, parse_review_verdict, + resolve_consensus, }; #[cfg(test)] use crate::review_context::discover_review_context_for_branch; -use crate::review_context::{ - ResolvedReviewContext, ResolvedReviewContextKind, render_spec_review_context, - resolve_review_context, -}; -use crate::review_routing::{ - ReviewRoutingMetadata, detect_review_routing_metadata, persist_review_routing_artifact, -}; -use csa_config::global::{heterogeneous_counterpart, select_heterogeneous_tool}; +use crate::review_context::resolve_review_context; +#[cfg(test)] +use crate::review_context::{ResolvedReviewContext, ResolvedReviewContextKind}; +use crate::review_routing::{ReviewRoutingMetadata, persist_review_routing_artifact}; use csa_config::{GlobalConfig, ProjectConfig}; use csa_core::consensus::AgentResponse; use csa_core::types::{OutputFormat, ToolName}; -use csa_session::review_artifact::ReviewArtifact; #[path = "review_cmd_output.rs"] mod output; use output::sanitize_review_output; +#[path = "review_cmd_resolve.rs"] +mod resolve; +#[cfg(test)] +use resolve::build_review_instruction; +use resolve::{ + ANTI_RECURSION_PREAMBLE, build_review_instruction_for_project, derive_scope, + resolve_review_stream_mode, resolve_review_thinking, resolve_review_tool, + review_scope_allows_auto_discovery, verify_review_skill_available, + write_multi_reviewer_consolidated_artifact, +}; + #[derive(Debug, Clone)] struct ReviewerOutcome { reviewer_index: usize, @@ -52,6 +58,64 @@ pub(crate) async fn handle_review(args: ReviewArgs, current_depth: u32) -> Resul // 2b. Verify review skill is available (fail fast before any execution) verify_review_skill_available(&project_root, args.allow_fallback)?; + // 2c. Run pre-review quality gate (after skill check, before tool execution) + { + let gate_command = config + .as_ref() + .and_then(|c| c.review.as_ref()) + .and_then(|r| r.gate_command.as_deref()); + let gate_timeout = config + .as_ref() + .and_then(|c| c.review.as_ref()) + .map(|r| r.gate_timeout_secs) + .unwrap_or_else(csa_config::ReviewConfig::default_gate_timeout); + let gate_mode = &global_config.review.gate_mode; + + let gate_result = crate::pipeline::gate::evaluate_quality_gate( + &project_root, + gate_command, + gate_timeout, + gate_mode, + ) + .await?; + + if gate_result.skipped { + debug!( + reason = gate_result.skip_reason.as_deref().unwrap_or("unknown"), + "Quality gate skipped" + ); + } else if !gate_result.passed() { + match gate_mode { + csa_config::GateMode::Monitor => { + warn!( + command = %gate_result.command, + exit_code = ?gate_result.exit_code, + "Quality gate failed (monitor mode — continuing with review)" + ); + } + csa_config::GateMode::CriticalOnly | csa_config::GateMode::Full => { + let mut msg = format!( + "Pre-review quality gate failed (mode={gate_mode:?}).\n\ + Command: {}\nExit code: {:?}", + gate_result.command, gate_result.exit_code + ); + if !gate_result.stdout.is_empty() { + msg.push_str(&format!("\n--- stdout ---\n{}", gate_result.stdout)); + } + if !gate_result.stderr.is_empty() { + msg.push_str(&format!("\n--- stderr ---\n{}", gate_result.stderr)); + } + anyhow::bail!(msg); + } + } + } else { + debug!( + command = %gate_result.command, + "Quality gate passed" + ); + } + } + // 3. Derive scope and mode from CLI args let scope = derive_scope(&args); let mode = if args.fix { @@ -118,12 +182,12 @@ pub(crate) async fn handle_review(args: ReviewArgs, current_depth: u32) -> Resul crate::pipeline::resolve_idle_timeout_seconds(config.as_ref(), args.idle_timeout); if args.reviewers == 1 { - // Keep single-reviewer behavior unchanged. + // Single-reviewer path (with optional --fix loop). let review_future = execute_review( tool, - prompt, - args.session, - args.model, + prompt.clone(), + args.session.clone(), + args.model.clone(), tier_model_spec.clone(), review_thinking.clone(), format!( @@ -159,8 +223,109 @@ pub(crate) async fn handle_review(args: ReviewArgs, current_depth: u32) -> Resul review_future.await? }; - print!("{}", sanitize_review_output(&result.output)); - return Ok(result.exit_code); + print!("{}", sanitize_review_output(&result.execution.output)); + + // If --fix is not enabled or review found no issues, return immediately. + let verdict = parse_review_verdict(&result.execution.output, result.execution.exit_code); + if !args.fix || verdict == CLEAN { + return Ok(result.execution.exit_code); + } + + // --- Fix loop: resume the review session to apply fixes, then re-gate --- + let max_rounds = args.max_rounds; + let mut session_id = result.meta_session_id.clone(); + + for round in 1..=max_rounds { + info!(round, max_rounds, session_id = %session_id, "Fix round starting"); + + // Step 1: Resume the review session with a fix prompt + let fix_prompt = format!( + "{ANTI_RECURSION_PREAMBLE}\ + Fix round {round}/{max_rounds}.\n\ + Fix all issues found in the review. Run formatting and linting commands as needed.\n\ + After applying fixes, verify the changes compile and pass basic checks.\n\ + If no issues remain, emit verdict: CLEAN." + ); + + let fix_future = execute_review( + tool, + fix_prompt, + Some(session_id.clone()), + args.model.clone(), + tier_model_spec.clone(), + review_thinking.clone(), + format!("fix round {round}/{max_rounds}"), + &project_root, + config.as_ref(), + &global_config, + review_routing.clone(), + stream_mode, + idle_timeout_seconds, + args.force_override_user_config, + ); + + let fix_result = if let Some(timeout_secs) = args.timeout { + match tokio::time::timeout(std::time::Duration::from_secs(timeout_secs), fix_future) + .await + { + Ok(inner) => inner?, + Err(_) => { + error!( + timeout_secs = timeout_secs, + round, "Fix round aborted: wall-clock timeout exceeded" + ); + anyhow::bail!( + "Fix round {round}/{max_rounds} aborted: --timeout {timeout_secs}s exceeded." + ); + } + } + } else { + fix_future.await? + }; + + print!("{}", sanitize_review_output(&fix_result.execution.output)); + session_id = fix_result.meta_session_id.clone(); + + // Step 2: Run the quality gate after fix + let gate_command = config + .as_ref() + .and_then(|c| c.review.as_ref()) + .and_then(|r| r.gate_command.as_deref()); + let gate_timeout = config + .as_ref() + .and_then(|c| c.review.as_ref()) + .map(|r| r.gate_timeout_secs) + .unwrap_or_else(csa_config::ReviewConfig::default_gate_timeout); + let gate_mode = &global_config.review.gate_mode; + + let gate_result = crate::pipeline::gate::evaluate_quality_gate( + &project_root, + gate_command, + gate_timeout, + gate_mode, + ) + .await?; + + if gate_result.passed() { + info!(round, "Fix round succeeded — quality gate passed"); + return Ok(0); + } + + warn!( + round, + max_rounds, + command = %gate_result.command, + exit_code = ?gate_result.exit_code, + "Quality gate still failing after fix round" + ); + } + + // All fix rounds exhausted; gate still fails. + error!( + max_rounds, + "All fix rounds exhausted — quality gate still failing" + ); + return Ok(1); } if args.fix { @@ -207,7 +372,7 @@ pub(crate) async fn handle_review(args: ReviewArgs, current_depth: u32) -> Resul }; let reviewer_thinking = review_thinking.clone(); join_set.spawn(async move { - let result = execute_review( + let session_result = execute_review( reviewer_tool, reviewer_prompt, None, @@ -224,6 +389,7 @@ pub(crate) async fn handle_review(args: ReviewArgs, current_depth: u32) -> Resul reviewer_force_override, ) .await?; + let result = &session_result.execution; Ok::(ReviewerOutcome { reviewer_index, tool: reviewer_tool, @@ -336,7 +502,7 @@ async fn execute_review( stream_mode: csa_process::StreamMode, idle_timeout_seconds: u64, force_override_user_config: bool, -) -> Result { +) -> Result { let executor = crate::pipeline::build_and_validate_executor( &tool, tier_model_spec.as_deref(), @@ -398,366 +564,7 @@ async fn execute_review( persist_review_routing_artifact(project_root, &execution.meta_session_id, &review_routing); - Ok(execution.execution) -} - -/// Verify the review pattern is installed before attempting execution. -/// -/// By default this fails fast with actionable install guidance if the pattern -/// is missing. When `allow_fallback` is true, it downgrades to warning and -/// lets review continue without the structured pattern protocol. -fn verify_review_skill_available(project_root: &Path, allow_fallback: bool) -> Result<()> { - match crate::pattern_resolver::resolve_pattern("csa-review", project_root) { - Ok(resolved) => { - debug!( - pattern_dir = %resolved.dir.display(), - has_config = resolved.config.is_some(), - has_agent = resolved.agent_config().is_some(), - skill_md_len = resolved.skill_md.len(), - "Review pattern resolved" - ); - Ok(()) - } - Err(resolve_err) => { - if allow_fallback { - warn!( - "Review pattern not found; continuing because --allow-fallback is set. \ - Install with `weave install RyderFreeman4Logos/cli-sub-agent` for structured review protocol." - ); - return Ok(()); - } - - anyhow::bail!( - "Review pattern not found — `csa review` requires the 'csa-review' pattern.\n\n\ - {resolve_err}\n\n\ - Install the review pattern with one of:\n\ - 1) weave install RyderFreeman4Logos/cli-sub-agent\n\ - 2) Manually place skills/csa-review/SKILL.md (or PATTERN.md) inside .csa/patterns/csa-review/ or patterns/csa-review/\n\n\ - Note: `csa skill install` only installs `.claude/skills/*`; it does NOT install `.csa/patterns/*`.\n\n\ - Without the pattern, the review tool cannot follow the structured review protocol." - ) - } - } -} - -/// Resolve stream mode for review command. -/// -/// - `--stream-stdout` forces TeeToStderr (progressive output) -/// - `--no-stream-stdout` forces BufferOnly (silent until complete) -/// - Default: BufferOnly to prevent raw provider noise from polluting review -/// output. Long-running progress is still surfaced by periodic heartbeats. -fn resolve_review_stream_mode( - stream_stdout: bool, - no_stream_stdout: bool, -) -> csa_process::StreamMode { - if no_stream_stdout { - csa_process::StreamMode::BufferOnly - } else if stream_stdout { - csa_process::StreamMode::TeeToStderr - } else { - csa_process::StreamMode::BufferOnly - } -} - -/// Returns (tool, optional_model_spec). When tier resolves, model_spec is set. -fn resolve_review_tool( - arg_tool: Option, - project_config: Option<&ProjectConfig>, - global_config: &GlobalConfig, - parent_tool: Option<&str>, - project_root: &Path, - force_override_user_config: bool, -) -> Result<(ToolName, Option)> { - // CLI --tool override always wins - if let Some(tool) = arg_tool { - if let Some(cfg) = project_config { - cfg.enforce_tool_enabled(tool.as_str(), force_override_user_config)?; - } - return Ok((tool, None)); - } - - // Tier-based resolution: project tier > global tier > tool-based fallback. - // Tier takes priority over tool when both are set. - let tier_name = project_config - .and_then(|cfg| cfg.review.as_ref()) - .and_then(|r| r.tier.as_deref()) - .or(global_config.review.tier.as_deref()); - - if let Some(tier) = tier_name { - if let Some(cfg) = project_config { - if let Some(resolution) = - crate::run_helpers::resolve_tool_from_tier(tier, cfg, parent_tool) - { - return Ok((resolution.tool, Some(resolution.model_spec))); - } - } - // Tier set but no available tool found — fall through to tool-based resolution - debug!( - tier = tier, - "Tier set but no available tool found, falling through to tool-based resolution" - ); - } - - if let Some(project_review) = project_config.and_then(|cfg| cfg.review.as_ref()) { - return resolve_review_tool_from_value( - &project_review.tool, - parent_tool, - project_config, - global_config, - project_root, - ) - .map(|t| (t, None)) - .with_context(|| { - format!( - "Failed to resolve review tool from project config: {}", - ProjectConfig::config_path(project_root).display() - ) - }); - } - - // When global [review].tool is "auto", always try heterogeneous auto-selection first. - if global_config.review.tool == "auto" { - if let Some(tool) = select_auto_review_tool(parent_tool, project_config, global_config) { - return Ok((tool, None)); - } - } - - match global_config.resolve_review_tool(parent_tool) { - Ok(tool_name) => { - if let Some(cfg) = project_config { - if !cfg.is_tool_enabled(&tool_name) { - return Err(review_auto_resolution_error(parent_tool, project_root)); - } - } - crate::run_helpers::parse_tool_name(&tool_name) - .map(|t| (t, None)) - .map_err(|_| { - anyhow::anyhow!( - "Invalid [review].tool value '{}'. Supported values: gemini-cli, opencode, codex, claude-code.", - tool_name - ) - }) - } - Err(_) => Err(review_auto_resolution_error(parent_tool, project_root)), - } -} - -/// Resolve review thinking: CLI > config review.thinking. -fn resolve_review_thinking( - cli_thinking: Option<&str>, - config_thinking: Option<&str>, -) -> Option { - cli_thinking - .map(str::to_string) - .or_else(|| config_thinking.map(str::to_string)) -} - -fn resolve_review_tool_from_value( - tool_value: &str, - parent_tool: Option<&str>, - project_config: Option<&ProjectConfig>, - global_config: &GlobalConfig, - project_root: &Path, -) -> Result { - if tool_value == "auto" { - if let Some(tool) = select_auto_review_tool(parent_tool, project_config, global_config) { - return Ok(tool); - } - - // Try old heterogeneous_counterpart first for backward compatibility, - // but only if the counterpart tool is enabled. - if let Some(resolved) = parent_tool.and_then(heterogeneous_counterpart) { - let counterpart_enabled = - project_config.is_none_or(|cfg| cfg.is_tool_enabled(resolved)); - if counterpart_enabled { - return crate::run_helpers::parse_tool_name(resolved).map_err(|_| { - anyhow::anyhow!( - "BUG: auto review tool resolution returned invalid tool '{}'", - resolved - ) - }); - } - } - - // Both methods failed - return Err(review_auto_resolution_error(parent_tool, project_root)); - } - - crate::run_helpers::parse_tool_name(tool_value).map_err(|_| { - anyhow::anyhow!( - "Invalid project [review].tool value '{}'. Supported values: auto, gemini-cli, opencode, codex, claude-code.", - tool_value - ) - }) -} - -fn select_auto_review_tool( - parent_tool: Option<&str>, - project_config: Option<&ProjectConfig>, - global_config: &GlobalConfig, -) -> Option { - let parent_str = parent_tool?; - let parent_tool_name = crate::run_helpers::parse_tool_name(parent_str).ok()?; - let enabled_tools: Vec<_> = if let Some(cfg) = project_config { - let tools: Vec<_> = csa_config::global::all_known_tools() - .iter() - .filter(|t| cfg.is_tool_auto_selectable(t.as_str())) - .copied() - .collect(); - csa_config::global::sort_tools_by_effective_priority(&tools, project_config, global_config) - } else { - csa_config::global::sort_tools_by_effective_priority( - csa_config::global::all_known_tools(), - project_config, - global_config, - ) - }; - - select_heterogeneous_tool(&parent_tool_name, &enabled_tools) -} - -fn review_auto_resolution_error(parent_tool: Option<&str>, project_root: &Path) -> anyhow::Error { - let parent = parent_tool.unwrap_or("").escape_default().to_string(); - let global_path = GlobalConfig::config_path() - .ok() - .map(|p| p.display().to_string()) - .unwrap_or_else(|| "~/.config/cli-sub-agent/config.toml".to_string()); - let project_path = ProjectConfig::config_path(project_root) - .display() - .to_string(); - - anyhow::anyhow!( - "AUTO review tool selection failed (tool = \"auto\").\n\n\ -STOP: Do not proceed. Ask the user to configure the review tool explicitly.\n\n\ -Parent tool context: {parent}\n\ -Supported auto mapping: claude-code <-> codex\n\n\ -Choose one:\n\ -1) Global config (user-level): {global_path}\n\ - [review]\n\ - tool = \"codex\" # or \"claude-code\", \"opencode\", \"gemini-cli\"\n\ -2) Project config override: {project_path}\n\ - [review]\n\ - tool = \"codex\" # or \"claude-code\", \"opencode\", \"gemini-cli\"\n\ -3) CLI override: csa review --sa-mode --tool codex\n\n\ -Reason: CSA enforces heterogeneity in auto mode and will not fall back." - ) -} - -fn write_multi_reviewer_consolidated_artifact(reviewers: usize) -> Result<()> { - let Some(session_dir) = std::env::var_os("CSA_SESSION_DIR") else { - return Ok(()); - }; - let output_dir = PathBuf::from(session_dir); - let session_id = std::env::var("CSA_SESSION_ID").unwrap_or_else(|_| "unknown".to_string()); - - let mut reviewer_artifacts = Vec::new(); - for reviewer_index in 1..=reviewers { - let artifact_path = output_dir - .join(format!("reviewer-{reviewer_index}")) - .join("review-findings.json"); - - if !artifact_path.exists() { - continue; - } - - let content = std::fs::read_to_string(&artifact_path) - .with_context(|| format!("failed to read {}", artifact_path.display()))?; - let artifact: ReviewArtifact = serde_json::from_str(&content) - .with_context(|| format!("failed to parse {}", artifact_path.display()))?; - reviewer_artifacts.push(artifact); - } - - let consolidated = build_consolidated_artifact(reviewer_artifacts, &session_id); - write_consolidated_artifact(&consolidated, &output_dir) -} - -/// Derive the review scope string from CLI arguments. -/// -/// Priority order (first match wins): -/// 1. `--range ...` → "range:..." -/// 2. `--files ` → "files:" -/// 3. `--commit ` → "commit:" -/// 4. `--diff` → "uncommitted" -/// 5. default → "base:" (branch defaults to "main") -fn derive_scope(args: &ReviewArgs) -> String { - if let Some(ref range) = args.range { - return format!("range:{range}"); - } - if let Some(ref files) = args.files { - return format!("files:{files}"); - } - if let Some(ref commit) = args.commit { - return format!("commit:{commit}"); - } - if args.diff { - return "uncommitted".to_string(); - } - format!("base:{}", args.branch.as_deref().unwrap_or("main")) -} - -fn review_scope_allows_auto_discovery(args: &ReviewArgs) -> bool { - args.range.is_some() || (!args.diff && args.commit.is_none() && args.files.is_none()) -} - -/// Anti-recursion preamble injected into every review/debate subprocess prompt. -/// -/// Prevents the spawned tool (e.g. claude-code-acp) from reading CLAUDE.md rules -/// that say "use csa review" and recursively invoking `csa run` / `csa review`. -const ANTI_RECURSION_PREAMBLE: &str = "\ -CRITICAL: You are running INSIDE a CSA subprocess (csa review / csa debate). \ -Do NOT invoke `csa run`, `csa review`, `csa debate`, or ANY `csa` CLI command — \ -this causes infinite recursion. Perform the task DIRECTLY using your own \ -capabilities (Read, Grep, Glob, Bash for read-only git commands). \ -REVIEW-ONLY SAFETY: Do NOT run git add/commit/push/merge/rebase/tag/stash/reset/checkout/cherry-pick, \ -and do NOT run gh pr/create/comment/merge or any command that mutates repository/PR state. \ -Ignore prompt-guard reminders about commit/push in this subprocess. \ -Ignore any CLAUDE.md or AGENTS.md rules that instruct you to delegate to CSA.\n\n"; - -/// Build a concise review instruction that tells the tool to use the csa-review skill. -/// -/// The tool loads the skill from `.claude/skills/csa-review/` automatically. -/// CSA only passes scope, mode, and optional parameters — no diff content. -/// An anti-recursion preamble is prepended to prevent the spawned tool from -/// re-invoking CSA commands (see GitHub issue #272). -fn build_review_instruction( - scope: &str, - mode: &str, - security_mode: &str, - review_mode: ReviewMode, - context: Option<&ResolvedReviewContext>, -) -> String { - let mut instruction = format!( - "{ANTI_RECURSION_PREAMBLE}Use the csa-review skill. scope={scope}, mode={mode}, security_mode={security_mode}, review_mode={review_mode}." - ); - if let Some(ctx) = context { - instruction.push_str(&format!(" context={}", ctx.path)); - if let ResolvedReviewContextKind::SpecToml { spec } = &ctx.kind { - instruction.push_str( - "\nSpec alignment context (parsed from spec.toml; use this criteria set directly):\n", - ); - instruction.push_str(&render_spec_review_context(spec)); - } - } - instruction -} - -fn build_review_instruction_for_project( - scope: &str, - mode: &str, - security_mode: &str, - review_mode: ReviewMode, - context: Option<&ResolvedReviewContext>, - project_root: &Path, - project_config: Option<&ProjectConfig>, -) -> (String, ReviewRoutingMetadata) { - let review_routing = detect_review_routing_metadata(project_root, project_config); - let mut instruction = - build_review_instruction(scope, mode, security_mode, review_mode, context); - instruction.push_str(&format!( - "\n[project_profile: {}]", - review_routing.project_profile - )); - (instruction, review_routing) + Ok(execution) } #[cfg(test)] diff --git a/crates/cli-sub-agent/src/review_cmd_resolve.rs b/crates/cli-sub-agent/src/review_cmd_resolve.rs new file mode 100644 index 00000000..b5d31f99 --- /dev/null +++ b/crates/cli-sub-agent/src/review_cmd_resolve.rs @@ -0,0 +1,377 @@ +use std::path::{Path, PathBuf}; + +use anyhow::{Context, Result}; +use tracing::{debug, warn}; + +use crate::cli::{ReviewArgs, ReviewMode}; +use crate::review_consensus::{build_consolidated_artifact, write_consolidated_artifact}; +use crate::review_context::{ + ResolvedReviewContext, ResolvedReviewContextKind, render_spec_review_context, +}; +use crate::review_routing::{ReviewRoutingMetadata, detect_review_routing_metadata}; +use csa_config::global::{heterogeneous_counterpart, select_heterogeneous_tool}; +use csa_config::{GlobalConfig, ProjectConfig}; +use csa_core::types::ToolName; +use csa_session::review_artifact::ReviewArtifact; + +/// Verify the review pattern is installed before attempting execution. +/// +/// By default this fails fast with actionable install guidance if the pattern +/// is missing. When `allow_fallback` is true, it downgrades to warning and +/// lets review continue without the structured pattern protocol. +pub(crate) fn verify_review_skill_available( + project_root: &Path, + allow_fallback: bool, +) -> Result<()> { + match crate::pattern_resolver::resolve_pattern("csa-review", project_root) { + Ok(resolved) => { + debug!( + pattern_dir = %resolved.dir.display(), + has_config = resolved.config.is_some(), + has_agent = resolved.agent_config().is_some(), + skill_md_len = resolved.skill_md.len(), + "Review pattern resolved" + ); + Ok(()) + } + Err(resolve_err) => { + if allow_fallback { + warn!( + "Review pattern not found; continuing because --allow-fallback is set. \ + Install with `weave install RyderFreeman4Logos/cli-sub-agent` for structured review protocol." + ); + return Ok(()); + } + + anyhow::bail!( + "Review pattern not found — `csa review` requires the 'csa-review' pattern.\n\n\ + {resolve_err}\n\n\ + Install the review pattern with one of:\n\ + 1) weave install RyderFreeman4Logos/cli-sub-agent\n\ + 2) Manually place skills/csa-review/SKILL.md (or PATTERN.md) inside .csa/patterns/csa-review/ or patterns/csa-review/\n\n\ + Note: `csa skill install` only installs `.claude/skills/*`; it does NOT install `.csa/patterns/*`.\n\n\ + Without the pattern, the review tool cannot follow the structured review protocol." + ) + } + } +} + +/// Resolve stream mode for review command. +/// +/// - `--stream-stdout` forces TeeToStderr (progressive output) +/// - `--no-stream-stdout` forces BufferOnly (silent until complete) +/// - Default: BufferOnly to prevent raw provider noise from polluting review +/// output. Long-running progress is still surfaced by periodic heartbeats. +pub(crate) fn resolve_review_stream_mode( + stream_stdout: bool, + no_stream_stdout: bool, +) -> csa_process::StreamMode { + if no_stream_stdout { + csa_process::StreamMode::BufferOnly + } else if stream_stdout { + csa_process::StreamMode::TeeToStderr + } else { + csa_process::StreamMode::BufferOnly + } +} + +/// Returns (tool, optional_model_spec). When tier resolves, model_spec is set. +pub(crate) fn resolve_review_tool( + arg_tool: Option, + project_config: Option<&ProjectConfig>, + global_config: &GlobalConfig, + parent_tool: Option<&str>, + project_root: &Path, + force_override_user_config: bool, +) -> Result<(ToolName, Option)> { + // CLI --tool override always wins + if let Some(tool) = arg_tool { + if let Some(cfg) = project_config { + cfg.enforce_tool_enabled(tool.as_str(), force_override_user_config)?; + } + return Ok((tool, None)); + } + + // Tier-based resolution: project tier > global tier > tool-based fallback. + // Tier takes priority over tool when both are set. + let tier_name = project_config + .and_then(|cfg| cfg.review.as_ref()) + .and_then(|r| r.tier.as_deref()) + .or(global_config.review.tier.as_deref()); + + if let Some(tier) = tier_name { + if let Some(cfg) = project_config { + if let Some(resolution) = + crate::run_helpers::resolve_tool_from_tier(tier, cfg, parent_tool) + { + return Ok((resolution.tool, Some(resolution.model_spec))); + } + } + // Tier set but no available tool found — fall through to tool-based resolution + debug!( + tier = tier, + "Tier set but no available tool found, falling through to tool-based resolution" + ); + } + + if let Some(project_review) = project_config.and_then(|cfg| cfg.review.as_ref()) { + return resolve_review_tool_from_value( + &project_review.tool, + parent_tool, + project_config, + global_config, + project_root, + ) + .map(|t| (t, None)) + .with_context(|| { + format!( + "Failed to resolve review tool from project config: {}", + ProjectConfig::config_path(project_root).display() + ) + }); + } + + // When global [review].tool is "auto", always try heterogeneous auto-selection first. + if global_config.review.tool == "auto" { + if let Some(tool) = select_auto_review_tool(parent_tool, project_config, global_config) { + return Ok((tool, None)); + } + } + + match global_config.resolve_review_tool(parent_tool) { + Ok(tool_name) => { + if let Some(cfg) = project_config { + if !cfg.is_tool_enabled(&tool_name) { + return Err(review_auto_resolution_error(parent_tool, project_root)); + } + } + crate::run_helpers::parse_tool_name(&tool_name) + .map(|t| (t, None)) + .map_err(|_| { + anyhow::anyhow!( + "Invalid [review].tool value '{}'. Supported values: gemini-cli, opencode, codex, claude-code.", + tool_name + ) + }) + } + Err(_) => Err(review_auto_resolution_error(parent_tool, project_root)), + } +} + +/// Resolve review thinking: CLI > config review.thinking. +pub(crate) fn resolve_review_thinking( + cli_thinking: Option<&str>, + config_thinking: Option<&str>, +) -> Option { + cli_thinking + .map(str::to_string) + .or_else(|| config_thinking.map(str::to_string)) +} + +fn resolve_review_tool_from_value( + tool_value: &str, + parent_tool: Option<&str>, + project_config: Option<&ProjectConfig>, + global_config: &GlobalConfig, + project_root: &Path, +) -> Result { + if tool_value == "auto" { + if let Some(tool) = select_auto_review_tool(parent_tool, project_config, global_config) { + return Ok(tool); + } + + // Try old heterogeneous_counterpart first for backward compatibility, + // but only if the counterpart tool is enabled. + if let Some(resolved) = parent_tool.and_then(heterogeneous_counterpart) { + let counterpart_enabled = + project_config.is_none_or(|cfg| cfg.is_tool_enabled(resolved)); + if counterpart_enabled { + return crate::run_helpers::parse_tool_name(resolved).map_err(|_| { + anyhow::anyhow!( + "BUG: auto review tool resolution returned invalid tool '{}'", + resolved + ) + }); + } + } + + // Both methods failed + return Err(review_auto_resolution_error(parent_tool, project_root)); + } + + crate::run_helpers::parse_tool_name(tool_value).map_err(|_| { + anyhow::anyhow!( + "Invalid project [review].tool value '{}'. Supported values: auto, gemini-cli, opencode, codex, claude-code.", + tool_value + ) + }) +} + +fn select_auto_review_tool( + parent_tool: Option<&str>, + project_config: Option<&ProjectConfig>, + global_config: &GlobalConfig, +) -> Option { + let parent_str = parent_tool?; + let parent_tool_name = crate::run_helpers::parse_tool_name(parent_str).ok()?; + let enabled_tools: Vec<_> = if let Some(cfg) = project_config { + let tools: Vec<_> = csa_config::global::all_known_tools() + .iter() + .filter(|t| cfg.is_tool_auto_selectable(t.as_str())) + .copied() + .collect(); + csa_config::global::sort_tools_by_effective_priority(&tools, project_config, global_config) + } else { + csa_config::global::sort_tools_by_effective_priority( + csa_config::global::all_known_tools(), + project_config, + global_config, + ) + }; + + select_heterogeneous_tool(&parent_tool_name, &enabled_tools) +} + +fn review_auto_resolution_error(parent_tool: Option<&str>, project_root: &Path) -> anyhow::Error { + let parent = parent_tool.unwrap_or("").escape_default().to_string(); + let global_path = GlobalConfig::config_path() + .ok() + .map(|p| p.display().to_string()) + .unwrap_or_else(|| "~/.config/cli-sub-agent/config.toml".to_string()); + let project_path = ProjectConfig::config_path(project_root) + .display() + .to_string(); + + anyhow::anyhow!( + "AUTO review tool selection failed (tool = \"auto\").\n\n\ +STOP: Do not proceed. Ask the user to configure the review tool explicitly.\n\n\ +Parent tool context: {parent}\n\ +Supported auto mapping: claude-code <-> codex\n\n\ +Choose one:\n\ +1) Global config (user-level): {global_path}\n\ + [review]\n\ + tool = \"codex\" # or \"claude-code\", \"opencode\", \"gemini-cli\"\n\ +2) Project config override: {project_path}\n\ + [review]\n\ + tool = \"codex\" # or \"claude-code\", \"opencode\", \"gemini-cli\"\n\ +3) CLI override: csa review --sa-mode --tool codex\n\n\ +Reason: CSA enforces heterogeneity in auto mode and will not fall back." + ) +} + +pub(crate) fn write_multi_reviewer_consolidated_artifact(reviewers: usize) -> Result<()> { + let Some(session_dir) = std::env::var_os("CSA_SESSION_DIR") else { + return Ok(()); + }; + let output_dir = PathBuf::from(session_dir); + let session_id = std::env::var("CSA_SESSION_ID").unwrap_or_else(|_| "unknown".to_string()); + + let mut reviewer_artifacts = Vec::new(); + for reviewer_index in 1..=reviewers { + let artifact_path = output_dir + .join(format!("reviewer-{reviewer_index}")) + .join("review-findings.json"); + + if !artifact_path.exists() { + continue; + } + + let content = std::fs::read_to_string(&artifact_path) + .with_context(|| format!("failed to read {}", artifact_path.display()))?; + let artifact: ReviewArtifact = serde_json::from_str(&content) + .with_context(|| format!("failed to parse {}", artifact_path.display()))?; + reviewer_artifacts.push(artifact); + } + + let consolidated = build_consolidated_artifact(reviewer_artifacts, &session_id); + write_consolidated_artifact(&consolidated, &output_dir) +} + +/// Derive the review scope string from CLI arguments. +/// +/// Priority order (first match wins): +/// 1. `--range ...` → "range:..." +/// 2. `--files ` → "files:" +/// 3. `--commit ` → "commit:" +/// 4. `--diff` → "uncommitted" +/// 5. default → "base:" (branch defaults to "main") +pub(crate) fn derive_scope(args: &ReviewArgs) -> String { + if let Some(ref range) = args.range { + return format!("range:{range}"); + } + if let Some(ref files) = args.files { + return format!("files:{files}"); + } + if let Some(ref commit) = args.commit { + return format!("commit:{commit}"); + } + if args.diff { + return "uncommitted".to_string(); + } + format!("base:{}", args.branch.as_deref().unwrap_or("main")) +} + +pub(crate) fn review_scope_allows_auto_discovery(args: &ReviewArgs) -> bool { + args.range.is_some() || (!args.diff && args.commit.is_none() && args.files.is_none()) +} + +/// Anti-recursion preamble injected into every review/debate subprocess prompt. +/// +/// Prevents the spawned tool (e.g. claude-code-acp) from reading CLAUDE.md rules +/// that say "use csa review" and recursively invoking `csa run` / `csa review`. +pub(crate) const ANTI_RECURSION_PREAMBLE: &str = "\ +CRITICAL: You are running INSIDE a CSA subprocess (csa review / csa debate). \ +Do NOT invoke `csa run`, `csa review`, `csa debate`, or ANY `csa` CLI command — \ +this causes infinite recursion. Perform the task DIRECTLY using your own \ +capabilities (Read, Grep, Glob, Bash for read-only git commands). \ +REVIEW-ONLY SAFETY: Do NOT run git add/commit/push/merge/rebase/tag/stash/reset/checkout/cherry-pick, \ +and do NOT run gh pr/create/comment/merge or any command that mutates repository/PR state. \ +Ignore prompt-guard reminders about commit/push in this subprocess. \ +Ignore any CLAUDE.md or AGENTS.md rules that instruct you to delegate to CSA.\n\n"; + +/// Build a concise review instruction that tells the tool to use the csa-review skill. +/// +/// The tool loads the skill from `.claude/skills/csa-review/` automatically. +/// CSA only passes scope, mode, and optional parameters — no diff content. +/// An anti-recursion preamble is prepended to prevent the spawned tool from +/// re-invoking CSA commands (see GitHub issue #272). +pub(crate) fn build_review_instruction( + scope: &str, + mode: &str, + security_mode: &str, + review_mode: ReviewMode, + context: Option<&ResolvedReviewContext>, +) -> String { + let mut instruction = format!( + "{ANTI_RECURSION_PREAMBLE}Use the csa-review skill. scope={scope}, mode={mode}, security_mode={security_mode}, review_mode={review_mode}." + ); + if let Some(ctx) = context { + instruction.push_str(&format!(" context={}", ctx.path)); + if let ResolvedReviewContextKind::SpecToml { spec } = &ctx.kind { + instruction.push_str( + "\nSpec alignment context (parsed from spec.toml; use this criteria set directly):\n", + ); + instruction.push_str(&render_spec_review_context(spec)); + } + } + instruction +} + +pub(crate) fn build_review_instruction_for_project( + scope: &str, + mode: &str, + security_mode: &str, + review_mode: ReviewMode, + context: Option<&ResolvedReviewContext>, + project_root: &Path, + project_config: Option<&ProjectConfig>, +) -> (String, ReviewRoutingMetadata) { + let review_routing = detect_review_routing_metadata(project_root, project_config); + let mut instruction = + build_review_instruction(scope, mode, security_mode, review_mode, context); + instruction.push_str(&format!( + "\n[project_profile: {}]", + review_routing.project_profile + )); + (instruction, review_routing) +} diff --git a/crates/cli-sub-agent/src/review_cmd_tests.rs b/crates/cli-sub-agent/src/review_cmd_tests.rs index 935b3f4b..c08061a9 100644 --- a/crates/cli-sub-agent/src/review_cmd_tests.rs +++ b/crates/cli-sub-agent/src/review_cmd_tests.rs @@ -76,6 +76,8 @@ fn project_config_with_enabled_tools(tools: &[&str]) -> ProjectConfig { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), } } @@ -322,6 +324,7 @@ fn derive_scope_uncommitted() { range: None, files: None, fix: false, + max_rounds: 3, review_mode: None, red_team: false, security_mode: "auto".to_string(), @@ -352,6 +355,7 @@ fn derive_scope_commit() { range: None, files: None, fix: false, + max_rounds: 3, review_mode: None, red_team: false, security_mode: "auto".to_string(), @@ -382,6 +386,7 @@ fn derive_scope_range() { range: Some("main...HEAD".to_string()), files: None, fix: false, + max_rounds: 3, review_mode: None, red_team: false, security_mode: "auto".to_string(), @@ -412,6 +417,7 @@ fn derive_scope_files() { range: None, files: Some("src/**/*.rs".to_string()), fix: false, + max_rounds: 3, review_mode: None, red_team: false, security_mode: "auto".to_string(), @@ -442,6 +448,7 @@ fn derive_scope_default_branch() { range: None, files: None, fix: false, + max_rounds: 3, review_mode: None, red_team: false, security_mode: "auto".to_string(), diff --git a/crates/cli-sub-agent/src/review_cmd_tests_tail.rs b/crates/cli-sub-agent/src/review_cmd_tests_tail.rs index 4fb4b069..164ef57b 100644 --- a/crates/cli-sub-agent/src/review_cmd_tests_tail.rs +++ b/crates/cli-sub-agent/src/review_cmd_tests_tail.rs @@ -369,5 +369,5 @@ async fn execute_review_ignores_inherited_csa_session_id_without_explicit_sessio .await; let execution = result.expect("review should ignore inherited stale CSA_SESSION_ID"); - assert_eq!(execution.exit_code, 0); + assert_eq!(execution.execution.exit_code, 0); } diff --git a/crates/cli-sub-agent/src/review_consensus.rs b/crates/cli-sub-agent/src/review_consensus.rs index e6ac8ebf..1c26b81f 100644 --- a/crates/cli-sub-agent/src/review_consensus.rs +++ b/crates/cli-sub-agent/src/review_consensus.rs @@ -321,6 +321,8 @@ mod tests { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), } } diff --git a/crates/cli-sub-agent/src/run_helpers_tests.rs b/crates/cli-sub-agent/src/run_helpers_tests.rs index 361d5f1a..fdb97da7 100644 --- a/crates/cli-sub-agent/src/run_helpers_tests.rs +++ b/crates/cli-sub-agent/src/run_helpers_tests.rs @@ -133,6 +133,8 @@ fn build_executor_uses_project_tool_defaults_when_cli_missing() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; let exec = build_executor(&ToolName::Codex, None, None, None, Some(&config), true).unwrap(); @@ -167,6 +169,8 @@ fn build_executor_ignores_project_tool_defaults_when_disabled() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; let exec = build_executor(&ToolName::Codex, None, None, None, Some(&config), false).unwrap(); @@ -207,6 +211,8 @@ fn build_executor_cli_overrides_project_tool_defaults() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; let exec = build_executor( @@ -446,6 +452,8 @@ fn build_executor_model_spec_overrides_both() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; // When explicit thinking is provided alongside model_spec, it overrides @@ -525,6 +533,8 @@ fn resolve_tool_and_model_disabled_tool_explicit_errors() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; let result = super::resolve_tool_and_model( @@ -570,6 +580,8 @@ fn resolve_tool_and_model_disabled_tool_with_override_succeeds() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; let result = super::resolve_tool_and_model( @@ -615,6 +627,8 @@ fn resolve_tool_and_model_disabled_tool_model_spec_errors() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; let result = super::resolve_tool_and_model( @@ -675,6 +689,8 @@ fn config_with_tier(tier_name: &str, models: Vec<&str>, enabled_tools: &[&str]) preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), } } diff --git a/crates/cli-sub-agent/tests/e2e.rs b/crates/cli-sub-agent/tests/e2e.rs index eb21c797..3b0f0ce6 100644 --- a/crates/cli-sub-agent/tests/e2e.rs +++ b/crates/cli-sub-agent/tests/e2e.rs @@ -162,7 +162,7 @@ fn review_cli_validation_applies_red_team_defaults() { match &cli.command { Commands::Review(args) => { - validate_command_args(&cli.command).expect("review args should validate"); + validate_command_args(&cli.command, 1800).expect("review args should validate"); assert_eq!(args.effective_review_mode().as_str(), "red-team"); assert_eq!(args.effective_security_mode(), "on"); } @@ -182,7 +182,8 @@ fn review_cli_validation_rejects_red_team_with_security_off() { ]) .expect("review args should parse before validation"); - let err = validate_command_args(&cli.command).expect_err("validation should reject conflict"); + let err = + validate_command_args(&cli.command, 1800).expect_err("validation should reject conflict"); assert_eq!(err.kind(), clap::error::ErrorKind::ArgumentConflict); } diff --git a/crates/cli-sub-agent/tests/review_mode_compat.rs b/crates/cli-sub-agent/tests/review_mode_compat.rs index 505f0ad1..6cf97310 100644 --- a/crates/cli-sub-agent/tests/review_mode_compat.rs +++ b/crates/cli-sub-agent/tests/review_mode_compat.rs @@ -105,7 +105,8 @@ fn review_mode_parses_standard_and_red_team_cli_args() { fn review_cli_validation_and_consensus_helpers_remain_compatible() { let cli = Cli::try_parse_from(["csa", "review", "--red-team", "--diff"]) .expect("red-team review args should parse"); - cli_defs::validate_command_args(&cli.command).expect("red-team review args should validate"); + cli_defs::validate_command_args(&cli.command, 1800) + .expect("red-team review args should validate"); let instruction = review_consensus::build_multi_reviewer_instruction("Base prompt", 2, ToolName::Codex); diff --git a/crates/csa-config/src/config.rs b/crates/csa-config/src/config.rs index 9092f64c..beae0cd1 100644 --- a/crates/csa-config/src/config.rs +++ b/crates/csa-config/src/config.rs @@ -5,7 +5,10 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; use crate::acp::AcpConfig; -use crate::config_merge::{enforce_global_tool_disables, merge_toml_values, warn_deprecated_keys}; +use crate::config_merge::{ + enforce_global_tool_disables, merge_toml_values, strip_review_project_only_from_global, + warn_deprecated_keys, +}; pub use crate::config_resources::ResourcesConfig; use crate::global::{PreferencesConfig, ReviewConfig}; use crate::memory::MemoryConfig; @@ -122,6 +125,99 @@ impl SessionConfig { } } +/// Project-level hook overrides (`[hooks]` in `.csa/config.toml`). +/// +/// When set, these commands take PRIORITY over `hooks.toml` PreRun/PostRun +/// entries. They are injected as runtime overrides into the hook loading +/// pipeline, so they sit at the highest-priority layer. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct HooksSection { + /// Shell command to run before every `csa run`/`review`/`debate`. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub pre_run: Option, + /// Shell command to run after every `csa run`/`review`/`debate`. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub post_run: Option, + /// Timeout (seconds) for hook commands (default: 60). + #[serde( + default = "default_hooks_timeout_secs", + skip_serializing_if = "is_default_hooks_timeout" + )] + pub timeout_secs: u64, +} + +const fn default_hooks_timeout_secs() -> u64 { + 60 +} + +fn is_default_hooks_timeout(val: &u64) -> bool { + *val == default_hooks_timeout_secs() +} + +impl Default for HooksSection { + fn default() -> Self { + Self { + pre_run: None, + post_run: None, + timeout_secs: default_hooks_timeout_secs(), + } + } +} + +impl HooksSection { + /// Returns true when all fields are at their defaults (per rust/016 serde-default rule). + pub fn is_default(&self) -> bool { + self.pre_run.is_none() + && self.post_run.is_none() + && self.timeout_secs == default_hooks_timeout_secs() + } +} + +/// Execution tuning (`[execution]` in config). +/// +/// Present in both project and global configs. Project values override global +/// during config merge (standard TOML deep-merge). +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ExecutionConfig { + /// Floor for the `--timeout` flag (seconds). + /// + /// Any `--timeout` value below this is rejected at the CLI validation layer. + /// Default: 1800 (30 minutes). The previous hardcoded floor was 1200. + #[serde( + default = "default_min_timeout_seconds", + skip_serializing_if = "is_default_min_timeout" + )] + pub min_timeout_seconds: u64, +} + +const fn default_min_timeout_seconds() -> u64 { + 1800 +} + +fn is_default_min_timeout(val: &u64) -> bool { + *val == default_min_timeout_seconds() +} + +impl Default for ExecutionConfig { + fn default() -> Self { + Self { + min_timeout_seconds: default_min_timeout_seconds(), + } + } +} + +impl ExecutionConfig { + /// Returns true when all fields are at their defaults (per rust/016 serde-default rule). + pub fn is_default(&self) -> bool { + self.min_timeout_seconds == default_min_timeout_seconds() + } + + /// The compile-time default minimum timeout in seconds. + pub const fn default_min_timeout() -> u64 { + default_min_timeout_seconds() + } +} + /// Current schema version for config.toml pub const CURRENT_SCHEMA_VERSION: u32 = 1; @@ -175,6 +271,16 @@ pub struct ProjectConfig { /// When set, overrides the global `[preferences].tool_priority`. #[serde(default)] pub preferences: Option, + /// Project-level hook overrides for pre/post run commands. + /// + /// When set, `.csa/config.toml` hooks take PRIORITY over `hooks.toml` + /// for PreRun/PostRun events. The commands specified here are injected + /// as runtime overrides into the hook loading pipeline. + #[serde(default, skip_serializing_if = "HooksSection::is_default")] + pub hooks: HooksSection, + /// Execution tuning knobs (timeout floors, etc.). + #[serde(default, skip_serializing_if = "ExecutionConfig::is_default")] + pub execution: ExecutionConfig, } fn default_schema_version() -> u32 { @@ -389,7 +495,12 @@ impl ProjectConfig { .get("schema_version") .and_then(|v| v.as_integer()); - let mut merged = merge_toml_values(base_val.clone(), overlay_val); + // Strip project-only review keys from global config before merge. + // These fields are meaningful only in project config. + let mut base_for_merge = base_val.clone(); + strip_review_project_only_from_global(&mut base_for_merge); + + let mut merged = merge_toml_values(base_for_merge, overlay_val); // Set schema_version to max of both sources (only when at least one is explicit) if let Some(max_ver) = match (base_schema, overlay_schema) { (Some(b), Some(o)) => Some(b.max(o)), @@ -627,6 +738,12 @@ init_timeout_seconds = 60 # [tool_aliases] # gem = "gemini-cli" # cc = "claude-code" +# [hooks] +# pre_run = "cargo fmt --all" +# post_run = "cargo fmt --all" +# timeout_secs = 60 +# [execution] +# min_timeout_seconds = 1800 "# .to_string() } @@ -658,3 +775,7 @@ mod tier_tests; #[cfg(test)] #[path = "config_merge_tests.rs"] mod merge_tests; + +#[cfg(test)] +#[path = "config_merge_tests_tail.rs"] +mod merge_tests_tail; diff --git a/crates/csa-config/src/config_merge.rs b/crates/csa-config/src/config_merge.rs index d7201ba7..2bb8cca6 100644 --- a/crates/csa-config/src/config_merge.rs +++ b/crates/csa-config/src/config_merge.rs @@ -29,6 +29,34 @@ pub(crate) fn merge_toml_values(base: toml::Value, overlay: toml::Value) -> toml } } +/// Project-only keys under `[review]`. These fields are meaningful only in +/// project config; values from global config are stripped before merge to +/// prevent accidental inheritance. +const REVIEW_PROJECT_ONLY_KEYS: &[&str] = &["gate_command", "gate_timeout_secs"]; + +/// Strip project-only keys from the global `[review]` table before merge. +/// +/// Some review config fields (e.g. `gate_command`, `gate_timeout_secs`) are +/// project-specific and must not be inherited from the global config. +/// If the global config sets them, emit a warning and remove them so the +/// merge only preserves values from the project config. +pub(crate) fn strip_review_project_only_from_global(global: &mut toml::Value) { + let review_table = match global.get_mut("review").and_then(|t| t.as_table_mut()) { + Some(t) => t, + None => return, + }; + + for key in REVIEW_PROJECT_ONLY_KEYS { + if review_table.remove(*key).is_some() { + tracing::warn!( + key = *key, + "Global config sets review.{} which is project-only; ignoring global value", + key + ); + } + } +} + /// Re-apply `tools.*.enabled = false` from the global config into a merged /// TOML value. This ensures that global disablement is a hard override: /// project configs cannot set a globally-disabled tool back to `enabled = true`. diff --git a/crates/csa-config/src/config_merge_tests.rs b/crates/csa-config/src/config_merge_tests.rs index 71a442b6..877996d4 100644 --- a/crates/csa-config/src/config_merge_tests.rs +++ b/crates/csa-config/src/config_merge_tests.rs @@ -437,3 +437,195 @@ suppress_notify = true "project can still disable a globally-enabled tool" ); } + +#[test] +fn test_global_gate_command_ignored_during_merge() { + // gate_command is project-only. If global sets it, it should be stripped + // and the merged config should not inherit it. + let tmp = tempdir().unwrap(); + let user_path = tmp.path().join("user.toml"); + let project_path = tmp.path().join("project.toml"); + + std::fs::write( + &user_path, + r#" +schema_version = 1 +[review] +tool = "auto" +gate_command = "make lint" +gate_timeout_secs = 999 +"#, + ) + .unwrap(); + + std::fs::write( + &project_path, + r#" +schema_version = 1 +[project] +name = "test" +"#, + ) + .unwrap(); + + let config = ProjectConfig::load_with_paths(Some(&user_path), &project_path) + .unwrap() + .expect("Should load merged config"); + + // gate_command from global must be stripped (project-only field) + assert!( + config + .review + .as_ref() + .map_or(true, |r| r.gate_command.is_none()), + "global gate_command must not be inherited by project config" + ); + // gate_timeout_secs from global must be stripped, falling back to default 300 + assert!( + config + .review + .as_ref() + .map_or(true, |r| r.gate_timeout_secs == 300), + "global gate_timeout_secs must not be inherited; should be default 300" + ); +} + +#[test] +fn test_project_gate_command_preserved_during_merge() { + // When the project sets gate_command, it should be preserved. + let tmp = tempdir().unwrap(); + let user_path = tmp.path().join("user.toml"); + let project_path = tmp.path().join("project.toml"); + + std::fs::write( + &user_path, + r#" +schema_version = 1 +[review] +tool = "auto" +"#, + ) + .unwrap(); + + std::fs::write( + &project_path, + r#" +schema_version = 1 +[review] +gate_command = "just pre-commit" +gate_timeout_secs = 600 +"#, + ) + .unwrap(); + + let config = ProjectConfig::load_with_paths(Some(&user_path), &project_path) + .unwrap() + .expect("Should load merged config"); + + assert_eq!( + config + .review + .as_ref() + .and_then(|r| r.gate_command.as_deref()), + Some("just pre-commit"), + "project gate_command must be preserved" + ); + assert_eq!( + config.review.as_ref().map(|r| r.gate_timeout_secs), + Some(600), + "project gate_timeout_secs must be preserved" + ); +} + +#[test] +fn test_project_gate_command_overrides_global_gate_command() { + // When both global and project set gate_command, project wins + // AND global value is stripped (not merely overridden). + let tmp = tempdir().unwrap(); + let user_path = tmp.path().join("user.toml"); + let project_path = tmp.path().join("project.toml"); + + std::fs::write( + &user_path, + r#" +schema_version = 1 +[review] +tool = "codex" +gate_command = "make lint" +gate_timeout_secs = 999 +"#, + ) + .unwrap(); + + std::fs::write( + &project_path, + r#" +schema_version = 1 +[review] +gate_command = "just pre-commit" +gate_timeout_secs = 120 +"#, + ) + .unwrap(); + + let config = ProjectConfig::load_with_paths(Some(&user_path), &project_path) + .unwrap() + .expect("Should load merged config"); + + // Project gate_command wins + assert_eq!( + config + .review + .as_ref() + .and_then(|r| r.gate_command.as_deref()), + Some("just pre-commit"), + ); + assert_eq!( + config.review.as_ref().map(|r| r.gate_timeout_secs), + Some(120), + ); + // tool should still be inherited from global (since project didn't set it) + assert_eq!( + config.review.as_ref().map(|r| r.tool.as_str()), + Some("codex"), + ); +} + +#[test] +fn test_gate_mode_still_inherits_from_global() { + // gate_mode is NOT project-only; normal merge applies. + let tmp = tempdir().unwrap(); + let user_path = tmp.path().join("user.toml"); + let project_path = tmp.path().join("project.toml"); + + std::fs::write( + &user_path, + r#" +schema_version = 1 +[review] +gate_mode = "full" +"#, + ) + .unwrap(); + + std::fs::write( + &project_path, + r#" +schema_version = 1 +[project] +name = "test" +"#, + ) + .unwrap(); + + let config = ProjectConfig::load_with_paths(Some(&user_path), &project_path) + .unwrap() + .expect("Should load merged config"); + + // gate_mode from global should be inherited (not project-only) + assert_eq!( + config.review.as_ref().map(|r| r.gate_mode.clone()), + Some(crate::global::GateMode::Full), + "gate_mode should be inherited from global config" + ); +} diff --git a/crates/csa-config/src/config_merge_tests_tail.rs b/crates/csa-config/src/config_merge_tests_tail.rs new file mode 100644 index 00000000..3021d053 --- /dev/null +++ b/crates/csa-config/src/config_merge_tests_tail.rs @@ -0,0 +1,260 @@ +use super::*; +use tempfile::tempdir; + +// ── Task 2: HooksSection tests ────────────────────────────────────── + +#[test] +fn test_hooks_section_parses_from_toml() { + let toml_str = r#" +schema_version = 1 +[hooks] +pre_run = "cargo fmt --all" +post_run = "cargo clippy" +timeout_secs = 120 +"#; + let config: ProjectConfig = toml::from_str(toml_str).unwrap(); + assert_eq!(config.hooks.pre_run.as_deref(), Some("cargo fmt --all")); + assert_eq!(config.hooks.post_run.as_deref(), Some("cargo clippy")); + assert_eq!(config.hooks.timeout_secs, 120); +} + +#[test] +fn test_hooks_section_default_is_empty() { + let config: ProjectConfig = toml::from_str("schema_version = 1\n").unwrap(); + assert!(config.hooks.is_default()); + assert!(config.hooks.pre_run.is_none()); + assert!(config.hooks.post_run.is_none()); + assert_eq!(config.hooks.timeout_secs, 60); +} + +#[test] +fn test_hooks_section_is_default_returns_false_with_pre_run() { + let mut hooks = crate::config::HooksSection::default(); + assert!(hooks.is_default()); + hooks.pre_run = Some("echo hi".to_string()); + assert!(!hooks.is_default()); +} + +#[test] +fn test_hooks_section_is_default_returns_false_with_post_run() { + let mut hooks = crate::config::HooksSection::default(); + hooks.post_run = Some("echo bye".to_string()); + assert!(!hooks.is_default()); +} + +#[test] +fn test_hooks_section_is_default_returns_false_with_custom_timeout() { + let mut hooks = crate::config::HooksSection::default(); + hooks.timeout_secs = 120; + assert!(!hooks.is_default()); +} + +#[test] +fn test_hooks_section_default_serialization_omits_section() { + let config: ProjectConfig = toml::from_str("schema_version = 1\n").unwrap(); + let output = toml::to_string(&config).unwrap(); + // Default hooks should not appear in serialized output (skip_serializing_if) + assert!( + !output.contains("[hooks]"), + "Default hooks section should be omitted from TOML output" + ); +} + +#[test] +fn test_hooks_section_non_default_serialized() { + let toml_str = r#" +schema_version = 1 +[hooks] +pre_run = "cargo fmt" +"#; + let config: ProjectConfig = toml::from_str(toml_str).unwrap(); + let output = toml::to_string(&config).unwrap(); + assert!( + output.contains("[hooks]"), + "Non-default hooks should appear in TOML output" + ); + assert!(output.contains("pre_run")); +} + +#[test] +fn test_hooks_section_project_overrides_user_during_merge() { + let tmp = tempdir().unwrap(); + let user_path = tmp.path().join("user.toml"); + let project_path = tmp.path().join("project.toml"); + + std::fs::write( + &user_path, + r#" +schema_version = 1 +[hooks] +pre_run = "echo user-pre" +post_run = "echo user-post" +timeout_secs = 30 +"#, + ) + .unwrap(); + + std::fs::write( + &project_path, + r#" +schema_version = 1 +[hooks] +pre_run = "echo project-pre" +"#, + ) + .unwrap(); + + let config = ProjectConfig::load_with_paths(Some(&user_path), &project_path) + .unwrap() + .expect("Should load merged config"); + + // Project pre_run wins + assert_eq!(config.hooks.pre_run.as_deref(), Some("echo project-pre")); + // post_run inherited from user (deep merge) + assert_eq!(config.hooks.post_run.as_deref(), Some("echo user-post")); + // timeout_secs inherited from user (not overridden by project) + assert_eq!(config.hooks.timeout_secs, 30); +} + +#[test] +fn test_hooks_section_partial_config_with_only_timeout() { + let toml_str = r#" +schema_version = 1 +[hooks] +timeout_secs = 90 +"#; + let config: ProjectConfig = toml::from_str(toml_str).unwrap(); + assert!(config.hooks.pre_run.is_none()); + assert!(config.hooks.post_run.is_none()); + assert_eq!(config.hooks.timeout_secs, 90); + // Non-default timeout means is_default() should be false + assert!(!config.hooks.is_default()); +} + +// ── Task 5: ExecutionConfig tests ─────────────────────────────────── + +#[test] +fn test_execution_config_parses_from_toml() { + let toml_str = r#" +schema_version = 1 +[execution] +min_timeout_seconds = 2400 +"#; + let config: ProjectConfig = toml::from_str(toml_str).unwrap(); + assert_eq!(config.execution.min_timeout_seconds, 2400); +} + +#[test] +fn test_execution_config_default() { + let config: ProjectConfig = toml::from_str("schema_version = 1\n").unwrap(); + assert!(config.execution.is_default()); + assert_eq!(config.execution.min_timeout_seconds, 1800); +} + +#[test] +fn test_execution_config_is_default() { + let exec = crate::config::ExecutionConfig::default(); + assert!(exec.is_default()); + assert_eq!(exec.min_timeout_seconds, 1800); +} + +#[test] +fn test_execution_config_is_not_default_with_custom_value() { + let mut exec = crate::config::ExecutionConfig::default(); + exec.min_timeout_seconds = 2400; + assert!(!exec.is_default()); +} + +#[test] +fn test_execution_config_default_serialization_omits_section() { + let config: ProjectConfig = toml::from_str("schema_version = 1\n").unwrap(); + let output = toml::to_string(&config).unwrap(); + assert!( + !output.contains("[execution]"), + "Default execution section should be omitted from TOML output" + ); +} + +#[test] +fn test_execution_config_non_default_serialized() { + let toml_str = r#" +schema_version = 1 +[execution] +min_timeout_seconds = 2400 +"#; + let config: ProjectConfig = toml::from_str(toml_str).unwrap(); + let output = toml::to_string(&config).unwrap(); + assert!( + output.contains("[execution]"), + "Non-default execution should appear in TOML output" + ); + assert!(output.contains("min_timeout_seconds = 2400")); +} + +#[test] +fn test_execution_config_project_overrides_user_during_merge() { + let tmp = tempdir().unwrap(); + let user_path = tmp.path().join("user.toml"); + let project_path = tmp.path().join("project.toml"); + + std::fs::write( + &user_path, + r#" +schema_version = 1 +[execution] +min_timeout_seconds = 1800 +"#, + ) + .unwrap(); + + std::fs::write( + &project_path, + r#" +schema_version = 1 +[execution] +min_timeout_seconds = 2400 +"#, + ) + .unwrap(); + + let config = ProjectConfig::load_with_paths(Some(&user_path), &project_path) + .unwrap() + .expect("Should load merged config"); + + // Project overrides user + assert_eq!(config.execution.min_timeout_seconds, 2400); +} + +#[test] +fn test_execution_config_user_fallback_when_project_omits() { + let tmp = tempdir().unwrap(); + let user_path = tmp.path().join("user.toml"); + let project_path = tmp.path().join("project.toml"); + + std::fs::write( + &user_path, + r#" +schema_version = 1 +[execution] +min_timeout_seconds = 3600 +"#, + ) + .unwrap(); + + std::fs::write( + &project_path, + r#" +schema_version = 1 +[project] +name = "test" +"#, + ) + .unwrap(); + + let config = ProjectConfig::load_with_paths(Some(&user_path), &project_path) + .unwrap() + .expect("Should load merged config"); + + // User value inherited when project doesn't set execution + assert_eq!(config.execution.min_timeout_seconds, 3600); +} diff --git a/crates/csa-config/src/config_runtime_tests.rs b/crates/csa-config/src/config_runtime_tests.rs index 057286be..c773b38f 100644 --- a/crates/csa-config/src/config_runtime_tests.rs +++ b/crates/csa-config/src/config_runtime_tests.rs @@ -28,6 +28,8 @@ fn empty_config() -> ProjectConfig { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), } } diff --git a/crates/csa-config/src/config_tests.rs b/crates/csa-config/src/config_tests.rs index 6bb99674..f55e481a 100644 --- a/crates/csa-config/src/config_tests.rs +++ b/crates/csa-config/src/config_tests.rs @@ -44,6 +44,8 @@ fn test_save_and_load_roundtrip() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -90,6 +92,8 @@ fn test_save_and_load_roundtrip_with_review_override() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -126,6 +130,8 @@ fn test_is_tool_enabled_configured_enabled() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; assert!(config.is_tool_enabled("codex")); @@ -163,6 +169,8 @@ fn test_is_tool_enabled_configured_disabled() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; assert!(!config.is_tool_enabled("codex")); @@ -189,6 +197,8 @@ fn test_is_tool_enabled_unconfigured_defaults_to_true() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; assert!(config.is_tool_enabled("codex")); @@ -229,6 +239,8 @@ fn test_is_tool_configured_in_tiers_detects_presence() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; assert!(config.is_tool_configured_in_tiers("codex")); @@ -283,6 +295,8 @@ fn test_is_tool_auto_selectable_requires_enabled_and_tier_membership() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; assert!(config.is_tool_auto_selectable("codex")); @@ -324,6 +338,8 @@ fn test_can_tool_edit_existing_with_restrictions_false() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; assert!(!config.can_tool_edit_existing("gemini-cli")); @@ -353,6 +369,8 @@ fn test_can_tool_edit_existing_without_restrictions() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; assert!(config.can_tool_edit_existing("codex")); @@ -379,6 +397,8 @@ fn test_can_tool_edit_existing_unconfigured_defaults_to_true() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; assert!(config.can_tool_edit_existing("codex")); @@ -408,6 +428,8 @@ fn test_max_recursion_depth_override() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -538,6 +560,8 @@ fn test_schema_version_current_is_ok() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; assert!(config.check_schema_version().is_ok()); @@ -565,6 +589,8 @@ fn test_schema_version_older_is_ok() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; assert!(config.check_schema_version().is_ok()); @@ -591,6 +617,8 @@ fn test_schema_version_newer_fails() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; let result = config.check_schema_version(); @@ -644,6 +672,8 @@ fn test_enforce_tool_enabled_disabled_tool_returns_error() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; let result = config.enforce_tool_enabled("codex", false); diff --git a/crates/csa-config/src/config_tests_tail.rs b/crates/csa-config/src/config_tests_tail.rs index 55f2d35f..656b9071 100644 --- a/crates/csa-config/src/config_tests_tail.rs +++ b/crates/csa-config/src/config_tests_tail.rs @@ -20,6 +20,8 @@ fn test_enforce_tool_enabled_enabled_tool_returns_ok() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; assert!(config.enforce_tool_enabled("codex", false).is_ok()); @@ -42,6 +44,8 @@ fn test_enforce_tool_enabled_unconfigured_tool_returns_ok() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; assert!(config.enforce_tool_enabled("codex", false).is_ok()); @@ -73,6 +77,8 @@ fn test_enforce_tool_enabled_force_override_bypasses_disabled() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; assert!(config.enforce_tool_enabled("codex", true).is_ok()); diff --git a/crates/csa-config/src/config_tests_tier.rs b/crates/csa-config/src/config_tests_tier.rs index 6a728957..931d6123 100644 --- a/crates/csa-config/src/config_tests_tier.rs +++ b/crates/csa-config/src/config_tests_tier.rs @@ -42,6 +42,8 @@ fn test_resolve_tier_default_selection() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; let result = config.resolve_tier_tool("default"); @@ -86,6 +88,8 @@ fn test_resolve_tier_fallback_to_tier3() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; // Should fallback to tier3 @@ -146,6 +150,8 @@ fn test_resolve_tier_skips_disabled_tools() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; // Should skip disabled gemini-cli and select codex @@ -186,6 +192,8 @@ fn test_resolve_alias() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; // Resolve alias @@ -238,6 +246,8 @@ fn enabled_tier_models_returns_all_when_no_tools_disabled() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; let models = config.enabled_tier_models("tier-1"); @@ -287,6 +297,8 @@ fn enabled_tier_models_excludes_disabled_tool() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; let models = config.enabled_tier_models("tier-3"); @@ -313,6 +325,8 @@ fn enabled_tier_models_returns_empty_for_unknown_tier() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; assert!(config.enabled_tier_models("nonexistent").is_empty()); @@ -365,6 +379,8 @@ fn enabled_tier_models_returns_empty_when_all_tools_disabled() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; assert!(config.enabled_tier_models("tier-1").is_empty()); diff --git a/crates/csa-config/src/config_tests_tier_whitelist.rs b/crates/csa-config/src/config_tests_tier_whitelist.rs index a3c88435..d53cf9b1 100644 --- a/crates/csa-config/src/config_tests_tier_whitelist.rs +++ b/crates/csa-config/src/config_tests_tier_whitelist.rs @@ -26,6 +26,8 @@ fn config_with_tiers(tier_models: &[&str]) -> ProjectConfig { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), } } diff --git a/crates/csa-config/src/global.rs b/crates/csa-config/src/global.rs index ee37bfcb..2bac9064 100644 --- a/crates/csa-config/src/global.rs +++ b/crates/csa-config/src/global.rs @@ -58,6 +58,11 @@ pub struct GlobalConfig { /// Project-level `[tool_aliases]` take precedence over global ones. #[serde(default, skip_serializing_if = "HashMap::is_empty")] pub tool_aliases: HashMap, + /// Execution tuning (timeout floors, etc.). + /// + /// Project-level `[execution]` overrides global values during config merge. + #[serde(default)] + pub execution: crate::config::ExecutionConfig, } /// User preferences for tool selection and routing. @@ -113,12 +118,33 @@ pub struct ReviewConfig { /// When a tier is used, this overrides the tier's model_spec thinking budget. #[serde(default, skip_serializing_if = "Option::is_none")] pub thinking: Option, + /// Custom pre-review gate command. Overrides auto-detection of git hooks. + /// + /// PROJECT-ONLY: values set in global config are ignored during merge. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub gate_command: Option, + /// Timeout (seconds) for the pre-review quality gate command. + /// + /// PROJECT-ONLY: values set in global config are ignored during merge. + #[serde( + default = "default_gate_timeout_secs", + skip_serializing_if = "is_default_gate_timeout" + )] + pub gate_timeout_secs: u64, } fn default_review_tool() -> String { "auto".to_string() } +const fn default_gate_timeout_secs() -> u64 { + 300 +} + +fn is_default_gate_timeout(val: &u64) -> bool { + *val == default_gate_timeout_secs() +} + impl Default for ReviewConfig { fn default() -> Self { Self { @@ -126,10 +152,29 @@ impl Default for ReviewConfig { gate_mode: GateMode::default(), tier: None, thinking: None, + gate_command: None, + gate_timeout_secs: default_gate_timeout_secs(), } } } +impl ReviewConfig { + /// Returns true when all fields match defaults (per rust/016 serde-default rule). + pub fn is_default(&self) -> bool { + self.tool == default_review_tool() + && self.gate_mode == GateMode::Monitor + && self.tier.is_none() + && self.thinking.is_none() + && self.gate_command.is_none() + && self.gate_timeout_secs == default_gate_timeout_secs() + } + + /// Default gate timeout in seconds. + pub const fn default_gate_timeout() -> u64 { + default_gate_timeout_secs() + } +} + /// Configuration for the debate workflow. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct DebateConfig { @@ -574,6 +619,10 @@ cloud_review_exhausted = "ask-user" # # Optional shared MCP hub socket path. # mcp_proxy_socket = "/run/user/1000/cli-sub-agent/mcp-hub.sock" + +# Execution tuning. Project-level [execution] overrides these values. +# [execution] +# min_timeout_seconds = 1800 # Floor for --timeout flag (seconds) "# .to_string() } diff --git a/crates/csa-config/src/global_tests.rs b/crates/csa-config/src/global_tests.rs index 94928082..920d199a 100644 --- a/crates/csa-config/src/global_tests.rs +++ b/crates/csa-config/src/global_tests.rs @@ -229,14 +229,90 @@ fn test_gate_mode_serde_roundtrip_all_variants() { gate_mode: gate_mode.clone(), tier: None, thinking: None, + gate_command: None, + gate_timeout_secs: ReviewConfig::default_gate_timeout(), }; let toml = toml::to_string(&review).unwrap(); let parsed: ReviewConfig = toml::from_str(&toml).unwrap(); assert_eq!(parsed.tool, review.tool); assert_eq!(parsed.gate_mode, review.gate_mode); + assert_eq!(parsed.gate_command, review.gate_command); + assert_eq!(parsed.gate_timeout_secs, review.gate_timeout_secs); } } +#[test] +fn test_review_config_gate_command_parses() { + let toml_str = r#" +[review] +tool = "auto" +gate_command = "just pre-commit" +gate_timeout_secs = 600 +"#; + let config: GlobalConfig = toml::from_str(toml_str).unwrap(); + assert_eq!( + config.review.gate_command.as_deref(), + Some("just pre-commit") + ); + assert_eq!(config.review.gate_timeout_secs, 600); +} + +#[test] +fn test_review_config_gate_fields_default() { + let config = ReviewConfig::default(); + assert!(config.gate_command.is_none()); + assert_eq!(config.gate_timeout_secs, 300); +} + +#[test] +fn test_review_config_is_default() { + let config = ReviewConfig::default(); + assert!(config.is_default()); +} + +#[test] +fn test_review_config_is_not_default_with_gate_command() { + let mut config = ReviewConfig::default(); + config.gate_command = Some("make lint".to_string()); + assert!(!config.is_default()); +} + +#[test] +fn test_review_config_is_not_default_with_gate_timeout() { + let mut config = ReviewConfig::default(); + config.gate_timeout_secs = 600; + assert!(!config.is_default()); +} + +#[test] +fn test_review_config_gate_timeout_default_skipped_in_serialization() { + let config = ReviewConfig::default(); + let toml_str = toml::to_string(&config).unwrap(); + // Default gate_timeout_secs (300) should be skipped via skip_serializing_if + assert!( + !toml_str.contains("gate_timeout_secs"), + "Default gate_timeout_secs should be omitted from TOML output" + ); + // gate_command=None should also be omitted + assert!( + !toml_str.contains("gate_command"), + "None gate_command should be omitted from TOML output" + ); +} + +#[test] +fn test_review_config_gate_timeout_non_default_serialized() { + let config = ReviewConfig { + gate_timeout_secs: 600, + ..Default::default() + }; + let toml_str = toml::to_string(&config).unwrap(); + assert!( + toml_str.contains("gate_timeout_secs = 600"), + "Non-default gate_timeout_secs should appear in TOML output" + ); +} + #[test] fn test_resolve_debate_tool_auto_claude_code_parent() { let config = GlobalConfig::default(); @@ -626,3 +702,57 @@ fn test_state_base_dir_returns_ok() { "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-config/src/global_tests_priority.rs b/crates/csa-config/src/global_tests_priority.rs index b8190e2b..dcbe68d5 100644 --- a/crates/csa-config/src/global_tests_priority.rs +++ b/crates/csa-config/src/global_tests_priority.rs @@ -143,6 +143,8 @@ fn project_config_with_preferences(prefs: Option) -> crate::P preferences: prefs, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), } } diff --git a/crates/csa-config/src/init.rs b/crates/csa-config/src/init.rs index 2fd242b2..5b022757 100644 --- a/crates/csa-config/src/init.rs +++ b/crates/csa-config/src/init.rs @@ -235,6 +235,8 @@ pub fn init_project( aliases: HashMap::new(), tool_aliases: HashMap::new(), preferences: None, + hooks: Default::default(), + execution: Default::default(), } } else { ProjectConfig { @@ -260,6 +262,8 @@ pub fn init_project( aliases: HashMap::new(), tool_aliases: HashMap::new(), preferences: None, + hooks: Default::default(), + execution: Default::default(), } }; diff --git a/crates/csa-config/src/lib.rs b/crates/csa-config/src/lib.rs index eb98b4cb..fe2d5a1a 100644 --- a/crates/csa-config/src/lib.rs +++ b/crates/csa-config/src/lib.rs @@ -19,13 +19,13 @@ pub mod weave_lock; pub use acp::AcpConfig; pub use config::{ - EnforcementMode, ProjectConfig, ProjectMeta, SessionConfig, TierConfig, ToolConfig, - ToolResourceProfile, ToolRestrictions, + EnforcementMode, ExecutionConfig, HooksSection, ProjectConfig, ProjectMeta, SessionConfig, + TierConfig, ToolConfig, ToolResourceProfile, ToolRestrictions, }; pub use config_resources::ResourcesConfig; pub use config_runtime::{DefaultSandboxOptions, default_sandbox_for_tool}; pub use gc::GcConfig; -pub use global::{GateMode, GlobalConfig, GlobalMcpConfig}; +pub use global::{GateMode, GlobalConfig, GlobalMcpConfig, ReviewConfig}; pub use init::{detect_installed_tools, init_project}; pub use mcp::{McpFilter, McpRegistry, McpServerConfig, McpTransport}; pub use memory::{MemoryConfig, MemoryEphemeralConfig, MemoryLlmConfig}; diff --git a/crates/csa-config/src/validate_tests.rs b/crates/csa-config/src/validate_tests.rs index 7aca0b82..d3f044b1 100644 --- a/crates/csa-config/src/validate_tests.rs +++ b/crates/csa-config/src/validate_tests.rs @@ -47,6 +47,8 @@ fn test_validate_config_succeeds_on_valid() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -78,6 +80,8 @@ fn test_validate_config_fails_on_empty_name() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -113,6 +117,8 @@ fn test_validate_config_fails_on_unknown_tool() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -149,6 +155,8 @@ fn test_validate_config_fails_on_zero_idle_timeout() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -189,6 +197,8 @@ fn test_validate_config_fails_on_invalid_review_tool() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -237,6 +247,8 @@ fn test_validate_config_fails_on_invalid_model_spec() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -288,6 +300,8 @@ fn test_validate_config_fails_on_invalid_tier_mapping() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -348,6 +362,8 @@ fn test_validate_config_fails_on_empty_models() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -399,6 +415,8 @@ fn test_validate_config_accepts_custom_tier_names() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -433,6 +451,8 @@ fn test_validate_config_fails_on_invalid_debate_tool() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -470,6 +490,8 @@ fn test_validate_max_recursion_depth_boundary_20() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -500,6 +522,8 @@ fn test_validate_max_recursion_depth_boundary_21() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); diff --git a/crates/csa-config/src/validate_tests_preferences.rs b/crates/csa-config/src/validate_tests_preferences.rs index 470b6f7d..051c2a3f 100644 --- a/crates/csa-config/src/validate_tests_preferences.rs +++ b/crates/csa-config/src/validate_tests_preferences.rs @@ -26,6 +26,8 @@ fn test_validate_config_warns_but_passes_on_unknown_tool_priority() { }), session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); diff --git a/crates/csa-config/src/validate_tests_sandbox.rs b/crates/csa-config/src/validate_tests_sandbox.rs index 781cbfab..c80f44e3 100644 --- a/crates/csa-config/src/validate_tests_sandbox.rs +++ b/crates/csa-config/src/validate_tests_sandbox.rs @@ -26,6 +26,8 @@ fn test_validate_liveness_dead_seconds_zero_rejected() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -65,6 +67,8 @@ fn test_validate_memory_max_mb_too_low() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -104,6 +108,8 @@ fn test_validate_memory_max_mb_at_minimum() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -137,6 +143,8 @@ fn test_validate_pids_max_too_low() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -176,6 +184,8 @@ fn test_validate_pids_max_at_minimum() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -209,6 +219,8 @@ fn test_validate_node_heap_limit_mb_too_low_in_resources() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -254,6 +266,8 @@ fn test_validate_per_tool_required_enforcement_without_memory_fails() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -303,6 +317,8 @@ fn test_validate_per_tool_required_enforcement_with_tool_memory_passes() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -346,6 +362,8 @@ fn test_validate_per_tool_required_enforcement_with_global_memory_passes() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -389,6 +407,8 @@ fn test_validate_node_heap_limit_mb_too_low_in_tool() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); diff --git a/crates/csa-config/src/validate_tests_tail.rs b/crates/csa-config/src/validate_tests_tail.rs index 19d85ff0..d2cb70a7 100644 --- a/crates/csa-config/src/validate_tests_tail.rs +++ b/crates/csa-config/src/validate_tests_tail.rs @@ -41,6 +41,8 @@ fn test_validate_model_spec_two_parts() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -88,6 +90,8 @@ fn test_validate_model_spec_five_parts() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -127,6 +131,8 @@ fn test_validate_review_tool_auto_accepted() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -162,6 +168,8 @@ fn test_validate_all_known_review_tools_accepted() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -202,6 +210,8 @@ fn test_validate_all_known_debate_tools_accepted() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -242,6 +252,8 @@ fn test_validate_all_four_known_tools_accepted() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -272,6 +284,8 @@ fn test_validate_no_review_no_debate_is_ok() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -302,6 +316,8 @@ fn test_validate_max_recursion_depth_zero() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); diff --git a/crates/csa-config/src/validate_tests_tiers.rs b/crates/csa-config/src/validate_tests_tiers.rs index 141fb7dd..b90bed2c 100644 --- a/crates/csa-config/src/validate_tests_tiers.rs +++ b/crates/csa-config/src/validate_tests_tiers.rs @@ -55,6 +55,8 @@ fn test_validate_multiple_tiers_all_valid() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -99,6 +101,8 @@ fn test_validate_tier_with_multiple_models_all_valid() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -143,6 +147,8 @@ fn test_validate_tier_with_one_bad_model_in_list() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -190,6 +196,8 @@ fn test_validate_tier_token_budget_zero_rejected() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -232,6 +240,8 @@ fn test_validate_tier_max_turns_zero_rejected() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -274,6 +284,8 @@ fn test_validate_tier_with_valid_budget_and_turns() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -315,6 +327,8 @@ fn test_validate_tier_model_spec_unknown_tool_rejected() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -361,6 +375,8 @@ fn test_validate_tier_model_spec_known_tool_accepted() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -394,6 +410,8 @@ fn test_validate_review_tier_unknown_rejected() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -432,6 +450,8 @@ fn test_validate_debate_tier_unknown_rejected() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); @@ -481,6 +501,8 @@ fn test_validate_review_tier_valid_accepted() { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; config.save(dir.path()).unwrap(); diff --git a/crates/csa-scheduler/src/failover.rs b/crates/csa-scheduler/src/failover.rs index 94f0b7a0..b3c4d9b0 100644 --- a/crates/csa-scheduler/src/failover.rs +++ b/crates/csa-scheduler/src/failover.rs @@ -239,6 +239,8 @@ mod tests { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), } } @@ -627,6 +629,8 @@ mod tests { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), } } diff --git a/crates/csa-scheduler/src/rotation.rs b/crates/csa-scheduler/src/rotation.rs index 51ecd644..173d91d7 100644 --- a/crates/csa-scheduler/src/rotation.rs +++ b/crates/csa-scheduler/src/rotation.rs @@ -285,6 +285,8 @@ mod tests { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), } } @@ -589,6 +591,8 @@ mod tests { preferences: None, session: Default::default(), memory: Default::default(), + hooks: Default::default(), + execution: Default::default(), }; assert_eq!(resolve_tier_name(&config, "anything"), None); }