codewhale-hq/Codewhale
1
0 Fork 0
mirror of https://github.com/codewhale-hq/Codewhale.git synced 2026-11-11 01:51:34 -10:00
Code Issues Packages Projects Releases 1 Wiki Activity

fix(review): collect complete PR diffs beyond GitHub's file limit

Browse Source 
Move CLI, interactive PR and model-tool collection to one pinned PR input
owner. Fall back from GitHub's 300-file diff limit or incomplete patches to
local Git merge-base/head objects without fetching or checking out PR code.
Require complete file/addition/deletion counts and text hunks, bounded output,
valid commit IDs and full history. Recheck the PR revision after collection
and before accepting/posting a result or receipt.

Refuse oversized PR inputs instead of truncating them. Preserve the entire
admitted diff in both prompt builders and record binary semantic-review
limitations in returned summaries, posted reviews and receipts.

Validation actually executed:
- Standalone compilation of the actual shared source modules using existing
  dependency rlibs: 23 passed, 0 failed (8 collector + 15 hunk-parser tests).
- Synthetic full-history fixture reusing local integration trees: complete
  510-file diff, 72211 additions, 12358 deletions, 5999455 bytes accepted.
- Focused module Clippy with -D warnings, rustfmt and git diff --check passed.
- Real integration checkout correctly refused because it is shallow.

No Cargo/TUI build or combined gate run by this lane; 3 new caller-level
regressions await the coordinator's build. No real GitHub/provider calls,
publication, fetch, checkout or workflow change. Coordinator owns full-history
CI checkout, exact PR objects and explicit model-appropriate max-chars.
This commit is contained in:
CodeWhale Bot committed 2026-09-08 06:31:27 -07:00
1 parent 69764bc4e8
commit a58cc8cd83
5 files changed
+785 -132
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

crates/tui/src/lib.rs
+63 -87
View File
@@ -1264,7 +1264,7 @@ struct ReviewArgs {
/// with "available from configured provider route(s): ...").
#[arg(long)]
provider: Option<String>,
/// Maximum diff characters to include
/// Maximum diff characters; an oversized PR is refused, never truncated
#[arg(long, default_value_t = 200_000)]
max_chars: usize,
/// Write a durable pre-push review receipt after a successful review
@@ -8335,7 +8335,7 @@ async fn run_review(config: &Config, args: ReviewArgs) -> Result<()> {
Some(number) => Some((number, run_gh_pr_view(number, args.repo.as_deref())?)),
None => None,
};
let diff = collect_diff(&args)?;
let diff = collect_diff(&args, pr_view.as_ref().map(|(_, view)| view))?;
if diff.trim().is_empty() {
bail!("No diff to review.");
}
@@ -8402,9 +8402,19 @@ Provide findings ordered by severity with file references, then open questions,
output.push_str(&text);
}
}
let structured = pr_view
.as_ref()
.map(|_| crate::tools::review::ReviewOutput::from_str(&output));
let structured = pr_view.as_ref().map(|_| {
let mut review = crate::tools::review::ReviewOutput::from_str(&output);
review.note_binary_coverage(&diff);
review
});
if !review_incomplete && let Some((number, view)) = &pr_view {
crate::tools::review_pr::ensure_current(
*number,
args.repo.as_deref(),
&std::env::current_dir()?,
view,
)?;
}
// A truncated review must not be posted or become a receipt. The partial
// text is still printed for diagnostics below.
if args.post && !review_incomplete {
@@ -8417,7 +8427,10 @@ Provide findings ordered by severity with file references, then open questions,
post_pr_review(*number, view, args.repo.as_deref(), review, &diff)?;
}
let receipt = if args.write_receipt && !review_incomplete {
let parsed_output = crate::tools::review::ReviewOutput::from_str(&output);
let mut parsed_output = crate::tools::review::ReviewOutput::from_str(&output);
if args.pr.is_some() {
parsed_output.note_binary_coverage(&diff);
}
let receipt = crate::tools::review::build_review_receipt(
review_target_label(&args),
&diff,
@@ -8633,7 +8646,7 @@ async fn run_pr(
}
let view = run_gh_pr_view(number, repo)?;
let diff = run_gh_pr_diff(number, repo)?;
let diff = run_gh_pr_diff(number, repo, &view)?;
if checkout {
match run_gh_pr_checkout(number, repo) {
@@ -8697,69 +8710,14 @@ fn is_command_available(name: &str) -> bool {
false
}
#[derive(Debug, Clone, Default)]
struct GhPullRequest {
title: String,
body: String,
base: String,
head: String,
url: String,
/// Head commit SHA (`headRefOid`). Anchors posted review comments to the
/// exact revision that was reviewed.
head_sha: String,
}
use crate::tools::review_pr::GhPullRequest;
fn run_gh_pr_view(number: u32, repo: Option<&str>) -> Result<GhPullRequest> {
let mut cmd = crate::dependencies::Gh::command()
.ok_or_else(|| anyhow::anyhow!("gh not found on PATH"))?;
cmd.arg("pr").arg("view").arg(number.to_string());
if let Some(r) = repo {
cmd.arg("--repo").arg(r);
}
cmd.arg("--json")
.arg("title,body,baseRefName,headRefName,url,headRefOid");
let output = cmd
.output()
.map_err(|e| anyhow::anyhow!("Failed to run `gh pr view`: {e}"))?;
if !output.status.success() {
let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string();
bail!("gh pr view #{number} failed: {stderr}");
}
let raw = String::from_utf8_lossy(&output.stdout).to_string();
let value: serde_json::Value = serde_json::from_str(&raw)
.map_err(|e| anyhow::anyhow!("gh pr view returned non-JSON output: {e}"))?;
let pick = |key: &str| {
value
.get(key)
.and_then(serde_json::Value::as_str)
.unwrap_or_default()
.to_string()
};
Ok(GhPullRequest {
title: pick("title"),
body: pick("body"),
base: pick("baseRefName"),
head: pick("headRefName"),
url: pick("url"),
head_sha: pick("headRefOid"),
})
crate::tools::review_pr::fetch_view(number, repo, &std::env::current_dir()?)
}
fn run_gh_pr_diff(number: u32, repo: Option<&str>) -> Result<String> {
let mut cmd = crate::dependencies::Gh::command()
.ok_or_else(|| anyhow::anyhow!("gh not found on PATH"))?;
cmd.arg("pr").arg("diff").arg(number.to_string());
if let Some(r) = repo {
cmd.arg("--repo").arg(r);
}
let output = cmd
.output()
.map_err(|e| anyhow::anyhow!("Failed to run `gh pr diff`: {e}"))?;
if !output.status.success() {
let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string();
bail!("gh pr diff #{number} failed: {stderr}");
}
Ok(String::from_utf8_lossy(&output.stdout).to_string())
fn run_gh_pr_diff(number: u32, repo: Option<&str>, view: &GhPullRequest) -> Result<String> {
crate::tools::review_pr::fetch_diff(number, repo, &std::env::current_dir()?, view)
}
fn run_gh_pr_checkout(number: u32, repo: Option<&str>) -> Result<()> {
@@ -9236,6 +9194,7 @@ fn post_pr_review(
review: &crate::tools::review::ReviewOutput,
diff: &str,
) -> Result<()> {
crate::tools::review_pr::ensure_current(number, repo, &std::env::current_dir()?, view)?;
let repo_name = match repo.map(str::trim).filter(|repo| !repo.is_empty()) {
Some(repo) => repo.to_string(),
None => run_gh_repo_name()?,
@@ -9261,30 +9220,16 @@ fn post_pr_review(
"warning: {} inline review comment(s) rejected ({err}); retrying summary-only",
inline.len()
);
crate::tools::review_pr::ensure_current(number, repo, &std::env::current_dir()?, view)?;
run_gh_post_pr_review(&repo_name, number, &body, &view.head_sha, &[])?;
}
Ok(())
}
/// Format the PR review prompt that lands in the composer. Caps the
/// diff at 200 KiB so a massive PR doesn't blow the model's context
/// window before the user even hits Enter — they can always ask the
/// model to fetch more via `gh pr diff #N` from inside the session.
/// Both the CLI review and interactive composer receive the complete diff.
/// Collection and review-budget checks must fail before any partial review.
fn format_pr_prompt(number: u32, view: &GhPullRequest, diff: &str) -> String {
const MAX_DIFF_BYTES: usize = 200 * 1024;
let diff_section = if diff.len() > MAX_DIFF_BYTES {
let cut = (0..=MAX_DIFF_BYTES)
.rev()
.find(|&i| diff.is_char_boundary(i))
.unwrap_or(0);
format!(
"{}\n\n[…diff truncated at {} KiB; ask me to fetch more if needed]\n",
&diff[..cut],
MAX_DIFF_BYTES / 1024
)
} else {
diff.to_string()
};
let diff_section = diff;
let body = if view.body.trim().is_empty() {
"(no description)".to_string()
} else {
@@ -9306,6 +9251,8 @@ fn format_pr_prompt(number: u32, view: &GhPullRequest, diff: &str) -> String {
\n\
URL: {url}\n\
Branches: {branches}\n\
Revision: {head_sha} (base {base_sha}); {changed_files} file patches.\n\
Binary patches contain Git binary data, not a semantic inspection of their contents. Do not claim binary contents were inspected.\n\
\n\
## Description\n\
\n\
@@ -9316,6 +9263,9 @@ fn format_pr_prompt(number: u32, view: &GhPullRequest, diff: &str) -> String {
```diff\n\
{diff_section}\n\
```\n",
head_sha = view.head_sha,
base_sha = view.base_sha,
changed_files = view.changed_files,
url = if view.url.is_empty() {
"(unavailable)"
} else {
@@ -9324,9 +9274,13 @@ fn format_pr_prompt(number: u32, view: &GhPullRequest, diff: &str) -> String {
)
}
fn collect_diff(args: &ReviewArgs) -> Result<String> {
fn collect_diff(args: &ReviewArgs, pr_view: Option<&GhPullRequest>) -> Result<String> {
let mut diff = if let Some(number) = args.pr {
run_gh_pr_diff(number, args.repo.as_deref())?
run_gh_pr_diff(
number,
args.repo.as_deref(),
pr_view.context("PR snapshot is required")?,
)?
} else {
let mut cmd = crate::dependencies::Git::command()
.ok_or_else(|| anyhow::anyhow!("git not found on PATH"))?;
@@ -9350,7 +9304,9 @@ fn collect_diff(args: &ReviewArgs) -> Result<String> {
}
String::from_utf8_lossy(&output.stdout).to_string()
};
if diff.len() > args.max_chars {
if args.pr.is_some() {
crate::tools::review_pr::ensure_input_fits(&diff, args.max_chars)?;
} else if diff.len() > args.max_chars {
diff = crate::utils::truncate_with_ellipsis(&diff, args.max_chars, "\n...[truncated]\n");
}
Ok(diff)
@@ -16012,6 +15968,25 @@ api_key = "test-only-key"
assert!(title_body.contains("```text\nrm -rf ~"), "{title_body}");
}
#[test]
fn pr_prompt_preserves_the_last_patch_beyond_the_old_200kib_cutoff() {
let diff = format!(
"{}\ndiff --git a/last.rs b/last.rs\n+LAST_PATCH\n",
"x".repeat(210 * 1024)
);
let view = GhPullRequest {
head_sha: "b".repeat(40),
base_sha: "a".repeat(40),
changed_files: 301,
..Default::default()
};
let prompt = format_pr_prompt(6002, &view, &diff);
assert!(prompt.contains(&diff));
assert!(prompt.contains("+LAST_PATCH"));
assert!(prompt.contains(&view.head_sha));
assert!(!prompt.contains("diff truncated"));
}
#[test]
fn pr_review_markdown_shows_the_computed_replacement() {
// A plain `codewhale review --pr` (no --post) computes and validates
@@ -16023,6 +15998,7 @@ api_key = "test-only-key"
head: "feature".to_string(),
url: "https://example.invalid/pr/1".to_string(),
head_sha: "abc123".to_string(),
..Default::default()
};
let mut single = review_suggestion(
Some("crates/tui/src/lib.rs"),
crates/tui/src/tools/mod.rs
+1
View File
@@ -53,6 +53,7 @@ mod resource_admission;
pub mod revert_turn;
pub mod review;
pub mod review_hunks;
pub(crate) mod review_pr;
pub mod rlm;
pub mod run_tool;
pub mod runtime_mcp;
crates/tui/src/tools/review.rs
+114 -44
View File
@@ -125,6 +125,12 @@ pub struct ReviewOutput {
}
impl ReviewOutput {
pub(crate) fn note_binary_coverage(&mut self, diff: &str) {
if diff.contains("\nGIT binary patch\n") {
self.summary.push_str("\nCoverage limitation: binary patches were included as Git binary data; their contents were not semantically inspected.");
}
}
#[must_use]
pub fn from_str(raw: &str) -> Self {
if let Some(parsed) = parse_review_output_json(raw) {
@@ -643,7 +649,7 @@ impl ToolSpec for ReviewTool {
},
"max_chars": {
"type": "integer",
"description": "Maximum characters to include from the source (default: 200000)."
"description": "Maximum source characters (default: 200000). A larger PR is refused before review, never truncated."
}
},
"required": ["target"]
@@ -679,6 +685,10 @@ impl ToolSpec for ReviewTool {
let source =
resolve_review_source(target, kind.as_deref(), staged, base.as_deref(), context)
.await?;
if let ReviewSource::PullRequest { diff, .. } = &source {
super::review_pr::ensure_input_fits(diff, max_chars)
.map_err(|error| ToolError::invalid_input(error.to_string()))?;
}
let prompt = build_review_prompt(&source, max_chars);
let route = client.effective_route_envelope(&self.model, chrono::Utc::now());
@@ -722,8 +732,12 @@ impl ToolSpec for ReviewTool {
.with_metadata(review_usage_metadata(&route, &response.usage)));
}
ensure_pr_source_current(&source, &context.workspace).await?;
let response_text = extract_text(&response.content);
let output = ReviewOutput::from_str(&response_text);
let mut output = ReviewOutput::from_str(&response_text);
if let ReviewSource::PullRequest { diff, .. } = &source {
output.note_binary_coverage(diff);
}
let metadata = review_usage_metadata(&route, &response.usage);
let result =
ToolResult::json(&output).map_err(|e| ToolError::execution_failed(e.to_string()))?;
@@ -747,9 +761,20 @@ fn review_usage_metadata(
}
enum ReviewSource {
File { display: String, content: String },
Diff { label: String, diff: String },
PullRequest { label: String, diff: String },
File {
display: String,
content: String,
},
Diff {
label: String,
diff: String,
},
PullRequest {
label: String,
diff: String,
pr: PullRequestRef,
view: super::review_pr::GhPullRequest,
},
}
async fn resolve_review_source(
@@ -772,11 +797,7 @@ async fn resolve_review_source(
"pr" | "pull" | "pull_request" => {
let pr = parse_pr_url(target)
.ok_or_else(|| ToolError::invalid_input("Invalid pull request URL"))?;
let diff = gh_pr_diff(&pr, &context.workspace).await?;
Ok(ReviewSource::PullRequest {
label: pr.label(),
diff,
})
gh_pr_source(pr, &context.workspace).await
}
other => Err(ToolError::invalid_input(format!(
"Unknown review kind '{other}'"
@@ -785,11 +806,7 @@ async fn resolve_review_source(
}
if let Some(pr) = parse_pr_url(target) {
let diff = gh_pr_diff(&pr, &context.workspace).await?;
return Ok(ReviewSource::PullRequest {
label: pr.label(),
diff,
});
return gh_pr_source(pr, &context.workspace).await;
}
if let Some(staged_override) = diff_mode_from_target(target) {
@@ -911,35 +928,56 @@ async fn run_review_git(
.map_err(|e| ToolError::execution_failed(format!("git {operation} task panicked: {e}")))?
}
async fn gh_pr_diff(pr: &PullRequestRef, workspace: &Path) -> Result<String, ToolError> {
let Some(mut cmd) = crate::dependencies::Gh::command() else {
return Err(ToolError::execution_failed("gh not found"));
};
cmd.arg("pr")
.arg("diff")
.arg(&pr.number)
.arg("--repo")
.arg(format!("{}/{}", pr.owner, pr.repo))
.current_dir(workspace);
async fn gh_pr_source(pr: PullRequestRef, workspace: &Path) -> Result<ReviewSource, ToolError> {
let workspace = workspace.to_path_buf();
tokio::task::spawn_blocking(move || {
let number = pr
.number
.parse::<u32>()
.map_err(|_| ToolError::invalid_input("Invalid pull request number"))?;
let repo = format!("{}/{}", pr.owner, pr.repo);
let view = super::review_pr::fetch_view(number, Some(&repo), &workspace)
.map_err(|error| ToolError::execution_failed(format!("{error:#}")))?;
let diff = super::review_pr::fetch_diff(number, Some(&repo), &workspace, &view)
.map_err(|error| ToolError::execution_failed(format!("{error:#}")))?;
Ok(ReviewSource::PullRequest {
label: pr.label(),
diff,
pr,
view,
})
})
.await
.map_err(|error| ToolError::execution_failed(format!("PR input task failed: {error}")))?
}
let output = tokio::task::spawn_blocking(move || cmd.output())
async fn ensure_pr_source_current(
source: &ReviewSource,
workspace: &Path,
) -> Result<(), ToolError> {
if let ReviewSource::PullRequest { pr, view, .. } = source {
let pr = pr.clone();
let view = view.clone();
let workspace = workspace.to_path_buf();
tokio::task::spawn_blocking(move || {
let number = pr
.number
.parse::<u32>()
.map_err(|_| ToolError::invalid_input("Invalid pull request number"))?;
super::review_pr::ensure_current(
number,
Some(&format!("{}/{}", pr.owner, pr.repo)),
&workspace,
&view,
)
.map_err(|error| ToolError::execution_failed(format!("{error:#}")))
})
.await
.map_err(|e| ToolError::execution_failed(format!("gh pr diff task panicked: {e}")))?
.map_err(|e| {
ToolError::execution_failed(format!("Failed to run gh pr diff (is gh installed?): {e}"))
})?;
if !output.status.success() {
let stderr = String::from_utf8_lossy(&output.stderr);
return Err(ToolError::execution_failed(format!(
"gh pr diff failed: {}",
stderr.trim()
)));
.map_err(|error| {
ToolError::execution_failed(format!("PR revision check failed: {error}"))
})??;
}
let diff = String::from_utf8_lossy(&output.stdout).to_string();
if diff.trim().is_empty() {
return Err(ToolError::invalid_input("Pull request diff is empty."));
}
Ok(diff)
Ok(())
}
fn build_review_prompt(source: &ReviewSource, max_chars: usize) -> String {
@@ -960,10 +998,12 @@ Path: {display}\n\n{truncated}\n\nEnd of file."
"Review the following {label} and provide feedback.\n\n{truncated}\n\nEnd of diff."
)
}
ReviewSource::PullRequest { label, diff } => {
let truncated = truncate_with_ellipsis(diff, max_chars, "\n...[truncated]\n");
ReviewSource::PullRequest {
label, diff, view, ..
} => {
format!(
"Review the following pull request diff ({label}) and provide feedback.\n\n{truncated}\n\nEnd of diff."
"Review the complete pull request diff ({label}) at head {} and base {}. Binary patches are supplied as Git binary data, not a semantic review of their contents. Do not claim binary contents were inspected.\n\n{diff}\n\nEnd of diff.",
view.head_sha, view.base_sha,
)
}
}
@@ -1134,6 +1174,36 @@ mod tests {
assert!(!diff.contains("unstaged"), "{diff}");
}
#[test]
fn binary_coverage_limit_is_part_of_the_returned_review_summary() {
let mut review = ReviewOutput::from_str(r#"{"summary":"Review findings"}"#);
review.note_binary_coverage("diff --git a/image b/image\nGIT binary patch\nliteral 4\n");
assert!(review.summary.contains("not semantically inspected"));
let mut text_review = ReviewOutput::from_str(r#"{"summary":"Text review"}"#);
text_review.note_binary_coverage("diff --git a/a b/a\n@@ -0,0 +1 @@\n+text\n");
assert_eq!(text_review.summary, "Text review");
}
#[test]
fn pr_tool_prompt_preserves_the_complete_diff_after_budget_admission() {
let diff = format!("{}\n+LAST_PATCH\n", "x".repeat(DEFAULT_MAX_CHARS + 1));
let source = ReviewSource::PullRequest {
label: "example/repo#6002".into(),
diff: diff.clone(),
pr: PullRequestRef {
owner: "example".into(),
repo: "repo".into(),
number: "6002".into(),
},
view: super::super::review_pr::GhPullRequest::default(),
};
assert!(super::super::review_pr::ensure_input_fits(&diff, DEFAULT_MAX_CHARS).is_err());
super::super::review_pr::ensure_input_fits(&diff, diff.len()).unwrap();
let prompt = build_review_prompt(&source, diff.len());
assert!(prompt.contains(&diff));
assert!(!prompt.contains("[truncated]"));
}
#[test]
fn parses_pr_url() {
let pr =
crates/tui/src/tools/review_hunks.rs
+1 -1
View File
@@ -218,7 +218,7 @@ fn post_image_path(rest: &str) -> Option<String> {
/// Parse `@@ -a,b +c,d @@ optional context` into
/// `(right_start, old_count, new_count)`. Counts may be omitted
/// (`@@ -1 +1 @@` means one line on each side).
fn parse_hunk_header(line: &str) -> Option<(u32, u32, u32)> {
pub(super) fn parse_hunk_header(line: &str) -> Option<(u32, u32, u32)> {
let rest = line.strip_prefix("@@ ")?;
let end = rest.find(" @@")?;
let ranges = &rest[..end];
crates/tui/src/tools/review_pr.rs
+606
View File
@@ -0,0 +1,606 @@
//! One immutable PR input for CLI, interactive and model-tool reviews.
//! Replaces their separate `gh pr diff` readers; large PRs use local pinned
//! Git objects without fetching, checking out or executing pull-request code.
use std::io::Read;
use std::path::Path;
use std::process::Stdio;
use std::time::Duration;
use anyhow::{Context, Result, bail};
use serde::Deserialize;
use wait_timeout::ChildExt;
use crate::dependencies::{ExternalTool, Gh, Git};
const MAX_OUTPUT_BYTES: usize = 8 * 1024 * 1024;
const VIEW_FIELDS: &str =
"title,body,baseRefName,headRefName,url,headRefOid,baseRefOid,changedFiles,additions,deletions";
#[derive(Debug, Clone, Default, Deserialize)]
pub(crate) struct GhPullRequest {
pub title: String,
pub body: String,
#[serde(rename = "baseRefName")]
pub base: String,
#[serde(rename = "headRefName")]
pub head: String,
pub url: String,
#[serde(rename = "headRefOid")]
pub head_sha: String,
#[serde(rename = "baseRefOid")]
pub base_sha: String,
#[serde(rename = "changedFiles")]
pub changed_files: usize,
pub additions: usize,
pub deletions: usize,
}
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
enum Program {
Gh,
Git,
}
fn pr_args(action: &str, number: u32, repo: Option<&str>) -> Vec<String> {
let mut args = vec!["pr".into(), action.into(), number.to_string()];
if let Some(repo) = repo {
args.extend(["--repo".into(), repo.into()]);
}
args
}
fn commit_id(value: &str) -> bool {
matches!(value.len(), 40 | 64) && value.bytes().all(|byte| byte.is_ascii_hexdigit())
}
fn view_with(
number: u32,
repo: Option<&str>,
run: &mut impl FnMut(Program, &[String]) -> Result<String>,
) -> Result<GhPullRequest> {
if number == 0 {
bail!("A positive pull request number is required");
}
let mut args = pr_args("view", number, repo);
args.extend(["--json".into(), VIEW_FIELDS.into()]);
let view: GhPullRequest = serde_json::from_str(&run(Program::Gh, &args)?)
.context("gh pr view returned incomplete PR metadata")?;
if !commit_id(&view.base_sha) || !commit_id(&view.head_sha) {
bail!("gh pr view did not return exact base and head commit IDs");
}
Ok(view)
}
pub(crate) fn fetch_view(
number: u32,
repo: Option<&str>,
workspace: &Path,
) -> Result<GhPullRequest> {
view_with(number, repo, &mut |program, args| {
run_command(workspace, program, args)
})
}
fn same_revision(expected: &GhPullRequest, current: &GhPullRequest) -> Result<()> {
if expected.head_sha != current.head_sha
|| expected.base_sha != current.base_sha
|| expected.changed_files != current.changed_files
|| expected.additions != current.additions
|| expected.deletions != current.deletions
|| expected.url != current.url
{
bail!(
"Pull request changed during review; no current-PR review or receipt can be accepted. Run the review again."
);
}
Ok(())
}
pub(crate) fn ensure_current(
number: u32,
repo: Option<&str>,
workspace: &Path,
expected: &GhPullRequest,
) -> Result<()> {
same_revision(expected, &fetch_view(number, repo, workspace)?)
}
/// A PR review must never turn its configured input budget into a partial
/// review or a receipt that appears to cover the whole PR.
pub(crate) fn ensure_input_fits(diff: &str, max_chars: usize) -> Result<()> {
let chars = diff.chars().count();
if chars > max_chars {
bail!(
"Complete PR diff requires {chars} characters, exceeding the review limit of {max_chars}. No review was run or posted. Increase max_chars/--max-chars only if the selected model can accept the complete input."
);
}
Ok(())
}
fn complete_file_set(diff: &str, view: &GhPullRequest) -> Result<()> {
let mut files = 0;
let (mut additions, mut deletions) = (0, 0);
let mut remaining = (0_u32, 0_u32);
let mut has_patch = false;
for line in diff.lines() {
if remaining != (0, 0) {
match line.as_bytes().first() {
Some(b'+') if remaining.1 > 0 => {
remaining.1 -= 1;
additions += 1;
}
Some(b'-') if remaining.0 > 0 => {
remaining.0 -= 1;
deletions += 1;
}
Some(b' ') if remaining.0 > 0 && remaining.1 > 0 => {
remaining.0 -= 1;
remaining.1 -= 1;
}
Some(b'\\') => {}
_ => bail!("Incomplete PR diff: a text hunk is truncated or malformed"),
}
continue;
}
if line.starts_with("diff --git ") {
if files > 0 && !has_patch {
bail!("Incomplete PR diff: a file patch is missing");
}
files += 1;
has_patch = false;
} else if let Some((_, old, new)) = super::review_hunks::parse_hunk_header(line) {
remaining = (old, new);
has_patch = true;
} else if line.starts_with("@@") {
bail!("Incomplete PR diff: malformed hunk header");
} else if [
"new file mode ",
"deleted file mode ",
"old mode ",
"new mode ",
"rename from ",
"rename to ",
"GIT binary patch",
]
.iter()
.any(|prefix| line.starts_with(prefix))
{
has_patch = true;
} else if line.starts_with("Binary files ") {
bail!(
"PR diff contains a missing binary patch; complete local Git objects are required"
);
}
}
if remaining != (0, 0)
|| !has_patch
|| files == 0
|| files != view.changed_files
|| additions != view.additions
|| deletions != view.deletions
{
bail!(
"Incomplete PR diff: received {files} file patches, {additions} additions and {deletions} deletions; expected {}, {} and {}. No partial review is accepted.",
view.changed_files,
view.additions,
view.deletions
);
}
Ok(())
}
fn diff_with(
number: u32,
repo: Option<&str>,
view: &GhPullRequest,
run: &mut impl FnMut(Program, &[String]) -> Result<String>,
) -> Result<String> {
// GitHub's diff representation refuses PRs with more than 300 files.
// Preserve remote-only small-PR usage, but never rely on that limit for
// completeness: also check the metadata's changed-file count.
let remote = if view.changed_files <= 300 {
run(Program::Gh, &pr_args("diff", number, repo)).and_then(|diff| {
complete_file_set(&diff, view)?;
Ok(diff)
})
} else {
Err(anyhow::anyhow!("GitHub diff exceeds its 300-file limit"))
};
let diff = match remote {
Ok(diff) => diff,
Err(remote_error) => {
let local: Result<String> = (|| {
let shallow = run(
Program::Git,
&["rev-parse".into(), "--is-shallow-repository".into()],
)?;
if shallow.trim() != "false" {
bail!("A full Git history is required to establish the PR merge base");
}
let base = run(
Program::Git,
&[
"merge-base".into(),
"--all".into(),
view.base_sha.clone(),
view.head_sha.clone(),
],
)?;
let base = base.trim();
if !commit_id(base) {
bail!("The pinned PR commits do not have one available merge base");
}
let diff = run(
Program::Git,
&[
"diff".into(),
"--no-ext-diff".into(),
"--no-textconv".into(),
"--no-color".into(),
"--no-relative".into(),
"--binary".into(),
"--full-index".into(),
"--find-renames=50%".into(),
"--src-prefix=a/".into(),
"--dst-prefix=b/".into(),
"--ignore-submodules=none".into(),
"--submodule=short".into(),
base.into(),
view.head_sha.clone(),
"--".into(),
],
)?;
complete_file_set(&diff, view)?;
Ok(diff)
})();
local.with_context(|| format!("Cannot obtain the complete PR diff ({remote_error}). Make the exact base {} and head {} commits and their full history available in this repository (CI: fetch-depth: 0 and fetch the PR head ref). No fetch or checkout was performed.", view.base_sha, view.head_sha))?
}
};
same_revision(view, &view_with(number, repo, run)?)?;
Ok(diff)
}
pub(crate) fn fetch_diff(
number: u32,
repo: Option<&str>,
workspace: &Path,
view: &GhPullRequest,
) -> Result<String> {
diff_with(number, repo, view, &mut |program, args| {
run_command(workspace, program, args)
})
}
fn read_bounded(reader: impl Read, limit: usize) -> std::io::Result<Vec<u8>> {
let mut bytes = Vec::new();
reader.take(limit as u64 + 1).read_to_end(&mut bytes)?;
Ok(bytes)
}
fn run_command(workspace: &Path, program: Program, args: &[String]) -> Result<String> {
let mut command = match program {
Program::Gh => Gh::command(),
Program::Git => Git::command(),
}
.context("PR review requires Git and GitHub CLI on PATH")?;
command
.args(args)
.current_dir(workspace)
.env("GIT_NO_REPLACE_OBJECTS", "1")
.env("GIT_NO_LAZY_FETCH", "1")
.env("GIT_TERMINAL_PROMPT", "0")
.env("GH_PROMPT_DISABLED", "1")
.stdin(Stdio::null())
.stdout(Stdio::piped())
.stderr(Stdio::piped());
let mut child = command
.spawn()
.context("Failed to start PR input command")?;
let stdout = child
.stdout
.take()
.context("PR command stdout unavailable")?;
let stderr = child
.stderr
.take()
.context("PR command stderr unavailable")?;
let stdout = std::thread::spawn(move || read_bounded(stdout, MAX_OUTPUT_BYTES));
let stderr = std::thread::spawn(move || read_bounded(stderr, 64 * 1024));
let status = match child.wait_timeout(Duration::from_secs(60))? {
Some(status) => status,
None => {
let _ = child.kill();
let _ = child.wait();
bail!("PR input command timed out; no partial output was accepted");
}
};
let stdout = stdout
.join()
.map_err(|_| anyhow::anyhow!("PR stdout reader failed"))??;
let stderr = stderr
.join()
.map_err(|_| anyhow::anyhow!("PR stderr reader failed"))??;
if stdout.len() > MAX_OUTPUT_BYTES || stderr.len() > 64 * 1024 {
bail!(
"PR input exceeds the bounded capture limit (8 MiB diff); no partial output was accepted"
);
}
if !status.success() {
bail!(
"PR input command failed: {}",
String::from_utf8_lossy(&stderr).trim()
);
}
String::from_utf8(stdout).context("PR diff is not valid UTF-8; no lossy review is accepted")
}
#[cfg(test)]
mod tests {
use super::*;
fn view(files: usize) -> GhPullRequest {
GhPullRequest {
title: "Fixture".into(),
body: String::new(),
base: "main".into(),
head: "feature".into(),
url: "https://github.com/example/repo/pull/6002".into(),
base_sha: "a".repeat(40),
head_sha: "b".repeat(40),
changed_files: files,
additions: files,
deletions: 0,
}
}
fn metadata(view: &GhPullRequest) -> String {
serde_json::json!({
"title": view.title, "body": view.body, "baseRefName": view.base,
"headRefName": view.head, "url": view.url, "headRefOid": view.head_sha,
"baseRefOid": view.base_sha, "changedFiles": view.changed_files,
"additions": view.additions, "deletions": view.deletions,
})
.to_string()
}
fn patch(name: &str) -> String {
format!(
"diff --git a/{name} b/{name}\nnew file mode 100644\n--- /dev/null\n+++ b/{name}\n@@ -0,0 +1 @@\n+complete\n"
)
}
fn git(workspace: &Path, args: &[&str]) -> String {
run_command(
workspace,
Program::Git,
&args.iter().map(|arg| (*arg).into()).collect::<Vec<_>>(),
)
.expect("local Git fixture")
}
fn repository() -> tempfile::TempDir {
let dir = tempfile::tempdir().unwrap();
git(dir.path(), &["init", "--template="]);
git(dir.path(), &["config", "user.name", "Review Fixture"]);
git(
dir.path(),
&["config", "user.email", "[email protected]"],
);
git(dir.path(), &["config", "commit.gpgsign", "false"]);
let hooks = dir.path().join("empty-hooks");
std::fs::create_dir(&hooks).unwrap();
git(
dir.path(),
&["config", "core.hooksPath", hooks.to_str().unwrap()],
);
dir
}
#[test]
fn large_pr_uses_all_pinned_git_patches_including_binary_not_the_checkout() {
let dir = repository();
git(dir.path(), &["commit", "--allow-empty", "-m", "base"]);
let base = git(dir.path(), &["rev-parse", "HEAD"]).trim().to_string();
for i in 0..301 {
std::fs::write(
dir.path().join(format!("file-{i}.txt")),
format!("file {i}\n"),
)
.unwrap();
}
std::fs::write(dir.path().join("binary.dat"), b"\0\x01\x02\xff").unwrap();
git(dir.path(), &["add", "*.txt", "binary.dat"]);
git(dir.path(), &["commit", "-m", "PR head"]);
let head = git(dir.path(), &["rev-parse", "HEAD"]).trim().to_string();
std::fs::write(
dir.path().join("file-300.txt"),
"unreviewed checkout content\n",
)
.unwrap();
git(dir.path(), &["add", "file-300.txt"]);
git(dir.path(), &["commit", "-m", "unrelated local head"]);
std::fs::write(dir.path().join("file-0.txt"), "dirty worktree content\n").unwrap();
let view = GhPullRequest {
base_sha: base,
head_sha: head,
additions: 301,
..view(302)
};
let diff = diff_with(6002, Some("example/repo"), &view, &mut |program, args| {
if program == Program::Gh {
assert_eq!(
args[1], "view",
"large PR must not call the 300-file endpoint"
);
Ok(metadata(&view))
} else {
run_command(dir.path(), program, args)
}
})
.unwrap();
assert_eq!(
diff.lines()
.filter(|line| line.starts_with("diff --git "))
.count(),
302
);
assert!(diff.contains("+file 300\n"));
assert!(diff.contains("GIT binary patch\nliteral 4\n"));
assert!(!diff.contains("unreviewed checkout content"));
assert!(!diff.contains("dirty worktree content"));
}
#[test]
fn limit_error_missing_files_and_missing_binary_patch_use_the_same_fallback() {
let view = view(2);
let complete = patch("a.txt") + &patch("b.txt");
for remote in [
None,
Some(patch("a.txt")),
Some(
patch("a.txt")
+ "diff --git a/b.txt b/b.txt\nBinary files a/b.txt and b/b.txt differ\n",
),
] {
let mut used_local = false;
let result = diff_with(6002, None, &view, &mut |program, args| match (
program,
args[0].as_str(),
) {
(Program::Gh, _) if args[1] == "diff" => remote
.clone()
.ok_or_else(|| anyhow::anyhow!("HTTP 406: diff exceeds 300 files")),
(Program::Gh, _) => Ok(metadata(&view)),
(Program::Git, "rev-parse") => Ok("false\n".into()),
(Program::Git, "merge-base") => {
assert_eq!(&args[2..], &[view.base_sha.clone(), view.head_sha.clone()]);
Ok(view.base_sha.clone())
}
(Program::Git, "diff") => {
used_local = true;
assert!(args.contains(&"--no-ext-diff".into()));
assert!(args.contains(&"--no-textconv".into()));
assert!(args.contains(&"--binary".into()));
assert_eq!(
&args[args.len() - 3..],
&[view.base_sha.clone(), view.head_sha.clone(), "--".into()]
);
Ok(complete.clone())
}
_ => panic!("unexpected command"),
})
.unwrap();
assert!(used_local);
assert_eq!(result, complete);
}
}
#[test]
fn missing_shallow_or_ambiguous_history_never_returns_a_partial_diff() {
let view = view(301);
for failure in ["missing", "shallow", "multiple"] {
let error = diff_with(6002, None, &view, &mut |program, args| {
assert_eq!(program, Program::Git);
match args[0].as_str() {
"rev-parse" => Ok(if failure == "shallow" {
"true"
} else {
"false"
}
.into()),
"merge-base" if failure == "multiple" => {
Ok(format!("{}\n{}\n", "c".repeat(40), "d".repeat(40)))
}
"merge-base" => bail!("missing pinned commit object"),
_ => panic!("must not generate a diff without exact history"),
}
})
.unwrap_err();
let error = format!("{error:#}");
assert!(error.contains("Cannot obtain the complete PR diff"));
assert!(error.contains("fetch-depth: 0"));
assert!(error.contains(&view.head_sha));
}
}
#[test]
fn head_base_and_file_count_changes_after_collection_invalidate_the_review() {
let view = view(1);
for field in ["head", "base", "files"] {
let mut current = view.clone();
match field {
"head" => current.head_sha = "c".repeat(40),
"base" => current.base_sha = "d".repeat(40),
_ => current.changed_files = 2,
}
let error = diff_with(6002, None, &view, &mut |program, args| {
assert_eq!(program, Program::Gh);
Ok(if args[1] == "diff" {
patch("a.txt")
} else {
metadata(&current)
})
})
.unwrap_err();
assert!(
error
.to_string()
.contains("Pull request changed during review")
);
assert!(
same_revision(&view, &current).is_err(),
"publication uses the same revision check"
);
}
}
#[test]
fn review_budget_preserves_unicode_and_rejects_the_whole_oversized_input() {
let diff = patch("unicode.txt") + "+\u{1f433}\n";
ensure_input_fits(&diff, diff.chars().count()).unwrap();
let error = ensure_input_fits(&diff, diff.chars().count() - 1).unwrap_err();
assert!(error.to_string().contains("No review was run or posted"));
assert!(diff.ends_with("+\u{1f433}\n"));
}
#[test]
fn malformed_commit_metadata_cannot_become_git_arguments() {
let mut invalid = view(1);
invalid.head_sha = "--output=/tmp/unsafe".into();
assert!(view_with(6002, None, &mut |_, _| Ok(metadata(&invalid))).is_err());
}
#[test]
fn missing_or_truncated_text_patches_cannot_pass_with_a_matching_file_count() {
let view = view(1);
for diff in [
"diff --git a/a b/a\nindex 123..456 100644\n",
"diff --git a/a b/a\nnew file mode 100644\n",
"diff --git a/a b/a\n--- a/a\n+++ b/a\n@@ -0,0 +1,2 @@\n+first\n",
"diff --git a/a b/a\n--- a/a\n+++ b/a\n@@ malformed @@\n+first\n",
] {
assert!(complete_file_set(diff, &view).is_err(), "{diff}");
}
complete_file_set(&patch("a"), &view).unwrap();
}
#[test]
fn oversized_command_output_is_refused_without_unbounded_capture() {
let dir = repository();
std::fs::write(
dir.path().join("large.txt"),
vec![b'x'; MAX_OUTPUT_BYTES + 1],
)
.unwrap();
git(dir.path(), &["add", "large.txt"]);
git(dir.path(), &["commit", "-m", "large fixture"]);
let error = run_command(
dir.path(),
Program::Git,
&["show".into(), "HEAD:large.txt".into()],
)
.unwrap_err();
assert!(error.to_string().contains("no partial output was accepted"));
}
}