Merge pull request #62 from 100monkeys-ai/copilot/refactor-error-extraction-logic

Refactor error extraction, fix misclassified error variants, ceiling division for judge timeout, and remove dead code

Author: Theaxiom

cli/src/daemon/client.rs

@@ -394,6 +394,33 @@ fn is_error_event(event: &serde_json::Value) -> bool {
     )
 }
 
+fn extract_iteration_error_message(event: &serde_json::Value) -> &str {
+    // Precedence:
+    // 1. event.data.error.message (when error is an object)
+    // 2. event.data.error as a string
+    // 3. event.error as a string
+    // 4. Fallback: "Unknown error"
+    if let Some(err_obj) = event.get("data").and_then(|d| d.get("error")).and_then(|e| e.as_object()) {
+        if let Some(msg) = err_obj.get("message").and_then(|m| m.as_str()) {
+            return msg;
+        }
+    }
+
+    if let Some(msg) = event
+        .get("data")
+        .and_then(|d| d.get("error"))
+        .and_then(|e| e.as_str())
+    {
+        return msg;
+    }
+
+    if let Some(msg) = event.get("error").and_then(|e| e.as_str()) {
+        return msg;
+    }
+
+    "Unknown error"
+}
+
 fn print_event(event: &serde_json::Value, verbose: bool) {
     use colored::Colorize;
 
@@ -454,15 +481,7 @@ fn print_event(event: &serde_json::Value, verbose: bool) {
         }
         "IterationFailed" => {
             let iteration = event["iteration_number"].as_u64().unwrap_or(0);
-            let error = if let Some(err_obj) = event["data"]["error"].as_object() {
-                err_obj["message"]
-                    .as_str()
-                    .unwrap_or(event["data"]["error"].as_str().unwrap_or("Unknown error"))
-            } else {
-                event["data"]["error"]
-                    .as_str()
-                    .unwrap_or(event["error"].as_str().unwrap_or(""))
-            };
+            let error = extract_iteration_error_message(event);
             println!(
                 "{} {} {} {} - {}",
                 format!("[{}]", timestamp).dimmed(),

orchestrator/core/src/application/tool_invocation_service.rs

@@ -235,10 +235,7 @@ impl ToolInvocationService {
 
         // Enforce SecurityContext constraints (e.g. subcommand_allowlist for cmd.run)
         if let Err(violation) = security_context.evaluate(&tool_name, &args) {
-            return Err(SmcpSessionError::SignatureVerificationFailed(format!(
-                "Policy violation: {:?}",
-                violation
-            )));
+            return Err(SmcpSessionError::PolicyViolation(violation));
         }
 
         // --- Inner-Loop Semantic Pre-Execution Validation (ADR-049) ---
@@ -330,7 +327,9 @@ impl ToolInvocationService {
                             })?;
 
                         let poll_interval_ms = 500u64;
-                        let max_attempts = (*timeout_seconds * 1000) / poll_interval_ms;
+                        let timeout_ms = timeout_seconds.saturating_mul(1000);
+                        let max_attempts =
+                            timeout_ms.saturating_add(poll_interval_ms.saturating_sub(1)) / poll_interval_ms;
                         let mut attempts = 0;
 
                         loop {
@@ -346,7 +345,10 @@ impl ToolInvocationService {
                                 .get_execution(exec_id)
                                 .await
                                 .map_err(|e| {
-                                    SmcpSessionError::SignatureVerificationFailed(e.to_string())
+                                    SmcpSessionError::InternalError(format!(
+                                        "Failed to get judge execution {}: {}",
+                                        exec_id, e
+                                    ))
                                 })?;
 
                             match exec.status {

orchestrator/core/src/domain/smcp_session.rs

@@ -105,6 +105,8 @@ pub enum SmcpSessionError {
     SignatureVerificationFailed(String),
     /// A semantic judge agent did not respond within the allotted timeout window.
     JudgeTimeout(String),
+    /// An unexpected internal error occurred (e.g. infrastructure or service failure).
+    InternalError(String),
 }
 
 impl std::fmt::Display for SmcpSessionError {
@@ -118,6 +120,7 @@ impl std::fmt::Display for SmcpSessionError {
                 write!(f, "Signature verification failed: {}", e)
             }
             Self::JudgeTimeout(msg) => write!(f, "Judge timed out: {}", msg),
+            Self::InternalError(msg) => write!(f, "Internal error: {}", msg),
         }
     }
 }
@@ -215,7 +218,7 @@ impl SmcpSession {
             security_context,
             status: SessionStatus::Active,
             created_at: now,
-            expires_at: now + chrono::TimeDelta::hours(SESSION_TTL_HOURS),
+            expires_at: now + chrono::Duration::hours(SESSION_TTL_HOURS),
         }
     }
 

orchestrator/core/src/infrastructure/temporal_event_listener.rs

@@ -385,6 +385,10 @@ impl TemporalEventListener {
             };
 
             let code_diff = crate::domain::execution::CodeDiff {
+                // NOTE: RefinementApplied code diffs are persisted under the canonical
+                // validation feedback artifact path. The file_path here is intentionally
+                // set to VALIDATION_FEEDBACK_FILE_NAME to indicate where in the workspace
+                // this diff content should be stored/loaded.
                 file_path: VALIDATION_FEEDBACK_FILE_NAME.to_string(),
                 diff: diff_str,
             };
@@ -418,13 +422,6 @@ impl TemporalEventListener {
         let domain_event = TemporalEventMapper::to_domain_event(&payload)
             .context("Failed to map Temporal event to domain event")?;
 
-        // Definition-time event — no execution_id exists.
-        // Publish to the event bus so subscribers are notified, then return early.
-        if let WorkflowEvent::WorkflowRegistered { .. } = &domain_event {
-            self.event_bus.publish_workflow_event(domain_event.clone());
-            return Ok(String::new());
-        }
-
         // All remaining workflow events are execution-scoped and carry an execution_id.
         let execution_id_obj = match &domain_event {
             WorkflowEvent::WorkflowExecutionStarted { execution_id, .. }
@@ -439,13 +436,11 @@ impl TemporalEventListener {
             WorkflowEvent::WorkflowRegistered { .. } => {
                 return Err(anyhow!(
                     "WorkflowRegistered event unexpectedly reached execution-scoped handling; \
-                     this variant should be handled before deriving an execution_id"
+                     this variant should be handled by TemporalEventMapper::to_domain_event"
                 ));
             }
         };
 
-        let execution_id_str = execution_id_obj.0.to_string();
-
         // Step 2: Persist event to the repository for event sourcing
         self.execution_repository
             .append_event(
@@ -462,7 +457,7 @@ impl TemporalEventListener {
         self.event_bus.publish_workflow_event(domain_event.clone());
 
         // Step 4: Return execution ID for response
-        Ok(execution_id_str)
+        Ok(execution_id_obj.0.to_string())
     }
 }