mirror of
https://github.com/codewhale-hq/Codewhale.git
synced 2026-11-11 01:51:34 -10:00
Merge feat/review-suggestions-20260829: committable review suggestions, fences neutralized
Safety: model text can no longer smuggle a committable suggestion fence through issue titles/descriptions; replacements render in local reports and degrade to plain blocks when posted; receipts record committable provenance (counts + anchor spans, never the text). Integrator verification on the merged tree: full tui lib suite 11,498 passed / 0 failed; 345/345 review tests; fmt clean; CI-exact clippy clean.
This commit is contained in:
CodeWhale Bot committed
commit
53cfc37011
4 files changed
+1558
-45
No files matched your search
+713
-43
@@ -8630,42 +8630,277 @@ fn run_gh_repo_name() -> Result<String> {
|
||||
Ok(name)
|
||||
}
|
||||
|
||||
/// True when `path` appears as a file touched by the unified diff. Guards the
|
||||
/// inline-comment payload: GitHub rejects the whole review with a 422 when a
|
||||
/// comment's path is not part of the diff.
|
||||
fn diff_touches_path(diff: &str, path: &str) -> bool {
|
||||
diff.contains(&format!("+++ b/{path}")) || diff.contains(&format!("--- a/{path}"))
|
||||
/// Pick a fence long enough to wrap `replacement` without the replacement's
|
||||
/// own backticks closing the block early.
|
||||
fn suggestion_fence(replacement: &str) -> String {
|
||||
let mut longest = 0usize;
|
||||
let mut run = 0usize;
|
||||
for ch in replacement.chars() {
|
||||
if ch == '`' {
|
||||
run += 1;
|
||||
longest = longest.max(run);
|
||||
} else {
|
||||
run = 0;
|
||||
}
|
||||
}
|
||||
"`".repeat(longest.saturating_add(1).max(3))
|
||||
}
|
||||
|
||||
/// GitHub turns any fenced `suggestion` block in a review comment into a
|
||||
/// one-click commit. Model prose is not vetted for that, so a fence the model
|
||||
/// wrote inside its own explanation is downgraded to a plain code block:
|
||||
/// only the `replacement` this function validated against the diff hunks may
|
||||
/// ever be committable.
|
||||
fn neutralize_model_suggestion_fences(prose: &str) -> String {
|
||||
prose
|
||||
.split('\n')
|
||||
.map(neutralize_suggestion_fence_line)
|
||||
.collect::<Vec<_>>()
|
||||
.join("\n")
|
||||
}
|
||||
|
||||
/// Neutralize one line's fence if it opens a `suggestion` block.
|
||||
///
|
||||
/// The fence may sit behind leading whitespace or behind a blockquote cue or
|
||||
/// list marker (`- ```suggestion`, `> ```suggestion`, `1. ```suggestion`).
|
||||
/// Whether GitHub renders those container-nested blocks applicable is
|
||||
/// unverified, so all of those shapes are treated as live and rewritten.
|
||||
fn neutralize_suggestion_fence_line(line: &str) -> String {
|
||||
let trimmed = line.trim_start();
|
||||
let indent = &line[..line.len() - trimmed.len()];
|
||||
let (prefix, fence) = split_container_prefix(trimmed);
|
||||
let fence_char = match fence.chars().next() {
|
||||
Some(ch @ ('`' | '~')) => ch,
|
||||
_ => return line.to_string(),
|
||||
};
|
||||
let ticks = fence.chars().take_while(|ch| *ch == fence_char).count();
|
||||
if ticks < 3 {
|
||||
return line.to_string();
|
||||
}
|
||||
let info = fence[ticks..].trim();
|
||||
if info.to_ascii_lowercase().starts_with("suggestion") {
|
||||
let fence: String = std::iter::repeat_n(fence_char, ticks).collect();
|
||||
format!("{indent}{prefix}{fence}text")
|
||||
} else {
|
||||
line.to_string()
|
||||
}
|
||||
}
|
||||
|
||||
/// Split the blockquote cues and list markers off the front of a line,
|
||||
/// returning `(prefix_to_preserve, rest)`. Nesting is followed (`> - `),
|
||||
/// but a line of ordinary prose is left untouched — the prefix only matters
|
||||
/// when what follows it is a fence.
|
||||
fn split_container_prefix(trimmed: &str) -> (&str, &str) {
|
||||
let mut rest = trimmed;
|
||||
while let Some(after) = strip_container_token(rest) {
|
||||
rest = after;
|
||||
}
|
||||
let split = trimmed.len() - rest.len();
|
||||
trimmed.split_at(split)
|
||||
}
|
||||
|
||||
/// Strip one container token (`> ` blockquote cue, `-`/`*`/`+` bullet, or an
|
||||
/// ordered-list marker) plus its trailing whitespace, or `None` when the
|
||||
/// line does not start with one.
|
||||
fn strip_container_token(rest: &str) -> Option<&str> {
|
||||
if let Some(after) = rest.strip_prefix('>') {
|
||||
return Some(after.trim_start_matches([' ', '\t']));
|
||||
}
|
||||
if let Some(after) = rest
|
||||
.strip_prefix(['-', '*', '+'])
|
||||
.filter(|after| after.starts_with([' ', '\t']))
|
||||
{
|
||||
return Some(after.trim_start_matches([' ', '\t']));
|
||||
}
|
||||
let digits = rest.chars().take_while(char::is_ascii_digit).count();
|
||||
if digits > 0 {
|
||||
let after_marker = &rest[digits..];
|
||||
if let Some(after) = after_marker
|
||||
.strip_prefix(['.', ')'])
|
||||
.filter(|after| after.starts_with([' ', '\t']))
|
||||
{
|
||||
return Some(after.trim_start_matches([' ', '\t']));
|
||||
}
|
||||
}
|
||||
None
|
||||
}
|
||||
|
||||
/// Map structured review issues to GitHub inline-review-comment payloads.
|
||||
///
|
||||
/// Anchors are checked against the diff's actual hunks, not just its file
|
||||
/// lists: GitHub 422s the *entire* review when one comment lands outside a
|
||||
/// hunk, so a model-estimated line that misses now drops a single comment.
|
||||
/// Issues without a locatable position stay in the summary body instead.
|
||||
fn inline_review_comments(
|
||||
///
|
||||
/// SAFETY: `title` and `description` are raw model text interpolated into a
|
||||
/// comment body. A model-written ```suggestion fence there would become a
|
||||
/// one-click-mergeable block that bypasses every span and size check, so
|
||||
/// both fields pass through `neutralize_model_suggestion_fences` — only a
|
||||
/// replacement validated against the diff hunks may ever be committable.
|
||||
/// (`severity` is safe unneutralized: it is normalized to a fixed
|
||||
/// error/warning/info vocabulary before it reaches here.)
|
||||
fn inline_issue_comments(
|
||||
review: &crate::tools::review::ReviewOutput,
|
||||
diff: &str,
|
||||
hunks: &crate::tools::review_hunks::DiffHunks,
|
||||
plan: &mut InlineReviewPlan,
|
||||
) -> Vec<serde_json::Value> {
|
||||
review
|
||||
.issues
|
||||
.iter()
|
||||
.filter_map(|issue| {
|
||||
let path = issue.path.as_deref()?.trim().trim_start_matches("./");
|
||||
if path.is_empty() || !diff_touches_path(diff, path) {
|
||||
let (Some(path), Some(line)) = (
|
||||
crate::tools::review::normalize_review_path(issue.path.as_deref()),
|
||||
issue.line,
|
||||
) else {
|
||||
// No position at all: the summary body is the only home.
|
||||
return None;
|
||||
};
|
||||
if !hunks.contains_line(&path, line) {
|
||||
plan.note_unanchorable(hunks, &path);
|
||||
return None;
|
||||
}
|
||||
let line = issue.line?;
|
||||
Some(serde_json::json!({
|
||||
"path": path,
|
||||
"line": line,
|
||||
"side": "RIGHT",
|
||||
"body": format!(
|
||||
"**[{}] {}**\n\n{}",
|
||||
issue.severity.to_uppercase(),
|
||||
issue.title,
|
||||
issue.description
|
||||
neutralize_model_suggestion_fences(&issue.title),
|
||||
neutralize_model_suggestion_fences(&issue.description)
|
||||
),
|
||||
}))
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
/// Render one structured suggestion as an inline review comment.
|
||||
///
|
||||
/// A committable ```` ```suggestion ```` block is emitted only when **both**
|
||||
/// safety conditions hold:
|
||||
///
|
||||
/// 1. the model supplied literal `replacement` code (not prose), and
|
||||
/// 2. every line of the replaced span is a RIGHT-side line inside a diff hunk
|
||||
/// (never a deleted LEFT-side line, which GitHub rejects), and the span is
|
||||
/// small enough to be a mechanical fix.
|
||||
///
|
||||
/// Otherwise the comment degrades to prose at the same anchor — a wrong
|
||||
/// committable suggestion is worse than prose because it is one click from
|
||||
/// being merged. With no valid anchor at all the suggestion stays in the
|
||||
/// summary body and `None` is returned.
|
||||
fn inline_suggestion_comment(
|
||||
suggestion: &crate::tools::review::ReviewSuggestion,
|
||||
hunks: &crate::tools::review_hunks::DiffHunks,
|
||||
plan: &mut InlineReviewPlan,
|
||||
) -> Option<serde_json::Value> {
|
||||
// The committable decision lives in `resolve_suggestion_anchor` so the
|
||||
// review receipt records exactly what this path emits for the same diff.
|
||||
let (path, start, end, committable) =
|
||||
match crate::tools::review::resolve_suggestion_anchor(suggestion, hunks) {
|
||||
crate::tools::review::SuggestionAnchor::Anchored {
|
||||
path,
|
||||
start,
|
||||
end,
|
||||
committable,
|
||||
} => (path, start, end, committable),
|
||||
crate::tools::review::SuggestionAnchor::Unanchorable { path } => {
|
||||
plan.note_unanchorable(hunks, &path);
|
||||
return None;
|
||||
}
|
||||
crate::tools::review::SuggestionAnchor::NoPosition => return None,
|
||||
};
|
||||
let prose = if suggestion.suggestion.is_empty() {
|
||||
"Suggested change.".to_string()
|
||||
} else {
|
||||
neutralize_model_suggestion_fences(&suggestion.suggestion)
|
||||
};
|
||||
|
||||
let Some(replacement) = suggestion.replacement.as_deref().filter(|_| committable) else {
|
||||
// Degradation path: keep the finding, drop the one-click apply.
|
||||
plan.degraded_to_prose += 1;
|
||||
return Some(serde_json::json!({
|
||||
"path": path,
|
||||
"line": end,
|
||||
"side": "RIGHT",
|
||||
"body": prose,
|
||||
}));
|
||||
};
|
||||
|
||||
let fence = suggestion_fence(replacement);
|
||||
let body = format!("{prose}\n\n{fence}suggestion\n{replacement}\n{fence}");
|
||||
let mut comment = serde_json::json!({
|
||||
"path": path,
|
||||
"line": end,
|
||||
"side": "RIGHT",
|
||||
"body": body,
|
||||
});
|
||||
if start < end {
|
||||
comment["start_line"] = serde_json::json!(start);
|
||||
comment["start_side"] = serde_json::json!("RIGHT");
|
||||
}
|
||||
Some(comment)
|
||||
}
|
||||
|
||||
/// The inline-comment payload for one review, plus a truthful count of what
|
||||
/// did not survive anchoring. Findings are never silently discarded: the
|
||||
/// counts are reported on stderr next to the posted review.
|
||||
#[derive(Debug, Default)]
|
||||
struct InlineReviewPlan {
|
||||
comments: Vec<serde_json::Value>,
|
||||
/// Findings whose file is in the diff but whose line is not inside any
|
||||
/// hunk — a model-estimated line number that missed.
|
||||
dropped_out_of_hunk: usize,
|
||||
/// Findings pointing at a file this diff does not touch at all.
|
||||
dropped_untouched_file: usize,
|
||||
/// Suggestions posted as prose because committing them was not safe.
|
||||
degraded_to_prose: usize,
|
||||
}
|
||||
|
||||
impl InlineReviewPlan {
|
||||
fn note_unanchorable(&mut self, hunks: &crate::tools::review_hunks::DiffHunks, path: &str) {
|
||||
if hunks.touches_path(path) {
|
||||
self.dropped_out_of_hunk += 1;
|
||||
} else {
|
||||
self.dropped_untouched_file += 1;
|
||||
}
|
||||
}
|
||||
|
||||
/// One-line receipt, or `None` when every finding landed as intended.
|
||||
fn receipt(&self) -> Option<String> {
|
||||
if self.dropped_out_of_hunk == 0
|
||||
&& self.dropped_untouched_file == 0
|
||||
&& self.degraded_to_prose == 0
|
||||
{
|
||||
return None;
|
||||
}
|
||||
Some(format!(
|
||||
"note: {} finding(s) had no line inside a diff hunk, {} pointed at a file \
|
||||
outside the diff (both stay in the summary body), and {} suggestion(s) \
|
||||
posted as prose instead of a committable block",
|
||||
self.dropped_out_of_hunk, self.dropped_untouched_file, self.degraded_to_prose
|
||||
))
|
||||
}
|
||||
}
|
||||
|
||||
/// Every inline comment for a review: issues first, then suggestions (which
|
||||
/// carry the committable ```` ```suggestion ```` blocks).
|
||||
fn plan_inline_review_comments(
|
||||
review: &crate::tools::review::ReviewOutput,
|
||||
diff: &str,
|
||||
) -> InlineReviewPlan {
|
||||
let hunks = crate::tools::review_hunks::DiffHunks::parse(diff);
|
||||
let mut plan = InlineReviewPlan::default();
|
||||
let mut comments = inline_issue_comments(review, &hunks, &mut plan);
|
||||
comments.extend(
|
||||
review
|
||||
.suggestions
|
||||
.iter()
|
||||
.filter_map(|suggestion| inline_suggestion_comment(suggestion, &hunks, &mut plan)),
|
||||
);
|
||||
plan.comments = comments;
|
||||
plan
|
||||
}
|
||||
|
||||
/// Render a structured review as the markdown body printed for — and, with
|
||||
/// `--post`, attached to — a pull-request review.
|
||||
fn render_pr_review_markdown(
|
||||
@@ -8720,6 +8955,29 @@ fn render_pr_review_markdown(
|
||||
} else {
|
||||
body.push_str(&format!("- {location} — {}\n", suggestion.suggestion));
|
||||
}
|
||||
// The replacement is the computed fix itself; rendering only the
|
||||
// prose used to compute and validate it, then throw it away.
|
||||
// Show it as a fenced block indented into the list item. In the
|
||||
// local report the fence is live — the user asked for the fix and
|
||||
// this block is the artifact to apply. In a *posted* PR body it
|
||||
// degrades to a plain code block: GitHub must only ever be handed
|
||||
// a one-click suggestion block this pipeline validated against
|
||||
// the diff hunks (the inline suggestion comments), never one the
|
||||
// summary duplicated from a suggestion that failed anchoring or
|
||||
// the span gates.
|
||||
if let Some(replacement) = suggestion
|
||||
.replacement
|
||||
.as_deref()
|
||||
.filter(|replacement| !replacement.trim().is_empty())
|
||||
{
|
||||
let fence = suggestion_fence(replacement);
|
||||
let info = if posted { "text" } else { "suggestion" };
|
||||
body.push_str(&format!("\n {fence}{info}\n"));
|
||||
for line in replacement.split('\n') {
|
||||
body.push_str(&format!(" {line}\n"));
|
||||
}
|
||||
body.push_str(&format!(" {fence}\n"));
|
||||
}
|
||||
}
|
||||
body.push('\n');
|
||||
}
|
||||
@@ -8732,7 +8990,8 @@ fn render_pr_review_markdown(
|
||||
body.push_str(&format!(
|
||||
"---\n*Advisory review by Codewhale (`codewhale review --pr {number} --post`, \
|
||||
head `{head}`). Line-specific findings are also posted as inline review \
|
||||
comments. CODEOWNERS approval still governs merge.*\n",
|
||||
comments; mechanical fixes arrive as committable suggestions you can \
|
||||
apply from the Files tab. CODEOWNERS approval still governs merge.*\n",
|
||||
head = if view.head_sha.is_empty() {
|
||||
"unknown"
|
||||
} else {
|
||||
@@ -8806,16 +9065,27 @@ fn post_pr_review(
|
||||
Some(repo) => repo.to_string(),
|
||||
None => run_gh_repo_name()?,
|
||||
};
|
||||
let inline = inline_review_comments(review, diff);
|
||||
let plan = plan_inline_review_comments(review, diff);
|
||||
if let Some(receipt) = plan.receipt() {
|
||||
eprintln!("{receipt}");
|
||||
}
|
||||
let inline = plan.comments;
|
||||
let body = render_pr_review_markdown(number, view, review, true);
|
||||
if inline.is_empty() {
|
||||
run_gh_post_pr_review(&repo_name, number, &body, &view.head_sha, &[])?;
|
||||
} else if let Err(err) =
|
||||
run_gh_post_pr_review(&repo_name, number, &body, &view.head_sha, &inline)
|
||||
{
|
||||
// One model-estimated line outside the diff hunks makes GitHub reject
|
||||
// the whole review; retry summary-only rather than lose the review.
|
||||
eprintln!("warning: inline review comments rejected ({err}); retrying summary-only");
|
||||
// Anchors are pre-filtered against the parsed diff hunks, so this
|
||||
// path should now be unreachable in the common case. It stays as a
|
||||
// last resort for anchors GitHub rejects for reasons the diff cannot
|
||||
// show (a stale head SHA, a suppressed large file), and it announces
|
||||
// exactly how many inline comments were lost rather than failing
|
||||
// silently.
|
||||
eprintln!(
|
||||
"warning: {} inline review comment(s) rejected ({err}); retrying summary-only",
|
||||
inline.len()
|
||||
);
|
||||
run_gh_post_pr_review(&repo_name, number, &body, &view.head_sha, &[])?;
|
||||
}
|
||||
Ok(())
|
||||
@@ -14713,38 +14983,438 @@ reasoning = "high"
|
||||
assert!(!serialized.contains("local-test-key"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn inline_review_comments_keep_only_diff_locatable_issues() {
|
||||
let issue = |severity: &str, path: Option<&str>, line: Option<u32>| {
|
||||
crate::tools::review::ReviewIssue {
|
||||
severity: severity.to_string(),
|
||||
title: format!("{severity} finding"),
|
||||
description: "detail".to_string(),
|
||||
path: path.map(str::to_string),
|
||||
line,
|
||||
}
|
||||
};
|
||||
let review = crate::tools::review::ReviewOutput {
|
||||
fn review_issue(
|
||||
severity: &str,
|
||||
path: Option<&str>,
|
||||
line: Option<u32>,
|
||||
) -> crate::tools::review::ReviewIssue {
|
||||
crate::tools::review::ReviewIssue {
|
||||
severity: severity.to_string(),
|
||||
title: format!("{severity} finding"),
|
||||
description: "detail".to_string(),
|
||||
path: path.map(str::to_string),
|
||||
line,
|
||||
}
|
||||
}
|
||||
|
||||
fn review_suggestion(
|
||||
path: Option<&str>,
|
||||
line: Option<u32>,
|
||||
replacement: Option<&str>,
|
||||
) -> crate::tools::review::ReviewSuggestion {
|
||||
crate::tools::review::ReviewSuggestion {
|
||||
path: path.map(str::to_string),
|
||||
line,
|
||||
start_line: None,
|
||||
end_line: None,
|
||||
suggestion: "Use the checked variant".to_string(),
|
||||
replacement: replacement.map(str::to_string),
|
||||
}
|
||||
}
|
||||
|
||||
fn review_with(
|
||||
issues: Vec<crate::tools::review::ReviewIssue>,
|
||||
suggestions: Vec<crate::tools::review::ReviewSuggestion>,
|
||||
) -> crate::tools::review::ReviewOutput {
|
||||
crate::tools::review::ReviewOutput {
|
||||
summary: "summary".to_string(),
|
||||
issues: vec![
|
||||
issue("error", Some("./crates/tui/src/lib.rs"), Some(10)),
|
||||
issue("warning", Some("docs/NOT_IN_DIFF.md"), Some(3)),
|
||||
issue("info", Some("crates/tui/src/lib.rs"), None),
|
||||
issue("error", None, Some(7)),
|
||||
],
|
||||
suggestions: Vec::new(),
|
||||
issues,
|
||||
suggestions,
|
||||
overall_assessment: String::new(),
|
||||
};
|
||||
let diff = "diff --git a/crates/tui/src/lib.rs b/crates/tui/src/lib.rs\n\
|
||||
index 111..222 100644\n\
|
||||
--- a/crates/tui/src/lib.rs\n\
|
||||
+++ b/crates/tui/src/lib.rs\n";
|
||||
let comments = inline_review_comments(&review, diff);
|
||||
}
|
||||
}
|
||||
|
||||
/// Two hunks in one file: right lines 10..=12 and 40..=41.
|
||||
///
|
||||
/// Built from a slice, not one string literal: a `\` continuation strips
|
||||
/// the leading space that marks a context line.
|
||||
fn review_test_diff() -> String {
|
||||
[
|
||||
"diff --git a/crates/tui/src/lib.rs b/crates/tui/src/lib.rs",
|
||||
"index 1111111..2222222 100644",
|
||||
"--- a/crates/tui/src/lib.rs",
|
||||
"+++ b/crates/tui/src/lib.rs",
|
||||
"@@ -10,2 +10,3 @@ fn head() {",
|
||||
" let a = 1;",
|
||||
"+let b = a.unwrap();",
|
||||
" let c = 2;",
|
||||
"@@ -40,2 +40,2 @@ fn tail() {",
|
||||
" let d = 3;",
|
||||
"-let e = d;",
|
||||
"+let e = d + 1;",
|
||||
"",
|
||||
]
|
||||
.join("\n")
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn inline_review_comments_keep_only_hunk_locatable_issues() {
|
||||
let review = review_with(
|
||||
vec![
|
||||
review_issue("error", Some("./crates/tui/src/lib.rs"), Some(11)),
|
||||
review_issue("warning", Some("docs/NOT_IN_DIFF.md"), Some(3)),
|
||||
review_issue("info", Some("crates/tui/src/lib.rs"), None),
|
||||
review_issue("error", None, Some(7)),
|
||||
],
|
||||
Vec::new(),
|
||||
);
|
||||
let comments = plan_inline_review_comments(&review, &review_test_diff()).comments;
|
||||
// GitHub rejects the entire review (422) when a comment's path is not
|
||||
// part of the diff, and inline comments need a line to anchor to.
|
||||
assert_eq!(comments.len(), 1);
|
||||
assert_eq!(comments[0]["path"], "crates/tui/src/lib.rs");
|
||||
assert_eq!(comments[0]["line"], 10);
|
||||
assert_eq!(comments[0]["line"], 11);
|
||||
assert_eq!(comments[0]["side"], "RIGHT");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn inline_review_comments_drop_one_out_of_hunk_anchor_not_the_whole_review() {
|
||||
// Line 25 is inside the file but between the two hunks. Before the
|
||||
// hunk filter this single bad anchor 422'd the review and every inline
|
||||
// comment was lost to the summary-only retry.
|
||||
let review = review_with(
|
||||
vec![
|
||||
review_issue("error", Some("crates/tui/src/lib.rs"), Some(11)),
|
||||
review_issue("error", Some("crates/tui/src/lib.rs"), Some(25)),
|
||||
review_issue("warning", Some("crates/tui/src/lib.rs"), Some(41)),
|
||||
],
|
||||
Vec::new(),
|
||||
);
|
||||
let plan = plan_inline_review_comments(&review, &review_test_diff());
|
||||
let lines: Vec<_> = plan.comments.iter().map(|c| c["line"].clone()).collect();
|
||||
assert_eq!(lines, vec![serde_json::json!(11), serde_json::json!(41)]);
|
||||
// The loss is counted and reported, never silent.
|
||||
assert_eq!(plan.dropped_out_of_hunk, 1);
|
||||
assert_eq!(plan.dropped_untouched_file, 0);
|
||||
assert!(
|
||||
plan.receipt()
|
||||
.expect("receipt")
|
||||
.contains("no line inside a diff hunk")
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn review_suggestion_with_replacement_posts_a_committable_block() {
|
||||
let review = review_with(
|
||||
Vec::new(),
|
||||
vec![review_suggestion(
|
||||
Some("crates/tui/src/lib.rs"),
|
||||
Some(11),
|
||||
Some("let b = a.unwrap_or_default();"),
|
||||
)],
|
||||
);
|
||||
let comments = plan_inline_review_comments(&review, &review_test_diff()).comments;
|
||||
assert_eq!(comments.len(), 1);
|
||||
let body = comments[0]["body"].as_str().expect("body");
|
||||
assert!(body.starts_with("Use the checked variant"), "{body}");
|
||||
assert!(
|
||||
body.contains("```suggestion\nlet b = a.unwrap_or_default();\n```"),
|
||||
"{body}"
|
||||
);
|
||||
assert_eq!(comments[0]["side"], "RIGHT");
|
||||
// Single-line suggestions must not carry start_line.
|
||||
assert!(comments[0].get("start_line").is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn multi_line_review_suggestion_spans_start_line_to_line() {
|
||||
let mut suggestion = review_suggestion(
|
||||
Some("crates/tui/src/lib.rs"),
|
||||
Some(12),
|
||||
Some("let b = a.unwrap_or_default();\nlet c = 2;"),
|
||||
);
|
||||
suggestion.start_line = Some(11);
|
||||
suggestion.end_line = Some(12);
|
||||
let review = review_with(Vec::new(), vec![suggestion]);
|
||||
let comments = plan_inline_review_comments(&review, &review_test_diff()).comments;
|
||||
assert_eq!(comments.len(), 1);
|
||||
assert_eq!(comments[0]["start_line"], 11);
|
||||
assert_eq!(comments[0]["start_side"], "RIGHT");
|
||||
assert_eq!(comments[0]["line"], 12);
|
||||
assert_eq!(comments[0]["side"], "RIGHT");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn review_suggestion_without_replacement_degrades_to_prose() {
|
||||
// SAFETY: a committable suggestion is one click from being merged, so
|
||||
// a suggestion the model did not back with literal code must never
|
||||
// render as an applicable block.
|
||||
let review = review_with(
|
||||
Vec::new(),
|
||||
vec![review_suggestion(
|
||||
Some("crates/tui/src/lib.rs"),
|
||||
Some(11),
|
||||
None,
|
||||
)],
|
||||
);
|
||||
let plan = plan_inline_review_comments(&review, &review_test_diff());
|
||||
assert_eq!(plan.comments.len(), 1);
|
||||
let body = plan.comments[0]["body"].as_str().expect("body");
|
||||
assert_eq!(body, "Use the checked variant");
|
||||
assert!(!body.contains("```suggestion"), "{body}");
|
||||
assert_eq!(plan.degraded_to_prose, 1);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn review_suggestion_spanning_outside_a_hunk_degrades_to_prose() {
|
||||
// Line 12 is in a hunk but line 13 is not, so the span cannot be
|
||||
// committed. Keep the finding, drop the one-click apply.
|
||||
let mut suggestion = review_suggestion(
|
||||
Some("crates/tui/src/lib.rs"),
|
||||
Some(12),
|
||||
Some("let c = 2;\nlet d = 3;"),
|
||||
);
|
||||
suggestion.start_line = Some(12);
|
||||
suggestion.end_line = Some(13);
|
||||
let review = review_with(Vec::new(), vec![suggestion]);
|
||||
let comments = plan_inline_review_comments(&review, &review_test_diff()).comments;
|
||||
// end_line 13 is outside every hunk, so there is no anchor at all.
|
||||
assert!(comments.is_empty());
|
||||
|
||||
// Same replacement anchored at an in-hunk end line but with a start
|
||||
// line outside the hunk: anchor is valid, span is not -> prose.
|
||||
let mut suggestion = review_suggestion(
|
||||
Some("crates/tui/src/lib.rs"),
|
||||
Some(11),
|
||||
Some("let a = 1;\nlet b = a.unwrap_or_default();"),
|
||||
);
|
||||
suggestion.start_line = Some(9);
|
||||
suggestion.end_line = Some(11);
|
||||
let review = review_with(Vec::new(), vec![suggestion]);
|
||||
let comments = plan_inline_review_comments(&review, &review_test_diff()).comments;
|
||||
assert_eq!(comments.len(), 1);
|
||||
let body = comments[0]["body"].as_str().expect("body");
|
||||
assert!(!body.contains("```suggestion"), "{body}");
|
||||
assert!(comments[0].get("start_line").is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn review_suggestion_anchored_to_a_deleted_line_is_not_committable() {
|
||||
// Right line 41 is `+let e = d + 1;`; the deleted `-let e = d;` has no
|
||||
// RIGHT-side number, so a model that anchors at the pre-image line 42
|
||||
// gets nothing rather than a suggestion on the wrong line.
|
||||
let review = review_with(
|
||||
Vec::new(),
|
||||
vec![review_suggestion(
|
||||
Some("crates/tui/src/lib.rs"),
|
||||
Some(42),
|
||||
Some("let e = d + 2;"),
|
||||
)],
|
||||
);
|
||||
assert!(
|
||||
plan_inline_review_comments(&review, &review_test_diff())
|
||||
.comments
|
||||
.is_empty()
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn review_suggestion_replacement_containing_backticks_is_fenced_safely() {
|
||||
let review = review_with(
|
||||
Vec::new(),
|
||||
vec![review_suggestion(
|
||||
Some("crates/tui/src/lib.rs"),
|
||||
Some(11),
|
||||
Some("let b = \"```\";"),
|
||||
)],
|
||||
);
|
||||
let comments = plan_inline_review_comments(&review, &review_test_diff()).comments;
|
||||
let body = comments[0]["body"].as_str().expect("body");
|
||||
assert!(body.contains("````suggestion\n"), "{body}");
|
||||
assert!(body.ends_with("\n````"), "{body}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn review_suggestion_multi_line_replacement_without_span_degrades_to_prose() {
|
||||
// SAFETY: with no start_line/end_line, GitHub would insert these two
|
||||
// lines at line 11 and leave the original line 11 duplicated below.
|
||||
let review = review_with(
|
||||
Vec::new(),
|
||||
vec![review_suggestion(
|
||||
Some("crates/tui/src/lib.rs"),
|
||||
Some(11),
|
||||
Some("let b = a.unwrap_or_default();\nlet c = 2;"),
|
||||
)],
|
||||
);
|
||||
let plan = plan_inline_review_comments(&review, &review_test_diff());
|
||||
assert_eq!(plan.comments.len(), 1);
|
||||
assert!(
|
||||
!plan.comments[0]["body"]
|
||||
.as_str()
|
||||
.expect("body")
|
||||
.contains("```suggestion")
|
||||
);
|
||||
assert_eq!(plan.degraded_to_prose, 1);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn review_suggestion_prose_fence_is_never_committable() {
|
||||
// SAFETY: only the replacement this reviewer validated against the
|
||||
// diff may be one click from merging. A fence the model wrote inside
|
||||
// its explanation is downgraded to a plain code block.
|
||||
let mut suggestion = review_suggestion(Some("crates/tui/src/lib.rs"), Some(11), None);
|
||||
suggestion.suggestion =
|
||||
"Try this:\n\n```suggestion\nlet b = a.unwrap_or_default();\n```".to_string();
|
||||
let review = review_with(Vec::new(), vec![suggestion]);
|
||||
let comments = plan_inline_review_comments(&review, &review_test_diff()).comments;
|
||||
let body = comments[0]["body"].as_str().expect("body");
|
||||
assert!(!body.contains("```suggestion"), "{body}");
|
||||
assert!(body.contains("```text"), "{body}");
|
||||
assert!(body.contains("let b = a.unwrap_or_default();"), "{body}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn review_suggestion_tilde_prose_fence_is_neutralized() {
|
||||
let mut suggestion = review_suggestion(Some("crates/tui/src/lib.rs"), Some(11), None);
|
||||
suggestion.suggestion = " ~~~suggestion\n x\n ~~~".to_string();
|
||||
let review = review_with(Vec::new(), vec![suggestion]);
|
||||
let comments = plan_inline_review_comments(&review, &review_test_diff()).comments;
|
||||
let body = comments[0]["body"].as_str().expect("body");
|
||||
assert!(body.contains(" ~~~text"), "{body}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn suggestion_fence_behind_list_marker_or_blockquote_is_neutralized() {
|
||||
// Whether GitHub renders a fence nested behind a list marker,
|
||||
// blockquote cue, or ordered-list marker applicable is unverified,
|
||||
// so those shapes are treated as live and downgraded too. Ordinary
|
||||
// prose lines that merely start with a marker stay untouched.
|
||||
let neutralized = neutralize_model_suggestion_fences(
|
||||
"```suggestion\n- ```suggestion\n rm -rf /\n> ```suggestion\n1. ```suggestion\n> - ~~~suggestion\n- fix the `foo` call",
|
||||
);
|
||||
assert_eq!(
|
||||
neutralized,
|
||||
"```text\n- ```text\n rm -rf /\n> ```text\n1. ```text\n> - ~~~text\n- fix the `foo` call"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn inline_issue_comment_neutralizes_model_suggestion_fences_in_issue_text() {
|
||||
// SAFETY: regression for a proven hole — the issue title and
|
||||
// description are raw model text, and a ```suggestion fence in
|
||||
// either used to reach the inline comment body verbatim: a
|
||||
// one-click mergeable block that bypassed every span and size
|
||||
// check. Only a replacement validated against the diff hunks may
|
||||
// ever be committable.
|
||||
let mut fenced_description = review_issue("error", Some("crates/tui/src/lib.rs"), Some(11));
|
||||
fenced_description.title = "Cleanup script".to_string();
|
||||
fenced_description.description = "Run this:\n\n```suggestion\nrm -rf /\n```\n".to_string();
|
||||
let mut fenced_title = review_issue("warning", Some("crates/tui/src/lib.rs"), Some(12));
|
||||
fenced_title.title = "Fix all\n```suggestion\nrm -rf ~\n```".to_string();
|
||||
fenced_title.description = "detail".to_string();
|
||||
let review = review_with(vec![fenced_description, fenced_title], Vec::new());
|
||||
|
||||
let comments = plan_inline_review_comments(&review, &review_test_diff()).comments;
|
||||
assert_eq!(comments.len(), 2);
|
||||
for comment in &comments {
|
||||
let body = comment["body"].as_str().expect("body");
|
||||
assert!(!body.contains("```suggestion"), "{body}");
|
||||
}
|
||||
let description_body = comments[0]["body"].as_str().expect("body");
|
||||
assert!(
|
||||
description_body.contains("```text\nrm -rf /"),
|
||||
"{description_body}"
|
||||
);
|
||||
let title_body = comments[1]["body"].as_str().expect("body");
|
||||
assert!(title_body.contains("```text\nrm -rf ~"), "{title_body}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pr_review_markdown_shows_the_computed_replacement() {
|
||||
// A plain `codewhale review --pr` (no --post) computes and validates
|
||||
// the literal fix; the local report must show it, not just the prose.
|
||||
let view = GhPullRequest {
|
||||
title: "T".to_string(),
|
||||
body: String::new(),
|
||||
base: "main".to_string(),
|
||||
head: "feature".to_string(),
|
||||
url: "https://example.invalid/pr/1".to_string(),
|
||||
head_sha: "abc123".to_string(),
|
||||
};
|
||||
let mut single = review_suggestion(
|
||||
Some("crates/tui/src/lib.rs"),
|
||||
Some(11),
|
||||
Some("let b = a.unwrap_or_default();"),
|
||||
);
|
||||
single.suggestion = "Use the checked variant".to_string();
|
||||
let mut multi = review_suggestion(
|
||||
Some("crates/tui/src/lib.rs"),
|
||||
Some(11),
|
||||
Some("let b = a.unwrap_or_default();\nlet c = 2;"),
|
||||
);
|
||||
multi.suggestion = "Replace both lines".to_string();
|
||||
multi.start_line = Some(11);
|
||||
multi.end_line = Some(12);
|
||||
let review = review_with(Vec::new(), vec![single, multi]);
|
||||
|
||||
let local = render_pr_review_markdown(1, &view, &review, false);
|
||||
assert!(local.contains("### Suggestions"), "{local}");
|
||||
assert!(
|
||||
local.contains("\n ```suggestion\n let b = a.unwrap_or_default();\n ```\n"),
|
||||
"{local}"
|
||||
);
|
||||
assert!(
|
||||
local.contains(
|
||||
"\n ```suggestion\n let b = a.unwrap_or_default();\n let c = 2;\n ```\n"
|
||||
),
|
||||
"{local}"
|
||||
);
|
||||
|
||||
// The posted body keeps the fix visible but never as a live
|
||||
// one-click block: only diff-validated inline suggestion comments
|
||||
// may carry those to GitHub.
|
||||
let posted = render_pr_review_markdown(1, &view, &review, true);
|
||||
assert!(
|
||||
posted.contains("let b = a.unwrap_or_default();"),
|
||||
"{posted}"
|
||||
);
|
||||
assert!(!posted.contains("```suggestion"), "{posted}");
|
||||
assert!(posted.contains("```text"), "{posted}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn oversized_review_suggestion_degrades_to_prose() {
|
||||
use crate::tools::review::MAX_COMMITTABLE_SUGGESTION_LINES;
|
||||
|
||||
// A whole-hunk rewrite is not a mechanical fix, so it must not be
|
||||
// committable even though every line is inside the diff.
|
||||
let span = MAX_COMMITTABLE_SUGGESTION_LINES + 5;
|
||||
let added: String = (1..=span).map(|i| format!("+line {i}\n")).collect();
|
||||
let diff = format!(
|
||||
"diff --git a/big.rs b/big.rs\nnew file mode 100644\n--- /dev/null\n\
|
||||
+++ b/big.rs\n@@ -0,0 +1,{span} @@\n{added}"
|
||||
);
|
||||
let replacement: String = (1..=span)
|
||||
.map(|i| format!("line {i} fixed"))
|
||||
.collect::<Vec<_>>()
|
||||
.join("\n");
|
||||
|
||||
let mut suggestion = review_suggestion(Some("big.rs"), Some(span), Some(&replacement));
|
||||
suggestion.start_line = Some(1);
|
||||
suggestion.end_line = Some(span);
|
||||
let review = review_with(Vec::new(), vec![suggestion]);
|
||||
let comments = plan_inline_review_comments(&review, &diff).comments;
|
||||
assert_eq!(comments.len(), 1);
|
||||
let body = comments[0]["body"].as_str().expect("body");
|
||||
assert!(!body.contains("```suggestion"), "{body}");
|
||||
|
||||
// The same shape, trimmed to the budget, stays committable.
|
||||
let mut suggestion = review_suggestion(
|
||||
Some("big.rs"),
|
||||
Some(MAX_COMMITTABLE_SUGGESTION_LINES),
|
||||
Some("line 1 fixed"),
|
||||
);
|
||||
suggestion.start_line = Some(1);
|
||||
suggestion.end_line = Some(MAX_COMMITTABLE_SUGGESTION_LINES);
|
||||
let review = review_with(Vec::new(), vec![suggestion]);
|
||||
let comments = plan_inline_review_comments(&review, &diff).comments;
|
||||
assert!(
|
||||
comments[0]["body"]
|
||||
.as_str()
|
||||
.expect("body")
|
||||
.contains("```suggestion"),
|
||||
"a span at the budget limit stays committable"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -52,6 +52,7 @@ pub mod request_plugin_install;
|
||||
mod resource_admission;
|
||||
pub mod revert_turn;
|
||||
pub mod review;
|
||||
pub mod review_hunks;
|
||||
pub mod rlm;
|
||||
pub mod run_tool;
|
||||
pub mod runtime_mcp;
|
||||
|
||||
@@ -24,6 +24,11 @@ const DEFAULT_MAX_CHARS: usize = 200_000;
|
||||
const MAX_MAX_CHARS: usize = 1_000_000;
|
||||
const FALLBACK_MAX_CHARS: usize = 4000;
|
||||
const REVIEW_RECEIPT_SCHEMA_VERSION: u32 = 1;
|
||||
|
||||
/// Budget for how many lines a committable suggestion may replace. A
|
||||
/// mechanical fix is small; anything larger is judgement wearing a
|
||||
/// suggestion fence, so it must degrade to prose.
|
||||
pub const MAX_COMMITTABLE_SUGGESTION_LINES: u32 = 25;
|
||||
const REVIEW_CLIENT_UNAVAILABLE: &str = "Review tool requires an active Codewhale model client";
|
||||
|
||||
const REVIEW_SYSTEM_PROMPT: &str = "You are a senior code reviewer. Return ONLY valid JSON with \
|
||||
@@ -43,12 +48,22 @@ the following schema:\n\
|
||||
{\n\
|
||||
\"path\": \"relative/file/path or null\",\n\
|
||||
\"line\": 123,\n\
|
||||
\"suggestion\": \"actionable improvement\"\n\
|
||||
\"start_line\": 121,\n\
|
||||
\"end_line\": 123,\n\
|
||||
\"suggestion\": \"why this change is needed\",\n\
|
||||
\"replacement\": \"the exact literal lines that replace start_line..end_line\"\n\
|
||||
}\n\
|
||||
],\n\
|
||||
\"overall_assessment\": \"final assessment\"\n\
|
||||
}\n\
|
||||
If a field is unknown, use an empty string or null. Prioritize correctness and missing tests.";
|
||||
If a field is unknown, use an empty string or null. Prioritize correctness and missing tests.\n\
|
||||
\n\
|
||||
Rules for \"suggestions\":\n\
|
||||
- \"suggestion\" is prose explaining the change.\n\
|
||||
- \"replacement\" is NOT a description. It is the literal replacement source code, verbatim, with the exact indentation it must have in the file, and with no diff markers, no line numbers, and no fences. It replaces lines start_line..end_line (inclusive) of the NEW version of the file; when the change is a single line, set start_line == end_line == line.\n\
|
||||
- Supply \"replacement\" ONLY for a mechanical, high-confidence fix you are certain compiles and is correct as written (a typo, a wrong comparison operator, a missing await/unwrap guard, a renamed symbol, a wrong constant). Anything requiring judgement, new imports, or edits elsewhere in the file must omit \"replacement\" and stay prose-only.\n\
|
||||
- Anchor a suggestion only to lines that appear in the diff you were given, and never to a deleted line. If you are not sure of the exact line numbers, omit \"replacement\".\n\
|
||||
- A wrong replacement is worse than no replacement: it is one click from being merged. When in doubt, omit it.";
|
||||
|
||||
/// The system prompt shared by every structured review path (`review`
|
||||
/// tool and `codewhale review --pr`). Callers parse the reply with
|
||||
@@ -79,8 +94,22 @@ pub struct ReviewSuggestion {
|
||||
pub path: Option<String>,
|
||||
#[serde(default)]
|
||||
pub line: Option<u32>,
|
||||
/// First line of the replaced span (inclusive). `None` means the
|
||||
/// suggestion covers a single line, `line`.
|
||||
#[serde(default)]
|
||||
pub start_line: Option<u32>,
|
||||
/// Last line of the replaced span (inclusive). Defaults to `line`.
|
||||
#[serde(default)]
|
||||
pub end_line: Option<u32>,
|
||||
/// Prose: why the change is wanted.
|
||||
#[serde(default)]
|
||||
pub suggestion: String,
|
||||
/// Literal replacement source for `start_line..=end_line`, indentation
|
||||
/// included. `Some` only for mechanical, high-confidence fixes; when it
|
||||
/// is `None` the reviewer posts prose instead of a committable
|
||||
/// GitHub suggestion block.
|
||||
#[serde(default)]
|
||||
pub replacement: Option<String>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Serialize, Deserialize)]
|
||||
@@ -136,11 +165,96 @@ impl ReviewOutput {
|
||||
for suggestion in &mut self.suggestions {
|
||||
suggestion.suggestion = suggestion.suggestion.trim().to_string();
|
||||
suggestion.path = normalize_optional(suggestion.path.take());
|
||||
// Leading whitespace in `replacement` is load-bearing indentation,
|
||||
// so only trailing newlines and all-whitespace payloads are
|
||||
// normalized away.
|
||||
suggestion.replacement = suggestion
|
||||
.replacement
|
||||
.take()
|
||||
.map(|replacement| replacement.trim_end_matches(['\n', '\r']).to_string())
|
||||
.filter(|replacement| !replacement.trim().is_empty());
|
||||
}
|
||||
self
|
||||
}
|
||||
}
|
||||
|
||||
/// Resolve a model-supplied review path to the post-image form diff hunks
|
||||
/// are keyed by: trimmed, with any `./` prefix removed. `None` means the
|
||||
/// finding has no position at all.
|
||||
#[must_use]
|
||||
pub fn normalize_review_path(path: Option<&str>) -> Option<String> {
|
||||
let path = path?.trim().trim_start_matches("./");
|
||||
if path.is_empty() {
|
||||
return None;
|
||||
}
|
||||
Some(path.to_string())
|
||||
}
|
||||
|
||||
/// Where a suggestion can anchor in a diff, and whether its replacement may
|
||||
/// be emitted as a one-click committable block.
|
||||
///
|
||||
/// This is the single source of truth for "is this committable": the PR
|
||||
/// inline-comment path and the review receipt both derive from it, so a
|
||||
/// receipt can never claim a suggestion was committable while the posted
|
||||
/// comment degraded it to prose (or the reverse).
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub enum SuggestionAnchor {
|
||||
/// No path or no line at all: only the summary body can carry it.
|
||||
NoPosition,
|
||||
/// Path and line exist but no hunk contains the line — a model-estimated
|
||||
/// position that missed the diff.
|
||||
Unanchorable { path: String },
|
||||
/// Anchored to RIGHT-side `path:start..=end`. `committable` is true only
|
||||
/// when the literal replacement passed every safety gate: non-empty,
|
||||
/// within the span-size budget, an explicitly bounded (or single-line)
|
||||
/// span, and fully covered by RIGHT-side hunk lines.
|
||||
Anchored {
|
||||
path: String,
|
||||
start: u32,
|
||||
end: u32,
|
||||
committable: bool,
|
||||
},
|
||||
}
|
||||
|
||||
/// Resolve one suggestion against a diff's hunks.
|
||||
#[must_use]
|
||||
pub fn resolve_suggestion_anchor(
|
||||
suggestion: &ReviewSuggestion,
|
||||
hunks: &super::review_hunks::DiffHunks,
|
||||
) -> SuggestionAnchor {
|
||||
let Some(path) = normalize_review_path(suggestion.path.as_deref()) else {
|
||||
return SuggestionAnchor::NoPosition;
|
||||
};
|
||||
let Some(end) = suggestion.end_line.or(suggestion.line) else {
|
||||
return SuggestionAnchor::NoPosition;
|
||||
};
|
||||
if !hunks.contains_line(&path, end) {
|
||||
return SuggestionAnchor::Unanchorable { path };
|
||||
}
|
||||
let start = suggestion.start_line.unwrap_or(end);
|
||||
// A model that gives neither start_line nor end_line has told us nothing
|
||||
// about how much code it means to replace. GitHub would happily *insert*
|
||||
// a multi-line replacement at a single-line anchor, duplicating the lines
|
||||
// the model meant to replace, so that shape is not committable.
|
||||
let explicit_span = suggestion.start_line.is_some() || suggestion.end_line.is_some();
|
||||
let committable = suggestion
|
||||
.replacement
|
||||
.as_deref()
|
||||
.is_some_and(|replacement| {
|
||||
!replacement.trim().is_empty()
|
||||
&& start <= end
|
||||
&& end.saturating_sub(start).saturating_add(1) <= MAX_COMMITTABLE_SUGGESTION_LINES
|
||||
&& (explicit_span || start < end || !replacement.contains('\n'))
|
||||
&& hunks.contains_span(&path, start, end)
|
||||
});
|
||||
SuggestionAnchor::Anchored {
|
||||
path,
|
||||
start,
|
||||
end,
|
||||
committable,
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
|
||||
pub struct ReviewReceipt {
|
||||
pub schema_version: u32,
|
||||
@@ -171,6 +285,42 @@ pub struct ReviewReceiptFindings {
|
||||
pub suggestion_count: usize,
|
||||
pub highest_severity: String,
|
||||
pub issues: Vec<ReviewReceiptIssue>,
|
||||
/// What the suggestion pipeline would do with each suggestion against
|
||||
/// this diff. `#[serde(default)]` keeps receipts written before the
|
||||
/// field existed readable, so the schema version does not move.
|
||||
#[serde(default)]
|
||||
pub suggestions: ReviewReceiptSuggestions,
|
||||
}
|
||||
|
||||
/// One suggestion emitted as a one-click committable block: where it
|
||||
/// anchored, nothing else. The replacement text is deliberately absent —
|
||||
/// a receipt is an audit record, never a second channel for model-written
|
||||
/// code.
|
||||
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
|
||||
pub struct ReviewReceiptSuggestion {
|
||||
pub path: String,
|
||||
pub start_line: u32,
|
||||
pub end_line: u32,
|
||||
}
|
||||
|
||||
/// Receipt provenance for the suggestion pipeline. The three counters use
|
||||
/// the same [`SuggestionAnchor`] resolution as the posted inline comments,
|
||||
/// so the numbers a receipt records are exactly what the PR path would
|
||||
/// emit for the same diff.
|
||||
#[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq, Eq)]
|
||||
pub struct ReviewReceiptSuggestions {
|
||||
/// Suggestions emitted as committable blocks (one entry each, below).
|
||||
pub committable_count: usize,
|
||||
/// Anchor spans of the committable suggestions: path + line range, no
|
||||
/// replacement text.
|
||||
pub committable: Vec<ReviewReceiptSuggestion>,
|
||||
/// Suggestions whose anchor was valid but whose replacement failed a
|
||||
/// safety gate, so they posted as prose instead.
|
||||
pub degraded_to_prose: usize,
|
||||
/// Suggestions whose line missed every hunk (or whose file is not in
|
||||
/// the diff), so nothing was posted inline. Suggestions with no
|
||||
/// position at all are not counted — they never had an anchor to lose.
|
||||
pub dropped_unanchorable: usize,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
|
||||
@@ -198,6 +348,35 @@ pub struct ReviewReceiptValidation {
|
||||
pub unresolved_risk: Option<ReviewReceiptRisk>,
|
||||
}
|
||||
|
||||
/// Classify every suggestion in `output` against `diff` exactly as the PR
|
||||
/// inline-comment path would, producing the receipt's provenance counts.
|
||||
#[must_use]
|
||||
pub fn suggestion_provenance(output: &ReviewOutput, diff: &str) -> ReviewReceiptSuggestions {
|
||||
let hunks = super::review_hunks::DiffHunks::parse(diff);
|
||||
let mut suggestions = ReviewReceiptSuggestions::default();
|
||||
for suggestion in &output.suggestions {
|
||||
match resolve_suggestion_anchor(suggestion, &hunks) {
|
||||
SuggestionAnchor::Anchored {
|
||||
path,
|
||||
start,
|
||||
end,
|
||||
committable: true,
|
||||
} => suggestions.committable.push(ReviewReceiptSuggestion {
|
||||
path,
|
||||
start_line: start,
|
||||
end_line: end,
|
||||
}),
|
||||
SuggestionAnchor::Anchored {
|
||||
committable: false, ..
|
||||
} => suggestions.degraded_to_prose += 1,
|
||||
SuggestionAnchor::Unanchorable { .. } => suggestions.dropped_unanchorable += 1,
|
||||
SuggestionAnchor::NoPosition => {}
|
||||
}
|
||||
}
|
||||
suggestions.committable_count = suggestions.committable.len();
|
||||
suggestions
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn build_review_receipt(
|
||||
target: impl Into<String>,
|
||||
@@ -240,6 +419,7 @@ pub fn build_review_receipt(
|
||||
issue_count: output.issues.len(),
|
||||
suggestion_count: output.suggestions.len(),
|
||||
highest_severity: highest_severity.clone(),
|
||||
suggestions: suggestion_provenance(output, diff),
|
||||
issues: output
|
||||
.issues
|
||||
.iter()
|
||||
@@ -1113,7 +1293,10 @@ mod tests {
|
||||
suggestions: vec![ReviewSuggestion {
|
||||
path: Some("src/lib.rs".to_string()),
|
||||
line: Some(12),
|
||||
start_line: None,
|
||||
end_line: None,
|
||||
suggestion: "Add a regression test".to_string(),
|
||||
replacement: None,
|
||||
}],
|
||||
overall_assessment: "Needs a test".to_string(),
|
||||
};
|
||||
@@ -1150,6 +1333,127 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn review_receipt_records_committable_suggestion_provenance() {
|
||||
// Built from a slice, not one string literal: a `\` continuation
|
||||
// strips the leading space that marks a context line.
|
||||
let diff = [
|
||||
"diff --git a/src/lib.rs b/src/lib.rs",
|
||||
"--- a/src/lib.rs",
|
||||
"+++ b/src/lib.rs",
|
||||
"@@ -10,2 +10,3 @@ fn head() {",
|
||||
" let a = 1;",
|
||||
"+let b = a.unwrap();",
|
||||
" let c = 2;",
|
||||
"",
|
||||
]
|
||||
.join("\n");
|
||||
let output = ReviewOutput {
|
||||
summary: "One fix, one judgement call, one miss".to_string(),
|
||||
issues: Vec::new(),
|
||||
suggestions: vec![
|
||||
// Valid anchor, explicit in-hunk span, literal replacement:
|
||||
// the only shape that may become a committable block.
|
||||
ReviewSuggestion {
|
||||
path: Some("src/lib.rs".to_string()),
|
||||
line: Some(12),
|
||||
start_line: Some(11),
|
||||
end_line: Some(12),
|
||||
suggestion: "Use the checked variant".to_string(),
|
||||
replacement: Some("let b = a.unwrap_or_default();\nlet c = 2;".to_string()),
|
||||
},
|
||||
// Valid anchor, no literal replacement: degrades to prose.
|
||||
ReviewSuggestion {
|
||||
path: Some("src/lib.rs".to_string()),
|
||||
line: Some(11),
|
||||
start_line: None,
|
||||
end_line: None,
|
||||
suggestion: "Add a test".to_string(),
|
||||
replacement: None,
|
||||
},
|
||||
// Line 25 sits between hunks: unanchorable.
|
||||
ReviewSuggestion {
|
||||
path: Some("src/lib.rs".to_string()),
|
||||
line: Some(25),
|
||||
start_line: None,
|
||||
end_line: None,
|
||||
suggestion: "Wrong line".to_string(),
|
||||
replacement: Some("x = 1;".to_string()),
|
||||
},
|
||||
// No position at all: summary-body only, counted nowhere here.
|
||||
ReviewSuggestion {
|
||||
path: None,
|
||||
line: None,
|
||||
start_line: None,
|
||||
end_line: None,
|
||||
suggestion: "Consider renaming".to_string(),
|
||||
replacement: None,
|
||||
},
|
||||
],
|
||||
overall_assessment: "Fix the unwrap".to_string(),
|
||||
};
|
||||
|
||||
let receipt = build_review_receipt(
|
||||
"working-tree",
|
||||
&diff,
|
||||
"deepseek",
|
||||
"deepseek-v4-pro",
|
||||
&output,
|
||||
"review body",
|
||||
Vec::new(),
|
||||
);
|
||||
|
||||
let provenance = &receipt.findings.suggestions;
|
||||
assert_eq!(provenance.committable_count, 1);
|
||||
assert_eq!(
|
||||
provenance.committable,
|
||||
vec![ReviewReceiptSuggestion {
|
||||
path: "src/lib.rs".to_string(),
|
||||
start_line: 11,
|
||||
end_line: 12,
|
||||
}]
|
||||
);
|
||||
assert_eq!(provenance.degraded_to_prose, 1);
|
||||
assert_eq!(provenance.dropped_unanchorable, 1);
|
||||
assert_eq!(receipt.findings.suggestion_count, 4);
|
||||
|
||||
// Provenance records anchors, never code: the replacement text must
|
||||
// not leak into the receipt.
|
||||
let serialized = serde_json::to_string(&receipt).expect("serialize receipt");
|
||||
assert!(!serialized.contains("unwrap_or_default"), "{serialized}");
|
||||
assert!(!serialized.contains("x = 1;"), "{serialized}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn review_receipt_without_suggestion_provenance_still_decodes() {
|
||||
// Receipts written before the provenance field existed are schema v1;
|
||||
// the field is additive and serde-defaulted, so they must keep
|
||||
// decoding and the schema version must not move.
|
||||
let output = ReviewOutput::from_str("Looks good");
|
||||
let receipt = build_review_receipt(
|
||||
"working-tree",
|
||||
"diff --git a/a b/a\n",
|
||||
"deepseek",
|
||||
"deepseek-v4-flash",
|
||||
&output,
|
||||
"Looks good",
|
||||
Vec::new(),
|
||||
);
|
||||
let mut value = serde_json::to_value(&receipt).expect("serialize");
|
||||
value
|
||||
.get_mut("findings")
|
||||
.expect("findings")
|
||||
.as_object_mut()
|
||||
.expect("findings object")
|
||||
.remove("suggestions");
|
||||
let legacy: ReviewReceipt = serde_json::from_value(value).expect("legacy receipt decodes");
|
||||
assert_eq!(legacy.schema_version, REVIEW_RECEIPT_SCHEMA_VERSION);
|
||||
assert_eq!(
|
||||
legacy.findings.suggestions,
|
||||
ReviewReceiptSuggestions::default()
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn write_review_receipt_accepts_override_path() {
|
||||
let dir = tempfile::tempdir().expect("tempdir");
|
||||
|
||||
@@ -0,0 +1,538 @@
|
||||
//! Unified-diff hunk parsing for GitHub review anchors.
|
||||
//!
|
||||
//! GitHub's "create a review" API rejects the *entire* review with a 422 when
|
||||
//! any single inline comment anchors to a line that is not part of the diff.
|
||||
//! Before this module existed the reviewer only checked that the *file* was
|
||||
//! touched, so one model-estimated line number silently discarded every
|
||||
//! inline comment (the summary-only retry in `post_pr_review` hid the loss).
|
||||
//!
|
||||
//! [`DiffHunks::parse`] turns a unified diff into the exact set of RIGHT-side
|
||||
//! (post-image) line numbers GitHub will accept per file, so a bad anchor
|
||||
//! drops one comment instead of the whole review.
|
||||
|
||||
use std::collections::BTreeMap;
|
||||
|
||||
/// A contiguous run of RIGHT-side line numbers, inclusive on both ends.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
struct LineRange {
|
||||
start: u32,
|
||||
end: u32,
|
||||
}
|
||||
|
||||
/// The RIGHT-side lines a unified diff exposes, keyed by post-image path.
|
||||
#[derive(Debug, Clone, Default, PartialEq, Eq)]
|
||||
pub struct DiffHunks {
|
||||
files: BTreeMap<String, FileHunks>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Default, PartialEq, Eq)]
|
||||
struct FileHunks {
|
||||
/// Every RIGHT-side line inside a hunk: context lines plus added lines.
|
||||
/// These are exactly the lines GitHub accepts with `side: "RIGHT"`.
|
||||
commentable: Vec<LineRange>,
|
||||
}
|
||||
|
||||
/// Cursor state while walking a hunk body.
|
||||
#[derive(Debug, Clone, Copy, Default)]
|
||||
struct HunkCursor {
|
||||
right_line: u32,
|
||||
remaining_old: u32,
|
||||
remaining_new: u32,
|
||||
}
|
||||
|
||||
impl HunkCursor {
|
||||
fn exhausted(self) -> bool {
|
||||
self.remaining_old == 0 && self.remaining_new == 0
|
||||
}
|
||||
}
|
||||
|
||||
impl DiffHunks {
|
||||
/// Parse a unified diff (`git diff`, `gh pr diff`) into per-file
|
||||
/// RIGHT-side line coverage.
|
||||
///
|
||||
/// Handles multiple hunks per file, added-only and deleted files,
|
||||
/// renames (the post-image path wins), `\ No newline at end of file`
|
||||
/// markers, CRLF line endings, and hunk headers with omitted counts
|
||||
/// (`@@ -1 +1 @@`). Hunk bodies are bounded by the counts in the header
|
||||
/// so a file whose *content* contains `--- `, `+++ `, or `diff --git`
|
||||
/// lines cannot be mistaken for a new file header.
|
||||
#[must_use]
|
||||
pub fn parse(diff: &str) -> Self {
|
||||
let mut files: BTreeMap<String, FileHunks> = BTreeMap::new();
|
||||
let mut current: Option<String> = None;
|
||||
let mut cursor = HunkCursor::default();
|
||||
let mut in_hunk = false;
|
||||
|
||||
for raw in diff.split('\n') {
|
||||
let line = raw.strip_suffix('\r').unwrap_or(raw);
|
||||
|
||||
if in_hunk && !cursor.exhausted() {
|
||||
match consume_hunk_line(line, &mut cursor, current.as_deref(), &mut files) {
|
||||
BodyStep::Consumed => continue,
|
||||
BodyStep::NotABodyLine => in_hunk = false,
|
||||
}
|
||||
}
|
||||
|
||||
if line.starts_with("diff --git ") {
|
||||
current = None;
|
||||
in_hunk = false;
|
||||
} else if let Some(rest) = line.strip_prefix("+++ ") {
|
||||
in_hunk = false;
|
||||
current = post_image_path(rest);
|
||||
if let Some(path) = current.as_ref() {
|
||||
files.entry(path.clone()).or_default();
|
||||
}
|
||||
} else if line.starts_with("--- ") {
|
||||
// Pre-image header; the `+++` that follows decides the path we
|
||||
// anchor against (which is what makes renames work).
|
||||
in_hunk = false;
|
||||
} else if let Some((right_start, old_count, new_count)) = parse_hunk_header(line) {
|
||||
cursor = HunkCursor {
|
||||
right_line: right_start,
|
||||
remaining_old: old_count,
|
||||
remaining_new: new_count,
|
||||
};
|
||||
in_hunk = true;
|
||||
}
|
||||
}
|
||||
|
||||
Self { files }
|
||||
}
|
||||
|
||||
/// True when the diff touches `path` at all (post-image path match).
|
||||
#[must_use]
|
||||
pub fn touches_path(&self, path: &str) -> bool {
|
||||
self.files.contains_key(path)
|
||||
}
|
||||
|
||||
/// True when `line` is a RIGHT-side line GitHub will accept an inline
|
||||
/// comment on for `path` — a context line or an added line inside a hunk.
|
||||
#[must_use]
|
||||
pub fn contains_line(&self, path: &str, line: u32) -> bool {
|
||||
self.files
|
||||
.get(path)
|
||||
.is_some_and(|file| covers(&file.commentable, line))
|
||||
}
|
||||
|
||||
/// True when every line in `start..=end` is a valid RIGHT-side anchor.
|
||||
/// Gates multi-line committable suggestions: GitHub rejects a
|
||||
/// `start_line`/`line` span that leaves the diff.
|
||||
#[must_use]
|
||||
pub fn contains_span(&self, path: &str, start: u32, end: u32) -> bool {
|
||||
if start > end {
|
||||
return false;
|
||||
}
|
||||
let Some(file) = self.files.get(path) else {
|
||||
return false;
|
||||
};
|
||||
(start..=end).all(|line| covers(&file.commentable, line))
|
||||
}
|
||||
}
|
||||
|
||||
enum BodyStep {
|
||||
Consumed,
|
||||
NotABodyLine,
|
||||
}
|
||||
|
||||
fn consume_hunk_line(
|
||||
line: &str,
|
||||
cursor: &mut HunkCursor,
|
||||
current: Option<&str>,
|
||||
files: &mut BTreeMap<String, FileHunks>,
|
||||
) -> BodyStep {
|
||||
// `\ No newline at end of file` annotates the previous line and consumes
|
||||
// no line number on either side.
|
||||
if line.starts_with('\\') {
|
||||
return BodyStep::Consumed;
|
||||
}
|
||||
match line.as_bytes().first() {
|
||||
Some(b'+') => {
|
||||
if let Some(path) = current {
|
||||
let entry = files.entry(path.to_string()).or_default();
|
||||
push_line(&mut entry.commentable, cursor.right_line);
|
||||
}
|
||||
cursor.right_line = cursor.right_line.saturating_add(1);
|
||||
cursor.remaining_new = cursor.remaining_new.saturating_sub(1);
|
||||
BodyStep::Consumed
|
||||
}
|
||||
Some(b'-') => {
|
||||
// Deleted: LEFT side only, never a valid RIGHT anchor.
|
||||
cursor.remaining_old = cursor.remaining_old.saturating_sub(1);
|
||||
BodyStep::Consumed
|
||||
}
|
||||
// A context line is " text"; some transports strip the trailing space
|
||||
// from an empty context line, leaving a bare empty line.
|
||||
Some(b' ') | None => {
|
||||
if let Some(path) = current {
|
||||
let entry = files.entry(path.to_string()).or_default();
|
||||
push_line(&mut entry.commentable, cursor.right_line);
|
||||
}
|
||||
cursor.right_line = cursor.right_line.saturating_add(1);
|
||||
cursor.remaining_old = cursor.remaining_old.saturating_sub(1);
|
||||
cursor.remaining_new = cursor.remaining_new.saturating_sub(1);
|
||||
BodyStep::Consumed
|
||||
}
|
||||
Some(_) => BodyStep::NotABodyLine,
|
||||
}
|
||||
}
|
||||
|
||||
fn covers(ranges: &[LineRange], line: u32) -> bool {
|
||||
ranges
|
||||
.iter()
|
||||
.any(|range| line >= range.start && line <= range.end)
|
||||
}
|
||||
|
||||
fn push_line(ranges: &mut Vec<LineRange>, line: u32) {
|
||||
if let Some(last) = ranges.last_mut()
|
||||
&& last.end.saturating_add(1) == line
|
||||
{
|
||||
last.end = line;
|
||||
return;
|
||||
}
|
||||
ranges.push(LineRange {
|
||||
start: line,
|
||||
end: line,
|
||||
});
|
||||
}
|
||||
|
||||
/// Extract the post-image path from a `+++ ` header body. Returns `None` for
|
||||
/// `/dev/null` (a deleted file has no RIGHT side to comment on).
|
||||
fn post_image_path(rest: &str) -> Option<String> {
|
||||
// Some diff formats append a tab plus timestamp.
|
||||
let rest = rest.split('\t').next().unwrap_or(rest).trim_end();
|
||||
if rest.is_empty() || rest == "/dev/null" {
|
||||
return None;
|
||||
}
|
||||
// Quoted paths (`"b/we\tird.rs"`) carry C-style escapes; anchoring on a
|
||||
// mangled path would 422 the review, so decline rather than guess.
|
||||
if rest.starts_with('"') {
|
||||
return None;
|
||||
}
|
||||
let path = rest.strip_prefix("b/").unwrap_or(rest);
|
||||
if path.is_empty() || path == "/dev/null" {
|
||||
return None;
|
||||
}
|
||||
Some(path.to_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)> {
|
||||
let rest = line.strip_prefix("@@ ")?;
|
||||
let end = rest.find(" @@")?;
|
||||
let ranges = &rest[..end];
|
||||
let mut old_count = None;
|
||||
let mut new_count = None;
|
||||
let mut right_start = None;
|
||||
for part in ranges.split(' ') {
|
||||
if let Some(spec) = part.strip_prefix('-') {
|
||||
old_count = Some(parse_range_count(spec)?);
|
||||
} else if let Some(spec) = part.strip_prefix('+') {
|
||||
let (start, count) = parse_range(spec)?;
|
||||
right_start = Some(start);
|
||||
new_count = Some(count);
|
||||
}
|
||||
}
|
||||
Some((right_start?, old_count?, new_count?))
|
||||
}
|
||||
|
||||
fn parse_range(spec: &str) -> Option<(u32, u32)> {
|
||||
let mut parts = spec.split(',');
|
||||
let start = parts.next()?.parse::<u32>().ok()?;
|
||||
let count = match parts.next() {
|
||||
Some(count) => count.parse::<u32>().ok()?,
|
||||
None => 1,
|
||||
};
|
||||
Some((start, count))
|
||||
}
|
||||
|
||||
fn parse_range_count(spec: &str) -> Option<u32> {
|
||||
parse_range(spec).map(|(_, count)| count)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
/// Join diff lines verbatim. Written as a slice rather than one string
|
||||
/// literal because a `\` continuation would strip the leading space that
|
||||
/// marks a context line — which is exactly the byte this parser reads.
|
||||
fn diff(lines: &[&str]) -> String {
|
||||
let mut joined = lines.join("\n");
|
||||
joined.push('\n');
|
||||
joined
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parses_multiple_hunks_in_one_file() {
|
||||
let diff = diff(&[
|
||||
"diff --git a/src/lib.rs b/src/lib.rs",
|
||||
"index 1111111..2222222 100644",
|
||||
"--- a/src/lib.rs",
|
||||
"+++ b/src/lib.rs",
|
||||
"@@ -10,3 +10,4 @@ fn one() {",
|
||||
" context_a",
|
||||
"-removed",
|
||||
"+added_one",
|
||||
"+added_two",
|
||||
" context_b",
|
||||
"@@ -80,2 +81,2 @@ fn two() {",
|
||||
" ctx",
|
||||
"-old",
|
||||
"+new",
|
||||
]);
|
||||
let hunks = DiffHunks::parse(&diff);
|
||||
assert!(hunks.touches_path("src/lib.rs"));
|
||||
// First hunk covers right lines 10..=13 (ctx, +, +, ctx).
|
||||
for line in 10..=13 {
|
||||
assert!(hunks.contains_line("src/lib.rs", line), "line {line}");
|
||||
}
|
||||
assert!(!hunks.contains_line("src/lib.rs", 9));
|
||||
assert!(!hunks.contains_line("src/lib.rs", 14));
|
||||
// Second hunk: ctx at 81, `-old` consumes no right line, `+new` at 82.
|
||||
assert!(hunks.contains_line("src/lib.rs", 81));
|
||||
assert!(hunks.contains_line("src/lib.rs", 82));
|
||||
assert!(!hunks.contains_line("src/lib.rs", 83));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parses_added_only_file() {
|
||||
let diff = diff(&[
|
||||
"diff --git a/new.rs b/new.rs",
|
||||
"new file mode 100644",
|
||||
"index 0000000..3333333",
|
||||
"--- /dev/null",
|
||||
"+++ b/new.rs",
|
||||
"@@ -0,0 +1,3 @@",
|
||||
"+fn a() {}",
|
||||
"+fn b() {}",
|
||||
"+fn c() {}",
|
||||
]);
|
||||
let hunks = DiffHunks::parse(&diff);
|
||||
assert!(hunks.contains_span("new.rs", 1, 3));
|
||||
assert!(!hunks.contains_line("new.rs", 4));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn deleted_file_has_no_right_side_anchor() {
|
||||
let diff = diff(&[
|
||||
"diff --git a/gone.rs b/gone.rs",
|
||||
"deleted file mode 100644",
|
||||
"index 3333333..0000000",
|
||||
"--- a/gone.rs",
|
||||
"+++ /dev/null",
|
||||
"@@ -1,2 +0,0 @@",
|
||||
"-fn a() {}",
|
||||
"-fn b() {}",
|
||||
]);
|
||||
let hunks = DiffHunks::parse(&diff);
|
||||
assert!(!hunks.touches_path("gone.rs"));
|
||||
assert!(!hunks.contains_line("gone.rs", 1));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pure_deletion_hunk_exposes_no_right_lines() {
|
||||
let diff = diff(&[
|
||||
"diff --git a/src/a.rs b/src/a.rs",
|
||||
"--- a/src/a.rs",
|
||||
"+++ b/src/a.rs",
|
||||
"@@ -5,3 +4,0 @@",
|
||||
"-one",
|
||||
"-two",
|
||||
"-three",
|
||||
]);
|
||||
let hunks = DiffHunks::parse(&diff);
|
||||
assert!(hunks.touches_path("src/a.rs"));
|
||||
assert!(!hunks.contains_line("src/a.rs", 4));
|
||||
assert!(!hunks.contains_line("src/a.rs", 5));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn rename_anchors_on_the_post_image_path() {
|
||||
let diff = diff(&[
|
||||
"diff --git a/old/name.rs b/new/name.rs",
|
||||
"similarity index 92%",
|
||||
"rename from old/name.rs",
|
||||
"rename to new/name.rs",
|
||||
"--- a/old/name.rs",
|
||||
"+++ b/new/name.rs",
|
||||
"@@ -1,2 +1,2 @@",
|
||||
" kept",
|
||||
"-old",
|
||||
"+new",
|
||||
]);
|
||||
let hunks = DiffHunks::parse(&diff);
|
||||
assert!(hunks.touches_path("new/name.rs"));
|
||||
assert!(!hunks.touches_path("old/name.rs"));
|
||||
assert!(hunks.contains_span("new/name.rs", 1, 2));
|
||||
assert!(!hunks.contains_line("new/name.rs", 3));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn rename_without_content_change_has_no_anchors() {
|
||||
let diff = diff(&[
|
||||
"diff --git a/old.rs b/new.rs",
|
||||
"similarity index 100%",
|
||||
"rename from old.rs",
|
||||
"rename to new.rs",
|
||||
]);
|
||||
let hunks = DiffHunks::parse(&diff);
|
||||
// No `+++` header and no hunk: nothing is commentable, and we must not
|
||||
// invent an anchor.
|
||||
assert!(!hunks.touches_path("new.rs"));
|
||||
assert!(!hunks.contains_line("new.rs", 1));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn no_newline_marker_consumes_no_line_number() {
|
||||
let diff = diff(&[
|
||||
"diff --git a/eof.rs b/eof.rs",
|
||||
"--- a/eof.rs",
|
||||
"+++ b/eof.rs",
|
||||
"@@ -1,2 +1,2 @@",
|
||||
" first",
|
||||
"-last",
|
||||
"\\ No newline at end of file",
|
||||
"+last!",
|
||||
"\\ No newline at end of file",
|
||||
]);
|
||||
let hunks = DiffHunks::parse(&diff);
|
||||
assert!(hunks.contains_span("eof.rs", 1, 2));
|
||||
assert!(!hunks.contains_line("eof.rs", 3));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn crlf_diff_parses_paths_and_lines() {
|
||||
let diff = [
|
||||
"diff --git a/crlf.rs b/crlf.rs",
|
||||
"index 1111111..2222222 100644",
|
||||
"--- a/crlf.rs",
|
||||
"+++ b/crlf.rs",
|
||||
"@@ -1,2 +1,3 @@",
|
||||
" ctx",
|
||||
"+added",
|
||||
" tail",
|
||||
]
|
||||
.join("\r\n")
|
||||
+ "\r\n";
|
||||
let hunks = DiffHunks::parse(&diff);
|
||||
assert!(
|
||||
hunks.touches_path("crlf.rs"),
|
||||
"CRLF path must not keep the carriage return"
|
||||
);
|
||||
assert!(hunks.contains_span("crlf.rs", 1, 3));
|
||||
assert!(!hunks.contains_line("crlf.rs", 4));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn hunk_header_without_counts_is_supported() {
|
||||
let diff = diff(&[
|
||||
"diff --git a/one.rs b/one.rs",
|
||||
"--- a/one.rs",
|
||||
"+++ b/one.rs",
|
||||
"@@ -7 +9 @@",
|
||||
"-old",
|
||||
"+new",
|
||||
]);
|
||||
let hunks = DiffHunks::parse(&diff);
|
||||
assert!(hunks.contains_line("one.rs", 9));
|
||||
assert!(!hunks.contains_line("one.rs", 8));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn multi_file_diff_keeps_files_independent() {
|
||||
let diff = diff(&[
|
||||
"diff --git a/a.rs b/a.rs",
|
||||
"--- a/a.rs",
|
||||
"+++ b/a.rs",
|
||||
"@@ -1,0 +1,1 @@",
|
||||
"+alpha",
|
||||
"diff --git a/b.rs b/b.rs",
|
||||
"--- a/b.rs",
|
||||
"+++ b/b.rs",
|
||||
"@@ -50,0 +50,1 @@",
|
||||
"+beta",
|
||||
]);
|
||||
let hunks = DiffHunks::parse(&diff);
|
||||
assert!(hunks.contains_line("a.rs", 1));
|
||||
assert!(!hunks.contains_line("a.rs", 50));
|
||||
assert!(hunks.contains_line("b.rs", 50));
|
||||
assert!(!hunks.contains_line("b.rs", 1));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn file_content_that_looks_like_a_diff_header_stays_in_the_hunk() {
|
||||
// A patch that itself edits a stored diff fixture. Naive parsers read
|
||||
// the ` --- a/inner.rs` / ` +++ b/inner.rs` context lines as a new file
|
||||
// header and mis-anchor every later comment.
|
||||
let diff = diff(&[
|
||||
"diff --git a/fixtures/sample.diff b/fixtures/sample.diff",
|
||||
"--- a/fixtures/sample.diff",
|
||||
"+++ b/fixtures/sample.diff",
|
||||
"@@ -1,4 +1,4 @@",
|
||||
" --- a/inner.rs",
|
||||
" +++ b/inner.rs",
|
||||
"-@@ -1 +1 @@",
|
||||
"+@@ -2 +2 @@",
|
||||
" tail",
|
||||
]);
|
||||
let hunks = DiffHunks::parse(&diff);
|
||||
assert!(hunks.contains_span("fixtures/sample.diff", 1, 4));
|
||||
assert!(!hunks.touches_path("inner.rs"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn quoted_path_is_declined_rather_than_mangled() {
|
||||
let diff = diff(&[
|
||||
"diff --git \"a/we\\tird.rs\" \"b/we\\tird.rs\"",
|
||||
"--- \"a/we\\tird.rs\"",
|
||||
"+++ \"b/we\\tird.rs\"",
|
||||
"@@ -1,0 +1,1 @@",
|
||||
"+x",
|
||||
]);
|
||||
let hunks = DiffHunks::parse(&diff);
|
||||
assert!(hunks.files.is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn empty_context_line_still_advances_the_cursor() {
|
||||
// Some transports strip the trailing space from an empty context line.
|
||||
let diff = diff(&[
|
||||
"diff --git a/s.rs b/s.rs",
|
||||
"--- a/s.rs",
|
||||
"+++ b/s.rs",
|
||||
"@@ -1,3 +1,4 @@",
|
||||
" one",
|
||||
"",
|
||||
"+three",
|
||||
" four",
|
||||
]);
|
||||
let hunks = DiffHunks::parse(&diff);
|
||||
assert!(hunks.contains_span("s.rs", 1, 4));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn contains_span_rejects_partial_and_inverted_ranges() {
|
||||
let diff = diff(&[
|
||||
"diff --git a/x.rs b/x.rs",
|
||||
"--- a/x.rs",
|
||||
"+++ b/x.rs",
|
||||
"@@ -1,1 +1,2 @@",
|
||||
" a",
|
||||
"+b",
|
||||
]);
|
||||
let hunks = DiffHunks::parse(&diff);
|
||||
assert!(hunks.contains_span("x.rs", 1, 2));
|
||||
assert!(!hunks.contains_span("x.rs", 1, 3));
|
||||
assert!(!hunks.contains_span("x.rs", 2, 1));
|
||||
assert!(!hunks.contains_span("missing.rs", 1, 1));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn empty_diff_yields_no_anchors() {
|
||||
let hunks = DiffHunks::parse("");
|
||||
assert!(!hunks.touches_path("anything.rs"));
|
||||
assert!(!hunks.contains_line("anything.rs", 1));
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user