diff --git a/README.md b/README.md index 33068d6..c217b30 100644 --- a/README.md +++ b/README.md @@ -33,6 +33,11 @@ Oversized hunks receive recalculated old/new coordinates in every fragment, and each bounded batch is synthesised independently before reports are joined. The collected file count is also checked against the pull request metadata so GitHub's 3,000-file endpoint limit cannot look like complete coverage. +The deterministic team verdict is `INCOMPLETE` whenever any input gap remains +or the verification cap leaves findings unchecked; a confirmed critical still +takes precedence as `NEEDS_WORK`. Re-running a mode on the same head commit +updates its global and inline bot comments instead of publishing duplicates, +and removes inline results that are no longer confirmed by that mode. ### Calling the reusable workflow diff --git a/tools/ai-review/src/github.rs b/tools/ai-review/src/github.rs index bb31c62..7801626 100644 --- a/tools/ai-review/src/github.rs +++ b/tools/ai-review/src/github.rs @@ -1,6 +1,6 @@ #![allow(clippy::missing_errors_doc)] -use std::collections::{BTreeSet, HashMap, HashSet}; +use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::fmt::Write as _; use anyhow::Context; @@ -375,14 +375,19 @@ pub async fn upsert_global_comment( body: &str, marker: &str, ) -> anyhow::Result<()> { - let comments = octo + let first_page = octo .issues(owner, repo) .list_comments(pr_number) + .per_page(100) .send() .await .context("failed to list PR comments")?; + let comments = octo + .all_pages(first_page) + .await + .context("failed to fetch every PR comment page")?; - let existing_id = comments.items.iter().find_map(|c| { + let existing_id = comments.iter().find_map(|c| { c.body .as_deref() .filter(|b| has_bot_marker(b, marker)) @@ -420,63 +425,248 @@ struct GhComment<'a> { body: &'a str, } -/// Post inline review comments for critical findings via GitHub REST API. +#[derive(Debug)] +struct PreparedInlineComment { + path: String, + line: u32, + body: String, + marker: String, +} + +#[derive(Debug, serde::Deserialize)] +struct ExistingReviewComment { + id: u64, + #[serde(default)] + body: String, +} + +/// Upserts inline review comments for one mode and head commit. /// -/// Falls back silently (warning to stderr) if the review API returns an error. -pub async fn post_inline_comments( +/// Findings that share a line are combined into one comment. A stable hidden +/// marker lets reruns update that comment instead of publishing duplicates. +pub async fn upsert_inline_comments( token: &str, owner: &str, repo: &str, pr_number: u64, head_sha: &str, + namespace: &str, comments: &[InlineComment], ) -> anyhow::Result<()> { - if comments.is_empty() { - return Ok(()); - } - - let gh_comments: Vec> = comments - .iter() - .map(|c| GhComment { - path: &c.path, - line: c.line, - side: "RIGHT", - body: &c.body, - }) - .collect(); - - let request = ReviewRequest { - commit_id: head_sha, - body: "", - event: "COMMENT", - comments: gh_comments, - }; - - let url = format!("https://api.github.com/repos/{owner}/{repo}/pulls/{pr_number}/reviews"); - let client = reqwest::Client::builder() .user_agent("ai-review-bot/0.1") .build()?; + upsert_inline_comments_at( + &client, + "https://api.github.com", + token, + owner, + repo, + pr_number, + head_sha, + namespace, + comments, + ) + .await +} - let resp = client - .post(&url) - .bearer_auth(token) - .header("Accept", "application/vnd.github+json") - .header("X-GitHub-Api-Version", "2022-11-28") - .json(&request) - .send() - .await - .context("failed to post inline review")?; +#[allow(clippy::too_many_arguments)] +async fn upsert_inline_comments_at( + client: &reqwest::Client, + api_base: &str, + token: &str, + owner: &str, + repo: &str, + pr_number: u64, + head_sha: &str, + namespace: &str, + comments: &[InlineComment], +) -> anyhow::Result<()> { + let prepared = prepare_inline_comments(namespace, head_sha, comments); + let existing = list_review_comments(client, api_base, token, owner, repo, pr_number).await?; + let mut new_comments = Vec::new(); + let mut retained_existing_ids = HashSet::new(); - let status = resp.status(); - if !status.is_success() { - let body_text = resp.text().await.unwrap_or_default(); - eprintln!("warning: inline review returned {status}: {body_text}"); + for comment in &prepared { + if let Some(found) = existing + .iter() + .find(|existing| existing.body.contains(&comment.marker)) + { + retained_existing_ids.insert(found.id); + if found.body != comment.body { + let url = format!( + "{api_base}/repos/{owner}/{repo}/pulls/comments/{}", + found.id + ); + client + .patch(url) + .bearer_auth(token) + .header("Accept", "application/vnd.github+json") + .header("X-GitHub-Api-Version", "2022-11-28") + .json(&serde_json::json!({ "body": comment.body })) + .send() + .await + .context("failed to update inline review comment")? + .error_for_status() + .context("GitHub rejected inline review comment update")?; + } + } else { + new_comments.push(comment); + } + } + + if !new_comments.is_empty() { + let gh_comments: Vec> = new_comments + .iter() + .map(|comment| GhComment { + path: &comment.path, + line: comment.line, + side: "RIGHT", + body: &comment.body, + }) + .collect(); + let request = ReviewRequest { + commit_id: head_sha, + body: "", + event: "COMMENT", + comments: gh_comments, + }; + let url = format!("{api_base}/repos/{owner}/{repo}/pulls/{pr_number}/reviews"); + + client + .post(&url) + .bearer_auth(token) + .header("Accept", "application/vnd.github+json") + .header("X-GitHub-Api-Version", "2022-11-28") + .json(&request) + .send() + .await + .context("failed to post inline review")? + .error_for_status() + .context("GitHub rejected inline review")?; + } + + let scope_prefix = inline_scope_prefix(namespace, head_sha); + for stale in existing.iter().filter(|existing| { + existing.body.contains(&scope_prefix) && !retained_existing_ids.contains(&existing.id) + }) { + let url = format!( + "{api_base}/repos/{owner}/{repo}/pulls/comments/{}", + stale.id + ); + client + .delete(url) + .bearer_auth(token) + .header("Accept", "application/vnd.github+json") + .header("X-GitHub-Api-Version", "2022-11-28") + .send() + .await + .context("failed to delete stale inline review comment")? + .error_for_status() + .context("GitHub rejected stale inline review comment deletion")?; } Ok(()) } +async fn list_review_comments( + client: &reqwest::Client, + api_base: &str, + token: &str, + owner: &str, + repo: &str, + pr_number: u64, +) -> anyhow::Result> { + const PER_PAGE: usize = 100; + let url = format!("{api_base}/repos/{owner}/{repo}/pulls/{pr_number}/comments"); + let mut all = Vec::new(); + let mut page = 1_u32; + loop { + let current: Vec = client + .get(&url) + .bearer_auth(token) + .header("Accept", "application/vnd.github+json") + .header("X-GitHub-Api-Version", "2022-11-28") + .query(&[("per_page", PER_PAGE), ("page", page as usize)]) + .send() + .await + .context("failed to list inline review comments")? + .error_for_status() + .context("GitHub rejected inline review comment listing")? + .json() + .await + .context("failed to decode inline review comments")?; + let is_last = current.len() < PER_PAGE; + all.extend(current); + if is_last { + break; + } + page = page + .checked_add(1) + .context("inline review comment pagination overflowed")?; + } + Ok(all) +} + +fn prepare_inline_comments( + namespace: &str, + head_sha: &str, + comments: &[InlineComment], +) -> Vec { + let mut grouped: BTreeMap<(String, u32), Vec> = BTreeMap::new(); + for comment in comments { + grouped + .entry((comment.path.clone(), comment.line)) + .or_default() + .push(comment.body.clone()); + } + + grouped + .into_iter() + .map(|((path, line), mut bodies)| { + bodies.sort(); + bodies.dedup(); + let marker = inline_comment_marker(namespace, head_sha, &path, line); + PreparedInlineComment { + path, + line, + body: format!("{}\n\n{marker}", bodies.join("\n\n---\n\n")), + marker, + } + }) + .collect() +} + +fn inline_comment_marker(namespace: &str, head_sha: &str, path: &str, line: u32) -> String { + let scope = stable_inline_hash(&[namespace.as_bytes(), head_sha.as_bytes()], None); + let location = stable_inline_hash(&[path.as_bytes()], Some(line)); + format!("") +} + +fn inline_scope_prefix(namespace: &str, head_sha: &str) -> String { + let scope = stable_inline_hash(&[namespace.as_bytes(), head_sha.as_bytes()], None); + format!("\n## Team Review\n\n".to_string(); @@ -205,6 +206,21 @@ pub fn render_team_comment(view: &TeamCommentView) -> String { md } +/// Renders the fail-closed team result used when no synthesis can be scored. +#[must_use] +pub fn render_incomplete_team_comment(reason: &str, coverage_gaps: &[CoverageGap]) -> String { + let mut md = "\n## Team Review\n\n".to_string(); + md.push_str("**Verdict: INCOMPLETE ⛔**\n\n"); + let _ = writeln!(md, "⚠️ Team review unavailable: {reason}."); + if !coverage_gaps.is_empty() { + md.push('\n'); + for gap in coverage_gaps { + let _ = writeln!(md, "- `{}`: {} ({:?})", gap.file, gap.detail, gap.kind); + } + } + md +} + /// Renders one language block as a `
` section (French opens by default). fn render_lang_block(view: &TeamCommentView, lang: Lang) -> String { let labels = Labels::for_lang(lang); @@ -554,7 +570,7 @@ mod tests { strengths: &[], strengths_fr: &[], scored: &[], - verdict: Verdict::Ship, + verdict: Verdict::Incomplete, file_count: 1, agents_ok: &[], agents_failed: &[], @@ -566,7 +582,22 @@ mod tests { }; let md = render_team_comment(&view); assert!(md.contains("Partial review coverage")); + assert!(md.contains("INCOMPLETE")); assert!(md.contains("`asset.bin`")); assert!(md.contains("PatchUnavailable")); } + + #[test] + fn renders_incomplete_when_no_synthesis_is_available() { + let gaps = [CoverageGap { + kind: crate::types::CoverageGapKind::SynthesisFailed, + file: "batch 1".to_string(), + detail: "synthesis failed".to_string(), + }]; + let md = render_incomplete_team_comment("every batch synthesis failed", &gaps); + assert!(md.starts_with("")); + assert!(md.contains("Verdict: INCOMPLETE")); + assert!(md.contains("every batch synthesis failed")); + assert!(md.contains("`batch 1`")); + } } diff --git a/tools/ai-review/src/team.rs b/tools/ai-review/src/team.rs index 0610daa..0cef114 100644 --- a/tools/ai-review/src/team.rs +++ b/tools/ai-review/src/team.rs @@ -35,7 +35,16 @@ pub async fn run_team( println!("Fetching PR #{pr_number} diff for team review…"); let mut ctx = github::fetch_diff_context(&clients.octo, owner, repo, pr_number).await?; if ctx.full.trim().is_empty() { - println!("Empty diff: nothing to review."); + if ctx.coverage_gaps.is_empty() { + println!("Empty diff: nothing to review."); + } else { + let body = review::render_incomplete_team_comment( + "no textual patch was available for review", + &ctx.coverage_gaps, + ); + github::upsert_global_comment(&clients.octo, owner, repo, pr_number, &body, MARKER) + .await?; + } return Ok(()); } @@ -43,19 +52,12 @@ pub async fn run_team( run_batch_plan(clients, &ctx.batches).await; ctx.coverage_gaps.extend(agent_gaps); if batch_runs.iter().all(|run| run.reports.is_empty()) { - let gaps = ctx - .coverage_gaps - .iter() - .map(|gap| format!("- `{}`: {} ({:?})", gap.file, gap.detail, gap.kind)) - .collect::>() - .join("\n"); let reason = if ctx.batches.is_empty() { "no textual patch could be placed in a review batch" } else { "all specialist agents failed" }; - let body = - format!("{MARKER}\n## Team Review\n\n⚠️ Team review unavailable: {reason}.\n\n{gaps}"); + let body = review::render_incomplete_team_comment(reason, &ctx.coverage_gaps); github::upsert_global_comment(&clients.octo, owner, repo, pr_number, &body, MARKER).await?; return Ok(()); } @@ -65,16 +67,16 @@ pub async fn run_team( .flat_map(|run| &run.reports) .map(|(_, report)| report.findings.len()) .sum(); - let (mut synth, synthesis_gaps) = synthesize_batches(clients, &batch_runs).await; + let (synth, synthesis_gaps) = synthesize_batches(clients, &batch_runs).await; ctx.coverage_gaps.extend(synthesis_gaps); - if synth.is_none() { - let body = format!( - "{MARKER}\n## Team Review\n\n⚠️ Team review unavailable: every batch synthesis failed." + let Some(mut synth) = synth else { + let body = review::render_incomplete_team_comment( + "every batch synthesis failed", + &ctx.coverage_gaps, ); github::upsert_global_comment(&clients.octo, owner, repo, pr_number, &body, MARKER).await?; return Ok(()); - } - let mut synth = synth.take().expect("checked above"); + }; let mut findings = std::mem::take(&mut synth.findings); findings.sort_by_key(|f| severity_rank(&f.severity)); @@ -96,7 +98,7 @@ pub async fn run_team( ); let verdicts = verify_findings(clients, &ctx, &findings).await; let scored: Vec<(SynthFinding, FindingVerdict)> = findings.into_iter().zip(verdicts).collect(); - let verdict = compute_verdict(&scored); + let verdict = compute_verdict(&scored, capped > 0 || !ctx.coverage_gaps.is_empty()); let model = format!( "{} + {}", @@ -439,17 +441,14 @@ async fn post_confirmed_inline( }) .collect(); - if inline.is_empty() { - return Ok(()); - } - - println!("Posting {} confirmed inline comment(s)…", inline.len()); - github::post_inline_comments( + println!("Upserting {} confirmed inline comment(s)…", inline.len()); + github::upsert_inline_comments( &clients.github_token, owner, repo, pr_number, head_sha, + MARKER, &inline, ) .await @@ -500,16 +499,23 @@ pub fn aggregate_lens_votes(votes: &[LensVerdict]) -> FindingVerdict { } } -/// Computes the overall verdict deterministically: a confirmed critical blocks, -/// an only-contested critical invites discussion, otherwise the PR may ship. +/// Computes the overall verdict deterministically. A confirmed critical always +/// blocks. Without one, incomplete coverage prevents a ship/discuss conclusion; +/// otherwise a contested critical invites discussion and the remaining cases ship. #[must_use] -pub fn compute_verdict(scored: &[(SynthFinding, FindingVerdict)]) -> Verdict { +pub fn compute_verdict( + scored: &[(SynthFinding, FindingVerdict)], + incomplete_coverage: bool, +) -> Verdict { let confirmed_critical = scored .iter() .any(|(f, v)| f.severity == Severity::Critical && !v.contested); if confirmed_critical { return Verdict::NeedsWork; } + if incomplete_coverage { + return Verdict::Incomplete; + } let contested_critical = scored .iter() .any(|(f, v)| f.severity == Severity::Critical && v.contested); @@ -628,7 +634,7 @@ mod tests { #[test] fn verdict_needs_work_on_confirmed_critical() { let scored = vec![(finding(Severity::Critical), verdict(false))]; - assert_eq!(compute_verdict(&scored), Verdict::NeedsWork); + assert_eq!(compute_verdict(&scored, false), Verdict::NeedsWork); } #[test] @@ -637,14 +643,27 @@ mod tests { (finding(Severity::Critical), verdict(true)), (finding(Severity::Minor), verdict(false)), ]; - assert_eq!(compute_verdict(&scored), Verdict::Discuss); + assert_eq!(compute_verdict(&scored, false), Verdict::Discuss); } #[test] fn verdict_ship_without_criticals() { let scored = vec![(finding(Severity::Minor), verdict(false))]; - assert_eq!(compute_verdict(&scored), Verdict::Ship); - assert_eq!(compute_verdict(&[]), Verdict::Ship); + assert_eq!(compute_verdict(&scored, false), Verdict::Ship); + assert_eq!(compute_verdict(&[], false), Verdict::Ship); + } + + #[test] + fn verdict_incomplete_when_coverage_has_gaps_or_findings_are_capped() { + let scored = vec![(finding(Severity::Minor), verdict(false))]; + assert_eq!(compute_verdict(&scored, true), Verdict::Incomplete); + assert_eq!(compute_verdict(&[], true), Verdict::Incomplete); + } + + #[test] + fn confirmed_critical_takes_precedence_over_incomplete_coverage() { + let scored = vec![(finding(Severity::Critical), verdict(false))]; + assert_eq!(compute_verdict(&scored, true), Verdict::NeedsWork); } fn finding_in_file(file: &str, line: u32) -> SynthFinding { diff --git a/tools/ai-review/src/types.rs b/tools/ai-review/src/types.rs index 1837e1b..b19bbf4 100644 --- a/tools/ai-review/src/types.rs +++ b/tools/ai-review/src/types.rs @@ -266,6 +266,7 @@ pub enum Verdict { Ship, NeedsWork, Discuss, + Incomplete, } /// Default severity for a finding whose severity field is missing or unparseable.