fix: 4 TDD-discovered bugs — comment detection, function boundaries, dedup severity, empty LLM responses

haasonsaas · claude · haasonsaas · commit fcfe9778164c · 2026-03-11T13:29:30.000-07:00
Bug 1: `is_comment_line` missed bare `#` (shell/Makefile/Python comments)
  - Changed `#!` → `#![` in non-comment prefixes (Rust inner attrs always have `[`)
  - Any `#` line not matching a known code prefix is now treated as a comment

Bug 2: `find_function_end` ignored single-quoted strings
  - `'{'` in JS/Python strings was counted as a real brace, breaking function
    boundary detection. Now tracks both `"` and `'` string delimiters.

Bug 3: `deduplicate_comments` could drop higher-severity comments
  - `dedup_by` keeps the first element; sort didn't include severity.
    Now sorts by severity (highest first) within same file/line/content group.

Bug 4: OpenAI/Anthropic silently returned empty string for empty responses
  - Empty `choices`/`content` arrays now return errors instead of succeeding
    with empty content, preventing silent failures in the review pipeline.

Co-Authored-By: Claude Opus 4.6 &lt;noreply@anthropic.com&gt;
diff --git a/src/adapters/anthropic.rs b/src/adapters/anthropic.rs
@@ -121,14 +121,13 @@ impl LLMAdapter for AnthropicAdapter {
             .content
             .first()
             .map(|c| {
-                // Verify it's a text content type
                 if c.content_type == "text" {
-                    c.text.clone()
+                    Ok(c.text.clone())
                 } else {
-                    format!("Unsupported content type: {}", c.content_type)
+                    Err(anyhow::anyhow!("Unsupported content type: {}", c.content_type))
                 }
             })
-            .unwrap_or_default();
+            .ok_or_else(|| anyhow::anyhow!("Anthropic returned empty content array — no content generated"))??;
 
         Ok(LLMResponse {
             content,
@@ -292,7 +291,7 @@ mod tests {
     }
 
     #[tokio::test]
-    async fn test_unsupported_content_type() {
+    async fn test_unsupported_content_type_returns_error() {
         let mut server = mockito::Server::new_async().await;
         let _mock = server
             .mock("POST", "/messages")
@@ -311,17 +310,17 @@ mod tests {
         let adapter = AnthropicAdapter::new(test_config(&server.url())).unwrap();
         let result = adapter.complete(test_request()).await;
 
-        assert!(result.is_ok());
-        let response = result.unwrap();
+        assert!(result.is_err(), "Unsupported content type should return an error");
+        let err = result.unwrap_err().to_string();
         assert!(
-            response.content.contains("Unsupported content type"),
-            "Expected 'Unsupported content type' in response, got: {}",
-            response.content
+            err.contains("Unsupported content type"),
+            "Error should mention unsupported type, got: {}",
+            err
         );
     }
 
     #[tokio::test]
-    async fn test_empty_content_array() {
+    async fn test_empty_content_array_returns_error() {
         let mut server = mockito::Server::new_async().await;
         let _mock = server
             .mock("POST", "/messages")
@@ -340,9 +339,9 @@ mod tests {
         let adapter = AnthropicAdapter::new(test_config(&server.url())).unwrap();
         let result = adapter.complete(test_request()).await;
 
-        assert!(result.is_ok());
-        let response = result.unwrap();
-        assert_eq!(response.content, "");
+        assert!(result.is_err(), "Empty content array should return an error, not silently succeed");
+        let err = result.unwrap_err().to_string();
+        assert!(err.contains("empty content"), "Error should mention empty content: {}", err);
     }
 
     #[test]
diff --git a/src/adapters/openai.rs b/src/adapters/openai.rs
@@ -196,7 +196,7 @@ impl OpenAIAdapter {
             .choices
             .first()
             .map(|c| c.message.content.clone())
-            .unwrap_or_default();
+            .ok_or_else(|| anyhow::anyhow!("OpenAI returned empty choices array — no content generated"))?;
 
         Ok(LLMResponse {
             content,
@@ -416,7 +416,7 @@ mod tests {
     }
 
     #[tokio::test]
-    async fn test_empty_choices_returns_empty_content() {
+    async fn test_empty_choices_returns_error() {
         let mut server = mockito::Server::new_async().await;
         let _mock = server
             .mock("POST", "/chat/completions")
@@ -435,9 +435,9 @@ mod tests {
         let adapter = OpenAIAdapter::new(test_config(&server.url())).unwrap();
         let result = adapter.complete(test_request()).await;
 
-        assert!(result.is_ok());
-        let response = result.unwrap();
-        assert_eq!(response.content, "");
+        assert!(result.is_err(), "Empty choices should return an error, not silently succeed");
+        let err = result.unwrap_err().to_string();
+        assert!(err.contains("empty choices"), "Error should mention empty choices: {}", err);
     }
 
     #[test]
diff --git a/src/core/comment.rs b/src/core/comment.rs
@@ -449,12 +449,23 @@ impl CommentSynthesizer {
     }
 
     fn deduplicate_comments(comments: &mut Vec<Comment>) {
+        let severity_rank = |s: &Severity| match s {
+            Severity::Error => 0,
+            Severity::Warning => 1,
+            Severity::Info => 2,
+            Severity::Suggestion => 3,
+        };
+
+        // Sort by file/line/content, then by severity (highest first)
         comments.sort_by(|a, b| {
             a.file_path
                 .cmp(&b.file_path)
                 .then(a.line_number.cmp(&b.line_number))
                 .then(a.content.cmp(&b.content))
+                .then(severity_rank(&a.severity).cmp(&severity_rank(&b.severity)))
         });
+        // dedup_by keeps the first element (b) of consecutive duplicates,
+        // which is the highest severity due to our sort order
         comments.dedup_by(|a, b| {
             a.file_path == b.file_path && a.line_number == b.line_number && a.content == b.content
         });
@@ -548,6 +559,49 @@ pub struct RawComment {
 mod tests {
     use super::*;
 
+    #[test]
+    fn test_deduplicate_preserves_highest_severity() {
+        // BUG: dedup_by keeps the first element of a consecutive pair,
+        // but doesn't consider severity. If Warning comes before Error
+        // (due to stable sort on file/line/content), the Error is dropped.
+        let raw_comments = vec![
+            RawComment {
+                file_path: PathBuf::from("src/lib.rs"),
+                line_number: 10,
+                content: "Missing null check".to_string(),
+                rule_id: None,
+                suggestion: None,
+                severity: Some(Severity::Warning),
+                category: Some(Category::Bug),
+                confidence: Some(0.8),
+                fix_effort: None,
+                tags: Vec::new(),
+                code_suggestion: None,
+            },
+            RawComment {
+                file_path: PathBuf::from("src/lib.rs"),
+                line_number: 10,
+                content: "Missing null check".to_string(),
+                rule_id: None,
+                suggestion: None,
+                severity: Some(Severity::Error),
+                category: Some(Category::Bug),
+                confidence: Some(0.9),
+                fix_effort: None,
+                tags: Vec::new(),
+                code_suggestion: None,
+            },
+        ];
+
+        let comments = CommentSynthesizer::synthesize(raw_comments).unwrap();
+        assert_eq!(comments.len(), 1, "Should deduplicate to one comment");
+        assert_eq!(
+            comments[0].severity,
+            Severity::Error,
+            "Should keep the higher severity (Error), not the lower (Warning)"
+        );
+    }
+
     #[test]
     fn test_severity_display() {
         assert_eq!(Severity::Error.to_string(), "Error");
diff --git a/src/core/function_chunker.rs b/src/core/function_chunker.rs
@@ -255,16 +255,19 @@ fn find_function_end(lines: &[&str], start: usize, language: &str) -> usize {
             let mut depth = 0i32;
             let mut found_open = false;
             for (i, line) in lines.iter().enumerate().skip(start) {
-                let mut in_string = false;
+                let mut in_double_quote = false;
+                let mut in_single_quote = false;
                 let mut escaped = false;
                 for ch in line.chars() {
                     if escaped {
                         escaped = false;
-                    } else if in_string && ch == '\\' {
+                    } else if (in_double_quote || in_single_quote) && ch == '\\' {
                         escaped = true;
-                    } else if ch == '"' {
-                        in_string = !in_string;
-                    } else if !in_string {
+                    } else if ch == '"' && !in_single_quote {
+                        in_double_quote = !in_double_quote;
+                    } else if ch == '\'' && !in_double_quote {
+                        in_single_quote = !in_single_quote;
+                    } else if !in_double_quote && !in_single_quote {
                         if ch == '{' {
                             depth += 1;
                             found_open = true;
@@ -911,6 +914,26 @@ fn next_func() {
         assert_eq!(result, Some(2));
     }
 
+    #[test]
+    fn test_find_function_end_single_quoted_string_with_brace() {
+        // BUG: single-quoted strings aren't tracked, so `'{'` is counted as a real brace
+        let lines: Vec<&str> = vec![
+            "function foo() {",         // line 0: depth = 1
+            "    let s = '{';",         // line 1: depth should stay 1, but goes to 2
+            "    console.log(s);",      // line 2
+            "}",                        // line 3: depth goes to 1 (not 0!), so foo "never ends"
+            "",                         // line 4
+            "function bar() {",         // line 5: depth goes to 2
+            "    return 1;",            // line 6
+            "}",                        // line 7: depth goes to 1, and foo is "found" here (wrong!)
+        ];
+        let end = find_function_end(&lines, 0, "js");
+        assert_eq!(
+            end, 3,
+            "foo() should end at line 3, not bleed into bar() due to untracked single-quoted brace"
+        );
+    }
+
     #[test]
     fn test_find_function_end_escaped_backslash_in_string() {
         // A string containing "\\" (escaped backslash) on the same line as braces.
diff --git a/src/review/triage.rs b/src/review/triage.rs
@@ -45,7 +45,7 @@ const COMMENT_PREFIXES: &[&str] = &["//", "# ", "/*", "*/", "* ", "--", "<!--",
 
 /// Patterns that start with `#` but are NOT comments (Rust attributes, C preprocessor, etc.).
 const HASH_NON_COMMENT_PREFIXES: &[&str] = &[
-    "#[", "#!", "#include", "#define", "#ifdef", "#ifndef", "#endif", "#pragma", "#undef", "#elif",
+    "#[", "#![", "#include", "#define", "#ifdef", "#ifndef", "#endif", "#pragma", "#undef", "#elif",
     "#else", "#if ", "#error", "#warning", "#line",
 ];
 
@@ -179,13 +179,18 @@ fn is_comment_line(content: &str) -> bool {
         // Blank lines in a comment-only change are fine
         return true;
     }
-    // Handle `#` lines: exclude Rust attributes, C preprocessor, etc.
-    if trimmed.starts_with('#')
-        && HASH_NON_COMMENT_PREFIXES
+    // Handle `#` lines: treat as comment UNLESS it matches a known non-comment prefix
+    // (Rust attributes, C preprocessor, etc.)
+    if trimmed.starts_with('#') {
+        // If it matches a non-comment prefix like #[, #include, etc., it's code
+        if HASH_NON_COMMENT_PREFIXES
             .iter()
             .any(|p| trimmed.starts_with(p))
-    {
-        return false;
+        {
+            return false;
+        }
+        // Otherwise bare `#`, `#comment`, `# comment` are all comments
+        return true;
     }
     COMMENT_PREFIXES
         .iter()
@@ -888,13 +893,14 @@ mod tests {
     }
 
     #[test]
-    fn test_comment_hash_without_space_is_not_comment() {
-        // "#tag" — not a comment prefix since we changed to "# "
+    fn test_comment_hash_without_space_is_comment() {
+        // "#tag" IS a valid comment in Python/shell/Ruby/Makefile
+        // Only known code patterns (#[, #include, etc.) are excluded
         let diff = make_diff(
             "src/lib.rs",
             vec![make_line(1, ChangeType::Added, "#tag: important")],
         );
-        assert_eq!(triage_diff(&diff), TriageResult::NeedsReview);
+        assert_eq!(triage_diff(&diff), TriageResult::SkipCommentOnly);
     }
 
     #[test]
@@ -950,6 +956,33 @@ mod tests {
         assert_eq!(triage_diff(&diff), TriageResult::NeedsReview);
     }
 
+    #[test]
+    fn test_comment_bare_hash_is_comment() {
+        // Bare `#` without trailing space is a valid comment in shell, Makefile, Python
+        let diff = make_diff(
+            "deploy.sh",
+            vec![
+                make_line(1, ChangeType::Added, "#"),
+                make_line(2, ChangeType::Added, "#!"),
+            ],
+        );
+        // BUG: currently returns NeedsReview because `#` doesn't match `"# "` prefix
+        assert_eq!(triage_diff(&diff), TriageResult::SkipCommentOnly);
+    }
+
+    #[test]
+    fn test_comment_hash_with_no_space_in_makefile() {
+        // Makefile comments use `#` without requiring a space
+        let diff = make_diff(
+            "Makefile",
+            vec![
+                make_line(1, ChangeType::Removed, "#old target comment"),
+                make_line(1, ChangeType::Added, "#new target comment"),
+            ],
+        );
+        assert_eq!(triage_diff(&diff), TriageResult::SkipCommentOnly);
+    }
+
     #[test]
     fn test_all_blank_lines_is_comment_only() {
         // Blank lines count as "comment" in is_comment_line
