diff --git a/README.md b/README.md index 8f318f3..e85b105 100755 --- a/README.md +++ b/README.md @@ -13,7 +13,7 @@ The action downloads a pre-compiled **Rust binary** from GitHub Releases and exe | Endpoint | Any OpenAI-compatible, HTTPS-reachable `…/v1` endpoint (self-hosted or hosted) | | Authentication | Optional bearer key (`vllm-api-key`) and/or extra headers (e.g. gateway tokens) | | Repository secrets | `VLLM_URL`, `VLLM_API_KEY` (+ any header secrets your endpoint needs) | -| Repository vars | `VLLM_MODEL`, `VLLM_TIMEOUT`, `VLLM_RETRIES` (optional; consumed via `vars.*`) | +| Repository vars | `VLLM_MODEL`, `VLLM_TIMEOUT`, `VLLM_RETRIES`, `VLLM_VERIFY_MAX_TOKENS` (optional; consumed via `vars.*`) | The endpoint must be reachable from GitHub-hosted runners. If it is private, publish it first — see [**Reaching a private inference endpoint**](docs/recipes/expose-private-endpoint.md) @@ -99,7 +99,8 @@ Any provider exposing OpenAI-style `POST /v1/chat/completions` and `GET /v1/mode 4. **Review strategy** (chosen automatically based on diff size): - **Single-round** (1 file changed) — the full diff is sent in one `chat/completions` call, returning a structured Markdown report - **Two-round** (multiple files) — the diff is split by file into ~2000-word chunks and reviewed **sequentially** (bullet findings); a **verification** pass then checks each finding against the actual source files (fetched from the repo at the PR head SHA via the GitHub Contents API) to confirm, downgrade, or filter it. If verification cannot run, it falls back to a summarization pass. -5. **Post comment** — posts the structured Markdown report as a PR comment (truncated at 60 000 characters to respect GitHub's limit) +5. **Deterministic report cleanup** — the two-round output is post-processed locally (no extra LLM call): only the final report is kept (verification reasoning and YAML front-matter are stripped), exact-duplicate table rows are merged, dangling headings left by a truncated generation are trimmed, and the claimed `findings_total` is reconciled with the actual number of rows. When some modified files could not be fetched, a coverage caveat is prepended to the report. +6. **Post comment** — posts the structured Markdown report as a PR comment (truncated at 60 000 characters to respect GitHub's limit). The comment carries a hidden marker and is **updated in place** on subsequent runs instead of stacking a new comment per run. → [Full documentation](docs/code-review.md) diff --git a/action.yml b/action.yml index 8b9ca6d..72af7e7 100755 --- a/action.yml +++ b/action.yml @@ -23,6 +23,10 @@ inputs: description: "Number of retries on LLM request failure" required: false default: "2" + vllm-verify-max-tokens: + description: "Max tokens for the two-round verification call (final report generation)" + required: false + default: "8192" vllm-api-key: description: "API key for vLLM server authentication (optional, defaults to 'none')" required: false @@ -72,6 +76,7 @@ runs: VLLM_MODEL: ${{ inputs.vllm-model }} VLLM_TIMEOUT_SECS: ${{ inputs.vllm-timeout }} VLLM_RETRIES: ${{ inputs.vllm-retries }} + VLLM_VERIFY_MAX_TOKENS: ${{ inputs.vllm-verify-max-tokens }} VLLM_API_KEY: ${{ inputs.vllm-api-key }} TARGET_TYPE: ${{ inputs.target-type }} TARGET_NUMBER: ${{ inputs.target-number || github.event.pull_request.number || github.event.issue.number }} diff --git a/crates/code-review-cli/src/config.rs b/crates/code-review-cli/src/config.rs index e63d0ce..83770d7 100755 --- a/crates/code-review-cli/src/config.rs +++ b/crates/code-review-cli/src/config.rs @@ -8,6 +8,10 @@ pub struct Config { pub vllm_timeout_secs: u64, pub vllm_retries: u32, pub review_max_context: usize, + /// Max tokens for the two-round verification call (the final report is + /// long; a too-small cap truncates it mid-structure). Env: + /// `VLLM_VERIFY_MAX_TOKENS`, default 8192. + pub verify_max_tokens: u32, /// Extra HTTP headers added to every vLLM request (gateway/auth headers, e.g. /// Cloudflare Access service tokens). Parsed from `EXTRA_HEADERS`. pub extra_headers: Vec<(String, String)>, @@ -46,6 +50,10 @@ impl Config { .ok() .and_then(|v| v.parse().ok()) .unwrap_or(100_000), + verify_max_tokens: env::var("VLLM_VERIFY_MAX_TOKENS") + .ok() + .and_then(|v| v.parse().ok()) + .unwrap_or(8192), extra_headers: parse_extra_headers(&env::var("EXTRA_HEADERS").unwrap_or_default()), } } diff --git a/crates/code-review-cli/src/github.rs b/crates/code-review-cli/src/github.rs index 6c84b4e..7f1d624 100755 --- a/crates/code-review-cli/src/github.rs +++ b/crates/code-review-cli/src/github.rs @@ -120,8 +120,97 @@ pub async fn fetch_pr_diff( Ok(diff) } -/// Post the review text as a PR comment via the GitHub REST API. -pub async fn post_comment( +/// Hidden marker identifying comments owned by this tool, enabling +/// update-in-place instead of posting a new comment on every run. +const COMMENT_MARKER: &str = ""; + +/// Find the id of a previously posted review comment in a list-comments JSON +/// response. A comment qualifies only when its body starts with the hidden +/// marker AND it was authored by `author` — this prevents overwriting a human +/// comment that happens to start with the marker. When `author` is `None` +/// (e.g. the identity lookup failed), the marker alone is trusted. +fn find_existing_comment(comments: &Value, author: Option<&str>) -> Option { + comments.as_array()?.iter().find_map(|c| { + let body = c["body"].as_str()?; + if !body.trim_start().starts_with(COMMENT_MARKER) { + return None; + } + match author { + Some(login) if c["user"]["login"].as_str() == Some(login) => c["id"].as_u64(), + Some(_) => None, + None => c["id"].as_u64(), + } + }) +} + +/// Return the login of the authenticated user (used to identify our own +/// previous comments). +async fn authenticated_login(token: &str, client: &reqwest::Client) -> Result { + let resp = client + .get("https://api.github.com/user") + .header("Authorization", format!("token {token}")) + .header("User-Agent", "code-review-cli") + .send() + .await + .map_err(|e| format!("HTTP request failed: {e}"))?; + if !resp.status().is_success() { + let status = resp.status(); + let body = resp.text().await.unwrap_or_default(); + return Err(format!("GitHub API returned {status}: {body}")); + } + let json: Value = resp + .json() + .await + .map_err(|e| format!("invalid JSON: {e}"))?; + json["login"] + .as_str() + .map(|s| s.to_string()) + .ok_or_else(|| "no login in /user response".to_string()) +} + +/// Fetch all comments of an issue/PR, following pagination. +async fn list_all_comments( + repo: &str, + number: u64, + token: &str, + client: &reqwest::Client, +) -> Result, String> { + let mut all = Vec::new(); + let mut page = 1u32; + loop { + let url = format!( + "https://api.github.com/repos/{repo}/issues/{number}/comments?per_page=100&page={page}" + ); + let resp = client + .get(&url) + .header("Authorization", format!("token {token}")) + .header("User-Agent", "code-review-cli") + .header("Accept", "application/vnd.github+json") + .send() + .await + .map_err(|e| format!("HTTP request failed: {e}"))?; + if !resp.status().is_success() { + let status = resp.status(); + let body = resp.text().await.unwrap_or_default(); + return Err(format!("GitHub API returned {status}: {body}")); + } + let batch: Vec = resp + .json() + .await + .map_err(|e| format!("invalid JSON: {e}"))?; + let done = batch.len() < 100; + all.extend(batch); + if done { + return Ok(all); + } + page += 1; + } +} + +/// Post the review as a PR comment, updating the previous review comment in +/// place when one exists (identified by a hidden marker) instead of stacking +/// a new comment on every run. +pub async fn post_or_update_comment( review: &str, repo: &str, pr_number: u64, @@ -129,7 +218,7 @@ pub async fn post_comment( client: &reqwest::Client, ) -> Result<(), String> { const MAX_LEN: usize = 60_000; - let mut body = review.to_string(); + let mut body = format!("{COMMENT_MARKER}\n{review}"); if body.len() > MAX_LEN { let cutoff = MAX_LEN - 50; let safe_cut = body @@ -142,9 +231,42 @@ pub async fn post_comment( body.push_str("\n\n[Truncated due to GitHub comment size limit]"); } - let url = format!("https://api.github.com/repos/{repo}/issues/{pr_number}/comments"); + let author = match authenticated_login(token, client).await { + Ok(login) => Some(login), + Err(e) => { + eprintln!( + "Warning: could not resolve authenticated user ({e}); matching on marker alone." + ); + None + } + }; + + let existing = match list_all_comments(repo, pr_number, token, client).await { + Ok(comments) => { + find_existing_comment(&serde_json::Value::Array(comments), author.as_deref()) + } + Err(e) => { + eprintln!("Warning: could not list existing comments ({e}); posting a new one."); + None + } + }; + + let (method, url) = match existing { + Some(id) => { + println!("Updating existing review comment #{id}."); + ( + reqwest::Method::PATCH, + format!("https://api.github.com/repos/{repo}/issues/comments/{id}"), + ) + } + None => ( + reqwest::Method::POST, + format!("https://api.github.com/repos/{repo}/issues/{pr_number}/comments"), + ), + }; + let resp = client - .post(&url) + .request(method, &url) .header("Authorization", format!("token {token}")) .header("User-Agent", "code-review-cli") .json(&serde_json::json!({ "body": body })) @@ -299,18 +421,23 @@ fn contents_url(repository: &str, path: &str, git_ref: &str) -> Result Vec<(String, String)> { - let mut result = Vec::new(); +) -> (Vec<(String, String)>, Vec) { + let mut fetched = Vec::new(); + let mut failed = Vec::new(); for path in paths { if !crate::source::is_safe_path(path) { eprintln!("Unsafe path rejected (skipping): {path}"); + failed.push(path.clone()); continue; } @@ -318,6 +445,7 @@ pub async fn fetch_source_files( Ok(u) => u, Err(e) => { eprintln!("Failed to build URL for {path}: {e}"); + failed.push(path.clone()); continue; } }; @@ -335,7 +463,7 @@ pub async fn fetch_source_files( let lines: Vec<&str> = content.lines().collect(); if lines.len() > crate::source::MAX_SOURCE_LINES { let truncated = lines[..crate::source::MAX_SOURCE_LINES].join("\n"); - result.push(( + fetched.push(( path.clone(), format!( "{truncated}\n\n[... truncated at {} lines, {} total ...]", @@ -344,17 +472,26 @@ pub async fn fetch_source_files( ), )); } else { - result.push((path.clone(), content)); + fetched.push((path.clone(), content)); } } - Err(e) => eprintln!("Failed to read {path}: {e}"), + Err(e) => { + eprintln!("Failed to read {path}: {e}"); + failed.push(path.clone()); + } }, - Ok(r) => eprintln!("Source fetch failed for {path}: HTTP {}", r.status()), - Err(e) => eprintln!("Failed to fetch {path}: {e}"), + Ok(r) => { + eprintln!("Source fetch failed for {path}: HTTP {}", r.status()); + failed.push(path.clone()); + } + Err(e) => { + eprintln!("Failed to fetch {path}: {e}"); + failed.push(path.clone()); + } } } - result + (fetched, failed) } #[cfg(test)] @@ -394,6 +531,46 @@ mod tests { assert_eq!(first_pr_number(&v), Some(42)); } + #[test] + fn find_existing_comment_matches_marker_and_author() { + let comments = serde_json::json!([ + { "id": 1, "user": { "login": "someone" }, "body": "a normal comment" }, + { "id": 2, "user": { "login": "github-actions[bot]" }, + "body": "\n## 🔍 AI Code Review\n..." }, + { "id": 3, "user": { "login": "github-actions[bot]" }, "body": "sonar says hi" } + ]); + assert_eq!( + find_existing_comment(&comments, Some("github-actions[bot]")), + Some(2) + ); + } + + #[test] + fn find_existing_comment_rejects_foreign_marker_comment() { + let comments = serde_json::json!([ + { "id": 9, "user": { "login": "mallory" }, + "body": "\nmy precious content" } + ]); + assert_eq!( + find_existing_comment(&comments, Some("github-actions[bot]")), + None + ); + // Without a known author, the marker alone qualifies. + assert_eq!(find_existing_comment(&comments, None), Some(9)); + } + + #[test] + fn find_existing_comment_returns_none_when_absent() { + let comments = serde_json::json!([ + { "id": 1, "body": "just text" } + ]); + assert_eq!( + find_existing_comment(&comments, Some("github-actions[bot]")), + None + ); + assert_eq!(find_existing_comment(&serde_json::json!([]), None), None); + } + #[test] fn contents_url_encodes_path_segments() { let url = contents_url("owner/repo", "src/a b#c.rs", "deadbeef").unwrap(); diff --git a/crates/code-review-cli/src/main.rs b/crates/code-review-cli/src/main.rs index 456592f..d660eea 100755 --- a/crates/code-review-cli/src/main.rs +++ b/crates/code-review-cli/src/main.rs @@ -1,6 +1,7 @@ mod config; mod error; mod github; +mod report; mod review; mod source; mod vllm; @@ -82,8 +83,9 @@ async fn main() { println!("{review_text}"); - // Post comment (the issues endpoint serves both PRs and issues) - match github::post_comment( + // Post comment (the issues endpoint serves both PRs and issues). + // Updates the previous review comment in place when one exists. + match github::post_or_update_comment( &review_text, &gh.repository, target_number, @@ -92,7 +94,7 @@ async fn main() { ) .await { - Ok(()) => println!("Review comment posted successfully."), + Ok(()) => println!("Review comment posted/updated successfully."), Err(e) => { eprintln!("Review generated but comment posting failed: {e}"); std::process::exit(1); @@ -164,11 +166,19 @@ async fn review_pr( }; let paths = source::extract_modified_files(&diff); - let source_files = if head_sha.is_empty() { - Vec::new() + let (source_files, unfetched) = if head_sha.is_empty() { + (Vec::new(), paths) } else { - github::fetch_source_files(&gh.repository, &head_sha, &paths, &gh.token, client).await + let (fetched, failed) = + github::fetch_source_files(&gh.repository, &head_sha, &paths, &gh.token, client).await; + if !failed.is_empty() { + eprintln!( + "Warning: {} file(s) could not be fetched for verification.", + failed.len() + ); + } + (fetched, failed) }; - review::review_diff(&diff, &source_files, model, client, cfg).await + review::review_diff(&diff, &source_files, &unfetched, model, client, cfg).await } diff --git a/crates/code-review-cli/src/report.rs b/crates/code-review-cli/src/report.rs new file mode 100644 index 0000000..9cd7b86 --- /dev/null +++ b/crates/code-review-cli/src/report.rs @@ -0,0 +1,309 @@ +//! Deterministic post-processing of LLM review output. +//! +//! The two-round strategy asks the model for a verification pass followed by a +//! final report. Real-world runs leak internal scaffolding into the published +//! comment: the raw verification reasoning (including chain-of-thought and +//! self-corrections), a YAML front-matter block, duplicated table rows, and +//! dangling headings when the generation hits the token cap. This module +//! cleans all of that deterministically — no LLM involved — and reconciles the +//! claimed `findings_total` with the actual number of table rows. + +/// Header that starts the publishable final report section. +const REPORT_HEADER: &str = "## 🔍 AI Code Review"; + +/// Keep only the final report section of a verifier output: everything from +/// the LAST `## 🔍 AI Code Review` header to the end of the text. Verification +/// reasoning, verdict tables and YAML front-matter that precede the report are +/// dropped. If the header is absent (single-round / summarize outputs), the +/// input is returned unchanged. +pub fn extract_final_report(raw: &str) -> String { + match raw.rfind(REPORT_HEADER) { + Some(pos) => raw[pos..].trim_start().to_string(), + None => raw.to_string(), + } +} + +/// Remove trailing heading lines that are followed by nothing (a symptom of a +/// generation truncated by the token cap, e.g. a dangling `#### Issue`). +pub fn trim_trailing_empty_headings(md: &str) -> String { + let trimmed = md.trim_end(); + let mut lines: Vec<&str> = trimmed.lines().collect(); + while let Some(last) = lines.last() { + if last.trim_start().starts_with('#') { + lines.pop(); + } else { + break; + } + } + let mut out = lines.join("\n"); + if !out.is_empty() { + out.push('\n'); + } + out +} + +/// Normalize a table row for comparison: strip the leading `#` index column +/// and the `Location` column, lowercase and collapse whitespace in the +/// remaining cells. Two rows describing the same issue at different locations +/// are therefore duplicates; the first occurrence (with its location) is kept. +fn normalize_row(row: &str) -> String { + let cells: Vec = row + .trim() + .trim_start_matches('|') + .trim_end_matches('|') + .split('|') + .skip(2) // ignore the leading # column and the Location column + .map(|c| { + c.split_whitespace() + .collect::>() + .join(" ") + .to_lowercase() + }) + .collect(); + cells.join("|") +} + +fn is_table_row(line: &str) -> bool { + let t = line.trim_start(); + t.starts_with('|') && t.ends_with('|') +} + +fn is_separator_row(line: &str) -> bool { + let t = line.trim(); + t.starts_with('|') && t.contains("---") +} + +/// Drop duplicate data rows inside each markdown table. Two rows are +/// duplicates when every cell except the leading `#` index column matches +/// (case- and whitespace-insensitive). Header and separator rows are kept. +pub fn dedup_exact_table_rows(md: &str) -> (String, usize) { + let mut out: Vec = Vec::new(); + let mut seen: Vec = Vec::new(); + let mut removed = 0usize; + + for line in md.lines() { + if is_table_row(line) && !is_separator_row(line) { + let key = normalize_row(line); + if seen.contains(&key) { + removed += 1; + continue; + } + seen.push(key); + } else if !is_table_row(line) { + // Leaving a table resets dedup scope to the next table. + seen.clear(); + } + out.push(line.to_string()); + } + + let mut result = out.join("\n"); + if !result.is_empty() { + result.push('\n'); + } + (result, removed) +} + +/// Count data rows in the report's tables (rows starting a table are counted +/// except headers/separators). +fn count_table_rows(md: &str) -> usize { + md.lines() + .filter(|l| is_table_row(l) && !is_separator_row(l)) + .filter(|l| !l.trim().starts_with("| #") && !l.trim().starts_with("|#")) + .count() +} + +/// Extract the claimed `findings_total` from a YAML front-matter block, if any. +fn claimed_findings_total(md: &str) -> Option { + for line in md.lines() { + let t = line.trim(); + if let Some(rest) = t.strip_prefix("findings_total:") { + return rest.trim().parse().ok(); + } + } + None +} + +/// Reconcile the claimed findings count with the actual number of table rows. +/// Returns `(actual_rows, claimed)`. +pub fn reconcile_findings_total(md: &str) -> (usize, Option) { + (count_table_rows(md), claimed_findings_total(md)) +} + +/// Build a caveat disclosing that some modified files could not be fetched, +/// so findings touching them were only verified against the diff. +pub fn coverage_caveat(unfetched: &[String]) -> String { + let list = unfetched.join(", "); + format!( + "> ⚠️ Coverage: {} modified file(s) could not be fetched for verification: {}. \ + Findings touching these files are based on the diff only.", + unfetched.len(), + list + ) +} + +/// Full cleaning pipeline for LLM review output. Returns the publishable +/// Markdown plus human-readable notes about what was changed. +pub fn sanitize(raw: &str) -> (String, Vec) { + let mut notes = Vec::new(); + let claimed = claimed_findings_total(raw); + + let extracted = extract_final_report(raw); + if extracted != raw.trim_start() { + notes.push("Removed verification scaffolding (raw reasoning, front-matter) — only the final report is published.".to_string()); + } + + let (deduped, removed) = dedup_exact_table_rows(&extracted); + if removed > 0 { + notes.push(format!("Removed {removed} duplicated table row(s).")); + } + + let trimmed = trim_trailing_empty_headings(&deduped); + if trimmed != deduped { + notes.push("Trimmed dangling heading(s) left by a truncated generation.".to_string()); + } + + let (actual, _) = reconcile_findings_total(&trimmed); + if let Some(claimed) = claimed { + if claimed != actual { + notes.push(format!( + "Reconciled findings count: report claims {claimed} findings but contains {actual} table rows." + )); + } + } + println!("Report findings (table rows): {actual}"); + + (trimmed, notes) +} + +#[cfg(test)] +mod tests { + use super::*; + + const HEADER: &str = "## 🔍 AI Code Review"; + + #[test] + fn extract_keeps_only_final_report() { + let raw = "### Verification of Findings\n\n#### 1. `a.py:1` — thing\n\ + **Status:** CONFIRMED\nWait, let's re-read.\n*Correction*: blah\n\ + **Status: REJECTED**\n\n---\n\n### Final Report\n\n---\n\ + findings_total: 1\ntop_files:\n - a.py\nrisk_score: low\n---\n\n\ + ## 🔍 AI Code Review\n\n### 📋 Summary\nAll good.\n"; + let out = extract_final_report(raw); + assert!(out.starts_with(HEADER)); + assert!(out.contains("All good.")); + assert!(!out.contains("Verification of Findings")); + assert!(!out.contains("Wait, let's re-read")); + assert!(!out.contains("findings_total")); + assert!(!out.contains("Status: REJECTED")); + } + + #[test] + fn extract_falls_back_when_no_report_header() { + let raw = "just some markdown\nwith no header"; + assert_eq!(extract_final_report(raw), raw); + } + + #[test] + fn extract_uses_last_occurrence_of_header() { + let raw = "## 🔍 AI Code Review\n\nstale draft\n\n## 🔍 AI Code Review\n\nfinal\n"; + let out = extract_final_report(raw); + assert!(out.starts_with(HEADER)); + assert!(out.contains("final")); + assert!(!out.contains("stale draft")); + } + + #[test] + fn trim_removes_dangling_trailing_heading() { + let md = "| 1 | `a.py:1` | bug | 🟡 Medium |\n\n#### Issue 2\n"; + let out = trim_trailing_empty_headings(md); + assert!(out.contains("| 1 |")); + assert!(!out.contains("#### Issue 2")); + assert!(out.ends_with('\n')); + } + + #[test] + fn trim_keeps_content_after_heading() { + let md = "#### Issue 1\nsome detail text\n"; + assert_eq!(trim_trailing_empty_headings(md), md); + } + + #[test] + fn dedup_removes_identical_rows_ignoring_index_and_case() { + let md = "| # | Location | Issue | Severity |\n\ + |---|----------|-------|----------|\n\ + | 14 | `tests/a.py:145` | Test dependency: calls another test. | 🟢 Low |\n\ + | 15 | `tests/a.py:153` | Test dependency: calls another test. | 🟢 Low |\n\ + | 16 | `tests/a.py:145` | Test dependency: calls another test. | 🟢 Low |\n\ + | 17 | `tests/b.py:1` | Different issue. | 🟢 Low |\n"; + let (out, removed) = dedup_exact_table_rows(md); + assert_eq!(removed, 2); + assert!(out.contains("| 14 |")); + assert!(out.contains("`tests/b.py:1`")); + assert!(!out.contains("| 15 |")); + assert!(!out.contains("| 16 |")); + } + + #[test] + fn dedup_does_not_cross_table_boundaries() { + let md = "| # | Location | Issue | Severity |\n\ + |---|----------|-------|----------|\n\ + | 1 | `a.py:1` | bug | 🟡 Medium |\n\ + \n\ + | # | Location | Issue | Risk Level |\n\ + |---|----------|-------|------------|\n\ + | 1 | `a.py:1` | bug | 🟡 Medium |\n"; + let (_, removed) = dedup_exact_table_rows(md); + assert_eq!(removed, 0); + } + + #[test] + fn reconcile_counts_rows_and_parses_claim() { + let md = "---\nfindings_total: 5\n---\n\n## 🔍 AI Code Review\n\n\ + | # | Location | Issue | Severity |\n|---|---|---|---|\n\ + | 1 | `a.py:1` | x | 🟡 Medium |\n| 2 | `b.py:2` | y | 🟢 Low |\n"; + let (actual, claimed) = reconcile_findings_total(md); + assert_eq!(actual, 2); + assert_eq!(claimed, Some(5)); + } + + #[test] + fn sanitize_full_pipeline_on_realistic_verifier_output() { + let raw = "### Verification of Findings\n\n#### 1. `a.py:1` — Hardcoded secret\n\ + **Status:** CONFIRMED\nReasoning: yes.\n\n#### 2. `b.py:2` — thing\n\ + **Status:** CONFIRMED\nWait, let's re-read.\n**Status: REJECTED**\n\n\ + ---\n\n### Final Report\n\n---\nfindings_total: 18\n---\n\n\ + ## 🔍 AI Code Review\n\n### 📋 Summary\nBad.\n\n\ + | # | Location | Issue | Severity |\n|---|---|---|---|\n\ + | 1 | `a.py:1` | leak | 🔴 Critical |\n\ + | 2 | `t.py:9` | calls another test | 🟢 Low |\n\ + | 3 | `t.py:9` | calls another test | 🟢 Low |\n\n#### Issue\n"; + let (out, notes) = sanitize(raw); + assert!(out.starts_with(HEADER)); + assert!(!out.contains("Verification of Findings")); + assert!(!out.contains("findings_total")); + assert!(!out.contains("| 3 |")); + assert!(!out.ends_with("#### Issue")); + assert!(notes.iter().any(|n| n.contains("duplicated table row"))); + assert!(notes.iter().any(|n| n.contains("dangling heading"))); + assert!(notes.iter().any(|n| n.contains("claims 18"))); + } + + #[test] + fn sanitize_is_noop_for_clean_single_round_output() { + let raw = "## 🔍 AI Code Review\n\n### 📋 Summary\nFine.\n"; + let (out, notes) = sanitize(raw); + assert_eq!( + out.trim_end(), + "## 🔍 AI Code Review\n\n### 📋 Summary\nFine." + ); + assert!(notes.is_empty()); + } + + #[test] + fn coverage_caveat_lists_files_and_count() { + let caveat = coverage_caveat(&["app/a.py".to_string(), "app/b.py".to_string()]); + assert!(caveat.contains("2 modified file(s)")); + assert!(caveat.contains("app/a.py, app/b.py")); + assert!(caveat.contains("diff only")); + } +} diff --git a/crates/code-review-cli/src/review.rs b/crates/code-review-cli/src/review.rs index 0d24381..b191032 100755 --- a/crates/code-review-cli/src/review.rs +++ b/crates/code-review-cli/src/review.rs @@ -174,9 +174,13 @@ const SINGLE_ROUND_USER_TEMPLATE: &str = concat!( /// Review a diff using the appropriate strategy: /// - 1 file -> single round (one direct `chat_complete` call) /// - N files -> two rounds (per-chunk bullets + verification against source) +/// +/// `unfetched` lists modified files whose source could not be retrieved; when +/// non-empty, a coverage caveat is prepended to the published report. pub async fn review_diff( diff: &str, source_files: &[(String, String)], + unfetched: &[String], model: &str, client: &reqwest::Client, cfg: &Config, @@ -195,11 +199,19 @@ pub async fn review_diff( } ); - if file_count <= 1 { + let report = if file_count <= 1 { single_round_review(diff, source_files, model, client, cfg).await } else { multi_round_review(diff, source_files, model, client, cfg).await - } + }; + + report.map(|r| { + if unfetched.is_empty() { + r + } else { + format!("{}\n\n{}", crate::report::coverage_caveat(unfetched), r) + } + }) } async fn single_round_review( @@ -280,7 +292,20 @@ async fn multi_round_review( // Round 2: verify findings against source code, fallback to summarize let verified = verify_findings(&valid_reviews, source_files, model, client, cfg).await; match verified { - Some(report) => Some(report), + Some(report) => { + // Deterministic post-processing: keep only the final report, + // strip leaked reasoning/front-matter, dedup rows, trim dangling + // headings. Falls back to the raw output if cleaning empties it. + let (clean, notes) = crate::report::sanitize(&report); + for note in ¬es { + eprintln!("report: {note}"); + } + if clean.trim().is_empty() { + Some(report) + } else { + Some(clean) + } + } None => { // Fallback to summarization if verification fails match summarize_review(&valid_reviews, model, client, cfg).await { @@ -302,16 +327,24 @@ const VERIFY_SYSTEM_PROMPT: &str = Your job is to FILTER, not to ADD. For each finding, determine if it is correct based on the \ evidence in the source code provided. Be skeptical of findings that make claims about APIs, \ frameworks, or language features that cannot be verified from the code. \ - Reject any finding that is speculative or factually incorrect."; + Reject any finding that is speculative or factually incorrect. \ + Never reason out loud: no self-corrections, no re-readings, no chain-of-thought. \ + Decide silently, then write only the final verdict and its short reason."; const VERIFY_USER_TEMPLATE: &str = concat!( "Verify each finding below against the source code provided.\n\n", - "For each finding, classify as:\n", + "## Step 1 — verdict table\n\n", + "Output one compact table with your verdict for every finding:\n\n", + "| # | Verdict | Reason |\n", + "|---|---------|--------|\n", + "| | CONFIRMED / REJECTED / DOWNGRADED | One short sentence, max 25 words |\n\n", + "Verdicts:\n", "- CONFIRMED -- the issue is real and verifiable in the source code\n", "- REJECTED -- the issue is incorrect, speculative, or based on wrong assumptions\n", - "- DOWNGRADED -- the issue exists but the severity is inflated (specify correct severity)\n\n", - "Then produce the final report containing ONLY confirmed (and downgraded) findings,\n", - "using this exact structure:\n\n", + "- DOWNGRADED -- the issue exists but the severity is inflated (give correct severity in the report)\n\n", + "## Step 2 — final report\n\n", + "Then output the final report containing ONLY confirmed and downgraded findings, ", + "in exactly this structure:\n\n", "---\n", "findings_total: \n", "top_files:\n", @@ -319,27 +352,40 @@ const VERIFY_USER_TEMPLATE: &str = concat!( "risk_score: \n", "---\n\n", "## 🔍 AI Code Review\n\n", - "### 📋 Summary\n", + "### 📋 Summary\n\n", "Write 2-4 sentences summarizing the changes and verified findings.\n\n", - "### 🛠 Code Quality Issues\n", + "### 🛠 Code Quality Issues\n\n", "| # | Location | Issue | Severity |\n", "|---|----------|-------|----------|\n", - "(only confirmed/downgraded findings)\n\n", - "For each confirmed issue:\n", - "#### Issue N: \n", - "**Verified:** Explain what in the source code confirms this issue.\n", - "**Suggestion:** Concrete fix.\n\n", - "### 🔒 Security Issues\n", + "| <n> | `file:line` | Description | 🔴 Critical / 🟠 High / 🟡 Medium / 🟢 Low |\n\n", + "### 🔒 Security Issues\n\n", "| # | Location | Issue | Risk Level |\n", "|---|----------|-------|------------|\n", - "(only confirmed/downgraded findings)\n\n", - "### ✅ What Looks Good\n", + "| <n> | `file:line` | Description | 🔴 Critical / 🟠 High / 🟡 Medium / 🟢 Low |\n\n", + "For each finding above, add a detail block after its table:\n\n", + "#### Issue N: <short title>\n", + "**Verified:** What in the source code confirms this (1-2 sentences).\n", + "**Suggestion:** Concrete fix.\n\n", + "### ✅ What Looks Good\n\n", "List 2-3 positive aspects.\n\n", "Rules:\n", "- Do NOT add new findings -- only verify the ones provided\n", - "- REJECTED findings must NOT appear in the final report\n", - "- An empty table is a valid outcome if all findings were rejected\n", - "- Sort confirmed findings: Critical first, then High, Medium, Low\n\n", + "- CRITICAL: EVERY CONFIRMED or DOWNGRADED finding MUST appear in one of the two report tables. ", + "A confirmed finding missing from the report tables is an output error.\n", + "- findings_total MUST equal the total number of table rows in both tables\n", + "- REJECTED findings must NOT appear in the report\n", + "- Deduplicate: merge findings that describe the same underlying problem found in several places ", + "into a single row; do not emit near-identical rows\n", + "- Sort table rows: Critical first, then High, Medium, Low; use the emoji severity badges\n", + "- EVERY table row MUST have a matching detail block below its table\n", + "- An empty report is a valid outcome if all findings were rejected\n", + "- Do NOT cite standards (RFCs, specs) unless you are certain of their EXACT requirement. ", + "Example of a common mistake: RFC 6749 section 5.2 actually mandates HTTP 400 for invalid ", + "credentials at the OAuth2 token endpoint, so returning 400 there is compliant. ", + "If you are not certain something violates a standard, DOWNGRADE it to Low or REJECT it -- ", + "never CONFIRM a spec violation you have not verified.\n", + "- The verdict table is data, not prose: no chain-of-thought, no self-corrections, ", + "no 'let me re-read', no reasoning longer than the one-sentence Reason cell\n\n", "[FINDINGS TO VERIFY]\n\n", ); @@ -510,7 +556,7 @@ pub async fn verify_findings( source_files.len() ); - match vllm::chat_complete(&messages, model, 4096, 0.1, client, cfg).await { + match vllm::chat_complete(&messages, model, cfg.verify_max_tokens, 0.1, client, cfg).await { Ok(text) => Some(text), Err(e) => { eprintln!("Warning: verification failed: {e}"); @@ -913,6 +959,82 @@ mod tests { assert!(VERIFY_USER_TEMPLATE.contains("[FINDINGS TO VERIFY]")); } + #[test] + fn verify_template_forbids_chain_of_thought() { + assert!( + VERIFY_SYSTEM_PROMPT.contains("no self-corrections") + && VERIFY_SYSTEM_PROMPT.contains("no chain-of-thought"), + "system prompt must forbid visible reasoning" + ); + assert!( + VERIFY_USER_TEMPLATE.contains("no 'let me re-read'"), + "user template must ban self-correction prose in the verdict table" + ); + } + + #[test] + fn verify_template_uses_same_report_format_as_single_round() { + for marker in &["🔴 Critical", "🟠 High", "🟡 Medium", "🟢 Low"] { + assert!( + VERIFY_USER_TEMPLATE.contains(marker), + "verify report must use the same emoji severity badges: {marker}" + ); + } + assert!(VERIFY_USER_TEMPLATE.contains("### ✅ What Looks Good")); + assert!(VERIFY_USER_TEMPLATE.contains("findings_total:")); + assert!(VERIFY_USER_TEMPLATE.contains("EVERY table row MUST have a matching detail block")); + } + + #[test] + fn verify_template_requires_complete_report() { + assert!( + VERIFY_USER_TEMPLATE + .contains("A confirmed finding missing from the report tables is an output error"), + "template must forbid losing confirmed findings between verdict and report" + ); + assert!( + VERIFY_USER_TEMPLATE + .contains("findings_total MUST equal the total number of table rows"), + "template must reconcile findings_total with actual rows" + ); + assert!( + VERIFY_USER_TEMPLATE + .contains("Deduplicate: merge findings that describe the same underlying problem"), + "template must require deduplication of repeated findings" + ); + } + + #[test] + fn verify_template_guards_spec_citations() { + assert!( + VERIFY_USER_TEMPLATE + .contains("Do NOT cite standards (RFCs, specs) unless you are certain"), + "template must forbid unverified spec citations" + ); + assert!( + VERIFY_USER_TEMPLATE.contains("RFC 6749 section 5.2"), + "template must carry the known counter-example (OAuth2 token endpoint returns 400)" + ); + assert!( + VERIFY_USER_TEMPLATE.contains("never CONFIRM a spec violation you have not verified"), + "template must downgrade uncertain spec claims instead of confirming them" + ); + } + + #[test] + fn verify_template_outputs_verdict_table_before_report() { + let verdict_pos = VERIFY_USER_TEMPLATE + .find("| # | Verdict | Reason |") + .expect("verdict table header"); + let report_pos = VERIFY_USER_TEMPLATE + .rfind("## 🔍 AI Code Review") + .expect("report header"); + assert!( + verdict_pos < report_pos, + "verdicts must come before the final report so the report can be extracted cleanly" + ); + } + #[test] fn issue_template_contains_sections() { assert!(ISSUE_USER_TEMPLATE.contains("AI Issue Triage")); diff --git a/docs/code-review.md b/docs/code-review.md index e30d367..265885a 100755 --- a/docs/code-review.md +++ b/docs/code-review.md @@ -50,7 +50,7 @@ completes. Repos that already run CI on PRs may instead point | Endpoint | Any OpenAI-compatible, HTTPS-reachable `…/v1` endpoint (self-hosted or hosted) | | Authentication | Optional bearer key (`vllm-api-key`) and/or extra headers (e.g. gateway tokens) | | Repository secrets | `VLLM_URL`, `VLLM_API_KEY` (+ any header secrets your endpoint needs) | -| Repository vars | `VLLM_MODEL`, `VLLM_TIMEOUT`, `VLLM_RETRIES` (optional; consumed via `vars.*`) | +| Repository vars | `VLLM_MODEL`, `VLLM_TIMEOUT`, `VLLM_RETRIES`, `VLLM_VERIFY_MAX_TOKENS` (optional; consumed via `vars.*`) | Any provider exposing OpenAI-style `POST /v1/chat/completions` and `GET /v1/models` works (self-hosted vLLM, OpenAI, OpenRouter, Groq, Together, Ollama…). The endpoint @@ -65,7 +65,8 @@ see [**Reaching a private inference endpoint**](recipes/expose-private-endpoint. 4. **Review strategy** (chosen automatically based on diff size): - **Single-round** (1 file changed) — the full diff is sent in one `chat/completions` call - **Two-round** (multiple files) — the diff is split by file into ~2000-word chunks and reviewed **sequentially** (bullet findings); a **verification** pass then checks each finding against the actual source files (fetched from the repo at the PR head SHA via the GitHub Contents API) to confirm, downgrade, or filter it. If verification cannot run, it falls back to a summarization pass. -5. **Post comment** — aggregates the findings into a single Markdown comment and posts it on the PR (truncated safely at 60 000 characters) +5. **Deterministic report cleanup** — the two-round output is post-processed locally (no extra LLM call): only the final report is kept (verification reasoning and YAML front-matter are stripped), exact-duplicate table rows are merged, dangling headings left by a truncated generation are trimmed, and the claimed `findings_total` is reconciled with the actual number of rows. When some modified files could not be fetched, a coverage caveat is prepended to the report. +6. **Post comment** — aggregates the findings into a single Markdown comment and posts it on the PR (truncated safely at 60 000 characters). The comment carries a hidden marker and is **updated in place** on subsequent runs instead of stacking a new comment per run. ## Issue triage