fix: dedup severity bug in composable pipeline, accept ssh:// URLs, clean up comments

- Fix DeduplicateStage in composable_pipeline.rs to preserve highest
  severity when deduplicating (same bug as comment.rs, different path)
- Accept ssh:// URLs in is_safe_git_url (was rejecting legitimate SSH)
- Consolidate is_git_source to delegate to is_safe_git_url
- Replace stale "BUG:" test comments with "Regression:" phrasing

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

Author: haasonsaas

src/core/comment.rs

@@ -561,7 +561,7 @@ mod tests {
 
     #[test]
     fn test_deduplicate_preserves_highest_severity() {
-        // BUG: dedup_by keeps the first element of a consecutive pair,
+        // Regression: 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![

src/core/composable_pipeline.rs

@@ -211,12 +211,21 @@ impl PipelineStage for DeduplicateStage {
     }
 
     fn execute(&self, ctx: &mut PipelineContext) -> Result<()> {
+        use crate::core::comment::Severity;
+        let severity_rank = |s: &Severity| match s {
+            Severity::Error => 0,
+            Severity::Warning => 1,
+            Severity::Info => 2,
+            Severity::Suggestion => 3,
+        };
         ctx.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 b (the earlier element); our sort puts highest severity first
         ctx.comments.dedup_by(|a, b| {
             a.file_path == b.file_path && a.line_number == b.line_number && a.content == b.content
         });
@@ -688,6 +697,28 @@ mod tests {
         assert_eq!(ctx.comments.len(), 2);
     }
 
+    // Regression: DeduplicateStage must keep the highest severity when deduplicating
+    #[test]
+    fn test_deduplicate_preserves_highest_severity() {
+        let mut ctx = PipelineContext::new();
+        let mut info = make_comment("test.rs", 10, "duplicate issue", 0.7);
+        info.severity = Severity::Info;
+        let mut error = make_comment("test.rs", 10, "duplicate issue", 0.7);
+        error.severity = Severity::Error;
+        // Insert lower severity first to ensure sort fixes order
+        ctx.comments.push(info);
+        ctx.comments.push(error);
+
+        let stage = DeduplicateStage;
+        stage.execute(&mut ctx).unwrap();
+        assert_eq!(ctx.comments.len(), 1);
+        assert_eq!(
+            ctx.comments[0].severity,
+            Severity::Error,
+            "DeduplicateStage should keep the highest severity"
+        );
+    }
+
     // Regression: SortBySeverityStage must sort Error > Warning > Info
     #[test]
     fn test_sort_by_severity_order() {

src/core/diff_parser.rs

@@ -544,7 +544,7 @@ index 0000000..f735c20\n\
 
     #[test]
     fn test_parse_hunk_empty_lines_not_skipped() {
-        // BUG: empty lines in diff body are skipped, causing line number desync.
+        // Regression: empty lines in diff body were previously skipped.
         // 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 = "\
@@ -579,7 +579,7 @@ index abc..def 100644\n\
 
     #[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
+        // Regression: parse_text_diff must set is_new when old_content is empty
         let diff =
             DiffParser::parse_text_diff("", "new content\n", PathBuf::from("new_file.rs")).unwrap();
         assert!(
@@ -590,7 +590,7 @@ index abc..def 100644\n\
 
     #[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
+        // Regression: parse_text_diff must set is_deleted when new_content is empty
         let diff =
             DiffParser::parse_text_diff("old content\n", "", PathBuf::from("deleted_file.rs"))
                 .unwrap();

src/core/function_chunker.rs

@@ -916,7 +916,7 @@ fn next_func() {
 
     #[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
+        // Regression: single-quoted strings must be tracked so `'{'` isn't 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
@@ -936,8 +936,8 @@ fn next_func() {
 
     #[test]
     fn test_find_function_end_rust_lifetime_not_string() {
-        // BUG: Rust lifetime annotations ('a, 'static) toggle in_single_quote,
-        // causing the opening brace on the same line to be ignored.
+        // Regression: Rust lifetime annotations ('a, 'static) must not toggle
+        // in_single_quote, which would cause the opening brace to be ignored.
         let lines: Vec<&str> = vec![
             "pub fn new(name: &'static str) -> Self {", // line 0: has lifetime '
             "    Self { name }",                        // line 1

src/review/context_helpers.rs

@@ -56,30 +56,25 @@ pub fn resolve_pattern_repositories(
     resolved
 }
 
-fn is_git_source(source: &str) -> bool {
-    if source.starts_with("https://") || source.starts_with("git@") {
-        return true;
-    }
-    if source.starts_with("http://") && source.ends_with(".git") {
-        return true;
-    }
-    false
-}
-
-/// Validate that a pattern repository source URL uses an allowed scheme.
-/// Only `https://`, `git@`, and `http://` (with `.git` suffix) are permitted.
+/// Check whether a source string looks like a git URL and uses an allowed scheme.
+/// Accepts `https://`, `ssh://`, `git@` (SSH shorthand), and `http://` (with `.git` suffix).
 fn is_safe_git_url(source: &str) -> bool {
     source.starts_with("https://")
+        || source.starts_with("ssh://")
         || source.starts_with("git@")
         || (source.starts_with("http://") && source.ends_with(".git"))
 }
 
+fn is_git_source(source: &str) -> bool {
+    is_safe_git_url(source)
+}
+
 fn prepare_pattern_repository_checkout(source: &str) -> Option<PathBuf> {
     use std::process::Command;
 
     if !is_safe_git_url(source) {
         warn!(
-            "Rejecting pattern repository '{}': only https:// and git@ URLs are allowed",
+            "Rejecting pattern repository '{}': only https://, ssh://, and git@ URLs are allowed",
             source
         );
         return None;
@@ -527,7 +522,11 @@ mod tests {
     #[test]
     fn test_is_git_source_rejects_other_schemes() {
         assert!(!is_git_source("ftp://example.com/repo.git"));
-        assert!(!is_git_source("ssh://example.com/repo"));
+    }
+
+    #[test]
+    fn test_is_git_source_accepts_ssh() {
+        assert!(is_git_source("ssh://example.com/repo"));
     }
 
     #[test]
@@ -539,6 +538,8 @@ mod tests {
     #[test]
     fn test_is_safe_git_url_allows_ssh() {
         assert!(is_safe_git_url("git@github.com:org/repo.git"));
+        assert!(is_safe_git_url("ssh://example.com/repo"));
+        assert!(is_safe_git_url("ssh://git@gitlab.internal/org/rules.git"));
     }
 
     #[test]
@@ -551,7 +552,6 @@ mod tests {
     #[test]
     fn test_is_safe_git_url_rejects_arbitrary_schemes() {
         assert!(!is_safe_git_url("ftp://example.com/repo"));
-        assert!(!is_safe_git_url("ssh://example.com/repo"));
         assert!(!is_safe_git_url("gopher://example.com/repo"));
     }
 

src/review/triage.rs

@@ -966,7 +966,7 @@ mod tests {
                 make_line(2, ChangeType::Added, "#!"),
             ],
         );
-        // BUG: currently returns NeedsReview because `#` doesn't match `"# "` prefix
+        // Regression: bare `#` and `#!` must be treated as comments
         assert_eq!(triage_diff(&diff), TriageResult::SkipCommentOnly);
     }
 

src/server/storage_json.rs

@@ -1377,7 +1377,7 @@ mod tests {
 
     #[tokio::test]
     async fn test_error_rate_only_counts_explicit_failures() {
-        // BUG: `failed = total - completed` misclassifies non-failure events as failures.
+        // Regression: `failed = total - completed` used to misclassify non-failure events.
         // 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"));
@@ -1425,8 +1425,8 @@ mod tests {
             .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
+        // Only "review.failed" should count as a failure (1 out of 3).
+        // "review.timeout" is a separate event type and should not inflate error_rate.
         let expected_error_rate = 1.0 / 3.0;
         assert!(
             (stats.error_rate - expected_error_rate).abs() < 0.01,