chore: scheduled code-cleanup

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")]

crates/aether-core/src/mcp/run_mcp_task.rs

@@ -40,12 +40,12 @@ pub enum McpCommand {
         tx: mpsc::Sender<ToolExecutionEvent>,
     },
     ListPrompts {
-        tx: oneshot::Sender<Result<Vec<Prompt>, String>>,
+        tx: oneshot::Sender<Result<Vec<Prompt>, McpError>>,
     },
     GetPrompt {
         name: String,
         arguments: Option<serde_json::Map<String, serde_json::Value>>,
-        tx: oneshot::Sender<Result<GetPromptResult, String>>,
+        tx: oneshot::Sender<Result<GetPromptResult, McpError>>,
     },
     GetServerStatuses {
         tx: oneshot::Sender<Vec<McpServerStatusEntry>>,
@@ -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 } => {

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<CallToolRequestParams, String> {
-    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::<serde_json::Value>(&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(

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

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<T, E: std::fmt::Display> IntoMcpResult<T> for Result<T, E> {
     }
 }
 
+/// 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<CodingError> 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<RoleServer>,
         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<RoleServer>, command: &str) -> Result<(), String> {
+    async fn check_bash_permission(
+        &self,
+        context: &RequestContext<RoleServer>,
+        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<RoleServer>,
         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<String, String> {
-        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<String, CodingError> {
+        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<Json<ReadFileResult>, String> {
+    async fn read_and_track(&self, args: ReadFileArgs) -> Result<Json<ReadFileResult>, 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<T: CodingTools + 'static> CodingMcp<T> {
     /// 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<Json<ReadFileResult>, 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).

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<ElicitRequestParams, String> {
-        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<ElicitRequestParams, PlanError> {
+        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<PlanError> for String {
+    fn from(error: PlanError) -> String {
+        error.to_string()
+    }
 }
 
 struct PlanName(String);