refactor(review): prepare / judge / finalize with the LLM as the seam (#649)

CHANGELOG.md

@@ -13,6 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- **Review is now prepare / judge / finalize with the LLM as the seam (#649).** `review::prepare` turns a diff into a `judge::Packet` (judge-visible text, assembled context, system prompt override, `valid_files`, diff chunks, deterministic findings); a `judge::Judge` returns findings (`HttpJudge` = today's LLM call, `NoJudge` = deterministic only); `review::review_with` merges, guards file paths, post-processes and decides `should_block`. Output of `cora review` is unchanged. The review pipeline now has an end-to-end test with a fake judge (no network), and the `llm` module no longer prints: spinners moved to `progress::Spinner` and `llm::review_diff` / `llm::scan_files` take an events observer like the stream variant.
+
 - **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

src/commands/scan.rs

@@ -179,17 +179,20 @@ pub async fn execute_scan(
 
         eprintln!("  Reviewing{batch_label}…");
 
-        match crate::engine::llm::scan_files(
+        let spinner = crate::progress::Spinner::new("Scanning files…", false);
+        let scanned = crate::engine::llm::scan_files(
             llm_config,
             &files_content,
             &effective_focus,
             &config.rules,
             &config.response_format,
             None,
             brain_ctx.as_deref(),
+            &spinner,
         )
-        .await
-        {
+        .await;
+        spinner.finish();
+        match scanned {
             Ok((issues, _summary, tokens)) => {
                 all_issues.extend(issues);
                 total_tokens = match (total_tokens, tokens) {

src/engine/judge.rs

@@ -0,0 +1,107 @@
+//! The seam between *preparing* a review and *judging* it (#649).
+//!
+//! A review is three steps:
+//!
+//! 1. [`review::prepare`](crate::engine::review::prepare) turns a diff into a
+//!    [`Packet`]: everything a judge needs (the text it should see, the
+//!    assembled context, the system prompt override) plus everything the
+//!    finishing step needs (`valid_files`, the diff chunks, the deterministic
+//!    findings).
+//! 2. A [`Judge`] reads the packet and returns findings. This is the seam.
+//! 3. [`review::review_with`](crate::engine::review::review_with) merges the
+//!    deterministic findings, guards the judge's file paths, post-processes,
+//!    and decides `should_block`.
+//!
+//! Adapters at the seam: [`HttpJudge`] (an OpenAI-compatible LLM, today's
+//! behavior), [`NoJudge`] (deterministic checks only, no LLM), and, later, an
+//! agent that judges for itself. A fake `Judge` makes the whole pipeline
+//! testable without a network.
+
+use crate::config::schema::Config;
+use crate::engine::deterministic::DeterministicReport;
+use crate::engine::diff_parser::FileChunk;
+use crate::engine::llm;
+use crate::engine::types::{LLMConfig, ReviewResponse};
+use crate::error::CoraError;
+
+/// Everything about one review that does not depend on who judges it.
+#[derive(Debug)]
+pub struct Packet {
+    /// The diff text a judge sees (comment-sanitized when configured).
+    pub diff_text: String,
+    /// Assembled prompt context (static analysis, deterministic findings,
+    /// cross-file, language, profile, memory, brain), if any.
+    pub context: Option<String>,
+    /// Resolved system prompt override, if any.
+    pub system_prompt: Option<String>,
+    /// File paths present in the diff; a judge may only report on these.
+    pub valid_files: Vec<String>,
+    /// Parsed diff of the ORIGINAL (unsanitized) text.
+    pub chunks: Vec<FileChunk>,
+    /// Findings of every deterministic scanner family.
+    pub deterministic: DeterministicReport,
+}
+
+/// Something that turns a [`Packet`] into findings.
+pub trait Judge {
+    async fn judge(&self, config: &Config, packet: &Packet) -> Result<ReviewResponse, CoraError>;
+}
+
+/// Judge backed by an OpenAI-compatible LLM.
+pub struct HttpJudge<'a> {
+    pub llm_config: &'a LLMConfig,
+    /// Stream tokens to stdout as they arrive.
+    pub stream: bool,
+    /// No spinner (ignored when streaming).
+    pub quiet: bool,
+}
+
+impl Judge for HttpJudge<'_> {
+    async fn judge(&self, config: &Config, packet: &Packet) -> Result<ReviewResponse, CoraError> {
+        if self.stream {
+            return llm::review_diff_stream(
+                self.llm_config,
+                &packet.diff_text,
+                &config.focus,
+                &config.rules,
+                &config.response_format,
+                packet.system_prompt.as_deref(),
+                packet.context.as_deref(),
+                &crate::progress::StdoutStream,
+            )
+            .await
+            .inspect(|_| println!()); // trailing newline after streamed output
+        }
+        let spinner = crate::progress::Spinner::new("Reviewing diff…", self.quiet);
+        let result = llm::review_diff(
+            self.llm_config,
+            &packet.diff_text,
+            &config.focus,
+            &config.rules,
+            &config.response_format,
+            packet.system_prompt.as_deref(),
+            packet.context.as_deref(),
+            &spinner,
+        )
+        .await;
+        spinner.finish();
+        result
+    }
+}
+
+/// Judge that finds nothing: the review is the deterministic findings only.
+// Selected by `cora review --no-llm` / a missing API key (#646).
+#[allow(dead_code)]
+pub struct NoJudge;
+
+impl Judge for NoJudge {
+    async fn judge(&self, _config: &Config, _packet: &Packet) -> Result<ReviewResponse, CoraError> {
+        Ok(ReviewResponse {
+            issues: Vec::new(),
+            summary: "Deterministic checks only (no LLM judge).".to_string(),
+            tokens_used: None,
+            should_block: false,
+            dropped_findings: 0,
+        })
+    }
+}

src/engine/llm/mod.rs

@@ -20,8 +20,6 @@ mod prompts;
 mod repair;
 mod transport;
 
-use indicatif::{ProgressBar, ProgressDrawTarget, ProgressStyle};
-
 use crate::engine::types::{LLMConfig, ReviewIssue, ReviewResponse, TokenUsage};
 use crate::error::CoraError;
 
@@ -56,42 +54,6 @@ pub trait LlmEvents: Sync {
 pub struct NoEvents;
 impl LlmEvents for NoEvents {}
 
-/// Spinner-backed events for interactive non-streaming runs.
-struct SpinnerEvents(ProgressBar);
-
-impl LlmEvents for SpinnerEvents {
-    fn status(&self, msg: &str) {
-        self.0.set_message(msg.to_string());
-    }
-}
-
-/// Create an animated spinner for LLM operations.
-///
-/// Automatically hidden when stderr is not a TTY (piped/redirected),
-/// preventing ANSI pollution in captured output.
-fn create_spinner(message: &str) -> ProgressBar {
-    let spinner = ProgressBar::new_spinner();
-    // Hide spinner when stderr is not a terminal (piped/redirected)
-    if !atty_check() {
-        spinner.set_draw_target(ProgressDrawTarget::hidden());
-        return spinner;
-    }
-    spinner.enable_steady_tick(std::time::Duration::from_millis(80));
-    spinner.set_style(
-        ProgressStyle::with_template("{spinner:.cyan} {msg}")
-            .expect("valid spinner template")
-            .tick_chars("⠁⠂⠄⡀⢀⠠⠐⠈ "),
-    );
-    spinner.set_message(message.to_string());
-    spinner
-}
-
-/// Check if stderr is connected to a TTY.
-fn atty_check() -> bool {
-    use std::io::IsTerminal;
-    std::io::stderr().is_terminal()
-}
-
 /// Raw chat completion — returns the raw string response.
 /// Used by commit message generation and other non-review tasks.
 ///
@@ -182,6 +144,8 @@ async fn review_with<T: Transport>(
 }
 
 /// Review a diff using the LLM. Returns a `ReviewResponse`.
+///
+/// Status text goes to `events`; the caller decides whether that is a spinner.
 #[allow(clippy::too_many_arguments)]
 pub async fn review_diff(
     llm_config: &LLMConfig,
@@ -190,33 +154,26 @@ pub async fn review_diff(
     rules: &[String],
     response_format: &str,
     system_prompt_override: Option<&str>,
-    quiet: bool,
     static_context: Option<&str>,
+    events: &dyn LlmEvents,
 ) -> std::result::Result<ReviewResponse, CoraError> {
-    let events = SpinnerEvents(if quiet {
-        ProgressBar::hidden()
-    } else {
-        create_spinner("Reviewing diff…")
-    });
     let transport = HttpTransport {
         config: llm_config,
         response_format,
         stream: false,
-        events: &events,
+        events,
     };
-    let result = review_with(
+    review_with(
         &transport,
-        &events,
+        events,
         llm_config.max_tokens,
         diff,
         focus,
         rules,
         system_prompt_override,
         static_context,
     )
-    .await;
-    events.0.finish_and_clear();
-    result
+    .await
 }
 
 /// Review a diff using the LLM with streaming. Returns a `ReviewResponse`.
@@ -297,27 +254,25 @@ pub async fn scan_files(
     response_format: &str,
     system_prompt_override: Option<&str>,
     brain_context: Option<&str>,
+    events: &dyn LlmEvents,
 ) -> std::result::Result<(Vec<ReviewIssue>, Option<String>, Option<TokenUsage>), CoraError> {
-    let events = SpinnerEvents(create_spinner("Scanning files…"));
     let transport = HttpTransport {
         config: llm_config,
         response_format,
         stream: false,
-        events: &events,
+        events,
     };
-    let result = scan_with(
+    scan_with(
         &transport,
-        &events,
+        events,
         llm_config.max_tokens,
         files_content,
         focus,
         rules,
         system_prompt_override,
         brain_context,
     )
-    .await;
-    events.0.finish_and_clear();
-    result
+    .await
 }
 
 #[cfg(test)]

src/engine/mod.rs

@@ -11,6 +11,7 @@ pub mod finalize;
 pub mod index_bridge;
 pub mod index_scanner;
 pub mod inline_suppress;
+pub mod judge;
 pub mod language_analyzer;
 pub mod llm;
 pub mod markdown;