feat(hooks,session): PostReview hook, NEXT_STEP directive, diff fingerprint#529
Conversation
…rprint - Add PostReview hook event (observational) fired after csa review completes - Add fire_observational_hook() helper for easy dispatch with auto-loaded config - Add run_hook_capturing() for stdout-captured hook execution - Add CSA:NEXT_STEP directive parser (csa-hooks/directive.rs) - Add diff_fingerprint (SHA-256 content hash) to ReviewSessionMeta Enables review deduplication for revert-revert scenarios with identical diffs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db3bc229af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
crates/csa-hooks/src/runner.rs
Outdated
| .arg(&expanded_command) | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::null()) | ||
| .output()?; |
There was a problem hiding this comment.
run_hook_capturing claims to behave like run_hook, but this implementation uses blocking Command::output() and never applies config.timeout_secs (or process-group kill), so a hung hook command (for example sleep 9999) can block the caller indefinitely. Any workflow that uses this API for directive parsing can deadlock instead of failing fast like run_hook does.
Useful? React with 👍 / 👎.
| crate::pipeline::fire_observational_hook( | ||
| csa_hooks::HookEvent::PostReview, | ||
| &[ | ||
| ("session_id", result.meta_session_id.as_str()), | ||
| ("decision", decision.as_str()), |
There was a problem hiding this comment.
This PostReview hook is emitted before the --fix path runs, so when an initial review fails but subsequent fix rounds succeed, hook variables (decision/verdict) still report the pre-fix failure. Pipelines that branch on post_review outcomes can therefore trigger the wrong next step or notification for csa review --fix runs.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request increments the workspace version to 0.1.195 and introduces several enhancements to the hook and review systems. Key changes include the implementation of a diff fingerprinting mechanism using SHA-256 for deduplication, the addition of a PostReview hook event, and a new directive parsing system to extract instructions like NEXT_STEP from hook output. Feedback identifies a critical missing timeout in the run_hook_capturing function that could cause processes to hang, a suggestion to simplify the git argument construction logic, and a note regarding an unused enum that will trigger a dead code warning.
| pub fn run_hook_capturing( | ||
| event: HookEvent, | ||
| config: &HookConfig, | ||
| variables: &HashMap<String, String>, | ||
| ) -> Result<String> { | ||
| 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 output = Command::new("sh") | ||
| .arg("-c") | ||
| .arg(&expanded_command) | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::null()) | ||
| .output()?; | ||
|
|
||
| if !output.status.success() { | ||
| let exit_code = output.status.code().unwrap_or(-1); | ||
| bail!("Hook {event:?} exited with code {exit_code}"); | ||
| } | ||
|
|
||
| Ok(String::from_utf8_lossy(&output.stdout).to_string()) | ||
| } |
There was a problem hiding this comment.
The function run_hook_capturing does not implement a timeout, unlike run_hook. It uses Command::output() which blocks until the process completes, potentially indefinitely. The HookConfig contains a timeout_secs field which is used by run_hook but ignored here. This could cause a hook to hang the process.
A timeout mechanism similar to the one in run_hook should be implemented to prevent this.
| 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; | ||
| }; |
There was a problem hiding this comment.
The construction of diff_args can be simplified to avoid repetition and improve conciseness.
| 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 diff_param = if scope == "uncommitted" { | |
| "HEAD" | |
| } else if let Some(range) = scope.strip_prefix("range:") { | |
| range | |
| } else if let Some(base) = scope.strip_prefix("base:") { | |
| base | |
| } else { | |
| return None; | |
| }; | |
| let diff_args = ["diff", diff_param]; |
| pub enum CsaDirective { | ||
| /// Jump to the named weave step. | ||
| NextStep { step_id: String }, | ||
| } |
There was a problem hiding this comment.
The CsaDirective enum is defined but not used anywhere in the crate. This will result in a dead_code warning. If it's intended for future use, consider adding #[allow(dead_code)] to suppress the warning. Otherwise, it should be removed.
- Add timeout to run_hook_capturing (P1: prevents infinite blocking) - Move PostReview hook after fix loop (P2: fires only on final result) - Fix runner.rs syntax (stale code from edit)
Bot Findings Staleness Analysis (Round 2)All HIGH/P1/P2 findings are stale repeats of issues already fixed in commit 983cd05:
Remaining MEDIUM findings are non-blocking:
|
1 participant
Summary
PostReviewhook event (observational) with session_id/decision/verdict/scope variablesrun_hook_capturing()for stdout-captured hook execution (enables directive parsing)CSA:NEXT_STEPdirective parser (csa-hooks/directive.rs) for weave pipeline chainingdiff_fingerprint(SHA-256 content hash) field toReviewSessionMetafire_observational_hook()helper to pipeline.rs for easy dispatchTest plan
just pre-commitpassescsa review --range main...HEADPASS🤖 Generated with Claude Code