From 2b33afeefe39c69f06c1ff22aca0d9ee7fa36781 Mon Sep 17 00:00:00 2001 From: Mega Mind <146339422+M3gA-Mind@users.noreply.github.com> Date: Wed, 1 Apr 2026 02:28:56 +0530 Subject: [PATCH] fix(skills): enforce per-skill runtime tool isolation (#140) * Add unit tests for Mnemonic page - Introduced comprehensive tests for the Mnemonic page, covering initial render, copy to clipboard functionality, confirmation checkbox behavior, and mode switching between generate and import. - Validated user interactions, including input handling and button states, ensuring robust functionality and user experience. - Enhanced test coverage for various scenarios, including validation of mnemonic phrases and loading states during operations. * test: add cross-stack test coverage for core and tauri flows Add focused Rust and frontend tests for core process startup behavior, CLI argument parsing, JSON-RPC error handling, and Tauri command/RPC mapping paths to improve confidence for issue #57. Closes #57 Made-with: Cursor * refactor(tests): streamline test code and improve readability Consolidate mock imports and simplify function calls in coreRpcClient tests. Adjust formatting in Rust core_process and CLI tests for better clarity. Update mnemonic test assertions for improved accuracy. Made-with: Cursor * fix(e2e): harden deep-link login flow reliability Stabilize auth deep-link handling and E2E delivery with readiness guards, retries, and new listener unit tests so login/onboarding flows are deterministic for issue #70. Closes #70 Made-with: Cursor * refactor(tests): update agent initialization in tests for consistency Refactor test cases to use a tuple return from `build_agent_with`, improving consistency in agent setup across multiple tests. This change enhances readability and maintains uniformity in the test structure. Made-with: Cursor * fix(skills): enforce per-skill runtime tool isolation Add explicit tool-call origin policy in the QuickJS runtime so skills cannot invoke other skills' tools, while preserving external orchestration through RPC/socket surfaces. Also remove the generic skills_call tool path and document the isolation contract for skill authors. Closes #94 Made-with: Cursor --- docs/SKILLS-HOW-THEY-WORK.md | 29 +++ docs/skills-runtime-isolation.md | 67 +++++++ src/openhuman/agent/tests.rs | 98 +--------- src/openhuman/skills/bridge/skills_bridge.rs | 64 +------ src/openhuman/skills/qjs_engine.rs | 22 ++- .../skills/quickjs_libs/bootstrap.js | 6 +- src/openhuman/skills/skill_registry.rs | 126 +++++++++++- src/openhuman/skills/socket_manager.rs | 7 +- src/openhuman/skills/types.rs | 11 +- src/openhuman/tools/mod.rs | 2 - src/openhuman/tools/ops.rs | 3 - src/openhuman/tools/skills_call.rs | 179 ------------------ 12 files changed, 270 insertions(+), 344 deletions(-) create mode 100644 docs/skills-runtime-isolation.md delete mode 100644 src/openhuman/tools/skills_call.rs diff --git a/docs/SKILLS-HOW-THEY-WORK.md b/docs/SKILLS-HOW-THEY-WORK.md index 3215a98a0..7d2322b18 100644 --- a/docs/SKILLS-HOW-THEY-WORK.md +++ b/docs/SKILLS-HOW-THEY-WORK.md @@ -201,6 +201,13 @@ Typical operations: Tool calls can be sync or async in JS. Async calls are awaited with runtime polling and timeout handling in the QuickJS event loop layer. +### Isolation guarantees + +- Each started skill runs in its own QuickJS context (`AsyncContext`) and does not share mutable JS globals with other skills. +- Restarting a skill creates a fresh context; prior `globalThis` mutations are not retained. +- Host-level policy blocks skill-to-skill tool invocation. A running skill can only invoke its own tool surface. +- External orchestrators (UI/RPC/socket MCP) can still target tools on any running skill by explicit `skill_id`. + --- ## 8) JSON-RPC Surface (`openhuman.skills_*`) @@ -283,6 +290,8 @@ High-level pattern: 3. Incoming tool calls route to `skill_id` + `tool_name`. 4. Core executes via runtime and returns `ToolResult`. +Tool naming over MCP remains `skillId__toolName` for external orchestration. + --- ## 12) Environment Variables and Configuration @@ -345,6 +354,7 @@ High-level pattern: 5. Tool-call failures: - verify skill status is `Running` - inspect skill error in snapshot + - cross-skill denied errors mean a skill attempted to invoke another skill's tool; this is blocked by design ### Useful runtime truths @@ -399,3 +409,22 @@ When changing behavior, test both: - Avoid introducing new legacy skill metadata paths. - Add traceable logs around install/start/setup/tool call boundaries. +--- + +## 18) Skill Author Isolation Contract + +### Guaranteed + +- Your skill runs in its own QuickJS context and event loop when started. +- Your skill's `globalThis` state is isolated from other running skills. +- Your skill can publish state only for itself; state is namespaced by `skill_id`. +- Cross-skill tool invocation from within a skill is denied by host policy. +- External host orchestration (UI/RPC/socket MCP) may call your tools by explicit `skill_id`. + +### Not supported / undefined behavior + +- Do not rely on `globalThis.skills.callTool` for inter-skill calls. +- Do not rely on in-memory JS globals surviving a stop/restart cycle. +- Do not assume execution ordering across different skills. +- Do not treat runtime-internal bridge objects as stable public APIs. + diff --git a/docs/skills-runtime-isolation.md b/docs/skills-runtime-isolation.md new file mode 100644 index 000000000..6a7949fc6 --- /dev/null +++ b/docs/skills-runtime-isolation.md @@ -0,0 +1,67 @@ +# Skills Runtime Isolation (QuickJS) + +This document defines OpenHuman's skill isolation contract for the QuickJS runtime. + +## Goals + +- Keep each running skill isolated in JavaScript execution state. +- Prevent lateral movement where one skill invokes another skill's tools. +- Preserve external orchestration from trusted host surfaces (RPC/UI/socket MCP). + +## Runtime Architecture + +### Per-skill components + +- `QjsSkillInstance` (`src/openhuman/skills/qjs_skill_instance`) per started skill. +- One dedicated `AsyncRuntime` + `AsyncContext` created during skill spawn. +- Skill-local JS globals (`globalThis`), module state, and in-memory variables. +- Skill-local data path (`skills_data//...`) and namespaced memory writes. +- Skill-local message loop handling `SkillMessage` commands. + +### Shared infrastructure + +- `RuntimeEngine` as lifecycle orchestrator. +- `SkillRegistry` for tracking/routing running skills. +- `SocketManager` for external MCP transport. +- Schedulers (`CronScheduler`, `PingScheduler`) and preferences store. + +## Lifecycle and Reset Semantics + +1. `start_skill(skill_id)` creates a fresh skill instance and QuickJS context. +2. The instance initializes (`init/start`) and registers tools/state. +3. During runtime, only that skill's event loop mutates its JS state. +4. `stop_skill(skill_id)` stops the loop and transitions status. +5. Restart creates a new instance/context; previous JS globals are not reused. + +Failure modes: + +- Tool dispatch to non-running skill returns a status error. +- Reply channel drop returns a deterministic runtime error. +- Policy denials return explicit "cross-skill forbidden" errors. + +## Tool Invocation Policy + +Policy is enforced at host boundary (Rust), not in JS: + +- `External` origin (JSON-RPC/UI/socket MCP): may call any target skill tool. +- `SkillSelf { skill_id }` origin: may call only tools owned by the same `skill_id`. +- Cross-skill attempt from `SkillSelf` is denied before dispatch. + +This policy is implemented in `SkillRegistry::call_tool_scoped`. + +## Forbidden Paths + +- A running skill may not invoke another skill's tools. +- Inter-skill bridge helpers must not bypass host policy. +- `skills_call` generic cross-skill agent tool is not part of the default tool registry. + +## Testing Requirements + +Runtime policy tests must cover: + +- external origin allowed, +- same-skill origin allowed, +- cross-skill origin denied with clear error. + +Regression checks should also ensure the default agent tool registry does not include +legacy cross-skill helper surfaces. diff --git a/src/openhuman/agent/tests.rs b/src/openhuman/agent/tests.rs index 44b3cbedb..54a347183 100644 --- a/src/openhuman/agent/tests.rs +++ b/src/openhuman/agent/tests.rs @@ -159,54 +159,6 @@ impl Tool for EchoTool { } } -/// A generic skills bridge test-double tool. -struct SkillsCallEchoTool; - -#[async_trait] -impl Tool for SkillsCallEchoTool { - fn name(&self) -> &str { - "skills_call" - } - - fn description(&self) -> &str { - "Calls a skill tool" - } - - fn parameters_schema(&self) -> serde_json::Value { - serde_json::json!({ - "type": "object", - "properties": { - "skill_id": {"type": "string"}, - "tool_name": {"type": "string"}, - "arguments": {"type": "object"} - }, - "required": ["skill_id", "tool_name"] - }) - } - - async fn execute(&self, args: serde_json::Value) -> Result { - let skill_id = args - .get("skill_id") - .and_then(|v| v.as_str()) - .unwrap_or("unknown"); - let tool_name = args - .get("tool_name") - .and_then(|v| v.as_str()) - .unwrap_or("unknown"); - let message = args - .get("arguments") - .and_then(|v| v.get("message")) - .and_then(|v| v.as_str()) - .unwrap_or("empty"); - - Ok(ToolResult { - success: true, - output: format!("{skill_id}:{tool_name}:{message}"), - error: None, - }) - } -} - /// A tool that always fails execution. struct FailingTool; @@ -893,7 +845,7 @@ async fn e2e_native_loop_executes_text_fallback_tool_calls_and_persists_history( text_response("Completed via tool"), ])); - let mut agent = build_agent_with( + let (mut agent, _tmp) = build_agent_with( provider, vec![Box::new(EchoTool)], Box::new(NativeToolDispatcher), @@ -1337,54 +1289,6 @@ fn native_dispatcher_converts_tool_results_to_tool_messages() { assert_eq!(messages[1].role, "tool"); } -#[tokio::test] -async fn native_dispatcher_executes_generic_skills_call_tool() { - let provider = Box::new(ScriptedProvider::new(vec![ - tool_response(vec![ToolCall { - id: "tc-skills-1".into(), - name: "skills_call".into(), - arguments: serde_json::json!({ - "skill_id": "e2e-runtime", - "tool_name": "echo", - "arguments": { "message": "hello from agent test" } - }) - .to_string(), - }]), - text_response("skills call done"), - ])); - - let mut agent = build_agent_with( - provider, - vec![Box::new(SkillsCallEchoTool)], - Box::new(NativeToolDispatcher), - ); - - let response = agent.turn("Use skills_call").await.unwrap(); - assert_eq!(response, "skills call done"); - - let result_payloads: Vec = agent - .history() - .iter() - .filter_map(|msg| match msg { - ConversationMessage::ToolResults(results) => Some( - results - .iter() - .map(|r| r.content.clone()) - .collect::>() - .join("\n"), - ), - _ => None, - }) - .collect(); - - assert!( - result_payloads - .iter() - .any(|payload| payload.contains("e2e-runtime:echo:hello from agent test")), - "expected skills_call output in tool results, got: {result_payloads:?}" - ); -} - // ═══════════════════════════════════════════════════════════════════════════ // 23. XML tool instructions generation // ═══════════════════════════════════════════════════════════════════════════ diff --git a/src/openhuman/skills/bridge/skills_bridge.rs b/src/openhuman/skills/bridge/skills_bridge.rs index 15b1664d4..365dea2ee 100644 --- a/src/openhuman/skills/bridge/skills_bridge.rs +++ b/src/openhuman/skills/bridge/skills_bridge.rs @@ -1,8 +1,8 @@ //! Inter-skill communication bridge. //! -//! Allows skills to list other running skills and call their tools. -//! Tool calls are performed on a separate OS thread with a mini Tokio -//! runtime to avoid deadlocking the V8 runtime. +//! Deliberately restricted: running skills cannot invoke tools owned by +//! other skills. Keep this boundary explicit even if bridge APIs are +//! reintroduced in the future. use std::sync::Arc; @@ -33,58 +33,14 @@ pub fn list_skills(registry: &Arc) -> String { /// /// Returns the ToolResult as a JSON string. pub fn call_tool( - registry: &Arc, + _registry: &Arc, caller_skill_id: &str, target_skill_id: &str, - tool_name: &str, - arguments_json: &str, + _tool_name: &str, + _arguments_json: &str, ) -> Result { - // Prevent self-calls (would deadlock — the skill's message loop - // is blocked waiting for us, so it can't process the tool call) - if caller_skill_id == target_skill_id { - return Err("Cannot call tools on self (would deadlock)".to_string()); - } - - let registry = registry.clone(); - let target = target_skill_id.to_string(); - let tool = tool_name.to_string(); - let arguments: serde_json::Value = - serde_json::from_str(arguments_json).unwrap_or(serde_json::json!({})); - - let (tx, rx) = std::sync::mpsc::sync_channel(1); - - std::thread::spawn(move || { - // Try to use existing runtime first, fallback to creating new one if needed - let arguments_clone = arguments.clone(); - let runtime_result = tokio::runtime::Handle::try_current() - .map(|handle| { - // Use existing runtime by blocking on it - handle.block_on(async { registry.call_tool(&target, &tool, arguments).await }) - }) - .or_else(|_| { - // Only create new runtime if we're not in an async context - tokio::runtime::Builder::new_current_thread() - .enable_all() - .build() - .map_err(|e| format!("Failed to create runtime: {e}")) - .map(|rt| { - rt.block_on(async { - registry.call_tool(&target, &tool, arguments_clone).await - }) - }) - }); - - let result = match runtime_result { - Ok(tool_result) => tool_result, - Err(e) => Err(e), - }; - - let _ = tx.send(result); - }); - - let result = rx - .recv_timeout(std::time::Duration::from_secs(60)) - .map_err(|e| format!("Inter-skill tool call timed out: {e}"))??; - - serde_json::to_string(&result).map_err(|e| format!("Failed to serialize tool result: {e}")) + Err(format!( + "Cross-skill tool invocation is disabled: '{}' cannot call '{}'", + caller_skill_id, target_skill_id + )) } diff --git a/src/openhuman/skills/qjs_engine.rs b/src/openhuman/skills/qjs_engine.rs index 9298f26a2..8c15fcb6f 100644 --- a/src/openhuman/skills/qjs_engine.rs +++ b/src/openhuman/skills/qjs_engine.rs @@ -38,7 +38,7 @@ use crate::openhuman::skills::preferences::PreferencesStore; use crate::openhuman::skills::qjs_skill_instance::{BridgeDeps, QjsSkillInstance}; use crate::openhuman::skills::skill_registry::SkillRegistry; use crate::openhuman::skills::socket_manager::SocketManager; -use crate::openhuman::skills::types::{SkillSnapshot, SkillStatus, ToolResult}; +use crate::openhuman::skills::types::{SkillSnapshot, SkillStatus, ToolCallOrigin, ToolResult}; // IdbStorage removed during runtime cleanup /// The central runtime engine using QuickJS. @@ -506,6 +506,26 @@ impl RuntimeEngine { .await } + /// Call a tool from inside a running skill. Enforces self-only invocation. + pub async fn call_tool_as_skill( + &self, + caller_skill_id: &str, + target_skill_id: &str, + tool_name: &str, + arguments: serde_json::Value, + ) -> Result { + self.registry + .call_tool_scoped( + ToolCallOrigin::SkillSelf { + skill_id: caller_skill_id.to_string(), + }, + target_skill_id, + tool_name, + arguments, + ) + .await + } + /// Broadcast a server event to all running skills. pub async fn broadcast_event(&self, event: &str, data: serde_json::Value) { self.registry.broadcast_event(event, data).await; diff --git a/src/openhuman/skills/quickjs_libs/bootstrap.js b/src/openhuman/skills/quickjs_libs/bootstrap.js index 5f74e64c1..8413c1885 100644 --- a/src/openhuman/skills/quickjs_libs/bootstrap.js +++ b/src/openhuman/skills/quickjs_libs/bootstrap.js @@ -927,12 +927,12 @@ globalThis.cron = { // ============================================================================ globalThis.skills = { list: function () { - console.warn('[skills] list not implemented in QuickJS runtime yet'); + console.warn('[skills] list is intentionally unavailable in isolated runtime'); return []; }, callTool: function (skillId, toolName, args) { - console.warn('[skills] callTool not implemented in QuickJS runtime yet'); - return { error: 'Not implemented' }; + console.warn('[skills] callTool is disabled by runtime isolation policy'); + return { error: 'Cross-skill invocation is disabled' }; }, }; diff --git a/src/openhuman/skills/skill_registry.rs b/src/openhuman/skills/skill_registry.rs index af24ae77f..018f736c1 100644 --- a/src/openhuman/skills/skill_registry.rs +++ b/src/openhuman/skills/skill_registry.rs @@ -8,7 +8,8 @@ use tokio::sync::{mpsc, oneshot}; use crate::openhuman::skills::qjs_skill_instance::SkillState; use crate::openhuman::skills::types::{ - SkillConfig, SkillMessage, SkillSnapshot, SkillStatus, ToolDefinition, ToolResult, + SkillConfig, SkillMessage, SkillSnapshot, SkillStatus, ToolCallOrigin, ToolDefinition, + ToolResult, }; /// Entry in the registry for a single skill. @@ -112,13 +113,37 @@ impl SkillRegistry { .map(|e| e.state.read().status) } - /// Call a tool on a specific skill. Returns a oneshot receiver for the result. + /// Call a tool on a specific skill from external host surfaces. pub async fn call_tool( &self, skill_id: &str, tool_name: &str, arguments: serde_json::Value, ) -> Result { + self.call_tool_scoped(ToolCallOrigin::External, skill_id, tool_name, arguments) + .await + } + + /// Call a tool with an explicit origin so runtime policy can enforce boundaries. + pub async fn call_tool_scoped( + &self, + origin: ToolCallOrigin, + skill_id: &str, + tool_name: &str, + arguments: serde_json::Value, + ) -> Result { + if let ToolCallOrigin::SkillSelf { + skill_id: caller_skill_id, + } = &origin + { + if caller_skill_id != skill_id { + return Err(format!( + "Cross-skill tool calls are forbidden: '{}' cannot call '{}.{}'", + caller_skill_id, skill_id, tool_name + )); + } + } + log::info!( "[skill:{}] call_tool '{}' — dispatching to event loop", skill_id, @@ -323,3 +348,100 @@ impl Default for SkillRegistry { Self::new() } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::openhuman::skills::types::{SkillConfig, SkillStatus, ToolCallOrigin}; + + fn register_running_skill(registry: &SkillRegistry, skill_id: &str) { + let (tx, mut rx) = mpsc::channel(8); + let state = Arc::new(RwLock::new(SkillState { + status: SkillStatus::Running, + tools: vec![], + error: None, + published_state: HashMap::new(), + })); + let task = tokio::spawn(async move { + while let Some(msg) = rx.recv().await { + if let SkillMessage::CallTool { reply, .. } = msg { + let _ = reply.send(Ok(ToolResult { + content: vec![], + is_error: false, + })); + } + } + }); + + registry.register( + skill_id, + SkillConfig { + skill_id: skill_id.to_string(), + name: skill_id.to_string(), + entry_point: "index.js".to_string(), + memory_limit: 1024, + auto_start: false, + }, + tx, + state, + task, + ); + } + + #[tokio::test] + async fn external_origin_can_call_any_skill_tool() { + let registry = SkillRegistry::new(); + register_running_skill(®istry, "alpha"); + + let result = registry + .call_tool_scoped( + ToolCallOrigin::External, + "alpha", + "echo", + serde_json::json!({}), + ) + .await; + + assert!(result.is_ok()); + } + + #[tokio::test] + async fn skill_origin_cannot_call_other_skill_tool() { + let registry = SkillRegistry::new(); + register_running_skill(®istry, "alpha"); + register_running_skill(®istry, "beta"); + + let err = registry + .call_tool_scoped( + ToolCallOrigin::SkillSelf { + skill_id: "alpha".to_string(), + }, + "beta", + "echo", + serde_json::json!({}), + ) + .await + .expect_err("cross-skill call should be denied"); + + assert!(err.contains("Cross-skill tool calls are forbidden")); + } + + #[tokio::test] + async fn skill_origin_can_call_own_tool() { + let registry = SkillRegistry::new(); + register_running_skill(®istry, "alpha"); + + let result = registry + .call_tool_scoped( + ToolCallOrigin::SkillSelf { + skill_id: "alpha".to_string(), + }, + "alpha", + "echo", + serde_json::json!({}), + ) + .await; + + assert!(result.is_ok()); + } +} diff --git a/src/openhuman/skills/socket_manager.rs b/src/openhuman/skills/socket_manager.rs index 3fccef66e..c347c1f23 100644 --- a/src/openhuman/skills/socket_manager.rs +++ b/src/openhuman/skills/socket_manager.rs @@ -29,7 +29,7 @@ use { // SkillRegistry only available on desktop use crate::openhuman::skills::skill_registry::SkillRegistry; -use crate::openhuman::skills::types::{SkillSnapshot, SkillStatus}; +use crate::openhuman::skills::types::{SkillSnapshot, SkillStatus, ToolCallOrigin}; /// Events emitted to the frontend via Tauri. #[allow(dead_code)] @@ -727,7 +727,10 @@ async fn handle_mcp_tool_call( let registry = shared.registry.read().clone(); if let Some(registry) = registry { - match registry.call_tool(skill_id, tool_name, arguments).await { + match registry + .call_tool_scoped(ToolCallOrigin::External, skill_id, tool_name, arguments) + .await + { Ok(tool_result) => json!({ "content": tool_result.content, "isError": tool_result.is_error, diff --git a/src/openhuman/skills/types.rs b/src/openhuman/skills/types.rs index 3245f5489..adb7877de 100644 --- a/src/openhuman/skills/types.rs +++ b/src/openhuman/skills/types.rs @@ -1,4 +1,4 @@ -//! Core type definitions for the V8 skill runtime. +//! Core type definitions for the QuickJS skill runtime. use serde::{Deserialize, Serialize}; use std::collections::HashMap; @@ -105,6 +105,15 @@ pub enum SkillMessage { LoadParams { params: serde_json::Value }, } +/// Origin of a tool-call request entering the skill runtime. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ToolCallOrigin { + /// Calls initiated by trusted host surfaces (RPC/UI/socket MCP). + External, + /// Calls initiated from inside a running skill. + SkillSelf { skill_id: String }, +} + /// Result of executing a tool. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ToolResult { diff --git a/src/openhuman/tools/mod.rs b/src/openhuman/tools/mod.rs index 9aa5c3eae..8480daa57 100644 --- a/src/openhuman/tools/mod.rs +++ b/src/openhuman/tools/mod.rs @@ -29,7 +29,6 @@ pub mod schema; mod schemas; pub mod screenshot; pub mod shell; -pub mod skills_call; pub mod traits; pub mod web_search_tool; @@ -66,7 +65,6 @@ pub use schemas::{ }; pub use screenshot::ScreenshotTool; pub use shell::ShellTool; -pub use skills_call::SkillsCallTool; pub use traits::Tool; #[allow(unused_imports)] pub use traits::{ToolResult, ToolSpec}; diff --git a/src/openhuman/tools/ops.rs b/src/openhuman/tools/ops.rs index 6aa7e4453..878647a46 100644 --- a/src/openhuman/tools/ops.rs +++ b/src/openhuman/tools/ops.rs @@ -94,7 +94,6 @@ pub fn all_tools_with_runtime( security.clone(), workspace_dir.to_path_buf(), )), - Box::new(SkillsCallTool::new()), ]; if browser_config.enabled { @@ -253,7 +252,6 @@ mod tests { assert!(names.contains(&"schedule")); assert!(names.contains(&"pushover")); assert!(names.contains(&"proxy_config")); - assert!(names.contains(&"skills_call")); } #[test] @@ -293,7 +291,6 @@ mod tests { assert!(names.contains(&"browser_open")); assert!(names.contains(&"pushover")); assert!(names.contains(&"proxy_config")); - assert!(names.contains(&"skills_call")); } #[test] diff --git a/src/openhuman/tools/skills_call.rs b/src/openhuman/tools/skills_call.rs deleted file mode 100644 index 343ab0a6b..000000000 --- a/src/openhuman/tools/skills_call.rs +++ /dev/null @@ -1,179 +0,0 @@ -use async_trait::async_trait; -use serde_json::{Map, Value}; - -use crate::openhuman::skills::require_engine; -use crate::openhuman::tools::{Tool, ToolResult}; - -pub struct SkillsCallTool; - -impl SkillsCallTool { - pub fn new() -> Self { - Self - } - - fn validate_args(args: Value) -> anyhow::Result<(String, String, Value)> { - let obj = args - .as_object() - .ok_or_else(|| anyhow::anyhow!("arguments must be a JSON object"))?; - - let skill_id = obj - .get("skill_id") - .and_then(Value::as_str) - .map(str::trim) - .filter(|v| !v.is_empty()) - .ok_or_else(|| anyhow::anyhow!("missing required field: skill_id"))? - .to_string(); - - let tool_name = obj - .get("tool_name") - .and_then(Value::as_str) - .map(str::trim) - .filter(|v| !v.is_empty()) - .ok_or_else(|| anyhow::anyhow!("missing required field: tool_name"))? - .to_string(); - - let arguments = obj - .get("arguments") - .cloned() - .unwrap_or_else(|| Value::Object(Map::new())); - - Ok((skill_id, tool_name, arguments)) - } - - fn render_skill_output(result: &crate::openhuman::skills::types::ToolResult) -> String { - let mut parts = Vec::new(); - for block in &result.content { - match block { - crate::openhuman::skills::types::ToolContent::Text { text } => { - parts.push(text.clone()); - } - crate::openhuman::skills::types::ToolContent::Json { data } => { - parts.push(data.to_string()); - } - } - } - - if parts.is_empty() { - String::new() - } else { - parts.join("\n") - } - } -} - -#[async_trait] -impl Tool for SkillsCallTool { - fn name(&self) -> &str { - "skills_call" - } - - fn description(&self) -> &str { - "Call a running QuickJS skill tool by skill_id and tool_name." - } - - fn parameters_schema(&self) -> Value { - serde_json::json!({ - "type": "object", - "properties": { - "skill_id": { - "type": "string", - "description": "The runtime skill id (for example: notion, google, github)." - }, - "tool_name": { - "type": "string", - "description": "The tool name exported by that skill." - }, - "arguments": { - "type": "object", - "description": "Arguments for the skill tool call. Defaults to {}." - } - }, - "required": ["skill_id", "tool_name"], - "additionalProperties": false - }) - } - - async fn execute(&self, args: Value) -> anyhow::Result { - let (skill_id, tool_name, arguments) = Self::validate_args(args)?; - let engine = require_engine().map_err(anyhow::Error::msg)?; - - let result = engine - .call_tool(&skill_id, &tool_name, arguments) - .await - .map_err(anyhow::Error::msg)?; - - Ok(ToolResult { - success: !result.is_error, - output: Self::render_skill_output(&result), - error: result - .is_error - .then(|| "skill tool reported an error".to_string()), - }) - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn schema_declares_required_fields() { - let schema = SkillsCallTool::new().parameters_schema(); - let required = schema["required"] - .as_array() - .expect("required must be an array"); - assert!(required.contains(&Value::String("skill_id".to_string()))); - assert!(required.contains(&Value::String("tool_name".to_string()))); - } - - #[test] - fn validate_args_defaults_arguments_object() { - let (skill_id, tool_name, arguments) = SkillsCallTool::validate_args(serde_json::json!({ - "skill_id": "notion", - "tool_name": "search" - })) - .expect("args should be valid"); - - assert_eq!(skill_id, "notion"); - assert_eq!(tool_name, "search"); - assert!(arguments.is_object()); - } - - #[test] - fn render_skill_output_joins_content_blocks() { - let rendered = - SkillsCallTool::render_skill_output(&crate::openhuman::skills::types::ToolResult { - content: vec![ - crate::openhuman::skills::types::ToolContent::Text { - text: "first".to_string(), - }, - crate::openhuman::skills::types::ToolContent::Json { - data: serde_json::json!({"ok": true}), - }, - ], - is_error: false, - }); - - assert!(rendered.contains("first")); - assert!(rendered.contains(r#"{"ok":true}"#)); - } - - #[tokio::test] - async fn execute_fails_when_runtime_is_not_initialized() { - let tool = SkillsCallTool::new(); - let result = tool - .execute(serde_json::json!({ - "skill_id": "missing-runtime", - "tool_name": "echo", - "arguments": {} - })) - .await; - - assert!(result.is_err()); - let err = result.err().expect("error expected").to_string(); - assert!( - err.contains("skill runtime not initialized"), - "unexpected error: {err}" - ); - } -}