From 2337d8e30da740018a5d50022254ea7c1ff4c8ab Mon Sep 17 00:00:00 2001 From: Pierrick Fonquerne Date: Wed, 12 Aug 2026 19:06:52 +0200 Subject: [PATCH] Cache unchanged AI review verdicts --- .github/workflows/ai-review.yml | 2 + Cargo.lock | 73 +++++ README.md | 8 + tools/ai-review/Cargo.toml | 2 + tools/ai-review/src/github.rs | 86 +++++- tools/ai-review/src/main.rs | 1 + tools/ai-review/src/team.rs | 214 ++++++++++++++- tools/ai-review/src/team_cache.rs | 322 +++++++++++++++++++++++ tools/ai-review/src/types.rs | 4 +- tools/ai-review/tests/workflow_policy.rs | 2 + 10 files changed, 693 insertions(+), 21 deletions(-) create mode 100644 tools/ai-review/src/team_cache.rs diff --git a/.github/workflows/ai-review.yml b/.github/workflows/ai-review.yml index 6a9be03..bcee811 100644 --- a/.github/workflows/ai-review.yml +++ b/.github/workflows/ai-review.yml @@ -63,6 +63,8 @@ jobs: GITHUB_REPOSITORY: ${{ github.repository }} PR_NUMBER: ${{ inputs.pr-number }} AI_MODE: ${{ inputs.mode }} + AI_REVIEW_REVISION: ${{ job.workflow_sha }} + AI_REVIEW_COMMENT_AUTHOR: github-actions[bot] run: ./target/release/ai-review ai-review-skipped: diff --git a/Cargo.lock b/Cargo.lock index ef3d83b..77331dd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7,10 +7,12 @@ name = "ai-review" version = "0.1.0" dependencies = [ "anyhow", + "base64", "octocrab", "reqwest", "serde", "serde_json", + "sha2", "tokio", ] @@ -73,6 +75,15 @@ version = "2.12.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "84d7ced0ae9557296835c32bf1b1e02b44c746701f898460fb000d7eaa84f00a" +[[package]] +name = "block-buffer" +version = "0.10.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3078c7629b62d3f0439517fa394996acacc5cbc91c5a20d8c658e77abd503a71" +dependencies = [ + "generic-array", +] + [[package]] name = "bumpalo" version = "3.20.3" @@ -137,6 +148,25 @@ version = "0.8.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "773648b94d0e5d620f64f280777445740e61fe701025087ec8b57f45c791888b" +[[package]] +name = "cpufeatures" +version = "0.2.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "59ed5838eebb26a2bb2e58f6d5b5316989ae9d08bab10e0e6d103e656d1b0280" +dependencies = [ + "libc", +] + +[[package]] +name = "crypto-common" +version = "0.1.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "78c8292055d1c1df0cce5d180393dc8cce0abec0a7102adb6c7b1eef6016d60a" +dependencies = [ + "generic-array", + "typenum", +] + [[package]] name = "deranged" version = "0.5.8" @@ -146,6 +176,16 @@ dependencies = [ "powerfmt", ] +[[package]] +name = "digest" +version = "0.10.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9ed9a281f7bc9b7576e61468ba615a66a5c8cfdff42420a70aa82701a3b1e292" +dependencies = [ + "block-buffer", + "crypto-common", +] + [[package]] name = "displaydoc" version = "0.2.6" @@ -276,6 +316,16 @@ dependencies = [ "slab", ] +[[package]] +name = "generic-array" +version = "0.14.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "85649ca51fd72272d7821adaf274ad91c288277713d9c18820d8499a7ff69e9a" +dependencies = [ + "typenum", + "version_check", +] + [[package]] name = "getrandom" version = "0.2.17" @@ -1161,6 +1211,17 @@ dependencies = [ "serde", ] +[[package]] +name = "sha2" +version = "0.10.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a7507d819769d01a365ab707794a4084392c824f54a7a6a7862f8c3d0892b283" +dependencies = [ + "cfg-if", + "cpufeatures", + "digest", +] + [[package]] name = "shlex" version = "2.0.1" @@ -1499,6 +1560,12 @@ version = "0.2.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e421abadd41a4225275504ea4d6566923418b7f05506fbc9c0fe86ba7396114b" +[[package]] +name = "typenum" +version = "1.20.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b6f5e870be6c3b371b77fe0ee0bafb859fa4964b4404c27de1d380043c4dda20" + [[package]] name = "unicode-ident" version = "1.0.24" @@ -1530,6 +1597,12 @@ version = "1.0.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b6c140620e7ffbb22c2dee59cafe6084a59b5ffc27a8859a5f0d494b5d52b6be" +[[package]] +name = "version_check" +version = "0.9.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0b928f33d975fc6ad9f86c8f283853ad26bdd5b10b7f1542aa2fa15e2289105a" + [[package]] name = "want" version = "0.3.1" diff --git a/README.md b/README.md index c217b30..eea7f20 100644 --- a/README.md +++ b/README.md @@ -39,6 +39,14 @@ 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. +Successful `team` reviews also keep a versioned, size-bounded cache in an +invisible HTML marker owned by the workflow bot. An identical diff reviewed by +the same workflow revision returns before any Mistral call. When only part of +the diff changes, unchanged findings reuse their previous three-lens verdict +only if their exact identity and review context still match. Invalid cache data, +workflow changes, incomplete model responses, and changed context all fall back +to fresh verification. + ### Calling the reusable workflow Default per-PR setup, the multi-agent team review plus the PR description. diff --git a/tools/ai-review/Cargo.toml b/tools/ai-review/Cargo.toml index a05f445..a6e181a 100644 --- a/tools/ai-review/Cargo.toml +++ b/tools/ai-review/Cargo.toml @@ -17,6 +17,8 @@ octocrab = "0.44" serde = { version = "1", features = ["derive"] } serde_json = "1" anyhow = "1" +base64 = "0.22" +sha2 = "0.10" [lints.rust] unsafe_code = "forbid" diff --git a/tools/ai-review/src/github.rs b/tools/ai-review/src/github.rs index 7801626..2c57f73 100644 --- a/tools/ai-review/src/github.rs +++ b/tools/ai-review/src/github.rs @@ -387,12 +387,20 @@ pub async fn upsert_global_comment( .await .context("failed to fetch every PR comment page")?; - let existing_id = comments.iter().find_map(|c| { - c.body - .as_deref() - .filter(|b| has_bot_marker(b, marker)) - .map(|_| c.id) - }); + let trusted_author = std::env::var("AI_REVIEW_COMMENT_AUTHOR") + .ok() + .filter(|author| !author.trim().is_empty()); + let existing_id = comments + .iter() + .find(|comment| { + is_trusted_global_comment( + &comment.user.login, + comment.body.as_deref(), + marker, + trusted_author.as_deref(), + ) + }) + .map(|comment| comment.id); if let Some(comment_id) = existing_id { octo.issues(owner, repo) @@ -409,6 +417,49 @@ pub async fn upsert_global_comment( Ok(()) } +/// Returns the existing global bot comment containing `marker`, if any. +pub async fn fetch_global_comment( + octo: &Octocrab, + owner: &str, + repo: &str, + pr_number: u64, + marker: &str, + author_login: &str, +) -> anyhow::Result> { + 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")?; + Ok(comments + .into_iter() + .find(|comment| { + is_trusted_global_comment( + &comment.user.login, + comment.body.as_deref(), + marker, + Some(author_login), + ) + }) + .and_then(|comment| comment.body)) +} + +fn is_trusted_global_comment( + comment_author: &str, + body: Option<&str>, + marker: &str, + trusted_author: Option<&str>, +) -> bool { + trusted_author.is_none_or(|author| comment_author == author) + && body.is_some_and(|value| has_bot_marker(value, marker)) +} + #[derive(serde::Serialize)] struct ReviewRequest<'a> { commit_id: &'a str, @@ -995,6 +1046,29 @@ mod tests { assert_eq!(ctx.lens_context(&finding), ctx.patch_for(&finding)); } + #[test] + fn global_comment_markers_are_accepted_only_from_the_trusted_author() { + let body = Some("review\n"); + assert!(is_trusted_global_comment( + "github-actions[bot]", + body, + "", + Some("github-actions[bot]") + )); + assert!(!is_trusted_global_comment( + "pull-request-author", + body, + "", + Some("github-actions[bot]") + )); + assert!(!is_trusted_global_comment( + "github-actions[bot]", + Some("ordinary comment"), + "", + Some("github-actions[bot]") + )); + } + #[test] fn prepares_one_stable_inline_comment_per_location() { let comments = vec![ diff --git a/tools/ai-review/src/main.rs b/tools/ai-review/src/main.rs index 317e5ef..6751242 100644 --- a/tools/ai-review/src/main.rs +++ b/tools/ai-review/src/main.rs @@ -3,6 +3,7 @@ mod github; mod mistral; mod review; mod team; +mod team_cache; mod text; mod types; diff --git a/tools/ai-review/src/team.rs b/tools/ai-review/src/team.rs index 0cef114..d9ffca7 100644 --- a/tools/ai-review/src/team.rs +++ b/tools/ai-review/src/team.rs @@ -11,7 +11,7 @@ use crate::types::{ Agent, CoverageGap, CoverageGapKind, FindingVerdict, Lens, LensVerdict, Severity, SynthFinding, SynthReport, Verdict, }; -use crate::{github, mistral, review, Clients}; +use crate::{github, mistral, review, team_cache, Clients}; /// Maximum number of synthesised findings put through adversarial verification. const MAX_VERIFIED_FINDINGS: usize = 15; @@ -48,6 +48,12 @@ pub async fn run_team( return Ok(()); } + let cache_state = load_team_cache(clients, owner, repo, pr_number, &ctx).await?; + if cache_state.matches_run() { + println!("Identical diff and reviewer revision: keeping the cached team review."); + return Ok(()); + } + let (batch_runs, agents_ok, agents_failed, agent_gaps) = run_batch_plan(clients, &ctx.batches).await; ctx.coverage_gaps.extend(agent_gaps); @@ -96,7 +102,10 @@ pub async fn run_team( "Verifying {} finding(s) with the 3-lens vote…", findings.len() ); - let verdicts = verify_findings(clients, &ctx, &findings).await; + let VerificationResults { + verdicts, + cacheable, + } = verify_findings(clients, &ctx, &findings, cache_state.reusable()).await; let scored: Vec<(SynthFinding, FindingVerdict)> = findings.into_iter().zip(verdicts).collect(); let verdict = compute_verdict(&scored, capped > 0 || !ctx.coverage_gaps.is_empty()); @@ -121,7 +130,8 @@ pub async fn run_team( model: &model, coverage_gaps: &ctx.coverage_gaps, }; - let body = review::render_team_comment(&view); + let mut body = review::render_team_comment(&view); + append_team_cache(&mut body, &cache_state, &ctx, &scored, cacheable)?; println!("Upserting team comment…"); github::upsert_global_comment(&clients.octo, owner, repo, pr_number, &body, MARKER).await?; @@ -132,6 +142,94 @@ pub async fn run_team( Ok(()) } +struct TeamCacheState { + reviewer_revision: Option, + trusted_author_configured: bool, + diff_hash: String, + previous: Option, +} + +impl TeamCacheState { + fn matches_run(&self) -> bool { + match (&self.reviewer_revision, &self.previous) { + (Some(revision), Some(cache)) => cache.matches_run(revision, &self.diff_hash), + _ => false, + } + } + + fn reusable(&self) -> Option<&team_cache::ReviewCache> { + match (&self.reviewer_revision, &self.previous) { + (Some(revision), Some(cache)) if cache.supports_revision(revision) => Some(cache), + _ => None, + } + } +} + +async fn load_team_cache( + clients: &Clients, + owner: &str, + repo: &str, + pr_number: u64, + ctx: &github::DiffContext, +) -> anyhow::Result { + let reviewer_revision = std::env::var("AI_REVIEW_REVISION") + .ok() + .filter(|revision| !revision.trim().is_empty()); + let comment_author = std::env::var("AI_REVIEW_COMMENT_AUTHOR") + .ok() + .filter(|author| !author.trim().is_empty()); + let existing_comment = if let Some(author) = comment_author.as_deref() { + github::fetch_global_comment(&clients.octo, owner, repo, pr_number, MARKER, author).await? + } else { + None + }; + let previous = existing_comment.as_deref().and_then(|body| { + match team_cache::ReviewCache::from_comment(body) { + Ok(cache) => cache, + Err(error) => { + eprintln!("warning: ignoring invalid team cache: {error}"); + None + } + } + }); + Ok(TeamCacheState { + reviewer_revision, + trusted_author_configured: comment_author.is_some(), + diff_hash: team_cache::diff_hash(ctx), + previous, + }) +} + +fn append_team_cache( + body: &mut String, + state: &TeamCacheState, + ctx: &github::DiffContext, + scored: &[(SynthFinding, FindingVerdict)], + cacheable: Vec, +) -> anyhow::Result<()> { + let complete = !has_transient_coverage_gap(ctx) && cacheable.iter().all(|value| *value); + if let Some(revision) = state + .reviewer_revision + .as_deref() + .filter(|_| state.trusted_author_configured && complete) + { + let mut cache = team_cache::ReviewCache::new(revision, state.diff_hash.clone()); + for ((finding, finding_verdict), is_cacheable) in scored.iter().zip(cacheable) { + if is_cacheable { + cache.record(ctx, finding, finding_verdict); + } + } + *body = team_cache::append_marker(body, &cache.encode_bounded()?); + } else if state.reviewer_revision.is_some() && state.trusted_author_configured { + eprintln!("warning: incomplete verification detected; team cache was not updated"); + } else { + eprintln!( + "warning: AI_REVIEW_REVISION or AI_REVIEW_COMMENT_AUTHOR is unset; team caching is disabled" + ); + } + Ok(()) +} + type AgentReports = Vec<(Agent, crate::types::ReviewResponse)>; struct BatchRun { @@ -355,18 +453,59 @@ fn prefilter_verdict(ctx: &github::DiffContext, finding: &SynthFinding) -> Optio /// Verifies each finding with the 3-lens adversarial vote under a concurrency /// bound, returning one [`FindingVerdict`] per finding (index-aligned). /// Findings contested by the deterministic prefilter skip the vote. +struct VerificationResults { + verdicts: Vec, + cacheable: Vec, +} + +struct PrefilledVerdicts { + verdicts: Vec>, + deterministic_count: usize, + cache_hit_count: usize, +} + +fn prefill_verdicts( + ctx: &github::DiffContext, + findings: &[SynthFinding], + previous_cache: Option<&team_cache::ReviewCache>, +) -> PrefilledVerdicts { + let mut verdicts = Vec::with_capacity(findings.len()); + let mut deterministic_count = 0; + let mut cache_hit_count = 0; + for finding in findings { + if let Some(verdict) = prefilter_verdict(ctx, finding) { + deterministic_count += 1; + verdicts.push(Some(verdict)); + } else if let Some(verdict) = previous_cache.and_then(|cache| cache.lookup(ctx, finding)) { + cache_hit_count += 1; + verdicts.push(Some(verdict)); + } else { + verdicts.push(None); + } + } + PrefilledVerdicts { + verdicts, + deterministic_count, + cache_hit_count, + } +} + async fn verify_findings( clients: &Clients, ctx: &github::DiffContext, findings: &[SynthFinding], -) -> Vec { - let prefilled: Vec> = findings - .iter() - .map(|finding| prefilter_verdict(ctx, finding)) - .collect(); - let skipped = prefilled.iter().filter(|slot| slot.is_some()).count(); - if skipped > 0 { - println!("Prefiltered {skipped} finding(s) outside the diff (no lens calls)."); + previous_cache: Option<&team_cache::ReviewCache>, +) -> VerificationResults { + let PrefilledVerdicts { + verdicts: prefilled, + deterministic_count, + cache_hit_count, + } = prefill_verdicts(ctx, findings, previous_cache); + if deterministic_count > 0 { + println!("Prefiltered {deterministic_count} finding(s) outside the diff (no lens calls)."); + } + if cache_hit_count > 0 { + println!("Reused {cache_hit_count} cached finding verdict(s) (no lens calls)."); } let semaphore = Arc::new(Semaphore::new(MAX_CONCURRENT_CALLS)); @@ -406,11 +545,29 @@ async fn verify_findings( } } - prefilled + let cacheable = prefilled + .iter() + .enumerate() + .map(|(idx, pre)| pre.is_some() || votes[idx].len() >= MIN_CONFIRM_VOTES) + .collect(); + let verdicts = prefilled .into_iter() .enumerate() .map(|(idx, pre)| pre.unwrap_or_else(|| aggregate_lens_votes(&votes[idx]))) - .collect() + .collect(); + VerificationResults { + verdicts, + cacheable, + } +} + +fn has_transient_coverage_gap(ctx: &github::DiffContext) -> bool { + ctx.coverage_gaps.iter().any(|gap| { + matches!( + gap.kind, + CoverageGapKind::AgentFailed | CoverageGapKind::SynthesisFailed + ) + }) } /// Posts inline comments for confirmed, critical, line-located findings. @@ -717,4 +874,35 @@ mod tests { assert!(prefilter_verdict(&ctx, &finding_in_file("a.rs", 0)).is_none()); assert!(prefilter_verdict(&ctx, &finding_in_file("other.rs", 50)).is_none()); } + + #[test] + fn unchanged_finding_context_is_prefilled_from_cache() { + let ctx = diff_ctx("a.rs", ONE_HUNK_PATCH); + let finding = finding_in_file("a.rs", 0); + let cached_verdict = verdict(false); + let mut cache = team_cache::ReviewCache::new("revision", team_cache::diff_hash(&ctx)); + cache.record(&ctx, &finding, &cached_verdict); + + let hit = prefill_verdicts(&ctx, std::slice::from_ref(&finding), Some(&cache)); + assert_eq!(hit.cache_hit_count, 1); + assert_eq!(hit.deterministic_count, 0); + assert_eq!(hit.verdicts, vec![Some(cached_verdict)]); + + let changed = diff_ctx("a.rs", "@@ -1,2 +1,3 @@\n fn main() {\n+ changed();\n }"); + let miss = prefill_verdicts(&changed, &[finding], Some(&cache)); + assert_eq!(miss.cache_hit_count, 0); + assert_eq!(miss.verdicts, vec![None]); + } + + #[test] + fn transient_model_gaps_prevent_whole_run_caching() { + let mut ctx = diff_ctx("a.rs", ONE_HUNK_PATCH); + assert!(!has_transient_coverage_gap(&ctx)); + ctx.coverage_gaps.push(CoverageGap { + kind: CoverageGapKind::AgentFailed, + file: "a.rs".to_string(), + detail: "temporary failure".to_string(), + }); + assert!(has_transient_coverage_gap(&ctx)); + } } diff --git a/tools/ai-review/src/team_cache.rs b/tools/ai-review/src/team_cache.rs new file mode 100644 index 0000000..c0a41fc --- /dev/null +++ b/tools/ai-review/src/team_cache.rs @@ -0,0 +1,322 @@ +use base64::engine::general_purpose::URL_SAFE_NO_PAD; +use base64::Engine as _; +use serde::{Deserialize, Serialize}; +use sha2::{Digest, Sha256}; +use std::fmt::Write as _; + +use crate::github::DiffContext; +use crate::types::{FindingVerdict, PatchAvailability, Severity, SynthFinding}; + +const CACHE_SCHEMA: u8 = 1; +const CACHE_PREFIX: &str = ""; +/// Leaves ample room below GitHub's comment limit for the rendered report. +pub const MAX_CACHE_MARKER_BYTES: usize = 16_000; + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +pub struct ReviewCache { + schema: u8, + reviewer_revision: String, + diff_hash: String, + entries: Vec, +} + +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +struct CachedVerdict { + finding_hash: String, + context_hash: String, + verdict: FindingVerdict, +} + +impl ReviewCache { + #[must_use] + pub fn new(reviewer_revision: &str, diff_hash: String) -> Self { + Self { + schema: CACHE_SCHEMA, + reviewer_revision: reviewer_revision.to_string(), + diff_hash, + entries: Vec::new(), + } + } + + #[must_use] + pub fn matches_run(&self, reviewer_revision: &str, diff_hash: &str) -> bool { + self.schema == CACHE_SCHEMA + && self.reviewer_revision == reviewer_revision + && self.diff_hash == diff_hash + } + + #[must_use] + pub fn supports_revision(&self, reviewer_revision: &str) -> bool { + self.schema == CACHE_SCHEMA && self.reviewer_revision == reviewer_revision + } + + #[must_use] + pub fn lookup(&self, ctx: &DiffContext, finding: &SynthFinding) -> Option { + let finding_hash = finding_hash(finding); + let context_hash = context_hash(ctx, finding); + self.entries + .iter() + .find(|entry| entry.finding_hash == finding_hash && entry.context_hash == context_hash) + .map(|entry| entry.verdict.clone()) + } + + pub fn record(&mut self, ctx: &DiffContext, finding: &SynthFinding, verdict: &FindingVerdict) { + self.entries.push(CachedVerdict { + finding_hash: finding_hash(finding), + context_hash: context_hash(ctx, finding), + verdict: verdict.clone(), + }); + } + + pub fn encode_bounded(&self) -> anyhow::Result { + let mut bounded = self.clone(); + loop { + let json = serde_json::to_vec(&bounded)?; + let marker = format!( + "{CACHE_PREFIX}{}{CACHE_SUFFIX}", + URL_SAFE_NO_PAD.encode(json) + ); + if marker.len() <= MAX_CACHE_MARKER_BYTES { + return Ok(marker); + } + anyhow::ensure!( + !bounded.entries.is_empty(), + "team cache metadata exceeds the marker size limit" + ); + bounded.entries.pop(); + } + } + + pub fn from_comment(body: &str) -> anyhow::Result> { + let Some(start) = body.find(CACHE_PREFIX) else { + return Ok(None); + }; + let encoded_start = start + CACHE_PREFIX.len(); + let suffix_offset = body[encoded_start..] + .find(CACHE_SUFFIX) + .ok_or_else(|| anyhow::anyhow!("team cache marker has no closing delimiter"))?; + let encoded = &body[encoded_start..encoded_start + suffix_offset]; + let bytes = URL_SAFE_NO_PAD + .decode(encoded) + .map_err(|error| anyhow::anyhow!("invalid team cache encoding: {error}"))?; + let cache: Self = serde_json::from_slice(&bytes) + .map_err(|error| anyhow::anyhow!("invalid team cache payload: {error}"))?; + if cache.schema != CACHE_SCHEMA { + anyhow::bail!("unsupported team cache schema: {}", cache.schema); + } + Ok(Some(cache)) + } +} + +#[must_use] +pub fn append_marker(body: &str, marker: &str) -> String { + format!("{body}\n\n{marker}") +} + +#[must_use] +pub fn diff_hash(ctx: &DiffContext) -> String { + let mut hasher = Sha256::new(); + hash_part(&mut hasher, b"ai-team-diff-v1"); + hash_part(&mut hasher, &ctx.file_count.to_le_bytes()); + for file in &ctx.files { + hash_part(&mut hasher, file.path.as_bytes()); + hash_part(&mut hasher, format!("{:?}", file.status).as_bytes()); + hash_part( + &mut hasher, + file.previous_path.as_deref().unwrap_or_default().as_bytes(), + ); + hash_part(&mut hasher, &file.additions.to_le_bytes()); + hash_part(&mut hasher, &file.deletions.to_le_bytes()); + match &file.patch { + PatchAvailability::Present(patch) => { + hash_part(&mut hasher, b"present"); + hash_part(&mut hasher, patch.as_bytes()); + } + PatchAvailability::Missing => hash_part(&mut hasher, b"missing"), + } + } + for gap in &ctx.coverage_gaps { + hash_part(&mut hasher, format!("{:?}", gap.kind).as_bytes()); + hash_part(&mut hasher, gap.file.as_bytes()); + hash_part(&mut hasher, gap.detail.as_bytes()); + } + hex_digest(hasher) +} + +fn finding_hash(finding: &SynthFinding) -> String { + let mut hasher = Sha256::new(); + hash_part(&mut hasher, b"ai-team-finding-v1"); + hash_part(&mut hasher, finding.file.as_bytes()); + hash_part(&mut hasher, &finding.line.to_le_bytes()); + let severity = match finding.severity { + Severity::Critical => b"critical".as_slice(), + Severity::Minor => b"minor".as_slice(), + }; + hash_part(&mut hasher, severity); + hash_part(&mut hasher, finding.category.label().as_bytes()); + hash_part(&mut hasher, finding.message.trim().as_bytes()); + hex_digest(hasher) +} + +fn context_hash(ctx: &DiffContext, finding: &SynthFinding) -> String { + let mut hasher = Sha256::new(); + hash_part(&mut hasher, b"ai-team-context-v1"); + hash_part(&mut hasher, ctx.lens_context(finding).as_bytes()); + hex_digest(hasher) +} + +fn hash_part(hasher: &mut Sha256, value: &[u8]) { + hasher.update(value.len().to_le_bytes()); + hasher.update(value); +} + +fn hex_digest(hasher: Sha256) -> String { + let digest = hasher.finalize(); + let mut output = String::with_capacity(digest.len() * 2); + for byte in digest { + let _ = write!(output, "{byte:02x}"); + } + output +} + +#[cfg(test)] +mod tests { + use std::collections::{BTreeSet, HashMap}; + + use super::*; + use crate::types::{ + Category, ChangedFile, ChangedFileStatus, CoverageGap, CoverageGapKind, PatchAvailability, + }; + + fn finding(message: &str) -> SynthFinding { + SynthFinding { + file: "src/lib.rs".to_string(), + line: 2, + severity: Severity::Critical, + category: Category::Bug, + message: message.to_string(), + message_fr: String::new(), + sources: vec!["correctness".to_string()], + } + } + + fn context(patch: &str) -> DiffContext { + let file = ChangedFile { + path: "src/lib.rs".to_string(), + status: ChangedFileStatus::Modified, + previous_path: None, + additions: 1, + deletions: 0, + patch: PatchAvailability::Present(patch.to_string()), + added_lines: BTreeSet::from([2]), + }; + DiffContext { + full: format!("--- src/lib.rs\n{patch}\n"), + by_file: HashMap::from([("src/lib.rs".to_string(), patch.to_string())]), + head_files: HashMap::new(), + file_count: 1, + files: vec![file], + batches: vec![], + coverage_gaps: vec![], + } + } + + fn verdict(reason: &str) -> FindingVerdict { + FindingVerdict { + contested: true, + reasons: vec![reason.to_string()], + reasons_fr: vec![], + } + } + + #[test] + fn round_trips_an_invisible_bounded_cache() { + let ctx = context("@@ -1 +1,2 @@\n context\n+added"); + let mut cache = ReviewCache::new("revision", diff_hash(&ctx)); + cache.record(&ctx, &finding("panic"), &verdict("not reachable")); + let marker = cache.encode_bounded().expect("encode"); + assert!(marker.starts_with(CACHE_PREFIX)); + assert!(marker.ends_with(CACHE_SUFFIX)); + assert!(marker.len() <= MAX_CACHE_MARKER_BYTES); + let decoded = ReviewCache::from_comment(&format!("report\n{marker}")) + .expect("decode") + .expect("cache"); + assert_eq!(decoded, cache); + } + + #[test] + fn invalidates_revision_diff_context_and_finding_changes() { + let first = context("@@ -1 +1,2 @@\n context\n+added"); + let changed = context("@@ -1 +1,2 @@\n context\n+different"); + let original = finding("panic"); + let mut cache = ReviewCache::new("revision", diff_hash(&first)); + cache.record(&first, &original, &verdict("cached")); + + assert!(cache.matches_run("revision", &diff_hash(&first))); + assert!(!cache.matches_run("other", &diff_hash(&first))); + assert!(!cache.matches_run("revision", &diff_hash(&changed))); + assert!(cache.lookup(&first, &original).is_some()); + assert!(cache.lookup(&changed, &original).is_none()); + assert!(cache + .lookup(&first, &finding("different finding")) + .is_none()); + } + + #[test] + fn corrupted_or_unsupported_payloads_fail_without_panicking() { + assert!(ReviewCache::from_comment("plain report") + .expect("absent") + .is_none()); + assert!(ReviewCache::from_comment("").is_err()); + + let unsupported = serde_json::json!({ + "schema": 99, + "reviewer_revision": "revision", + "diff_hash": "hash", + "entries": [] + }); + let marker = format!( + "{CACHE_PREFIX}{}{CACHE_SUFFIX}", + URL_SAFE_NO_PAD.encode(serde_json::to_vec(&unsupported).expect("JSON")) + ); + assert!(ReviewCache::from_comment(&marker).is_err()); + } + + #[test] + fn oversized_entries_are_dropped_until_the_marker_fits() { + let ctx = context("@@ -1 +1,2 @@\n context\n+added"); + let mut cache = ReviewCache::new("revision", diff_hash(&ctx)); + for index in 0..100 { + let mut item = finding(&format!("finding {index}")); + item.line = index + 1; + cache.record(&ctx, &item, &verdict(&"x".repeat(1_000))); + } + let marker = cache.encode_bounded().expect("bounded encode"); + assert!(marker.len() <= MAX_CACHE_MARKER_BYTES); + let decoded = ReviewCache::from_comment(&marker) + .expect("decode") + .expect("cache"); + assert!(decoded.entries.len() < cache.entries.len()); + assert!(decoded.matches_run("revision", &diff_hash(&ctx))); + } + + #[test] + fn oversized_metadata_is_rejected_instead_of_exceeding_the_limit() { + let ctx = context("@@ -1 +1,2 @@\n context\n+added"); + let cache = ReviewCache::new(&"x".repeat(MAX_CACHE_MARKER_BYTES), diff_hash(&ctx)); + assert!(cache.encode_bounded().is_err()); + } + + #[test] + fn diff_hash_includes_missing_input_accounting() { + let mut ctx = context("@@ -1 +1,2 @@\n context\n+added"); + let clean = diff_hash(&ctx); + ctx.coverage_gaps.push(CoverageGap { + kind: CoverageGapKind::PatchUnavailable, + file: "asset.bin".to_string(), + detail: "missing patch".to_string(), + }); + assert_ne!(clean, diff_hash(&ctx)); + } +} diff --git a/tools/ai-review/src/types.rs b/tools/ai-review/src/types.rs index b19bbf4..958b8f7 100644 --- a/tools/ai-review/src/types.rs +++ b/tools/ai-review/src/types.rs @@ -1,6 +1,6 @@ use std::collections::BTreeSet; -use serde::Deserialize; +use serde::{Deserialize, Serialize}; /// Normalised status of a file changed by a pull request. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -253,7 +253,7 @@ pub struct LensVerdict { } /// Aggregated verdict for one finding after the multi-lens vote. -#[derive(Debug, Clone)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] pub struct FindingVerdict { pub contested: bool, pub reasons: Vec, diff --git a/tools/ai-review/tests/workflow_policy.rs b/tools/ai-review/tests/workflow_policy.rs index de25482..c747c6e 100644 --- a/tools/ai-review/tests/workflow_policy.rs +++ b/tools/ai-review/tests/workflow_policy.rs @@ -46,6 +46,8 @@ fn reusable_workflow_checks_out_its_own_revision() { assert!(workflow.contains("ref: ${{ job.workflow_sha }}")); assert!(!workflow.contains("repository: nubster-opensources/.github")); assert!(!workflow.contains("ref: main")); + assert!(workflow.contains("AI_REVIEW_REVISION: ${{ job.workflow_sha }}")); + assert!(workflow.contains("AI_REVIEW_COMMENT_AUTHOR: github-actions[bot]")); } #[test]