diff --git a/src/forge/github.rs b/src/forge/github.rs index 6497242..8c8743d 100644 --- a/src/forge/github.rs +++ b/src/forge/github.rs @@ -832,6 +832,64 @@ impl GitHub { .context("GitHub repository file is not valid UTF-8") } + pub(crate) async fn fetch_repository_file_with_base_fallback( + &self, + head_revision: &str, + base_revision: Option<&str>, + path: &str, + ) -> Result { + if let Some(content) = self + .fetch_repository_file_if_present(head_revision, path) + .await? + { + return Ok(content); + } + let base_revision = base_revision.context(format!( + "GitHub repository file is absent at head {head_revision} and no base SHA is available" + ))?; + self.fetch_repository_file_at_revision(base_revision, path) + .await + .with_context(|| { + format!( + "GitHub repository file is absent at head {head_revision} and the base fetch at {base_revision} failed" + ) + }) + } + + async fn fetch_repository_file_if_present( + &self, + revision: &str, + path: &str, + ) -> Result> { + ensure!( + super::valid_repository_path(path), + "GitHub returned an unsafe repository path" + ); + let mut url = reqwest::Url::parse(&self.url(&format!("/contents/{}", encode_path(path)))) + .context("building GitHub contents URL")?; + url.query_pairs_mut().append_pair("ref", revision); + let response = self + .send_retryable( + self.request(reqwest::Method::GET, url.to_string()) + .header("Accept", "application/vnd.github.raw+json"), + "repository file fetch", + ) + .await?; + if response.status() == reqwest::StatusCode::NOT_FOUND { + return Ok(None); + } + let snapshot = super::response_snapshot_in( + Self::check_ok(response, "repository file fetch").await?, + "GitHub repository file", + WorkspaceBudget::new(), + None, + ) + .await?; + String::from_utf8(snapshot.as_bytes().to_vec()) + .context("GitHub repository file is not valid UTF-8") + .map(Some) + } + async fn build_complete_diff( &self, files: Vec, @@ -2193,6 +2251,67 @@ mod tests { assert!(error.to_string().contains("not valid UTF-8")); } + #[tokio::test] + async fn fetch_repository_file_with_base_fallback_reads_deleted_head_files_from_base() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/repos/owner/repo/contents/config/review.toml")) + .and(query_param("ref", "head123")) + .respond_with(ResponseTemplate::new(404)) + .expect(1) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/owner/repo/contents/config/review.toml")) + .and(query_param("ref", "base123")) + .respond_with(ResponseTemplate::new(200).set_body_string("enabled = true\n")) + .expect(1) + .mount(&server) + .await; + + let github = test_github(&server); + let content = github + .fetch_repository_file_with_base_fallback( + "head123", + Some("base123"), + "config/review.toml", + ) + .await + .unwrap(); + + assert_eq!(content, "enabled = true\n"); + } + + #[tokio::test] + async fn fetch_repository_file_with_base_fallback_does_not_mask_head_failures() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/repos/owner/repo/contents/config/review.toml")) + .and(query_param("ref", "head123")) + .respond_with(ResponseTemplate::new(500)) + .expect(3) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/owner/repo/contents/config/review.toml")) + .and(query_param("ref", "base123")) + .respond_with(ResponseTemplate::new(200).set_body_string("stale = true\n")) + .expect(0) + .mount(&server) + .await; + + let error = test_github(&server) + .fetch_repository_file_with_base_fallback( + "head123", + Some("base123"), + "config/review.toml", + ) + .await + .unwrap_err(); + + assert!(error.to_string().contains("500")); + } + async fn mount_current_delivery_snapshot(server: &MockServer) { Mock::given(method("GET")) .and(path("/repos/owner/repo/pulls/1")) diff --git a/src/resolve.rs b/src/resolve.rs index 4e2e4fe..c92832f 100644 --- a/src/resolve.rs +++ b/src/resolve.rs @@ -9,7 +9,7 @@ use serde_json::json; use crate::config::Config; use crate::diff; use crate::envelope::{ - Finding, Kind, ModelIncident, ModelUsage, SuppressedFinding, SuppressionReason, Usage, + Finding, Kind, ModelIncident, ModelUsage, Severity, SuppressedFinding, SuppressionReason, Usage, }; use crate::forge::{github::GitHub, valid_repository_path}; use crate::llm::{LlmClient, UncertaintyResolution, UncertaintyResolutionReview, add_usage}; @@ -29,6 +29,11 @@ pub(crate) enum RepositorySource<'a> { Unavailable, } +pub(crate) struct ResolutionRevisions<'a> { + pub(crate) head: Option<&'a str>, + pub(crate) base: Option<&'a str>, +} + #[derive(Default)] pub(crate) struct ResolutionPass { pub suppressed_findings: Vec, @@ -54,7 +59,7 @@ pub(crate) async fn resolve_uncertainties( cfg: &Config, client: &LlmClient, source: &RepositorySource<'_>, - revision: Option<&str>, + revisions: ResolutionRevisions<'_>, finding_contexts: &[String], diff_text: &str, findings: &mut Vec, @@ -77,24 +82,31 @@ pub(crate) async fn resolve_uncertainties( .filter_map(|(index, finding)| (finding.kind == Kind::Uncertainty).then_some(index)) .take(MAX_FINDINGS) .collect::>(); + for (index, finding) in findings.iter_mut().enumerate() { + if finding.kind == Kind::Uncertainty && !eligible.contains(&index) { + demote_unresolved_uncertainty(finding); + } + } let mut confirmed = 0usize; let mut unresolved = uncertainty_count.saturating_sub(MAX_FINDINGS); let mut refuted = Vec::new(); for index in eligible { let original = findings[index].clone(); - let files = match fetch_referenced_files(source, revision, &original.body).await { - Ok(files) if !files.is_empty() => files, - Ok(_) => { - unresolved += 1; - continue; - } + let files = match fetch_referenced_files( + source, + revisions.head, + revisions.base, + &original.body, + ) + .await + { + Ok(files) => files, Err(error) => { eprintln!( - "postil: uncertainty resolution kept the original finding after repository file acquisition failed: {error:#}" + "postil: uncertainty resolution is continuing with diff evidence after repository file acquisition failed: {error:#}" ); - unresolved += 1; - continue; + Vec::new() } }; let diff_hunk = finding_contexts @@ -141,7 +153,10 @@ pub(crate) async fn resolve_uncertainties( }; match resolution_disposition(resolution.as_ref(), &files, diff_text) { - Disposition::KeepOriginal => unresolved += 1, + Disposition::KeepOriginal => { + demote_unresolved_uncertainty(&mut findings[index]); + unresolved += 1; + } Disposition::KeepConfirmed(body) => { findings[index].body = body; confirmed += 1; @@ -191,6 +206,15 @@ fn resolution_disposition( } } +fn demote_unresolved_uncertainty(finding: &mut Finding) { + if finding.kind == Kind::Uncertainty + && !crate::envelope::is_reserved_anchor(&finding.path) + && finding.severity == Severity::Error + { + finding.severity = Severity::Warn; + } +} + fn evidence_is_grounded(evidence: &str, files: &[ReferencedFile], diff_text: &str) -> bool { if evidence.is_empty() { return false; @@ -211,7 +235,8 @@ fn byte_contains(haystack: &[u8], needle: &[u8]) -> bool { async fn fetch_referenced_files( source: &RepositorySource<'_>, - revision: Option<&str>, + head_revision: Option<&str>, + base_revision: Option<&str>, body: &str, ) -> Result> { let mut files = Vec::new(); @@ -220,7 +245,7 @@ async fn fetch_referenced_files( if files.len() == MAX_FILES_PER_FINDING || total == MAX_TOTAL_FILE_BYTES { break; } - let Some(content) = fetch_file(source, revision, &path).await? else { + let Some(content) = fetch_file(source, head_revision, base_revision, &path).await? else { continue; }; let limit = MAX_FILE_BYTES.min(MAX_TOTAL_FILE_BYTES - total); @@ -237,15 +262,17 @@ async fn fetch_referenced_files( async fn fetch_file( source: &RepositorySource<'_>, - revision: Option<&str>, + head_revision: Option<&str>, + base_revision: Option<&str>, path: &str, ) -> Result> { match source { RepositorySource::Local(root) => read_local_file(root, path), RepositorySource::GitHub(github) => { - let revision = revision.context("GitHub uncertainty resolution requires a head SHA")?; + let head_revision = + head_revision.context("GitHub uncertainty resolution requires a head SHA")?; github - .fetch_repository_file_at_revision(revision, path) + .fetch_repository_file_with_base_fallback(head_revision, base_revision, path) .await .map(Some) } @@ -385,6 +412,7 @@ mod tests { let files = fetch_referenced_files( &RepositorySource::Local(directory.path()), None, + None, "Inspect `src/a.rs`, `src/missing.rs`, `src/b.rs`, `src/c.rs`, and `src/d.rs`.", ) .await diff --git a/src/review.rs b/src/review.rs index 4049e98..dcfbded 100644 --- a/src/review.rs +++ b/src/review.rs @@ -1925,7 +1925,10 @@ async fn review_diff(cfg: &Config, args: &ReviewArgs, input: ReviewInput<'_>) -> cfg, &client, &repository_source, - head_sha.as_deref(), + crate::resolve::ResolutionRevisions { + head: head_sha.as_deref(), + base: meta.map(|metadata| metadata.base_sha.as_str()), + }, &finding_contexts, diff_snapshot.as_str(), &mut kept, diff --git a/tests/e2e.rs b/tests/e2e.rs index b441659..059c83b 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -5001,10 +5001,14 @@ async fn large_confidence_disagreement_escalates_to_uncertainty_with_default_gat } fn uncertainty_finding(body: &str) -> Value { + uncertainty_finding_with_severity(body, "warn") +} + +fn uncertainty_finding_with_severity(body: &str, severity: &str) -> Value { json!({ "path": "src/auth.rs", "line": 41, - "severity": "warn", + "severity": severity, "kind": "uncertainty", "confidence": 0.9, "title": "Verify the repository-wide caller contract", @@ -5236,6 +5240,88 @@ async fn uncertainty_resolution_defaults_on_and_fails_open_when_unresolved() { assert_eq!(envelope["counts"]["suppressed"], 0); } +#[tokio::test] +async fn uncertainty_resolution_uses_diff_when_no_referenced_files_exist() { + let server = MockServer::start().await; + let original_body = "The added call may pass the wrong value to the query executor."; + let revised_body = "The added call passes the request token directly to the query executor."; + mock_review_model( + &server, + "generator-model", + json!([uncertainty_finding(original_body)]), + ) + .await; + mock_uncertainty_resolution( + &server, + "generator-model", + &json!({ + "resolution": "confirmed", + "revisedBody": revised_body, + "evidence": "exec_query(&token);" + }) + .to_string(), + 1, + ) + .await; + + let directory = tempfile::tempdir().unwrap(); + enable_uncertainty_resolution(directory.path()); + let diff = write_diff(directory.path()); + let output = postil() + .current_dir(directory.path()) + .env("POSTIL_API_BASE", server.uri()) + .env("REVIEW_MODEL", "generator-model") + .env("POSTIL_DISABLE_SCORER", "1") + .args(["review", "--diff-file"]) + .arg(&diff) + .args(["--output", "json"]) + .assert() + .success(); + + let envelope: Value = serde_json::from_slice(&output.get_output().stdout).unwrap(); + assert_eq!(envelope["findings"][0]["body"], revised_body); + assert_eq!(envelope["findings"][0]["severity"], "warn"); + assert_eq!(envelope["counts"]["suppressed"], 0); +} + +#[tokio::test] +async fn unresolved_error_uncertainty_is_retained_but_demoted_to_warn() { + let server = MockServer::start().await; + let original_body = "The added call may pass the wrong value to the query executor."; + mock_review_model( + &server, + "generator-model", + json!([uncertainty_finding_with_severity(original_body, "error")]), + ) + .await; + mock_uncertainty_resolution( + &server, + "generator-model", + r#"{"resolution":"unresolved","revisedBody":"","evidence":""}"#, + 1, + ) + .await; + + let directory = tempfile::tempdir().unwrap(); + enable_uncertainty_resolution(directory.path()); + let diff = write_diff(directory.path()); + let output = postil() + .current_dir(directory.path()) + .env("POSTIL_API_BASE", server.uri()) + .env("REVIEW_MODEL", "generator-model") + .env("POSTIL_DISABLE_SCORER", "1") + .args(["review", "--diff-file"]) + .arg(&diff) + .args(["--output", "json"]) + .assert() + .success(); + + let envelope: Value = serde_json::from_slice(&output.get_output().stdout).unwrap(); + assert_eq!(envelope["findings"][0]["body"], original_body); + assert_eq!(envelope["findings"][0]["severity"], "warn"); + assert_eq!(envelope["gate"]["failing"], false); +} + #[tokio::test] async fn uncertainty_resolution_explicit_off_makes_no_resolution_call() { let server = MockServer::start().await;