From 8b44e4fe3b85d83cbdce907ea80eb7bb9f74e038 Mon Sep 17 00:00:00 2001 From: ajianaz Date: Thu, 15 Oct 2026 11:49:28 +0700 Subject: [PATCH] feat(cli): run review/scan without an API key (select_judge, --no-llm) (#646) A missing API key now selects the deterministic judge instead of failing: checks run, a stderr notice is printed, and the summary (and SARIF invocation) says no LLM judged. --no-llm forces it. The judge is chosen once at the entry point. Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: ajianaz --- CHANGELOG.md | 4 ++ docs/cli-reference.md | 4 ++ src/commands/review.rs | 23 ++++++---- src/commands/scan.rs | 73 +++++++++++++++++++++++++------- src/engine/judge.rs | 93 +++++++++++++++++++++++++++++++++++++++-- src/engine/review.rs | 58 +++++++++++++++++++++---- src/formatters/sarif.rs | 55 ++++++++++++++++++++++-- src/main.rs | 50 ++++++++++++++++------ 8 files changed, 308 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4161d52..ab72801 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- **`cora review` and `cora scan` run without an API key (#646).** With no key they run the deterministic checks only, print a one-line notice on stderr, and label the result "Deterministic checks only (no LLM judge)" (SARIF: `cora.judge = none` on the invocation, even with zero results). `--no-llm` forces it and silences the notice. A missing key is no longer an error for these two commands; any other config error still is. The judge is chosen once at the entry point (`judge::select_judge`, `JudgeChoice`). `cora commit` and MCP `cora.review_diff` still need a key. + ### Security - **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. diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 322cc9a..c70fbcf 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -74,6 +74,7 @@ Complete command reference for the cora CLI. | `cora review --max-diff-size` `` | Override max diff size | | `cora review --memory` | Recall project patterns from Uteke before review | | `cora review --learn` | Save findings to Uteke after review (implies `--memory`) | +| `cora review --no-llm` | Deterministic checks only, never call an LLM | | `cora commit` | Review staged + generate commit message + commit (HITL prompt) | | `cora commit --yolo` | Auto-commit without prompts | | `cora commit --force` | Commit even if quality gate fails | @@ -89,6 +90,9 @@ Complete command reference for the cora CLI. | `cora scan --focus` `security` | Override focus areas | | `cora scan --batch-files` `N` | Max files per LLM batch (default: 20) | | `cora scan --no-continue-on-batch-error` | Abort on batch failure (default: skip and continue) | +| `cora scan --no-llm` | Deterministic checks only, never call an LLM | + +**No API key needed for deterministic checks.** `cora review` and `cora scan` run without any LLM configured: with no key they run the deterministic checks only (rules, secrets, security, index scans), print a one-line notice on stderr, and mark the result "Deterministic checks only (no LLM judge)" (in SARIF: `invocations[0].properties["cora.judge"] = "none"`), so a clean result is never mistaken for an AI review. `--no-llm` forces this even when a key exists and silences the notice. `cora commit` still needs a key for the commit message. `cora scan` runs the same deterministic checks as `cora review` (built-in and custom rules, secrets, security patterns, plus unused imports and dead code from the index) on every file, then asks the LLM about each batch. Findings are filtered by `ignore.rules` and inline `cora-ignore:` markers and capped by `rules_engine.max_findings`. Unlike `review`, scan has no `--ci`, `--severity`, `--quiet`, `--stream`, `--output-file` or `--no-cache`, does not apply the quality gate, and keeps every finding in the output (`hook.min_severity` only decides the exit code in `block` mode). diff --git a/src/commands/review.rs b/src/commands/review.rs index 0b1ea01..08ad754 100644 --- a/src/commands/review.rs +++ b/src/commands/review.rs @@ -6,6 +6,7 @@ use crate::config::schema::Config; use crate::engine::Severity; use crate::engine::chunker; use crate::engine::finalize::{self, HistoryEntry, ShapeOptions}; +use crate::engine::judge::JudgeChoice; use crate::engine::quality_gate; use crate::engine::types::ReviewResponse; use crate::formatters::{OutputFormat, formatter_for}; @@ -121,7 +122,7 @@ impl ReviewResult { #[allow(clippy::cast_possible_truncation, clippy::too_many_arguments)] pub async fn execute_review( config: &Config, - llm_config: &crate::engine::LLMConfig, + judge: &JudgeChoice, opts: &ReviewOptions, format: OutputFormat, progress: &ProgressReporter, @@ -151,7 +152,7 @@ pub async fn execute_review( // Auto-chunk mode: split diff and review each chunk return execute_chunked_review( config, - llm_config, + judge, opts, format, progress, @@ -191,15 +192,17 @@ pub async fn execute_review( // 4. Emit calling_llm event if progress.is_enabled() { - progress.calling_llm(&llm_config.provider, &llm_config.model); + if let JudgeChoice::Llm(llm_config) = judge { + progress.calling_llm(&llm_config.provider, &llm_config.model); + } } let llm_start = std::time::Instant::now(); // 5. Call the LLM engine - let response = match crate::engine::review::review_diff_with_cache( + let response = match crate::engine::review::review_diff_with_judge( config, - llm_config, + judge, &diff, opts.stream, !opts.no_cache, @@ -387,7 +390,7 @@ fn get_diff(opts: &ReviewOptions, _config: &Config) -> Result { #[allow(clippy::cast_possible_truncation, clippy::too_many_arguments)] async fn execute_chunked_review( config: &Config, - llm_config: &crate::engine::LLMConfig, + judge: &JudgeChoice, opts: &ReviewOptions, format: OutputFormat, progress: &ProgressReporter, @@ -445,14 +448,16 @@ async fn execute_chunked_review( } if progress.is_enabled() { - progress.calling_llm(&llm_config.provider, &llm_config.model); + if let JudgeChoice::Llm(llm_config) = judge { + progress.calling_llm(&llm_config.provider, &llm_config.model); + } } let chunk_start = std::time::Instant::now(); - match crate::engine::review::review_diff_with_cache( + match crate::engine::review::review_diff_with_judge( config, - llm_config, + judge, &chunk.diff, opts.stream, !opts.no_cache, diff --git a/src/commands/scan.rs b/src/commands/scan.rs index 5239b1a..e382a09 100644 --- a/src/commands/scan.rs +++ b/src/commands/scan.rs @@ -7,7 +7,9 @@ use tracing::debug; use crate::config::schema::Config; use crate::engine::ReviewIssue; use crate::engine::finalize::{self, HistoryEntry, ShapeOptions}; -use crate::engine::judge::{HttpScanJudge, ScanBatch, ScanJudge, admit}; +use crate::engine::judge::{ + DETERMINISTIC_ONLY, HttpScanJudge, JudgeChoice, ScanBatch, ScanJudge, admit, +}; use crate::engine::postprocess::{Source, dropped_warning, postprocess_report}; use crate::engine::scanner::{FileEntry, batch_files, format_batch_for_prompt, walk_project}; use crate::engine::types::TokenUsage; @@ -51,7 +53,7 @@ pub struct ScanOptions { #[allow(clippy::too_many_lines)] pub async fn execute_scan( config: &Config, - llm_config: &crate::engine::LLMConfig, + judge: &JudgeChoice, opts: &ScanOptions, format: OutputFormat, ) -> Result { @@ -166,20 +168,27 @@ pub async fn execute_scan( None }; - // 5. Judge the batches and collect issues - let judge = HttpScanJudge { llm_config }; + // 5. Judge the batches and collect issues (no LLM: deterministic only) let BatchRun { issues: mut all_issues, tokens: total_tokens, - } = judge_batches( - &judge, - config, - &batches, - &effective_focus, - brain_ctx.as_deref(), - opts.continue_on_batch_error, - ) - .await?; + } = match judge { + JudgeChoice::Llm(llm_config) => { + judge_batches( + &HttpScanJudge { llm_config }, + config, + &batches, + &effective_focus, + brain_ctx.as_deref(), + opts.continue_on_batch_error, + ) + .await? + } + JudgeChoice::Deterministic => BatchRun { + issues: Vec::new(), + tokens: None, + }, + }; // 5. Merge deterministic + index findings with the LLM findings, then // apply ignore.rules and inline cora-ignore markers. Deterministic @@ -203,6 +212,13 @@ pub async fn execute_scan( ); let should_block = shaped.should_block; + // A clean deterministic result is not an AI scan: say so in the summary. + let no_judge_note = if matches!(judge, JudgeChoice::Deterministic) { + format!(" {DETERMINISTIC_ONLY}") + } else { + String::new() + }; + let response = crate::engine::ScanResponse { issues: all_issues, files_scanned: files.len(), @@ -219,7 +235,7 @@ pub async fn execute_scan( } else { String::new() } - ), + ) + &no_judge_note, tokens_used: total_tokens, should_block, }; @@ -522,7 +538,7 @@ mod tests { batch_files: 0, continue_on_batch_error: true, }; - let code = execute_scan(&config, &llm, &opts, OutputFormat::Json) + let code = execute_scan(&config, &JudgeChoice::Llm(llm), &opts, OutputFormat::Json) .await .unwrap(); assert_eq!(code, 2, "deterministic finding must survive LLM failure"); @@ -625,4 +641,31 @@ mod tests { .unwrap(); assert!(run.issues.is_empty()); } + + #[tokio::test] + async fn deterministic_scan_needs_no_llm_and_still_blocks() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("app.py"), "password = \"hunter2hunter2\"\n").unwrap(); + let mut config = Config::default(); + config.hook.mode = "block".to_string(); + let opts = ScanOptions { + path: Some(dir.path().to_string_lossy().to_string()), + include: vec![], + exclude: vec![], + extensions: vec![], + incremental: false, + focus: vec![], + batch_files: 0, + continue_on_batch_error: true, + }; + let code = execute_scan( + &config, + &JudgeChoice::Deterministic, + &opts, + OutputFormat::Json, + ) + .await + .unwrap(); + assert_eq!(code, 2); + } } diff --git a/src/engine/judge.rs b/src/engine/judge.rs index c55a8cd..0c5e135 100644 --- a/src/engine/judge.rs +++ b/src/engine/judge.rs @@ -135,6 +135,59 @@ impl Judge for HttpJudge<'_> { } } +/// Who judges a `cora review` / `cora scan` run, chosen once at the entry point. +#[derive(Debug, Clone)] +pub enum JudgeChoice { + /// An OpenAI-compatible LLM. + Llm(LLMConfig), + /// No LLM: deterministic checks only. + Deterministic, +} + +/// One-line stderr notice when a missing API key turned a run deterministic. +pub const NO_LLM_NOTICE: &str = "ℹ No LLM configured: running deterministic checks only. \ +Run `cora auth login` (or set an API key) to enable AI review; `--no-llm` silences this notice."; + +/// Pick the judge from `--no-llm` and the result of building the LLM config. +/// +/// A missing API key (`NoApiKey`) is not an error: the run is deterministic. +/// Any other config error is real and is returned. The bool says whether the +/// choice was a fallback (key missing, `--no-llm` not given), so the caller can +/// print [`NO_LLM_NOTICE`]. +pub fn choose( + no_llm: bool, + built: Result, +) -> Result<(JudgeChoice, bool), CoraError> { + if no_llm { + return Ok((JudgeChoice::Deterministic, false)); + } + match built { + Ok(llm) => Ok((JudgeChoice::Llm(llm), false)), + Err(CoraError::NoApiKey) => Ok((JudgeChoice::Deterministic, true)), + Err(e) => Err(e), + } +} + +/// Choose the judge for a command from the loaded config. Prints the notice on +/// fallback unless `quiet`. +pub fn select_judge( + config: &Config, + cli_api_key: Option<&str>, + no_llm: bool, + quiet: bool, +) -> Result { + let built = if no_llm { + Err(CoraError::NoApiKey) // not consulted + } else { + crate::config::loader::build_llm_config(config, cli_api_key) + }; + let (judge, fell_back) = choose(no_llm, built)?; + if fell_back && !quiet { + eprintln!("{NO_LLM_NOTICE}"); + } + Ok(judge) +} + /// What a judge returned for one scan batch. #[derive(Debug, Default)] pub struct ScanBatch { @@ -184,16 +237,18 @@ impl ScanJudge for HttpScanJudge<'_> { } } +/// Summary text of a run that had no LLM judge. Formatters key off it to say, +/// in every output format, that a clean result is not an AI review. +pub const DETERMINISTIC_ONLY: &str = "Deterministic checks only (no LLM judge)."; + /// 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 { Ok(ReviewResponse { issues: Vec::new(), - summary: "Deterministic checks only (no LLM judge).".to_string(), + summary: DETERMINISTIC_ONLY.to_string(), tokens_used: None, should_block: false, dropped_findings: 0, @@ -457,4 +512,36 @@ mod tests { assert!(out[0].rule_id.is_none()); assert!(out[0].body.contains("ignore previous instructions")); } + + // ─── #646: choosing the judge ─── + + fn llm() -> LLMConfig { + LLMConfig::default() + } + + #[test] + fn key_present_means_llm() { + let (j, fell_back) = choose(false, Ok(llm())).unwrap(); + assert!(matches!(j, JudgeChoice::Llm(_))); + assert!(!fell_back); + } + + #[test] + fn missing_key_falls_back_to_deterministic_with_notice() { + let (j, fell_back) = choose(false, Err(CoraError::NoApiKey)).unwrap(); + assert!(matches!(j, JudgeChoice::Deterministic)); + assert!(fell_back, "caller must print the notice"); + } + + #[test] + fn no_llm_flag_wins_even_with_a_key_and_is_silent() { + let (j, fell_back) = choose(true, Ok(llm())).unwrap(); + assert!(matches!(j, JudgeChoice::Deterministic)); + assert!(!fell_back); + } + + #[test] + fn other_config_errors_are_not_swallowed() { + assert!(choose(false, Err(CoraError::LlmParse("bad".into()))).is_err()); + } } diff --git a/src/engine/review.rs b/src/engine/review.rs index 5c16583..f578e96 100644 --- a/src/engine/review.rs +++ b/src/engine/review.rs @@ -3,7 +3,7 @@ use tracing::{debug, instrument}; use crate::config::schema::Config; use crate::engine::index_bridge::IndexBridge; -use crate::engine::judge::{HttpJudge, Judge, Packet, admit}; +use crate::engine::judge::{HttpJudge, Judge, JudgeChoice, NoJudge, Packet, admit}; use crate::engine::llm; use crate::engine::postprocess::{Source, postprocess_report}; use crate::engine::types::{LLMConfig, ReviewResponse}; @@ -77,6 +77,54 @@ pub async fn review_diff_with_cache( .await } +/// [`review_diff_with_cache`] for whichever judge the run chose: the LLM (with +/// cache and streaming), or no judge at all, which returns the deterministic +/// findings only. +#[instrument(skip_all)] +#[allow(clippy::too_many_arguments)] +pub async fn review_diff_with_judge( + config: &Config, + judge: &JudgeChoice, + diff: &str, + stream: bool, + use_cache: bool, + quiet: bool, + memory_context: Option<&str>, +) -> std::result::Result { + match judge { + JudgeChoice::Llm(llm_config) => { + review_diff_inner( + config, + llm_config, + diff, + stream, + use_cache, + quiet, + memory_context, + ) + .await + } + JudgeChoice::Deterministic => { + if diff.trim().is_empty() { + return Ok(empty_response()); + } + let index_bridge = crate::engine::index_bridge::IndexBridge::open_cwd(); + let packet = prepare(config, diff, memory_context, &index_bridge); + review_with(&NoJudge, config, packet).await + } + } +} + +fn empty_response() -> ReviewResponse { + ReviewResponse { + issues: vec![], + summary: "No changes to review.".to_string(), + tokens_used: None, + should_block: false, + dropped_findings: 0, + } +} + #[allow(clippy::too_many_arguments)] async fn review_diff_inner( config: &Config, @@ -94,13 +142,7 @@ async fn review_diff_inner( ); if diff.trim().is_empty() { - return Ok(ReviewResponse { - issues: vec![], - summary: "No changes to review.".to_string(), - tokens_used: None, - should_block: false, - dropped_findings: 0, - }); + return Ok(empty_response()); } // Index bridge: one project handle, rooted via resolve_project_root, shared diff --git a/src/formatters/sarif.rs b/src/formatters/sarif.rs index 898d902..55595ef 100644 --- a/src/formatters/sarif.rs +++ b/src/formatters/sarif.rs @@ -12,20 +12,29 @@ pub struct SarifFormatter; impl Formatter for SarifFormatter { fn format_review(&self, response: &ReviewResponse) -> Result { - let sarif = build_sarif(&response.issues); + let sarif = build_sarif(&response.issues, is_deterministic_only(&response.summary)); let json = serde_json::to_string_pretty(&sarif)?; Ok(json) } fn format_scan(&self, response: &ScanResponse) -> Result { - let sarif = build_sarif(&response.issues); + let sarif = build_sarif(&response.issues, is_deterministic_only(&response.summary)); let json = serde_json::to_string_pretty(&sarif)?; Ok(json) } } +/// True when the run had no LLM judge (see `judge::DETERMINISTIC_ONLY`). +fn is_deterministic_only(summary: &str) -> bool { + summary.contains(crate::engine::judge::DETERMINISTIC_ONLY) +} + /// Build a SARIF v2.1.0 document from a list of issues. -fn build_sarif(issues: &[ReviewIssue]) -> Value { +/// +/// `deterministic_only` marks a run without an LLM judge: the invocation then +/// says so, even when there are no results, so an empty SARIF file is never +/// read as "reviewed and clean". +fn build_sarif(issues: &[ReviewIssue], deterministic_only: bool) -> Value { // Deduplicate rules by id (multiple issues can share same rule) let mut rule_map = serde_json::Map::new(); for issue in issues { @@ -109,7 +118,16 @@ fn build_sarif(issues: &[ReviewIssue]) -> Value { .collect(); // Build invocations array with watermark - let invocations = if issues.is_empty() { + let invocations = if deterministic_only { + json!([{ + "executionSuccessful": true, + "properties": { + "cora.watermark": format!("Reviewed by CodeCora v{}", env!("CARGO_PKG_VERSION")), + "cora.judge": "none", + "cora.note": crate::engine::judge::DETERMINISTIC_ONLY + } + }]) + } else if issues.is_empty() { json!([]) } else { json!([{ @@ -347,4 +365,33 @@ mod tests { let result = &parsed["runs"][0]["results"][0]; assert!(result.get("fixes").is_some()); } + + #[test] + fn deterministic_only_run_says_so_even_with_no_results() { + let resp = ReviewResponse { + issues: vec![], + summary: crate::engine::judge::DETERMINISTIC_ONLY.to_string(), + tokens_used: None, + should_block: false, + dropped_findings: 0, + }; + let out = SarifFormatter.format_review(&resp).unwrap(); + let v: Value = serde_json::from_str(&out).unwrap(); + let inv = &v["runs"][0]["invocations"][0]["properties"]; + assert_eq!(inv["cora.judge"], "none"); + assert!(inv["cora.note"].as_str().unwrap().contains("Deterministic")); + } + + #[test] + fn llm_run_without_issues_has_no_judge_marker() { + let resp = ReviewResponse { + issues: vec![], + summary: "all good".to_string(), + tokens_used: None, + should_block: false, + dropped_findings: 0, + }; + let out = SarifFormatter.format_review(&resp).unwrap(); + assert!(!out.contains("cora.judge")); + } } diff --git a/src/main.rs b/src/main.rs index fa8b198..e0f43ee 100644 --- a/src/main.rs +++ b/src/main.rs @@ -22,6 +22,7 @@ use commands::{ scan, upload, }; use config::loader; +use engine::judge; use engine::memory; use formatters::OutputFormat; @@ -330,6 +331,11 @@ enum Command { /// Save review findings to Uteke after review (implies --memory) #[clap(long)] learn: bool, + + /// Deterministic checks only: never call an LLM (also the automatic + /// behavior when no API key is configured) + #[clap(long)] + no_llm: bool, }, /// Scan a project directory for issues @@ -367,6 +373,11 @@ enum Command { /// skipping it and continuing (default: skip and continue). #[clap(long)] no_continue_on_batch_error: bool, + + /// Deterministic checks only: never call an LLM (also the automatic + /// behavior when no API key is configured) + #[clap(long)] + no_llm: bool, }, /// Manage quality profiles (preset rule sets) @@ -1147,6 +1158,7 @@ async fn main() -> Result<()> { token, memory, learn, + no_llm, } => { cmd_review( &cli.global, @@ -1172,6 +1184,7 @@ async fn main() -> Result<()> { ci, memory, learn, + no_llm, }, ) .await? @@ -1185,6 +1198,7 @@ async fn main() -> Result<()> { focus, batch_files, no_continue_on_batch_error, + no_llm, } => { cmd_scan( &cli.global, @@ -1197,6 +1211,7 @@ async fn main() -> Result<()> { focus, batch_files, continue_on_batch_error: !no_continue_on_batch_error, + no_llm, }, ) .await? @@ -1443,6 +1458,7 @@ struct ReviewOpts { ci: bool, memory: bool, learn: bool, + no_llm: bool, } /// Struct to hold scan options from CLI. @@ -1455,6 +1471,7 @@ struct ScanOpts { focus: Vec, batch_files: usize, continue_on_batch_error: bool, + no_llm: bool, } /// Handle the `review` subcommand. @@ -1467,7 +1484,12 @@ async fn cmd_review(globals: &GlobalOptions, opts: ReviewOpts) -> Result { globals.format.as_deref(), globals.no_color, )?; - let llm_config = loader::build_llm_config(&config, globals.api_key.as_deref())?; + let judge = judge::select_judge( + &config, + globals.api_key.as_deref(), + opts.no_llm, + opts.quiet || opts.progress, + )?; // If --upload is set, force SARIF format let effective_format = if opts.upload { @@ -1502,15 +1524,17 @@ async fn cmd_review(globals: &GlobalOptions, opts: ReviewOpts) -> Result { }; // When streaming and not quiet/progress, show a simpler message - if opts.stream && !opts.quiet && !opts.progress { - eprintln!( - "{}", - format!( - "⏳ Streaming review from {} ({})…", - llm_config.provider, llm_config.model - ) - .dimmed() - ); + if let judge::JudgeChoice::Llm(llm_config) = &judge { + if opts.stream && !opts.quiet && !opts.progress { + eprintln!( + "{}", + format!( + "⏳ Streaming review from {} ({})…", + llm_config.provider, llm_config.model + ) + .dimmed() + ); + } } // Uteke memory integration: recall context before review @@ -1546,7 +1570,7 @@ async fn cmd_review(globals: &GlobalOptions, opts: ReviewOpts) -> Result { // Execute the review (returns formatted output) let result = review::execute_review( &config, - &llm_config, + &judge, &review_opts, effective_format, &progress_reporter, @@ -1672,7 +1696,7 @@ async fn cmd_scan(globals: &GlobalOptions, opts: ScanOpts) -> Result { globals.format.as_deref(), globals.no_color, )?; - let llm_config = loader::build_llm_config(&config, globals.api_key.as_deref())?; + let judge = judge::select_judge(&config, globals.api_key.as_deref(), opts.no_llm, false)?; let format = resolve_format(globals.format.as_deref(), &config)?; @@ -1687,7 +1711,7 @@ async fn cmd_scan(globals: &GlobalOptions, opts: ScanOpts) -> Result { continue_on_batch_error: opts.continue_on_batch_error, }; - scan::execute_scan(&config, &llm_config, &scan_opts, format).await + scan::execute_scan(&config, &judge, &scan_opts, format).await } /// Resolve the output format: CLI flag > config > default.