Fix: Prevent cross-contamination of cache data across analysis modes for analyze tool (#5075)

crates/goose-mcp/src/developer/analyze/cache.rs

@@ -5,7 +5,7 @@ use std::sync::{Arc, Mutex};
 use std::time::SystemTime;
 
 use super::lock_or_recover;
-use crate::developer::analyze::types::AnalysisResult;
+use crate::developer::analyze::types::{AnalysisMode, AnalysisResult};
 
 #[derive(Clone)]
 pub struct AnalysisCache {
@@ -18,6 +18,7 @@ pub struct AnalysisCache {
 struct CacheKey {
     path: PathBuf,
     modified: SystemTime,
+    mode: AnalysisMode,
 }
 
 impl AnalysisCache {
@@ -35,30 +36,43 @@ impl AnalysisCache {
         }
     }
 
-    pub fn get(&self, path: &PathBuf, modified: SystemTime) -> Option<AnalysisResult> {
+    pub fn get(
+        &self,
+        path: &PathBuf,
+        modified: SystemTime,
+        mode: &AnalysisMode,
+    ) -> Option<AnalysisResult> {
         let mut cache = lock_or_recover(&self.cache, |c| c.clear());
         let key = CacheKey {
             path: path.clone(),
             modified,
+            mode: *mode,
         };
 
         if let Some(result) = cache.get(&key) {
-            tracing::trace!("Cache hit for {:?}", path);
+            tracing::trace!("Cache hit for {:?} in {:?} mode", path, mode);
             Some((**result).clone())
         } else {
-            tracing::trace!("Cache miss for {:?}", path);
+            tracing::trace!("Cache miss for {:?} in {:?} mode", path, mode);
             None
         }
     }
 
-    pub fn put(&self, path: PathBuf, modified: SystemTime, result: AnalysisResult) {
+    pub fn put(
+        &self,
+        path: PathBuf,
+        modified: SystemTime,
+        mode: &AnalysisMode,
+        result: AnalysisResult,
+    ) {
         let mut cache = lock_or_recover(&self.cache, |c| c.clear());
         let key = CacheKey {
             path: path.clone(),
             modified,
+            mode: *mode,
         };
 
-        tracing::trace!("Caching result for {:?}", path);
+        tracing::trace!("Caching result for {:?} in {:?} mode", path, mode);
         cache.put(key, Arc::new(result));
     }
 

crates/goose-mcp/src/developer/analyze/mod.rs

@@ -179,7 +179,7 @@ impl CodeAnalyzer {
             )
         })?;
 
-        if let Some(cached) = self.cache.get(&path.to_path_buf(), modified) {
+        if let Some(cached) = self.cache.get(&path.to_path_buf(), modified, mode) {
             tracing::trace!("Using cached result for {:?}", path);
             return Ok(cached);
         }
@@ -224,7 +224,8 @@ impl CodeAnalyzer {
 
         result.line_count = line_count;
 
-        self.cache.put(path.to_path_buf(), modified, result.clone());
+        self.cache
+            .put(path.to_path_buf(), modified, mode, result.clone());
 
         Ok(result)
     }

crates/goose-mcp/src/developer/analyze/tests/cache_tests.rs

@@ -1,7 +1,7 @@
 // Tests for the cache module
 
 use crate::developer::analyze::cache::AnalysisCache;
-use crate::developer::analyze::types::{AnalysisResult, FunctionInfo};
+use crate::developer::analyze::types::{AnalysisMode, AnalysisResult, FunctionInfo};
 use std::path::PathBuf;
 use std::time::SystemTime;
 
@@ -32,15 +32,15 @@ fn test_cache_hit_miss() {
     let result = create_test_result();
 
     // Initial miss
-    assert!(cache.get(&path, time).is_none());
+    assert!(cache.get(&path, time, &AnalysisMode::Semantic).is_none());
 
     // Store and hit
-    cache.put(path.clone(), time, result.clone());
-    assert!(cache.get(&path, time).is_some());
+    cache.put(path.clone(), time, &AnalysisMode::Semantic, result.clone());
+    assert!(cache.get(&path, time, &AnalysisMode::Semantic).is_some());
 
     // Different time = miss
     let later = time + std::time::Duration::from_secs(1);
-    assert!(cache.get(&path, later).is_none());
+    assert!(cache.get(&path, later, &AnalysisMode::Semantic).is_none());
 }
 
 #[test]
@@ -50,18 +50,39 @@ fn test_cache_eviction() {
     let time = SystemTime::now();
 
     // Fill cache
-    cache.put(PathBuf::from("file1.rs"), time, result.clone());
-    cache.put(PathBuf::from("file2.rs"), time, result.clone());
+    cache.put(
+        PathBuf::from("file1.rs"),
+        time,
+        &AnalysisMode::Semantic,
+        result.clone(),
+    );
+    cache.put(
+        PathBuf::from("file2.rs"),
+        time,
+        &AnalysisMode::Semantic,
+        result.clone(),
+    );
     assert_eq!(cache.len(), 2);
 
     // Add third item, should evict first
-    cache.put(PathBuf::from("file3.rs"), time, result.clone());
+    cache.put(
+        PathBuf::from("file3.rs"),
+        time,
+        &AnalysisMode::Semantic,
+        result.clone(),
+    );
     assert_eq!(cache.len(), 2);
 
     // First item should be evicted
-    assert!(cache.get(&PathBuf::from("file1.rs"), time).is_none());
-    assert!(cache.get(&PathBuf::from("file2.rs"), time).is_some());
-    assert!(cache.get(&PathBuf::from("file3.rs"), time).is_some());
+    assert!(cache
+        .get(&PathBuf::from("file1.rs"), time, &AnalysisMode::Semantic)
+        .is_none());
+    assert!(cache
+        .get(&PathBuf::from("file2.rs"), time, &AnalysisMode::Semantic)
+        .is_some());
+    assert!(cache
+        .get(&PathBuf::from("file3.rs"), time, &AnalysisMode::Semantic)
+        .is_some());
 }
 
 #[test]
@@ -71,12 +92,12 @@ fn test_cache_clear() {
     let time = SystemTime::now();
     let result = create_test_result();
 
-    cache.put(path.clone(), time, result);
+    cache.put(path.clone(), time, &AnalysisMode::Semantic, result);
     assert!(!cache.is_empty());
 
     cache.clear();
     assert!(cache.is_empty());
-    assert!(cache.get(&path, time).is_none());
+    assert!(cache.get(&path, time, &AnalysisMode::Semantic).is_none());
 }
 
 #[test]
@@ -89,6 +110,31 @@ fn test_cache_default() {
     let time = SystemTime::now();
     let result = create_test_result();
 
-    cache.put(path.clone(), time, result);
-    assert!(cache.get(&path, time).is_some());
+    cache.put(path.clone(), time, &AnalysisMode::Semantic, result);
+    assert!(cache.get(&path, time, &AnalysisMode::Semantic).is_some());
+}
+
+#[test]
+fn test_cache_mode_separation() {
+    let cache = AnalysisCache::new(10);
+    let path = PathBuf::from("test.rs");
+    let time = SystemTime::now();
+    let result = create_test_result();
+
+    // Store in structure mode
+    cache.put(path.clone(), time, &AnalysisMode::Structure, result.clone());
+    assert!(cache.get(&path, time, &AnalysisMode::Structure).is_some());
+
+    // Different mode should be a miss
+    assert!(cache.get(&path, time, &AnalysisMode::Semantic).is_none());
+
+    // Store in semantic mode
+    cache.put(path.clone(), time, &AnalysisMode::Semantic, result.clone());
+
+    // Both modes should now have cached results
+    assert!(cache.get(&path, time, &AnalysisMode::Structure).is_some());
+    assert!(cache.get(&path, time, &AnalysisMode::Semantic).is_some());
+
+    // Cache should contain 2 entries (one per mode)
+    assert_eq!(cache.len(), 2);
 }

crates/goose-mcp/src/developer/analyze/types.rs

@@ -131,7 +131,7 @@ pub struct FocusedAnalysisData<'a> {
 }
 
 /// Analysis modes
-#[derive(Debug, Clone, Copy, PartialEq)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
 pub enum AnalysisMode {
     Structure, // Directory overview
     Semantic,  // File details