From 74d512d0345233c3b532bba58c8845768634f8c8 Mon Sep 17 00:00:00 2001 From: ajianaz Date: Thu, 15 Oct 2026 11:10:28 +0700 Subject: [PATCH] refactor(review): one finalize module for the result tail (#648) Replace six hand-copied versions of severity filtering, should_block, quality gate, exit code, debt snapshot and review history (review single and chunked, commit, scan, MCP, engine fallback) with engine::finalize. MCP review_diff now honors min_severity and gates on the reported findings; commit records its review in the history. Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: ajianaz --- CHANGELOG.md | 4 + src/commands/commit_cmd.rs | 89 +++------- src/commands/review.rs | 351 +++++++------------------------------ src/commands/scan.rs | 59 +++---- src/engine/finalize.rs | 351 +++++++++++++++++++++++++++++++++++++ src/engine/mod.rs | 1 + src/engine/review.rs | 18 +- src/mcp/tools.rs | 35 +++- 8 files changed, 497 insertions(+), 411 deletions(-) create mode 100644 src/engine/finalize.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b80daf..cce3b7c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Bumped `rustls` 0.23.45 → 0.23.46 and `quinn-proto` 0.11.16 → 0.11.19** (RUSTSEC-2026-0338/0339/0340/0341/0342 and RUSTSEC-2026-0346/0347/0348/0349), plus the yanked `chacha20` 0.10.1 → 0.10.2. Lockfile-only patch bumps; this was failing the `Security Audit (CVE)` job on every PR. +### Changed + +- **One `finalize` module owns the result tail of review, scan, commit and MCP (#648).** `engine::finalize` (`shape`, `shape_issues`, `blocks`, `exit_code`, `save_debt`, `save_history`) replaces six hand-copied versions of severity filtering, `should_block`, quality gate, exit code, debt snapshot and review history. Behavior of `cora review` (single and chunked) and `cora scan` is unchanged. Fixed along the way: the MCP `cora.review_diff` tool now honors its `min_severity` parameter and evaluates the quality gate on the reported findings; `cora commit` now records its review in the review history (`command = "commit"`). + ## [0.18.0] - 2026-10-09 ### Security diff --git a/src/commands/commit_cmd.rs b/src/commands/commit_cmd.rs index d555fa0..038b1c0 100644 --- a/src/commands/commit_cmd.rs +++ b/src/commands/commit_cmd.rs @@ -5,6 +5,7 @@ use colored::Colorize; use std::io::{self, Write}; use crate::config::schema::Config; +use crate::engine::finalize::{self, HistoryEntry, ShapeOptions}; use crate::engine::llm; use crate::engine::quality_gate; use crate::engine::types::LLMConfig; @@ -95,41 +96,34 @@ pub async fn execute_commit( ) .await?; - // Filter by severity - let min_severity = config.hook.min_severity_level(); + // Filter by severity, evaluate the quality gate let mut filtered = response.clone(); - filtered.issues.retain(|i| i.severity <= min_severity); - - // Quality gate - let gate_result = if config.quality_gate.enabled { - let result = quality_gate::evaluate(&filtered.issues, &config.quality_gate); - Some(result) - } else { - None - }; - - // Check gate - let gate_failed = gate_result - .as_ref() - .is_some_and(|g| g.status == quality_gate::GateStatus::Fail); - - // Save debt snapshot (best-effort) - if config.debt.enabled { - let (commit, branch) = get_git_context(); - let snapshot = crate::engine::debt_tracker::DebtSnapshot::from_review( - &filtered.issues, - gate_result.as_ref(), - commit, - branch, - 0, - None, - None, - ); - crate::engine::debt_tracker::save_snapshot( - &snapshot, - config.debt.history_dir.as_deref(), - ); - } + let shaped = finalize::shape( + &mut filtered, + config, + &ShapeOptions { + min_severity: config.hook.min_severity_level(), + filter: true, + gate: true, + ci: false, + }, + ); + let gate_result = shaped.gate.clone(); + let gate_failed = shaped.gate_failed(); + + // Debt snapshot + review history (best-effort) + finalize::save_debt(config, &filtered.issues, gate_result.as_ref(), 0); + finalize::save_history( + &HistoryEntry { + command: "commit", + summary: &filtered.summary, + files_scanned: 0, + lines_scanned: 0, + tokens: response.tokens_used.as_ref(), + }, + &filtered.issues, + &shaped, + ); if gate_failed && !opts.force { // Print findings @@ -457,33 +451,6 @@ fn do_commit(message: &str, quiet: bool) -> Result { } } -/// Get git context (cached). -fn get_git_context() -> (Option, Option) { - use std::sync::LazyLock; - - static CONTEXT: LazyLock<(Option, Option)> = LazyLock::new(|| { - let commit = std::process::Command::new("git") - .args(["rev-parse", "--short", "HEAD"]) - .output() - .ok() - .and_then(|o| { - if o.status.success() { - String::from_utf8(o.stdout) - .ok() - .map(|s| s.trim().to_string()) - } else { - None - } - }); - - let branch = crate::git::get_current_branch().ok(); - - (commit, branch) - }); - - (CONTEXT.0.clone(), CONTEXT.1.clone()) -} - /// Count files and lines in a diff. fn diff_stats(diff: &str) -> (usize, usize) { let mut files = 0usize; diff --git a/src/commands/review.rs b/src/commands/review.rs index f8572d1..0b1ea01 100644 --- a/src/commands/review.rs +++ b/src/commands/review.rs @@ -5,16 +5,15 @@ use tracing::debug; use crate::config::schema::Config; use crate::engine::Severity; use crate::engine::chunker; +use crate::engine::finalize::{self, HistoryEntry, ShapeOptions}; use crate::engine::quality_gate; -use crate::engine::review_store; use crate::engine::types::ReviewResponse; use crate::formatters::{OutputFormat, formatter_for}; use crate::git; use crate::progress::{ProgressReporter, TokenInfo, diff_stats}; /// Exit codes for the review command. -pub const EXIT_OK: i32 = 0; -pub const EXIT_BLOCKED: i32 = 2; +pub use crate::engine::types::{EXIT_BLOCKED, EXIT_OK}; /// Review command options. #[allow(clippy::struct_excessive_bools)] @@ -256,7 +255,7 @@ pub async fn execute_review( warn_dropped(response.dropped_findings, config, opts.quiet); - // 6. Filter by severity if specified + // 6. Filter by severity, decide gate / blocking / exit code let min_severity = if let Some(ref sev) = opts.severity { Severity::from_str_lossy(sev) } else { @@ -264,94 +263,38 @@ pub async fn execute_review( }; let mut filtered_response = response.clone(); - // Keep issues at or above min_severity (Critical=worst, Info=lowest in Ord but - // Ord order is Critical(0) < Major(1) < Minor(2) < Info(3), so we invert: keep - // where severity Ord value <= min_severity Ord value). - 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); + let shaped = finalize::shape( + &mut filtered_response, + config, + &ShapeOptions { + min_severity, + filter: true, + gate: true, + ci: opts.ci, + }, + ); // 7. Format output let formatter = formatter_for(format); let mut output = formatter.format_review(&filtered_response)?; - // 8. Quality gate evaluation - let gate_result = if config.quality_gate.enabled { - let result = quality_gate::evaluate(&filtered_response.issues, &config.quality_gate); - output.push_str(&quality_gate::format_gate_output(&result)); - Some(result) - } else { - None - }; - - // 8b. Save debt tracking snapshot (best-effort, never fails review) - if config.debt.enabled { - let (commit, branch) = get_git_context(); - let snapshot = crate::engine::debt_tracker::DebtSnapshot::from_review( - &filtered_response.issues, - gate_result.as_ref(), - commit, - branch, - 0, // files_reviewed not tracked per-review yet - None, - None, // duration not passed through yet - ); - crate::engine::debt_tracker::save_snapshot(&snapshot, config.debt.history_dir.as_deref()); + // 8. Quality gate output, debt snapshot, review history (best-effort) + if let Some(gate) = &shaped.gate { + output.push_str(&quality_gate::format_gate_output(gate)); } - - // 8c. Save review findings to cora.db (best-effort) - { - let (commit, branch) = get_git_context(); - let cmd = if opts.ci { "review --ci" } else { "review" }; - let gate_status = match gate_result.as_ref().map(|g| g.status) { - Some(quality_gate::GateStatus::Pass) => "passed", - Some(quality_gate::GateStatus::Fail) => "failed", - None => "disabled", - }; - let tokens = response.tokens_used.as_ref(); - let cwd = std::env::current_dir() - .map(|p| p.to_string_lossy().to_string()) - .unwrap_or_default(); - let record = review_store::ReviewRecord { - command: cmd, - project_root: &cwd, - commit_hash: commit.as_deref(), - branch: branch.as_deref(), + finalize::save_debt(config, &filtered_response.issues, shaped.gate.as_ref(), 0); + finalize::save_history( + &HistoryEntry { + command: review_command(opts), summary: &filtered_response.summary, - gate_status, files_scanned: 0, lines_scanned: 0, - should_block: filtered_response.should_block, - tokens, - issues: &filtered_response.issues, - }; - // Best-effort: a history-write failure never fails the run. - review_store::persist_review_best_effort(&record); - } - let exit_code = if gate_result - .as_ref() - .is_some_and(|g| g.status == quality_gate::GateStatus::Fail) - { - EXIT_BLOCKED - } else if opts.ci { - // CI mode: hard gate — any finding blocks - if !filtered_response.issues.is_empty() { - EXIT_BLOCKED - } else { - EXIT_OK - } - } else if filtered_response.should_block && config.hook.mode == "block" { - EXIT_BLOCKED - } else { - EXIT_OK - }; + tokens: response.tokens_used.as_ref(), + }, + &filtered_response.issues, + &shaped, + ); + let exit_code = shaped.exit_code; // 10. Emit complete event if progress.is_enabled() { @@ -372,32 +315,9 @@ pub async fn execute_review( &filtered_response, )) } -/// Returns (Some(commit), Some(branch)) if in a git repo, (None, None) otherwise. -/// Results are cached per process to avoid spawning git on every review. -fn get_git_context() -> (Option, Option) { - use std::sync::LazyLock; - - static CONTEXT: LazyLock<(Option, Option)> = LazyLock::new(|| { - let commit = std::process::Command::new("git") - .args(["rev-parse", "--short", "HEAD"]) - .output() - .ok() - .and_then(|o| { - if o.status.success() { - String::from_utf8(o.stdout) - .ok() - .map(|s| s.trim().to_string()) - } else { - None - } - }); - - let branch = crate::git::get_current_branch().ok(); - - (commit, branch) - }); - - (CONTEXT.0.clone(), CONTEXT.1.clone()) +/// Command label stored in the review history. +fn review_command(opts: &ReviewOptions) -> &'static str { + if opts.ci { "review --ci" } else { "review" } } /// Merged summary for chunked review reports (#556). @@ -643,7 +563,7 @@ async fn execute_chunked_review( dropped_findings, }; - // 6. Filter by severity if specified + // 6. Filter by severity, decide gate / blocking / exit code let min_severity = if let Some(ref sev) = opts.severity { Severity::from_str_lossy(sev) } else { @@ -651,89 +571,50 @@ async fn execute_chunked_review( }; let mut filtered_response = merged_response.clone(); - 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); + let shaped = finalize::shape( + &mut filtered_response, + config, + &ShapeOptions { + min_severity, + filter: true, + gate: true, + ci: opts.ci, + }, + ); // 7. Format output let formatter = formatter_for(format); let mut output = formatter.format_review(&filtered_response)?; // Add chunking summary header if not quiet and format is pretty + let files_total: usize = chunks.iter().map(|c| c.file_count).sum(); if !opts.quiet && format == OutputFormat::Pretty { - let header = format!( - "📦 Reviewed in {} chunks ({} files total)\n", - total_chunks, - chunks.iter().map(|c| c.file_count).sum::() - ); + let header = format!("📦 Reviewed in {total_chunks} chunks ({files_total} files total)\n"); output = format!("{header}{output}"); } - // 8. Quality gate evaluation - let gate_result = if config.quality_gate.enabled { - let result = quality_gate::evaluate(&filtered_response.issues, &config.quality_gate); - output.push_str(&quality_gate::format_gate_output(&result)); - Some(result) - } else { - None - }; - - // 8b. Save debt tracking snapshot (best-effort, never fails review) - if config.debt.enabled { - let (commit, branch) = get_git_context(); - let snapshot = crate::engine::debt_tracker::DebtSnapshot::from_review( - &filtered_response.issues, - gate_result.as_ref(), - commit, - branch, - chunks.iter().map(|c| c.file_count).sum(), - None, - None, - ); - crate::engine::debt_tracker::save_snapshot(&snapshot, config.debt.history_dir.as_deref()); + // 8. Quality gate output, debt snapshot, review history (best-effort) + if let Some(gate) = &shaped.gate { + output.push_str(&quality_gate::format_gate_output(gate)); } - - // 8c. Save review findings to cora.db (best-effort) - { - let (commit, branch) = get_git_context(); - let cmd = if opts.ci { "review --ci" } else { "review" }; - let gate_status = match gate_result.as_ref().map(|g| g.status) { - Some(quality_gate::GateStatus::Pass) => "passed", - Some(quality_gate::GateStatus::Fail) => "failed", - None => "disabled", - }; - let tokens = merged_response.tokens_used.as_ref(); - let cwd = std::env::current_dir() - .map(|p| p.to_string_lossy().to_string()) - .unwrap_or_default(); - let record = review_store::ReviewRecord { - command: cmd, - project_root: &cwd, - commit_hash: commit.as_deref(), - branch: branch.as_deref(), + finalize::save_debt( + config, + &filtered_response.issues, + shaped.gate.as_ref(), + files_total, + ); + finalize::save_history( + &HistoryEntry { + command: review_command(opts), summary: &filtered_response.summary, - gate_status, files_scanned: 0, lines_scanned: 0, - should_block: filtered_response.should_block, - tokens, - issues: &filtered_response.issues, - }; - // Best-effort: a history-write failure never fails the run. - review_store::persist_review_best_effort(&record); - } - let exit_code = compute_exit_code( - gate_result.as_ref().map(|g| g.status), - opts.ci, - &filtered_response, - config.hook.mode.as_str(), + tokens: merged_response.tokens_used.as_ref(), + }, + &filtered_response.issues, + &shaped, ); + let exit_code = shaped.exit_code; // 10. Emit complete event if progress.is_enabled() { @@ -755,42 +636,6 @@ async fn execute_chunked_review( )) } -/// 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 - } -} - /// Print the one-line "findings not shown" warning to stderr (#624). fn warn_dropped(dropped: usize, config: &Config, quiet: bool) { if quiet { @@ -804,25 +649,6 @@ fn warn_dropped(dropped: usize, config: &Config, quiet: bool) { #[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::new("src/main.rs", Some(1), severity, "test") - .with_type("bug") - .with_body("body") - } - - fn response(issues: Vec, should_block: bool) -> ReviewResponse { - ReviewResponse { - issues, - summary: String::new(), - tokens_used: None, - should_block, - dropped_findings: 0, - } - } // ─── #556: chunked summary fallback must reflect actual findings ─── @@ -867,63 +693,4 @@ mod tests { let s = merged_chunk_summary(&summaries, 0, false); assert_eq!(s, "[Chunk 1/2 — a] ok\n\n[Chunk 2/2 — b] fine"); } - - // ─── #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 - ); - } } diff --git a/src/commands/scan.rs b/src/commands/scan.rs index de17856..b2d9352 100644 --- a/src/commands/scan.rs +++ b/src/commands/scan.rs @@ -6,8 +6,8 @@ use tracing::debug; use crate::config::schema::Config; use crate::engine::ReviewIssue; +use crate::engine::finalize::{self, HistoryEntry, ShapeOptions}; use crate::engine::postprocess::{Source, dropped_warning, postprocess_report}; -use crate::engine::review_store; use crate::engine::scanner::{FileEntry, batch_files, format_batch_for_prompt, walk_project}; use crate::engine::types::TokenUsage; use crate::formatters::{OutputFormat, formatter_for}; @@ -254,15 +254,23 @@ pub async fn execute_scan( // apply ignore.rules and inline cora-ignore markers. Deterministic // findings survive an LLM failure (#595). all_issues.extend(index_findings); - let (all_issues, dropped) = finalize_issues(config, &files, all_issues); + let (mut all_issues, dropped) = finalize_issues(config, &files, all_issues); if let Some(w) = dropped_warning(dropped, config) { eprintln!("{}", w.yellow()); } let issue_count = all_issues.len(); - let min_severity = config.hook.min_severity_level(); - // Ord order is Critical(0) < Major(1) < Minor(2) < Info(3), so "at or above - // min_severity" means Ord value <= min_severity. - let should_block = all_issues.iter().any(|i| i.severity <= min_severity); + // Scan lists every finding; only `hook.min_severity` decides blocking. + let shaped = finalize::shape_issues( + &mut all_issues, + config, + &ShapeOptions { + min_severity: config.hook.min_severity_level(), + filter: false, + gate: false, + ci: false, + }, + ); + let should_block = shaped.should_block; let response = crate::engine::ScanResponse { issues: all_issues, @@ -305,44 +313,19 @@ pub async fn execute_scan( } // 7. Save scan findings to cora.db (best-effort) - { - let commit = std::process::Command::new("git") - .args(["rev-parse", "--short", "HEAD"]) - .output() - .ok() - .filter(|o| o.status.success()) - .and_then(|o| String::from_utf8_lossy(&o.stdout).trim().to_string().into()); - let branch = std::process::Command::new("git") - .args(["rev-parse", "--abbrev-ref", "HEAD"]) - .output() - .ok() - .filter(|o| o.status.success()) - .and_then(|o| String::from_utf8_lossy(&o.stdout).trim().to_string().into()); - let cwd = std::env::current_dir() - .map(|p| p.to_string_lossy().to_string()) - .unwrap_or_default(); - let record = review_store::ReviewRecord { + finalize::save_history( + &HistoryEntry { command: "scan", - project_root: &cwd, - commit_hash: commit.as_deref(), - branch: branch.as_deref(), summary: &response.summary, - gate_status: "disabled", files_scanned: response.files_scanned, lines_scanned: response.lines_scanned, - should_block: response.should_block, tokens: response.tokens_used.as_ref(), - issues: &response.issues, - }; - // Best-effort: a history-write failure never fails the run. - review_store::persist_review_best_effort(&record); - } + }, + &response.issues, + &shaped, + ); - if response.should_block && config.hook.mode == "block" { - Ok(2) - } else { - Ok(0) - } + Ok(shaped.exit_code) } /// Run the deterministic scanners (secrets, security) over whole files and diff --git a/src/engine/finalize.rs b/src/engine/finalize.rs new file mode 100644 index 0000000..9460daf --- /dev/null +++ b/src/engine/finalize.rs @@ -0,0 +1,351 @@ +//! The result tail of a review: everything that happens to the merged findings +//! after the judge (LLM or deterministic scanners) is done (#648). +//! +//! One place owns: +//! +//! - the severity filter and the `should_block` decision, +//! - quality-gate evaluation, +//! - the exit code, +//! - the best-effort side effects (debt snapshot, `cora.db` history). +//! +//! Callers (`review`, chunked review, `commit`, `scan`, MCP) do formatting and +//! printing themselves and call [`shape`] first, then format, then the +//! `save_*` functions. `shape` is pure (no IO beyond gate evaluation), so it is +//! testable without an LLM; the `save_*` functions never fail a run. + +use crate::config::schema::Config; +use crate::engine::debt_tracker::{self, DebtSnapshot}; +use crate::engine::quality_gate::{self, GateResult, GateStatus}; +use crate::engine::review_store::{self, ReviewRecord}; +use crate::engine::types::{ + EXIT_BLOCKED, EXIT_OK, ReviewIssue, ReviewResponse, Severity, TokenUsage, +}; + +/// True when any issue is at or above `min` severity. +/// +/// `Severity` orders Critical(0) < Major(1) < Minor(2) < Info(3), so "at or +/// above `min`" means `severity <= min`. +pub fn blocks(issues: &[ReviewIssue], min: Severity) -> bool { + issues.iter().any(|i| i.severity <= min) +} + +/// How [`shape`] should treat the findings. +#[derive(Debug, Clone, Copy)] +pub struct ShapeOptions { + /// Findings below this severity are dropped (when `filter`) and never + /// block. + pub min_severity: Severity, + /// Drop findings below `min_severity` from the list. `cora scan` keeps + /// them all and only uses `min_severity` for the blocking decision. + pub filter: bool, + /// Evaluate the quality gate (when `quality_gate.enabled`). + pub gate: bool, + /// CI mode: any remaining finding blocks. + pub ci: bool, +} + +/// What [`shape`] decided besides the (mutated) findings. +#[derive(Debug, Clone)] +pub struct Shape { + /// Quality-gate result, when the gate ran. + pub gate: Option, + /// Whether the run blocks (see [`exit_code`]). + pub should_block: bool, + /// Process exit code. + pub exit_code: i32, +} + +impl Shape { + /// `passed` / `failed` / `disabled`, as stored in the review history. + pub fn gate_label(&self) -> &'static str { + match self.gate.as_ref().map(|g| g.status) { + Some(GateStatus::Pass) => "passed", + Some(GateStatus::Fail) => "failed", + None => "disabled", + } + } + + /// True when the quality gate ran and failed. + pub fn gate_failed(&self) -> bool { + self.gate + .as_ref() + .is_some_and(|g| g.status == GateStatus::Fail) + } +} + +/// Filter `issues` in place, then decide gate, blocking and exit code against +/// the list the user will actually see (#312). +pub fn shape_issues(issues: &mut Vec, config: &Config, opts: &ShapeOptions) -> Shape { + if opts.filter { + issues.retain(|i| i.severity <= opts.min_severity); + } + let should_block = blocks(issues, opts.min_severity); + let gate = (opts.gate && config.quality_gate.enabled) + .then(|| quality_gate::evaluate(issues, &config.quality_gate)); + let exit_code = exit_code( + gate.as_ref().map(|g| g.status), + opts.ci, + !issues.is_empty(), + should_block, + config.hook.mode.as_str(), + ); + Shape { + gate, + should_block, + exit_code, + } +} + +/// [`shape_issues`] for a [`ReviewResponse`]: also stores the recomputed +/// `should_block` on the response. +pub fn shape(response: &mut ReviewResponse, config: &Config, opts: &ShapeOptions) -> Shape { + let shaped = shape_issues(&mut response.issues, config, opts); + response.should_block = shaped.should_block; + shaped +} + +/// Exit code from the gate status, CI flag, and the **filtered** findings. +/// +/// | Code | Meaning | +/// |------|---------| +/// | 0 | Completed; no findings at or above the severity threshold | +/// | 2 | Findings are blocking: gate fail, CI with any issue, or hook in block mode with blocking severity | +pub fn exit_code( + gate_status: Option, + ci: bool, + has_issues: bool, + should_block: bool, + hook_mode: &str, +) -> i32 { + if gate_status == Some(GateStatus::Fail) { + return EXIT_BLOCKED; + } + if ci { + return if has_issues { EXIT_BLOCKED } else { EXIT_OK }; + } + if should_block && hook_mode == "block" { + EXIT_BLOCKED + } else { + EXIT_OK + } +} + +/// Short commit hash and branch of the current checkout, cached per process. +fn git_context() -> (Option, Option) { + use std::sync::LazyLock; + + static CONTEXT: LazyLock<(Option, Option)> = LazyLock::new(|| { + let commit = std::process::Command::new("git") + .args(["rev-parse", "--short", "HEAD"]) + .output() + .ok() + .filter(|o| o.status.success()) + .and_then(|o| String::from_utf8(o.stdout).ok()) + .map(|s| s.trim().to_string()); + let branch = crate::git::get_current_branch().ok(); + (commit, branch) + }); + + (CONTEXT.0.clone(), CONTEXT.1.clone()) +} + +/// Save a debt-tracking snapshot (best-effort, no-op when `debt.enabled` is +/// off). +pub fn save_debt( + config: &Config, + issues: &[ReviewIssue], + gate: Option<&GateResult>, + files_reviewed: usize, +) { + if !config.debt.enabled { + return; + } + let (commit, branch) = git_context(); + let snapshot = + DebtSnapshot::from_review(issues, gate, commit, branch, files_reviewed, None, None); + debt_tracker::save_snapshot(&snapshot, config.debt.history_dir.as_deref()); +} + +/// What a run records in the review history. +#[derive(Debug, Clone, Copy)] +pub struct HistoryEntry<'a> { + /// `review`, `review --ci`, `scan`, `commit`. + pub command: &'a str, + /// Summary text of the run. + pub summary: &'a str, + /// Files covered by the run (0 when not tracked). + pub files_scanned: usize, + /// Lines covered by the run (0 when not tracked). + pub lines_scanned: usize, + /// Token usage of the judge, if any. + pub tokens: Option<&'a TokenUsage>, +} + +/// Save the findings to `cora.db` (best-effort: a history-write failure never +/// fails the run). +pub fn save_history(entry: &HistoryEntry<'_>, issues: &[ReviewIssue], shaped: &Shape) { + let (commit, branch) = git_context(); + let cwd = std::env::current_dir() + .map(|p| p.to_string_lossy().to_string()) + .unwrap_or_default(); + let record = ReviewRecord { + command: entry.command, + project_root: &cwd, + commit_hash: commit.as_deref(), + branch: branch.as_deref(), + summary: entry.summary, + gate_status: shaped.gate_label(), + files_scanned: entry.files_scanned, + lines_scanned: entry.lines_scanned, + should_block: shaped.should_block, + tokens: entry.tokens, + issues, + }; + review_store::persist_review_best_effort(&record); +} + +#[cfg(test)] +mod tests { + use super::*; + + fn issue(severity: Severity) -> ReviewIssue { + ReviewIssue::new("src/main.rs", Some(1), severity, "test") + .with_type("bug") + .with_body("body") + } + + fn opts(min: Severity) -> ShapeOptions { + ShapeOptions { + min_severity: min, + filter: true, + gate: true, + ci: false, + } + } + + fn config(mode: &str, gate: bool) -> Config { + let mut c = Config::default(); + c.hook.mode = mode.to_string(); + c.quality_gate.enabled = gate; + c + } + + #[test] + fn blocks_is_at_or_above_min_severity() { + let issues = vec![issue(Severity::Minor)]; + assert!(blocks(&issues, Severity::Minor)); + assert!(blocks(&issues, Severity::Info)); + assert!(!blocks(&issues, Severity::Major)); + assert!(!blocks(&[], Severity::Info)); + } + + // ─── #312: the decision matches what the user sees ─── + + #[test] + fn filter_drops_below_threshold_and_recomputes_should_block() { + let mut issues = vec![issue(Severity::Minor), issue(Severity::Info)]; + let shaped = shape_issues(&mut issues, &config("block", false), &opts(Severity::Major)); + assert!(issues.is_empty()); + assert!(!shaped.should_block); + assert_eq!(shaped.exit_code, EXIT_OK); + } + + #[test] + fn filter_keeps_blocking_findings() { + let mut issues = vec![issue(Severity::Critical), issue(Severity::Info)]; + let shaped = shape_issues(&mut issues, &config("block", false), &opts(Severity::Major)); + assert_eq!(issues.len(), 1); + assert!(shaped.should_block); + assert_eq!(shaped.exit_code, EXIT_BLOCKED); + } + + #[test] + fn no_filter_keeps_every_issue_but_still_decides_blocking() { + // `cora scan`: every finding is listed; only min_severity blocks. + let mut issues = vec![issue(Severity::Info)]; + let o = ShapeOptions { + filter: false, + ..opts(Severity::Major) + }; + let shaped = shape_issues(&mut issues, &config("block", false), &o); + assert_eq!(issues.len(), 1); + assert!(!shaped.should_block); + } + + #[test] + fn shape_stores_should_block_on_the_response() { + let mut resp = ReviewResponse { + issues: vec![issue(Severity::Minor)], + summary: String::new(), + tokens_used: None, + should_block: true, // stale value from the judge + dropped_findings: 0, + }; + shape(&mut resp, &config("warn", false), &opts(Severity::Major)); + assert!(resp.issues.is_empty()); + assert!(!resp.should_block); + } + + #[test] + fn gate_is_skipped_when_disabled_by_config_or_option() { + let mut issues = vec![issue(Severity::Critical)]; + let shaped = shape_issues(&mut issues, &config("warn", false), &opts(Severity::Info)); + assert!(shaped.gate.is_none()); + assert_eq!(shaped.gate_label(), "disabled"); + + let o = ShapeOptions { + gate: false, + ..opts(Severity::Info) + }; + let shaped = shape_issues(&mut issues, &config("warn", true), &o); + assert!(shaped.gate.is_none()); + } + + #[test] + fn gate_runs_on_the_filtered_list() { + // A critical finding fails the default gate (max_critical = 0)... + let mut issues = vec![issue(Severity::Critical)]; + let shaped = shape_issues(&mut issues, &config("warn", true), &opts(Severity::Info)); + assert!(shaped.gate_failed()); + assert_eq!(shaped.gate_label(), "failed"); + assert_eq!(shaped.exit_code, EXIT_BLOCKED); + + // ...and a clean list passes it. + let mut none: Vec = vec![]; + let shaped = shape_issues(&mut none, &config("warn", true), &opts(Severity::Info)); + assert!(!shaped.gate_failed()); + assert_eq!(shaped.gate_label(), "passed"); + assert_eq!(shaped.exit_code, EXIT_OK); + } + + // ─── exit code ─── + + #[test] + fn exit_code_zero_when_hook_mode_not_block() { + assert_eq!(exit_code(None, false, true, true, "warn"), EXIT_OK); + assert_eq!(exit_code(None, false, true, true, ""), EXIT_OK); + } + + #[test] + fn exit_code_blocked_when_should_block_and_hook_blocks() { + assert_eq!(exit_code(None, false, true, true, "block"), EXIT_BLOCKED); + } + + #[test] + fn exit_code_ci_blocks_on_any_finding() { + assert_eq!(exit_code(None, true, false, false, "block"), EXIT_OK); + assert_eq!(exit_code(None, true, true, false, "block"), EXIT_BLOCKED); + } + + #[test] + fn exit_code_gate_fail_overrides_everything() { + assert_eq!( + exit_code(Some(GateStatus::Fail), false, false, false, "block"), + EXIT_BLOCKED + ); + assert_eq!( + exit_code(Some(GateStatus::Pass), false, false, false, "block"), + EXIT_OK + ); + } +} diff --git a/src/engine/mod.rs b/src/engine/mod.rs index 599348f..123574f 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -7,6 +7,7 @@ pub mod debt_tracker; pub mod deterministic; pub mod diff_parser; pub mod enclosing; +pub mod finalize; pub mod index_bridge; pub mod index_scanner; pub mod inline_suppress; diff --git a/src/engine/review.rs b/src/engine/review.rs index 6b76a88..eae1248 100644 --- a/src/engine/review.rs +++ b/src/engine/review.rs @@ -314,11 +314,10 @@ async fn review_diff_inner( let pp = postprocess_report(fallback.issues, &Source::Diff(&diff_chunks), config); fallback.issues = pp.issues; fallback.dropped_findings = pp.dropped; - let min_sev = config.hook.min_severity_level(); - fallback.should_block = fallback - .issues - .iter() - .any(|issue| issue.severity <= min_sev); + fallback.should_block = crate::engine::finalize::blocks( + &fallback.issues, + config.hook.min_severity_level(), + ); return Ok(fallback); } return Err(e); @@ -352,13 +351,8 @@ async fn review_diff_inner( response.dropped_findings = pp.dropped; // Calculate should_block based on min_severity - let min_severity = config.hook.min_severity_level(); - // Ord order is Critical(0) < Major(1) < Minor(2) < Info(3), so "at or above - // min_severity" means Ord value <= min_severity. - response.should_block = response - .issues - .iter() - .any(|issue| issue.severity <= min_severity); + response.should_block = + crate::engine::finalize::blocks(&response.issues, config.hook.min_severity_level()); debug!( issues = response.issues.len(), diff --git a/src/mcp/tools.rs b/src/mcp/tools.rs index 3e9b359..9a1ef3c 100644 --- a/src/mcp/tools.rs +++ b/src/mcp/tools.rs @@ -767,11 +767,32 @@ fn handle_review_diff(params: &serde_json::Value) -> ToolResult { )); match result { - Ok(response) => { + Ok(mut response) => { + // Honor `min_severity` (default: info = everything), then gate on + // what is reported. `should_block` still follows the project's + // `hook.min_severity`. + let min_severity = params + .get("min_severity") + .and_then(|v| v.as_str()) + .map_or(crate::engine::Severity::Info, |s| { + crate::engine::Severity::from_str_lossy(s) + }); + let shaped = crate::engine::finalize::shape( + &mut response, + &config, + &crate::engine::finalize::ShapeOptions { + min_severity, + filter: true, + gate: true, + ci: false, + }, + ); + let should_block = + crate::engine::finalize::blocks(&response.issues, config.hook.min_severity_level()); let json = serde_json::json!({ "summary": response.summary, "total_issues": response.issues.len(), - "should_block": response.should_block, + "should_block": should_block, "issues": response.issues.iter().map(|i| serde_json::json!({ "title": i.title, "severity": i.severity.label(), @@ -780,14 +801,12 @@ fn handle_review_diff(params: &serde_json::Value) -> ToolResult { "type": i.issue_type, "body": i.body, })).collect::>(), - "gate": if config.quality_gate.enabled { - let gate = crate::engine::quality_gate::evaluate(&response.issues, &config.quality_gate); - serde_json::json!({ + "gate": match &shaped.gate { + Some(gate) => serde_json::json!({ "status": format!("{:?}", gate.status), "total_findings": gate.total_findings, - }) - } else { - serde_json::json!({"enabled": false}) + }), + None => serde_json::json!({"enabled": false}), }, }); ToolResult::text(serde_json::to_string_pretty(&json).unwrap_or_default())