Harden forge posting: transient failures no longer discard a completed review

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(&section);
         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(&section);
+        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(&section);
+        assert!(!parsed.files[0].binary);
+        assert!(!parsed.files[0].hunks.is_empty());
+    }
 }

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(&note)
+            .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<String> = 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(&note)
-                    .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)> {

src/llm.rs

@@ -484,6 +484,28 @@ fn consensus_merge(runs: Vec<ModelReview>) -> 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::<ProviderError>().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::<ProviderError>().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```";

src/review.rs

@@ -167,9 +167,16 @@ async fn run_remote<F: Forge>(
                 } 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<F: Forge>(
                     .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<F: Forge>(
                 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 })

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;