diff --git a/Cargo.lock b/Cargo.lock index 8fb74169..650e10d7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -515,7 +515,7 @@ checksum = "c8d4a3bb8b1e0c1050499d1815f5ab16d04f0959b233085fb31653fbfc9d98f9" [[package]] name = "cli-sub-agent" -version = "0.1.194" +version = "0.1.195" dependencies = [ "anyhow", "chrono", @@ -703,7 +703,7 @@ dependencies = [ [[package]] name = "csa-acp" -version = "0.1.194" +version = "0.1.195" dependencies = [ "agent-client-protocol", "anyhow", @@ -723,7 +723,7 @@ dependencies = [ [[package]] name = "csa-config" -version = "0.1.194" +version = "0.1.195" dependencies = [ "anyhow", "chrono", @@ -739,7 +739,7 @@ dependencies = [ [[package]] name = "csa-core" -version = "0.1.194" +version = "0.1.195" dependencies = [ "agent-teams", "chrono", @@ -754,7 +754,7 @@ dependencies = [ [[package]] name = "csa-eval" -version = "0.1.194" +version = "0.1.195" dependencies = [ "anyhow", "chrono", @@ -768,7 +768,7 @@ dependencies = [ [[package]] name = "csa-executor" -version = "0.1.194" +version = "0.1.195" dependencies = [ "agent-teams", "anyhow", @@ -794,7 +794,7 @@ dependencies = [ [[package]] name = "csa-hooks" -version = "0.1.194" +version = "0.1.195" dependencies = [ "anyhow", "chrono", @@ -811,7 +811,7 @@ dependencies = [ [[package]] name = "csa-lock" -version = "0.1.194" +version = "0.1.195" dependencies = [ "anyhow", "chrono", @@ -823,7 +823,7 @@ dependencies = [ [[package]] name = "csa-mcp-hub" -version = "0.1.194" +version = "0.1.195" dependencies = [ "anyhow", "axum", @@ -845,7 +845,7 @@ dependencies = [ [[package]] name = "csa-memory" -version = "0.1.194" +version = "0.1.195" dependencies = [ "anyhow", "async-trait", @@ -863,7 +863,7 @@ dependencies = [ [[package]] name = "csa-process" -version = "0.1.194" +version = "0.1.195" dependencies = [ "anyhow", "chrono", @@ -881,7 +881,7 @@ dependencies = [ [[package]] name = "csa-resource" -version = "0.1.194" +version = "0.1.195" dependencies = [ "anyhow", "csa-core", @@ -897,7 +897,7 @@ dependencies = [ [[package]] name = "csa-scheduler" -version = "0.1.194" +version = "0.1.195" dependencies = [ "anyhow", "chrono", @@ -915,7 +915,7 @@ dependencies = [ [[package]] name = "csa-session" -version = "0.1.194" +version = "0.1.195" dependencies = [ "anyhow", "chrono", @@ -936,7 +936,7 @@ dependencies = [ [[package]] name = "csa-todo" -version = "0.1.194" +version = "0.1.195" dependencies = [ "anyhow", "chrono", @@ -4325,7 +4325,7 @@ dependencies = [ [[package]] name = "weave" -version = "0.1.194" +version = "0.1.195" dependencies = [ "anyhow", "clap", diff --git a/Cargo.toml b/Cargo.toml index 336492c7..c6e22714 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -3,7 +3,7 @@ members = ["crates/*"] resolver = "2" [workspace.package] -version = "0.1.194" +version = "0.1.195" edition = "2024" rust-version = "1.88" license = "Apache-2.0" diff --git a/crates/cli-sub-agent/src/pipeline.rs b/crates/cli-sub-agent/src/pipeline.rs index d05f5798..59a61a41 100644 --- a/crates/cli-sub-agent/src/pipeline.rs +++ b/crates/cli-sub-agent/src/pipeline.rs @@ -385,6 +385,28 @@ pub(crate) fn run_pipeline_hook( Ok(()) } +/// Fire an observational hook with auto-loaded config. Failures are logged and +/// silently discarded (observational events never block the caller). +pub(crate) fn fire_observational_hook( + event: HookEvent, + pairs: &[(&str, &str)], + project_root: &std::path::Path, +) { + let hooks_config = csa_hooks::load_hooks_config( + csa_session::get_session_root(project_root) + .ok() + .map(|r| r.join("hooks.toml")) + .as_deref(), + csa_hooks::global_hooks_path().as_deref(), + None, + ); + let variables: std::collections::HashMap = pairs + .iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect(); + let _ = run_pipeline_hook(event, &hooks_config, &variables); +} + pub(crate) fn determine_project_root(cd: Option<&str>) -> Result { let path = if let Some(cd_path) = cd { PathBuf::from(cd_path) diff --git a/crates/cli-sub-agent/src/review_cmd.rs b/crates/cli-sub-agent/src/review_cmd.rs index ccce57b3..c1beeac7 100644 --- a/crates/cli-sub-agent/src/review_cmd.rs +++ b/crates/cli-sub-agent/src/review_cmd.rs @@ -344,6 +344,7 @@ pub(crate) async fn handle_review(args: ReviewArgs, current_depth: u32) -> Resul } else { result.execution.exit_code }; + let diff_fingerprint = compute_diff_fingerprint(&project_root, &scope); persist_review_meta( &project_root, &ReviewSessionMeta { @@ -357,10 +358,22 @@ pub(crate) async fn handle_review(args: ReviewArgs, current_depth: u32) -> Resul fix_attempted: args.fix, fix_rounds: 0, timestamp: chrono::Utc::now(), + diff_fingerprint, }, ); if !args.fix || verdict == CLEAN { + // Fire PostReview hook only for final results (no fix loop pending). + crate::pipeline::fire_observational_hook( + csa_hooks::HookEvent::PostReview, + &[ + ("session_id", result.meta_session_id.as_str()), + ("decision", decision.as_str()), + ("verdict", verdict), + ("scope", &scope), + ], + &project_root, + ); return Ok(effective_exit_code); } @@ -379,7 +392,8 @@ pub(crate) async fn handle_review(args: ReviewArgs, current_depth: u32) -> Resul } // --- Fix loop: resume the review session to apply fixes, then re-gate --- - return fix::run_fix_loop(fix::FixLoopContext { + let scope_for_hook = scope.clone(); + let fix_exit_code = fix::run_fix_loop(fix::FixLoopContext { tool, config: config.as_ref(), global_config: &global_config, @@ -402,6 +416,21 @@ pub(crate) async fn handle_review(args: ReviewArgs, current_depth: u32) -> Resul initial_session_id: result.meta_session_id.clone(), }) .await; + + // Fire PostReview hook after fix loop completes (final result). + let fix_passed = matches!(&fix_exit_code, Ok(0)); + crate::pipeline::fire_observational_hook( + csa_hooks::HookEvent::PostReview, + &[ + ("session_id", result.meta_session_id.as_str()), + ("decision", if fix_passed { "pass" } else { "fail" }), + ("verdict", if fix_passed { CLEAN } else { verdict }), + ("scope", &scope_for_hook), + ], + &project_root, + ); + + return fix_exit_code; } if args.fix { @@ -669,6 +698,38 @@ async fn execute_review( Ok(execution) } +/// Compute a SHA-256 content hash of the diff being reviewed. +/// +/// The fingerprint enables diff-level deduplication: if two review +/// invocations produce the same diff content (e.g., revert-then-revert), +/// the second can reuse the first review's result. +fn compute_diff_fingerprint(project_root: &Path, scope: &str) -> Option { + use sha2::{Digest, Sha256}; + + let diff_args: Vec<&str> = if scope == "uncommitted" { + vec!["diff", "HEAD"] + } else if let Some(range) = scope.strip_prefix("range:") { + vec!["diff", range] + } else if let Some(base) = scope.strip_prefix("base:") { + vec!["diff", base] + } else { + return None; + }; + + let output = std::process::Command::new("git") + .args(&diff_args) + .current_dir(project_root) + .output() + .ok()?; + + if !output.status.success() || output.stdout.is_empty() { + return None; + } + + let digest = Sha256::digest(&output.stdout); + Some(format!("sha256:{digest:x}")) +} + #[cfg(test)] #[path = "review_cmd_tests.rs"] mod tests; diff --git a/crates/cli-sub-agent/src/review_cmd_fix.rs b/crates/cli-sub-agent/src/review_cmd_fix.rs index 92193273..f021b6e5 100644 --- a/crates/cli-sub-agent/src/review_cmd_fix.rs +++ b/crates/cli-sub-agent/src/review_cmd_fix.rs @@ -177,6 +177,7 @@ pub(crate) async fn run_fix_loop(ctx: FixLoopContext<'_>) -> Result { fix_attempted: true, fix_rounds: u32::from(round), timestamp: chrono::Utc::now(), + diff_fingerprint: None, }, ); return Ok(0); @@ -197,6 +198,7 @@ pub(crate) async fn run_fix_loop(ctx: FixLoopContext<'_>) -> Result { fix_attempted: true, fix_rounds: u32::from(ctx.max_rounds), timestamp: chrono::Utc::now(), + diff_fingerprint: None, }, ); error!( diff --git a/crates/csa-hooks/src/directive.rs b/crates/csa-hooks/src/directive.rs new file mode 100644 index 00000000..cfd90da5 --- /dev/null +++ b/crates/csa-hooks/src/directive.rs @@ -0,0 +1,70 @@ +//! CSA directive parsing from hook/step output. +//! +//! Directives are HTML-comment-style markers embedded in stdout: +//! `` — instruct weave to jump to a step. + +/// A parsed CSA directive from hook or step output. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum CsaDirective { + /// Jump to the named weave step. + NextStep { step_id: String }, +} + +/// Parse all `CSA:NEXT_STEP` directives from text output. +/// +/// Returns the **last** `NEXT_STEP` directive found (later directives +/// override earlier ones, matching the "last writer wins" convention). +pub fn parse_next_step(output: &str) -> Option { + let mut last_step_id = None; + + for line in output.lines() { + let trimmed = line.trim(); + // Match: + if let Some(rest) = trimmed.strip_prefix("") + { + let rest = rest.trim(); + if let Some(value) = rest.strip_prefix("step_id=") { + let id = value.trim().trim_matches('"').trim(); + if !id.is_empty() { + last_step_id = Some(id.to_string()); + } + } + } + } + + last_step_id +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parse_next_step_basic() { + let output = "some output\n\nmore output"; + assert_eq!(parse_next_step(output), Some("merge".to_string())); + } + + #[test] + fn parse_next_step_quoted() { + let output = ""; + assert_eq!(parse_next_step(output), Some("push_and_review".to_string())); + } + + #[test] + fn parse_next_step_last_wins() { + let output = "\n"; + assert_eq!(parse_next_step(output), Some("second".to_string())); + } + + #[test] + fn parse_next_step_none() { + assert_eq!(parse_next_step("no directives here"), None); + } + + #[test] + fn parse_next_step_empty_id() { + assert_eq!(parse_next_step(""), None); + } +} diff --git a/crates/csa-hooks/src/event.rs b/crates/csa-hooks/src/event.rs index 3aef4245..e4bc99f4 100644 --- a/crates/csa-hooks/src/event.rs +++ b/crates/csa-hooks/src/event.rs @@ -30,6 +30,10 @@ pub enum HookEvent { /// Observational: runs a quick clippy check on changed crates. /// Triggered in `pipeline_post_exec::process_execution_result`. PostEdit, + /// After a review completes (success or failure). + /// Observational: useful for chaining review → next step in pipelines. + /// Template vars: `{session_id}`, `{decision}`, `{verdict}`, `{scope}`. + PostReview, } impl HookEvent { @@ -49,6 +53,7 @@ impl HookEvent { HookEvent::PreRun => "pre_run", HookEvent::PostRun => "post_run", HookEvent::PostEdit => "post_edit", + HookEvent::PostReview => "post_review", } } @@ -102,7 +107,8 @@ impl HookEvent { HookEvent::TodoCreate | HookEvent::TodoSave | HookEvent::PreRun - | HookEvent::PostRun => None, + | HookEvent::PostRun + | HookEvent::PostReview => None, } } } @@ -122,6 +128,7 @@ mod tests { assert_eq!(HookEvent::PreRun.as_config_key(), "pre_run"); assert_eq!(HookEvent::PostRun.as_config_key(), "post_run"); assert_eq!(HookEvent::PostEdit.as_config_key(), "post_edit"); + assert_eq!(HookEvent::PostReview.as_config_key(), "post_review"); } #[test] @@ -137,6 +144,7 @@ mod tests { assert!(HookEvent::TodoSave.builtin_command().is_none()); assert!(HookEvent::PreRun.builtin_command().is_none()); assert!(HookEvent::PostRun.builtin_command().is_none()); + assert!(HookEvent::PostReview.builtin_command().is_none()); } #[test] @@ -158,6 +166,7 @@ mod tests { HookEvent::PreRun, HookEvent::PostRun, HookEvent::PostEdit, + HookEvent::PostReview, ]; let mut seen_keys = std::collections::HashSet::new(); @@ -169,8 +178,8 @@ mod tests { "Duplicate config key: {key} (from {event:?})" ); } - // Ensure we covered all 6 variants - assert_eq!(seen_keys.len(), 6, "Expected 6 unique config keys"); + // Ensure we covered all 7 variants + assert_eq!(seen_keys.len(), 7, "Expected 7 unique config keys"); } #[test] @@ -203,6 +212,11 @@ mod tests { assert!(!HookEvent::PostEdit.is_gatekeeping()); } + #[test] + fn test_is_gatekeeping_post_review_false() { + assert!(!HookEvent::PostReview.is_gatekeeping()); + } + /// Verify config keys match the expected snake_case convention. #[test] fn test_config_keys_are_snake_case() { @@ -213,6 +227,7 @@ mod tests { HookEvent::PreRun, HookEvent::PostRun, HookEvent::PostEdit, + HookEvent::PostReview, ]; for event in &all_events { diff --git a/crates/csa-hooks/src/lib.rs b/crates/csa-hooks/src/lib.rs index 9e74ae64..b5ebbd29 100644 --- a/crates/csa-hooks/src/lib.rs +++ b/crates/csa-hooks/src/lib.rs @@ -43,6 +43,7 @@ //! - `{message}`: Commit message pub mod config; +pub mod directive; pub mod event; pub mod event_bus; pub mod guard; @@ -53,6 +54,7 @@ pub mod waiver; // Re-export key types pub use config::{HookConfig, HooksConfig, global_hooks_path, load_hooks_config}; +pub use directive::parse_next_step; pub use event::HookEvent; #[cfg(feature = "async-hooks")] pub use event_bus::AsyncEventBus; @@ -62,5 +64,5 @@ pub use guard::{ run_prompt_guards, }; pub use policy::FailPolicy; -pub use runner::{run_hook, run_hooks_for_event}; +pub use runner::{run_hook, run_hook_capturing, run_hooks_for_event}; pub use waiver::{Waiver, WaiverSet}; diff --git a/crates/csa-hooks/src/runner.rs b/crates/csa-hooks/src/runner.rs index 2af4884f..822f099a 100644 --- a/crates/csa-hooks/src/runner.rs +++ b/crates/csa-hooks/src/runner.rs @@ -161,6 +161,62 @@ pub fn run_hook( } } +/// Execute a hook command capturing stdout for directive parsing. +/// +/// Same as `run_hook` but returns the stdout content on success, +/// enabling callers to parse directives like `CSA:NEXT_STEP`. +pub fn run_hook_capturing( + event: HookEvent, + config: &HookConfig, + variables: &HashMap, +) -> Result { + if !config.enabled { + return Ok(String::new()); + } + + let template = match config.command.as_deref() { + Some(cmd) => cmd, + None => match event.builtin_command() { + Some(cmd) => cmd, + None => return Ok(String::new()), + }, + }; + + let expanded_command = substitute_variables(template, variables); + tracing::debug!(event = ?event, "Executing hook (capturing)"); + + let mut child = Command::new("sh") + .arg("-c") + .arg(&expanded_command) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn()?; + + let timeout = Duration::from_secs(config.timeout_secs); + let start = Instant::now(); + + loop { + match child.try_wait()? { + Some(status) => { + let output = child.wait_with_output()?; + if !status.success() { + let exit_code = status.code().unwrap_or(-1); + bail!("Hook {event:?} exited with code {exit_code}"); + } + return Ok(String::from_utf8_lossy(&output.stdout).to_string()); + } + None => { + if start.elapsed() >= timeout { + let _ = child.kill(); + let _ = child.wait(); + bail!("Hook {event:?} timed out after {}s", config.timeout_secs); + } + std::thread::sleep(Duration::from_millis(100)); + } + } + } +} + /// Execute all hooks for an event, using the merged config. /// /// This wraps `run_hook` with per-hook fail-policy and waiver enforcement. diff --git a/crates/csa-session/src/state.rs b/crates/csa-session/src/state.rs index a74afcff..5b699daa 100644 --- a/crates/csa-session/src/state.rs +++ b/crates/csa-session/src/state.rs @@ -146,6 +146,11 @@ pub struct ReviewSessionMeta { pub fix_rounds: u32, /// ISO 8601 timestamp of when this metadata was written. pub timestamp: DateTime, + /// Content hash of the diff being reviewed (e.g., "sha256:abc123..."). + /// Enables deduplication: revert-revert scenarios with identical diffs + /// can reuse a previous review without re-running. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub diff_fingerprint: Option, } /// Write review session metadata to the session directory. diff --git a/crates/csa-session/src/state_tests.rs b/crates/csa-session/src/state_tests.rs index 5e38102d..b0795a7d 100644 --- a/crates/csa-session/src/state_tests.rs +++ b/crates/csa-session/src/state_tests.rs @@ -661,6 +661,7 @@ fn review_session_meta_serde_roundtrip() { fix_attempted: true, fix_rounds: 2, timestamp: chrono::Utc::now(), + diff_fingerprint: Some("sha256:abc123".to_string()), }; let json = serde_json::to_string_pretty(&meta).expect("serialize"); @@ -682,6 +683,7 @@ fn review_session_meta_write_and_read() { fix_attempted: false, fix_rounds: 0, timestamp: chrono::Utc::now(), + diff_fingerprint: None, }; write_review_meta(td.path(), &meta).expect("write"); @@ -712,6 +714,7 @@ fn review_session_meta_overwrite_on_fix_round() { fix_attempted: false, fix_rounds: 0, timestamp: chrono::Utc::now(), + diff_fingerprint: None, }; write_review_meta(td.path(), &meta1).expect("write initial"); @@ -727,6 +730,7 @@ fn review_session_meta_overwrite_on_fix_round() { fix_attempted: true, fix_rounds: 1, timestamp: chrono::Utc::now(), + diff_fingerprint: Some("sha256:def456".to_string()), }; write_review_meta(td.path(), &meta2).expect("write after fix");