Skip to content

Commit 486ac7a

Browse files
haasonsaasclaude
andcommitted
fix: 2 TDD bugs — hunk header off-by-one, walkthrough false truncation
- smart_review_prompt.rs: Use saturating_sub(1) for inclusive end line in hunk headers. "Lines 10-15" → "Lines 10-14" for 5-line hunk. - format.rs: Move truncation check before push in build_change_walkthrough so exactly max_entries files don't falsely report truncation. 985 tests pass, zero clippy warnings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent d9d232f commit 486ac7a

File tree

2 files changed

+106
-7
lines changed

2 files changed

+106
-7
lines changed

src/core/smart_review_prompt.rs

Lines changed: 54 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,9 +138,9 @@ TAGS: [comma-separated relevant tags]
138138
let hunk_header = format!(
139139
"### Hunk: Lines {}-{} (was {}-{})\n\n",
140140
hunk.new_start,
141-
hunk.new_start + hunk.new_lines,
141+
hunk.new_start + hunk.new_lines.saturating_sub(1),
142142
hunk.old_start,
143-
hunk.old_start + hunk.old_lines
143+
hunk.old_start + hunk.old_lines.saturating_sub(1)
144144
);
145145
if max_diff_chars > 0 && diff_chars.saturating_add(hunk_header.len()) > max_diff_chars {
146146
diff_truncated = true;
@@ -211,3 +211,55 @@ TAGS: [comma-separated relevant tags]
211211
Ok(prompt)
212212
}
213213
}
214+
215+
#[cfg(test)]
216+
mod tests {
217+
use super::*;
218+
use crate::core::diff_parser::{ChangeType, DiffHunk, DiffLine};
219+
use std::path::PathBuf;
220+
221+
// ── Bug: hunk header shows one-past-end line number ──────────────
222+
//
223+
// A hunk with new_start=10, new_lines=5 covers lines 10-14 (inclusive).
224+
// The old code displayed "Lines 10-15", which is off by one.
225+
226+
#[test]
227+
fn test_hunk_header_line_range_is_inclusive() {
228+
let diff = UnifiedDiff {
229+
file_path: PathBuf::from("test.rs"),
230+
old_content: None,
231+
new_content: None,
232+
is_new: false,
233+
is_deleted: false,
234+
is_binary: false,
235+
hunks: vec![DiffHunk {
236+
old_start: 5,
237+
old_lines: 3,
238+
new_start: 10,
239+
new_lines: 5,
240+
context: String::new(),
241+
changes: vec![DiffLine {
242+
content: "line".to_string(),
243+
change_type: ChangeType::Added,
244+
old_line_no: None,
245+
new_line_no: Some(10),
246+
}],
247+
}],
248+
};
249+
let (_system, user) =
250+
SmartReviewPromptBuilder::build_enhanced_review_prompt(&diff, &[], 1000, 10000, None)
251+
.unwrap();
252+
// Should say "Lines 10-14" (inclusive), not "Lines 10-15"
253+
assert!(
254+
user.contains("Lines 10-14"),
255+
"Hunk header should use inclusive end line; prompt contains: {}",
256+
user.lines()
257+
.find(|l| l.contains("Hunk:"))
258+
.unwrap_or("(no hunk header found)")
259+
);
260+
assert!(
261+
user.contains("was 5-7"),
262+
"Old range should also be inclusive: 5 + 3 - 1 = 7"
263+
);
264+
}
265+
}

src/output/format.rs

Lines changed: 52 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -546,18 +546,18 @@ pub fn build_change_walkthrough(diffs: &[core::UnifiedDiff]) -> String {
546546
"modified"
547547
};
548548

549+
if entries.len() >= max_entries {
550+
truncated = true;
551+
break;
552+
}
553+
549554
entries.push(format!(
550555
"- `{}` ({}; +{}, -{})",
551556
diff.file_path.display(),
552557
status,
553558
added,
554559
removed
555560
));
556-
557-
if entries.len() >= max_entries {
558-
truncated = true;
559-
break;
560-
}
561561
}
562562

563563
if entries.is_empty() {
@@ -910,4 +910,51 @@ mod tests {
910910
"Files should appear in sorted order: a_first({a_pos}) < m_middle({m_pos}) < z_last({z_pos})"
911911
);
912912
}
913+
914+
// ── Bug: walkthrough falsely reports truncation at exactly max_entries ──
915+
//
916+
// The truncation check ran after pushing the entry, so when exactly
917+
// max_entries (50) files exist, it would say "truncated" even though
918+
// all entries were included.
919+
920+
#[test]
921+
fn test_walkthrough_not_truncated_at_exactly_max() {
922+
let diffs: Vec<core::UnifiedDiff> = (0..50)
923+
.map(|i| core::UnifiedDiff {
924+
file_path: PathBuf::from(format!("file{}.rs", i)),
925+
old_content: None,
926+
new_content: None,
927+
is_new: false,
928+
is_deleted: false,
929+
is_binary: false,
930+
hunks: vec![core::diff_parser::DiffHunk {
931+
old_start: 1,
932+
old_lines: 1,
933+
new_start: 1,
934+
new_lines: 1,
935+
context: String::new(),
936+
changes: vec![core::diff_parser::DiffLine {
937+
content: "line".to_string(),
938+
change_type: core::diff_parser::ChangeType::Added,
939+
old_line_no: None,
940+
new_line_no: Some(1),
941+
}],
942+
}],
943+
})
944+
.collect();
945+
946+
let output = build_change_walkthrough(&diffs);
947+
assert!(
948+
!output.contains("truncated"),
949+
"50 files (exactly max_entries) should not be truncated"
950+
);
951+
// All 50 files should be present
952+
for i in 0..50 {
953+
assert!(
954+
output.contains(&format!("file{}.rs", i)),
955+
"Missing file{}.rs in walkthrough",
956+
i
957+
);
958+
}
959+
}
913960
}

0 commit comments

Comments
 (0)