Route always-on SafeOutputs through Stage 3 executors

[Copilot]

This PR addresses a gap where builtin always-on SafeOutputs (noop, missing-tool, missing-data, report-incomplete) were handled ad hoc in Stage 3 instead of via the executor framework. The result was inconsistent execution semantics and divergent dry-run behavior across tool paths.

• Executor coverage for always-on tools

  • Added Executor implementations for:
    • NoopResult
    • MissingToolResult
    • MissingDataResult
    • ReportIncompleteResult
  • Moved these tools onto the same deserialize → sanitize → execute flow used by other SafeOutputs.

• Dispatch unification in Stage 3

  • Updated execute_safe_output to dispatch always-on tools through dispatch_tool / dispatch_executor_tools!.
  • Removed inline special-casing for informational outputs and report-incomplete.
  • Kept unknown-tool handling as a single fallback path.

• Sanitization and semantics

  • Added SanitizeContent implementations for the newly executable always-on result types.
  • Preserved intentional failure semantics for report-incomplete in dry-run mode by overriding execute_sanitized for that tool.

• Behavior alignment in tests

  • Updated Stage 3 tests to reflect executor-driven handling, including dry-run expectations for noop.

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,
    // ...
})? {
    dispatched_result
} else {
    bail!("Unknown tool type: {}. No executor registered.", tool_name)
};

[jamesadevine]

/rust-review

[github-actions]

✅ Rust PR Reviewer completed successfully!

[github-actions]

🔍 Rust PR Review

Summary: Looks good — clean unification of the executor dispatch path with one minor suggestion.

Findings

⚠️ Suggestions

• src/safeoutputs/report_incomplete.rs:68 — The execute_impl only formats self.reason into the failure message; the sanitized context field is silently dropped. Every other tool in this PR (missing-data, missing-tool) includes context in its output message. Consider appending it here too for consistency and better diagnostics:

  async fn execute_impl(&self, _: &ExecutionContext) -> anyhow::Result<ExecutionResult> {
      let mut msg = format!("Agent reported task incomplete: {}", self.reason);
      if let Some(ctx) = &self.context {
          msg.push_str(&format!(" [{ctx}]"));
      }
      Ok(ExecutionResult::failure(msg))
  }

• src/safeoutputs/report_incomplete.rs:60 — The execute_sanitized override fully replaces the default. If the trait default ever gains additional logic (e.g., max-count enforcement, telemetry), this override won't inherit it. The comment explains the intent well, so this is a maintenance consideration rather than a bug — but worth tracking.

✅ What Looks Good

• The removal of the match tool_name inline block in execute_safe_output and routing all tools through dispatch_executor_tools! is a straightforward, well-executed refactoring. All tools now follow the same deserialize → sanitize → execute flow.
• The dry-run bypass in ReportIncompleteResult::execute_sanitized is the right design: report-incomplete signals a task-level failure and must surface that failure even in dry-run. The comment explains the reasoning clearly.
• noop/missing-tool/missing-data correctly use the default execute_sanitized (inheriting dry-run behavior), while report-incomplete overrides it — exactly the right distinction.
• Removing the use crate::sanitize::SanitizeContent import from execute.rs is correct; sanitization responsibility now lives fully within each tool module.
• The updated test comments accurately describe the new executor-driven behavior.

Generated by Rust PR Reviewer for issue #310 · ● 764.9K · ◷

[jamesadevine]

@copilot implement suggestions from PR feedback

[Copilot]

@copilot implement suggestions from PR feedback

Implemented the actionable Rust review suggestion in commit ed92934: report-incomplete now includes optional sanitized context in its failure message, and I added a focused test covering that behavior.