diff --git a/src/envelope.rs b/src/envelope.rs index 9cdda3e..f76008a 100644 --- a/src/envelope.rs +++ b/src/envelope.rs @@ -1823,6 +1823,7 @@ pub fn is_ephemeral_anchor(path: &str) -> bool { #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum IncompleteReviewReason { IncompleteInput, + LocalIncrementalFullComparisonUnavailable, ReservedInput, InsufficientContextBudget, InvalidModelFanOut, @@ -1833,6 +1834,9 @@ pub(crate) fn incomplete_review_finding(reason: IncompleteReviewReason) -> Findi IncompleteReviewReason::IncompleteInput => { "Postil could not acquire the complete immutable change from the forge, so no clean verdict was issued. Retry after the forge can supply it." } + IncompleteReviewReason::LocalIncrementalFullComparisonUnavailable => { + "An incremental local review touched the file of a carried error finding, but this input cannot reconstruct the complete comparison needed to re-adjudicate it. No clean verdict was issued. Review the pull request or rerun locally with `--base ` without `--since-sha`." + } IncompleteReviewReason::ReservedInput => { "The change uses a path reserved for Postil's synthetic review evidence, so repository content cannot be separated safely from operational findings. No clean verdict was issued. Rename the conflicting path before retrying." } @@ -2008,6 +2012,10 @@ mod tests { IncompleteReviewReason::IncompleteInput, "complete immutable change", ), + ( + IncompleteReviewReason::LocalIncrementalFullComparisonUnavailable, + "complete comparison", + ), (IncompleteReviewReason::ReservedInput, "path reserved"), ( IncompleteReviewReason::InsufficientContextBudget, diff --git a/src/forge/mod.rs b/src/forge/mod.rs index 6ca7e1f..a89848c 100644 --- a/src/forge/mod.rs +++ b/src/forge/mod.rs @@ -3030,6 +3030,7 @@ mod tests { fn forge_summary_keeps_incomplete_review_reasons_generic() { for reason in [ crate::envelope::IncompleteReviewReason::IncompleteInput, + crate::envelope::IncompleteReviewReason::LocalIncrementalFullComparisonUnavailable, crate::envelope::IncompleteReviewReason::ReservedInput, crate::envelope::IncompleteReviewReason::InsufficientContextBudget, crate::envelope::IncompleteReviewReason::InvalidModelFanOut, diff --git a/src/review.rs b/src/review.rs index 576a3ad..8c8990a 100644 --- a/src/review.rs +++ b/src/review.rs @@ -470,6 +470,19 @@ enum ReviewFailureKind { InvalidOutput, } +#[derive(Debug)] +struct LocalIncrementalFullComparisonUnavailable; + +impl std::fmt::Display for LocalIncrementalFullComparisonUnavailable { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.write_str( + "local incremental input cannot reconstruct the complete comparison required to re-adjudicate a carried error finding", + ) + } +} + +impl std::error::Error for LocalIncrementalFullComparisonUnavailable {} + fn classify_exhausted_scorer_failure( incidents: &[ModelIncident], final_error_is_provider: bool, @@ -627,10 +640,20 @@ async fn run_local(args: &ReviewArgs, cfg: &Config, repo_root: &Path) -> Result< let head_sha = local::head_sha().await; let local_snapshot = local::acquire(&source, head_sha.as_deref(), repo_root).await?; let baseline = load_baseline(args)?; + let touched_carried_error = cfg.enabled + && args.since_sha.is_some() + && carried_error_outside_anchor_file_is_touched(&baseline, &local_snapshot.diff); + let supports_full_fallback = matches!(&source, LocalSource::Base(_)); + let fail_closed_baseline = + (touched_carried_error && !supports_full_fallback).then(|| baseline.clone()); // This is a fail-closed placeholder, not the completed review's trust. // review_diff replaces it with Failed, Bounded, or Exhaustive after the // selected requests finish; an early model failure deliberately keeps it. - let scope = if args.since_sha.is_some() { + let scope = if touched_carried_error && supports_full_fallback { + filter::ReconcileScope::Full { + trust: filter::ReviewTrust::Failed, + } + } else if args.since_sha.is_some() { filter::ReconcileScope::Incremental { trust: filter::ReviewTrust::Failed, } @@ -640,34 +663,45 @@ async fn run_local(args: &ReviewArgs, cfg: &Config, repo_root: &Path) -> Result< } }; let review_started = std::time::Instant::now(); - let result = review_diff( - cfg, - args, - ReviewInput { - diff_snapshot: &local_snapshot.diff, - meta: None, - head_sha: head_sha.clone(), - repository_revision: local_snapshot.repository_revision.clone(), - repo: None, - baseline, - scope, - force_model: false, - llm_budget_started_at: None, - repository_source: if local_snapshot.repository_revision.is_some() { - RepositorySource::Local(repo_root) - } else { - RepositorySource::Unavailable + let result = if touched_carried_error && !supports_full_fallback { + Err(anyhow::Error::new( + LocalIncrementalFullComparisonUnavailable, + )) + } else { + review_diff( + cfg, + args, + ReviewInput { + diff_snapshot: &local_snapshot.diff, + meta: None, + head_sha: head_sha.clone(), + repository_revision: local_snapshot.repository_revision.clone(), + repo: None, + baseline, + scope, + force_model: touched_carried_error, + llm_budget_started_at: None, + repository_source: if local_snapshot.repository_revision.is_some() { + RepositorySource::Local(repo_root) + } else { + RepositorySource::Unavailable + }, }, - }, - ) - .await; - let envelope = match result { + ) + .await + }; + let mut envelope = match result { Ok(envelope) => envelope, // Only a completed review that failed operationally has enough // review state to produce a truthful error envelope. Input, // planning, registration, and client-construction failures occur // before provider access and retain the CLI error contract (exit 2). - Err(error) if error.downcast_ref::().is_some() => { + Err(error) + if error.downcast_ref::().is_some() + || error + .downcast_ref::() + .is_some() => + { eprintln!("postil: review failed before completion ({error:#})"); error_envelope( cfg, @@ -679,6 +713,20 @@ async fn run_local(args: &ReviewArgs, cfg: &Config, repo_root: &Path) -> Result< } Err(error) => return Err(error), }; + if let Some(baseline) = fail_closed_baseline { + let index = diff::DiffIndex::build(&diff::parse(local_snapshot.diff.as_str())); + let reconciliation = filter::reconcile( + &baseline, + &index, + &[], + filter::ReconcileScope::Full { + trust: filter::ReviewTrust::Failed, + }, + ); + envelope.findings.extend(reconciliation.carried); + envelope.counts = Envelope::counts_of(&envelope.findings, 0); + envelope.confidence_buckets = Envelope::buckets_of(&envelope.findings); + } finish(args, cfg, envelope, None::<&GitHub>, None, None, false).await } @@ -1076,7 +1124,8 @@ async fn remote_review( let (diff_snapshot, scope, force_model) = if cfg.enabled && has_carryable_baseline && matches!(scope, filter::ReconcileScope::Incremental { .. }) - && diff_snapshot.as_str().trim().is_empty() + && (diff_snapshot.as_str().trim().is_empty() + || carried_error_outside_anchor_file_is_touched(&baseline, &diff_snapshot)) { ( run_with_hosted_budget( @@ -3103,6 +3152,27 @@ fn baseline_has_carryable_findings(findings: &[Finding]) -> bool { .any(|finding| !crate::envelope::is_ephemeral_anchor(&finding.path)) } +/// A carried repository Error can be fixed away from its original anchor. When +/// the incremental change touches that file but not the anchor itself, only the +/// complete comparison can provide enough evidence to adjudicate the prior +/// finding without treating incremental model silence as a resolution. +fn carried_error_outside_anchor_file_is_touched( + baseline: &[Finding], + snapshot: &diff::DiffSnapshot, +) -> bool { + let changed = diff::parse(snapshot.as_str()); + let index = diff::DiffIndex::build(&changed); + baseline.iter().any(|finding| { + finding.severity == crate::envelope::Severity::Error + && !crate::envelope::is_reserved_anchor(&finding.path) + && changed + .files + .iter() + .any(|file| file.path == finding.path || file.old_path == finding.path) + && !index.contains_old(&finding.path, finding.line) + }) +} + /// Reserve every baseline finding that could enter full re-review adjudication /// before generating fresh findings. The later candidate set can be smaller /// when a fresh finding supersedes a baseline entry, but it must never be @@ -3351,6 +3421,12 @@ fn error_envelope( duration_ms: u64, ) -> Envelope { let incomplete_input = crate::forge::is_incomplete_review_input(err); + let incomplete_reason = err + .downcast_ref::() + .map(|_| crate::envelope::IncompleteReviewReason::LocalIncrementalFullComparisonUnavailable) + .or_else(|| { + incomplete_input.then_some(crate::envelope::IncompleteReviewReason::IncompleteInput) + }); let review_failure = err.downcast_ref::(); let invalid_output = review_failure.is_some_and(|failure| failure.kind == ReviewFailureKind::InvalidOutput); @@ -3367,10 +3443,8 @@ fn error_envelope( .downcast_ref::() .is_some_and(|error| error.is_connect() || error.is_timeout()) }); - let findings = vec![if incomplete_input { - crate::envelope::incomplete_review_finding( - crate::envelope::IncompleteReviewReason::IncompleteInput, - ) + let findings = vec![if let Some(reason) = incomplete_reason { + crate::envelope::incomplete_review_finding(reason) } else if invalid_output || !advisory_operational_error { // Only a recorded scorer error is genuinely about model output. Every // other failure here reaches this branch with its cause in `err`, so @@ -4221,6 +4295,44 @@ mod tests { ])); } + #[test] + fn carried_error_escalation_excludes_anchor_warnings_synthetic_and_untouched_findings() { + let snapshot = diff::DiffSnapshot::from_bytes( + b"diff --git a/src/auth.rs b/src/auth.rs\n--- a/src/auth.rs\n+++ b/src/auth.rs\n@@ -40 +40 @@\n-old authorization\n+new authorization\n", + ) + .unwrap(); + let error = finding("src/auth.rs", 10, "prior blocker"); + assert!(carried_error_outside_anchor_file_is_touched( + std::slice::from_ref(&error), + &snapshot + )); + + let anchored = finding("src/auth.rs", 40, "anchored blocker"); + assert!(!carried_error_outside_anchor_file_is_touched( + &[anchored], + &snapshot + )); + + let mut warning = error.clone(); + warning.severity = Severity::Warn; + assert!(!carried_error_outside_anchor_file_is_touched( + &[warning], + &snapshot + )); + + let synthetic = finding(crate::envelope::CHANGE_METADATA_PATH, 1, "metadata blocker"); + assert!(!carried_error_outside_anchor_file_is_touched( + &[synthetic], + &snapshot + )); + + let untouched = finding("src/db.rs", 10, "database blocker"); + assert!(!carried_error_outside_anchor_file_is_touched( + &[untouched], + &snapshot + )); + } + #[test] fn only_fresh_unresolved_repository_claims_are_suppressed() { let claim = RepositoryClaim { diff --git a/tests/e2e.rs b/tests/e2e.rs index c3b9d9c..3b35962 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -12487,6 +12487,227 @@ async fn stale_incremental_baseline_falls_back_to_full_review() { ); } +#[tokio::test] +async fn incremental_touched_carried_error_falls_back_to_full_review() { + let server = MockServer::start().await; + mount_github_complete_diff(&server, 7).await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/compare/cccccccc...aaaaaaaa")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "merge_base_commit": {"sha": "cccccccc"}, + "files": [{"filename": "src/auth.rs", "status": "modified", "changes": 2}] + }))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .respond_with(ResponseTemplate::new(200).set_body_json(llm_content(json!([])))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/7")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "title": "t", "body": null, + "state": "open", "merged": false, + "head": {"sha": "aaaaaaaa"}, "base": {"sha": "bbbbbbbb"}, "changed_files": 1 + }))) + .mount(&server) + .await; + + let baseline = json!({ + "version": 1, "summary": "", "silent": false, + "findings": [{ + "path": "src/auth.rs", "line": 10, "severity": "error", "kind": "risk", + "confidence": 0.9, "title": "Prior authorization blocker", + "body": "The authorization path remains unsafe." + }], + "resolved": [], "counts": {"info": 0, "warn": 0, "error": 1, "suppressed": 0}, + "confidenceBuckets": [0,0,0,0,1], + "gate": {"failOn": "error", "failing": true}, + "modelUsed": "m", "usage": {"promptTokens": 0, "completionTokens": 0}, + "baseSha": "bbbbbbbb", "headSha": "cccccccc", "sinceSha": null + }); + let dir = tempfile::tempdir().unwrap(); + let baseline_path = dir.path().join("baseline.json"); + std::fs::write(&baseline_path, baseline.to_string()).unwrap(); + + let out = postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .env("GITHUB_API_URL", server.uri()) + .env("GITHUB_TOKEN", "gh-test-token") + .args([ + "review", + "--repo", + "acme/api", + "--pr", + "7", + "--sha", + "aaaaaaaa", + "--since-sha", + "cccccccc", + "--baseline", + ]) + .arg(&baseline_path) + .args(["--no-post", "--output-json"]) + .assert() + .code(1); + let env: Value = serde_json::from_slice(&out.get_output().stdout).unwrap(); + assert_eq!(env["sinceSha"], Value::Null); + + let requests = server.received_requests().await.unwrap(); + assert!( + requests + .iter() + .any(|request| request.url.path() == "/repos/acme/api/pulls/7/files"), + "a touched carried Error must fetch the complete change" + ); + let source_request = requests + .iter() + .find(|request| request.url.path() == "/chat/completions") + .unwrap(); + let body: Value = source_request.body_json().unwrap(); + assert!( + !body["messages"][1]["content"] + .as_str() + .unwrap() + .contains("This is an INCREMENTAL review"), + "full fallback must happen before model review and adjudication" + ); +} + +#[tokio::test] +async fn local_incremental_diff_file_with_touched_carried_error_fails_closed_actionably() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .respond_with(ResponseTemplate::new(200).set_body_json(llm_content(json!([])))) + .expect(0) + .mount(&server) + .await; + let dir = tempfile::tempdir().unwrap(); + let diff = write_diff(dir.path()); + let baseline = json!({ + "version": 1, "summary": "", "silent": false, + "findings": [{ + "path": "src/auth.rs", "line": 10, "severity": "error", "kind": "risk", + "confidence": 0.9, "title": "Prior authorization blocker", + "body": "The authorization path remains unsafe." + }], + "resolved": [], "counts": {"info": 0, "warn": 0, "error": 1, "suppressed": 0}, + "confidenceBuckets": [0,0,0,0,1], + "gate": {"failOn": "error", "failing": true}, + "modelUsed": "m", "usage": {"promptTokens": 0, "completionTokens": 0}, + "baseSha": null, "headSha": null, "sinceSha": null + }); + let baseline_path = dir.path().join("baseline.json"); + std::fs::write(&baseline_path, baseline.to_string()).unwrap(); + + let out = postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .args(["review", "--diff-file"]) + .arg(&diff) + .args(["--since-sha", "previous", "--baseline"]) + .arg(&baseline_path) + .args(["--output", "json"]) + .assert() + .code(1); + let env: Value = serde_json::from_slice(&out.get_output().stdout).unwrap(); + let stderr = String::from_utf8_lossy(&out.get_output().stderr); + assert!(stderr.contains("cannot reconstruct the complete comparison")); + let incomplete = env["findings"] + .as_array() + .unwrap() + .iter() + .find(|finding| finding["path"] == ".postil/model-output") + .unwrap(); + assert_eq!(incomplete["title"], "Review incomplete"); + assert!( + incomplete["body"] + .as_str() + .unwrap() + .contains("complete comparison") + ); + let carried = env["findings"] + .as_array() + .unwrap() + .iter() + .find(|finding| finding["title"] == "Prior authorization blocker") + .expect("the prior blocker must remain in the next baseline"); + assert!( + carried["body"] + .as_str() + .unwrap() + .starts_with("[carried from previous review]") + ); + assert_eq!(env["resolved"], json!([])); + assert_eq!(env["gate"]["failing"], true); +} + +#[tokio::test] +async fn local_incremental_staged_review_with_touched_carried_error_fails_closed() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .respond_with(ResponseTemplate::new(200).set_body_json(llm_content(json!([])))) + .expect(0) + .mount(&server) + .await; + let dir = tempfile::tempdir().unwrap(); + initialize_staged_repository(dir.path()); + std::fs::write(dir.path().join(".postil.yaml"), "enabled: true\n").unwrap(); + let baseline = json!({ + "version": 1, "summary": "", "silent": false, + "findings": [{ + "path": "src/auth.rs", "line": 10, "severity": "error", "kind": "risk", + "confidence": 0.9, "title": "Prior authorization blocker", + "body": "The authorization path remains unsafe." + }], + "resolved": [], "counts": {"info": 0, "warn": 0, "error": 1, "suppressed": 0}, + "confidenceBuckets": [0,0,0,0,1], + "gate": {"failOn": "error", "failing": true}, + "modelUsed": "m", "usage": {"promptTokens": 0, "completionTokens": 0}, + "baseSha": null, "headSha": null, "sinceSha": null + }); + let baseline_path = dir.path().join("baseline.json"); + std::fs::write(&baseline_path, baseline.to_string()).unwrap(); + + let out = postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .args([ + "review", + "--staged", + "--since-sha", + "previous", + "--baseline", + ]) + .arg(&baseline_path) + .args(["--output", "json"]) + .assert() + .code(1); + let env: Value = serde_json::from_slice(&out.get_output().stdout).unwrap(); + assert!( + env["findings"] + .as_array() + .unwrap() + .iter() + .any(|finding| finding["path"] == ".postil/model-output" + && finding["title"] == "Review incomplete") + ); + assert!(env["findings"].as_array().unwrap().iter().any(|finding| { + finding["title"] == "Prior authorization blocker" + && finding["body"] + .as_str() + .is_some_and(|body| body.starts_with("[carried from previous review]")) + })); + assert_eq!(env["resolved"], json!([])); + assert_eq!(env["gate"]["failing"], true); +} + #[tokio::test] async fn incremental_forge_outage_still_fails_the_review() { let server = MockServer::start().await;