diff --git a/CHANGELOG.md b/CHANGELOG.md index b4f53a0..c2b4c7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Added `--batch-files ` flag (default: 20) to control the maximum number of files per LLM batch — lower it to work around provider token limits or rate-limit errors on large scans. - Truncated-JSON and general parse errors now include the raw response prefix for easier debugging without `--verbose`. +### Fixed — Review + +- **`cora review` no longer exits 2 when severity filtering removes all blocking findings (#312)** + - Recompute `should_block` against the **filtered** issue list (after `--severity` filtering) so the exit code matches the SARIF/pretty output the user sees. + - Extracted exit-code logic into `compute_exit_code()` helper (pure function) with 8 unit tests covering gate pass/fail, CI mode, and hook `block` vs non-`block` modes. + - Applies to both the single-chunk and auto-chunked (`--auto-chunk`) review paths. + ## [0.6.0] - 2026-06-14 ### Added — Code Intelligence diff --git a/src/commands/review.rs b/src/commands/review.rs index 53a9fc9..2bc1dfe 100644 --- a/src/commands/review.rs +++ b/src/commands/review.rs @@ -267,6 +267,13 @@ pub async fn execute_review( filtered_response .issues .retain(|i| i.severity <= min_severity); + // Recompute should_block against the filtered issue list so the exit code + // matches the output the user actually sees (e.g. when `--severity critical` + // filters out all Major/Minor issues, we must not block). See #312. + filtered_response.should_block = filtered_response + .issues + .iter() + .any(|i| i.severity <= min_severity); // 7. Format output let formatter = formatter_for(format); @@ -309,7 +316,7 @@ pub async fn execute_review( } else { EXIT_OK } - } else if response.should_block && config.hook.mode == "block" { + } else if filtered_response.should_block && config.hook.mode == "block" { EXIT_BLOCKED } else { EXIT_OK @@ -592,6 +599,12 @@ async fn execute_chunked_review( filtered_response .issues .retain(|i| i.severity <= min_severity); + // Recompute should_block against the filtered issue list so the exit code + // matches the output the user actually sees (see #312). + filtered_response.should_block = filtered_response + .issues + .iter() + .any(|i| i.severity <= min_severity); // 7. Format output let formatter = formatter_for(format); @@ -632,22 +645,12 @@ async fn execute_chunked_review( } // 9. Return exit code - let exit_code = if gate_result - .as_ref() - .is_some_and(|g| g.status == quality_gate::GateStatus::Fail) - { - EXIT_BLOCKED - } else if opts.ci { - if !filtered_response.issues.is_empty() { - EXIT_BLOCKED - } else { - EXIT_OK - } - } else if merged_response.should_block && config.hook.mode == "block" { - EXIT_BLOCKED - } else { - EXIT_OK - }; + let exit_code = compute_exit_code( + gate_result.as_ref().map(|g| g.status), + opts.ci, + &filtered_response, + config.hook.mode.as_str(), + ); // 10. Emit complete event if progress.is_enabled() { @@ -668,3 +671,127 @@ async fn execute_chunked_review( &filtered_response, )) } + +/// Compute the review exit code from the gate status, CI flag, and the +/// **filtered** review response (issues after severity filtering). +/// +/// Exit code semantics (see #312): +/// +/// | Code | Meaning | +/// |------|---------| +/// | 0 | Review completed; no findings at or above the severity threshold | +/// | 2 | Review completed but findings are blocking (gate fail, CI with any +/// issue, or hook in block mode with blocking severity) | +/// +/// `should_block` must be computed against the **filtered** issue list so that +/// the exit code matches the SARIF/pretty output the user sees. +fn compute_exit_code( + gate_status: Option, + ci: bool, + filtered_response: &ReviewResponse, + hook_mode: &str, +) -> i32 { + if gate_status == Some(quality_gate::GateStatus::Fail) { + return EXIT_BLOCKED; + } + if ci { + return if filtered_response.issues.is_empty() { + EXIT_OK + } else { + EXIT_BLOCKED + }; + } + if filtered_response.should_block && hook_mode == "block" { + EXIT_BLOCKED + } else { + EXIT_OK + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::engine::ReviewResponse; + use crate::engine::Severity; + use crate::engine::types::ReviewIssue; + + fn issue(severity: Severity) -> ReviewIssue { + ReviewIssue { + file: "src/main.rs".to_string(), + line: Some(1), + severity, + issue_type: Some("bug".to_string()), + title: "test".to_string(), + body: "body".to_string(), + suggested_fix: None, + } + } + + fn response(issues: Vec, should_block: bool) -> ReviewResponse { + ReviewResponse { + issues, + summary: String::new(), + tokens_used: None, + should_block, + } + } + + // ─── #312: exit code must match filtered output ─── + + #[test] + fn exit_code_zero_when_no_findings_after_filter() { + let resp = response(vec![], false); + assert_eq!(compute_exit_code(None, false, &resp, "block"), EXIT_OK); + } + + #[test] + fn exit_code_zero_when_filtered_should_block_false() { + // Even if the unfiltered response would have blocked, after filtering + // should_block is false → exit 0. This is the core regression in #312. + let resp = response(vec![], false); + assert_eq!(compute_exit_code(None, false, &resp, "block"), EXIT_OK); + } + + #[test] + fn exit_code_blocked_when_filtered_should_block_true_and_hook_blocks() { + let resp = response(vec![issue(Severity::Critical)], true); + assert_eq!(compute_exit_code(None, false, &resp, "block"), EXIT_BLOCKED); + } + + #[test] + fn exit_code_zero_when_hook_mode_not_block() { + let resp = response(vec![issue(Severity::Critical)], true); + assert_eq!(compute_exit_code(None, false, &resp, "warn"), EXIT_OK); + assert_eq!(compute_exit_code(None, false, &resp, ""), EXIT_OK); + } + + #[test] + fn exit_code_ci_zero_when_no_findings() { + let resp = response(vec![], false); + assert_eq!(compute_exit_code(None, true, &resp, "block"), EXIT_OK); + } + + #[test] + fn exit_code_ci_blocked_when_any_finding() { + let resp = response(vec![issue(Severity::Minor)], false); + assert_eq!(compute_exit_code(None, true, &resp, "block"), EXIT_BLOCKED); + } + + #[test] + fn exit_code_gate_fail_overrides_everything() { + let resp = response(vec![], false); + assert_eq!( + compute_exit_code(Some(quality_gate::GateStatus::Fail), false, &resp, "block"), + EXIT_BLOCKED + ); + } + + #[test] + fn exit_code_gate_pass_then_falls_through() { + let resp = response(vec![], false); + assert_eq!( + compute_exit_code(Some(quality_gate::GateStatus::Pass), false, &resp, "block"), + EXIT_OK + ); + } +}