chore(tools, mcp): Improve error handling and output formatting (#373)

Refactor internal maintenance tools to use a consistent `Outcome` type
for errors and successes. This allows for better structured output when
tools fail or require user input.

Improve the user experience in the CLI by updating MCP tool definitions
to support inline result rendering and custom parameter formatting.
Specific tools like `fs_create_file` now provide a preview of the
content to be created with syntax highlighting.

Adjust `cargo_test` to only use one `--cargo-quiet` flag, ensuring that
compilation errors are still visible to the LLM when tests fail to run.

Signed-off-by: Jean Mertz <[email protected]>

Author: JeanMertz

.config/jp/tools/src/cargo/test.rs

@@ -29,8 +29,7 @@ pub(crate) async fn cargo_test(
         "nextest",
         "run",
         package,
-        // Twice to silence Cargo completely.
-        "--cargo-quiet",
+        // Once to still print any compilation errors.
         "--cargo-quiet",
         // Run all tests, even if one fails.
         "--no-fail-fast",
@@ -92,7 +91,8 @@ pub(crate) async fn cargo_test(
 
     if ran_tests == 0 {
         return Err(format!(
-            "Unable to find any tests. Are the package and test name correct?\n\n{stderr}"
+            "Unable to run any tests. This can be due to compilation issues, or incorrect package \
+             or test name:\n\n{stderr}"
         ))?;
     }
 

.config/jp/tools/src/fs.rs

@@ -22,14 +22,15 @@ pub async fn run(ctx: Context, t: Tool) -> std::result::Result<Outcome, Error> {
             .and_then(to_xml)
             .map(Into::into),
 
-        "read_file" => fs_read_file(
-            ctx.root,
-            t.req("path")?,
-            t.opt("start_line")?,
-            t.opt("end_line")?,
-        )
-        .await
-        .map(Into::into),
+        "read_file" => {
+            fs_read_file(
+                ctx.root,
+                t.req("path")?,
+                t.opt("start_line")?,
+                t.opt("end_line")?,
+            )
+            .await
+        }
 
         "grep_files" => fs_grep_files(
             ctx.root,
@@ -49,9 +50,7 @@ pub async fn run(ctx: Context, t: Tool) -> std::result::Result<Outcome, Error> {
         .await
         .map(Into::into),
 
-        "create_file" => {
-            fs_create_file(ctx.root, &t.answers, t.req("path")?, t.opt("content")?).await
-        }
+        "create_file" => fs_create_file(ctx, &t.answers, t.req("path")?, t.opt("content")?).await,
 
         "delete_file" => fs_delete_file(ctx.root, &t.answers, t.req("path")?).await,
 

.config/jp/tools/src/fs/create_file.rs

@@ -7,38 +7,63 @@ use std::{
 use jp_tool::{AnswerType, Outcome, Question};
 use serde_json::{Map, Value};
 
-use crate::Error;
+use crate::{
+    Context,
+    util::{ToolResult, error, fail},
+};
 
 pub(crate) async fn fs_create_file(
-    root: PathBuf,
+    ctx: Context,
     answers: &Map<String, Value>,
     path: String,
     content: Option<String>,
-) -> std::result::Result<Outcome, Error> {
+) -> ToolResult {
+    if ctx.format_parameters {
+        let lang = match path.split('.').next_back().unwrap_or_default() {
+            "rs" => "rust",
+            "js" => "javascript",
+            "ts" => "typescript",
+            "c" => "c",
+            "cpp" => "cpp",
+            "go" => "go",
+            "php" => "php",
+            "py" => "python",
+            "rb" => "ruby",
+            lang => lang,
+        };
+
+        let mut response = format!("Create file '{path}'");
+        if let Some(content) = content {
+            response.push_str(&format!(" with content:\n\n```{lang}\n{content}\n```"));
+        }
+
+        return Ok(response.into());
+    }
+
     let p = PathBuf::from(&path);
 
     if p.is_absolute() {
-        return Err("Path must be relative.".into());
+        return error("Path must be relative.");
     }
 
     if p.iter().any(|c| c.len() > 30) {
-        return Err("Individual path components must be less than 30 characters long.".into());
+        return error("Individual path components must be less than 30 characters long.");
     }
 
     if p.iter().count() > 20 {
-        return Err("Path must be less than 20 components long.".into());
+        return error("Path must be less than 20 components long.");
     }
 
-    let absolute_path = root.join(path.trim_start_matches('/'));
+    let absolute_path = ctx.root.join(path.trim_start_matches('/'));
     if absolute_path.is_dir() {
-        return Err("Path is an existing directory.".into());
+        return error("Path is an existing directory.");
     }
 
     if absolute_path.exists() {
         match answers.get("overwrite_file").and_then(Value::as_bool) {
             Some(true) => {}
             Some(false) => {
-                return Err("Path points to existing file".into());
+                return error("Path points to existing file");
             }
             None => {
                 return Ok(Outcome::NeedsInput {
@@ -54,7 +79,7 @@ pub(crate) async fn fs_create_file(
     }
 
     let Some(parent) = absolute_path.parent() else {
-        return Err("Path has no parent".into());
+        return fail("Path has no parent");
     };
 
     fs::create_dir_all(parent)?;

.config/jp/tools/src/fs/modify_file.rs

@@ -18,7 +18,10 @@ use serde_json::{Map, Value};
 use similar::{ChangeTag, TextDiff, udiff::UnifiedDiff};
 
 use super::utils::is_file_dirty;
-use crate::{Context, Error};
+use crate::{
+    Context, Error,
+    util::{ToolResult, error, fail},
+};
 
 pub struct Change {
     pub path: PathBuf,
@@ -112,23 +115,23 @@ pub(crate) async fn fs_modify_file(
     string_to_replace: String,
     new_string: String,
     replace_using_regex: bool,
-) -> std::result::Result<Outcome, Error> {
+) -> ToolResult {
     if string_to_replace == new_string {
-        return Err("String to replace is the same as the new string.".into());
+        return error("String to replace is the same as the new string.");
     }
 
     let p = PathBuf::from(&path);
 
     if p.is_absolute() {
-        return Err("Path must be relative.".into());
+        return error("Path must be relative.");
     }
 
     if p.iter().any(|c| c.len() > 30) {
-        return Err("Individual path components must be less than 30 characters long.".into());
+        return error("Individual path components must be less than 30 characters long.");
     }
 
     if p.iter().count() > 20 {
-        return Err("Path must be less than 20 components long.".into());
+        return error("Path must be less than 20 components long.");
     }
 
     let absolute_path = ctx.root.join(path.trim_start_matches('/'));
@@ -137,15 +140,15 @@ pub(crate) async fn fs_modify_file(
     for entry in glob::glob(&absolute_path.to_string_lossy())? {
         let entry = entry?;
         if !entry.exists() {
-            return Err("File does not exist.".into());
+            return error("File does not exist.");
         }
 
         if !entry.is_file() {
-            return Err("Path is not a regular file.".into());
+            return error("Path is not a regular file.");
         }
 
         let Ok(path) = entry.strip_prefix(&ctx.root) else {
-            return Err("Path is not within workspace root.".into());
+            return fail("Path is not within workspace root.");
         };
 
         let before = fs::read_to_string(&entry)?;
@@ -190,23 +193,23 @@ pub(crate) async fn fs_modify_file(
     }
 
     if ctx.format_parameters {
-        Ok(format_changes(changes, &ctx.root).into())
+        Ok(format_changes(changes).into())
     } else {
         apply_changes(changes, &ctx.root, answers)
     }
 }
 
-fn format_changes(changes: Vec<Change>, root: &Path) -> String {
+fn format_changes(changes: Vec<Change>) -> String {
     changes
         .into_iter()
         .map(|change| {
-            let path = root.join(change.path.to_string_lossy().trim_start_matches('/'));
+            let path = change.path.to_string_lossy();
 
             let diff = text_diff(&change.before, &change.after);
-            let unified = unified_diff(&diff, &path.display().to_string());
+            let unified = unified_diff(&diff, &path);
             let colored = colored_diff(&diff, &unified);
 
-            format!("{}:\n\n```diff\n{colored}\n```", path.display())
+            format!("{path}:\n\n```diff\n{colored}\n```")
         })
         .collect::<Vec<_>>()
         .join("\n\n")

.config/jp/tools/src/fs/read_file.rs

@@ -1,18 +1,18 @@
 use std::{ffi::OsStr, path::PathBuf};
 
-use crate::Error;
+use crate::util::{ToolResult, error};
 
 pub(crate) async fn fs_read_file(
     root: PathBuf,
     path: String,
     start_line: Option<usize>,
     end_line: Option<usize>,
-) -> std::result::Result<String, Error> {
+) -> ToolResult {
     let absolute_path = root.join(path.trim_start_matches('/'));
     if !absolute_path.exists() {
-        return Err("File not found.".into());
+        return error("File not found.");
     } else if !absolute_path.is_file() {
-        return Err("Path is not a file.".into());
+        return error("Path is not a file.");
     }
 
     let ext = absolute_path
@@ -36,5 +36,6 @@ pub(crate) async fn fs_read_file(
         ```{ext}
         {contents}
         ```
-    "})
+    "}
+    .into())
 }

.config/jp/tools/src/main.rs

@@ -16,16 +16,16 @@ async fn main() {
         Err(error) => return println!("{error}"),
     };
 
-    let format_parameters = context.format_parameters;
     let name = tool.name.clone();
-    match run(context, tool).await {
-        Ok(Outcome::Success { content }) if format_parameters => println!("{content}"),
-        Ok(outcome) => match serde_json::to_string(&outcome) {
-            Ok(content) => println!("{content}"),
-            Err(error) => handle_error(&error, &name),
-        },
-        Err(error) => handle_error(error.as_ref(), &name),
-    }
+    let result = run(context, tool)
+        .await
+        .unwrap_or_else(|error| error_outcome(error.as_ref(), &name));
+
+    let json = serde_json::to_string(&result).unwrap_or_else(|error| {
+        format!(r#"{{"type":"error","message":"Unable to serialize result: {error}","trace":[],"transient":false}}"#)
+    });
+
+    println!("{json}");
 }
 
 fn input<T: serde::de::DeserializeOwned>(index: usize, name: &str) -> Result<T, String> {
@@ -45,19 +45,17 @@ fn input<T: serde::de::DeserializeOwned>(index: usize, name: &str) -> Result<T,
         .map_err(|error| format!("```json\n{error:#}\n```"))
 }
 
-fn handle_error(error: &dyn std::error::Error, name: &str) {
-    let mut sources = vec![];
+fn error_outcome(error: &dyn std::error::Error, name: &str) -> Outcome {
+    let mut trace = vec![];
     let mut source = Some(error);
     while let Some(error) = source {
-        sources.push(format!("{error:#}"));
+        trace.push(format!("{error:#}"));
         source = error.source();
     }
 
-    println!(
-        "```json\n{:#}\n```",
-        json!({
-            "error": format!("An error occurred while running the '{name}' tool."),
-            "trace": sources,
-        })
-    );
+    Outcome::Error {
+        message: format!("An error occurred while running the '{name}' tool."),
+        trace,
+        transient: true,
+    }
 }

.config/jp/tools/src/util.rs

@@ -1,5 +1,18 @@
+use jp_tool::Outcome;
 use serde::{Deserialize, Serialize};
 
+pub type ToolResult = std::result::Result<Outcome, Box<dyn std::error::Error + Send + Sync>>;
+
+#[expect(clippy::unnecessary_wraps)]
+pub fn error(error: impl Into<Box<dyn std::error::Error + Send + Sync>>) -> ToolResult {
+    Ok(Outcome::error(error.into().as_ref()))
+}
+
+#[expect(clippy::unnecessary_wraps)]
+pub fn fail(error: impl Into<Box<dyn std::error::Error + Send + Sync>>) -> ToolResult {
+    Ok(Outcome::fail(error.into().as_ref()))
+}
+
 #[derive(Serialize, Deserialize)]
 #[serde(untagged)]
 pub enum OneOrMany<T> {

.jp/mcp/tools/cargo/check.toml

@@ -1,12 +1,15 @@
 [conversation.tools.cargo_check]
 enable = false
 run = "unattended"
-style.inline_results = "off"
-
 source = "local"
 command = "just serve-tools {{context}} {{tool}}"
 description = "Run `cargo check` for the given package, validating if the code compiles."
 
+[conversation.tools.cargo_check.style]
+inline_results = "full"
+results_file_link = "off"
+parameters = "function_call"
+
 [conversation.tools.cargo_check.parameters.package]
 description = "Package to run check for, if unspecified, all workspace packages will be checked."
 type = "string"

.jp/mcp/tools/cargo/test.toml

@@ -1,12 +1,15 @@
 [conversation.tools.cargo_test]
 enable = false
 run = "unattended"
-style.inline_results = "off"
-
 source = "local"
 command = "just serve-tools {{context}} {{tool}}"
 description = "Execute all unit and integration tests and build examples of the project."
 
+[conversation.tools.cargo_test.style]
+inline_results = "full"
+results_file_link = "off"
+parameters = "function_call"
+
 [conversation.tools.cargo_test.parameters.package]
 description = "Package to run tests for, if unspecified, all workspace packages will be tested."
 type = "string"

.jp/mcp/tools/fs/create_file.toml

@@ -6,6 +6,11 @@ description = """
 Create a new file in the project's local filesystem.
 """
 
+[conversation.tools.fs_create_file.style]
+parameters = "just serve-tools {{context}} {{tool}}"
+inline_results = "off"
+results_file_link = "off"
+
 [conversation.tools.fs_create_file.parameters.path]
 type = "string"
 required = true