Route always-on SafeOutputs through Stage 3 executors (#310)

Author: Copilot

src/execute.rs

@@ -11,14 +11,13 @@ use std::collections::HashMap;
 use std::path::Path;
 
 use crate::ndjson::{self, SAFE_OUTPUT_FILENAME};
-use crate::sanitize::SanitizeContent;
 use crate::safeoutputs::{
     AddBuildTagResult, AddPrCommentResult, CreateBranchResult, CreateGitTagResult,
     CreatePrResult, CreateWikiPageResult, CreateWorkItemResult, CommentOnWorkItemResult,
-    ExecutionContext, ExecutionResult, Executor, LinkWorkItemsResult, QueueBuildResult,
-    ReplyToPrCommentResult, ReportIncompleteResult, ResolvePrThreadResult, SubmitPrReviewResult,
-    ToolResult, UpdatePrResult, UpdateWikiPageResult, UpdateWorkItemResult,
-    UploadAttachmentResult,
+    ExecutionContext, ExecutionResult, Executor, LinkWorkItemsResult, MissingDataResult,
+    MissingToolResult, NoopResult, QueueBuildResult, ReplyToPrCommentResult,
+    ReportIncompleteResult, ResolvePrThreadResult, SubmitPrReviewResult, ToolResult,
+    UpdatePrResult, UpdateWikiPageResult, UpdateWorkItemResult, UploadAttachmentResult,
 };
 
 // Re-export memory types for use by main.rs
@@ -256,10 +255,13 @@ pub async fn execute_safe_output(
 
     debug!("Dispatching tool: {}", tool_name);
 
-    // Dispatch based on tool name. All standard tools go through `dispatch_tool` which
-    // handles deserialization and sanitized execution uniformly. Special cases (informational
-    // outputs and report-incomplete) are handled inline.
+    // Dispatch based on tool name. All registered tools go through `dispatch_tool`,
+    // which handles deserialization and sanitized execution uniformly.
     let result = if let Some(dispatched_result) = dispatch_executor_tools!(tool_name, entry, ctx, {
+        "noop" => NoopResult,
+        "missing-tool" => MissingToolResult,
+        "missing-data" => MissingDataResult,
+        "report-incomplete" => ReportIncompleteResult,
         "create-work-item" => CreateWorkItemResult,
         "comment-on-work-item" => CommentOnWorkItemResult,
         "update-work-item" => UpdateWorkItemResult,
@@ -280,25 +282,8 @@ pub async fn execute_safe_output(
     })? {
         dispatched_result
     } else {
-        match tool_name {
-        // Informational outputs — no side effects, always succeed
-        "noop" | "missing-tool" | "missing-data" => {
-            debug!("Skipping informational entry: {}", tool_name);
-            ExecutionResult::success(format!("Skipped informational output: {}", tool_name))
-        }
-        // report-incomplete does not implement Executor; Stage 3 surfaces its reason as a failure
-        "report-incomplete" => {
-            let mut output: ReportIncompleteResult = serde_json::from_value(entry.clone())
-                .map_err(|e| anyhow::anyhow!("Failed to parse report-incomplete: {}", e))?;
-            output.sanitize_content_fields();
-            debug!("report-incomplete: {}", output.reason);
-            ExecutionResult::failure(format!("Agent reported task incomplete: {}", output.reason))
-        }
-        other => {
-            error!("Unknown tool type: {}", other);
-            bail!("Unknown tool type: {}. No executor registered.", other)
-        }
-    }
+        error!("Unknown tool type: {}", tool_name);
+        bail!("Unknown tool type: {}. No executor registered.", tool_name)
     };
 
     Ok((tool_name.to_string(), result))
@@ -470,7 +455,7 @@ mod tests {
         let (tool_name, result) = result.unwrap();
         assert_eq!(tool_name, "noop");
         assert!(result.success);
-        assert!(result.message.contains("Skipped"));
+        assert!(result.message.contains("No operation"));
     }
 
     #[tokio::test]
@@ -1050,9 +1035,9 @@ mod tests {
         assert_eq!(results.len(), 2);
         // create-work-item goes through Executor trait → dry-run intercepted
         assert!(results[0].message.contains("[DRY-RUN]"));
-        // noop is handled inline, not through Executor → runs normally
+        // noop now also goes through Executor trait → dry-run intercepted
         assert!(results[1].success);
-        assert!(results[1].message.contains("noop"));
+        assert!(results[1].message.contains("[DRY-RUN]"));
     }
 
     #[tokio::test]
@@ -1124,8 +1109,8 @@ mod tests {
 
     #[tokio::test]
     async fn test_dry_run_report_incomplete_still_fails() {
-        // report-incomplete is dispatched inline (not through Executor trait),
-        // so it still returns ExecutionResult::failure even in dry-run mode.
+        // report-incomplete uses an Executor override that still returns
+        // ExecutionResult::failure even in dry-run mode.
         // This is correct: the agent declared it couldn't complete the task.
         let entry = serde_json::json!({
             "name": "report-incomplete",

src/safeoutputs/missing_data.rs

@@ -3,8 +3,9 @@
 use schemars::JsonSchema;
 use serde::Deserialize;
 
+use crate::sanitize::{SanitizeContent, sanitize as sanitize_text};
 use crate::tool_result;
-use crate::safeoutputs::Validate;
+use crate::safeoutputs::{ExecutionContext, ExecutionResult, Executor, Validate};
 
 /// Parameters for reporting missing data
 #[derive(Deserialize, JsonSchema)]
@@ -34,6 +35,32 @@ tool_result! {
     }
 }
 
+impl SanitizeContent for MissingDataResult {
+    fn sanitize_content_fields(&mut self) {
+        self.data_type = sanitize_text(&self.data_type);
+        self.reason = sanitize_text(&self.reason);
+        self.context = self.context.as_deref().map(sanitize_text);
+    }
+}
+
+#[async_trait::async_trait]
+impl Executor for MissingDataResult {
+    fn dry_run_summary(&self) -> String {
+        format!("report missing data '{}'", self.data_type)
+    }
+
+    async fn execute_impl(&self, _: &ExecutionContext) -> anyhow::Result<ExecutionResult> {
+        let mut message = format!(
+            "Missing data reported: {} ({})",
+            self.data_type, self.reason
+        );
+        if let Some(context) = &self.context {
+            message.push_str(&format!(" [{context}]"));
+        }
+        Ok(ExecutionResult::success(message))
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

src/safeoutputs/missing_tool.rs

@@ -3,8 +3,9 @@
 use schemars::JsonSchema;
 use serde::Deserialize;
 
+use crate::sanitize::{SanitizeContent, sanitize as sanitize_text};
 use crate::tool_result;
-use crate::safeoutputs::Validate;
+use crate::safeoutputs::{ExecutionContext, ExecutionResult, Executor, Validate};
 
 /// Parameters for reporting a missing tool
 #[derive(Deserialize, JsonSchema)]
@@ -29,6 +30,28 @@ tool_result! {
     }
 }
 
+impl SanitizeContent for MissingToolResult {
+    fn sanitize_content_fields(&mut self) {
+        self.tool_name = sanitize_text(&self.tool_name);
+        self.context = self.context.as_deref().map(sanitize_text);
+    }
+}
+
+#[async_trait::async_trait]
+impl Executor for MissingToolResult {
+    fn dry_run_summary(&self) -> String {
+        format!("report missing tool '{}'", self.tool_name)
+    }
+
+    async fn execute_impl(&self, _: &ExecutionContext) -> anyhow::Result<ExecutionResult> {
+        let message = match &self.context {
+            Some(context) => format!("Missing tool reported: {} ({context})", self.tool_name),
+            None => format!("Missing tool reported: {}", self.tool_name),
+        };
+        Ok(ExecutionResult::success(message))
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

src/safeoutputs/noop.rs

@@ -1,8 +1,9 @@
 use schemars::JsonSchema;
 use serde::Deserialize;
 
+use crate::sanitize::{SanitizeContent, sanitize as sanitize_text};
 use crate::tool_result;
-use crate::safeoutputs::Validate;
+use crate::safeoutputs::{ExecutionContext, ExecutionResult, Executor, Validate};
 
 /// Parameters for describing a no operation. Use this if there is no work to do.
 #[derive(Deserialize, JsonSchema)]
@@ -24,6 +25,27 @@ tool_result! {
     }
 }
 
+impl SanitizeContent for NoopResult {
+    fn sanitize_content_fields(&mut self) {
+        self.context = self.context.as_deref().map(sanitize_text);
+    }
+}
+
+#[async_trait::async_trait]
+impl Executor for NoopResult {
+    fn dry_run_summary(&self) -> String {
+        "noop".to_string()
+    }
+
+    async fn execute_impl(&self, _: &ExecutionContext) -> anyhow::Result<ExecutionResult> {
+        let message = match &self.context {
+            Some(context) => format!("No operation needed: {context}"),
+            None => "No operation needed".to_string(),
+        };
+        Ok(ExecutionResult::success(message))
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

src/safeoutputs/report_incomplete.rs

@@ -5,7 +5,7 @@ use serde::Deserialize;
 
 use crate::sanitize::{SanitizeContent, sanitize as sanitize_text};
 use crate::tool_result;
-use crate::safeoutputs::Validate;
+use crate::safeoutputs::{ExecutionContext, ExecutionResult, Executor, Validate};
 use anyhow::ensure;
 
 /// Parameters for reporting that a task could not be completed
@@ -49,10 +49,35 @@ impl SanitizeContent for ReportIncompleteResult {
     }
 }
 
+#[async_trait::async_trait]
+impl Executor for ReportIncompleteResult {
+    fn dry_run_summary(&self) -> String {
+        "report task incomplete".to_string()
+    }
+
+    // Intentionally bypass default dry-run behavior: this tool signals that the
+    // task itself failed, so Stage 3 should preserve failure semantics in dry-run.
+    async fn execute_sanitized(
+        &mut self,
+        ctx: &ExecutionContext,
+    ) -> anyhow::Result<ExecutionResult> {
+        self.sanitize_content_fields();
+        self.execute_impl(ctx).await
+    }
+
+    async fn execute_impl(&self, _: &ExecutionContext) -> anyhow::Result<ExecutionResult> {
+        let mut message = format!("Agent reported task incomplete: {}", self.reason);
+        if let Some(context) = &self.context {
+            message.push_str(&format!(" [{context}]"));
+        }
+        Ok(ExecutionResult::failure(message))
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::safeoutputs::ToolResult;
+    use crate::safeoutputs::{Executor, ToolResult};
 
     #[test]
     fn test_result_has_correct_name() {
@@ -102,4 +127,22 @@ mod tests {
         assert!(json.contains(r#""name":"report-incomplete""#));
         assert!(json.contains(r#""reason":"API timed out after 30s""#));
     }
+
+    #[tokio::test]
+    async fn test_execute_impl_includes_context_when_present() {
+        let mut result: ReportIncompleteResult = ReportIncompleteParams {
+            reason: "API timed out after 30s".to_string(),
+            context: Some("tried 3 retries".to_string()),
+        }
+        .try_into()
+        .unwrap();
+
+        let exec = result
+            .execute_sanitized(&crate::safeoutputs::ExecutionContext::default())
+            .await
+            .unwrap();
+        assert!(!exec.success);
+        assert!(exec.message.contains("API timed out after 30s"));
+        assert!(exec.message.contains("[tried 3 retries]"));
+    }
 }