diff --git a/src/forge/azure.rs b/src/forge/azure.rs index 5eb2681..83a8d13 100644 --- a/src/forge/azure.rs +++ b/src/forge/azure.rs @@ -19,6 +19,15 @@ use crate::envelope::{Envelope, Finding}; const API_VERSION: &str = "7.1"; +/// Per-file cap on fetched content before diffing. Azure's `/items` endpoint +/// returns full file text with no size limit and no cheap way to learn the +/// length up front, so without this a single huge file (a lockfile, a +/// vendored bundle) is fetched in full and handed to `similar::TextDiff` +/// before the assembled diff ever reaches `MAX_RAW_DIFF_BYTES` in review.rs. +/// Generous for any ordinary source file; anything larger is marked skipped, +/// the same way binary content is below. +const MAX_FILE_BYTES: usize = 2 * 1024 * 1024; + pub struct Azure { http: reqwest::Client, base: String, @@ -231,15 +240,7 @@ impl Azure { if old == new { continue; } - // Binary content cannot be line-diffed; mark it like git does so - // the path is still visible in the review context. - if old.contains('\0') || new.contains('\0') { - out.push_str(&format!( - "diff --git a/{path} b/{path}\nBinary files a/{path} and b/{path} differ\n" - )); - continue; - } - out.push_str(&unified_file_diff(path, &old, &new, is_add, is_delete)); + out.push_str(&diff_section(path, &old, &new, is_add, is_delete)); } Ok(out) } @@ -496,6 +497,30 @@ fn merge_commits( } } +/// One file's diff section, or a "differ" marker if the content is binary or +/// exceeds the per-file size cap. Split out of `build_diff` so the +/// classification is unit-testable without network access. +fn diff_section(path: &str, old: &str, new: &str, is_add: bool, is_delete: bool) -> String { + // One oversized file must not be diffed in full before any size cap ever + // sees it; mark it skipped like a binary file so the path is still + // visible in the review context. + if old.len() > MAX_FILE_BYTES || new.len() > MAX_FILE_BYTES { + return format!( + "diff --git a/{path} b/{path}\nBinary files a/{path} and b/{path} differ \ + (exceeds {}MiB per-file cap, not diffed)\n", + MAX_FILE_BYTES / (1024 * 1024) + ); + } + // Binary content cannot be line-diffed; mark it like git does so the path + // is still visible in the review context. + if old.contains('\0') || new.contains('\0') { + return format!( + "diff --git a/{path} b/{path}\nBinary files a/{path} and b/{path} differ\n" + ); + } + unified_file_diff(path, old, new, is_add, is_delete) +} + /// Build one file's unified-diff section in the format `diff.rs` expects: /// a `diff --git` header (so the parser seeds the path even for additions), /// the `---`/`+++` lines, and `similar`-generated hunks. @@ -560,4 +585,29 @@ mod tests { let parsed = diff::parse(§ion); assert_eq!(parsed.files.len(), 1); } + + #[test] + fn oversized_file_is_marked_skipped_not_diffed() { + // One side exceeds the per-file cap: the section must be marked + // skipped (parsed as binary, no hunks) rather than diffed in full. + let huge = "x".repeat(MAX_FILE_BYTES + 1); + let section = diff_section("big.bin", "", &huge, true, false); + assert!(section.contains("differ")); + assert!(section.contains("per-file cap")); + let parsed = diff::parse(§ion); + assert_eq!(parsed.files.len(), 1); + assert!(parsed.files[0].binary); + assert!(parsed.files[0].hunks.is_empty()); + } + + #[test] + fn ordinary_file_under_cap_is_diffed_normally() { + let old = "fn a() {\n let x = 1;\n}\n"; + let new = "fn a() {\n let x = 2;\n}\n"; + let section = diff_section("src/a.rs", old, new, false, false); + assert!(!section.contains("differ")); + let parsed = diff::parse(§ion); + assert!(!parsed.files[0].binary); + assert!(!parsed.files[0].hunks.is_empty()); + } } diff --git a/src/forge/gitlab.rs b/src/forge/gitlab.rs index bf2d7f4..d668b5d 100644 --- a/src/forge/gitlab.rs +++ b/src/forge/gitlab.rs @@ -124,6 +124,52 @@ impl GitLab { out } + /// Post one finding as an anchored discussion, falling back to a plain + /// note when GitLab refuses the position (e.g. line outside the visible + /// diff). + async fn post_finding(&self, mr: &MrResponse, f: &Finding) -> Result<()> { + let body = json!({ + "body": super::finding_comment_body(f, true), + "position": { + "position_type": "text", + "base_sha": mr.diff_refs.base_sha, + "start_sha": mr.diff_refs.start_sha, + "head_sha": mr.diff_refs.head_sha, + "new_path": f.path, + "new_line": f.line, + }, + }); + let resp = self + .request( + reqwest::Method::POST, + self.url(&format!("/merge_requests/{}/discussions", self.mr_iid)), + ) + .json(&body) + .send() + .await + .context("posting discussion")?; + if resp.status().is_success() { + return Ok(()); + } + let note = json!({ + "body": format!( + "`{}:{}` **{}** ({})\n\n{}", + f.path, f.line, f.title, f.severity.as_str(), f.body + ), + }); + let resp = self + .request( + reqwest::Method::POST, + self.url(&format!("/merge_requests/{}/notes", self.mr_iid)), + ) + .json(¬e) + .send() + .await + .context("posting fallback note")?; + Self::check_ok(resp, "note post").await?; + Ok(()) + } + async fn set_status( &self, sha: &str, @@ -207,6 +253,9 @@ impl Forge for GitLab { _head_sha: &str, ) -> Result<()> { let mr = self.mr().await?; + // One failed comment must not drop the rest: post everything we can, + // then report the failures together. + let mut failures: Vec = Vec::new(); for f in findings { if f.body.starts_with("[carried from previous review]") || super::is_synthetic_path(&f.path) @@ -216,45 +265,8 @@ impl Forge for GitLab { // and surface in the summary body instead. continue; } - let body = json!({ - "body": super::finding_comment_body(f, true), - "position": { - "position_type": "text", - "base_sha": mr.diff_refs.base_sha, - "start_sha": mr.diff_refs.start_sha, - "head_sha": mr.diff_refs.head_sha, - "new_path": f.path, - "new_line": f.line, - }, - }); - let resp = self - .request( - reqwest::Method::POST, - self.url(&format!("/merge_requests/{}/discussions", self.mr_iid)), - ) - .json(&body) - .send() - .await - .context("posting discussion")?; - // A position the API refuses (e.g. line outside the visible diff) - // falls back to a plain note rather than dropping the finding. - if !resp.status().is_success() { - let note = json!({ - "body": format!( - "`{}:{}` **{}** ({})\n\n{}", - f.path, f.line, f.title, f.severity.as_str(), f.body - ), - }); - let resp = self - .request( - reqwest::Method::POST, - self.url(&format!("/merge_requests/{}/notes", self.mr_iid)), - ) - .json(¬e) - .send() - .await - .context("posting fallback note")?; - Self::check_ok(resp, "note post").await?; + if let Err(e) = self.post_finding(&mr, f).await { + failures.push(format!("{}:{}: {e:#}", f.path, f.line)); } } if !summary.is_empty() { @@ -269,7 +281,15 @@ impl Forge for GitLab { .context("posting summary note")?; Self::check_ok(resp, "summary post").await?; } - Ok(()) + if failures.is_empty() { + Ok(()) + } else { + Err(anyhow!( + "{} finding comment(s) failed to post: {}", + failures.len(), + failures.join("; ") + )) + } } async fn start_checks(&self, head_sha: &str) -> Result<(String, String)> { diff --git a/src/llm.rs b/src/llm.rs index b210f1c..e93ed2a 100644 --- a/src/llm.rs +++ b/src/llm.rs @@ -484,6 +484,28 @@ fn consensus_merge(runs: Vec) -> ModelReview { mod tests { use super::*; use crate::envelope::{Kind, Severity}; + + #[test] + fn provider_error_downcast_survives_additional_context() { + // review.rs's fail-open/fail-closed classifier does + // `e.downcast_ref::().is_some()` on the error returned by + // the LLM client. That error picks up further `.context(...)` layers + // between here and there (e.g. review.rs wrapping client calls); this + // pins that anyhow's downcast_ref still finds the marker underneath any + // number of additional context layers, so the marker is never masked by + // later wrapping. + let base: anyhow::Error = + anyhow::Error::new(std::io::Error::other("boom")).context(ProviderError); + let wrapped = base + .context("fetching diff") + .context("running review") + .context("one more layer for good measure"); + assert!( + wrapped.downcast_ref::().is_some(), + "ProviderError marker must survive additional context wrapping" + ); + } + #[test] fn extracts_json_from_fenced_output() { let text = "Here you go:\n```json\n{\"summary\": \"s\", \"findings\": []}\n```"; diff --git a/src/review.rs b/src/review.rs index c18ec02..197c394 100644 --- a/src/review.rs +++ b/src/review.rs @@ -167,9 +167,16 @@ async fn run_remote( } else { CheckState::Success }; - forge + // A transient forge outage here (rate limit, timeout) must not + // discard the review this far along: log and keep going so the + // envelope/SARIF output and exit code below still land. Mirrors + // the Err(e) arm below, which best-effort's this same call. + if let Err(e) = forge .complete_checks(a, g, advisory_state, gate_state, &envelope) - .await?; + .await + { + eprintln!("postil: could not update check runs ({e:#})"); + } } finish(args, cfg, envelope, Some(forge)).await } @@ -195,11 +202,14 @@ async fn run_remote( .complete_checks(a, g, CheckState::Neutral, gate_state, &envelope) .await; } - // Emit envelope/SARIF and derive the exit code from the gate. The - // fetch that failed here is forge I/O, so a follow-up comment post - // may also fail; that must not mask the derived exit code, so - // posting failures on this already-errored path are downgraded to - // the gate-derived code rather than propagated as exit 2. + // Emit envelope/SARIF and derive the exit code from the gate. + // `finish` itself already downgrades forge posting failures + // (complete_checks/post_review) to warnings on stderr without + // touching its return value, so this only remains as a fallback + // for a local I/O failure inside `finish` (e.g. writing SARIF) on + // this already-errored path; even that must not mask the derived + // exit code, so it is downgraded to the gate-derived code rather + // than propagated as exit 2. let code = if envelope.gate.failing { 1 } else { 0 }; match finish(args, cfg, envelope, Some(forge)).await { Ok(c) => Ok(c), @@ -479,10 +489,14 @@ async fn finish( check_summary(&envelope, rich) }; let head = envelope.head_sha.clone().unwrap_or_default(); - forge - .post_review(&summary, &envelope.findings, &head) - .await - .context("posting review")?; + // A posting failure here (rate limit, transient 5xx, network blip) + // must not discard a review that already computed and persisted its + // envelope/SARIF/stdout output above. Log it and keep going — the + // exit code always derives from the gate below, never from whether + // the forge comment made it out. + if let Err(e) = forge.post_review(&summary, &envelope.findings, &head).await { + eprintln!("postil: could not post review comment ({e:#})"); + } } } Ok(if envelope.gate.failing { 1 } else { 0 }) diff --git a/tests/e2e.rs b/tests/e2e.rs index 054ded4..8a9c659 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -629,6 +629,84 @@ async fn cascade_falls_back_to_next_model() { assert_eq!(env["modelUsed"], "backup-model"); } +#[tokio::test] +async fn forge_post_failure_on_success_path_keeps_gate_derived_exit_code() { + // A completed review (gate failing on a real finding) whose forge comment + // post then fails (rate limit, transient 5xx) must not discard the + // already-computed envelope or exit 2: the exit code stays derived from + // the gate, the envelope is still emitted, and both check runs still + // complete — only the review comment itself is lost, and only a stderr + // warning marks that. + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/chat/completions")) + .respond_with( + ResponseTemplate::new(200) + .set_body_json(llm_content(json!([finding_at(41, "error", 0.92)]))), + ) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/acme/api/pulls/7")) + .and(header("Accept", "application/vnd.github.v3.diff")) + .respond_with(ResponseTemplate::new(200).set_body_string(DIFF)) + .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": "b", + "head": {"sha": "headsha111"}, "base": {"sha": "basesha222"} + }))) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/repos/acme/api/check-runs")) + .respond_with(ResponseTemplate::new(201).set_body_json(json!({"id": 11}))) + .mount(&server) + .await; + Mock::given(method("PATCH")) + .and(path_regex(r"^/repos/acme/api/check-runs/\d+$")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({}))) + .mount(&server) + .await; + // The review comment post itself fails — the fault this test is pinning. + Mock::given(method("POST")) + .and(path("/repos/acme/api/pulls/7/reviews")) + .respond_with(ResponseTemplate::new(500).set_body_string("secondary rate limit")) + .mount(&server) + .await; + + let dir = tempfile::tempdir().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", "--output-json"]) + .assert() + .code(1); // gate-derived exit code, unaffected by the failed post + + let stdout = String::from_utf8(out.get_output().stdout.clone()).unwrap(); + let env: Value = serde_json::from_str(&stdout).unwrap(); + assert_eq!(env["gate"]["failing"], true); + assert_eq!(env["findings"][0]["path"], "src/auth.rs"); + + let stderr = String::from_utf8(out.get_output().stderr.clone()).unwrap(); + assert!( + stderr.contains("could not post review comment"), + "expected a warning about the failed post, got: {stderr}" + ); + + // Both check runs still completed despite the review-comment failure. + let reqs = server.received_requests().await.unwrap(); + let patches = reqs + .iter() + .filter(|r| r.method == wiremock::http::Method::PATCH) + .count(); + assert_eq!(patches, 2); +} + #[tokio::test] async fn hosted_path_completes_provided_check_run_ids_without_creating_new_ones() { let server = MockServer::start().await;