Merge pull request #63 from 100monkeys-ai/copilot/add-tests-for-error-extraction

Add tests for error extraction and sanitization, extract constants, rename variable for clarity

Author: Theaxiom

cli/src/daemon/client.rs

@@ -756,7 +756,62 @@ impl DaemonClient {
 
 #[cfg(test)]
 mod tests {
+    use super::extract_iteration_error_message;
     use super::WorkflowListResponse;
+    use serde_json::json;
+
+    #[test]
+    fn test_error_object_with_message_precedence() {
+        let event = json!({
+            "data": {
+                "error": {
+                    "message": "object message",
+                    "code": "SOME_CODE"
+                }
+            },
+            "error": "top-level error"
+        });
+
+        let msg = extract_iteration_error_message(&event);
+        assert_eq!(msg, "object message");
+    }
+
+    #[test]
+    fn test_error_string_in_data_precedence() {
+        let event = json!({
+            "data": {
+                "error": "data error string"
+            },
+            "error": "top-level error"
+        });
+
+        let msg = extract_iteration_error_message(&event);
+        assert_eq!(msg, "data error string");
+    }
+
+    #[test]
+    fn test_error_string_top_level_precedence() {
+        let event = json!({
+            "error": "top-level error"
+        });
+
+        let msg = extract_iteration_error_message(&event);
+        assert_eq!(msg, "top-level error");
+    }
+
+    #[test]
+    fn test_error_fallback_unknown() {
+        let event = json!({
+            "data": {
+                "error": {
+                    "not_message": "no message field here"
+                }
+            }
+        });
+
+        let msg = extract_iteration_error_message(&event);
+        assert_eq!(msg, "Unknown error");
+    }
 
     #[test]
     fn parses_wrapped_workflow_list_response() {

orchestrator/core/src/application/tool_invocation_service.rs

@@ -37,6 +37,8 @@ use crate::infrastructure::smcp_gateway_proto::{
 };
 use crate::infrastructure::workflow_parser::WorkflowParser;
 
+const JUDGE_POLL_INTERVAL_MS: u64 = 500;
+
 pub enum ToolInvocationResult {
     Direct(Value),
     DispatchRequired(DispatchAction),
@@ -432,8 +434,8 @@ impl ToolInvocationService {
             // judge pipeline. When the node config marks a tool with `skip_judge: true`
             // (e.g. read-only tools such as fs.read, fs.list), we bypass the judge entirely
             // to reduce latency on low-risk operations (NODE_CONFIGURATION_SPEC_V1.md).
-            let skip_tool_judge = self.tool_router.is_skip_judge(&tool_name).await;
-            if skip_tool_judge {
+            let should_skip_judge = self.tool_router.is_skip_judge(&tool_name).await;
+            if should_skip_judge {
                 tracing::debug!(
                     tool_name = %tool_name,
                     "Inner-loop semantic judge skipped (skip_judge=true in node config for this tool)"
@@ -527,7 +529,7 @@ impl ToolInvocationService {
                                 ))
                             })?;
 
-                        let poll_interval_ms = 500u64;
+                        let poll_interval_ms = JUDGE_POLL_INTERVAL_MS;
                         let timeout_ms = timeout_seconds.saturating_mul(1000);
                         let max_attempts = timeout_ms
                             .saturating_add(poll_interval_ms.saturating_sub(1))
@@ -1771,4 +1773,35 @@ mod tests {
             .unwrap();
         assert_eq!(exec_mode, "remote_jsonrpc");
     }
+
+    #[test]
+    fn sanitize_segment_handles_empty_and_whitespace() {
+        assert_eq!(sanitize_segment(""), "unversioned");
+        assert_eq!(sanitize_segment("   "), "unversioned");
+    }
+
+    #[test]
+    fn sanitize_segment_blocks_traversal_patterns() {
+        assert_eq!(sanitize_segment("."), "unversioned");
+        assert_eq!(sanitize_segment(".."), "unversioned");
+        assert_eq!(sanitize_segment("..hidden"), "unversioned");
+        assert_eq!(sanitize_segment("hidden.."), "unversioned");
+        assert_eq!(sanitize_segment("a..b"), "unversioned");
+        assert_eq!(sanitize_segment("version..1"), "unversioned");
+    }
+
+    #[test]
+    fn sanitize_segment_replaces_special_characters() {
+        assert_eq!(sanitize_segment("foo/bar"), "foo_bar");
+        assert_eq!(sanitize_segment("foo\\bar"), "foo_bar");
+        assert_eq!(sanitize_segment("foo:bar"), "foo_bar");
+        assert_eq!(sanitize_segment("foo bar"), "foo_bar");
+        assert_eq!(sanitize_segment("name@domain.com"), "name_domain.com");
+    }
+
+    #[test]
+    fn sanitize_segment_preserves_safe_mixed_alphanumeric() {
+        assert_eq!(sanitize_segment("validName-123"), "validName-123");
+        assert_eq!(sanitize_segment("v1.2.3-beta_01"), "v1.2.3-beta_01");
+    }
 }

orchestrator/core/src/infrastructure/temporal_event_listener.rs

@@ -66,10 +66,14 @@ use serde::{Deserialize, Serialize};
 use std::sync::Arc;
 use uuid::Uuid;
 
+const EVENT_TYPE_REFINEMENT_APPLIED: &str = "RefinementApplied";
+
 /// Canonical file name used to store validation feedback artifacts produced by refinement
-/// workflows. This is consumed by the `RefinementApplied` event handler, which expects any
-/// validation feedback generated during a refinement iteration to be written under this
-/// name within the associated artifact set or storage location.
+/// workflows.
+///
+/// This is consumed by the `RefinementApplied` event handler, which expects any validation
+/// feedback generated during a refinement iteration to be written under this name within the
+/// associated artifact set or storage location.
 const VALIDATION_FEEDBACK_FILE_NAME: &str = "validation_feedback";
 
 /// External event payload from Temporal worker
@@ -356,7 +360,7 @@ impl TemporalEventListener {
     /// - Missing required fields
     pub async fn handle_event(&self, payload: TemporalEventPayload) -> Result<String> {
         // Special case for RefinementApplied execution event
-        if payload.event_type == "RefinementApplied" {
+        if payload.event_type == EVENT_TYPE_REFINEMENT_APPLIED {
             let execution_id = ExecutionId(uuid::Uuid::parse_str(&payload.execution_id)?);
             let agent_id_str = payload
                 .agent_id