From 9b73d00b2e819534e7743e4779a0dc9047db6da1 Mon Sep 17 00:00:00 2001 From: sanil-23 Date: Mon, 6 Apr 2026 22:33:07 +0530 Subject: [PATCH] fix(agent): inject skill tools into agent registry and unify ToolResult (#341) (#360) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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) * 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) * style: apply cargo fmt to skill_bridge.rs Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: Claude Opus 4.6 (1M context) --- src/openhuman/agent/agent.rs | 25 ++- src/openhuman/agent/harness/self_healing.rs | 19 +- src/openhuman/agent/loop_/session.rs | 20 ++ src/openhuman/agent/loop_/tool_loop.rs | 24 ++- src/openhuman/agent/prompt.rs | 6 +- src/openhuman/agent/tests.rs | 18 +- src/openhuman/agent/traits.rs | 29 +-- src/openhuman/channels/tests/common.rs | 12 +- src/openhuman/skills/types.rs | 57 ++++++ src/openhuman/tools/ask_clarification.rs | 6 +- src/openhuman/tools/browser.rs | 88 ++------ src/openhuman/tools/browser_open.rs | 42 +--- src/openhuman/tools/composio.rs | 82 ++------ src/openhuman/tools/cron_add.rs | 115 ++++------- src/openhuman/tools/cron_list.rs | 31 +-- src/openhuman/tools/cron_remove.rs | 35 +--- src/openhuman/tools/cron_run.rs | 51 ++--- src/openhuman/tools/cron_runs.rs | 37 +--- src/openhuman/tools/cron_update.rs | 57 ++---- src/openhuman/tools/delegate.rs | 163 +++++---------- src/openhuman/tools/file_read.rs | 141 +++++-------- src/openhuman/tools/file_write.rs | 135 +++++-------- src/openhuman/tools/git_operations.rs | 153 ++++---------- src/openhuman/tools/hardware_board_info.rs | 23 +-- src/openhuman/tools/hardware_memory_map.rs | 17 +- src/openhuman/tools/hardware_memory_read.rs | 48 ++--- src/openhuman/tools/http_request.rs | 56 ++--- src/openhuman/tools/image_info.rs | 48 ++--- src/openhuman/tools/insert_sql_record.rs | 62 ++---- src/openhuman/tools/local_cli.rs | 12 +- src/openhuman/tools/memory_forget.rs | 52 ++--- src/openhuman/tools/memory_recall.rs | 34 ++-- src/openhuman/tools/memory_store.rs | 44 ++-- src/openhuman/tools/mod.rs | 5 +- src/openhuman/tools/ops.rs | 21 +- src/openhuman/tools/proxy_config.rs | 137 +++++-------- src/openhuman/tools/pushover.rs | 63 ++---- src/openhuman/tools/read_diff.rs | 18 +- src/openhuman/tools/run_linter.rs | 48 ++--- src/openhuman/tools/run_tests.rs | 48 ++--- src/openhuman/tools/schedule.rs | 213 +++++++------------- src/openhuman/tools/screenshot.rs | 91 +++------ src/openhuman/tools/shell.rs | 108 ++++------ src/openhuman/tools/skill_bridge.rs | 193 ++++++++++++++++++ src/openhuman/tools/spawn_subagent.rs | 19 +- src/openhuman/tools/tool_stats.rs | 22 +- src/openhuman/tools/traits.rs | 46 ++--- src/openhuman/tools/update_memory_md.rs | 64 ++---- src/openhuman/tools/web_search_tool.rs | 6 +- src/openhuman/tools/workspace_state.rs | 6 +- 50 files changed, 1106 insertions(+), 1744 deletions(-) create mode 100644 src/openhuman/tools/skill_bridge.rs diff --git a/src/openhuman/agent/agent.rs b/src/openhuman/agent/agent.rs index 8a9830777..d9d29fa4c 100644 --- a/src/openhuman/agent/agent.rs +++ b/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 { - Ok(crate::openhuman::tools::ToolResult { - success: true, - output: "tool-out".into(), - error: None, - }) + Ok(crate::openhuman::tools::ToolResult::success("tool-out")) } } diff --git a/src/openhuman/agent/harness/self_healing.rs b/src/openhuman/agent/harness/self_healing.rs index 0fe48544b..4ef973965 100644 --- a/src/openhuman/agent/harness/self_healing.rs +++ b/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 { - 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()); } diff --git a/src/openhuman/agent/loop_/session.rs b/src/openhuman/agent/loop_/session.rs index b5e6867fb..edd9cefb1 100644 --- a/src/openhuman/agent/loop_/session.rs +++ b/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 { 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 diff --git a/src/openhuman/agent/loop_/tool_loop.rs b/src/openhuman/agent/loop_/tool_loop.rs index 46a86e431..63692dfba 100644 --- a/src/openhuman/agent/loop_/tool_loop.rs +++ b/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::>() + .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)) => { diff --git a/src/openhuman/agent/prompt.rs b/src/openhuman/agent/prompt.rs index 33e9039df..16607ed8f 100644 --- a/src/openhuman/agent/prompt.rs +++ b/src/openhuman/agent/prompt.rs @@ -334,11 +334,7 @@ mod tests { &self, _args: serde_json::Value, ) -> anyhow::Result { - Ok(crate::openhuman::tools::ToolResult { - success: true, - output: "ok".into(), - error: None, - }) + Ok(crate::openhuman::tools::ToolResult::success("ok")) } } diff --git a/src/openhuman/agent/tests.rs b/src/openhuman/agent/tests.rs index efa50b2cb..48e8f483a 100644 --- a/src/openhuman/agent/tests.rs +++ b/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 { - 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 { 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))) } } diff --git a/src/openhuman/agent/traits.rs b/src/openhuman/agent/traits.rs index 180759053..4a5a10c95 100644 --- a/src/openhuman/agent/traits.rs +++ b/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, -} +/// 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 { - 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"); } } diff --git a/src/openhuman/channels/tests/common.rs b/src/openhuman/channels/tests/common.rs index 073db2d8d..c76bcc2cf 100644 --- a/src/openhuman/channels/tests/common.rs +++ b/src/openhuman/channels/tests/common.rs @@ -348,18 +348,10 @@ impl Tool for MockPriceTool { async fn execute(&self, args: serde_json::Value) -> anyhow::Result { 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")) } } diff --git a/src/openhuman/skills/types.rs b/src/openhuman/skills/types.rs index f750d749f..965e4068b 100644 --- a/src/openhuman/skills/types.rs +++ b/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) -> 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) -> 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::>() + .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::>() + .join("\n") + } +} + /// A single content block within a `ToolResult`. #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(tag = "type", rename_all = "lowercase")] diff --git a/src/openhuman/tools/ask_clarification.rs b/src/openhuman/tools/ask_clarification.rs index fb811edcb..faed21176 100644 --- a/src/openhuman/tools/ask_clarification.rs +++ b/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)) } } diff --git a/src/openhuman/tools/browser.rs b/src/openhuman/tools/browser.rs index a1a107087..cf561559d 100644 --- a/src/openhuman/tools/browser.rs +++ b/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 { // 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())); } }; diff --git a/src/openhuman/tools/browser_open.rs b/src/openhuman/tools/browser_open.rs index e7d7f154c..123d9a05f 100644 --- a/src/openhuman/tools/browser_open.rs +++ b/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")); } } diff --git a/src/openhuman/tools/composio.rs b/src/openhuman/tools/composio.rs index 30fbfd420..918ece1cb 100644 --- a/src/openhuman/tools/composio.rs +++ b/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 ────────────────────────────────── diff --git a/src/openhuman/tools/cron_add.rs b/src/openhuman/tools/cron_add.rs index 81608be83..e199d527f 100644 --- a/src/openhuman/tools/cron_add.rs +++ b/src/openhuman/tools/cron_add.rs @@ -50,30 +50,22 @@ impl Tool for CronAddTool { async fn execute(&self, args: serde_json::Value) -> anyhow::Result { 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::(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::(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::(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'")); } } diff --git a/src/openhuman/tools/cron_list.rs b/src/openhuman/tools/cron_list.rs index caf2c0f13..3cf46e947 100644 --- a/src/openhuman/tools/cron_list.rs +++ b/src/openhuman/tools/cron_list.rs @@ -35,24 +35,14 @@ impl Tool for CronListTool { async fn execute(&self, _args: serde_json::Value) -> anyhow::Result { 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")); } } diff --git a/src/openhuman/tools/cron_remove.rs b/src/openhuman/tools/cron_remove.rs index 05f57fb14..829bbb8e7 100644 --- a/src/openhuman/tools/cron_remove.rs +++ b/src/openhuman/tools/cron_remove.rs @@ -37,35 +37,21 @@ impl Tool for CronRemoveTool { async fn execute(&self, args: serde_json::Value) -> anyhow::Result { 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'")); } } diff --git a/src/openhuman/tools/cron_run.rs b/src/openhuman/tools/cron_run.rs index 015a69e89..4c2bb7bb2 100644 --- a/src/openhuman/tools/cron_run.rs +++ b/src/openhuman/tools/cron_run.rs @@ -38,32 +38,22 @@ impl Tool for CronRunTool { async fn execute(&self, args: serde_json::Value) -> anyhow::Result { 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")); } } diff --git a/src/openhuman/tools/cron_runs.rs b/src/openhuman/tools/cron_runs.rs index ec7c352b6..6ceb3fe88 100644 --- a/src/openhuman/tools/cron_runs.rs +++ b/src/openhuman/tools/cron_runs.rs @@ -52,21 +52,15 @@ impl Tool for CronRunsTool { async fn execute(&self, args: serde_json::Value) -> anyhow::Result { 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'")); } } diff --git a/src/openhuman/tools/cron_update.rs b/src/openhuman/tools/cron_update.rs index e0f9a03af..341abc51c 100644 --- a/src/openhuman/tools/cron_update.rs +++ b/src/openhuman/tools/cron_update.rs @@ -40,67 +40,43 @@ impl Tool for CronUpdateTool { async fn execute(&self, args: serde_json::Value) -> anyhow::Result { 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::(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")); } } diff --git a/src/openhuman/tools/delegate.rs b/src/openhuman/tools/delegate.rs index 85d6b1d3c..dc9ed3669 100644 --- a/src/openhuman/tools/delegate.rs +++ b/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")); } } diff --git a/src/openhuman/tools/file_read.rs b/src/openhuman/tools/file_read.rs index b459bde55..a700590e1 100644 --- a/src/openhuman/tools/file_read.rs +++ b/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; } diff --git a/src/openhuman/tools/file_write.rs b/src/openhuman/tools/file_write.rs index 94a9011a5..9553d1b33 100644 --- a/src/openhuman/tools/file_write.rs +++ b/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; } diff --git a/src/openhuman/tools/git_operations.rs b/src/openhuman/tools/git_operations.rs index 3da5872fb..a4cf2127b 100644 --- a/src/openhuman/tools/git_operations.rs +++ b/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 { @@ -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 { @@ -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 { @@ -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] diff --git a/src/openhuman/tools/hardware_board_info.rs b/src/openhuman/tools/hardware_board_info.rs index 73b30fc5d..0d370cc6f 100644 --- a/src/openhuman/tools/hardware_board_info.rs +++ b/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)) } } diff --git a/src/openhuman/tools/hardware_memory_map.rs b/src/openhuman/tools/hardware_memory_map.rs index 41fd07b3d..e1f8f846c 100644 --- a/src/openhuman/tools/hardware_memory_map.rs +++ b/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)) } } diff --git a/src/openhuman/tools/hardware_memory_read.rs b/src/openhuman/tools/hardware_memory_read.rs index 3232c7874..3a87a8dae 100644 --- a/src/openhuman/tools/hardware_memory_read.rs +++ b/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 { 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", + )) } } } diff --git a/src/openhuman/tools/http_request.rs b/src/openhuman/tools/http_request.rs index 264652284..a1a572223 100644 --- a/src/openhuman/tools/http_request.rs +++ b/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] diff --git a/src/openhuman/tools/image_info.rs b/src/openhuman/tools/image_info.rs index fdf1538ba..49d8df923 100644 --- a/src/openhuman/tools/image_info.rs +++ b/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; } diff --git a/src/openhuman/tools/insert_sql_record.rs b/src/openhuman/tools/insert_sql_record.rs index da631bfa1..4eab04d3b 100644 --- a/src/openhuman/tools/insert_sql_record.rs +++ b/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] diff --git a/src/openhuman/tools/local_cli.rs b/src/openhuman/tools/local_cli.rs index 51d989aa0..88d4c5e86 100644 --- a/src/openhuman/tools/local_cli.rs +++ b/src/openhuman/tools/local_cli.rs @@ -70,7 +70,7 @@ pub async fn run_cli_screenshot(args: CliScreenshotArgs) -> Result Result Result }, "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:: }, }, "logs": logs diff --git a/src/openhuman/tools/memory_forget.rs b/src/openhuman/tools/memory_forget.rs index 6bdae750d..9f198a167 100644 --- a/src/openhuman/tools/memory_forget.rs +++ b/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()); } } diff --git a/src/openhuman/tools/memory_recall.rs b/src/openhuman/tools/memory_recall.rs index aab4a48ea..93768630a 100644 --- a/src/openhuman/tools/memory_recall.rs +++ b/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] diff --git a/src/openhuman/tools/memory_store.rs b/src/openhuman/tools/memory_store.rs index 8beda80c7..a9d948873 100644 --- a/src/openhuman/tools/memory_store.rs +++ b/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()); } } diff --git a/src/openhuman/tools/mod.rs b/src/openhuman/tools/mod.rs index 44cf12d03..d0f966541 100644 --- a/src/openhuman/tools/mod.rs +++ b/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; diff --git a/src/openhuman/tools/ops.rs b/src/openhuman/tools/ops.rs index a5b2fe61b..58b92248f 100644 --- a/src/openhuman/tools/ops.rs +++ b/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] diff --git a/src/openhuman/tools/proxy_config.rs b/src/openhuman/tools/proxy_config.rs index 40716eeb2..45061e1c3 100644 --- a/src/openhuman/tools/proxy_config.rs +++ b/src/openhuman/tools/proxy_config.rs @@ -40,19 +40,11 @@ impl ProxyConfigTool { fn require_write_access(&self) -> Option { 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 { 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 { - 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 { @@ -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 { @@ -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 { @@ -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 { 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()); } diff --git a/src/openhuman/tools/pushover.rs b/src/openhuman/tools/pushover.rs index f56a215f8..b2e883466 100644 --- a/src/openhuman/tools/pushover.rs +++ b/src/openhuman/tools/pushover.rs @@ -113,19 +113,11 @@ impl Tool for PushoverTool { async fn execute(&self, args: serde_json::Value) -> anyhow::Result { 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::(&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")); } } diff --git a/src/openhuman/tools/read_diff.rs b/src/openhuman/tools/read_diff.rs index 2dcb86b97..091776b3b 100644 --- a/src/openhuman/tools/read_diff.rs +++ b/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())) } } } diff --git a/src/openhuman/tools/run_linter.rs b/src/openhuman/tools/run_linter.rs index 45ac762b7..23dbf753e 100644 --- a/src/openhuman/tools/run_linter.rs +++ b/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 + ))) + } } } diff --git a/src/openhuman/tools/run_tests.rs b/src/openhuman/tools/run_tests.rs index 0ebd729e4..1a6f18038 100644 --- a/src/openhuman/tools/run_tests.rs +++ b/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 + ))) + } } } diff --git a/src/openhuman/tools/schedule.rs b/src/openhuman/tools/schedule.rs index 9af475000..e9c2a46f2 100644 --- a/src/openhuman/tools/schedule.rs +++ b/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 { 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 { 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 { @@ -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")); } } diff --git a/src/openhuman/tools/screenshot.rs b/src/openhuman/tools/screenshot.rs index 2a1aeb217..acf4e346e 100644 --- a/src/openhuman/tools/screenshot.rs +++ b/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 { 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] diff --git a/src/openhuman/tools/shell.rs b/src/openhuman/tools/shell.rs index 04e672b3a..127c61f97 100644 --- a/src/openhuman/tools/shell.rs +++ b/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 { @@ -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")); } } diff --git a/src/openhuman/tools/skill_bridge.rs b/src/openhuman/tools/skill_bridge.rs new file mode 100644 index 000000000..ba02515db --- /dev/null +++ b/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, +} + +impl SkillToolBridge { + fn new(skill_id: String, tool_def: &ToolDefinition, engine: Arc) -> 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 { + 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`. +/// +/// 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> { + 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::() + ); + Some(Box::new(SkillToolBridge::new( + skill_id, + &tool_def, + Arc::clone(&engine), + )) as Box) + }) + .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()); + } +} diff --git a/src/openhuman/tools/spawn_subagent.rs b/src/openhuman/tools/spawn_subagent.rs index e30c5c77c..231d73f12 100644 --- a/src/openhuman/tools/spawn_subagent.rs +++ b/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", + )) } } diff --git a/src/openhuman/tools/tool_stats.rs b/src/openhuman/tools/tool_stats.rs index 15b1f167a..8953fd4c6 100644 --- a/src/openhuman/tools/tool_stats.rs +++ b/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)) } } diff --git a/src/openhuman/tools/traits.rs b/src/openhuman/tools/traits.rs index 367d98b66..2c913029e 100644 --- a/src/openhuman/tools/traits.rs +++ b/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, -} - /// 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; /// Permission level required to execute this tool. @@ -107,15 +103,12 @@ mod tests { } async fn execute(&self, args: serde_json::Value) -> anyhow::Result { - 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"); } } diff --git a/src/openhuman/tools/update_memory_md.rs b/src/openhuman/tools/update_memory_md.rs index 1586d0c5a..21e3c0492 100644 --- a/src/openhuman/tools/update_memory_md.rs +++ b/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")); } } diff --git a/src/openhuman/tools/web_search_tool.rs b/src/openhuman/tools/web_search_tool.rs index b633f841f..5ef1bd362 100644 --- a/src/openhuman/tools/web_search_tool.rs +++ b/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)) } } diff --git a/src/openhuman/tools/workspace_state.rs b/src/openhuman/tools/workspace_state.rs index 9cc2fd3a9..82d5da1ba 100644 --- a/src/openhuman/tools/workspace_state.rs +++ b/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)) } }