fix(agent): inject skill tools into agent registry and unify ToolResult (#341) (#360)

* fix(agent): inject skill tools into agent registry and unify ToolResult (#341)

Skill tools (Notion, Gmail, etc.) from the QuickJS runtime were invisible
to the agent's tool loop — the LLM could never call them. This change:

1. Adds SkillToolBridge to wrap skill ToolDefinitions as Tool trait impls
   and inject them into the agent's tool registry via collect_skill_tools().

2. Unifies ToolResult across built-in and skill tools — both now use the
   MCP-spec type (content blocks + is_error) from skills::types, eliminating
   the need for result conversion between the two systems.

3. Adds convenience constructors (ToolResult::success/error/json) and
   accessor methods (output/text) to simplify all tool implementations.

4. Adds diagnostic logging in the tool loop for tool registry contents
   and tool lookup results.

Verified: agent_chat RPC successfully called Notion list-all-pages tool
and returned real workspace data through the agent loop.

Closes #341

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(tools): address CodeRabbit review feedback on PR #360

- cron_run: preserve execution details (result_output) on failure
- delegate: fix mismatched bracket in agent response header format string
- run_linter: include lint diagnostics in error output, not just exit code
- run_tests: include test output in error result, not just exit code
- skill_bridge: prevent namespaced tool-name collisions with dedup + __
  delimiter validation; sanitize runtime errors in model-facing output

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* style: apply cargo fmt to skill_bridge.rs

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

src/openhuman/agent/agent.rs

@@ -298,7 +298,7 @@ impl Agent {
             None
         };
 
-        let tools = tools::all_tools_with_runtime(
+        let mut tools = tools::all_tools_with_runtime(
             Arc::new(config.clone()),
             &security,
             runtime,
@@ -313,6 +313,17 @@ impl Agent {
             config,
         );
 
+        // Bridge skill tools (Notion, Gmail, etc.) from the QuickJS runtime
+        // into the agent's tool registry so the LLM can call them.
+        let skill_tools = tools::skill_bridge::collect_skill_tools();
+        if !skill_tools.is_empty() {
+            log::info!(
+                "[agent] Injecting {} skill tool(s) into agent registry",
+                skill_tools.len()
+            );
+            tools.extend(skill_tools);
+        }
+
         let model_name = config
             .default_model
             .as_deref()
@@ -562,10 +573,10 @@ impl Agent {
             if let Some(tool) = self.tools.iter().find(|t| t.name() == call.name) {
                 match tool.execute(call.arguments.clone()).await {
                     Ok(r) => {
-                        if r.success {
-                            (r.output, true)
+                        if !r.is_error {
+                            (r.output(), true)
                         } else {
-                            (format!("Error: {}", r.error.unwrap_or(r.output)), false)
+                            (format!("Error: {}", r.output()), false)
                         }
                     }
                     Err(e) => (format!("Error executing {}: {e}", call.name), false),
@@ -981,11 +992,7 @@ mod tests {
             &self,
             _args: serde_json::Value,
         ) -> Result<crate::openhuman::tools::ToolResult> {
-            Ok(crate::openhuman::tools::ToolResult {
-                success: true,
-                output: "tool-out".into(),
-                error: None,
-            })
+            Ok(crate::openhuman::tools::ToolResult::success("tool-out"))
         }
     }
 

src/openhuman/agent/harness/self_healing.rs

@@ -46,13 +46,12 @@ impl SelfHealingInterceptor {
     /// Returns `Some(command_name)` if the error matches a known missing-command pattern
     /// and we haven't exceeded the retry limit.
     pub fn detect_missing_command(&mut self, result: &ToolResult) -> Option<String> {
-        if !self.enabled || result.success {
+        if !self.enabled || !result.is_error {
             return None;
         }
 
-        let error_text = result.error.as_deref().unwrap_or("").to_lowercase();
-        let output_text = result.output.to_lowercase();
-        let combined = format!("{error_text} {output_text}");
+        let output_text = result.output().to_lowercase();
+        let combined = output_text;
 
         // Check if the error matches any missing-command pattern.
         let is_missing = MISSING_CMD_PATTERNS
@@ -185,11 +184,7 @@ mod tests {
     use super::*;
 
     fn make_error_result(error: &str) -> ToolResult {
-        ToolResult {
-            success: false,
-            output: String::new(),
-            error: Some(error.to_string()),
-        }
+        ToolResult::error(error)
     }
 
     #[test]
@@ -223,11 +218,7 @@ mod tests {
     #[test]
     fn ignores_successful_results() {
         let mut interceptor = SelfHealingInterceptor::new(Path::new("/tmp"), true);
-        let result = ToolResult {
-            success: true,
-            output: "command not found".into(), // misleading output
-            error: None,
-        };
+        let result = ToolResult::success("command not found"); // misleading output
         assert!(interceptor.detect_missing_command(&result).is_none());
     }
 

src/openhuman/agent/loop_/session.rs

@@ -79,6 +79,16 @@ pub async fn run(
         tools_registry.extend(peripheral_tools);
     }
 
+    // Bridge skill tools from the QuickJS runtime into the tool registry
+    let skill_tools = tools::skill_bridge::collect_skill_tools();
+    if !skill_tools.is_empty() {
+        tracing::info!(
+            count = skill_tools.len(),
+            "[skill-bridge] Skill tools added to session"
+        );
+        tools_registry.extend(skill_tools);
+    }
+
     // ── Inference (OpenHuman backend only) ───────────────────────
     let provider_name = providers::INFERENCE_BACKEND_ID;
 
@@ -484,6 +494,16 @@ pub async fn process_message(config: Config, message: &str) -> Result<String> {
         tools::create_peripheral_tools(&config.peripherals).await?;
     tools_registry.extend(peripheral_tools);
 
+    // Bridge skill tools from the QuickJS runtime
+    let skill_tools = tools::skill_bridge::collect_skill_tools();
+    if !skill_tools.is_empty() {
+        tracing::info!(
+            count = skill_tools.len(),
+            "[skill-bridge] Skill tools added to channel"
+        );
+        tools_registry.extend(skill_tools);
+    }
+
     let provider_name = providers::INFERENCE_BACKEND_ID;
     let model_name = config
         .default_model

src/openhuman/agent/loop_/tool_loop.rs

@@ -78,6 +78,16 @@ pub(crate) async fn run_tool_call_loop(
         tools_registry.iter().map(|tool| tool.spec()).collect();
     let use_native_tools = provider.supports_native_tools() && !tool_specs.is_empty();
 
+    log::debug!(
+        "[tool-loop] Registry has {} tool(s): [{}]",
+        tools_registry.len(),
+        tools_registry
+            .iter()
+            .map(|t| t.name())
+            .collect::<Vec<_>>()
+            .join(", ")
+    );
+
     let mut context_guard = ContextGuard::new();
 
     for iteration in 0..max_iterations {
@@ -278,9 +288,11 @@ pub(crate) async fn run_tool_call_loop(
                 }
             }
 
+            let tool_found = find_tool(tools_registry, &call.name).is_some();
             tracing::debug!(
                 iteration,
                 tool = call.name.as_str(),
+                found = tool_found,
                 "[agent_loop] executing tool"
             );
 
@@ -292,22 +304,22 @@ pub(crate) async fn run_tool_call_loop(
                     .await
                 {
                     Ok(Ok(r)) => {
-                        if r.success {
+                        let output = r.output();
+                        if !r.is_error {
                             tracing::debug!(
                                 iteration,
                                 tool = call.name.as_str(),
-                                output_len = r.output.len(),
+                                output_len = output.len(),
                                 "[agent_loop] tool succeeded"
                             );
-                            scrub_credentials(&r.output)
+                            scrub_credentials(&output)
                         } else {
-                            let err_msg = r.error.unwrap_or(r.output);
                             tracing::warn!(
                                 iteration,
                                 tool = call.name.as_str(),
-                                "[agent_loop] tool returned error: {err_msg}"
+                                "[agent_loop] tool returned error: {output}"
                             );
-                            format!("Error: {err_msg}")
+                            format!("Error: {output}")
                         }
                     }
                     Ok(Err(e)) => {

src/openhuman/agent/prompt.rs

@@ -334,11 +334,7 @@ mod tests {
             &self,
             _args: serde_json::Value,
         ) -> anyhow::Result<crate::openhuman::tools::ToolResult> {
-            Ok(crate::openhuman::tools::ToolResult {
-                success: true,
-                output: "ok".into(),
-                error: None,
-            })
+            Ok(crate::openhuman::tools::ToolResult::success("ok"))
         }
     }
 

src/openhuman/agent/tests.rs

@@ -152,11 +152,7 @@ impl Tool for EchoTool {
             .and_then(|v| v.as_str())
             .unwrap_or("(empty)")
             .to_string();
-        Ok(ToolResult {
-            success: true,
-            output: msg,
-            error: None,
-        })
+        Ok(ToolResult::success(msg))
     }
 }
 
@@ -178,11 +174,7 @@ impl Tool for FailingTool {
     }
 
     async fn execute(&self, _args: serde_json::Value) -> Result<ToolResult> {
-        Ok(ToolResult {
-            success: false,
-            output: String::new(),
-            error: Some("intentional failure".into()),
-        })
+        Ok(ToolResult::error("intentional failure"))
     }
 }
 
@@ -242,11 +234,7 @@ impl Tool for CountingTool {
     async fn execute(&self, _args: serde_json::Value) -> Result<ToolResult> {
         let mut c = self.count.lock().unwrap();
         *c += 1;
-        Ok(ToolResult {
-            success: true,
-            output: format!("call #{}", *c),
-            error: None,
-        })
+        Ok(ToolResult::success(format!("call #{}", *c)))
     }
 }
 

src/openhuman/agent/traits.rs

@@ -138,13 +138,8 @@ impl Provider for NoopProvider {
 // Tool trait — executable capabilities
 // ═══════════════════════════════════════════════════════════════════
 
-/// Result of a tool execution.
-#[derive(Debug, Clone, Serialize, Deserialize)]
-pub struct ToolResult {
-    pub success: bool,
-    pub output: String,
-    pub error: Option<String>,
-}
+/// Re-export the unified ToolResult from the tools module.
+pub use crate::openhuman::tools::ToolResult;
 
 /// Description of a tool for the LLM.
 #[derive(Debug, Clone, Serialize, Deserialize)]
@@ -186,11 +181,7 @@ impl Tool for NoopTool {
         serde_json::json!({"type": "object", "properties": {}})
     }
     async fn execute(&self, _args: serde_json::Value) -> anyhow::Result<ToolResult> {
-        Ok(ToolResult {
-            success: true,
-            output: String::new(),
-            error: None,
-        })
+        Ok(ToolResult::success(""))
     }
 }
 
@@ -420,8 +411,8 @@ mod tests {
     async fn noop_tool_execute() {
         let tool = NoopTool;
         let result = tool.execute(serde_json::json!({})).await.unwrap();
-        assert!(result.success);
-        assert!(result.output.is_empty());
+        assert!(!result.is_error);
+        assert!(result.output().is_empty() || result.output() == "");
     }
 
     #[tokio::test]
@@ -473,14 +464,10 @@ mod tests {
 
     #[test]
     fn tool_result_serialization_roundtrip() {
-        let result = ToolResult {
-            success: false,
-            output: String::new(),
-            error: Some("boom".into()),
-        };
+        let result = ToolResult::error("boom");
         let json = serde_json::to_string(&result).unwrap();
         let parsed: ToolResult = serde_json::from_str(&json).unwrap();
-        assert!(!parsed.success);
-        assert_eq!(parsed.error.as_deref(), Some("boom"));
+        assert!(parsed.is_error);
+        assert_eq!(parsed.output(), "boom");
     }
 }

src/openhuman/channels/tests/common.rs

@@ -348,18 +348,10 @@ impl Tool for MockPriceTool {
     async fn execute(&self, args: serde_json::Value) -> anyhow::Result<ToolResult> {
         let symbol = args.get("symbol").and_then(serde_json::Value::as_str);
         if symbol != Some("BTC") {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("unexpected symbol".to_string()),
-            });
+            return Ok(ToolResult::error("unexpected symbol"));
         }
 
-        Ok(ToolResult {
-            success: true,
-            output: "BTC is $65,000".to_string(),
-            error: None,
-        })
+        Ok(ToolResult::success("BTC is $65,000"))
     }
 }
 

src/openhuman/skills/types.rs

@@ -179,6 +179,9 @@ pub enum ToolCallOrigin {
 }
 
 /// Result of executing a tool, containing content blocks and error status.
+///
+/// This is the **unified** tool result type used by both built-in tools and
+/// QuickJS skill tools. Follows the MCP (Model Context Protocol) specification.
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct ToolResult {
     /// List of content blocks returned by the tool (follows MCP specification).
@@ -188,6 +191,60 @@ pub struct ToolResult {
     pub is_error: bool,
 }
 
+impl ToolResult {
+    /// Create a successful result with a single text block.
+    pub fn success(text: impl Into<String>) -> Self {
+        Self {
+            content: vec![ToolContent::Text { text: text.into() }],
+            is_error: false,
+        }
+    }
+
+    /// Create an error result with a single text block.
+    pub fn error(message: impl Into<String>) -> Self {
+        Self {
+            content: vec![ToolContent::Text {
+                text: message.into(),
+            }],
+            is_error: true,
+        }
+    }
+
+    /// Create a successful result with structured JSON data.
+    pub fn json(data: serde_json::Value) -> Self {
+        Self {
+            content: vec![ToolContent::Json { data }],
+            is_error: false,
+        }
+    }
+
+    /// Extract all text content as a single joined string.
+    pub fn text(&self) -> String {
+        self.content
+            .iter()
+            .filter_map(|c| match c {
+                ToolContent::Text { text } => Some(text.as_str()),
+                ToolContent::Json { data } => None,
+            })
+            .collect::<Vec<_>>()
+            .join("\n")
+    }
+
+    /// Extract all content (text + JSON) as a single string.
+    pub fn output(&self) -> String {
+        self.content
+            .iter()
+            .map(|c| match c {
+                ToolContent::Text { text } => text.clone(),
+                ToolContent::Json { data } => {
+                    serde_json::to_string_pretty(data).unwrap_or_default()
+                }
+            })
+            .collect::<Vec<_>>()
+            .join("\n")
+    }
+}
+
 /// A single content block within a `ToolResult`.
 #[derive(Debug, Clone, Serialize, Deserialize)]
 #[serde(tag = "type", rename_all = "lowercase")]

src/openhuman/tools/ask_clarification.rs

@@ -75,10 +75,6 @@ impl Tool for AskClarificationTool {
         // For now, return the question as output so the orchestrator can surface it.
         tracing::info!("[ask_clarification] question: {question}");
 
-        Ok(ToolResult {
-            success: true,
-            output,
-            error: None,
-        })
+        Ok(ToolResult::success(output))
     }
 }

src/openhuman/tools/browser.rs

@@ -644,11 +644,9 @@ impl BrowserTool {
                 )
                 .await?;
 
-            Ok(ToolResult {
-                success: true,
-                output: serde_json::to_string_pretty(&output).unwrap_or_default(),
-                error: None,
-            })
+            Ok(ToolResult::success(
+                serde_json::to_string_pretty(&output).unwrap_or_default(),
+            ))
         }
 
         #[cfg(not(feature = "browser-native"))]
@@ -791,11 +789,7 @@ impl BrowserTool {
                         .unwrap_or_default()
                     });
 
-                return Ok(ToolResult {
-                    success: true,
-                    output,
-                    error: None,
-                });
+                return Ok(ToolResult::success(output));
             }
 
             let error = parsed.error.or_else(|| {
@@ -808,29 +802,17 @@ impl BrowserTool {
                 }
             });
 
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error,
-            });
+            return Ok(ToolResult::error(error.unwrap_or_default()));
         }
 
         if status.is_success() {
-            return Ok(ToolResult {
-                success: true,
-                output: body,
-                error: None,
-            });
+            return Ok(ToolResult::success(body));
         }
 
-        Ok(ToolResult {
-            success: false,
-            output: String::new(),
-            error: Some(format!(
-                "computer-use sidecar request failed with status {status}: {}",
-                body.trim()
-            )),
-        })
+        Ok(ToolResult::error(format!(
+            "computer-use sidecar request failed with status {status}: {}",
+            body.trim()
+        )))
     }
 
     async fn execute_action(
@@ -854,17 +836,9 @@ impl BrowserTool {
                 .data
                 .map(|d| serde_json::to_string_pretty(&d).unwrap_or_default())
                 .unwrap_or_default();
-            Ok(ToolResult {
-                success: true,
-                output,
-                error: None,
-            })
+            Ok(ToolResult::success(output))
         } else {
-            Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: resp.error,
-            })
+            Ok(ToolResult::error(resp.error.unwrap_or_default()))
         }
     }
 }
@@ -1001,29 +975,17 @@ impl Tool for BrowserTool {
     async fn execute(&self, args: Value) -> anyhow::Result<ToolResult> {
         // Security checks
         if !self.security.can_act() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Action blocked: autonomy is read-only".into()),
-            });
+            return Ok(ToolResult::error("Action blocked: autonomy is read-only"));
         }
 
         if !self.security.record_action() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Action blocked: rate limit exceeded".into()),
-            });
+            return Ok(ToolResult::error("Action blocked: rate limit exceeded"));
         }
 
         let backend = match self.resolve_backend().await {
             Ok(selected) => selected,
             Err(error) => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(error.to_string()),
-                });
+                return Ok(ToolResult::error(error.to_string()));
             }
         };
 
@@ -1034,11 +996,7 @@ impl Tool for BrowserTool {
             .ok_or_else(|| anyhow::anyhow!("Missing 'action' parameter"))?;
 
         if !is_supported_browser_action(action_str) {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!("Unknown action: {action_str}")),
-            });
+            return Ok(ToolResult::error(format!("Unknown action: {action_str}")));
         }
 
         if backend == ResolvedBackend::ComputerUse {
@@ -1046,21 +1004,15 @@ impl Tool for BrowserTool {
         }
 
         if is_computer_use_only_action(action_str) {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(unavailable_action_for_backend_error(action_str, backend)),
-            });
+            return Ok(ToolResult::error(unavailable_action_for_backend_error(
+                action_str, backend,
+            )));
         }
 
         let action = match parse_browser_action(action_str, &args) {
             Ok(a) => a,
             Err(e) => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(e.to_string()),
-                });
+                return Ok(ToolResult::error(e.to_string()));
             }
         };
 

src/openhuman/tools/browser_open.rs

@@ -83,43 +83,23 @@ impl Tool for BrowserOpenTool {
             .ok_or_else(|| anyhow::anyhow!("Missing 'url' parameter"))?;
 
         if !self.security.can_act() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Action blocked: autonomy is read-only".into()),
-            });
+            return Ok(ToolResult::error("Action blocked: autonomy is read-only"));
         }
 
         if !self.security.record_action() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Action blocked: rate limit exceeded".into()),
-            });
+            return Ok(ToolResult::error("Action blocked: rate limit exceeded"));
         }
 
         let url = match self.validate_url(url) {
             Ok(v) => v,
-            Err(e) => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(e.to_string()),
-                })
-            }
+            Err(e) => return Ok(ToolResult::error(e.to_string())),
         };
 
         match open_in_brave(&url).await {
-            Ok(()) => Ok(ToolResult {
-                success: true,
-                output: format!("Opened in Brave: {url}"),
-                error: None,
-            }),
-            Err(e) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!("Failed to open Brave Browser: {e}")),
-            }),
+            Ok(()) => Ok(ToolResult::success(format!("Opened in Brave: {url}"))),
+            Err(e) => Ok(ToolResult::error(format!(
+                "Failed to open Brave Browser: {e}"
+            ))),
         }
     }
 }
@@ -436,8 +416,8 @@ mod tests {
             .execute(json!({"url": "https://example.com"}))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result.error.unwrap().contains("read-only"));
+        assert!(result.is_error);
+        assert!(result.output().contains("read-only"));
     }
 
     #[tokio::test]
@@ -451,7 +431,7 @@ mod tests {
             .execute(json!({"url": "https://example.com"}))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result.error.unwrap().contains("rate limit"));
+        assert!(result.is_error);
+        assert!(result.output().contains("rate limit"));
     }
 }

src/openhuman/tools/composio.rs

@@ -484,17 +484,9 @@ impl Tool for ComposioTool {
                                 String::new()
                             }
                         );
-                        Ok(ToolResult {
-                            success: true,
-                            output,
-                            error: None,
-                        })
+                        Ok(ToolResult::success(output))
                     }
-                    Err(e) => Ok(ToolResult {
-                        success: false,
-                        output: String::new(),
-                        error: Some(format!("Failed to list actions: {e}")),
-                    }),
+                    Err(e) => Ok(ToolResult::error(format!("Failed to list actions: {e}"))),
                 }
             }
 
@@ -503,11 +495,7 @@ impl Tool for ComposioTool {
                     .security
                     .enforce_tool_operation(ToolOperation::Act, "composio.execute")
                 {
-                    return Ok(ToolResult {
-                        success: false,
-                        output: String::new(),
-                        error: Some(error),
-                    });
+                    return Ok(ToolResult::error(error));
                 }
 
                 let action_name = args
@@ -528,17 +516,9 @@ impl Tool for ComposioTool {
                     Ok(result) => {
                         let output = serde_json::to_string_pretty(&result)
                             .unwrap_or_else(|_| format!("{result:?}"));
-                        Ok(ToolResult {
-                            success: true,
-                            output,
-                            error: None,
-                        })
+                        Ok(ToolResult::success(output))
                     }
-                    Err(e) => Ok(ToolResult {
-                        success: false,
-                        output: String::new(),
-                        error: Some(format!("Action execution failed: {e}")),
-                    }),
+                    Err(e) => Ok(ToolResult::error(format!("Action execution failed: {e}"))),
                 }
             }
 
@@ -547,11 +527,7 @@ impl Tool for ComposioTool {
                     .security
                     .enforce_tool_operation(ToolOperation::Act, "composio.connect")
                 {
-                    return Ok(ToolResult {
-                        success: false,
-                        output: String::new(),
-                        error: Some(error),
-                    });
+                    return Ok(ToolResult::error(error));
                 }
 
                 let app = args.get("app").and_then(|v| v.as_str());
@@ -568,27 +544,19 @@ impl Tool for ComposioTool {
                     Ok(url) => {
                         let target =
                             app.unwrap_or(auth_config_id.unwrap_or("provided auth config"));
-                        Ok(ToolResult {
-                            success: true,
-                            output: format!("Open this URL to connect {target}:\n{url}"),
-                            error: None,
-                        })
+                        Ok(ToolResult::success(format!(
+                            "Open this URL to connect {target}:\n{url}"
+                        )))
                     }
-                    Err(e) => Ok(ToolResult {
-                        success: false,
-                        output: String::new(),
-                        error: Some(format!("Failed to get connection URL: {e}")),
-                    }),
+                    Err(e) => Ok(ToolResult::error(format!(
+                        "Failed to get connection URL: {e}"
+                    ))),
                 }
             }
 
-            _ => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!(
-                    "Unknown action '{action}'. Use 'list', 'execute', or 'connect'."
-                )),
-            }),
+            _ => Ok(ToolResult::error(format!(
+                "Unknown action '{action}'. Use 'list', 'execute', or 'connect'."
+            ))),
         }
     }
 }
@@ -830,8 +798,8 @@ mod tests {
     async fn execute_unknown_action_returns_error() {
         let tool = ComposioTool::new("test-key", None, test_security());
         let result = tool.execute(json!({"action": "unknown"})).await.unwrap();
-        assert!(!result.success);
-        assert!(result.error.as_ref().unwrap().contains("Unknown action"));
+        assert!(result.is_error);
+        assert!(&result.output().contains("Unknown action"));
     }
 
     #[tokio::test]
@@ -862,12 +830,8 @@ mod tests {
             }))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result
-            .error
-            .as_deref()
-            .unwrap_or("")
-            .contains("read-only mode"));
+        assert!(result.is_error);
+        assert!(result.output().contains("read-only mode"));
     }
 
     #[tokio::test]
@@ -884,12 +848,8 @@ mod tests {
             }))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result
-            .error
-            .as_deref()
-            .unwrap_or("")
-            .contains("Rate limit exceeded"));
+        assert!(result.is_error);
+        assert!(result.output().contains("Rate limit exceeded"));
     }
 
     // ── API response parsing ──────────────────────────────────

src/openhuman/tools/cron_add.rs

@@ -50,30 +50,22 @@ impl Tool for CronAddTool {
 
     async fn execute(&self, args: serde_json::Value) -> anyhow::Result<ToolResult> {
         if !self.config.cron.enabled {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("cron is disabled by config (cron.enabled=false)".to_string()),
-            });
+            return Ok(ToolResult::error(
+                "cron is disabled by config (cron.enabled=false)".to_string(),
+            ));
         }
 
         let schedule = match args.get("schedule") {
             Some(v) => match serde_json::from_value::<Schedule>(v.clone()) {
                 Ok(schedule) => schedule,
                 Err(e) => {
-                    return Ok(ToolResult {
-                        success: false,
-                        output: String::new(),
-                        error: Some(format!("Invalid schedule: {e}")),
-                    });
+                    return Ok(ToolResult::error(format!("Invalid schedule: {e}")));
                 }
             },
             None => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some("Missing 'schedule' parameter".to_string()),
-                });
+                return Ok(ToolResult::error(
+                    "Missing 'schedule' parameter".to_string(),
+                ));
             }
         };
 
@@ -86,11 +78,7 @@ impl Tool for CronAddTool {
             Some("agent") => JobType::Agent,
             Some("shell") => JobType::Shell,
             Some(other) => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(format!("Invalid job_type: {other}")),
-                });
+                return Ok(ToolResult::error(format!("Invalid job_type: {other}")));
             }
             None => {
                 if args.get("prompt").is_some() {
@@ -112,20 +100,16 @@ impl Tool for CronAddTool {
                 let command = match args.get("command").and_then(serde_json::Value::as_str) {
                     Some(command) if !command.trim().is_empty() => command,
                     _ => {
-                        return Ok(ToolResult {
-                            success: false,
-                            output: String::new(),
-                            error: Some("Missing 'command' for shell job".to_string()),
-                        });
+                        return Ok(ToolResult::error(
+                            "Missing 'command' for shell job".to_string(),
+                        ));
                     }
                 };
 
                 if !self.security.is_command_allowed(command) {
-                    return Ok(ToolResult {
-                        success: false,
-                        output: String::new(),
-                        error: Some(format!("Command blocked by security policy: {command}")),
-                    });
+                    return Ok(ToolResult::error(format!(
+                        "Command blocked by security policy: {command}"
+                    )));
                 }
 
                 cron::add_shell_job(&self.config, name, schedule, command)
@@ -134,11 +118,9 @@ impl Tool for CronAddTool {
                 let prompt = match args.get("prompt").and_then(serde_json::Value::as_str) {
                     Some(prompt) if !prompt.trim().is_empty() => prompt,
                     _ => {
-                        return Ok(ToolResult {
-                            success: false,
-                            output: String::new(),
-                            error: Some("Missing 'prompt' for agent job".to_string()),
-                        });
+                        return Ok(ToolResult::error(
+                            "Missing 'prompt' for agent job".to_string(),
+                        ));
                     }
                 };
 
@@ -146,11 +128,7 @@ impl Tool for CronAddTool {
                     Some(v) => match serde_json::from_value::<SessionTarget>(v.clone()) {
                         Ok(target) => target,
                         Err(e) => {
-                            return Ok(ToolResult {
-                                success: false,
-                                output: String::new(),
-                                error: Some(format!("Invalid session_target: {e}")),
-                            });
+                            return Ok(ToolResult::error(format!("Invalid session_target: {e}")));
                         }
                     },
                     None => SessionTarget::Isolated,
@@ -165,11 +143,7 @@ impl Tool for CronAddTool {
                     Some(v) => match serde_json::from_value::<DeliveryConfig>(v.clone()) {
                         Ok(cfg) => Some(cfg),
                         Err(e) => {
-                            return Ok(ToolResult {
-                                success: false,
-                                output: String::new(),
-                                error: Some(format!("Invalid delivery config: {e}")),
-                            });
+                            return Ok(ToolResult::error(format!("Invalid delivery config: {e}")));
                         }
                     },
                     None => None,
@@ -189,23 +163,15 @@ impl Tool for CronAddTool {
         };
 
         match result {
-            Ok(job) => Ok(ToolResult {
-                success: true,
-                output: serde_json::to_string_pretty(&json!({
-                    "id": job.id,
-                    "name": job.name,
-                    "job_type": job.job_type,
-                    "schedule": job.schedule,
-                    "next_run": job.next_run,
-                    "enabled": job.enabled
-                }))?,
-                error: None,
-            }),
-            Err(e) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(e.to_string()),
-            }),
+            Ok(job) => Ok(ToolResult::success(serde_json::to_string_pretty(&json!({
+                "id": job.id,
+                "name": job.name,
+                "job_type": job.job_type,
+                "schedule": job.schedule,
+                "next_run": job.next_run,
+                "enabled": job.enabled
+            }))?)),
+            Err(e) => Ok(ToolResult::error(e.to_string())),
         }
     }
 }
@@ -250,8 +216,8 @@ mod tests {
             .await
             .unwrap();
 
-        assert!(result.success, "{:?}", result.error);
-        assert!(result.output.contains("next_run"));
+        assert!(!result.is_error, "{:?}", result.output());
+        assert!(result.output().contains("next_run"));
     }
 
     #[tokio::test]
@@ -279,11 +245,8 @@ mod tests {
             .await
             .unwrap();
 
-        assert!(!result.success);
-        assert!(result
-            .error
-            .unwrap_or_default()
-            .contains("blocked by security policy"));
+        assert!(result.is_error);
+        assert!(result.output().contains("blocked by security policy"));
     }
 
     #[tokio::test]
@@ -301,11 +264,8 @@ mod tests {
             .await
             .unwrap();
 
-        assert!(!result.success);
-        assert!(result
-            .error
-            .unwrap_or_default()
-            .contains("every_ms must be > 0"));
+        assert!(result.is_error);
+        assert!(result.output().contains("every_ms must be > 0"));
     }
 
     #[tokio::test]
@@ -321,10 +281,7 @@ mod tests {
             }))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result
-            .error
-            .unwrap_or_default()
-            .contains("Missing 'prompt'"));
+        assert!(result.is_error);
+        assert!(result.output().contains("Missing 'prompt'"));
     }
 }

src/openhuman/tools/cron_list.rs

@@ -35,24 +35,14 @@ impl Tool for CronListTool {
 
     async fn execute(&self, _args: serde_json::Value) -> anyhow::Result<ToolResult> {
         if !self.config.cron.enabled {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("cron is disabled by config (cron.enabled=false)".to_string()),
-            });
+            return Ok(ToolResult::error(
+                "cron is disabled by config (cron.enabled=false)".to_string(),
+            ));
         }
 
         match cron::list_jobs(&self.config) {
-            Ok(jobs) => Ok(ToolResult {
-                success: true,
-                output: serde_json::to_string_pretty(&jobs)?,
-                error: None,
-            }),
-            Err(e) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(e.to_string()),
-            }),
+            Ok(jobs) => Ok(ToolResult::success(serde_json::to_string_pretty(&jobs)?)),
+            Err(e) => Ok(ToolResult::error(e.to_string())),
         }
     }
 }
@@ -82,8 +72,8 @@ mod tests {
         let tool = CronListTool::new(cfg);
 
         let result = tool.execute(json!({})).await.unwrap();
-        assert!(result.success);
-        assert_eq!(result.output.trim(), "[]");
+        assert!(!result.is_error);
+        assert_eq!(result.output().trim(), "[]");
     }
 
     #[tokio::test]
@@ -94,10 +84,7 @@ mod tests {
         let tool = CronListTool::new(Arc::new(cfg));
 
         let result = tool.execute(json!({})).await.unwrap();
-        assert!(!result.success);
-        assert!(result
-            .error
-            .unwrap_or_default()
-            .contains("cron is disabled"));
+        assert!(result.is_error);
+        assert!(result.output().contains("cron is disabled"));
     }
 }

src/openhuman/tools/cron_remove.rs

@@ -37,35 +37,21 @@ impl Tool for CronRemoveTool {
 
     async fn execute(&self, args: serde_json::Value) -> anyhow::Result<ToolResult> {
         if !self.config.cron.enabled {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("cron is disabled by config (cron.enabled=false)".to_string()),
-            });
+            return Ok(ToolResult::error(
+                "cron is disabled by config (cron.enabled=false)".to_string(),
+            ));
         }
 
         let job_id = match args.get("job_id").and_then(serde_json::Value::as_str) {
             Some(v) if !v.trim().is_empty() => v,
             _ => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some("Missing 'job_id' parameter".to_string()),
-                });
+                return Ok(ToolResult::error("Missing 'job_id' parameter".to_string()));
             }
         };
 
         match cron::remove_job(&self.config, job_id) {
-            Ok(()) => Ok(ToolResult {
-                success: true,
-                output: format!("Removed cron job {job_id}"),
-                error: None,
-            }),
-            Err(e) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(e.to_string()),
-            }),
+            Ok(()) => Ok(ToolResult::success(format!("Removed cron job {job_id}"))),
+            Err(e) => Ok(ToolResult::error(e.to_string())),
         }
     }
 }
@@ -96,7 +82,7 @@ mod tests {
         let tool = CronRemoveTool::new(cfg.clone());
 
         let result = tool.execute(json!({"job_id": job.id})).await.unwrap();
-        assert!(result.success);
+        assert!(!result.is_error);
         assert!(cron::list_jobs(&cfg).unwrap().is_empty());
     }
 
@@ -107,10 +93,7 @@ mod tests {
         let tool = CronRemoveTool::new(cfg);
 
         let result = tool.execute(json!({})).await.unwrap();
-        assert!(!result.success);
-        assert!(result
-            .error
-            .unwrap_or_default()
-            .contains("Missing 'job_id'"));
+        assert!(result.is_error);
+        assert!(result.output().contains("Missing 'job_id'"));
     }
 }

src/openhuman/tools/cron_run.rs

@@ -38,32 +38,22 @@ impl Tool for CronRunTool {
 
     async fn execute(&self, args: serde_json::Value) -> anyhow::Result<ToolResult> {
         if !self.config.cron.enabled {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("cron is disabled by config (cron.enabled=false)".to_string()),
-            });
+            return Ok(ToolResult::error(
+                "cron is disabled by config (cron.enabled=false)".to_string(),
+            ));
         }
 
         let job_id = match args.get("job_id").and_then(serde_json::Value::as_str) {
             Some(v) if !v.trim().is_empty() => v,
             _ => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some("Missing 'job_id' parameter".to_string()),
-                });
+                return Ok(ToolResult::error("Missing 'job_id' parameter".to_string()));
             }
         };
 
         let job = match cron::get_job(&self.config, job_id) {
             Ok(job) => job,
             Err(e) => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(e.to_string()),
-                });
+                return Ok(ToolResult::error(e.to_string()));
             }
         };
 
@@ -84,20 +74,17 @@ impl Tool for CronRunTool {
         );
         let _ = cron::record_last_run(&self.config, &job.id, finished_at, success, &output);
 
-        Ok(ToolResult {
-            success,
-            output: serde_json::to_string_pretty(&json!({
-                "job_id": job.id,
-                "status": status,
-                "duration_ms": duration_ms,
-                "output": output
-            }))?,
-            error: if success {
-                None
-            } else {
-                Some("cron job execution failed".to_string())
-            },
-        })
+        let result_output = serde_json::to_string_pretty(&json!({
+            "job_id": job.id,
+            "status": status,
+            "duration_ms": duration_ms,
+            "output": output
+        }))?;
+        if success {
+            Ok(ToolResult::success(result_output))
+        } else {
+            Ok(ToolResult::error(result_output))
+        }
     }
 }
 
@@ -127,7 +114,7 @@ mod tests {
         let tool = CronRunTool::new(cfg.clone());
 
         let result = tool.execute(json!({ "job_id": job.id })).await.unwrap();
-        assert!(result.success, "{:?}", result.error);
+        assert!(!result.is_error, "{:?}", result.output());
 
         let runs = cron::list_runs(&cfg, &job.id, 10).unwrap();
         assert_eq!(runs.len(), 1);
@@ -143,7 +130,7 @@ mod tests {
             .execute(json!({ "job_id": "missing-job-id" }))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result.error.unwrap_or_default().contains("not found"));
+        assert!(result.is_error);
+        assert!(result.output().contains("not found"));
     }
 }

src/openhuman/tools/cron_runs.rs

@@ -52,21 +52,15 @@ impl Tool for CronRunsTool {
 
     async fn execute(&self, args: serde_json::Value) -> anyhow::Result<ToolResult> {
         if !self.config.cron.enabled {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("cron is disabled by config (cron.enabled=false)".to_string()),
-            });
+            return Ok(ToolResult::error(
+                "cron is disabled by config (cron.enabled=false)".to_string(),
+            ));
         }
 
         let job_id = match args.get("job_id").and_then(serde_json::Value::as_str) {
             Some(v) if !v.trim().is_empty() => v,
             _ => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some("Missing 'job_id' parameter".to_string()),
-                });
+                return Ok(ToolResult::error("Missing 'job_id' parameter".to_string()));
             }
         };
 
@@ -90,17 +84,9 @@ impl Tool for CronRunsTool {
                     })
                     .collect();
 
-                Ok(ToolResult {
-                    success: true,
-                    output: serde_json::to_string_pretty(&runs)?,
-                    error: None,
-                })
+                Ok(ToolResult::success(serde_json::to_string_pretty(&runs)?))
             }
-            Err(e) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(e.to_string()),
-            }),
+            Err(e) => Ok(ToolResult::error(e.to_string())),
         }
     }
 }
@@ -158,8 +144,8 @@ mod tests {
             .await
             .unwrap();
 
-        assert!(result.success);
-        assert!(result.output.contains("..."));
+        assert!(!result.is_error);
+        assert!(result.output().contains("..."));
     }
 
     #[tokio::test]
@@ -168,10 +154,7 @@ mod tests {
         let cfg = test_config(&tmp).await;
         let tool = CronRunsTool::new(cfg);
         let result = tool.execute(json!({})).await.unwrap();
-        assert!(!result.success);
-        assert!(result
-            .error
-            .unwrap_or_default()
-            .contains("Missing 'job_id'"));
+        assert!(result.is_error);
+        assert!(result.output().contains("Missing 'job_id'"));
     }
 }

src/openhuman/tools/cron_update.rs

@@ -40,67 +40,43 @@ impl Tool for CronUpdateTool {
 
     async fn execute(&self, args: serde_json::Value) -> anyhow::Result<ToolResult> {
         if !self.config.cron.enabled {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("cron is disabled by config (cron.enabled=false)".to_string()),
-            });
+            return Ok(ToolResult::error(
+                "cron is disabled by config (cron.enabled=false)".to_string(),
+            ));
         }
 
         let job_id = match args.get("job_id").and_then(serde_json::Value::as_str) {
             Some(v) if !v.trim().is_empty() => v,
             _ => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some("Missing 'job_id' parameter".to_string()),
-                });
+                return Ok(ToolResult::error("Missing 'job_id' parameter".to_string()));
             }
         };
 
         let patch_val = match args.get("patch") {
             Some(v) => v.clone(),
             None => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some("Missing 'patch' parameter".to_string()),
-                });
+                return Ok(ToolResult::error("Missing 'patch' parameter".to_string()));
             }
         };
 
         let patch = match serde_json::from_value::<CronJobPatch>(patch_val) {
             Ok(patch) => patch,
             Err(e) => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(format!("Invalid patch payload: {e}")),
-                });
+                return Ok(ToolResult::error(format!("Invalid patch payload: {e}")));
             }
         };
 
         if let Some(command) = &patch.command {
             if !self.security.is_command_allowed(command) {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(format!("Command blocked by security policy: {command}")),
-                });
+                return Ok(ToolResult::error(format!(
+                    "Command blocked by security policy: {command}"
+                )));
             }
         }
 
         match cron::update_job(&self.config, job_id, patch) {
-            Ok(job) => Ok(ToolResult {
-                success: true,
-                output: serde_json::to_string_pretty(&job)?,
-                error: None,
-            }),
-            Err(e) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(e.to_string()),
-            }),
+            Ok(job) => Ok(ToolResult::success(serde_json::to_string_pretty(&job)?)),
+            Err(e) => Ok(ToolResult::error(e.to_string())),
         }
     }
 }
@@ -145,8 +121,8 @@ mod tests {
             .await
             .unwrap();
 
-        assert!(result.success, "{:?}", result.error);
-        assert!(result.output.contains("\"enabled\": false"));
+        assert!(!result.is_error, "{:?}", result.output());
+        assert!(result.output().contains("\"enabled\": false"));
     }
 
     #[tokio::test]
@@ -172,10 +148,7 @@ mod tests {
             }))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result
-            .error
-            .unwrap_or_default()
-            .contains("blocked by security policy"));
+        assert!(result.is_error);
+        assert!(result.output().contains("blocked by security policy"));
     }
 }

src/openhuman/tools/delegate.rs

@@ -141,11 +141,7 @@ impl Tool for DelegateTool {
             .ok_or_else(|| anyhow::anyhow!("Missing 'agent' parameter"))?;
 
         if agent_name.is_empty() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("'agent' parameter must not be empty".into()),
-            });
+            return Ok(ToolResult::error("'agent' parameter must not be empty"));
         }
 
         let prompt = args
@@ -155,11 +151,7 @@ impl Tool for DelegateTool {
             .ok_or_else(|| anyhow::anyhow!("Missing 'prompt' parameter"))?;
 
         if prompt.is_empty() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("'prompt' parameter must not be empty".into()),
-            });
+            return Ok(ToolResult::error("'prompt' parameter must not be empty"));
         }
 
         let context = args
@@ -174,44 +166,32 @@ impl Tool for DelegateTool {
             None => {
                 let available: Vec<&str> =
                     self.agents.keys().map(|s: &String| s.as_str()).collect();
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(format!(
-                        "Unknown agent '{agent_name}'. Available agents: {}",
-                        if available.is_empty() {
-                            "(none configured)".to_string()
-                        } else {
-                            available.join(", ")
-                        }
-                    )),
-                });
+                return Ok(ToolResult::error(format!(
+                    "Unknown agent '{agent_name}'. Available agents: {}",
+                    if available.is_empty() {
+                        "(none configured)".to_string()
+                    } else {
+                        available.join(", ")
+                    }
+                )));
             }
         };
 
         // Check recursion depth (immutable — set at construction, incremented for sub-agents)
         if self.depth >= agent_config.max_depth {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!(
-                    "Delegation depth limit reached ({depth}/{max}). \
+            return Ok(ToolResult::error(format!(
+                "Delegation depth limit reached ({depth}/{max}). \
                      Cannot delegate further to prevent infinite loops.",
-                    depth = self.depth,
-                    max = agent_config.max_depth
-                )),
-            });
+                depth = self.depth,
+                max = agent_config.max_depth
+            )));
         }
 
         if let Err(error) = self
             .security
             .enforce_tool_operation(ToolOperation::Act, "delegate")
         {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(error),
-            });
+            return Ok(ToolResult::error(error));
         }
 
         // Create provider for this agent
@@ -229,13 +209,9 @@ impl Tool for DelegateTool {
         ) {
             Ok(p) => p,
             Err(e) => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(format!(
-                        "Failed to create inference client for delegate agent '{agent_name}': {e}"
-                    )),
-                });
+                return Ok(ToolResult::error(format!(
+                    "Failed to create inference client for delegate agent '{agent_name}': {e}"
+                )));
             }
         };
 
@@ -264,13 +240,9 @@ impl Tool for DelegateTool {
         let result = match result {
             Ok(inner) => inner,
             Err(_elapsed) => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(format!(
-                        "Agent '{agent_name}' timed out after {delegate_timeout_secs}s"
-                    )),
-                });
+                return Ok(ToolResult::error(format!(
+                    "Agent '{agent_name}' timed out after {delegate_timeout_secs}s"
+                )));
             }
         };
 
@@ -281,21 +253,15 @@ impl Tool for DelegateTool {
                     rendered = "[Empty response]".to_string();
                 }
 
-                Ok(ToolResult {
-                    success: true,
-                    output: format!(
-                        "[Agent '{agent_name}' ({}/{}])\n{rendered}",
-                        providers::INFERENCE_BACKEND_ID,
-                        agent_config.model
-                    ),
-                    error: None,
-                })
+                Ok(ToolResult::success(format!(
+                    "[Agent '{agent_name}' ({}/{})]\n{rendered}",
+                    providers::INFERENCE_BACKEND_ID,
+                    agent_config.model
+                )))
             }
-            Err(e) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!("Agent '{agent_name}' failed: {e}",)),
-            }),
+            Err(e) => Ok(ToolResult::error(format!(
+                "Agent '{agent_name}' failed: {e}",
+            ))),
         }
     }
 }
@@ -387,8 +353,8 @@ mod tests {
             .execute(json!({"agent": "nonexistent", "prompt": "test"}))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result.error.unwrap().contains("Unknown agent"));
+        assert!(result.is_error);
+        assert!(result.output().contains("Unknown agent"));
     }
 
     #[tokio::test]
@@ -398,8 +364,8 @@ mod tests {
             .execute(json!({"agent": "researcher", "prompt": "test"}))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result.error.unwrap().contains("depth limit"));
+        assert!(result.is_error);
+        assert!(result.output().contains("depth limit"));
     }
 
     #[tokio::test]
@@ -410,8 +376,8 @@ mod tests {
             .execute(json!({"agent": "coder", "prompt": "test"}))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result.error.unwrap().contains("depth limit"));
+        assert!(result.is_error);
+        assert!(result.output().contains("depth limit"));
     }
 
     #[test]
@@ -431,8 +397,8 @@ mod tests {
             .execute(json!({"agent": "  ", "prompt": "test"}))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result.error.unwrap().contains("must not be empty"));
+        assert!(result.is_error);
+        assert!(result.output().contains("must not be empty"));
     }
 
     #[tokio::test]
@@ -442,8 +408,8 @@ mod tests {
             .execute(json!({"agent": "researcher", "prompt": "  \t  "}))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result.error.unwrap().contains("must not be empty"));
+        assert!(result.is_error);
+        assert!(result.output().contains("must not be empty"));
     }
 
     #[tokio::test]
@@ -456,14 +422,7 @@ mod tests {
             .unwrap();
         // Should find "researcher" after trim — will fail at provider level
         // since ollama isn't running, but must NOT get "Unknown agent".
-        assert!(
-            result.error.is_none()
-                || !result
-                    .error
-                    .as_deref()
-                    .unwrap_or("")
-                    .contains("Unknown agent")
-        );
+        assert!(!result.output().contains("Unknown agent"));
     }
 
     #[tokio::test]
@@ -477,12 +436,8 @@ mod tests {
             .execute(json!({"agent": "researcher", "prompt": "test"}))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result
-            .error
-            .as_deref()
-            .unwrap_or("")
-            .contains("read-only mode"));
+        assert!(result.is_error);
+        assert!(result.output().contains("read-only mode"));
     }
 
     #[tokio::test]
@@ -496,12 +451,8 @@ mod tests {
             .execute(json!({"agent": "researcher", "prompt": "test"}))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result
-            .error
-            .as_deref()
-            .unwrap_or("")
-            .contains("Rate limit exceeded"));
+        assert!(result.is_error);
+        assert!(result.output().contains("Rate limit exceeded"));
     }
 
     #[tokio::test]
@@ -527,14 +478,10 @@ mod tests {
             .await
             .unwrap();
 
-        assert!(!result.success);
+        assert!(result.is_error);
         assert!(
-            result
-                .error
-                .as_deref()
-                .unwrap_or("")
-                .contains("Agent 'tester' failed")
-                || result.error.as_deref().unwrap_or("").contains("timed out")
+            result.output().contains("Agent 'tester' failed")
+                || result.output().contains("timed out")
         );
     }
 
@@ -561,14 +508,10 @@ mod tests {
             .await
             .unwrap();
 
-        assert!(!result.success);
+        assert!(result.is_error);
         assert!(
-            result
-                .error
-                .as_deref()
-                .unwrap_or("")
-                .contains("Agent 'tester' failed")
-                || result.error.as_deref().unwrap_or("").contains("timed out")
+            result.output().contains("Agent 'tester' failed")
+                || result.output().contains("timed out")
         );
     }
 
@@ -585,7 +528,7 @@ mod tests {
             .execute(json!({"agent": "any", "prompt": "test"}))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result.error.unwrap().contains("none configured"));
+        assert!(result.is_error);
+        assert!(result.output().contains("none configured"));
     }
 }

src/openhuman/tools/file_read.rs

@@ -47,31 +47,25 @@ impl Tool for FileReadTool {
             .ok_or_else(|| anyhow::anyhow!("Missing 'path' parameter"))?;
 
         if self.security.is_rate_limited() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Rate limit exceeded: too many actions in the last hour".into()),
-            });
+            return Ok(ToolResult::error(
+                "Rate limit exceeded: too many actions in the last hour",
+            ));
         }
 
         // Security check: validate path is within workspace
         if !self.security.is_path_allowed(path) {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!("Path not allowed by security policy: {path}")),
-            });
+            return Ok(ToolResult::error(format!(
+                "Path not allowed by security policy: {path}"
+            )));
         }
 
         // Record action BEFORE canonicalization so that every non-trivially-rejected
         // request consumes rate limit budget. This prevents attackers from probing
         // path existence (via canonicalize errors) without rate limit cost.
         if !self.security.record_action() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Rate limit exceeded: action budget exhausted".into()),
-            });
+            return Ok(ToolResult::error(
+                "Rate limit exceeded: action budget exhausted",
+            ));
         }
 
         let full_path = self.security.workspace_dir.join(path);
@@ -80,59 +74,39 @@ impl Tool for FileReadTool {
         let resolved_path = match tokio::fs::canonicalize(&full_path).await {
             Ok(p) => p,
             Err(e) => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(format!("Failed to resolve file path: {e}")),
-                });
+                return Ok(ToolResult::error(format!(
+                    "Failed to resolve file path: {e}"
+                )));
             }
         };
 
         if !self.security.is_resolved_path_allowed(&resolved_path) {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!(
-                    "Resolved path escapes workspace: {}",
-                    resolved_path.display()
-                )),
-            });
+            return Ok(ToolResult::error(format!(
+                "Resolved path escapes workspace: {}",
+                resolved_path.display()
+            )));
         }
 
         // Check file size AFTER canonicalization to prevent TOCTOU symlink bypass
         match tokio::fs::metadata(&resolved_path).await {
             Ok(meta) => {
                 if meta.len() > MAX_FILE_SIZE_BYTES {
-                    return Ok(ToolResult {
-                        success: false,
-                        output: String::new(),
-                        error: Some(format!(
-                            "File too large: {} bytes (limit: {MAX_FILE_SIZE_BYTES} bytes)",
-                            meta.len()
-                        )),
-                    });
+                    return Ok(ToolResult::error(format!(
+                        "File too large: {} bytes (limit: {MAX_FILE_SIZE_BYTES} bytes)",
+                        meta.len()
+                    )));
                 }
             }
             Err(e) => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(format!("Failed to read file metadata: {e}")),
-                });
+                return Ok(ToolResult::error(format!(
+                    "Failed to read file metadata: {e}"
+                )));
             }
         }
 
         match tokio::fs::read_to_string(&resolved_path).await {
-            Ok(contents) => Ok(ToolResult {
-                success: true,
-                output: contents,
-                error: None,
-            }),
-            Err(e) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!("Failed to read file: {e}")),
-            }),
+            Ok(contents) => Ok(ToolResult::success(contents)),
+            Err(e) => Ok(ToolResult::error(format!("Failed to read file: {e}"))),
         }
     }
 }
@@ -191,9 +165,9 @@ mod tests {
 
         let tool = FileReadTool::new(test_security(dir.clone()));
         let result = tool.execute(json!({"path": "test.txt"})).await.unwrap();
-        assert!(result.success);
-        assert_eq!(result.output, "hello world");
-        assert!(result.error.is_none());
+        assert!(!result.is_error);
+        assert_eq!(result.output(), "hello world");
+        assert!(!result.is_error);
 
         let _ = tokio::fs::remove_dir_all(&dir).await;
     }
@@ -206,8 +180,8 @@ mod tests {
 
         let tool = FileReadTool::new(test_security(dir.clone()));
         let result = tool.execute(json!({"path": "nope.txt"})).await.unwrap();
-        assert!(!result.success);
-        assert!(result.error.as_ref().unwrap().contains("Failed to resolve"));
+        assert!(result.is_error);
+        assert!(&result.output().contains("Failed to resolve"));
 
         let _ = tokio::fs::remove_dir_all(&dir).await;
     }
@@ -223,8 +197,8 @@ mod tests {
             .execute(json!({"path": "../../../etc/passwd"}))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result.error.as_ref().unwrap().contains("not allowed"));
+        assert!(result.is_error);
+        assert!(&result.output().contains("not allowed"));
 
         let _ = tokio::fs::remove_dir_all(&dir).await;
     }
@@ -233,8 +207,8 @@ mod tests {
     async fn file_read_blocks_absolute_path() {
         let tool = FileReadTool::new(test_security(std::env::temp_dir()));
         let result = tool.execute(json!({"path": "/etc/passwd"})).await.unwrap();
-        assert!(!result.success);
-        assert!(result.error.as_ref().unwrap().contains("not allowed"));
+        assert!(result.is_error);
+        assert!(&result.output().contains("not allowed"));
     }
 
     #[tokio::test]
@@ -253,12 +227,8 @@ mod tests {
         ));
         let result = tool.execute(json!({"path": "test.txt"})).await.unwrap();
 
-        assert!(!result.success);
-        assert!(result
-            .error
-            .as_deref()
-            .unwrap_or("")
-            .contains("Rate limit exceeded"));
+        assert!(result.is_error);
+        assert!(result.output().contains("Rate limit exceeded"));
 
         let _ = tokio::fs::remove_dir_all(&dir).await;
     }
@@ -275,8 +245,8 @@ mod tests {
         let tool = FileReadTool::new(test_security_with(dir.clone(), AutonomyLevel::ReadOnly, 20));
         let result = tool.execute(json!({"path": "test.txt"})).await.unwrap();
 
-        assert!(result.success);
-        assert_eq!(result.output, "readonly ok");
+        assert!(!result.is_error);
+        assert_eq!(result.output(), "readonly ok");
 
         let _ = tokio::fs::remove_dir_all(&dir).await;
     }
@@ -297,8 +267,8 @@ mod tests {
 
         let tool = FileReadTool::new(test_security(dir.clone()));
         let result = tool.execute(json!({"path": "empty.txt"})).await.unwrap();
-        assert!(result.success);
-        assert_eq!(result.output, "");
+        assert!(!result.is_error);
+        assert_eq!(result.output(), "");
 
         let _ = tokio::fs::remove_dir_all(&dir).await;
     }
@@ -319,8 +289,8 @@ mod tests {
             .execute(json!({"path": "sub/dir/deep.txt"}))
             .await
             .unwrap();
-        assert!(result.success);
-        assert_eq!(result.output, "deep content");
+        assert!(!result.is_error);
+        assert_eq!(result.output(), "deep content");
 
         let _ = tokio::fs::remove_dir_all(&dir).await;
     }
@@ -347,12 +317,8 @@ mod tests {
         let tool = FileReadTool::new(test_security(workspace.clone()));
         let result = tool.execute(json!({"path": "escape.txt"})).await.unwrap();
 
-        assert!(!result.success);
-        assert!(result
-            .error
-            .as_deref()
-            .unwrap_or("")
-            .contains("escapes workspace"));
+        assert!(result.is_error);
+        assert!(result.output().contains("escapes workspace"));
 
         let _ = tokio::fs::remove_dir_all(&root).await;
     }
@@ -372,20 +338,21 @@ mod tests {
 
         // Both reads fail (file doesn't exist) but should consume budget
         let r1 = tool.execute(json!({"path": "nope1.txt"})).await.unwrap();
-        assert!(!r1.success);
-        assert!(r1.error.as_ref().unwrap().contains("Failed to resolve"));
+        assert!(r1.is_error);
+        assert!(r1.output().contains("Failed to resolve"));
 
         let r2 = tool.execute(json!({"path": "nope2.txt"})).await.unwrap();
-        assert!(!r2.success);
-        assert!(r2.error.as_ref().unwrap().contains("Failed to resolve"));
+        assert!(r2.is_error);
+        assert!(r2.output().contains("Failed to resolve"));
 
         // Third attempt should be rate limited even though file doesn't exist
         let r3 = tool.execute(json!({"path": "nope3.txt"})).await.unwrap();
-        assert!(!r3.success);
+        assert!(r3.is_error);
+        let r3_output = r3.output();
         assert!(
-            r3.error.as_ref().unwrap().contains("Rate limit"),
+            r3_output.contains("Rate limit"),
             "Expected rate limit error, got: {:?}",
-            r3.error
+            r3_output
         );
 
         let _ = tokio::fs::remove_dir_all(&dir).await;
@@ -403,8 +370,8 @@ mod tests {
 
         let tool = FileReadTool::new(test_security(dir.clone()));
         let result = tool.execute(json!({"path": "huge.bin"})).await.unwrap();
-        assert!(!result.success);
-        assert!(result.error.as_ref().unwrap().contains("File too large"));
+        assert!(result.is_error);
+        assert!(&result.output().contains("File too large"));
 
         let _ = tokio::fs::remove_dir_all(&dir).await;
     }

src/openhuman/tools/file_write.rs

@@ -54,38 +54,26 @@ impl Tool for FileWriteTool {
             .ok_or_else(|| anyhow::anyhow!("Missing 'content' parameter"))?;
 
         if !self.security.can_act() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Action blocked: autonomy is read-only".into()),
-            });
+            return Ok(ToolResult::error("Action blocked: autonomy is read-only"));
         }
 
         if self.security.is_rate_limited() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Rate limit exceeded: too many actions in the last hour".into()),
-            });
+            return Ok(ToolResult::error(
+                "Rate limit exceeded: too many actions in the last hour",
+            ));
         }
 
         // Security check: validate path is within workspace
         if !self.security.is_path_allowed(path) {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!("Path not allowed by security policy: {path}")),
-            });
+            return Ok(ToolResult::error(format!(
+                "Path not allowed by security policy: {path}"
+            )));
         }
 
         let full_path = self.security.workspace_dir.join(path);
 
         let Some(parent) = full_path.parent() else {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Invalid path: missing parent directory".into()),
-            });
+            return Ok(ToolResult::error("Invalid path: missing parent directory"));
         };
 
         // Ensure parent directory exists
@@ -95,31 +83,21 @@ impl Tool for FileWriteTool {
         let resolved_parent = match tokio::fs::canonicalize(parent).await {
             Ok(p) => p,
             Err(e) => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(format!("Failed to resolve file path: {e}")),
-                });
+                return Ok(ToolResult::error(format!(
+                    "Failed to resolve file path: {e}"
+                )));
             }
         };
 
         if !self.security.is_resolved_path_allowed(&resolved_parent) {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!(
-                    "Resolved path escapes workspace: {}",
-                    resolved_parent.display()
-                )),
-            });
+            return Ok(ToolResult::error(format!(
+                "Resolved path escapes workspace: {}",
+                resolved_parent.display()
+            )));
         }
 
         let Some(file_name) = full_path.file_name() else {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Invalid path: missing file name".into()),
-            });
+            return Ok(ToolResult::error("Invalid path: missing file name"));
         };
 
         let resolved_target = resolved_parent.join(file_name);
@@ -127,36 +105,25 @@ impl Tool for FileWriteTool {
         // If the target already exists and is a symlink, refuse to follow it
         if let Ok(meta) = tokio::fs::symlink_metadata(&resolved_target).await {
             if meta.file_type().is_symlink() {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(format!(
-                        "Refusing to write through symlink: {}",
-                        resolved_target.display()
-                    )),
-                });
+                return Ok(ToolResult::error(format!(
+                    "Refusing to write through symlink: {}",
+                    resolved_target.display()
+                )));
             }
         }
 
         if !self.security.record_action() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Rate limit exceeded: action budget exhausted".into()),
-            });
+            return Ok(ToolResult::error(
+                "Rate limit exceeded: action budget exhausted",
+            ));
         }
 
         match tokio::fs::write(&resolved_target, content).await {
-            Ok(()) => Ok(ToolResult {
-                success: true,
-                output: format!("Written {} bytes to {path}", content.len()),
-                error: None,
-            }),
-            Err(e) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!("Failed to write file: {e}")),
-            }),
+            Ok(()) => Ok(ToolResult::success(format!(
+                "Written {} bytes to {path}",
+                content.len()
+            ))),
+            Err(e) => Ok(ToolResult::error(format!("Failed to write file: {e}"))),
         }
     }
 }
@@ -215,8 +182,8 @@ mod tests {
             .execute(json!({"path": "out.txt", "content": "written!"}))
             .await
             .unwrap();
-        assert!(result.success);
-        assert!(result.output.contains("8 bytes"));
+        assert!(!result.is_error);
+        assert!(result.output().contains("8 bytes"));
 
         let content = tokio::fs::read_to_string(dir.join("out.txt"))
             .await
@@ -237,7 +204,7 @@ mod tests {
             .execute(json!({"path": "a/b/c/deep.txt", "content": "deep"}))
             .await
             .unwrap();
-        assert!(result.success);
+        assert!(!result.is_error);
 
         let content = tokio::fs::read_to_string(dir.join("a/b/c/deep.txt"))
             .await
@@ -261,7 +228,7 @@ mod tests {
             .execute(json!({"path": "exist.txt", "content": "new"}))
             .await
             .unwrap();
-        assert!(result.success);
+        assert!(!result.is_error);
 
         let content = tokio::fs::read_to_string(dir.join("exist.txt"))
             .await
@@ -282,8 +249,8 @@ mod tests {
             .execute(json!({"path": "../../etc/evil", "content": "bad"}))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result.error.as_ref().unwrap().contains("not allowed"));
+        assert!(result.is_error);
+        assert!(&result.output().contains("not allowed"));
 
         let _ = tokio::fs::remove_dir_all(&dir).await;
     }
@@ -295,8 +262,8 @@ mod tests {
             .execute(json!({"path": "/etc/evil", "content": "bad"}))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result.error.as_ref().unwrap().contains("not allowed"));
+        assert!(result.is_error);
+        assert!(&result.output().contains("not allowed"));
     }
 
     #[tokio::test]
@@ -324,8 +291,8 @@ mod tests {
             .execute(json!({"path": "empty.txt", "content": ""}))
             .await
             .unwrap();
-        assert!(result.success);
-        assert!(result.output.contains("0 bytes"));
+        assert!(!result.is_error);
+        assert!(result.output().contains("0 bytes"));
 
         let _ = tokio::fs::remove_dir_all(&dir).await;
     }
@@ -351,12 +318,8 @@ mod tests {
             .await
             .unwrap();
 
-        assert!(!result.success);
-        assert!(result
-            .error
-            .as_deref()
-            .unwrap_or("")
-            .contains("escapes workspace"));
+        assert!(result.is_error);
+        assert!(result.output().contains("escapes workspace"));
         assert!(!outside.join("hijack.txt").exists());
 
         let _ = tokio::fs::remove_dir_all(&root).await;
@@ -374,8 +337,8 @@ mod tests {
             .await
             .unwrap();
 
-        assert!(!result.success);
-        assert!(result.error.as_deref().unwrap_or("").contains("read-only"));
+        assert!(result.is_error);
+        assert!(result.output().contains("read-only"));
         assert!(!dir.join("out.txt").exists());
 
         let _ = tokio::fs::remove_dir_all(&dir).await;
@@ -397,12 +360,8 @@ mod tests {
             .await
             .unwrap();
 
-        assert!(!result.success);
-        assert!(result
-            .error
-            .as_deref()
-            .unwrap_or("")
-            .contains("Rate limit exceeded"));
+        assert!(result.is_error);
+        assert!(result.output().contains("Rate limit exceeded"));
         assert!(!dir.join("out.txt").exists());
 
         let _ = tokio::fs::remove_dir_all(&dir).await;
@@ -435,9 +394,9 @@ mod tests {
             .await
             .unwrap();
 
-        assert!(!result.success, "writing through symlink must be blocked");
+        assert!(result.is_error, "writing through symlink must be blocked");
         assert!(
-            result.error.as_deref().unwrap_or("").contains("symlink"),
+            result.output().contains("symlink"),
             "error should mention symlink"
         );
 
@@ -461,7 +420,7 @@ mod tests {
             .execute(json!({"path": "file\u{0000}.txt", "content": "bad"}))
             .await
             .unwrap();
-        assert!(!result.success, "paths with null bytes must be blocked");
+        assert!(result.is_error, "paths with null bytes must be blocked");
 
         let _ = tokio::fs::remove_dir_all(&dir).await;
     }

src/openhuman/tools/git_operations.rs

@@ -124,11 +124,9 @@ impl GitOperationsTool {
             json!(staged.is_empty() && unstaged.is_empty() && untracked.is_empty()),
         );
 
-        Ok(ToolResult {
-            success: true,
-            output: serde_json::to_string_pretty(&result).unwrap_or_default(),
-            error: None,
-        })
+        Ok(ToolResult::success(
+            serde_json::to_string_pretty(&result).unwrap_or_default(),
+        ))
     }
 
     async fn git_diff(&self, args: serde_json::Value) -> anyhow::Result<ToolResult> {
@@ -203,11 +201,9 @@ impl GitOperationsTool {
         result.insert("hunks".to_string(), json!(hunks));
         result.insert("file_count".to_string(), json!(hunks.len()));
 
-        Ok(ToolResult {
-            success: true,
-            output: serde_json::to_string_pretty(&result).unwrap_or_default(),
-            error: None,
-        })
+        Ok(ToolResult::success(
+            serde_json::to_string_pretty(&result).unwrap_or_default(),
+        ))
     }
 
     async fn git_log(&self, args: serde_json::Value) -> anyhow::Result<ToolResult> {
@@ -239,12 +235,9 @@ impl GitOperationsTool {
             }
         }
 
-        Ok(ToolResult {
-            success: true,
-            output: serde_json::to_string_pretty(&json!({ "commits": commits }))
-                .unwrap_or_default(),
-            error: None,
-        })
+        Ok(ToolResult::success(
+            serde_json::to_string_pretty(&json!({ "commits": commits })).unwrap_or_default(),
+        ))
     }
 
     async fn git_branch(&self, _args: serde_json::Value) -> anyhow::Result<ToolResult> {
@@ -268,15 +261,13 @@ impl GitOperationsTool {
             }
         }
 
-        Ok(ToolResult {
-            success: true,
-            output: serde_json::to_string_pretty(&json!({
+        Ok(ToolResult::success(
+            serde_json::to_string_pretty(&json!({
                 "current": current,
                 "branches": branches
             }))
             .unwrap_or_default(),
-            error: None,
-        })
+        ))
     }
 
     fn truncate_commit_message(message: &str) -> String {
@@ -311,16 +302,8 @@ impl GitOperationsTool {
         let output = self.run_git_command(&["commit", "-m", &message]).await;
 
         match output {
-            Ok(_) => Ok(ToolResult {
-                success: true,
-                output: format!("Committed: {message}"),
-                error: None,
-            }),
-            Err(e) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!("Commit failed: {e}")),
-            }),
+            Ok(_) => Ok(ToolResult::success(format!("Committed: {message}"))),
+            Err(e) => Ok(ToolResult::error(format!("Commit failed: {e}"))),
         }
     }
 
@@ -336,16 +319,8 @@ impl GitOperationsTool {
         let output = self.run_git_command(&["add", "--", paths]).await;
 
         match output {
-            Ok(_) => Ok(ToolResult {
-                success: true,
-                output: format!("Staged: {paths}"),
-                error: None,
-            }),
-            Err(e) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!("Add failed: {e}")),
-            }),
+            Ok(_) => Ok(ToolResult::success(format!("Staged: {paths}"))),
+            Err(e) => Ok(ToolResult::error(format!("Add failed: {e}"))),
         }
     }
 
@@ -372,16 +347,10 @@ impl GitOperationsTool {
         let output = self.run_git_command(&["checkout", branch_name]).await;
 
         match output {
-            Ok(_) => Ok(ToolResult {
-                success: true,
-                output: format!("Switched to branch: {branch_name}"),
-                error: None,
-            }),
-            Err(e) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!("Checkout failed: {e}")),
-            }),
+            Ok(_) => Ok(ToolResult::success(format!(
+                "Switched to branch: {branch_name}"
+            ))),
+            Err(e) => Ok(ToolResult::error(format!("Checkout failed: {e}"))),
         }
     }
 
@@ -409,16 +378,8 @@ impl GitOperationsTool {
         };
 
         match output {
-            Ok(out) => Ok(ToolResult {
-                success: true,
-                output: out,
-                error: None,
-            }),
-            Err(e) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!("Stash {action} failed: {e}")),
-            }),
+            Ok(out) => Ok(ToolResult::success(out)),
+            Err(e) => Ok(ToolResult::error(format!("Stash {action} failed: {e}"))),
         }
     }
 }
@@ -484,11 +445,7 @@ impl Tool for GitOperationsTool {
         let operation = match args.get("operation").and_then(|v| v.as_str()) {
             Some(op) => op,
             None => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some("Missing 'operation' parameter".into()),
-                });
+                return Ok(ToolResult::error("Missing 'operation' parameter"));
             }
         };
 
@@ -506,33 +463,21 @@ impl Tool for GitOperationsTool {
             }
 
             if !found_git {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some("Not in a git repository".into()),
-                });
+                return Ok(ToolResult::error("Not in a git repository"));
             }
         }
 
         // Check autonomy level for write operations
         if self.requires_write_access(operation) {
             if !self.security.can_act() {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(
-                        "Action blocked: git write operations require higher autonomy level".into(),
-                    ),
-                });
+                return Ok(ToolResult::error(
+                    "Action blocked: git write operations require higher autonomy level",
+                ));
             }
 
             match self.security.autonomy {
                 AutonomyLevel::ReadOnly => {
-                    return Ok(ToolResult {
-                        success: false,
-                        output: String::new(),
-                        error: Some("Action blocked: read-only mode".into()),
-                    });
+                    return Ok(ToolResult::error("Action blocked: read-only mode"));
                 }
                 AutonomyLevel::Supervised | AutonomyLevel::Full => {}
             }
@@ -540,11 +485,7 @@ impl Tool for GitOperationsTool {
 
         // Record action for rate limiting
         if !self.security.record_action() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Action blocked: rate limit exceeded".into()),
-            });
+            return Ok(ToolResult::error("Action blocked: rate limit exceeded"));
         }
 
         // Execute the requested operation
@@ -557,11 +498,7 @@ impl Tool for GitOperationsTool {
             "add" => self.git_add(args).await,
             "checkout" => self.git_checkout(args).await,
             "stash" => self.git_stash(args).await,
-            _ => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!("Unknown operation: {operation}")),
-            }),
+            _ => Ok(ToolResult::error(format!("Unknown operation: {operation}"))),
         }
     }
 }
@@ -710,13 +647,9 @@ mod tests {
             .execute(json!({"operation": "commit", "message": "test"}))
             .await
             .unwrap();
-        assert!(!result.success);
+        assert!(result.is_error);
         // can_act() returns false for ReadOnly, so we get the "higher autonomy level" message
-        assert!(result
-            .error
-            .as_deref()
-            .unwrap_or("")
-            .contains("higher autonomy"));
+        assert!(result.output().contains("higher autonomy"));
     }
 
     #[tokio::test]
@@ -737,7 +670,7 @@ mod tests {
 
         let result = tool.execute(json!({"operation": "branch"})).await.unwrap();
         // Branch listing must not be blocked by read-only autonomy
-        let error_msg = result.error.as_deref().unwrap_or("");
+        let error_msg = result.output();
         assert!(
             !error_msg.contains("read-only") && !error_msg.contains("higher autonomy"),
             "branch listing should not be blocked in read-only mode, got: {error_msg}"
@@ -756,8 +689,8 @@ mod tests {
         // This will fail because there's no git repo, but it shouldn't be blocked by autonomy
         let result = tool.execute(json!({"operation": "status"})).await.unwrap();
         // The error should be about git (not about autonomy/read-only mode)
-        assert!(!result.success, "Expected failure due to missing git repo");
-        let error_msg = result.error.as_deref().unwrap_or("");
+        assert!(result.is_error, "Expected failure due to missing git repo");
+        let error_msg = result.output();
         assert!(
             !error_msg.is_empty(),
             "Expected a git-related error message"
@@ -774,12 +707,8 @@ mod tests {
         let tool = test_tool(tmp.path());
 
         let result = tool.execute(json!({})).await.unwrap();
-        assert!(!result.success);
-        assert!(result
-            .error
-            .as_deref()
-            .unwrap_or("")
-            .contains("Missing 'operation'"));
+        assert!(result.is_error);
+        assert!(result.output().contains("Missing 'operation'"));
     }
 
     #[tokio::test]
@@ -795,12 +724,8 @@ mod tests {
         let tool = test_tool(tmp.path());
 
         let result = tool.execute(json!({"operation": "push"})).await.unwrap();
-        assert!(!result.success);
-        assert!(result
-            .error
-            .as_deref()
-            .unwrap_or("")
-            .contains("Unknown operation"));
+        assert!(result.is_error);
+        assert!(result.output().contains("Unknown operation"));
     }
 
     #[test]

src/openhuman/tools/hardware_board_info.rs

@@ -96,14 +96,9 @@ impl Tool for HardwareBoardInfoTool {
         let board = board.as_deref().unwrap_or("unknown");
 
         if self.boards.is_empty() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(
-                    "No peripherals configured. Add boards to config.toml [peripherals.boards]."
-                        .into(),
-                ),
-            });
+            return Ok(ToolResult::error(
+                "No peripherals configured. Add boards to config.toml [peripherals.boards].",
+            ));
         }
 
         let mut output = String::new();
@@ -117,11 +112,7 @@ impl Tool for HardwareBoardInfoTool {
             };
             match probe_board_info(chip) {
                 Ok(info) => {
-                    return Ok(ToolResult {
-                        success: true,
-                        output: info,
-                        error: None,
-                    });
+                    return Ok(ToolResult::success(info));
                 }
                 Err(e) => {
                     use std::fmt::Write;
@@ -147,11 +138,7 @@ impl Tool for HardwareBoardInfoTool {
             );
         }
 
-        Ok(ToolResult {
-            success: true,
-            output,
-            error: None,
-        })
+        Ok(ToolResult::success(output))
     }
 }
 

src/openhuman/tools/hardware_memory_map.rs

@@ -82,14 +82,9 @@ impl Tool for HardwareMemoryMapTool {
         let board = board.as_deref().unwrap_or("unknown");
 
         if self.boards.is_empty() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(
-                    "No peripherals configured. Add boards to config.toml [peripherals.boards]."
-                        .into(),
-                ),
-            });
+            return Ok(ToolResult::error(
+                "No peripherals configured. Add boards to config.toml [peripherals.boards].",
+            ));
         }
 
         let mut output = String::new();
@@ -135,11 +130,7 @@ impl Tool for HardwareMemoryMapTool {
             }
         }
 
-        Ok(ToolResult {
-            success: true,
-            output,
-            error: None,
-        })
+        Ok(ToolResult::success(output))
     }
 }
 

src/openhuman/tools/hardware_memory_read.rs

@@ -61,14 +61,9 @@ impl Tool for HardwareMemoryReadTool {
 
     async fn execute(&self, args: serde_json::Value) -> anyhow::Result<ToolResult> {
         if self.boards.is_empty() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(
-                    "No peripherals configured. Add nucleo-f401re to config.toml [peripherals.boards]."
-                        .into(),
-                ),
-            });
+            return Ok(ToolResult::error(
+                "No peripherals configured. Add nucleo-f401re to config.toml [peripherals.boards].",
+            ));
         }
 
         let board = args
@@ -80,14 +75,10 @@ impl Tool for HardwareMemoryReadTool {
 
         let chip = Self::chip_for_board(&board);
         if chip.is_none() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!(
-                    "Memory read only supports nucleo-f401re, nucleo-f411re. Got: {}",
-                    board
-                )),
-            });
+            return Ok(ToolResult::error(format!(
+                "Memory read only supports nucleo-f401re, nucleo-f411re. Got: {}",
+                board
+            )));
         }
 
         let address_str = args
@@ -105,35 +96,22 @@ impl Tool for HardwareMemoryReadTool {
         {
             match probe_read_memory(chip.unwrap(), _address, _length) {
                 Ok(output) => {
-                    return Ok(ToolResult {
-                        success: true,
-                        output,
-                        error: None,
-                    });
+                    return Ok(ToolResult::success(output));
                 }
                 Err(e) => {
-                    return Ok(ToolResult {
-                        success: false,
-                        output: String::new(),
-                        error: Some(format!(
+                    return Ok(ToolResult::error(format!(
                             "probe-rs read failed: {}. Ensure Nucleo is connected via USB and built with --features probe.",
                             e
-                        )),
-                    });
+                        )));
                 }
             }
         }
 
         #[cfg(not(feature = "probe"))]
         {
-            Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(
-                    "Memory read requires probe feature. Build with: cargo build --features hardware,probe"
-                        .into(),
-                ),
-            })
+            Ok(ToolResult::error(
+                    "Memory read requires probe feature. Build with: cargo build --features hardware,probe",
+                ))
         }
     }
 }

src/openhuman/tools/http_request.rs

@@ -198,41 +198,21 @@ impl Tool for HttpRequestTool {
         let body = args.get("body").and_then(|v| v.as_str());
 
         if !self.security.can_act() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Action blocked: autonomy is read-only".into()),
-            });
+            return Ok(ToolResult::error("Action blocked: autonomy is read-only"));
         }
 
         if !self.security.record_action() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Action blocked: rate limit exceeded".into()),
-            });
+            return Ok(ToolResult::error("Action blocked: rate limit exceeded"));
         }
 
         let url = match self.validate_url(url) {
             Ok(v) => v,
-            Err(e) => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(e.to_string()),
-                })
-            }
+            Err(e) => return Ok(ToolResult::error(e.to_string())),
         };
 
         let method = match self.validate_method(method_str) {
             Ok(m) => m,
-            Err(e) => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(e.to_string()),
-                })
-            }
+            Err(e) => return Ok(ToolResult::error(e.to_string())),
         };
 
         let request_headers = self.parse_headers(&headers_val);
@@ -273,21 +253,13 @@ impl Tool for HttpRequestTool {
                     response_text
                 );
 
-                Ok(ToolResult {
-                    success: status.is_success(),
-                    output,
-                    error: if status.is_client_error() || status.is_server_error() {
-                        Some(format!("HTTP {}", status_code))
-                    } else {
-                        None
-                    },
-                })
+                if status.is_success() {
+                    Ok(ToolResult::success(output))
+                } else {
+                    Ok(ToolResult::error(format!("HTTP {}", status_code)))
+                }
             }
-            Err(e) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!("HTTP request failed: {e}")),
-            }),
+            Err(e) => Ok(ToolResult::error(format!("HTTP request failed: {e}"))),
         }
     }
 }
@@ -662,8 +634,8 @@ mod tests {
             .execute(json!({"url": "https://example.com"}))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result.error.unwrap().contains("read-only"));
+        assert!(result.is_error);
+        assert!(result.output().contains("read-only"));
     }
 
     #[tokio::test]
@@ -677,8 +649,8 @@ mod tests {
             .execute(json!({"url": "https://example.com"}))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result.error.unwrap().contains("rate limit"));
+        assert!(result.is_error);
+        assert!(result.output().contains("rate limit"));
     }
 
     #[test]

src/openhuman/tools/image_info.rs

@@ -160,21 +160,13 @@ impl Tool for ImageInfoTool {
 
         // Restrict reads to workspace directory to prevent arbitrary file exfiltration
         if !self.security.is_path_allowed(path_str) {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!(
-                    "Path not allowed: {path_str} (must be within workspace)"
-                )),
-            });
+            return Ok(ToolResult::error(format!(
+                "Path not allowed: {path_str} (must be within workspace)"
+            )));
         }
 
         if !path.exists() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!("File not found: {path_str}")),
-            });
+            return Ok(ToolResult::error(format!("File not found: {path_str}")));
         }
 
         let metadata = tokio::fs::metadata(path)
@@ -184,13 +176,9 @@ impl Tool for ImageInfoTool {
         let file_size = metadata.len();
 
         if file_size > MAX_IMAGE_BYTES {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!(
-                    "Image too large: {file_size} bytes (max {MAX_IMAGE_BYTES} bytes)"
-                )),
-            });
+            return Ok(ToolResult::error(format!(
+                "Image too large: {file_size} bytes (max {MAX_IMAGE_BYTES} bytes)"
+            )));
         }
 
         let bytes = tokio::fs::read(path)
@@ -220,11 +208,7 @@ impl Tool for ImageInfoTool {
             let _ = write!(output, "\ndata:{mime};base64,{encoded}");
         }
 
-        Ok(ToolResult {
-            success: true,
-            output,
-            error: None,
-        })
+        Ok(ToolResult::success(output))
     }
 }
 
@@ -420,8 +404,8 @@ mod tests {
             .execute(json!({"path": "/tmp/nonexistent_image_xyz.png"}))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result.error.as_ref().unwrap().contains("not found"));
+        assert!(result.is_error);
+        assert!(&result.output().contains("not found"));
     }
 
     #[tokio::test]
@@ -455,10 +439,10 @@ mod tests {
             .execute(json!({"path": png_path.to_string_lossy()}))
             .await
             .unwrap();
-        assert!(result.success);
-        assert!(result.output.contains("Format: png"));
-        assert!(result.output.contains("Dimensions: 1x1"));
-        assert!(!result.output.contains("data:"));
+        assert!(!result.is_error);
+        assert!(result.output().contains("Format: png"));
+        assert!(result.output().contains("Dimensions: 1x1"));
+        assert!(!result.output().contains("data:"));
 
         // Clean up
         let _ = tokio::fs::remove_dir_all(&dir).await;
@@ -485,8 +469,8 @@ mod tests {
             .execute(json!({"path": png_path.to_string_lossy(), "include_base64": true}))
             .await
             .unwrap();
-        assert!(result.success);
-        assert!(result.output.contains("data:image/png;base64,"));
+        assert!(!result.is_error);
+        assert!(result.output().contains("data:image/png;base64,"));
 
         let _ = tokio::fs::remove_dir_all(&dir).await;
     }

src/openhuman/tools/insert_sql_record.rs

@@ -91,29 +91,17 @@ impl Tool for InsertSqlRecordTool {
 
         // ── Validation ──────────────────────────────────────────────────────
         if !VALID_ROLES.contains(&role) {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!(
-                    "Invalid role '{role}'. Must be one of: user, assistant, tool."
-                )),
-            });
+            return Ok(ToolResult::error(format!(
+                "Invalid role '{role}'. Must be one of: user, assistant, tool."
+            )));
         }
 
         if session_id.trim().is_empty() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("'session_id' must not be empty.".into()),
-            });
+            return Ok(ToolResult::error("'session_id' must not be empty."));
         }
 
         if content.trim().is_empty() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("'content' must not be empty.".into()),
-            });
+            return Ok(ToolResult::error("'content' must not be empty."));
         }
 
         // ── Structured trace log ────────────────────────────────────────────
@@ -145,11 +133,9 @@ impl Tool for InsertSqlRecordTool {
             lesson.map_or("none".to_string(), |l| format!("{} chars", l.len())),
         );
 
-        Ok(ToolResult {
-            success: false,
-            output: summary,
-            error: Some("episodic memory write not yet wired (FTS5/SQLite insert pending)".into()),
-        })
+        Ok(ToolResult::error(format!(
+            "episodic memory write not yet wired (FTS5/SQLite insert pending). {summary}"
+        )))
     }
 }
 
@@ -172,14 +158,10 @@ mod tests {
             .await
             .unwrap();
         // The tool is a stub: success is false until FTS5 write is wired.
-        assert!(!result.success);
-        assert!(result
-            .error
-            .as_deref()
-            .unwrap_or("")
-            .contains("not yet wired"));
-        assert!(result.output.contains("sess-001"));
-        assert!(result.output.contains("user"));
+        assert!(result.is_error);
+        assert!(result.output().contains("not yet wired"));
+        assert!(result.output().contains("sess-001"));
+        assert!(result.output().contains("user"));
     }
 
     #[tokio::test]
@@ -194,8 +176,8 @@ mod tests {
             .await
             .unwrap();
         // The tool is a stub: success is false until FTS5 write is wired.
-        assert!(!result.success);
-        assert!(result.output.contains("lesson="));
+        assert!(result.is_error);
+        assert!(result.output().contains("lesson="));
     }
 
     #[tokio::test]
@@ -208,12 +190,8 @@ mod tests {
             }))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result
-            .error
-            .as_deref()
-            .unwrap_or("")
-            .contains("Invalid role"));
+        assert!(result.is_error);
+        assert!(result.output().contains("Invalid role"));
     }
 
     #[tokio::test]
@@ -226,8 +204,8 @@ mod tests {
             }))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result.error.as_deref().unwrap_or("").contains("session_id"));
+        assert!(result.is_error);
+        assert!(result.output().contains("session_id"));
     }
 
     #[tokio::test]
@@ -240,8 +218,8 @@ mod tests {
             }))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result.error.as_deref().unwrap_or("").contains("content"));
+        assert!(result.is_error);
+        assert!(result.output().contains("content"));
     }
 
     #[tokio::test]

src/openhuman/tools/local_cli.rs

@@ -70,7 +70,7 @@ pub async fn run_cli_screenshot(args: CliScreenshotArgs) -> Result<serde_json::V
     let mut logs = vec!["tools.screenshot executed".to_string()];
 
     if let Some(output_path) = args.output.as_ref() {
-        if let Some(saved_path) = extract_saved_path(&tool_result.output) {
+        if let Some(saved_path) = extract_saved_path(&tool_result.output()) {
             std::fs::copy(&saved_path, output_path).map_err(|e| {
                 format!(
                     "failed to copy screenshot from {} to {}: {e}",
@@ -79,7 +79,7 @@ pub async fn run_cli_screenshot(args: CliScreenshotArgs) -> Result<serde_json::V
                 )
             })?;
             logs.push(format!("copied screenshot to {}", output_path.display()));
-        } else if let Some(data_url) = extract_data_url(&tool_result.output) {
+        } else if let Some(data_url) = extract_data_url(&tool_result.output()) {
             let bytes = decode_data_url_bytes(&data_url)?;
             write_bytes_to_path(output_path, &bytes)?;
             logs.push(format!(
@@ -95,13 +95,13 @@ pub async fn run_cli_screenshot(args: CliScreenshotArgs) -> Result<serde_json::V
         }
     }
 
-    let data_url = extract_data_url(&tool_result.output);
+    let data_url = extract_data_url(&tool_result.output());
     Ok(json!({
         "result": {
-            "success": tool_result.success,
-            "error": tool_result.error,
+            "success": !tool_result.is_error,
+            "error": if tool_result.is_error { Some(tool_result.output()) } else { None::<String> },
             "output_path": args.output.as_ref().map(|p| p.display().to_string()),
-            "tool_output": tool_result.output,
+            "tool_output": tool_result.output(),
             "data_url": if args.print_data_url { data_url } else { None::<String> },
         },
         "logs": logs

src/openhuman/tools/memory_forget.rs

@@ -59,30 +59,18 @@ impl Tool for MemoryForgetTool {
             .security
             .enforce_tool_operation(ToolOperation::Act, "memory_forget")
         {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(error),
-            });
+            return Ok(ToolResult::error(error));
         }
 
         let namespaced_key = format!("{}/{}", namespace.trim(), key);
         match self.memory.forget(&namespaced_key).await {
-            Ok(true) => Ok(ToolResult {
-                success: true,
-                output: format!("Forgot memory: {namespaced_key}"),
-                error: None,
-            }),
-            Ok(false) => Ok(ToolResult {
-                success: true,
-                output: format!("No memory found with key: {key}"),
-                error: None,
-            }),
-            Err(e) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!("Failed to forget memory: {e}")),
-            }),
+            Ok(true) => Ok(ToolResult::success(format!(
+                "Forgot memory: {namespaced_key}"
+            ))),
+            Ok(false) => Ok(ToolResult::success(format!(
+                "No memory found with key: {key}"
+            ))),
+            Err(e) => Ok(ToolResult::error(format!("Failed to forget memory: {e}"))),
         }
     }
 }
@@ -129,8 +117,8 @@ mod tests {
             .execute(json!({"namespace": "global", "key": "temp"}))
             .await
             .unwrap();
-        assert!(result.success);
-        assert!(result.output.contains("Forgot"));
+        assert!(!result.is_error);
+        assert!(result.output().contains("Forgot"));
 
         assert!(mem.get("global/temp").await.unwrap().is_none());
     }
@@ -143,8 +131,8 @@ mod tests {
             .execute(json!({"namespace": "global", "key": "nope"}))
             .await
             .unwrap();
-        assert!(result.success);
-        assert!(result.output.contains("No memory found"));
+        assert!(!result.is_error);
+        assert!(result.output().contains("No memory found"));
     }
 
     #[tokio::test]
@@ -175,12 +163,8 @@ mod tests {
             .execute(json!({"namespace": "global", "key": "temp"}))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result
-            .error
-            .as_deref()
-            .unwrap_or("")
-            .contains("read-only mode"));
+        assert!(result.is_error);
+        assert!(result.output().contains("read-only mode"));
         assert!(mem.get("global/temp").await.unwrap().is_some());
     }
 
@@ -204,12 +188,8 @@ mod tests {
             .execute(json!({"namespace": "global", "key": "temp"}))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result
-            .error
-            .as_deref()
-            .unwrap_or("")
-            .contains("Rate limit exceeded"));
+        assert!(result.is_error);
+        assert!(result.output().contains("Rate limit exceeded"));
         assert!(mem.get("global/temp").await.unwrap().is_some());
     }
 }

src/openhuman/tools/memory_recall.rs

@@ -76,11 +76,9 @@ impl Tool for MemoryRecallTool {
         // `namespace` is still required by the tool contract; unified memory scopes recall to the
         // global document namespace until multi-namespace recall is wired through the `Memory` trait.
         match self.memory.recall(query, limit, None).await {
-            Ok(entries) if entries.is_empty() => Ok(ToolResult {
-                success: true,
-                output: "No memories found matching that query.".into(),
-                error: None,
-            }),
+            Ok(entries) if entries.is_empty() => Ok(ToolResult::success(
+                "No memories found matching that query.",
+            )),
             Ok(entries) => {
                 let mut output = format!("Found {} memories:\n", entries.len());
                 for entry in &entries {
@@ -93,17 +91,9 @@ impl Tool for MemoryRecallTool {
                         entry.category, entry.key, entry.content
                     );
                 }
-                Ok(ToolResult {
-                    success: true,
-                    output,
-                    error: None,
-                })
+                Ok(ToolResult::success(output))
             }
-            Err(e) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!("Memory recall failed: {e}")),
-            }),
+            Err(e) => Ok(ToolResult::error(format!("Memory recall failed: {e}"))),
         }
     }
 }
@@ -128,8 +118,8 @@ mod tests {
             .execute(json!({"namespace": "global", "query": "anything"}))
             .await
             .unwrap();
-        assert!(result.success);
-        assert!(result.output.contains("No memories found"));
+        assert!(!result.is_error);
+        assert!(result.output().contains("No memories found"));
     }
 
     #[tokio::test]
@@ -152,9 +142,9 @@ mod tests {
             .execute(json!({"namespace": "global", "query": "Rust"}))
             .await
             .unwrap();
-        assert!(result.success);
-        assert!(result.output.contains("Rust"));
-        assert!(result.output.contains("Found 1"));
+        assert!(!result.is_error);
+        assert!(result.output().contains("Rust"));
+        assert!(result.output().contains("Found 1"));
     }
 
     #[tokio::test]
@@ -176,8 +166,8 @@ mod tests {
             .execute(json!({"namespace": "global", "query": "Rust", "limit": 3}))
             .await
             .unwrap();
-        assert!(result.success);
-        assert!(result.output.contains("Found 3"));
+        assert!(!result.is_error);
+        assert!(result.output().contains("Found 3"));
     }
 
     #[tokio::test]

src/openhuman/tools/memory_store.rs

@@ -79,11 +79,7 @@ impl Tool for MemoryStoreTool {
             .security
             .enforce_tool_operation(ToolOperation::Act, "memory_store")
         {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(error),
-            });
+            return Ok(ToolResult::error(error));
         }
 
         let namespaced_key = format!("{}/{}", namespace.trim(), key);
@@ -92,16 +88,10 @@ impl Tool for MemoryStoreTool {
             .store(&namespaced_key, content, category, None)
             .await
         {
-            Ok(()) => Ok(ToolResult {
-                success: true,
-                output: format!("Stored memory: {namespaced_key}"),
-                error: None,
-            }),
-            Err(e) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!("Failed to store memory: {e}")),
-            }),
+            Ok(()) => Ok(ToolResult::success(format!(
+                "Stored memory: {namespaced_key}"
+            ))),
+            Err(e) => Ok(ToolResult::error(format!("Failed to store memory: {e}"))),
         }
     }
 }
@@ -141,8 +131,8 @@ mod tests {
             .execute(json!({"namespace": "global", "key": "lang", "content": "Prefers Rust"}))
             .await
             .unwrap();
-        assert!(result.success);
-        assert!(result.output.contains("lang"));
+        assert!(!result.is_error);
+        assert!(result.output().contains("lang"));
 
         let entry = mem.get("global/lang").await.unwrap();
         assert!(entry.is_some());
@@ -159,7 +149,7 @@ mod tests {
             )
             .await
             .unwrap();
-        assert!(result.success);
+        assert!(!result.is_error);
     }
 
     #[tokio::test]
@@ -172,7 +162,7 @@ mod tests {
             )
             .await
             .unwrap();
-        assert!(result.success);
+        assert!(!result.is_error);
 
         let entry = mem.get("global/proj_note").await.unwrap().unwrap();
         assert_eq!(entry.content, "Uses async runtime");
@@ -207,12 +197,8 @@ mod tests {
             .execute(json!({"namespace": "global", "key": "lang", "content": "Prefers Rust"}))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result
-            .error
-            .as_deref()
-            .unwrap_or("")
-            .contains("read-only mode"));
+        assert!(result.is_error);
+        assert!(result.output().contains("read-only mode"));
         assert!(mem.get("global/lang").await.unwrap().is_none());
     }
 
@@ -228,12 +214,8 @@ mod tests {
             .execute(json!({"namespace": "global", "key": "lang", "content": "Prefers Rust"}))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result
-            .error
-            .as_deref()
-            .unwrap_or("")
-            .contains("Rate limit exceeded"));
+        assert!(result.is_error);
+        assert!(result.output().contains("Rate limit exceeded"));
         assert!(mem.get("global/lang").await.unwrap().is_none());
     }
 }

src/openhuman/tools/mod.rs

@@ -34,6 +34,7 @@ pub mod schema;
 mod schemas;
 pub mod screenshot;
 pub mod shell;
+pub mod skill_bridge;
 pub mod spawn_subagent;
 pub mod tool_stats;
 pub mod traits;
@@ -81,9 +82,7 @@ pub use screenshot::ScreenshotTool;
 pub use shell::ShellTool;
 pub use spawn_subagent::SpawnSubagentTool;
 pub use tool_stats::ToolStatsTool;
-pub use traits::{PermissionLevel, Tool};
-#[allow(unused_imports)]
-pub use traits::{ToolResult, ToolSpec};
+pub use traits::{PermissionLevel, Tool, ToolContent, ToolResult, ToolSpec};
 pub use update_memory_md::UpdateMemoryMdTool;
 pub use web_search_tool::WebSearchTool;
 pub use workspace_state::WorkspaceStateTool;

src/openhuman/tools/ops.rs

@@ -360,29 +360,20 @@ mod tests {
 
     #[test]
     fn tool_result_serde() {
-        let result = ToolResult {
-            success: true,
-            output: "hello".into(),
-            error: None,
-        };
+        let result = ToolResult::success("hello");
         let json = serde_json::to_string(&result).unwrap();
         let parsed: ToolResult = serde_json::from_str(&json).unwrap();
-        assert!(parsed.success);
-        assert_eq!(parsed.output, "hello");
-        assert!(parsed.error.is_none());
+        assert!(!parsed.is_error);
+        assert_eq!(parsed.output(), "hello");
     }
 
     #[test]
     fn tool_result_with_error_serde() {
-        let result = ToolResult {
-            success: false,
-            output: String::new(),
-            error: Some("boom".into()),
-        };
+        let result = ToolResult::error("boom");
         let json = serde_json::to_string(&result).unwrap();
         let parsed: ToolResult = serde_json::from_str(&json).unwrap();
-        assert!(!parsed.success);
-        assert_eq!(parsed.error.as_deref(), Some("boom"));
+        assert!(parsed.is_error);
+        assert_eq!(parsed.output(), "boom");
     }
 
     #[test]

src/openhuman/tools/proxy_config.rs

@@ -40,19 +40,11 @@ impl ProxyConfigTool {
 
     fn require_write_access(&self) -> Option<ToolResult> {
         if !self.security.can_act() {
-            return Some(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Action blocked: autonomy is read-only".into()),
-            });
+            return Some(ToolResult::error("Action blocked: autonomy is read-only"));
         }
 
         if !self.security.record_action() {
-            return Some(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Action blocked: rate limit exceeded".into()),
-            });
+            return Some(ToolResult::error("Action blocked: rate limit exceeded"));
         }
 
         None
@@ -141,31 +133,23 @@ impl ProxyConfigTool {
     fn handle_get(&self) -> anyhow::Result<ToolResult> {
         let file_proxy = self.load_config_without_env()?.proxy;
         let runtime_proxy = runtime_proxy_config();
-        Ok(ToolResult {
-            success: true,
-            output: serde_json::to_string_pretty(&json!({
-                "proxy": Self::proxy_json(&file_proxy),
-                "runtime_proxy": Self::proxy_json(&runtime_proxy),
-                "environment": Self::env_snapshot(),
-            }))?,
-            error: None,
-        })
+        Ok(ToolResult::success(serde_json::to_string_pretty(&json!({
+            "proxy": Self::proxy_json(&file_proxy),
+            "runtime_proxy": Self::proxy_json(&runtime_proxy),
+            "environment": Self::env_snapshot(),
+        }))?))
     }
 
     fn handle_list_services(&self) -> anyhow::Result<ToolResult> {
-        Ok(ToolResult {
-            success: true,
-            output: serde_json::to_string_pretty(&json!({
-                "supported_service_keys": ProxyConfig::supported_service_keys(),
-                "supported_selectors": ProxyConfig::supported_service_selectors(),
-                "usage_example": {
-                    "action": "set",
-                    "scope": "services",
-                    "services": ["provider.openai", "tool.http_request", "channel.telegram"]
-                }
-            }))?,
-            error: None,
-        })
+        Ok(ToolResult::success(serde_json::to_string_pretty(&json!({
+            "supported_service_keys": ProxyConfig::supported_service_keys(),
+            "supported_selectors": ProxyConfig::supported_service_selectors(),
+            "usage_example": {
+                "action": "set",
+                "scope": "services",
+                "services": ["provider.openai", "tool.http_request", "channel.telegram"]
+            }
+        }))?))
     }
 
     async fn handle_set(&self, args: &Value) -> anyhow::Result<ToolResult> {
@@ -254,15 +238,11 @@ impl ProxyConfigTool {
             ProxyConfig::clear_process_env();
         }
 
-        Ok(ToolResult {
-            success: true,
-            output: serde_json::to_string_pretty(&json!({
-                "message": "Proxy configuration updated",
-                "proxy": Self::proxy_json(&proxy),
-                "environment": Self::env_snapshot(),
-            }))?,
-            error: None,
-        })
+        Ok(ToolResult::success(serde_json::to_string_pretty(&json!({
+            "message": "Proxy configuration updated",
+            "proxy": Self::proxy_json(&proxy),
+            "environment": Self::env_snapshot(),
+        }))?))
     }
 
     async fn handle_disable(&self, args: &Value) -> anyhow::Result<ToolResult> {
@@ -281,15 +261,11 @@ impl ProxyConfigTool {
             ProxyConfig::clear_process_env();
         }
 
-        Ok(ToolResult {
-            success: true,
-            output: serde_json::to_string_pretty(&json!({
-                "message": "Proxy disabled",
-                "proxy": Self::proxy_json(&cfg.proxy),
-                "environment": Self::env_snapshot(),
-            }))?,
-            error: None,
-        })
+        Ok(ToolResult::success(serde_json::to_string_pretty(&json!({
+            "message": "Proxy disabled",
+            "proxy": Self::proxy_json(&cfg.proxy),
+            "environment": Self::env_snapshot(),
+        }))?))
     }
 
     fn handle_apply_env(&self) -> anyhow::Result<ToolResult> {
@@ -311,27 +287,19 @@ impl ProxyConfigTool {
         proxy.apply_to_process_env();
         set_runtime_proxy_config(proxy.clone());
 
-        Ok(ToolResult {
-            success: true,
-            output: serde_json::to_string_pretty(&json!({
-                "message": "Proxy environment variables applied",
-                "proxy": Self::proxy_json(&proxy),
-                "environment": Self::env_snapshot(),
-            }))?,
-            error: None,
-        })
+        Ok(ToolResult::success(serde_json::to_string_pretty(&json!({
+            "message": "Proxy environment variables applied",
+            "proxy": Self::proxy_json(&proxy),
+            "environment": Self::env_snapshot(),
+        }))?))
     }
 
     fn handle_clear_env(&self) -> anyhow::Result<ToolResult> {
         ProxyConfig::clear_process_env();
-        Ok(ToolResult {
-            success: true,
-            output: serde_json::to_string_pretty(&json!({
-                "message": "Proxy environment variables cleared",
-                "environment": Self::env_snapshot(),
-            }))?,
-            error: None,
-        })
+        Ok(ToolResult::success(serde_json::to_string_pretty(&json!({
+            "message": "Proxy environment variables cleared",
+            "environment": Self::env_snapshot(),
+        }))?))
     }
 }
 
@@ -426,11 +394,7 @@ impl Tool for ProxyConfigTool {
 
         match result {
             Ok(outcome) => Ok(outcome),
-            Err(error) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(error.to_string()),
-            }),
+            Err(error) => Ok(ToolResult::error(error.to_string())),
         }
     }
 }
@@ -468,9 +432,9 @@ mod tests {
             .execute(json!({"action": "list_services"}))
             .await
             .unwrap();
-        assert!(result.success);
-        assert!(result.output.contains("provider.openai"));
-        assert!(result.output.contains("tool.http_request"));
+        assert!(!result.is_error);
+        assert!(result.output().contains("provider.openai"));
+        assert!(result.output().contains("tool.http_request"));
     }
 
     #[tokio::test]
@@ -489,11 +453,8 @@ mod tests {
             .await
             .unwrap();
 
-        assert!(!result.success);
-        assert!(result
-            .error
-            .unwrap_or_default()
-            .contains("proxy.scope='services'"));
+        assert!(result.is_error);
+        assert!(result.output().contains("proxy.scope='services'"));
     }
 
     #[tokio::test]
@@ -510,12 +471,12 @@ mod tests {
             }))
             .await
             .unwrap();
-        assert!(set_result.success, "{:?}", set_result.error);
+        assert!(!set_result.is_error, "{:?}", set_result.output());
 
         let get_result = tool.execute(json!({"action": "get"})).await.unwrap();
-        assert!(get_result.success);
-        assert!(get_result.output.contains("provider.openai"));
-        assert!(get_result.output.contains("services"));
+        assert!(!get_result.is_error);
+        assert!(get_result.output().contains("provider.openai"));
+        assert!(get_result.output().contains("services"));
     }
 
     #[tokio::test]
@@ -530,7 +491,7 @@ mod tests {
             }))
             .await
             .unwrap();
-        assert!(set_result.success, "{:?}", set_result.error);
+        assert!(!set_result.is_error, "{:?}", set_result.output());
 
         let clear_result = tool
             .execute(json!({
@@ -539,11 +500,11 @@ mod tests {
             }))
             .await
             .unwrap();
-        assert!(clear_result.success, "{:?}", clear_result.error);
+        assert!(!clear_result.is_error, "{:?}", clear_result.output());
 
         let get_result = tool.execute(json!({"action": "get"})).await.unwrap();
-        assert!(get_result.success);
-        let parsed: Value = serde_json::from_str(&get_result.output).unwrap();
+        assert!(!get_result.is_error);
+        let parsed: Value = serde_json::from_str(&get_result.output()).unwrap();
         assert!(parsed["proxy"]["http_proxy"].is_null());
         assert!(parsed["runtime_proxy"]["http_proxy"].is_null());
     }

src/openhuman/tools/pushover.rs

@@ -113,19 +113,11 @@ impl Tool for PushoverTool {
 
     async fn execute(&self, args: serde_json::Value) -> anyhow::Result<ToolResult> {
         if !self.security.can_act() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Action blocked: autonomy is read-only".into()),
-            });
+            return Ok(ToolResult::error("Action blocked: autonomy is read-only"));
         }
 
         if !self.security.record_action() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Action blocked: rate limit exceeded".into()),
-            });
+            return Ok(ToolResult::error("Action blocked: rate limit exceeded"));
         }
 
         let message = args
@@ -141,13 +133,9 @@ impl Tool for PushoverTool {
         let priority = match args.get("priority").and_then(|v| v.as_i64()) {
             Some(value) if (-2..=2).contains(&value) => Some(value),
             Some(value) => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(format!(
-                        "Invalid 'priority': {value}. Expected integer in range -2..=2"
-                    )),
-                })
+                return Ok(ToolResult::error(format!(
+                    "Invalid 'priority': {value}. Expected integer in range -2..=2"
+                )))
             }
             None => None,
         };
@@ -184,11 +172,10 @@ impl Tool for PushoverTool {
         let body = response.text().await.unwrap_or_default();
 
         if !status.is_success() {
-            return Ok(ToolResult {
-                success: false,
-                output: body,
-                error: Some(format!("Pushover API returned status {}", status)),
-            });
+            return Ok(ToolResult::error(format!(
+                "Pushover API returned status {}",
+                status
+            )));
         }
 
         let api_status = serde_json::from_str::<serde_json::Value>(&body)
@@ -196,20 +183,14 @@ impl Tool for PushoverTool {
             .and_then(|json| json.get("status").and_then(|value| value.as_i64()));
 
         if api_status == Some(1) {
-            Ok(ToolResult {
-                success: true,
-                output: format!(
-                    "Pushover notification sent successfully. Response: {}",
-                    body
-                ),
-                error: None,
-            })
+            Ok(ToolResult::success(format!(
+                "Pushover notification sent successfully. Response: {}",
+                body
+            )))
         } else {
-            Ok(ToolResult {
-                success: false,
-                output: body,
-                error: Some("Pushover API returned an application-level error".into()),
-            })
+            Ok(ToolResult::error(
+                "Pushover API returned an application-level error",
+            ))
         }
     }
 }
@@ -402,8 +383,8 @@ mod tests {
         );
 
         let result = tool.execute(json!({"message": "hello"})).await.unwrap();
-        assert!(!result.success);
-        assert!(result.error.unwrap().contains("read-only"));
+        assert!(result.is_error);
+        assert!(result.output().contains("read-only"));
     }
 
     #[tokio::test]
@@ -411,8 +392,8 @@ mod tests {
         let tool = PushoverTool::new(test_security(AutonomyLevel::Full, 0), PathBuf::from("/tmp"));
 
         let result = tool.execute(json!({"message": "hello"})).await.unwrap();
-        assert!(!result.success);
-        assert!(result.error.unwrap().contains("rate limit"));
+        assert!(result.is_error);
+        assert!(result.output().contains("rate limit"));
     }
 
     #[tokio::test]
@@ -427,7 +408,7 @@ mod tests {
             .await
             .unwrap();
 
-        assert!(!result.success);
-        assert!(result.error.unwrap().contains("-2..=2"));
+        assert!(result.is_error);
+        assert!(result.output().contains("-2..=2"));
     }
 }

src/openhuman/tools/read_diff.rs

@@ -91,26 +91,14 @@ impl Tool for ReadDiffTool {
             let diff = String::from_utf8_lossy(&output.stdout);
             tracing::debug!("[read_diff] success, diff length={}", diff.len());
             if diff.trim().is_empty() {
-                Ok(ToolResult {
-                    success: true,
-                    output: "No changes found.".into(),
-                    error: None,
-                })
+                Ok(ToolResult::success("No changes found."))
             } else {
-                Ok(ToolResult {
-                    success: true,
-                    output: diff.to_string(),
-                    error: None,
-                })
+                Ok(ToolResult::success(diff.to_string()))
             }
         } else {
             let stderr = String::from_utf8_lossy(&output.stderr);
             tracing::debug!("[read_diff] failed: {stderr}");
-            Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(stderr.to_string()),
-            })
+            Ok(ToolResult::error(stderr.to_string()))
         }
     }
 }

src/openhuman/tools/run_linter.rs

@@ -61,11 +61,9 @@ impl Tool for RunLinterTool {
             } else if self.workspace_dir.join("package.json").exists() {
                 "eslint"
             } else {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some("Could not detect project type for linting.".into()),
-                });
+                return Ok(ToolResult::error(
+                    "Could not detect project type for linting.",
+                ));
             }
         } else {
             linter
@@ -88,15 +86,10 @@ impl Tool for RunLinterTool {
             "eslint" => {
                 let path = args.get("path").and_then(|v| v.as_str()).unwrap_or(".");
                 if path.starts_with('/') || path.contains("..") {
-                    return Ok(ToolResult {
-                        success: false,
-                        output: String::new(),
-                        error: Some(
-                            "path must be a relative path within the workspace \
-                             (no absolute paths or '..')"
-                                .into(),
-                        ),
-                    });
+                    return Ok(ToolResult::error(
+                        "path must be a relative path within the workspace \
+                             (no absolute paths or '..')",
+                    ));
                 }
                 tokio::process::Command::new("npx")
                     .args(["eslint", "--format", "compact", path])
@@ -105,11 +98,7 @@ impl Tool for RunLinterTool {
                     .await?
             }
             other => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(format!("Unknown linter: {other}")),
-                });
+                return Ok(ToolResult::error(format!("Unknown linter: {other}")));
             }
         };
 
@@ -122,17 +111,14 @@ impl Tool for RunLinterTool {
             format!("{stdout}\n{stderr}")
         };
 
-        Ok(ToolResult {
-            success: output.status.success(),
-            output: combined,
-            error: if output.status.success() {
-                None
-            } else {
-                Some(format!(
-                    "Linter exited with code {:?}",
-                    output.status.code()
-                ))
-            },
-        })
+        if output.status.success() {
+            Ok(ToolResult::success(combined))
+        } else {
+            Ok(ToolResult::error(format!(
+                "Linter exited with code {:?}\n{}",
+                output.status.code(),
+                combined
+            )))
+        }
     }
 }

src/openhuman/tools/run_tests.rs

@@ -71,11 +71,9 @@ impl Tool for RunTestsTool {
             } else if self.workspace_dir.join("package.json").exists() {
                 "vitest"
             } else {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some("Could not detect project type for testing.".into()),
-                });
+                return Ok(ToolResult::error(
+                    "Could not detect project type for testing.",
+                ));
             }
         } else {
             runner
@@ -99,11 +97,7 @@ impl Tool for RunTestsTool {
                 c
             }
             other => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(format!("Unknown test runner: {other}")),
-                });
+                return Ok(ToolResult::error(format!("Unknown test runner: {other}")));
             }
         };
 
@@ -118,18 +112,14 @@ impl Tool for RunTestsTool {
             {
                 Ok(Ok(output)) => output,
                 Ok(Err(e)) => {
-                    return Ok(ToolResult {
-                        success: false,
-                        output: String::new(),
-                        error: Some(format!("failed to spawn test runner: {e}")),
-                    });
+                    return Ok(ToolResult::error(format!(
+                        "failed to spawn test runner: {e}"
+                    )));
                 }
                 Err(_) => {
-                    return Ok(ToolResult {
-                        success: false,
-                        output: String::new(),
-                        error: Some(format!("test execution timed out after {timeout_secs}s")),
-                    });
+                    return Ok(ToolResult::error(format!(
+                        "test execution timed out after {timeout_secs}s"
+                    )));
                 }
             };
 
@@ -160,14 +150,14 @@ impl Tool for RunTestsTool {
             truncated.len()
         );
 
-        Ok(ToolResult {
-            success: output.status.success(),
-            output: truncated,
-            error: if output.status.success() {
-                None
-            } else {
-                Some(format!("Tests exited with code {:?}", output.status.code()))
-            },
-        })
+        if output.status.success() {
+            Ok(ToolResult::success(truncated))
+        } else {
+            Ok(ToolResult::error(format!(
+                "Tests exited with code {:?}\n\n{}",
+                output.status.code(),
+                truncated
+            )))
+        }
     }
 }

src/openhuman/tools/schedule.rs

@@ -115,13 +115,9 @@ impl Tool for ScheduleTool {
                     .ok_or_else(|| anyhow::anyhow!("Missing 'id' parameter for resume action"))?;
                 Ok(self.handle_pause_resume(id, false))
             }
-            other => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!(
+            other => Ok(ToolResult::error(format!(
                     "Unknown action '{other}'. Use create/add/once/list/get/cancel/remove/pause/resume."
-                )),
-            }),
+                ))),
         }
     }
 }
@@ -129,21 +125,15 @@ impl Tool for ScheduleTool {
 impl ScheduleTool {
     fn enforce_mutation_allowed(&self, action: &str) -> Option<ToolResult> {
         if !self.security.can_act() {
-            return Some(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!(
-                    "Security policy: read-only mode, cannot perform '{action}'"
-                )),
-            });
+            return Some(ToolResult::error(format!(
+                "Security policy: read-only mode, cannot perform '{action}'"
+            )));
         }
 
         if !self.security.record_action() {
-            return Some(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Rate limit exceeded: action budget exhausted".to_string()),
-            });
+            return Some(ToolResult::error(
+                "Rate limit exceeded: action budget exhausted".to_string(),
+            ));
         }
 
         None
@@ -152,11 +142,7 @@ impl ScheduleTool {
     fn handle_list(&self) -> Result<ToolResult> {
         let jobs = cron::list_jobs(&self.config)?;
         if jobs.is_empty() {
-            return Ok(ToolResult {
-                success: true,
-                output: "No scheduled jobs.".to_string(),
-                error: None,
-            });
+            return Ok(ToolResult::success("No scheduled jobs.".to_string()));
         }
 
         let mut lines = Vec::with_capacity(jobs.len());
@@ -185,11 +171,11 @@ impl ScheduleTool {
             ));
         }
 
-        Ok(ToolResult {
-            success: true,
-            output: format!("Scheduled jobs ({}):\n{}", lines.len(), lines.join("\n")),
-            error: None,
-        })
+        Ok(ToolResult::success(format!(
+            "Scheduled jobs ({}):\n{}",
+            lines.len(),
+            lines.join("\n")
+        )))
     }
 
     fn handle_get(&self, id: &str) -> Result<ToolResult> {
@@ -205,17 +191,9 @@ impl ScheduleTool {
                     "enabled": job.enabled,
                     "one_shot": matches!(job.schedule, cron::Schedule::At { .. }),
                 });
-                Ok(ToolResult {
-                    success: true,
-                    output: serde_json::to_string_pretty(&detail)?,
-                    error: None,
-                })
+                Ok(ToolResult::success(serde_json::to_string_pretty(&detail)?))
             }
-            Err(_) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!("Job '{id}' not found")),
-            }),
+            Err(_) => Ok(ToolResult::error(format!("Job '{id}' not found"))),
         }
     }
 
@@ -233,27 +211,21 @@ impl ScheduleTool {
         match action {
             "add" => {
                 if expression.is_none() || delay.is_some() || run_at.is_some() {
-                    return Ok(ToolResult {
-                        success: false,
-                        output: String::new(),
-                        error: Some("'add' requires 'expression' and forbids delay/run_at".into()),
-                    });
+                    return Ok(ToolResult::error(
+                        "'add' requires 'expression' and forbids delay/run_at",
+                    ));
                 }
             }
             "once" => {
                 if expression.is_some() || (delay.is_none() && run_at.is_none()) {
-                    return Ok(ToolResult {
-                        success: false,
-                        output: String::new(),
-                        error: Some("'once' requires exactly one of 'delay' or 'run_at'".into()),
-                    });
+                    return Ok(ToolResult::error(
+                        "'once' requires exactly one of 'delay' or 'run_at'",
+                    ));
                 }
                 if delay.is_some() && run_at.is_some() {
-                    return Ok(ToolResult {
-                        success: false,
-                        output: String::new(),
-                        error: Some("'once' supports either delay or run_at, not both".into()),
-                    });
+                    return Ok(ToolResult::error(
+                        "'once' supports either delay or run_at, not both",
+                    ));
                 }
             }
             _ => {
@@ -262,45 +234,32 @@ impl ScheduleTool {
                     .filter(|value| *value)
                     .count();
                 if count != 1 {
-                    return Ok(ToolResult {
-                        success: false,
-                        output: String::new(),
-                        error: Some(
-                            "Exactly one of 'expression', 'delay', or 'run_at' must be provided"
-                                .into(),
-                        ),
-                    });
+                    return Ok(ToolResult::error(
+                        "Exactly one of 'expression', 'delay', or 'run_at' must be provided",
+                    ));
                 }
             }
         }
 
         if let Some(value) = expression {
             let job = cron::add_job(&self.config, value, command)?;
-            return Ok(ToolResult {
-                success: true,
-                output: format!(
-                    "Created recurring job {} (expr: {}, next: {}, cmd: {})",
-                    job.id,
-                    job.expression,
-                    job.next_run.to_rfc3339(),
-                    job.command
-                ),
-                error: None,
-            });
+            return Ok(ToolResult::success(format!(
+                "Created recurring job {} (expr: {}, next: {}, cmd: {})",
+                job.id,
+                job.expression,
+                job.next_run.to_rfc3339(),
+                job.command
+            )));
         }
 
         if let Some(value) = delay {
             let job = cron::add_once(&self.config, value, command)?;
-            return Ok(ToolResult {
-                success: true,
-                output: format!(
-                    "Created one-shot job {} (runs at: {}, cmd: {})",
-                    job.id,
-                    job.next_run.to_rfc3339(),
-                    job.command
-                ),
-                error: None,
-            });
+            return Ok(ToolResult::success(format!(
+                "Created one-shot job {} (runs at: {}, cmd: {})",
+                job.id,
+                job.next_run.to_rfc3339(),
+                job.command
+            )));
         }
 
         let run_at_raw = run_at.ok_or_else(|| anyhow::anyhow!("Missing scheduling parameters"))?;
@@ -309,30 +268,18 @@ impl ScheduleTool {
             .with_timezone(&Utc);
 
         let job = cron::add_once_at(&self.config, run_at_parsed, command)?;
-        Ok(ToolResult {
-            success: true,
-            output: format!(
-                "Created one-shot job {} (runs at: {}, cmd: {})",
-                job.id,
-                job.next_run.to_rfc3339(),
-                job.command
-            ),
-            error: None,
-        })
+        Ok(ToolResult::success(format!(
+            "Created one-shot job {} (runs at: {}, cmd: {})",
+            job.id,
+            job.next_run.to_rfc3339(),
+            job.command
+        )))
     }
 
     fn handle_cancel(&self, id: &str) -> ToolResult {
         match cron::remove_job(&self.config, id) {
-            Ok(()) => ToolResult {
-                success: true,
-                output: format!("Cancelled job {id}"),
-                error: None,
-            },
-            Err(error) => ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(error.to_string()),
-            },
+            Ok(()) => ToolResult::success(format!("Cancelled job {id}")),
+            Err(error) => ToolResult::error(error.to_string()),
         }
     }
 
@@ -344,20 +291,12 @@ impl ScheduleTool {
         };
 
         match operation {
-            Ok(_) => ToolResult {
-                success: true,
-                output: if pause {
-                    format!("Paused job {id}")
-                } else {
-                    format!("Resumed job {id}")
-                },
-                error: None,
-            },
-            Err(error) => ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(error.to_string()),
-            },
+            Ok(_) => ToolResult::success(if pause {
+                format!("Paused job {id}")
+            } else {
+                format!("Resumed job {id}")
+            }),
+            Err(error) => ToolResult::error(error.to_string()),
         }
     }
 }
@@ -400,8 +339,8 @@ mod tests {
         let tool = ScheduleTool::new(security, config);
 
         let result = tool.execute(json!({"action": "list"})).await.unwrap();
-        assert!(result.success);
-        assert!(result.output.contains("No scheduled jobs"));
+        assert!(!result.is_error);
+        assert!(result.output().contains("No scheduled jobs"));
     }
 
     #[tokio::test]
@@ -417,27 +356,28 @@ mod tests {
             }))
             .await
             .unwrap();
-        assert!(create.success);
-        assert!(create.output.contains("Created recurring job"));
+        assert!(!create.is_error);
+        assert!(create.output().contains("Created recurring job"));
 
         let list = tool.execute(json!({"action": "list"})).await.unwrap();
-        assert!(list.success);
-        assert!(list.output.contains("echo hello"));
+        assert!(!list.is_error);
+        assert!(list.output().contains("echo hello"));
 
-        let id = create.output.split_whitespace().nth(3).unwrap();
+        let create_output = create.output();
+        let id = create_output.split_whitespace().nth(3).unwrap();
 
         let get = tool
             .execute(json!({"action": "get", "id": id}))
             .await
             .unwrap();
-        assert!(get.success);
-        assert!(get.output.contains("echo hello"));
+        assert!(!get.is_error);
+        assert!(get.output().contains("echo hello"));
 
         let cancel = tool
             .execute(json!({"action": "cancel", "id": id}))
             .await
             .unwrap();
-        assert!(cancel.success);
+        assert!(!cancel.is_error);
     }
 
     #[tokio::test]
@@ -453,7 +393,7 @@ mod tests {
             }))
             .await
             .unwrap();
-        assert!(once.success);
+        assert!(!once.is_error);
 
         let add = tool
             .execute(json!({
@@ -463,20 +403,21 @@ mod tests {
             }))
             .await
             .unwrap();
-        assert!(add.success);
+        assert!(!add.is_error);
 
-        let id = add.output.split_whitespace().nth(3).unwrap();
+        let add_output = add.output();
+        let id = add_output.split_whitespace().nth(3).unwrap();
         let pause = tool
             .execute(json!({"action": "pause", "id": id}))
             .await
             .unwrap();
-        assert!(pause.success);
+        assert!(!pause.is_error);
 
         let resume = tool
             .execute(json!({"action": "resume", "id": id}))
             .await
             .unwrap();
-        assert!(resume.success);
+        assert!(!resume.is_error);
     }
 
     #[tokio::test]
@@ -509,11 +450,11 @@ mod tests {
             }))
             .await
             .unwrap();
-        assert!(!blocked.success);
-        assert!(blocked.error.as_deref().unwrap().contains("read-only"));
+        assert!(blocked.is_error);
+        assert!(blocked.output().contains("read-only"));
 
         let list = tool.execute(json!({"action": "list"})).await.unwrap();
-        assert!(list.success);
+        assert!(!list.is_error);
     }
 
     #[tokio::test]
@@ -522,7 +463,7 @@ mod tests {
         let tool = ScheduleTool::new(security, config);
 
         let result = tool.execute(json!({"action": "explode"})).await.unwrap();
-        assert!(!result.success);
-        assert!(result.error.as_deref().unwrap().contains("Unknown action"));
+        assert!(result.is_error);
+        assert!(result.output().contains("Unknown action"));
     }
 }

src/openhuman/tools/screenshot.rs

@@ -73,22 +73,18 @@ impl ScreenshotTool {
             '\'', '"', '`', '$', '\\', ';', '|', '&', '\n', '\0', '(', ')',
         ];
         if safe_name.contains(SHELL_UNSAFE) {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Filename contains characters unsafe for shell execution".into()),
-            });
+            return Ok(ToolResult::error(
+                "Filename contains characters unsafe for shell execution",
+            ));
         }
 
         let output_path = self.security.workspace_dir.join(&safe_name);
         let output_str = output_path.to_string_lossy().to_string();
 
         let Some(mut cmd_args) = Self::screenshot_command(&output_str) else {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Screenshot not supported on this platform".into()),
-            });
+            return Ok(ToolResult::error(
+                "Screenshot not supported on this platform",
+            ));
         };
 
         // macOS region flags
@@ -116,36 +112,23 @@ impl ScreenshotTool {
                 if !output.status.success() {
                     let stderr = String::from_utf8_lossy(&output.stderr);
                     if stderr.contains("NO_SCREENSHOT_TOOL") {
-                        return Ok(ToolResult {
-                            success: false,
-                            output: String::new(),
-                            error: Some(
-                                "No screenshot tool found. Install gnome-screenshot, scrot, or ImageMagick."
-                                    .into(),
-                            ),
-                        });
+                        return Ok(ToolResult::error(
+                                "No screenshot tool found. Install gnome-screenshot, scrot, or ImageMagick.",
+                            ));
                     }
-                    return Ok(ToolResult {
-                        success: false,
-                        output: String::new(),
-                        error: Some(format!("Screenshot command failed: {stderr}")),
-                    });
+                    return Ok(ToolResult::error(format!(
+                        "Screenshot command failed: {stderr}"
+                    )));
                 }
 
                 Self::read_and_encode(&output_path).await
             }
-            Ok(Err(e)) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!("Failed to execute screenshot command: {e}")),
-            }),
-            Err(_) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!(
-                    "Screenshot timed out after {SCREENSHOT_TIMEOUT_SECS}s"
-                )),
-            }),
+            Ok(Err(e)) => Ok(ToolResult::error(format!(
+                "Failed to execute screenshot command: {e}"
+            ))),
+            Err(_) => Ok(ToolResult::error(format!(
+                "Screenshot timed out after {SCREENSHOT_TIMEOUT_SECS}s"
+            ))),
         }
     }
 
@@ -155,15 +138,11 @@ impl ScreenshotTool {
         const MAX_RAW_BYTES: u64 = 1_572_864; // ~1.5 MB (base64 expands ~33%)
         if let Ok(meta) = tokio::fs::metadata(output_path).await {
             if meta.len() > MAX_RAW_BYTES {
-                return Ok(ToolResult {
-                    success: true,
-                    output: format!(
-                        "Screenshot saved to: {}\nSize: {} bytes (too large to base64-encode inline)",
-                        output_path.display(),
-                        meta.len(),
-                    ),
-                    error: None,
-                });
+                return Ok(ToolResult::success(format!(
+                    "Screenshot saved to: {}\nSize: {} bytes (too large to base64-encode inline)",
+                    output_path.display(),
+                    meta.len(),
+                )));
             }
         }
 
@@ -196,17 +175,11 @@ impl ScreenshotTool {
                 };
                 let _ = write!(output_msg, "\ndata:{mime};base64,{encoded}");
 
-                Ok(ToolResult {
-                    success: true,
-                    output: output_msg,
-                    error: None,
-                })
+                Ok(ToolResult::success(output_msg))
             }
-            Err(e) => Ok(ToolResult {
-                success: false,
-                output: format!("Screenshot saved to: {}", output_path.display()),
-                error: Some(format!("Failed to read screenshot file: {e}")),
-            }),
+            Err(e) => Ok(ToolResult::error(format!(
+                "Failed to read screenshot file: {e}"
+            ))),
         }
     }
 }
@@ -239,11 +212,7 @@ impl Tool for ScreenshotTool {
 
     async fn execute(&self, args: serde_json::Value) -> anyhow::Result<ToolResult> {
         if !self.security.can_act() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Action blocked: autonomy is read-only".into()),
-            });
+            return Ok(ToolResult::error("Action blocked: autonomy is read-only"));
         }
         self.capture(args).await
     }
@@ -309,8 +278,8 @@ mod tests {
             .execute(json!({"filename": "test'injection.png"}))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result.error.unwrap().contains("unsafe for shell execution"));
+        assert!(result.is_error);
+        assert!(result.output().contains("unsafe for shell execution"));
     }
 
     #[test]

src/openhuman/tools/shell.rs

@@ -67,30 +67,22 @@ impl Tool for ShellTool {
             .unwrap_or(false);
 
         if self.security.is_rate_limited() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Rate limit exceeded: too many actions in the last hour".into()),
-            });
+            return Ok(ToolResult::error(
+                "Rate limit exceeded: too many actions in the last hour",
+            ));
         }
 
         match self.security.validate_command_execution(command, approved) {
             Ok(_) => {}
             Err(reason) => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(reason),
-                });
+                return Ok(ToolResult::error(reason));
             }
         }
 
         if !self.security.record_action() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("Rate limit exceeded: action budget exhausted".into()),
-            });
+            return Ok(ToolResult::error(
+                "Rate limit exceeded: action budget exhausted",
+            ));
         }
 
         // Execute with timeout to prevent hanging commands.
@@ -102,11 +94,9 @@ impl Tool for ShellTool {
         {
             Ok(cmd) => cmd,
             Err(e) => {
-                return Ok(ToolResult {
-                    success: false,
-                    output: String::new(),
-                    error: Some(format!("Failed to build runtime command: {e}")),
-                });
+                return Ok(ToolResult::error(format!(
+                    "Failed to build runtime command: {e}"
+                )));
             }
         };
         cmd.env_clear();
@@ -135,28 +125,22 @@ impl Tool for ShellTool {
                     stderr.push_str("\n... [stderr truncated at 1MB]");
                 }
 
-                Ok(ToolResult {
-                    success: output.status.success(),
-                    output: stdout,
-                    error: if stderr.is_empty() {
-                        None
+                if output.status.success() {
+                    if stderr.is_empty() {
+                        Ok(ToolResult::success(stdout))
                     } else {
-                        Some(stderr)
-                    },
-                })
+                        // Successful exit but stderr present — attach stderr as output suffix
+                        Ok(ToolResult::success(format!("{stdout}\n[stderr]\n{stderr}")))
+                    }
+                } else {
+                    let err_msg = if stderr.is_empty() { stdout } else { stderr };
+                    Ok(ToolResult::error(err_msg))
+                }
             }
-            Ok(Err(e)) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!("Failed to execute command: {e}")),
-            }),
-            Err(_) => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!(
-                    "Command timed out after {SHELL_TIMEOUT_SECS}s and was killed"
-                )),
-            }),
+            Ok(Err(e)) => Ok(ToolResult::error(format!("Failed to execute command: {e}"))),
+            Err(_) => Ok(ToolResult::error(format!(
+                "Command timed out after {SHELL_TIMEOUT_SECS}s and was killed"
+            ))),
         }
     }
 }
@@ -210,17 +194,17 @@ mod tests {
             .execute(json!({"command": "echo hello"}))
             .await
             .unwrap();
-        assert!(result.success);
-        assert!(result.output.trim().contains("hello"));
-        assert!(result.error.is_none());
+        assert!(!result.is_error);
+        assert!(result.output().trim().contains("hello"));
+        assert!(!result.is_error);
     }
 
     #[tokio::test]
     async fn shell_blocks_disallowed_command() {
         let tool = ShellTool::new(test_security(AutonomyLevel::Supervised), test_runtime());
         let result = tool.execute(json!({"command": "rm -rf /"})).await.unwrap();
-        assert!(!result.success);
-        let error = result.error.as_deref().unwrap_or("");
+        assert!(result.is_error);
+        let error = result.output();
         assert!(error.contains("not allowed") || error.contains("high-risk"));
     }
 
@@ -228,8 +212,8 @@ mod tests {
     async fn shell_blocks_readonly() {
         let tool = ShellTool::new(test_security(AutonomyLevel::ReadOnly), test_runtime());
         let result = tool.execute(json!({"command": "ls"})).await.unwrap();
-        assert!(!result.success);
-        assert!(result.error.as_ref().unwrap().contains("not allowed"));
+        assert!(result.is_error);
+        assert!(&result.output().contains("not allowed"));
     }
 
     #[tokio::test]
@@ -254,7 +238,7 @@ mod tests {
             .execute(json!({"command": "ls /nonexistent_dir_xyz"}))
             .await
             .unwrap();
-        assert!(!result.success);
+        assert!(result.is_error);
     }
 
     fn test_security_with_env_cmd() -> Arc<SecurityPolicy> {
@@ -297,13 +281,13 @@ mod tests {
 
         let tool = ShellTool::new(test_security_with_env_cmd(), test_runtime());
         let result = tool.execute(json!({"command": "env"})).await.unwrap();
-        assert!(result.success);
+        assert!(!result.is_error);
         assert!(
-            !result.output.contains("sk-test-secret-12345"),
+            !result.output().contains("sk-test-secret-12345"),
             "API_KEY leaked to shell command output"
         );
         assert!(
-            !result.output.contains("sk-test-secret-67890"),
+            !result.output().contains("sk-test-secret-67890"),
             "OPENHUMAN_API_KEY leaked to shell command output"
         );
     }
@@ -316,9 +300,9 @@ mod tests {
             .execute(json!({"command": "echo $HOME"}))
             .await
             .unwrap();
-        assert!(result.success);
+        assert!(!result.is_error);
         assert!(
-            !result.output.trim().is_empty(),
+            !result.output().trim().is_empty(),
             "HOME should be available in shell"
         );
 
@@ -326,9 +310,9 @@ mod tests {
             .execute(json!({"command": "echo $PATH"}))
             .await
             .unwrap();
-        assert!(result.success);
+        assert!(!result.is_error);
         assert!(
-            !result.output.trim().is_empty(),
+            !result.output().trim().is_empty(),
             "PATH should be available in shell"
         );
     }
@@ -347,12 +331,8 @@ mod tests {
             .execute(json!({"command": "touch openhuman_shell_approval_test"}))
             .await
             .unwrap();
-        assert!(!denied.success);
-        assert!(denied
-            .error
-            .as_deref()
-            .unwrap_or("")
-            .contains("explicit approval"));
+        assert!(denied.is_error);
+        assert!(denied.output().contains("explicit approval"));
 
         let allowed = tool
             .execute(json!({
@@ -361,7 +341,7 @@ mod tests {
             }))
             .await
             .unwrap();
-        assert!(allowed.success);
+        assert!(!allowed.is_error);
 
         let _ = tokio::fs::remove_file(std::env::temp_dir().join("openhuman_shell_approval_test"))
             .await;
@@ -421,7 +401,7 @@ mod tests {
         });
         let tool = ShellTool::new(security, test_runtime());
         let result = tool.execute(json!({"command": "echo test"})).await.unwrap();
-        assert!(!result.success);
-        assert!(result.error.as_deref().unwrap_or("").contains("Rate limit"));
+        assert!(result.is_error);
+        assert!(result.output().contains("Rate limit"));
     }
 }

src/openhuman/tools/skill_bridge.rs

@@ -0,0 +1,193 @@
+//! Bridge between the QuickJS skill runtime and the agent's `Tool` trait registry.
+//!
+//! Each running skill exposes tools via `ToolDefinition`. This module wraps them
+//! as `Tool` trait implementations so the agent loop can discover and execute
+//! skill tools (Notion, Gmail, etc.) alongside built-in tools.
+//!
+//! Both built-in tools and skill tools now use the same unified `ToolResult`
+//! type (MCP content blocks), so no result conversion is needed.
+
+use async_trait::async_trait;
+use std::collections::HashSet;
+use std::sync::Arc;
+
+use crate::openhuman::skills::qjs_engine::RuntimeEngine;
+use crate::openhuman::skills::types::ToolDefinition;
+use crate::openhuman::tools::traits::{PermissionLevel, Tool, ToolResult};
+
+/// A `Tool` implementation that delegates execution to a running QuickJS skill instance.
+///
+/// The tool name uses the convention `{skill_id}__{tool_name}` so the agent loop
+/// can match tool calls from the LLM to the correct skill and tool.
+pub struct SkillToolBridge {
+    /// Namespaced tool name: `{skill_id}__{tool_name}`.
+    namespaced_name: String,
+    /// Skill identifier (e.g. "notion", "gmail").
+    skill_id: String,
+    /// Original tool name within the skill (e.g. "search-blocks", "send-email").
+    tool_name: String,
+    /// Human-readable description from the skill manifest.
+    description: String,
+    /// JSON Schema for the tool's input parameters.
+    input_schema: serde_json::Value,
+    /// Reference to the runtime engine for executing tool calls.
+    engine: Arc<RuntimeEngine>,
+}
+
+impl SkillToolBridge {
+    fn new(skill_id: String, tool_def: &ToolDefinition, engine: Arc<RuntimeEngine>) -> Self {
+        let namespaced_name = format!("{}__{}", skill_id, tool_def.name);
+        Self {
+            namespaced_name,
+            skill_id,
+            tool_name: tool_def.name.clone(),
+            description: tool_def.description.clone(),
+            input_schema: tool_def.input_schema.clone(),
+            engine,
+        }
+    }
+}
+
+#[async_trait]
+impl Tool for SkillToolBridge {
+    fn name(&self) -> &str {
+        &self.namespaced_name
+    }
+
+    fn description(&self) -> &str {
+        &self.description
+    }
+
+    fn parameters_schema(&self) -> serde_json::Value {
+        if self.input_schema.is_null() || self.input_schema == serde_json::json!({}) {
+            serde_json::json!({ "type": "object", "properties": {} })
+        } else {
+            self.input_schema.clone()
+        }
+    }
+
+    fn permission_level(&self) -> PermissionLevel {
+        // Skill tools interact with external services; treat as write-level.
+        PermissionLevel::Write
+    }
+
+    async fn execute(&self, args: serde_json::Value) -> anyhow::Result<ToolResult> {
+        log::debug!(
+            "[skill-bridge] Executing {}.{} (namespaced: {})",
+            self.skill_id,
+            self.tool_name,
+            self.namespaced_name,
+        );
+
+        // Both the skill runtime and the Tool trait now use the same ToolResult type,
+        // so we just forward the result directly — no conversion needed.
+        match self
+            .engine
+            .call_tool(&self.skill_id, &self.tool_name, args)
+            .await
+        {
+            Ok(result) => {
+                if result.is_error {
+                    log::warn!(
+                        "[skill-bridge] {}.{} returned error: {}",
+                        self.skill_id,
+                        self.tool_name,
+                        result.output()
+                    );
+                } else {
+                    log::debug!(
+                        "[skill-bridge] {}.{} succeeded ({} bytes)",
+                        self.skill_id,
+                        self.tool_name,
+                        result.output().len()
+                    );
+                }
+                Ok(result)
+            }
+            Err(err) => {
+                log::error!(
+                    "[skill-bridge] {}.{} execution failed: {}",
+                    self.skill_id,
+                    self.tool_name,
+                    err
+                );
+                Ok(ToolResult::error(format!(
+                    "Skill tool execution failed: {}__{}",
+                    self.skill_id, self.tool_name
+                )))
+            }
+        }
+    }
+}
+
+/// Collect all tools from running skills and wrap them as `Box<dyn Tool>`.
+///
+/// Returns an empty vec if the runtime engine is not initialized (e.g. CLI mode
+/// without the desktop app running).
+pub fn collect_skill_tools() -> Vec<Box<dyn Tool>> {
+    let engine = match crate::openhuman::skills::global_engine() {
+        Some(e) => e,
+        None => {
+            log::debug!("[skill-bridge] No global engine — skipping skill tools");
+            return Vec::new();
+        }
+    };
+
+    let all = engine.all_tools();
+    log::info!(
+        "[skill-bridge] Discovered {} tool(s) from running skills",
+        all.len()
+    );
+
+    let mut seen = HashSet::new();
+    all.into_iter()
+        .filter_map(|(skill_id, tool_def)| {
+            if skill_id.contains("__") || tool_def.name.contains("__") {
+                log::error!(
+                    "[skill-bridge] Skipping tool with reserved delimiter in name: {} / {}",
+                    skill_id,
+                    tool_def.name
+                );
+                return None;
+            }
+            let namespaced = format!("{}__{}", skill_id, tool_def.name);
+            if !seen.insert(namespaced.clone()) {
+                log::error!(
+                    "[skill-bridge] Skipping duplicate namespaced tool: {}",
+                    namespaced
+                );
+                return None;
+            }
+            log::debug!(
+                "[skill-bridge]   + {} — {}",
+                namespaced,
+                tool_def.description.chars().take(60).collect::<String>()
+            );
+            Some(Box::new(SkillToolBridge::new(
+                skill_id,
+                &tool_def,
+                Arc::clone(&engine),
+            )) as Box<dyn Tool>)
+        })
+        .collect()
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn namespaced_name_format() {
+        let skill_id = "notion";
+        let tool_name = "search-blocks";
+        let expected = format!("{}__{}", skill_id, tool_name);
+        assert_eq!(expected, "notion__search-blocks");
+    }
+
+    #[test]
+    fn collect_returns_empty_without_engine() {
+        // Global engine is not set in test context, so should return empty.
+        let tools = collect_skill_tools();
+        assert!(tools.is_empty());
+    }
+}

src/openhuman/tools/spawn_subagent.rs

@@ -66,11 +66,7 @@ impl Tool for SpawnSubagentTool {
         let context = args.get("context").and_then(|v| v.as_str()).unwrap_or("");
 
         if prompt.is_empty() {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some("prompt is required".into()),
-            });
+            return Ok(ToolResult::error("prompt is required"));
         }
 
         // Parse archetype (validation).
@@ -85,15 +81,8 @@ impl Tool for SpawnSubagentTool {
             prompt.len()
         );
 
-        Ok(ToolResult {
-            success: false,
-            output: format!(
-                "[Sub-agent {archetype_str}] Task received. Prompt: {prompt}\n\
-                 Context length: {} chars\n\
-                 (Full sub-agent execution will be wired in next phase)",
-                context.len()
-            ),
-            error: Some("spawn_subagent not yet wired to sub-agent execution".into()),
-        })
+        Ok(ToolResult::error(
+            "spawn_subagent not yet wired to sub-agent execution",
+        ))
     }
 }

src/openhuman/tools/tool_stats.rs

@@ -64,11 +64,9 @@ impl Tool for ToolStatsTool {
 
         if entries.is_empty() {
             log::debug!("[tool_stats] no entries, returning early");
-            return Ok(ToolResult {
-                success: true,
-                output: "No tool effectiveness data recorded yet.".into(),
-                error: None,
-            });
+            return Ok(ToolResult::success(
+                "No tool effectiveness data recorded yet.",
+            ));
         }
 
         let mut output = String::from("## Tool Effectiveness Stats\n\n");
@@ -120,18 +118,12 @@ impl Tool for ToolStatsTool {
         if !found {
             if let Some(name) = filter {
                 log::debug!("[tool_stats] filter '{name}' matched no entries");
-                return Ok(ToolResult {
-                    success: true,
-                    output: format!("No effectiveness data recorded for tool '{name}'."),
-                    error: None,
-                });
+                return Ok(ToolResult::success(format!(
+                    "No effectiveness data recorded for tool '{name}'."
+                )));
             }
         }
 
-        Ok(ToolResult {
-            success: true,
-            output,
-            error: None,
-        })
+        Ok(ToolResult::success(output))
     }
 }

src/openhuman/tools/traits.rs

@@ -1,6 +1,9 @@
 use async_trait::async_trait;
 use serde::{Deserialize, Serialize};
 
+// Re-export the unified ToolResult from the skills module so all tools use one type.
+pub use crate::openhuman::skills::types::{ToolContent, ToolResult};
+
 /// Permission level required to execute a tool.
 ///
 /// Channels can set a maximum permission level to restrict which tools
@@ -33,14 +36,6 @@ impl std::fmt::Display for PermissionLevel {
     }
 }
 
-/// Result of a tool execution
-#[derive(Debug, Clone, Serialize, Deserialize)]
-pub struct ToolResult {
-    pub success: bool,
-    pub output: String,
-    pub error: Option<String>,
-}
-
 /// Description of a tool for the LLM
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct ToolSpec {
@@ -49,7 +44,7 @@ pub struct ToolSpec {
     pub parameters: serde_json::Value,
 }
 
-/// Core tool trait — implement for any capability
+/// Core tool trait — implement for any capability (built-in or skill-based).
 #[async_trait]
 pub trait Tool: Send + Sync {
     /// Tool name (used in LLM function calling)
@@ -61,7 +56,8 @@ pub trait Tool: Send + Sync {
     /// JSON schema for parameters
     fn parameters_schema(&self) -> serde_json::Value;
 
-    /// Execute the tool with given arguments
+    /// Execute the tool with given arguments.
+    /// Returns a unified `ToolResult` (MCP content blocks + error flag).
     async fn execute(&self, args: serde_json::Value) -> anyhow::Result<ToolResult>;
 
     /// Permission level required to execute this tool.
@@ -107,15 +103,12 @@ mod tests {
         }
 
         async fn execute(&self, args: serde_json::Value) -> anyhow::Result<ToolResult> {
-            Ok(ToolResult {
-                success: true,
-                output: args
-                    .get("value")
-                    .and_then(serde_json::Value::as_str)
-                    .unwrap_or_default()
-                    .to_string(),
-                error: None,
-            })
+            let text = args
+                .get("value")
+                .and_then(serde_json::Value::as_str)
+                .unwrap_or_default()
+                .to_string();
+            Ok(ToolResult::success(text))
         }
     }
 
@@ -138,23 +131,18 @@ mod tests {
             .await
             .unwrap();
 
-        assert!(result.success);
-        assert_eq!(result.output, "hello-tool");
-        assert!(result.error.is_none());
+        assert!(!result.is_error);
+        assert_eq!(result.output(), "hello-tool");
     }
 
     #[test]
     fn tool_result_serialization_roundtrip() {
-        let result = ToolResult {
-            success: false,
-            output: String::new(),
-            error: Some("boom".into()),
-        };
+        let result = ToolResult::error("boom");
 
         let json = serde_json::to_string(&result).unwrap();
         let parsed: ToolResult = serde_json::from_str(&json).unwrap();
 
-        assert!(!parsed.success);
-        assert_eq!(parsed.error.as_deref(), Some("boom"));
+        assert!(parsed.is_error);
+        assert_eq!(parsed.output(), "boom");
     }
 }

src/openhuman/tools/update_memory_md.rs

@@ -86,13 +86,9 @@ impl Tool for UpdateMemoryMdTool {
 
         // Guard: only allow MEMORY.md and SKILL.md.
         if !ALLOWED_FILES.contains(&file) {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!(
-                    "File '{file}' is not allowed. Permitted files: MEMORY.md, SKILL.md"
-                )),
-            });
+            return Ok(ToolResult::error(format!(
+                "File '{file}' is not allowed. Permitted files: MEMORY.md, SKILL.md"
+            )));
         }
 
         let target_path = self.workspace_dir.join(file);
@@ -108,11 +104,9 @@ impl Tool for UpdateMemoryMdTool {
             .canonicalize()
             .unwrap_or_else(|_| parent.to_path_buf());
         if !parent_canon.starts_with(&workspace_canon) {
-            return Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!("File path '{file}' resolves outside workspace")),
-            });
+            return Ok(ToolResult::error(format!(
+                "File path '{file}' resolves outside workspace"
+            )));
         }
 
         tracing::debug!("[update_memory_md] action={action} file={file} path={target_path:?}");
@@ -129,13 +123,9 @@ impl Tool for UpdateMemoryMdTool {
                 self.do_replace_section(&target_path, file, section_title, content)
                     .await
             }
-            other => Ok(ToolResult {
-                success: false,
-                output: String::new(),
-                error: Some(format!(
-                    "Unknown action '{other}'. Use 'append' or 'replace_section'."
-                )),
-            }),
+            other => Ok(ToolResult::error(format!(
+                "Unknown action '{other}'. Use 'append' or 'replace_section'."
+            ))),
         }
     }
 }
@@ -168,14 +158,10 @@ impl UpdateMemoryMdTool {
             content.len()
         );
 
-        Ok(ToolResult {
-            success: true,
-            output: format!(
-                "Appended {} bytes to {file} ({bytes} bytes total).",
-                content.len()
-            ),
-            error: None,
-        })
+        Ok(ToolResult::success(format!(
+            "Appended {} bytes to {file} ({bytes} bytes total).",
+            content.len()
+        )))
     }
 
     /// Replace the body of the section headed `## {section_title}` in `path`.
@@ -241,15 +227,11 @@ impl UpdateMemoryMdTool {
             new_file_content.len()
         );
 
-        Ok(ToolResult {
-            success: true,
-            output: format!(
-                "Section '{}' updated in {file} ({} bytes).",
-                section_title,
-                new_file_content.len()
-            ),
-            error: None,
-        })
+        Ok(ToolResult::success(format!(
+            "Section '{}' updated in {file} ({} bytes).",
+            section_title,
+            new_file_content.len()
+        )))
     }
 }
 
@@ -282,7 +264,7 @@ mod tests {
             }))
             .await
             .unwrap();
-        assert!(result.success, "{:?}", result.error);
+        assert!(!result.is_error, "{:?}", result.output());
         let text = std::fs::read_to_string(dir.path().join("MEMORY.md")).unwrap();
         assert!(text.contains("first note"));
     }
@@ -360,11 +342,7 @@ mod tests {
             }))
             .await
             .unwrap();
-        assert!(!result.success);
-        assert!(result
-            .error
-            .as_deref()
-            .unwrap_or("")
-            .contains("not allowed"));
+        assert!(result.is_error);
+        assert!(result.output().contains("not allowed"));
     }
 }

src/openhuman/tools/web_search_tool.rs

@@ -315,11 +315,7 @@ impl Tool for WebSearchTool {
             _ => anyhow::bail!("Unknown search provider: {}", self.provider),
         };
 
-        Ok(ToolResult {
-            success: true,
-            output: result,
-            error: None,
-        })
+        Ok(ToolResult::success(result))
     }
 }
 

src/openhuman/tools/workspace_state.rs

@@ -122,11 +122,7 @@ impl Tool for WorkspaceStateTool {
         }
 
         tracing::debug!("[workspace_state] output length={}", output.len());
-        Ok(ToolResult {
-            success: true,
-            output,
-            error: None,
-        })
+        Ok(ToolResult::success(output))
     }
 }