fix: 5 TDD-discovered bugs — error rate, diff parsing, default model, file status

- Error rate now counts only explicit review.failed events, not all non-completed
- Empty lines in diffs treated as context instead of being silently skipped
- parse_text_diff sets is_new/is_deleted based on empty old/new content
- Default model changed from Sonnet to Opus per project conventions
- Added 5 new tests covering all discovered issues

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: haasonsaas

src/config.rs

@@ -1061,7 +1061,7 @@ impl Config {
 }
 
 fn default_model() -> String {
-    "claude-sonnet-4-6".to_string()
+    "claude-opus-4-6".to_string()
 }
 
 fn default_temperature() -> f32 {
@@ -1764,4 +1764,16 @@ temperature: 0.3
         assert!(config.model_embedding.is_none());
         assert!(config.fallback_models.is_empty());
     }
+
+    #[test]
+    fn test_default_model_is_frontier() {
+        // Per CLAUDE.md: "Always use frontier models (Opus) for AI-powered features
+        // — never default to Sonnet or smaller"
+        let model = default_model();
+        assert!(
+            model.contains("opus"),
+            "Default model should be Opus (frontier), got: {}",
+            model
+        );
+    }
 }

src/core/diff_parser.rs

@@ -181,8 +181,8 @@ impl DiffParser {
             new_content: Some(new_content.to_string()),
             hunks,
             is_binary: false,
-            is_deleted: false,
-            is_new: false,
+            is_deleted: new_content.is_empty() && !old_content.is_empty(),
+            is_new: old_content.is_empty() && !new_content.is_empty(),
         })
     }
 
@@ -368,17 +368,16 @@ impl DiffParser {
                 *i += 1;
                 continue;
             }
-            if line.is_empty() {
-                *i += 1;
-                continue;
-            }
-
-            let (change_type, content) = match line.chars().next() {
+            let (change_type, content) = if line.is_empty() {
+                // Some diff tools emit truly empty lines for empty context lines
+                // (omitting the leading space). Treat as context to keep line numbers in sync.
+                (ChangeType::Context, "")
+            } else { match line.chars().next() {
                 Some('+') => (ChangeType::Added, &line[1..]),
                 Some('-') => (ChangeType::Removed, &line[1..]),
                 Some(' ') => (ChangeType::Context, &line[1..]),
                 _ => (ChangeType::Context, line),
-            };
+            }};
 
             let diff_line = match change_type {
                 ChangeType::Added => {
@@ -540,4 +539,63 @@ index 0000000..f735c20\n\
         assert!(diffs[0].is_new);
         assert!(!diffs[0].is_deleted);
     }
+
+    #[test]
+    fn test_parse_hunk_empty_lines_not_skipped() {
+        // BUG: empty lines in diff body are skipped, causing line number desync.
+        // An empty line (no leading space) in some diff tools represents an empty
+        // context line. The parser should treat it as context, not skip it.
+        let diff_text = "\
+diff --git a/test.txt b/test.txt\n\
+index abc..def 100644\n\
+--- a/test.txt\n\
++++ b/test.txt\n\
+@@ -1,4 +1,4 @@\n\
+ line1\n\
+\n\
+-old_line3\n\
++new_line3\n";
+        // The empty line (between "line1" and "-old_line3") should be treated as
+        // context line 2. Without it, line numbers after the empty line are wrong.
+        let diffs = DiffParser::parse_unified_diff(diff_text).unwrap();
+        assert_eq!(diffs.len(), 1);
+        let hunk = &diffs[0].hunks[0];
+
+        // Find the removed line — it should be on line 3, not line 2
+        let removed = hunk
+            .changes
+            .iter()
+            .find(|c| c.change_type == ChangeType::Removed)
+            .expect("Should have a removed line");
+        assert_eq!(
+            removed.old_line_no,
+            Some(3),
+            "Removed line should be on old line 3 (after the empty context line 2), got {:?}",
+            removed.old_line_no
+        );
+    }
+
+    #[test]
+    fn test_parse_text_diff_sets_is_new_for_new_file() {
+        // BUG: parse_text_diff always sets is_new=false even when old_content is empty
+        let diff =
+            DiffParser::parse_text_diff("", "new content\n", PathBuf::from("new_file.rs"))
+                .unwrap();
+        assert!(
+            diff.is_new,
+            "parse_text_diff with empty old content should set is_new=true"
+        );
+    }
+
+    #[test]
+    fn test_parse_text_diff_sets_is_deleted_for_deleted_file() {
+        // BUG: parse_text_diff always sets is_deleted=false even when new_content is empty
+        let diff =
+            DiffParser::parse_text_diff("old content\n", "", PathBuf::from("deleted_file.rs"))
+                .unwrap();
+        assert!(
+            diff.is_deleted,
+            "parse_text_diff with empty new content should set is_deleted=true"
+        );
+    }
 }

src/server/github.rs

@@ -1177,4 +1177,5 @@ mod tests {
 
         assert!(verify_webhook_signature(secret, body, &signature).is_ok());
     }
+
 }

src/server/storage_json.rs

@@ -178,7 +178,10 @@ impl StorageBackend for JsonStorageBackend {
             .iter()
             .filter(|e| e.event_type == "review.completed")
             .count() as i64;
-        let failed = total - completed;
+        let failed = events
+            .iter()
+            .filter(|e| e.event_type == "review.failed")
+            .count() as i64;
         let error_rate = if total > 0 {
             failed as f64 / total as f64
         } else {
@@ -1371,4 +1374,47 @@ mod tests {
         let events = backend.list_events(&EventFilters::default()).await.unwrap();
         assert!(events.is_empty());
     }
+
+    #[tokio::test]
+    async fn test_error_rate_only_counts_explicit_failures() {
+        // BUG: `failed = total - completed` misclassifies non-failure events as failures.
+        // Events with type "review.started" or "review.timeout" are counted as failures.
+        let dir = tempfile::tempdir().unwrap();
+        let backend = JsonStorageBackend::new(&dir.path().join("reviews.json"));
+        let now = now_ts();
+
+        // 1 completed, 1 failed, 1 timeout (not a failure per se)
+        backend
+            .save_review(&make_session_with_event(
+                "r1", now, ReviewStatus::Complete, "review.completed", "gpt-4o", "github", 100,
+            ))
+            .await
+            .unwrap();
+        backend
+            .save_review(&make_session_with_event(
+                "r2", now, ReviewStatus::Failed, "review.failed", "gpt-4o", "github", 200,
+            ))
+            .await
+            .unwrap();
+        backend
+            .save_review(&make_session_with_event(
+                "r3", now, ReviewStatus::Failed, "review.timeout", "gpt-4o", "github", 300,
+            ))
+            .await
+            .unwrap();
+
+        let stats = backend
+            .get_event_stats(&EventFilters::default())
+            .await
+            .unwrap();
+        // Only "review.failed" should count as a failure (1 out of 3)
+        // BUG: currently reports 2/3 because timeout is also counted as failed
+        let expected_error_rate = 1.0 / 3.0;
+        assert!(
+            (stats.error_rate - expected_error_rate).abs() < 0.01,
+            "Error rate should be {:.2} (only explicit failures), got {:.2}",
+            expected_error_rate,
+            stats.error_rate
+        );
+    }
 }