fix: 7 TDD-discovered bugs — glob escaping, severity parsing, confidence normalization, time filters, negative offsets

- interactive.rs: Fix glob-to-regex to escape dots and anchor patterns
  (*.test.js no longer matches fooAtestBjs or foo.test.js.bak)
- smart_response.rs: Accept standard severity names (error/warning/info/suggestion)
  alongside prompt names (critical/high/medium/low)
- smart_response.rs: Treat bare decimals in 0..1 range as already-normalized
  confidence (0.85 → 85%, not 0.0085)
- storage_json.rs: Exclude events with created_at=None when time filters
  are active (is_none_or → is_some_and)
- storage_json.rs: Clamp negative limit/offset to 0 to prevent usize
  wraparound from negative i64 cast

All bugs discovered via TDD: failing test written first, then minimal fix.
977 tests pass.

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

Author: haasonsaas

src/core/interactive.rs

@@ -426,6 +426,40 @@ mod tests {
         assert!(processor.should_ignore("types.generated.ts"));
         assert!(!processor.should_ignore("src/main.rs"));
     }
+
+    // ── Bug: glob dots are not escaped before regex conversion ──────────
+    //
+    // `should_ignore` converts glob patterns to regex by replacing `*`
+    // with `.*`, but does NOT escape the `.` characters in the pattern.
+    // As a result, "*.test.js" becomes regex `".*..test..js"` where the
+    // dots match ANY character, causing false positives.
+    //
+    // For example, "fooAtestBjs" matches because the unescaped dots in
+    // the regex accept any character, not just literal periods.
+
+    #[test]
+    fn test_processor_glob_dot_is_literal() {
+        let mut processor = InteractiveProcessor::new();
+        processor.add_ignore_pattern("*.test.js");
+        // Should match files with literal dots
+        assert!(processor.should_ignore("foo.test.js"));
+        // Should NOT match when dots are replaced by other characters
+        assert!(
+            !processor.should_ignore("fooAtestBjs"),
+            "Glob dot should match literal '.' only, not arbitrary characters"
+        );
+    }
+
+    #[test]
+    fn test_processor_glob_anchored() {
+        let mut processor = InteractiveProcessor::new();
+        processor.add_ignore_pattern("*.test.js");
+        // Should NOT match files with a suffix after .js
+        assert!(
+            !processor.should_ignore("foo.test.js.bak"),
+            "Glob pattern should not match files with extra suffixes"
+        );
+    }
 }
 
 /// Manages per-session ignore patterns from @diffscope ignore commands.
@@ -451,7 +485,9 @@ impl InteractiveProcessor {
         self.ignored_patterns.iter().any(|pattern| {
             // Simple glob matching
             if pattern.contains('*') {
-                let regex_pattern = pattern.replace("*", ".*");
+                // Escape regex metacharacters, then convert glob * to .*
+                let escaped = regex::escape(pattern).replace(r"\*", ".*");
+                let regex_pattern = format!("^{}$", escaped);
                 regex::Regex::new(&regex_pattern)
                     .map(|re| re.is_match(path))
                     .unwrap_or(false)

src/parsing/smart_response.rs

@@ -143,10 +143,10 @@ fn append_suggestion(suggestion: &mut Option<String>, value: &str) {
 
 pub fn parse_smart_severity(value: &str) -> Option<core::comment::Severity> {
     match value.to_lowercase().as_str() {
-        "critical" => Some(core::comment::Severity::Error),
-        "high" => Some(core::comment::Severity::Warning),
-        "medium" => Some(core::comment::Severity::Info),
-        "low" => Some(core::comment::Severity::Suggestion),
+        "critical" | "error" => Some(core::comment::Severity::Error),
+        "high" | "warning" => Some(core::comment::Severity::Warning),
+        "medium" | "info" => Some(core::comment::Severity::Info),
+        "low" | "suggestion" => Some(core::comment::Severity::Suggestion),
         _ => None,
     }
 }
@@ -169,9 +169,17 @@ pub fn parse_smart_category(value: &str) -> Option<core::comment::Category> {
 }
 
 pub fn parse_smart_confidence(value: &str) -> Option<f32> {
-    let trimmed = value.trim().trim_end_matches('%');
-    if let Ok(percent) = trimmed.parse::<f32>() {
-        Some((percent / 100.0).clamp(0.0, 1.0))
+    let raw = value.trim();
+    let has_percent = raw.ends_with('%');
+    let trimmed = raw.trim_end_matches('%');
+    if let Ok(num) = trimmed.parse::<f32>() {
+        if !has_percent && num >= 0.0 && num <= 1.0 {
+            // Bare decimal in 0..1 range — treat as already-normalized confidence
+            Some(num.clamp(0.0, 1.0))
+        } else {
+            // Percentage value (e.g., "85" or "85%") — divide by 100
+            Some((num / 100.0).clamp(0.0, 1.0))
+        }
     } else {
         None
     }
@@ -376,4 +384,68 @@ TAGS: auth, security
         let tags = parse_smart_tags("auth,,, security");
         assert_eq!(tags, vec!["auth", "security"]);
     }
+
+    // ── Bug: parse_smart_severity rejects standard severity names ────────
+    //
+    // The prompt instructs the LLM to use CRITICAL|HIGH|MEDIUM|LOW, but
+    // parse_rule_severity_override (in rule_helpers.rs) accepts both name
+    // families.  LLMs frequently output the canonical Severity enum names
+    // (e.g. "SEVERITY: Error" or "SEVERITY: Warning") and those silently
+    // become None, losing severity information.
+
+    #[test]
+    fn parse_smart_severity_accepts_standard_names() {
+        // "error" / "warning" / "info" / "suggestion" are the enum names
+        // used everywhere else in the codebase and are commonly output by
+        // LLMs. parse_smart_severity should accept them.
+        assert_eq!(
+            parse_smart_severity("error"),
+            Some(core::comment::Severity::Error),
+            "\"error\" should map to Severity::Error"
+        );
+        assert_eq!(
+            parse_smart_severity("warning"),
+            Some(core::comment::Severity::Warning),
+            "\"warning\" should map to Severity::Warning"
+        );
+        assert_eq!(
+            parse_smart_severity("info"),
+            Some(core::comment::Severity::Info),
+            "\"info\" should map to Severity::Info"
+        );
+        assert_eq!(
+            parse_smart_severity("suggestion"),
+            Some(core::comment::Severity::Suggestion),
+            "\"suggestion\" should map to Severity::Suggestion"
+        );
+    }
+
+    // ── Bug: parse_smart_confidence misinterprets bare decimals ──────────
+    //
+    // When an LLM outputs "CONFIDENCE: 0.85" (a bare float instead of
+    // "85%"), parse_smart_confidence divides by 100 producing 0.0085,
+    // which effectively discards every comment at the default confidence
+    // threshold.  Values <= 1.0 without a '%' suffix should be treated
+    // as already-normalized (0.0..1.0).
+
+    #[test]
+    fn parse_smart_confidence_bare_decimal_treated_as_fraction() {
+        // 0.85 is clearly a 0..1 confidence, not 0.85%
+        let conf = parse_smart_confidence("0.85").unwrap();
+        assert!(
+            (conf - 0.85).abs() < 0.001,
+            "0.85 should be treated as 85% confidence, got {}",
+            conf
+        );
+    }
+
+    #[test]
+    fn parse_smart_confidence_bare_decimal_zero_point_five() {
+        let conf = parse_smart_confidence("0.5").unwrap();
+        assert!(
+            (conf - 0.5).abs() < 0.001,
+            "0.5 should be treated as 50% confidence, got {}",
+            conf
+        );
+    }
 }

src/server/storage_json.rs

@@ -105,8 +105,8 @@ impl StorageBackend for JsonStorageBackend {
         let reviews = self.reviews.read().await;
         let mut list: Vec<&ReviewSession> = reviews.values().collect();
         list.sort_by(|a, b| b.started_at.cmp(&a.started_at));
-        let offset = offset as usize;
-        let limit = limit as usize;
+        let offset = offset.max(0) as usize;
+        let limit = limit.max(0) as usize;
         Ok(list.into_iter().skip(offset).take(limit).cloned().collect())
     }
 
@@ -142,15 +142,16 @@ impl StorageBackend for JsonStorageBackend {
                     .status
                     .as_ref()
                     .is_none_or(|f| e.event_type.eq_ignore_ascii_case(&format!("review.{}", f)));
-                // Time filters (best-effort for JSON backend using created_at if available)
+                // Time filters: if a time bound is specified, events without a
+                // timestamp are excluded (they cannot satisfy the constraint).
                 let time_from_ok = filters
                     .time_from
                     .as_ref()
-                    .is_none_or(|from| e.created_at.is_none_or(|t| t >= *from));
+                    .is_none_or(|from| e.created_at.is_some_and(|t| t >= *from));
                 let time_to_ok = filters
                     .time_to
                     .as_ref()
-                    .is_none_or(|to| e.created_at.is_none_or(|t| t <= *to));
+                    .is_none_or(|to| e.created_at.is_some_and(|t| t <= *to));
                 let repo_ok = filters
                     .github_repo
                     .as_ref()
@@ -167,8 +168,8 @@ impl StorageBackend for JsonStorageBackend {
         });
 
         // Apply limit/offset
-        let offset = filters.offset.unwrap_or(0) as usize;
-        let limit = filters.limit.unwrap_or(500) as usize;
+        let offset = filters.offset.unwrap_or(0).max(0) as usize;
+        let limit = filters.limit.unwrap_or(500).max(0) as usize;
         events = events.into_iter().skip(offset).take(limit).collect();
 
         Ok(events)
@@ -177,8 +178,8 @@ impl StorageBackend for JsonStorageBackend {
     async fn get_event_stats(&self, filters: &EventFilters) -> anyhow::Result<EventStats> {
         // Stats must cover ALL matching events, not a paginated subset
         let mut stats_filters = filters.clone();
-        stats_filters.limit = None;
-        stats_filters.offset = None;
+        stats_filters.limit = Some(i64::MAX);
+        stats_filters.offset = Some(0);
         let events = self.list_events(&stats_filters).await?;
 
         let total = events.len() as i64;
@@ -1497,6 +1498,123 @@ mod tests {
         assert_eq!(events[0].github_repo.as_deref(), Some("owner/repo-a"));
     }
 
+    // ── Bug: time filters include events with created_at = None ──────
+    //
+    // When a time_from or time_to filter is active, events whose
+    // `created_at` is None should be *excluded* (they have no timestamp
+    // to satisfy the constraint).  Previously `is_none_or` let them
+    // through.
+
+    #[tokio::test]
+    async fn test_time_filter_excludes_events_without_timestamp() {
+        let dir = tempfile::tempdir().unwrap();
+        let backend = JsonStorageBackend::new(&dir.path().join("reviews.json"));
+        let now = now_ts();
+
+        // Event WITH a timestamp (via build())
+        let s1 = make_session_with_event(
+            "r1",
+            now,
+            ReviewStatus::Complete,
+            "review.completed",
+            "gpt-4o",
+            "github",
+            100,
+        );
+        backend.save_review(&s1).await.unwrap();
+
+        // Event WITHOUT a timestamp (manually set created_at = None)
+        let mut s2 = make_session_with_event(
+            "r2",
+            now + 1,
+            ReviewStatus::Complete,
+            "review.completed",
+            "gpt-4o",
+            "github",
+            200,
+        );
+        s2.event.as_mut().unwrap().created_at = None;
+        backend.save_review(&s2).await.unwrap();
+
+        // Filter with time_from = epoch (should match everything WITH a ts)
+        let filters = EventFilters {
+            time_from: Some(chrono::DateTime::from_timestamp(0, 0).unwrap()),
+            ..Default::default()
+        };
+        let events = backend.list_events(&filters).await.unwrap();
+        assert_eq!(
+            events.len(),
+            1,
+            "Events with created_at = None should be excluded by time filters, got {}",
+            events.len()
+        );
+        assert_eq!(events[0].review_id, "r1");
+    }
+
+    // ── Bug: negative limit/offset wraps to huge usize ───────────────
+    //
+    // Casting a negative i64 directly to usize wraps to a very large
+    // number, causing list_reviews to skip/take billions of entries.
+
+    #[tokio::test]
+    async fn test_list_reviews_negative_offset_does_not_panic() {
+        let dir = tempfile::tempdir().unwrap();
+        let backend = JsonStorageBackend::new(&dir.path().join("reviews.json"));
+        let now = now_ts();
+
+        backend
+            .save_review(&make_session("r1", now, ReviewStatus::Complete))
+            .await
+            .unwrap();
+
+        // Negative offset and limit should not panic or return nonsense
+        let result = backend.list_reviews(10, -1).await.unwrap();
+        assert_eq!(result.len(), 1, "Negative offset should be clamped to 0");
+
+        let result = backend.list_reviews(-1, 0).await.unwrap();
+        assert!(
+            result.is_empty(),
+            "Negative limit should be clamped to 0, returning no results"
+        );
+    }
+
+    #[tokio::test]
+    async fn test_list_events_negative_offset_does_not_panic() {
+        let dir = tempfile::tempdir().unwrap();
+        let backend = JsonStorageBackend::new(&dir.path().join("reviews.json"));
+        let now = now_ts();
+
+        let s1 = make_session_with_event(
+            "r1",
+            now,
+            ReviewStatus::Complete,
+            "review.completed",
+            "gpt-4o",
+            "github",
+            100,
+        );
+        backend.save_review(&s1).await.unwrap();
+
+        let filters = EventFilters {
+            offset: Some(-5),
+            limit: Some(100),
+            ..Default::default()
+        };
+        let events = backend.list_events(&filters).await.unwrap();
+        assert_eq!(events.len(), 1, "Negative offset should be clamped to 0");
+
+        let filters = EventFilters {
+            offset: Some(0),
+            limit: Some(-10),
+            ..Default::default()
+        };
+        let events = backend.list_events(&filters).await.unwrap();
+        assert!(
+            events.is_empty(),
+            "Negative limit should be clamped to 0, returning no results"
+        );
+    }
+
     #[tokio::test]
     async fn test_prune_persists_to_disk() {
         // BUG: prune removes from memory but doesn't flush to disk