From 77e4d988edd217304c8163a86e65f9b0d19a8c4e Mon Sep 17 00:00:00 2001 From: David Hagerty Date: Fri, 13 Feb 2026 09:53:08 -0500 Subject: [PATCH] fix: address code review feedback for Phase 5 - Critical Issue: Filter node_modules and target directories in code search. The filter_entry closure now checks skip_dirs for ALL directories, not just those starting with '.'. This ensures node_modules and target are properly skipped. Added unit test code_search_skips_node_modules_and_target to verify. - Important Issue: Collapse nested if-let chains in orchestrator.rs using Rust 2024 let-chains. The try_unblock_tasks method now uses a single if let ... && let ... condition instead of nested blocks, resolving the clippy collapsible_if warning. - Minor Issue: Tighten assertion in code_search_skips_binary_files test. Removed the vacuous disjunction - the '.' regex always matches text.txt, so the assertion now directly checks for "text.txt" presence. Verification: - All 7 code_search unit tests pass - Full test suite passes (all 100+ tests) - cargo clippy clean (collapsible_if warning resolved) - cargo build successful Co-Authored-By: Claude Opus 4.6 --- src/agent/orchestrator.rs | 48 +++++++++++++++++++-------------------- src/tools/search.rs | 47 +++++++++++++++++++++++++++++++++----- 2 files changed, 65 insertions(+), 30 deletions(-) diff --git a/src/agent/orchestrator.rs b/src/agent/orchestrator.rs index bed9f5c..1ee92fb 100644 --- a/src/agent/orchestrator.rs +++ b/src/agent/orchestrator.rs @@ -1141,31 +1141,31 @@ impl Orchestrator { for task in &blocked_tasks { // Look up blocker task ID from metadata (set during cascade) - if let Some(blocker_id) = task.metadata.get("blocker_task_id") { - if let Some(blocker) = self.graph_store.get_node(blocker_id).await? { - // If the blocker has been retried and is now completed, unblock - if blocker.status == NodeStatus::Completed { - // Remove blocker_task_id from metadata when unblocking - let mut metadata = task.metadata.clone(); - metadata.remove("blocker_task_id"); - - self.graph_store - .update_node( - &task.id, - Some(NodeStatus::Ready), - None, // title unchanged - None, // description unchanged - Some(""), // clear blocked_reason by setting to empty string - Some(&metadata), // metadata with blocker_task_id removed - ) - .await?; + if let Some(blocker_id) = task.metadata.get("blocker_task_id") + && let Some(blocker) = self.graph_store.get_node(blocker_id).await? + { + // If the blocker has been retried and is now completed, unblock + if blocker.status == NodeStatus::Completed { + // Remove blocker_task_id from metadata when unblocking + let mut metadata = task.metadata.clone(); + metadata.remove("blocker_task_id"); + + self.graph_store + .update_node( + &task.id, + Some(NodeStatus::Ready), + None, // title unchanged + None, // description unchanged + Some(""), // clear blocked_reason by setting to empty string + Some(&metadata), // metadata with blocker_task_id removed + ) + .await?; - tracing::info!( - task = %task.id, - blocker = %blocker_id, - "Task unblocked: dependency resolved" - ); - } + tracing::info!( + task = %task.id, + blocker = %blocker_id, + "Task unblocked: dependency resolved" + ); } } } diff --git a/src/tools/search.rs b/src/tools/search.rs index 4cf0597..510a28b 100644 --- a/src/tools/search.rs +++ b/src/tools/search.rs @@ -92,11 +92,10 @@ impl Tool for CodeSearchTool { // Walk directory tree for entry in WalkDir::new(&search_root).into_iter().filter_entry(|e| { - // Skip hidden directories - let file_name = e.file_name().to_string_lossy(); - if e.file_type().is_dir() && file_name.starts_with('.') { - let name_str = file_name.as_ref(); - !skip_dirs.contains(&name_str) + // Skip directories in skip_dirs list + if e.file_type().is_dir() { + let file_name = e.file_name().to_string_lossy(); + !skip_dirs.contains(&file_name.as_ref()) } else { true } @@ -269,7 +268,7 @@ mod tests { // Should not fail, just skip the binary let result = tool.execute(params).await.unwrap(); - assert!(result.contains("text.txt") || result.contains("No matches found")); + assert!(result.contains("text.txt")); } #[tokio::test] @@ -312,4 +311,40 @@ mod tests { let result = tool.execute(params).await.unwrap(); assert_eq!(result, "No matches found"); } + + #[tokio::test] + async fn code_search_skips_node_modules_and_target() { + let temp_dir = TempDir::new().unwrap(); + let project_root = temp_dir.path().to_path_buf(); + + // Create files in regular directory + std::fs::write(project_root.join("main.rs"), "fn main() {}").unwrap(); + + // Create files in node_modules (should be skipped) + std::fs::create_dir_all(project_root.join("node_modules")).unwrap(); + std::fs::write( + project_root.join("node_modules/package.txt"), + "fn should_skip", + ) + .unwrap(); + + // Create files in target (should be skipped) + std::fs::create_dir_all(project_root.join("target")).unwrap(); + std::fs::write( + project_root.join("target/artifact.rs"), + "fn should_skip", + ) + .unwrap(); + + let tool = CodeSearchTool::new(project_root); + let params = json!({ + "pattern": "fn" + }); + + let result = tool.execute(params).await.unwrap(); + // Should find main.rs but NOT files in node_modules or target + assert!(result.contains("main.rs")); + assert!(!result.contains("node_modules")); + assert!(!result.contains("target")); + } } -- 2.51.2