fix: harden review prompts, feedback signals, and security heuristics

Author: haasonsaas

src/commands/misc.rs

@@ -220,16 +220,12 @@ pub async fn feedback_command(
         let now = chrono::Utc::now().to_rfc3339();
         let is_accepted = action == "accept";
         for comment in &comments {
-            let ext = comment
-                .file_path
-                .extension()
-                .and_then(|e| e.to_str())
-                .map(|e| format!("*.{}", e));
+            let file_patterns = review::derive_file_patterns(&comment.file_path);
             cstore.record_feedback(
                 &comment.content,
                 &comment.category.to_string(),
                 is_accepted,
-                ext.as_deref(),
+                file_patterns.first().map(String::as_str),
                 &now,
             );
         }
@@ -253,6 +249,8 @@ fn apply_feedback_accept(store: &mut review::FeedbackStore, comments: &[core::Co
             let key = review::classify_comment_type(comment).as_str().to_string();
             let stats = store.by_comment_type.entry(key).or_default();
             stats.accepted = stats.accepted.saturating_add(1);
+            let file_patterns = review::derive_file_patterns(&comment.file_path);
+            store.record_feedback_patterns(&comment.category.to_string(), &file_patterns, true);
         }
         store.suppress.remove(&comment.id);
     }
@@ -268,6 +266,8 @@ fn apply_feedback_reject(store: &mut review::FeedbackStore, comments: &[core::Co
             let key = review::classify_comment_type(comment).as_str().to_string();
             let stats = store.by_comment_type.entry(key).or_default();
             stats.rejected = stats.rejected.saturating_add(1);
+            let file_patterns = review::derive_file_patterns(&comment.file_path);
+            store.record_feedback_patterns(&comment.category.to_string(), &file_patterns, false);
         }
         store.accept.remove(&comment.id);
     }
@@ -565,5 +565,8 @@ mod tests {
             stats.accepted, 1,
             "Stats should only count 1 acceptance, not 2 (double-counting bug)"
         );
+        assert_eq!(store.by_category["Bug"].accepted, 1);
+        assert_eq!(store.by_file_pattern["*.rs"].accepted, 1);
+        assert_eq!(store.by_category_file_pattern["Bug|*.rs"].accepted, 1);
     }
 }

src/core/agent_tools.rs

@@ -2,6 +2,7 @@ use anyhow::Result;
 use async_trait::async_trait;
 use serde::{Deserialize, Serialize};
 use serde_json::json;
+use std::collections::HashMap;
 use std::path::PathBuf;
 use std::sync::Arc;
 
@@ -612,19 +613,34 @@ impl ReviewTool for GitBlameTool {
 
         // git2::Repository is not Send, so we must open it inside spawn_blocking
         let result = tokio::task::spawn_blocking(move || -> Result<String> {
-            let repo = git2::Repository::open(&repo_path)?;
-            let blame = match repo.blame_file(std::path::Path::new(&file_path_owned), None) {
-                Ok(b) => b,
-                Err(e) => return Ok(format!("Error: could not blame file: {}", e)),
-            };
-
-            // Read file to count lines
+            // Read file first so we can validate and clamp the requested range.
             let content = std::fs::read_to_string(repo_path.join(&file_path_owned))?;
             let line_count = content.lines().count();
 
+            if line_count == 0 {
+                return Ok("No blame data available for the specified range.".to_string());
+            }
+
             let start = start_line.unwrap_or(1).max(1);
             let end = end_line.unwrap_or(line_count).min(line_count);
+            if start > end {
+                return Ok("No blame data available for the specified range.".to_string());
+            }
+
+            let repo = git2::Repository::open(&repo_path)?;
+            let mut blame_options = git2::BlameOptions::new();
+            blame_options.min_line(start);
+            blame_options.max_line(end);
+
+            let blame = match repo.blame_file(
+                std::path::Path::new(&file_path_owned),
+                Some(&mut blame_options),
+            ) {
+                Ok(b) => b,
+                Err(e) => return Ok(format!("Error: could not blame file: {}", e)),
+            };
 
+            let mut commit_message_cache: HashMap<git2::Oid, String> = HashMap::new();
             let mut output = String::new();
             for line_num in start..=end {
                 if let Some(hunk) = blame.get_line(line_num) {
@@ -639,14 +655,18 @@ impl ReviewTool for GitBlameTool {
                         .unwrap_or_else(|| "unknown".to_string());
 
                     // Get commit message (first line only)
-                    let message = repo
-                        .find_commit(oid)
-                        .ok()
-                        .and_then(|c| {
-                            c.message()
-                                .map(|m| m.lines().next().unwrap_or("").to_string())
+                    let message = commit_message_cache
+                        .entry(oid)
+                        .or_insert_with(|| {
+                            repo.find_commit(oid)
+                                .ok()
+                                .and_then(|c| {
+                                    c.message()
+                                        .map(|m| m.lines().next().unwrap_or("").to_string())
+                                })
+                                .unwrap_or_default()
                         })
-                        .unwrap_or_default();
+                        .clone();
 
                     output.push_str(&format!(
                         "L{}: {} ({}) [{}] {}\n",