From ecbdeec498b02214b40e5f568a23e2edda5e4d3a Mon Sep 17 00:00:00 2001 From: jcarver989 <117696+jcarver989@users.noreply.github.com> Date: Sun, 2 Aug 2026 05:43:02 +0000 Subject: [PATCH] chore: scheduled code-cleanup --- crates/aether-cli/src/slash_commands.rs | 3 +- crates/aether-core/src/mcp/run_mcp_task.rs | 11 +- crates/aether-core/src/mcp/tool_bridge.rs | 23 +--- crates/mcp-servers/src/coding/error.rs | 29 +++++ crates/mcp-servers/src/coding/mod.rs | 56 ++++++---- crates/mcp-servers/src/plan/server.rs | 29 +++-- .../src/subagents/tools/spawn_subagent/mod.rs | 104 +++++++++++------- 7 files changed, 156 insertions(+), 99 deletions(-) diff --git a/crates/aether-cli/src/slash_commands.rs b/crates/aether-cli/src/slash_commands.rs index 1b090a14..0937a372 100644 --- a/crates/aether-cli/src/slash_commands.rs +++ b/crates/aether-cli/src/slash_commands.rs @@ -1,5 +1,6 @@ use aether_core::mcp::run_mcp_task::McpCommand; use agent_client_protocol::schema::AvailableCommand; +use mcp_utils::client::McpError; use rmcp::model::{ContentBlock, GetPromptResult, Prompt as McpPrompt}; use std::collections::HashSet; use thiserror::Error; @@ -15,7 +16,7 @@ pub(crate) enum SlashCommandError { #[error("command channel error: {0}")] CommandChannel(String), #[error("MCP operation failed: {0}")] - McpOperation(String), + McpOperation(McpError), #[error("slash command '/{0}' not found")] NotFound(String), #[error("prompt result contains no text content")] diff --git a/crates/aether-core/src/mcp/run_mcp_task.rs b/crates/aether-core/src/mcp/run_mcp_task.rs index 85d3ad6d..b46b4c71 100644 --- a/crates/aether-core/src/mcp/run_mcp_task.rs +++ b/crates/aether-core/src/mcp/run_mcp_task.rs @@ -40,12 +40,12 @@ pub enum McpCommand { tx: mpsc::Sender, }, ListPrompts { - tx: oneshot::Sender, String>>, + tx: oneshot::Sender, McpError>>, }, GetPrompt { name: String, arguments: Option>, - tx: oneshot::Sender>, + tx: oneshot::Sender>, }, GetServerStatuses { tx: oneshot::Sender>, @@ -134,14 +134,11 @@ async fn on_command(command: McpCommand, mcp: &mut McpManager, auth_tasks: &mut } McpCommand::ListPrompts { tx } => { - let result = mcp.list_prompts().await.map_err(|e| format!("Failed to list prompts: {e}")); - let _ = tx.send(result); + let _ = tx.send(mcp.list_prompts().await); } McpCommand::GetPrompt { name: namespaced_name, arguments, tx } => { - let result = - mcp.get_prompt(&namespaced_name, arguments).await.map_err(|e| format!("Failed to get prompt: {e}")); - let _ = tx.send(result); + let _ = tx.send(mcp.get_prompt(&namespaced_name, arguments).await); } McpCommand::GetServerStatuses { tx } => { diff --git a/crates/aether-core/src/mcp/tool_bridge.rs b/crates/aether-core/src/mcp/tool_bridge.rs index b87d40aa..3832ec0d 100644 --- a/crates/aether-core/src/mcp/tool_bridge.rs +++ b/crates/aether-core/src/mcp/tool_bridge.rs @@ -1,7 +1,6 @@ use std::path::{Path, PathBuf}; -use mcp_utils::{client::SERVERNAME_DELIMITER, display_meta::ToolResultMeta}; -use rmcp::model::CallToolRequestParams; +use mcp_utils::display_meta::ToolResultMeta; use serde_json; use llm::{ToolCallError, ToolCallRequest, ToolCallResult}; @@ -13,26 +12,6 @@ const TOOL_RESULT_MAX_BYTES: usize = 200_000; /// Size of the head preview included inline when a result spills to disk. const SPILLOVER_PREVIEW_BYTES: usize = 10_000; -/// Convert a `ToolCallRequest` to `rmcp::CallToolRequestParams` -pub fn tool_call_request_to_mcp(request: &ToolCallRequest) -> Result { - let tool_name = request - .name - .split_once(SERVERNAME_DELIMITER) - .map_or_else(|| request.name.clone(), |(_, tool_name)| tool_name.to_string()); - - // Parse arguments from JSON string - let arguments = serde_json::from_str::(&request.arguments) - .map_err(|e| format!("Invalid tool arguments: {e}"))? - .as_object() - .cloned(); - - let mut params = CallToolRequestParams::new(tool_name); - if let Some(args) = arguments { - params = params.with_arguments(args); - } - Ok(params) -} - /// Convert an rmcp `CallToolResult` and request to `ToolCallResult` or `ToolCallError`, /// extracting any `_meta` metadata from structured content. pub fn mcp_result_to_tool_call_result( diff --git a/crates/mcp-servers/src/coding/error.rs b/crates/mcp-servers/src/coding/error.rs index 6d03390d..950b437d 100644 --- a/crates/mcp-servers/src/coding/error.rs +++ b/crates/mcp-servers/src/coding/error.rs @@ -6,6 +6,7 @@ use thiserror::Error; pub use crate::file_ops::FileError; +pub use crate::workspace_paths::EmptyFilePathError; #[doc = include_str!("../docs/coding_error.md")] #[derive(Debug, Error)] @@ -45,6 +46,34 @@ pub enum CodingError { /// Tool not configured/available #[error("{0}")] NotConfigured(String), + + /// User declined a tool invocation via elicitation + #[error("Operation declined by user: {tool_name}")] + PermissionDeclined { tool_name: String }, + + /// Elicitation request failed + #[error("Elicitation failed: {0}")] + ElicitationFailed(String), + + /// Required file path was empty or whitespace-only + #[error("{0}")] + EmptyFilePath(#[from] EmptyFilePathError), + + /// Read-before-overwrite safety check failed + #[error( + "Safety check failed: File '{file_path}' already exists. You must use read_file on it before overwriting. This prevents accidental data loss." + )] + OverwriteWithoutRead { file_path: String }, + + /// Read-before-edit safety check failed + #[error( + "Safety check failed: You must use read_file on '{file_path}' before editing it. This ensures you understand the current file contents before making changes." + )] + EditWithoutRead { file_path: String }, + + /// Failed to check whether a file exists + #[error("Failed to check existence of {file_path}: {reason}")] + ExistenceCheckFailed { file_path: String, reason: String }, } /// Errors related to bash command execution diff --git a/crates/mcp-servers/src/coding/mod.rs b/crates/mcp-servers/src/coding/mod.rs index ca199c63..73c6ef61 100644 --- a/crates/mcp-servers/src/coding/mod.rs +++ b/crates/mcp-servers/src/coding/mod.rs @@ -33,6 +33,8 @@ pub mod tools_trait; pub use default_tools::DefaultCodingTools; pub use tools_trait::CodingTools; +use error::CodingError; + use crate::lsp::tools::check_errors::{LspDiagnosticsOutput, LspDiagnosticsRequest, execute_lsp_diagnostics}; use crate::lsp::tools::symbol_lookup::{LspSymbolInput, LspSymbolOutput, execute_lsp_symbol}; use crate::lsp::tools::workspace_search::{ @@ -75,6 +77,16 @@ impl IntoMcpResult for Result { } } +/// MCP tool handlers return `Result<_, String>` (required by rmcp). +/// [`CodingError`](crate::coding::error::CodingError) carries richer, typed +/// context internally and converts to a `String` at that boundary so `?` +/// works seamlessly inside handlers. +impl From for String { + fn from(error: CodingError) -> String { + error.to_string() + } +} + #[doc = include_str!("../docs/permission_mode.md")] #[derive(Debug, Clone, Default, PartialEq, clap::ValueEnum)] pub enum PermissionMode { @@ -309,7 +321,7 @@ When using tools that take file paths, always use absolute paths from: context: &RequestContext, tool_name: &str, description: &str, - ) -> Result<(), String> { + ) -> Result<(), CodingError> { let message = format!("Allow {tool_name}: {description}?"); let result = context .peer @@ -329,16 +341,20 @@ When using tools that take file paths, always use absolute paths from: .unwrap(), }) .await - .map_err(|e| format!("Elicitation failed: {e}"))?; + .map_err(|e| CodingError::ElicitationFailed(e.to_string()))?; let allowed = result.content.as_ref().and_then(|c| c.get("decision")).and_then(|v| v.as_str()) == Some("allow"); - if allowed { Ok(()) } else { Err(format!("Operation declined by user: {tool_name}")) } + if allowed { Ok(()) } else { Err(CodingError::PermissionDeclined { tool_name: tool_name.to_string() }) } } /// Ask the user for permission to run a bash command. In `Auto` mode only /// triggers for destructive commands; in `AlwaysAsk` mode triggers always. - async fn check_bash_permission(&self, context: &RequestContext, command: &str) -> Result<(), String> { + async fn check_bash_permission( + &self, + context: &RequestContext, + command: &str, + ) -> Result<(), CodingError> { match self.permission_mode { PermissionMode::AlwaysAllow => Ok(()), PermissionMode::AlwaysAsk => self.elicit_permission(context, "bash", command).await, @@ -359,7 +375,7 @@ When using tools that take file paths, always use absolute paths from: context: &RequestContext, tool_name: &str, file_path: &str, - ) -> Result<(), String> { + ) -> Result<(), CodingError> { if self.permission_mode == PermissionMode::AlwaysAsk { self.elicit_permission(context, tool_name, file_path).await } else { @@ -379,15 +395,15 @@ When using tools that take file paths, always use absolute paths from: /// Resolves a required file-path argument against the root directory, /// returning the normalized absolute path as a string. - fn resolve_file_arg(&self, raw: &str) -> Result { - Ok(self.workspace_paths().resolve_file(raw).map_err(|e| e.to_string())?.to_string_lossy().to_string()) + fn resolve_file_arg(&self, raw: &str) -> Result { + Ok(self.workspace_paths().resolve_file(raw)?.to_string_lossy().to_string()) } /// Reads `args.file_path` (already resolved against the root directory), /// records it in the read set, and appends any matching read-rule reminders. - async fn read_and_track(&self, args: ReadFileArgs) -> Result, String> { + async fn read_and_track(&self, args: ReadFileArgs) -> Result, CodingError> { let file_path = args.file_path.clone(); - let mut result = self.tools.read_file(args).await.map_err(|e| e.to_string())?; + let mut result = self.tools.read_file(args).await?; self.files_read.write().await.insert(file_path.clone()); let total_lines = result.total_lines; @@ -408,23 +424,21 @@ When using tools that take file paths, always use absolute paths from: /// Read-before-overwrite safety check: an existing file must have been read /// first, preventing accidental data loss. - async fn ensure_read_before_overwrite(&self, file_path: &str) -> Result<(), String> { - if try_exists(file_path).await.map_err(|e| format!("Failed to check existence of {file_path}: {e}"))? - && !self.files_read.read().await.contains(file_path) + async fn ensure_read_before_overwrite(&self, file_path: &str) -> Result<(), CodingError> { + if try_exists(file_path).await.map_err(|e| CodingError::ExistenceCheckFailed { + file_path: file_path.to_string(), + reason: e.to_string(), + })? && !self.files_read.read().await.contains(file_path) { - return Err(format!( - "Safety check failed: File '{file_path}' already exists. You must use read_file on it before overwriting. This prevents accidental data loss." - )); + return Err(CodingError::OverwriteWithoutRead { file_path: file_path.to_string() }); } Ok(()) } /// Read-before-edit safety check: a file must have been read before editing. - async fn ensure_read_before_edit(&self, file_path: &str) -> Result<(), String> { + async fn ensure_read_before_edit(&self, file_path: &str) -> Result<(), CodingError> { if !self.files_read.read().await.contains(file_path) { - return Err(format!( - "Safety check failed: You must use read_file on '{file_path}' before editing it. This ensures you understand the current file contents before making changes." - )); + return Err(CodingError::EditWithoutRead { file_path: file_path.to_string() }); } Ok(()) } @@ -481,7 +495,7 @@ When using tools that take file paths, always use absolute paths from: let Parameters(mut args) = request; args.file_path = self.resolve_file_arg(&args.file_path)?; notify_preview(&context, ToolDisplayMeta::new("Read file", basename(&args.file_path))).await; - self.read_and_track(args).await + self.read_and_track(args).await.map_err(String::from) } #[doc = include_str!("tools/write_file/description.md")] @@ -752,7 +766,7 @@ impl CodingMcp { /// Read a file and track it in the read set (test helper, no MCP context needed). pub async fn test_read_file(&self, mut args: ReadFileArgs) -> Result, String> { args.file_path = self.resolve_file_arg(&args.file_path)?; - self.read_and_track(args).await + self.read_and_track(args).await.map_err(String::from) } /// Write a file with read-before-write safety check (test helper, no MCP context needed). diff --git a/crates/mcp-servers/src/plan/server.rs b/crates/mcp-servers/src/plan/server.rs index c43a1325..88a298db 100644 --- a/crates/mcp-servers/src/plan/server.rs +++ b/crates/mcp-servers/src/plan/server.rs @@ -202,18 +202,15 @@ impl PlanMcp { Ok(Json(SubmitPlanOutput { approved: false, feedback })) } - fn build_elicitation_form(plan: &Plan) -> Result { - let meta = PlanReviewElicitationMeta::new(&plan.path, &plan.content) - .to_json() - .map(RequestMetaObject::from) - .map_err(|e| format!("failed to serialize plan review metadata: {e}"))?; + fn build_elicitation_form(plan: &Plan) -> Result { + let meta = PlanReviewElicitationMeta::new(&plan.path, &plan.content).to_json().map(RequestMetaObject::from)?; let approve = PlanReviewDecision::Approve.as_str(); let deny = PlanReviewDecision::Deny.as_str(); let decision_schema = EnumSchema::builder(vec![approve.into(), deny.into()]) .untitled() .with_default(deny) - .map_err(|e| format!("failed to build decision schema: {e}"))? + .map_err(|e| PlanError::DecisionSchema(e.clone()))? .build(); Ok(ElicitRequestParams::FormElicitationParams { @@ -223,7 +220,7 @@ impl PlanMcp { .required_enum_schema(DECISION, decision_schema) .optional_string(FEEDBACK) .build() - .map_err(|e| format!("failed to build schema: {e}"))?, + .map_err(|e| PlanError::SchemaBuild(e.to_string()))?, }) } } @@ -356,6 +353,24 @@ pub enum PlanError { #[error("Submit command `{program}` exited with {status}{}", stderr_suffix(.stderr))] SubmitCommandFailed { program: String, status: std::process::ExitStatus, stderr: String }, + + #[error("failed to serialize plan review metadata: {0}")] + SerializeMetadata(#[from] serde_json::Error), + + #[error("failed to build decision schema: {0}")] + DecisionSchema(String), + + #[error("failed to build schema: {0}")] + SchemaBuild(String), +} + +/// MCP tool handlers return `Result<_, String>` (required by rmcp). +/// `PlanError` carries richer, typed context internally and converts to a +/// `String` at that boundary so `?` works seamlessly inside handlers. +impl From for String { + fn from(error: PlanError) -> String { + error.to_string() + } } struct PlanName(String); diff --git a/crates/mcp-servers/src/subagents/tools/spawn_subagent/mod.rs b/crates/mcp-servers/src/subagents/tools/spawn_subagent/mod.rs index 1ded64be..5a224ec9 100644 --- a/crates/mcp-servers/src/subagents/tools/spawn_subagent/mod.rs +++ b/crates/mcp-servers/src/subagents/tools/spawn_subagent/mod.rs @@ -1,16 +1,18 @@ use crate::setup::McpBuilderExt; use aether_core::{ agent_spec::McpConfigSource, - core::{AgentBuilder, AgentDeps, AgentHandle, Prompt}, + core::{AgentBuilder, AgentDeps, AgentError, AgentHandle, AgentRegistryError, Prompt}, events::{AgentEvent, Command, MessageEvent, TurnEvent, TurnOutcome, UserCommand}, mcp::{McpSpawnResult, mcp, run_mcp_task::McpCommand}, }; use llm::ToolDefinition; +use mcp_utils::client::{McpError, ParseError}; use mcp_utils::display_meta::{ToolDisplayMeta, ToolResultMeta}; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use std::path::PathBuf; use std::sync::Arc; +use tokio::sync::mpsc::error::SendError; use tokio::{spawn, sync::mpsc}; /// Reference to a file artifact discovered or modified by a sub-agent @@ -42,27 +44,27 @@ pub struct StructuredAgentOutput { } impl StructuredAgentOutput { - /// Parse agent output into structured format with fallback for non-JSON responses - pub fn parse(raw_output: &str) -> Result { + /// Parse agent output into structured format with fallback for non-JSON responses. + pub fn parse(raw_output: &str) -> Self { // Try to parse as JSON directly if let Ok(parsed) = serde_json::from_str::(raw_output) { - return Ok(parsed); + return parsed; } if let Some(json_str) = extract_json_from_markdown(raw_output) && let Ok(parsed) = serde_json::from_str::(&json_str) { - return Ok(parsed); + return parsed; } // Fallback: wrap raw output in structured format - Ok(Self { + Self { summary: "Agent did not return structured output".to_string(), artifacts: vec![], decisions: vec![], next_steps: vec![], details: Some(raw_output.to_string()), - }) + } } } @@ -147,6 +149,34 @@ pub fn extract_json_from_markdown(text: &str) -> Option { Some(json_content.to_string()) } +/// Errors that can occur while executing a single sub-agent task. +#[derive(Debug, thiserror::Error)] +pub enum SubAgentError { + #[error("Failed to resolve agent: {0}")] + AgentNotResolved(#[from] AgentRegistryError), + + #[error("Failed to load MCP configs: {0}")] + McpConfigLoad(#[from] ParseError), + + #[error("Failed to spawn MCP manager: {0}")] + McpSpawn(#[from] McpError), + + #[error("MCP bootstrap aborted before completion")] + McpBootstrapAborted, + + #[error("Failed to build agent: {0}")] + AgentBuild(#[from] AgentError), + + #[error("Failed to send message to agent: {0}")] + MessageSend(#[from] SendError), + + #[error("Agent error: {message}")] + AgentFailed { message: String }, + + #[error("Agent cancelled")] + AgentCancelled, +} + /// Callback for receiving progress updates during agent execution pub type ProgressCallback = Box; @@ -217,18 +247,11 @@ impl AgentExecutor { async fn execute_single(&self, task_id: String, task: SubAgentTask) -> SubAgentResult { let agent_name = task.agent_name.clone(); - let result: Result = async { - let mut spec = self - .deps - .agent_registry - .resolve_agent_invocable(&task.agent_name) - .map_err(|error| error.to_string())?; + let result: Result = async { + let mut spec = self.deps.agent_registry.resolve_agent_invocable(&task.agent_name)?; let mut spawn_result = self.spawn_mcps(&spec.mcp_config_sources).await?; - let snapshot = spawn_result - .block_until_ready() - .await - .ok_or_else(|| "MCP bootstrap aborted before completion".to_string())?; + let snapshot = spawn_result.block_until_ready().await.ok_or(SubAgentError::McpBootstrapAborted)?; let filtered_tools = spec.tools.apply(snapshot.tool_definitions); spec.prompts.push(Prompt::McpInstructions(snapshot.instructions)); @@ -241,8 +264,7 @@ impl AgentExecutor { .send(Command::UserCommand(UserCommand::Text { content: vec![llm::ContentBlock::text(&prompt_with_instructions)], })) - .await - .map_err(|e| format!("Failed to send message to agent: {e}"))?; + .await?; if let Some(ref callback) = self.progress_callback { callback(&task_id, &agent_name, &AgentEvent::Turn(TurnEvent::Started { content: vec![] })); @@ -262,8 +284,10 @@ impl AgentExecutor { AgentEvent::Turn(TurnEvent::Ended { outcome }) => match outcome { TurnOutcome::Completed => return Ok(final_output), - TurnOutcome::Failed { error } => return Err(format!("Agent error: {error}")), - TurnOutcome::Cancelled => return Err("Agent cancelled".to_string()), + TurnOutcome::Failed { error } => { + return Err(SubAgentError::AgentFailed { message: error.clone() }); + } + TurnOutcome::Cancelled => return Err(SubAgentError::AgentCancelled), }, _ => {} @@ -282,23 +306,27 @@ impl AgentExecutor { output: Some(output), error: None, }, - Err(error) => { - SubAgentResult { task_id, agent_name, status: SubAgentStatus::Error, output: None, error: Some(error) } - } + Err(error) => SubAgentResult { + task_id, + agent_name, + status: SubAgentStatus::Error, + output: None, + error: Some(error.to_string()), + }, } } - async fn spawn_mcps(&self, effective_mcp_config_sources: &[McpConfigSource]) -> Result { + async fn spawn_mcps( + &self, + effective_mcp_config_sources: &[McpConfigSource], + ) -> Result { let mut builder = mcp(&self.project_root).with_builtin_servers(self.deps.clone()); if !effective_mcp_config_sources.is_empty() { - builder = builder - .from_mcp_config_sources(effective_mcp_config_sources) - .await - .map_err(|e| format!("Failed to load mcp configs: {e}"))?; + builder = builder.from_mcp_config_sources(effective_mcp_config_sources).await?; } - builder.spawn().await.map_err(|e| format!("Failed to spawn MCP manager: {e}")) + Ok(builder.spawn().await?) } async fn spawn_agent( @@ -306,14 +334,8 @@ impl AgentExecutor { spec: aether_core::agent_spec::AgentSpec, mcp_tx: mpsc::Sender, tools: Vec, - ) -> Result<(mpsc::Sender, mpsc::Receiver, AgentHandle), String> { - AgentBuilder::from_spec(&spec, vec![], &self.deps) - .await - .map_err(|e| format!("Failed to build agent from spec: {e}"))? - .tools(mcp_tx, tools) - .spawn() - .await - .map_err(|e| format!("Failed to spawn agent: {e}")) + ) -> Result<(mpsc::Sender, mpsc::Receiver, AgentHandle), SubAgentError> { + Ok(AgentBuilder::from_spec(&spec, vec![], &self.deps).await?.tools(mcp_tx, tools).spawn().await?) } } @@ -331,7 +353,7 @@ mod tests { "details": null }"#; - let result = StructuredAgentOutput::parse(json).expect("Should parse valid JSON"); + let result = StructuredAgentOutput::parse(json); assert_eq!(result.summary, "Found the main entry point"); assert_eq!(result.artifacts.len(), 1); @@ -357,7 +379,7 @@ mod tests { Hope this helps!"#; - let result = StructuredAgentOutput::parse(markdown).expect("Should parse JSON from markdown"); + let result = StructuredAgentOutput::parse(markdown); assert_eq!(result.summary, "Analyzed the codebase"); assert_eq!(result.decisions.len(), 1); @@ -368,7 +390,7 @@ Hope this helps!"#; fn test_parse_fallback() { let plain_text = "I analyzed the code and found several issues."; - let result = StructuredAgentOutput::parse(plain_text).expect("Should fallback gracefully"); + let result = StructuredAgentOutput::parse(plain_text); assert_eq!(result.summary, "Agent did not return structured output"); assert!(result.artifacts.is_empty());