test: expand agent harness coverage

src/api/rest.rs

@@ -714,9 +714,7 @@ fn key_bytes_from_string(key: &str) -> Result<Vec<u8>> {
     let trimmed = key.trim();
 
     // Raw 32-byte ASCII key
-    if trimmed.len() == 32
-        && !trimmed.contains(|c: char| c == '+' || c == '/' || c == '-' || c == '_' || c == '=')
-    {
+    if trimmed.len() == 32 && !trimmed.contains(['+', '/', '-', '_', '=']) {
         return Ok(trimmed.as_bytes().to_vec());
     }
 

src/core/cli.rs

@@ -231,7 +231,6 @@ fn run_call_command(args: &[String]) -> Result<()> {
 ///
 /// Listens for a hotkey, records audio, transcribes via whisper, and inserts
 /// the result into the active text field.
-
 fn run_voice_server_command(args: &[String]) -> Result<()> {
     use crate::openhuman::voice::hotkey::ActivationMode;
     use crate::openhuman::voice::server::{run_standalone, VoiceServerConfig};

src/openhuman/accessibility/permissions.rs

@@ -73,8 +73,8 @@ pub fn open_macos_privacy_pane(pane: &str) {
 #[cfg(target_os = "macos")]
 pub fn request_accessibility_access() {
     unsafe {
-        let keys = [kAXTrustedCheckOptionPrompt as *const c_void];
-        let values = [kCFBooleanTrue as *const c_void];
+        let keys = [kAXTrustedCheckOptionPrompt];
+        let values = [kCFBooleanTrue];
         let options = CFDictionaryCreate(
             kCFAllocatorDefault,
             keys.as_ptr(),

src/openhuman/agent/agents/mod.rs

@@ -4,8 +4,8 @@
 //! two files:
 //!
 //! * `agent.toml`  — id, when_to_use, model, tool allowlist, sandbox,
-//!                   iteration cap, and the `omit_*` flags. Parsed
-//!                   directly into [`AgentDefinition`] via serde.
+//!   iteration cap, and the `omit_*` flags. Parsed
+//!   directly into [`AgentDefinition`] via serde.
 //! * `prompt.md`   — the sub-agent's system prompt body.
 //!
 //! Adding a new built-in agent = creating a new subfolder with those two

src/openhuman/agent/bus.rs

@@ -27,6 +27,12 @@ use super::harness::run_tool_call_loop;
 /// Method name used to dispatch an agentic turn through the native bus.
 pub const AGENT_RUN_TURN_METHOD: &str = "agent.run_turn";
 
+/// Full owned payload for a single agentic turn executed through the bus.
+///
+/// All fields are either owned values, [`Arc`]s, or channel handles — the
+/// bus carries them by value without touching serialization. Consumers can
+/// therefore pass trait objects (`Arc<dyn Provider>`, tool trait-object
+/// registries) and streaming senders (`on_delta`) through unchanged.
 /// Full owned payload for a single agentic turn executed through the bus.
 ///
 /// All fields are either owned values, [`Arc`]s, or channel handles — the
@@ -35,30 +41,45 @@ pub const AGENT_RUN_TURN_METHOD: &str = "agent.run_turn";
 /// registries) and streaming senders (`on_delta`) through unchanged.
 pub struct AgentTurnRequest {
     /// LLM provider, already constructed and warmed up by the caller.
+    /// Shared via Arc to allow sub-agents to reuse the same connection pool.
     pub provider: Arc<dyn Provider>,
+
     /// Full conversation history including system prompt and the incoming
     /// user message. The handler mutates an internal clone of this during
     /// the tool-call loop; callers should rebuild their per-session cache
     /// from their own records, not from this vector.
     pub history: Vec<ChatMessage>,
+
     /// Registered tool implementations available to this turn.
+    /// These are provided as trait objects to avoid tight coupling with tool implementations.
     pub tools_registry: Arc<Vec<Box<dyn Tool>>>,
-    /// Provider name token (e.g. `"openai"`) — routed to the loop as-is.
+
+    /// Provider name token (e.g. `"openai"`) — routed to the loop as-is for logging and tracking.
     pub provider_name: String,
+
     /// Model identifier (e.g. `"gpt-4"`) — routed to the loop as-is.
     pub model: String,
-    /// Sampling temperature.
+
+    /// Sampling temperature. Higher values (e.g., 0.7) are more creative,
+    /// lower (e.g., 0.0) are more deterministic.
     pub temperature: f64,
+
     /// When `true`, suppresses stdout during the tool loop (always set by
-    /// channel callers).
+    /// channel callers to prevent cluttering the main console).
     pub silent: bool,
+
     /// Channel name this turn belongs to (e.g. `"telegram"`, `"cli"`).
+    /// Used for context and telemetry.
     pub channel_name: String,
+
     /// Multimodal feature configuration (image inlining rules, payload
     /// size caps).
     pub multimodal: MultimodalConfig,
+
     /// Maximum number of LLM↔tool round-trips before bailing out.
+    /// Prevents infinite loops if a model gets "stuck" calling the same tool.
     pub max_tool_iterations: usize,
+
     /// Optional streaming sender — the loop forwards partial LLM text
     /// chunks here so channel providers can update "draft" messages in
     /// real time. `None` disables streaming for this turn.
@@ -67,15 +88,16 @@ pub struct AgentTurnRequest {
 
 /// Final response from an agentic turn.
 pub struct AgentTurnResponse {
-    /// Final assistant text after all tool calls resolved.
+    /// Final assistant text after all tool calls resolved and the loop terminated.
     pub text: String,
 }
 
 /// Register the agent domain's native request handlers on the global
 /// registry. Safe to call multiple times — the last registration wins.
 ///
-/// Called from the canonical bus wiring in
-/// `src/core/jsonrpc.rs::register_domain_subscribers`.
+/// This function wires the `agent.run_turn` method into the core event bus,
+/// allowing any part of the system to request an agentic turn without
+/// depending directly on the agent harness.
 pub fn register_agent_handlers() {
     register_native_global::<AgentTurnRequest, AgentTurnResponse, _, _>(
         AGENT_RUN_TURN_METHOD,

src/openhuman/agent/dispatcher.rs

@@ -9,47 +9,68 @@ use serde_json::Value;
 use std::fmt::Write;
 use std::sync::Arc;
 
+/// A parsed tool call representation after being extracted from an LLM response.
 #[derive(Debug, Clone)]
 pub struct ParsedToolCall {
+    /// The name of the tool to be invoked.
     pub name: String,
+    /// The arguments passed to the tool, as a JSON object.
     pub arguments: Value,
+    /// An optional unique identifier for the tool call, provided by native APIs.
     pub tool_call_id: Option<String>,
 }
 
+/// The result of executing a tool call, formatted for the LLM.
 #[derive(Debug, Clone)]
 pub struct ToolExecutionResult {
+    /// The name of the tool that was executed.
     pub name: String,
+    /// The output of the tool execution as a string.
     pub output: String,
+    /// Whether the tool execution was successful.
     pub success: bool,
+    /// The tool call ID that generated this result.
     pub tool_call_id: Option<String>,
 }
 
+/// Trait defining how an agent interacts with an LLM for tool use.
+///
+/// Different LLMs have different "dialects" for calling tools. The dispatcher
+/// abstracts these differences, allowing the agent loop to remain agnostic of
+/// the specific formatting required by the provider.
 pub trait ToolDispatcher: Send + Sync {
+    /// Parse the LLM response to extract narrative text and any tool calls.
     fn parse_response(&self, response: &ChatResponse) -> (String, Vec<ParsedToolCall>);
+
+    /// Format tool execution results into a message suitable for the next LLM turn.
     fn format_results(&self, results: &[ToolExecutionResult]) -> ConversationMessage;
+
+    /// Provide instructions for the system prompt on how the model should call tools.
     fn prompt_instructions(&self, tools: &[Box<dyn Tool>]) -> String;
+
+    /// Convert internal conversation history into provider-specific messages.
     fn to_provider_messages(&self, history: &[ConversationMessage]) -> Vec<ChatMessage>;
+
+    /// Whether the dispatcher requires tool specifications to be sent in the API request.
     fn should_send_tool_specs(&self) -> bool;
 
     /// Tell the prompt builder how to render each tool entry in the
     /// `## Tools` section. Defaults to [`ToolCallFormat::Json`] for
-    /// dispatchers that haven't opted in — `ToolsSection` then uses
-    /// the historic schema-dump rendering.
-    ///
-    /// `PFormatToolDispatcher` overrides this to return
-    /// [`ToolCallFormat::PFormat`] so the catalogue shows positional
-    /// signatures (`get_weather[location|unit]`) instead of full JSON
-    /// schemas — that's where most of the token saving comes from at
-    /// the prompt level.
+    /// dispatchers that haven't opted in.
     fn tool_call_format(&self) -> ToolCallFormat {
         ToolCallFormat::Json
     }
 }
 
+/// Legacy dispatcher using XML-style tags (`<tool_call>`) with JSON bodies.
+///
+/// This is robust and works well with models that aren't natively trained for
+/// tool calling but can follow instructions in a system prompt.
 #[derive(Default)]
 pub struct XmlToolDispatcher;
 
 impl XmlToolDispatcher {
+    /// Internal helper to extract tool calls from a raw text string.
     fn parse_tool_calls_from_text(response: &str) -> (String, Vec<ParsedToolCall>) {
         let (text, calls) = parse_tool_calls(response);
         let parsed_calls = calls
@@ -63,6 +84,7 @@ impl XmlToolDispatcher {
         (text, parsed_calls)
     }
 
+    /// Extract serializable specs for all tools in the registry.
     pub fn tool_specs(tools: &[Box<dyn Tool>]) -> Vec<ToolSpec> {
         tools.iter().map(|tool| tool.spec()).collect()
     }
@@ -145,42 +167,31 @@ impl ToolDispatcher for XmlToolDispatcher {
 }
 
 /// Text-based dispatcher that emits and parses **P-Format** ("Parameter
-/// Format") tool calls — the compact `tool_name[arg1|arg2|...]` syntax
-/// defined in [`crate::openhuman::agent::pformat`].
+/// Format") tool calls — the compact `tool_name[arg1|arg2|...]` syntax.
 ///
-/// This is the default dispatcher for providers that do not support
-/// native structured tool calls. Compared to the legacy
-/// [`XmlToolDispatcher`] (XML wrapper + JSON body), p-format cuts the
-/// per-call token cost by ~80% — a single weather lookup goes from
-/// ~25 tokens to ~5 — which compounds dramatically over a long agent
-/// loop.
-///
-/// The dispatcher caches a [`PFormatRegistry`] (a `name → params`
-/// lookup) at construction time so it never has to hold a reference to
-/// the live `Vec<Box<dyn Tool>>` (which the [`Agent`] owns). The
-/// caller is expected to build the registry from the same tool slice
-/// they pass into the agent — see `pformat::build_registry`.
+/// P-format is designed to significantly reduce token usage compared to JSON.
+/// It uses positional arguments based on an alphabetical sort of the tool's
+/// parameters.
 ///
 /// On the parse side the dispatcher tries p-format **first** and falls
 /// back to the existing JSON-in-tag parser if the body doesn't match
 /// the bracket pattern. This keeps the dispatcher backwards-compatible
-/// with models that still emit JSON tool calls — they just pay the
-/// usual token cost for their bytes.
+/// with models that still emit JSON tool calls.
 pub struct PFormatToolDispatcher {
+    /// Registry of tool parameter layouts used to reconstruct named arguments from positional ones.
     registry: Arc<PFormatRegistry>,
 }
 
 impl PFormatToolDispatcher {
+    /// Create a new P-Format dispatcher with the given tool registry.
     pub fn new(registry: PFormatRegistry) -> Self {
         Self {
             registry: Arc::new(registry),
         }
     }
 
-    /// Convert the registry-driven parser output into the dispatcher's
-    /// `ParsedToolCall` shape. Always called inside a `<tool_call>` tag
-    /// body — the tag-finding logic comes from the shared
-    /// [`parse_tool_calls`] helper.
+    /// Convert the registry-driven positional parser output into the dispatcher's
+    /// `ParsedToolCall` shape. Always called inside a `<tool_call>` tag.
     fn try_parse_pformat_body(&self, body: &str) -> Option<ParsedToolCall> {
         let (name, args) = pformat::parse_call(body, self.registry.as_ref())?;
         Some(ParsedToolCall {
@@ -359,6 +370,12 @@ impl ToolDispatcher for PFormatToolDispatcher {
     }
 }
 
+/// Dispatcher for models with native, structured tool-calling support (e.g., OpenAI, Anthropic).
+///
+/// This dispatcher leverages the provider's built-in APIs for identifying and
+/// reporting tool calls, which is generally more reliable than text-based parsing.
+/// It still supports a text-based fallback for robustness against models that
+/// might "forget" to use the structured API.
 pub struct NativeToolDispatcher;
 
 impl ToolDispatcher for NativeToolDispatcher {

src/openhuman/agent/error.rs

@@ -9,31 +9,43 @@ use std::fmt;
 /// Structured error type for agent loop operations.
 #[derive(Debug)]
 pub enum AgentError {
-    /// The LLM provider returned an error.
+    /// The LLM provider returned an error (e.g., API key invalid, network failure).
+    /// `retryable` indicates if the operation should be attempted again.
     ProviderError { message: String, retryable: bool },
-    /// Context window is exhausted and compaction cannot help.
+
+    /// Context window is exhausted and compaction/summarization cannot help.
+    /// The agent cannot proceed without dropping significant history.
     ContextLimitExceeded { utilization_pct: u8 },
-    /// A tool execution failed.
+
+    /// A tool execution failed during its `execute()` method.
     ToolExecutionError { tool_name: String, message: String },
-    /// The daily cost budget has been exceeded.
+
+    /// The daily cost budget for this user/agent has been exceeded.
+    /// Prevents unexpected runaway costs.
     CostBudgetExceeded {
         spent_microdollars: u64,
         budget_microdollars: u64,
     },
-    /// The agent exceeded its maximum tool iterations.
+
+    /// The agent exceeded its maximum allowed tool iterations for a single turn.
+    /// Typically indicates an infinite loop in the model's reasoning.
     MaxIterationsExceeded { max: usize },
-    /// History compaction failed.
+
+    /// Automated history compaction (summarization) failed.
     CompactionFailed {
         message: String,
         consecutive_failures: u8,
     },
-    /// Channel permission denied for a tool operation.
+
+    /// The current channel (e.g., Telegram) does not have permission to execute
+    /// the requested tool (e.g., shell access).
     PermissionDenied {
         tool_name: String,
         required_level: String,
         channel_max_level: String,
     },
-    /// Generic/untyped error (escape hatch for migration).
+
+    /// Generic/untyped error (escape hatch for migration or external dependencies).
     Other(anyhow::Error),
 }
 
@@ -122,6 +134,7 @@ pub fn is_context_limit_error(error_msg: &str) -> bool {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use std::error::Error;
 
     #[test]
     fn display_formatting() {
@@ -155,4 +168,44 @@ mod tests {
         assert!(err.to_string().contains("shell"));
         assert!(err.to_string().contains("Execute"));
     }
+
+    #[test]
+    fn display_formats_other_variants() {
+        assert!(AgentError::ProviderError {
+            message: "boom".into(),
+            retryable: true,
+        }
+        .to_string()
+        .contains("retryable=true"));
+        assert!(AgentError::ContextLimitExceeded {
+            utilization_pct: 98
+        }
+        .to_string()
+        .contains("98% utilized"));
+        assert!(AgentError::ToolExecutionError {
+            tool_name: "shell".into(),
+            message: "denied".into(),
+        }
+        .to_string()
+        .contains("Tool execution error [shell]"));
+        assert!(AgentError::CompactionFailed {
+            message: "summary failed".into(),
+            consecutive_failures: 3,
+        }
+        .to_string()
+        .contains("3 consecutive"));
+    }
+
+    #[test]
+    fn from_anyhow_recovers_typed_agent_error_and_other_source() {
+        let typed = anyhow::anyhow!(AgentError::MaxIterationsExceeded { max: 4 });
+        match AgentError::from(typed) {
+            AgentError::MaxIterationsExceeded { max } => assert_eq!(max, 4),
+            other => panic!("unexpected variant: {other}"),
+        }
+
+        let other = AgentError::from(anyhow::anyhow!("plain failure"));
+        assert!(matches!(other, AgentError::Other(_)));
+        assert!(other.source().is_some());
+    }
 }