Skip to content

Commit b7daef5

Browse files
haasonsaasclaude
andcommitted
TDD: fix non-deterministic markdown output, empty-string dedup, and escaped backslash parsing
- format.rs: use BTreeMap instead of HashMap for file grouping to ensure deterministic file section ordering in markdown output - multi_pass.rs: return similarity 1.0 when both comment texts are empty (identical empty strings were incorrectly treated as dissimilar) - function_chunker.rs: properly track escaped state for backslash handling in string literals (escaped backslash "\\" was leaving in_string=true) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 01c7d36 commit b7daef5

File tree

3 files changed

+92
-10
lines changed

3 files changed

+92
-10
lines changed

src/core/function_chunker.rs

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -257,9 +257,13 @@ fn find_function_end(lines: &[&str], start: usize, language: &str) -> usize {
257257
let mut found_open = false;
258258
for (i, line) in lines.iter().enumerate().skip(start) {
259259
let mut in_string = false;
260-
let mut prev_char = '\0';
260+
let mut escaped = false;
261261
for ch in line.chars() {
262-
if ch == '"' && prev_char != '\\' {
262+
if escaped {
263+
escaped = false;
264+
} else if in_string && ch == '\\' {
265+
escaped = true;
266+
} else if ch == '"' {
263267
in_string = !in_string;
264268
} else if !in_string {
265269
if ch == '{' {
@@ -269,7 +273,6 @@ fn find_function_end(lines: &[&str], start: usize, language: &str) -> usize {
269273
depth -= 1;
270274
}
271275
}
272-
prev_char = ch;
273276
}
274277
if found_open && depth <= 0 {
275278
return i;
@@ -837,4 +840,17 @@ fn next_func() {
837840
// Line 12: matches "valid" (1-20). Should NOT panic even if "broken" passes filter.
838841
assert!(result.is_some());
839842
}
843+
844+
#[test]
845+
fn test_find_function_end_escaped_backslash_in_string() {
846+
// A string containing "\\" (escaped backslash) on the same line as braces.
847+
// The closing quote after "\\" terminates the string, so the closing brace
848+
// on the same line should be counted.
849+
let lines: Vec<&str> = vec![
850+
r#"function foo() { let s = "\\"; }"#, // line 0: open+close on same line
851+
"function next() {", // line 1: next function
852+
];
853+
let end = find_function_end(&lines, 0, "js");
854+
assert_eq!(end, 0, "Function should end at line 0 (braces balanced on same line with escaped backslash)");
855+
}
840856
}

src/core/multi_pass.rs

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,9 @@ fn content_similarity(a: &str, b: &str) -> f32 {
312312
let words_a: std::collections::HashSet<&str> = a.split_whitespace().collect();
313313
let words_b: std::collections::HashSet<&str> = b.split_whitespace().collect();
314314

315+
if words_a.is_empty() && words_b.is_empty() {
316+
return 1.0;
317+
}
315318
if words_a.is_empty() || words_b.is_empty() {
316319
return 0.0;
317320
}
@@ -528,7 +531,7 @@ mod tests {
528531
fn test_content_similarity() {
529532
assert!(content_similarity("the cat sat on mat", "the cat sat on mat") > 0.99);
530533
assert!(content_similarity("the cat sat", "completely different words") < 0.2);
531-
assert!(content_similarity("", "") < 0.01);
534+
assert!(content_similarity("", "") > 0.99); // identical empty strings
532535
assert!(content_similarity("hello", "") < 0.01);
533536
}
534537

@@ -614,13 +617,12 @@ mod tests {
614617
}
615618

616619
#[test]
617-
fn test_merge_empty_comments() {
620+
fn test_merge_keeps_nonempty_different_comments() {
618621
let review = MultiPassReview::with_defaults();
619-
let existing = vec![make_comment("test.rs", 1, "")];
620-
let deep = vec![make_comment("test.rs", 1, "")];
621-
// Empty comments have similarity 0.0, so both should be kept
622+
let existing = vec![make_comment("test.rs", 1, "first issue")];
623+
let deep = vec![make_comment("test.rs", 1, "completely different issue")];
622624
let merged = review.merge_results(existing, deep);
623-
assert_eq!(merged.len(), 2);
625+
assert_eq!(merged.len(), 2, "Different comments should both be kept");
624626
}
625627

626628
#[test]
@@ -745,4 +747,22 @@ mod tests {
745747
assert!((config.hotspot_threshold - 0.5).abs() < 0.01);
746748
assert_eq!(config.max_deep_files, 5);
747749
}
750+
751+
#[test]
752+
fn test_content_similarity_both_empty() {
753+
// Two empty strings are identical — similarity should be 1.0, not 0.0
754+
assert_eq!(content_similarity("", ""), 1.0);
755+
assert_eq!(content_similarity(" ", " "), 1.0);
756+
assert_eq!(content_similarity(" \n\t ", ""), 1.0);
757+
}
758+
759+
#[test]
760+
fn test_merge_deduplicates_empty_comments() {
761+
let review = MultiPassReview::with_defaults();
762+
let existing = vec![make_comment("test.rs", 1, "")];
763+
let deep = vec![make_comment("test.rs", 1, "")];
764+
// Two identical empty comments at the same location should be deduped
765+
let merged = review.merge_results(existing, deep);
766+
assert_eq!(merged.len(), 1, "Empty duplicate comments should be deduped");
767+
}
748768
}

src/output/format.rs

Lines changed: 47 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ pub fn format_as_markdown(comments: &[core::Comment], rule_priority: &[String])
155155
output.push_str("---\n\n## Detailed Issues\n\n");
156156

157157
// Group comments by file
158-
let mut comments_by_file = std::collections::HashMap::new();
158+
let mut comments_by_file = std::collections::BTreeMap::new();
159159
for comment in comments {
160160
comments_by_file
161161
.entry(&comment.file_path)
@@ -841,4 +841,50 @@ mod tests {
841841
assert!(output.contains("Critical Issues"));
842842
assert!(output.contains("Fix bugs"));
843843
}
844+
845+
#[test]
846+
fn format_markdown_file_sections_are_deterministic() {
847+
// Comments across multiple files should produce deterministic file section ordering
848+
let mut c1 = build_test_comment(
849+
"c1",
850+
core::comment::Category::Bug,
851+
core::comment::Severity::Error,
852+
0.9,
853+
);
854+
c1.file_path = PathBuf::from("src/z_last.rs");
855+
856+
let mut c2 = build_test_comment(
857+
"c2",
858+
core::comment::Category::Bug,
859+
core::comment::Severity::Warning,
860+
0.8,
861+
);
862+
c2.file_path = PathBuf::from("src/a_first.rs");
863+
864+
let mut c3 = build_test_comment(
865+
"c3",
866+
core::comment::Category::Security,
867+
core::comment::Severity::Info,
868+
0.7,
869+
);
870+
c3.file_path = PathBuf::from("src/m_middle.rs");
871+
872+
let comments = vec![c1, c2, c3];
873+
let output = format_as_markdown(&comments, &[]);
874+
875+
// Run multiple times — should always produce the same output
876+
for _ in 0..5 {
877+
let output2 = format_as_markdown(&comments, &[]);
878+
assert_eq!(output, output2, "Markdown output should be deterministic across runs");
879+
}
880+
881+
// File sections should appear in sorted order
882+
let z_pos = output.find("src/z_last.rs").expect("should contain z_last.rs");
883+
let a_pos = output.find("src/a_first.rs").expect("should contain a_first.rs");
884+
let m_pos = output.find("src/m_middle.rs").expect("should contain m_middle.rs");
885+
assert!(
886+
a_pos < m_pos && m_pos < z_pos,
887+
"Files should appear in sorted order: a_first({a_pos}) < m_middle({m_pos}) < z_last({z_pos})"
888+
);
889+
}
844890
}

0 commit comments

Comments
 (0)