From 088cdf22f1d6366176e94b695f89758bb4d72eff Mon Sep 17 00:00:00 2001 From: Pierrick Fonquerne Date: Wed, 12 Aug 2026 17:31:59 +0200 Subject: [PATCH 1/2] fix(review): fail closed on incomplete coverage --- README.md | 4 + tools/ai-review/src/github.rs | 348 +++++++++++++++++++++++++++++++--- tools/ai-review/src/main.rs | 3 +- tools/ai-review/src/review.rs | 33 +++- tools/ai-review/src/team.rs | 71 ++++--- tools/ai-review/src/types.rs | 1 + 6 files changed, 409 insertions(+), 51 deletions(-) diff --git a/README.md b/README.md index 33068d6..399d770 100644 --- a/README.md +++ b/README.md @@ -33,6 +33,10 @@ 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. ### Calling the reusable workflow diff --git a/tools/ai-review/src/github.rs b/tools/ai-review/src/github.rs index bb31c62..a5a4099 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,45 +425,120 @@ 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 + 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 +} + +#[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(); + + for comment in &prepared { + if let Some(found) = existing + .iter() + .find(|existing| existing.body.contains(&comment.marker)) + { + 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() { + return Ok(()); + } + + let gh_comments: Vec> = new_comments .iter() - .map(|c| GhComment { - path: &c.path, - line: c.line, + .map(|comment| GhComment { + path: &comment.path, + line: comment.line, side: "RIGHT", - body: &c.body, + 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"); - 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()?; - - let resp = client + client .post(&url) .bearer_auth(token) .header("Accept", "application/vnd.github+json") @@ -466,15 +546,96 @@ pub async fn post_inline_comments( .json(&request) .send() .await - .context("failed to post inline review")?; + .context("failed to post inline review")? + .error_for_status() + .context("GitHub rejected inline review")?; + + Ok(()) +} - 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}"); +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) +} - Ok(()) +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 { + // Stable FNV-1a keeps arbitrary file names out of the HTML marker while + // preserving the same identity across binaries and workflow reruns. + let mut hash = 0xcbf2_9ce4_8422_2325_u64; + for part in [namespace.as_bytes(), head_sha.as_bytes(), path.as_bytes()] { + for byte in part.iter().copied().chain(std::iter::once(0)) { + hash ^= u64::from(byte); + hash = hash.wrapping_mul(0x0000_0100_0000_01b3); + } + } + for byte in line.to_le_bytes() { + hash ^= u64::from(byte); + hash = hash.wrapping_mul(0x0000_0100_0000_01b3); + } + format!("") } /// Update the PR description body. @@ -804,4 +965,141 @@ mod tests { let finding = finding_at("a.rs", 1000); assert_eq!(ctx.lens_context(&finding), ctx.patch_for(&finding)); } + + #[test] + fn prepares_one_stable_inline_comment_per_location() { + let comments = vec![ + InlineComment { + path: "src/lib.rs".to_string(), + line: 12, + body: "second issue".to_string(), + }, + InlineComment { + path: "src/lib.rs".to_string(), + line: 12, + body: "first issue".to_string(), + }, + InlineComment { + path: "src/lib.rs".to_string(), + line: 12, + body: "first issue".to_string(), + }, + ]; + let prepared = prepare_inline_comments("team", "abc123", &comments); + assert_eq!(prepared.len(), 1); + assert_eq!(prepared[0].path, "src/lib.rs"); + assert_eq!(prepared[0].line, 12); + assert_eq!( + prepared[0].body.matches("first issue").count(), + 1, + "duplicate messages must be collapsed" + ); + assert!(prepared[0] + .body + .contains("first issue\n\n---\n\nsecond issue")); + assert!(prepared[0].body.ends_with(&prepared[0].marker)); + assert_eq!( + prepared[0].marker, + inline_comment_marker("team", "abc123", "src/lib.rs", 12) + ); + } + + #[test] + fn inline_marker_is_scoped_to_mode_head_and_location() { + let marker = inline_comment_marker("team", "abc123", "src/lib.rs", 12); + assert_ne!( + marker, + inline_comment_marker("review", "abc123", "src/lib.rs", 12) + ); + assert_ne!( + marker, + inline_comment_marker("team", "def456", "src/lib.rs", 12) + ); + assert_ne!( + marker, + inline_comment_marker("team", "abc123", "src/main.rs", 12) + ); + assert_ne!( + marker, + inline_comment_marker("team", "abc123", "src/lib.rs", 13) + ); + } + + #[tokio::test] + async fn inline_upsert_finds_markers_after_the_first_page_and_updates() { + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + use tokio::net::TcpListener; + + let comment = InlineComment { + path: "src/lib.rs".to_string(), + line: 12, + body: "fresh finding".to_string(), + }; + let marker = inline_comment_marker("team", "abc123", &comment.path, comment.line); + let first_page = serde_json::to_string( + &(0..100) + .map(|id| serde_json::json!({ "id": id, "body": "human comment" })) + .collect::>(), + ) + .expect("first page JSON"); + let second_page = serde_json::json!([{ + "id": 4242, + "body": format!("stale finding\n\n{marker}") + }]) + .to_string(); + + let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); + let address = listener.local_addr().expect("address"); + let server = tokio::spawn(async move { + for (index, body) in [first_page, second_page, "{}".to_string()] + .into_iter() + .enumerate() + { + let (mut socket, _) = listener.accept().await.expect("accept request"); + let mut request = vec![0_u8; 16_384]; + let read = socket.read(&mut request).await.expect("read request"); + let request = String::from_utf8_lossy(&request[..read]); + match index { + 0 => assert!(request.starts_with( + "GET /repos/owner/repo/pulls/7/comments?per_page=100&page=1 " + )), + 1 => assert!(request.starts_with( + "GET /repos/owner/repo/pulls/7/comments?per_page=100&page=2 " + )), + 2 => { + assert!(request.starts_with("PATCH /repos/owner/repo/pulls/comments/4242 ")); + assert!(request.contains("fresh finding")); + } + _ => unreachable!(), + } + let response = format!( + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", + body.len() + ); + socket + .write_all(response.as_bytes()) + .await + .expect("write response"); + } + }); + + let client = reqwest::Client::builder() + .user_agent("test") + .build() + .expect("client"); + upsert_inline_comments_at( + &client, + &format!("http://{address}"), + "token", + "owner", + "repo", + 7, + "abc123", + "team", + &[comment], + ) + .await + .expect("upsert"); + server.await.expect("server task"); + } } diff --git a/tools/ai-review/src/main.rs b/tools/ai-review/src/main.rs index f797e26..155fbe0 100644 --- a/tools/ai-review/src/main.rs +++ b/tools/ai-review/src/main.rs @@ -124,12 +124,13 @@ async fn run_analysis( .collect(); println!("Posting {} inline comment(s)…", comments.len()); - github::post_inline_comments( + github::upsert_inline_comments( &clients.github_token, owner, repo, pr_number, &head_sha, + marker, &comments, ) .await?; diff --git a/tools/ai-review/src/review.rs b/tools/ai-review/src/review.rs index 7327757..643e732 100644 --- a/tools/ai-review/src/review.rs +++ b/tools/ai-review/src/review.rs @@ -156,6 +156,7 @@ pub fn render_team_comment(view: &TeamCommentView) -> String { Verdict::Ship => "SHIP ✅", Verdict::NeedsWork => "NEEDS_WORK ⚠️", Verdict::Discuss => "DISCUSS 💬", + Verdict::Incomplete => "INCOMPLETE ⛔", }; let mut md = "\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..842a648 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!( "{} + {}", @@ -444,12 +446,13 @@ async fn post_confirmed_inline( } println!("Posting {} confirmed inline comment(s)…", inline.len()); - github::post_inline_comments( + github::upsert_inline_comments( &clients.github_token, owner, repo, pr_number, head_sha, + MARKER, &inline, ) .await @@ -500,16 +503,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 +638,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 +647,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. From 4a55945c74571ba6414838297dd2b28af13dddfb Mon Sep 17 00:00:00 2001 From: Pierrick Fonquerne Date: Wed, 12 Aug 2026 17:48:46 +0200 Subject: [PATCH 2/2] fix(review): remove stale inline results --- README.md | 3 +- tools/ai-review/src/github.rs | 164 ++++++++++++++++++++++++++-------- tools/ai-review/src/main.rs | 44 +++++---- tools/ai-review/src/team.rs | 6 +- 4 files changed, 149 insertions(+), 68 deletions(-) diff --git a/README.md b/README.md index 399d770..c217b30 100644 --- a/README.md +++ b/README.md @@ -36,7 +36,8 @@ 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. +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 a5a4099..7801626 100644 --- a/tools/ai-review/src/github.rs +++ b/tools/ai-review/src/github.rs @@ -453,10 +453,6 @@ pub async fn upsert_inline_comments( namespace: &str, comments: &[InlineComment], ) -> anyhow::Result<()> { - if comments.is_empty() { - return Ok(()); - } - let client = reqwest::Client::builder() .user_agent("ai-review-bot/0.1") .build()?; @@ -489,12 +485,14 @@ async fn upsert_inline_comments_at( 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(); 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/{}", @@ -517,38 +515,56 @@ async fn upsert_inline_comments_at( } } - if new_comments.is_empty() { - return Ok(()); + 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 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(()) } @@ -622,20 +638,33 @@ fn prepare_inline_comments( } 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!("") + hash } /// Update the PR description body. @@ -1102,4 +1131,61 @@ mod tests { .expect("upsert"); server.await.expect("server task"); } + + #[tokio::test] + async fn inline_upsert_deletes_stale_comments_when_no_findings_remain() { + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + use tokio::net::TcpListener; + + let marker = inline_comment_marker("team", "abc123", "src/lib.rs", 12); + let existing = serde_json::json!([{ + "id": 4242, + "body": format!("stale finding\n\n{marker}") + }]) + .to_string(); + let listener = TcpListener::bind("127.0.0.1:0").await.expect("bind"); + let address = listener.local_addr().expect("address"); + let server = tokio::spawn(async move { + for (index, body) in [existing, "{}".to_string()].into_iter().enumerate() { + let (mut socket, _) = listener.accept().await.expect("accept request"); + let mut request = vec![0_u8; 4096]; + let read = socket.read(&mut request).await.expect("read request"); + let request = String::from_utf8_lossy(&request[..read]); + if index == 0 { + assert!(request.starts_with( + "GET /repos/owner/repo/pulls/7/comments?per_page=100&page=1 " + )); + } else { + assert!(request.starts_with("DELETE /repos/owner/repo/pulls/comments/4242 ")); + } + let response = format!( + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", + body.len() + ); + socket + .write_all(response.as_bytes()) + .await + .expect("write response"); + } + }); + + let client = reqwest::Client::builder() + .user_agent("test") + .build() + .expect("client"); + upsert_inline_comments_at( + &client, + &format!("http://{address}"), + "token", + "owner", + "repo", + 7, + "abc123", + "team", + &[], + ) + .await + .expect("remove stale comment"); + server.await.expect("server task"); + } } diff --git a/tools/ai-review/src/main.rs b/tools/ai-review/src/main.rs index 155fbe0..317e5ef 100644 --- a/tools/ai-review/src/main.rs +++ b/tools/ai-review/src/main.rs @@ -112,29 +112,27 @@ async fn run_analysis( .into_iter() .filter(|finding| ctx.line_is_added_at(&finding.file, finding.line) == Some(true)) .collect(); - if !inline.is_empty() { - let head_sha = github::fetch_head_sha(&clients.octo, owner, repo, pr_number).await?; - let comments: Vec = inline - .into_iter() - .map(|f| InlineComment { - path: f.file.clone(), - line: f.line, - body: f.message.clone(), - }) - .collect(); - - println!("Posting {} inline comment(s)…", comments.len()); - github::upsert_inline_comments( - &clients.github_token, - owner, - repo, - pr_number, - &head_sha, - marker, - &comments, - ) - .await?; - } + let head_sha = github::fetch_head_sha(&clients.octo, owner, repo, pr_number).await?; + let comments: Vec = inline + .into_iter() + .map(|f| InlineComment { + path: f.file.clone(), + line: f.line, + body: f.message.clone(), + }) + .collect(); + + println!("Upserting {} inline comment(s)…", comments.len()); + github::upsert_inline_comments( + &clients.github_token, + owner, + repo, + pr_number, + &head_sha, + marker, + &comments, + ) + .await?; println!("{label} complete."); Ok(()) diff --git a/tools/ai-review/src/team.rs b/tools/ai-review/src/team.rs index 842a648..0cef114 100644 --- a/tools/ai-review/src/team.rs +++ b/tools/ai-review/src/team.rs @@ -441,11 +441,7 @@ async fn post_confirmed_inline( }) .collect(); - if inline.is_empty() { - return Ok(()); - } - - println!("Posting {} confirmed inline comment(s)…", inline.len()); + println!("Upserting {} confirmed inline comment(s)…", inline.len()); github::upsert_inline_comments( &clients.github_token, owner,