fix(forge): address validation issues in Forge orchestration system

echobt · echobt · commit 548f7271696f · 2026-02-02T19:57:34.000Z
- Fix doctest in protocol.rs to use correct exported types from agents/mod.rs
  (ValidationStatus::Passed, RuleInfo::new with 3 args, is_passed() method)
- Add proper semver comparison in security.rs instead of string comparison
  (fixes version comparison for cases like '0.4.9' vs '0.4.20')
- Fix scroll_offset persistence in forge.rs view (now properly persists across renders)
- Add robust test file detection using path boundary checks instead of substring contains
  (avoids false positives for 'contest.rs', 'attestation.rs', etc.)
- Replace Arc::try_unwrap with lock().clone() in orchestrator for robustness
  (handles fail-fast early exit when tasks still hold Arc clones)
- Fix unwrap_or(&amp;mut 0) pattern with proper if-let handling
- Apply clippy fixes (is_some_and, pattern arrays, redundant closures)
diff --git a/src/cortex-agents/src/forge/agents/quality.rs b/src/cortex-agents/src/forge/agents/quality.rs
@@ -292,11 +292,8 @@ impl QualityAgent {
 
             // Check for bare .unwrap() calls (not followed by expect-like context)
             if line.contains(".unwrap()") {
-                // Check if it's in a test (heuristic: file or function name contains test)
-                let is_test_file = file_path.to_string_lossy().contains("test")
-                    || file_path
-                        .file_stem()
-                        .map_or(false, |s| s.to_string_lossy().ends_with("_test"));
+                // Check if it's in a test file using robust heuristics
+                let is_test_file = is_test_path(file_path);
 
                 if !is_test_file {
                     // Check if the next line or same line has a comment explaining it
@@ -659,7 +656,7 @@ async fn collect_files_recursive(
 
         if path.is_dir() {
             collect_files_recursive(&path, ctx, files).await?;
-        } else if path.is_file() && path.extension().map_or(false, |ext| ext == "rs") {
+        } else if path.is_file() && path.extension().is_some_and(|ext| ext == "rs") {
             files.push(path);
         }
     }
@@ -677,6 +674,52 @@ fn truncate_line(line: &str, max_len: usize) -> String {
     }
 }
 
+/// Check if a file path represents a test file.
+///
+/// Uses robust heuristics to detect test files:
+/// - Files in a `tests/` or `test/` directory
+/// - Files ending with `_test.rs` or `_tests.rs`
+/// - Files named `test.rs` or `tests.rs`
+/// - Files containing `/tests/` or `/test/` in their path
+fn is_test_path(file_path: &Path) -> bool {
+    let path_str = file_path.to_string_lossy();
+
+    // Check for standard test directories (with boundary detection)
+    // The path separators ensure we don't match "contest/", "latest/", etc.
+    if path_str.contains("/tests/")
+        || path_str.contains("/test/")
+        || path_str.starts_with("tests/")
+        || path_str.starts_with("test/")
+    {
+        return true;
+    }
+
+    // Check file stem (name without extension)
+    if let Some(file_stem) = file_path.file_stem() {
+        let stem = file_stem.to_string_lossy();
+
+        // Check for test file naming conventions
+        if stem == "test" || stem == "tests" || stem.ends_with("_test") || stem.ends_with("_tests")
+        {
+            return true;
+        }
+
+        // Check for `mod tests` pattern (file named `tests.rs`)
+        if stem == "mod" {
+            if let Some(parent) = file_path.parent() {
+                if let Some(parent_name) = parent.file_name() {
+                    let parent_str = parent_name.to_string_lossy();
+                    if parent_str == "tests" || parent_str == "test" {
+                        return true;
+                    }
+                }
+            }
+        }
+    }
+
+    false
+}
+
 /// Extract item name from a Rust declaration.
 fn extract_item_name(line: &str) -> String {
     // Simple extraction - works for basic cases
@@ -685,7 +728,7 @@ fn extract_item_name(line: &str) -> String {
         // pub fn name, pub struct Name, etc.
         let name = parts[2];
         // Remove generic params and parentheses
-        name.split(|c| c == '<' || c == '(' || c == '{')
+        name.split(['<', '(', '{'])
             .next()
             .unwrap_or(name)
             .to_string()
@@ -735,4 +778,30 @@ mod tests {
         assert_eq!(agent.name(), "Code Quality Agent");
         assert!(agent.dependencies().is_empty());
     }
+
+    #[test]
+    fn test_is_test_path() {
+        use std::path::Path;
+
+        // Should detect test files
+        assert!(is_test_path(Path::new("tests/unit.rs")));
+        assert!(is_test_path(Path::new("src/tests/mod.rs")));
+        assert!(is_test_path(Path::new("module_test.rs")));
+        assert!(is_test_path(Path::new("foo_tests.rs")));
+        assert!(is_test_path(Path::new("/project/tests/integration.rs")));
+        assert!(is_test_path(Path::new("test/helper.rs")));
+
+        // Should NOT detect non-test files that contain 'test' substring
+        assert!(!is_test_path(Path::new("contest.rs")));
+        assert!(!is_test_path(Path::new("contest_manager.rs")));
+        assert!(!is_test_path(Path::new("latest_report.rs")));
+        assert!(!is_test_path(Path::new("greatest.rs")));
+        assert!(!is_test_path(Path::new("src/attestation.rs")));
+        assert!(!is_test_path(Path::new("testable_trait.rs"))); // Not a test file itself
+
+        // Regular source files
+        assert!(!is_test_path(Path::new("src/main.rs")));
+        assert!(!is_test_path(Path::new("src/lib.rs")));
+        assert!(!is_test_path(Path::new("src/module/mod.rs")));
+    }
 }
diff --git a/src/cortex-agents/src/forge/agents/security.rs b/src/cortex-agents/src/forge/agents/security.rs
@@ -253,7 +253,7 @@ impl SecurityAgent {
         let files = collect_source_files(&ctx.project_path, ctx).await?;
 
         for file_path in files {
-            if file_path.extension().map_or(false, |ext| ext == "rs") {
+            if file_path.extension().is_some_and(|ext| ext == "rs") {
                 if let Err(e) = self
                     .scan_file_for_unsafe(&file_path, rule_id, severity, findings)
                     .await
@@ -333,7 +333,7 @@ impl SecurityAgent {
         let files = collect_source_files(&ctx.project_path, ctx).await?;
 
         for file_path in files {
-            if file_path.extension().map_or(false, |ext| ext == "rs") {
+            if file_path.extension().is_some_and(|ext| ext == "rs") {
                 if let Err(e) = self
                     .scan_file_for_input_issues(&file_path, rule_id, severity, findings)
                     .await
@@ -637,16 +637,48 @@ fn parse_cargo_lock(content: &str) -> Vec<(String, String)> {
 }
 
 /// Check if a version matches a vulnerable version range.
-/// This is a simplified check - in production would use semver properly.
+/// Uses semantic version comparison for accurate results.
 fn version_matches_vulnerable(version: &str, vulnerable_pattern: &str) -> bool {
     if let Some(max_version) = vulnerable_pattern.strip_prefix('<') {
-        // Simple string comparison - in production use semver crate
-        version < max_version
+        // Parse versions and compare semantically
+        compare_semver(version, max_version) == Some(std::cmp::Ordering::Less)
     } else {
         version == vulnerable_pattern
     }
 }
 
+/// Compare two semantic versions.
+/// Returns Ordering::Less if a < b, Equal if a == b, Greater if a > b.
+/// Returns None if parsing fails.
+fn compare_semver(a: &str, b: &str) -> Option<std::cmp::Ordering> {
+    let parse_version = |v: &str| -> Option<Vec<u64>> {
+        v.split('.')
+            .map(|part| {
+                // Handle versions with pre-release suffixes like "1.0.0-alpha"
+                let numeric_part = part.split('-').next()?;
+                numeric_part.parse::<u64>().ok()
+            })
+            .collect()
+    };
+
+    let a_parts = parse_version(a)?;
+    let b_parts = parse_version(b)?;
+
+    // Compare each component
+    let max_len = a_parts.len().max(b_parts.len());
+    for i in 0..max_len {
+        let a_val = a_parts.get(i).copied().unwrap_or(0);
+        let b_val = b_parts.get(i).copied().unwrap_or(0);
+
+        match a_val.cmp(&b_val) {
+            std::cmp::Ordering::Equal => continue,
+            other => return Some(other),
+        }
+    }
+
+    Some(std::cmp::Ordering::Equal)
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -681,9 +713,42 @@ version = "1.20.0"
 
     #[test]
     fn test_version_matches_vulnerable() {
+        // Basic comparison
         assert!(version_matches_vulnerable("0.4.19", "<0.4.20"));
         assert!(!version_matches_vulnerable("0.4.20", "<0.4.20"));
         assert!(!version_matches_vulnerable("0.5.0", "<0.4.20"));
+
+        // Semver edge cases - string comparison would fail these
+        assert!(version_matches_vulnerable("0.4.9", "<0.4.20")); // '9' > '2' in string comparison
+        assert!(version_matches_vulnerable("0.4.1", "<0.4.20"));
+        assert!(!version_matches_vulnerable("0.4.21", "<0.4.20"));
+        
+        // Major/minor version comparisons
+        assert!(version_matches_vulnerable("0.3.99", "<0.4.20"));
+        assert!(!version_matches_vulnerable("1.0.0", "<0.4.20"));
+        
+        // Exact match
+        assert!(version_matches_vulnerable("1.0.0", "1.0.0"));
+        assert!(!version_matches_vulnerable("1.0.1", "1.0.0"));
+    }
+
+    #[test]
+    fn test_compare_semver() {
+        use std::cmp::Ordering;
+        
+        assert_eq!(compare_semver("1.0.0", "2.0.0"), Some(Ordering::Less));
+        assert_eq!(compare_semver("2.0.0", "1.0.0"), Some(Ordering::Greater));
+        assert_eq!(compare_semver("1.0.0", "1.0.0"), Some(Ordering::Equal));
+        
+        // Multi-digit version numbers
+        assert_eq!(compare_semver("0.4.9", "0.4.20"), Some(Ordering::Less));
+        assert_eq!(compare_semver("0.4.19", "0.4.20"), Some(Ordering::Less));
+        assert_eq!(compare_semver("0.4.20", "0.4.20"), Some(Ordering::Equal));
+        assert_eq!(compare_semver("0.4.21", "0.4.20"), Some(Ordering::Greater));
+        
+        // Different number of components
+        assert_eq!(compare_semver("1.0", "1.0.0"), Some(Ordering::Equal));
+        assert_eq!(compare_semver("1.0.0", "1.0"), Some(Ordering::Equal));
     }
 
     #[test]
diff --git a/src/cortex-agents/src/forge/orchestrator.rs b/src/cortex-agents/src/forge/orchestrator.rs
@@ -416,14 +416,17 @@ impl ForgeOrchestrator {
             }
         }
 
-        // Collect final results
-        let final_results = Arc::try_unwrap(results)
-            .map_err(|_| OrchestratorError::Internal("Failed to unwrap results".to_string()))?
-            .into_inner();
+        // Collect final results - use lock() which is more robust than try_unwrap()
+        // when tasks might still be running (e.g., after fail-fast early exit)
+        let final_results = {
+            let guard = results.lock().await;
+            guard.clone()
+        };
 
-        let final_errors = Arc::try_unwrap(errors)
-            .map_err(|_| OrchestratorError::Internal("Failed to unwrap errors".to_string()))?
-            .into_inner();
+        let final_errors = {
+            let guard = errors.lock().await;
+            guard.clone()
+        };
 
         let execution_time_ms = start_time.elapsed().as_millis() as u64;
 
@@ -452,7 +455,10 @@ impl ForgeOrchestrator {
         for agent in enabled.values() {
             for dep in &agent.depends_on {
                 if enabled.contains_key(dep) {
-                    *in_degree.get_mut(&agent.id).unwrap_or(&mut 0) += 1;
+                    // Increment in-degree - entry must exist since we initialized all enabled agents
+                    if let Some(degree) = in_degree.get_mut(&agent.id) {
+                        *degree += 1;
+                    }
                 }
             }
         }
diff --git a/src/cortex-agents/src/forge/protocol.rs b/src/cortex-agents/src/forge/protocol.rs
@@ -1,27 +1,27 @@
 //! Agent validation protocol for Forge orchestration.
 //!
-//! This module defines the protocol for agent validation results,
+//! This module defines protocol types for agent validation results,
 //! including status, findings, and aggregated responses.
 //!
+//! Note: The primary types for validation are re-exported at `cortex_agents::forge`
+//! from the `agents` module. This protocol module provides additional types
+//! for the orchestrator, including `ForgeResponse` and `Location` types.
+//!
 //! # Example
 //!
 //! ```rust
 //! use cortex_agents::forge::{
-//!     ValidationStatus, ValidationResult, Finding, Severity, RuleInfo, ForgeResponse,
+//!     ValidationStatus, ValidationResult, Finding, Severity, RuleInfo,
 //! };
-//! use chrono::Utc;
 //!
-//! let result = ValidationResult {
-//!     status: ValidationStatus::Pass,
-//!     agent_id: "security-scanner".to_string(),
-//!     rules_applied: vec![
-//!         RuleInfo::new("SEC001", "No secrets in code"),
-//!     ],
-//!     findings: vec![],
-//!     timestamp: Utc::now(),
-//! };
+//! // Create a validation result using the builder pattern
+//! let mut result = ValidationResult::new("security-scanner");
+//! 
+//! // Add a finding
+//! result.add_finding(Finding::new("SEC001", Severity::Warning, "Potential issue detected"));
 //!
-//! assert!(result.is_success());
+//! // Check result status
+//! assert!(result.is_passed()); // No errors or criticals = passed
 //! ```
 
 use chrono::{DateTime, Utc};
diff --git a/src/cortex-tui/src/views/forge.rs b/src/cortex-tui/src/views/forge.rs
@@ -356,7 +356,7 @@ impl ForgeView {
     }
 
     /// Renders the Forge view.
-    pub fn render(&self, frame: &mut Frame, area: Rect) {
+    pub fn render(&mut self, frame: &mut Frame, area: Rect) {
         // Clear the area
         frame.render_widget(Clear, area);
 
@@ -489,7 +489,7 @@ impl ForgeView {
 
                 let duration_str = agent
                     .duration_ms
-                    .map(|ms| format_duration_ms(ms))
+                    .map(format_duration_ms)
                     .unwrap_or_else(|| "-".to_string());
 
                 Row::new(vec![
@@ -518,7 +518,7 @@ impl ForgeView {
     }
 
     /// Renders the findings panel.
-    fn render_findings_panel(&self, frame: &mut Frame, area: Rect) {
+    fn render_findings_panel(&mut self, frame: &mut Frame, area: Rect) {
         let is_active = self.active_panel == ForgePanel::Findings;
         let border_color = if is_active {
             Color::Cyan
@@ -546,11 +546,11 @@ impl ForgeView {
         // Calculate visible rows
         let visible_rows = inner.height as usize;
 
-        // Adjust scroll offset to keep selection visible
-        let scroll_offset = if self.selected_finding < self.scroll_offset {
+        // Adjust scroll offset to keep selection visible and persist it
+        self.scroll_offset = if self.selected_finding < self.scroll_offset {
             self.selected_finding
-        } else if self.selected_finding >= self.scroll_offset + visible_rows {
-            self.selected_finding - visible_rows + 1
+        } else if visible_rows > 0 && self.selected_finding >= self.scroll_offset + visible_rows {
+            self.selected_finding.saturating_sub(visible_rows.saturating_sub(1))
         } else {
             self.scroll_offset
         };
@@ -562,7 +562,7 @@ impl ForgeView {
             .findings
             .iter()
             .enumerate()
-            .skip(scroll_offset)
+            .skip(self.scroll_offset)
             .take(visible_rows)
         {
             let is_selected = idx == self.selected_finding && is_active;
