chore(tools): improve tool implementation and fix bugs

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

@@ -1,4 +1,4 @@
-use crate::{Context, Error, Tool};
+use crate::{Context, Tool, util::ToolResult};
 
 mod check;
 mod expand;
@@ -8,11 +8,15 @@ use check::cargo_check;
 use expand::cargo_expand;
 use test::cargo_test;
 
-pub async fn run(ctx: Context, t: Tool) -> std::result::Result<String, Error> {
+pub async fn run(ctx: Context, t: Tool) -> ToolResult {
     match t.name.trim_start_matches("cargo_") {
         "check" => cargo_check(&ctx, t.opt("package")?).await,
-        "expand" => cargo_expand(&ctx, t.req("item")?, t.opt("package")?).await,
-        "test" => cargo_test(&ctx, t.opt("package")?, t.opt("testname")?).await,
+        "expand" => cargo_expand(&ctx, t.req("item")?, t.opt("package")?)
+            .await
+            .map(Into::into),
+        "test" => cargo_test(&ctx, t.opt("package")?, t.opt("testname")?)
+            .await
+            .map(Into::into),
         _ => Err(format!("Unknown tool '{}'", t.name).into()),
     }
 }

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

@@ -1,9 +1,9 @@
 use duct::cmd;
 use jp_tool::Context;
 
-use crate::Result;
+use crate::util::{ToolResult, error};
 
-pub(crate) async fn cargo_check(ctx: &Context, package: Option<String>) -> Result<String> {
+pub(crate) async fn cargo_check(ctx: &Context, package: Option<String>) -> ToolResult {
     let package = package.map_or("--workspace".to_owned(), |v| format!("--package={v}"));
     let result = cmd!(
         "cargo",
@@ -23,12 +23,11 @@ pub(crate) async fn cargo_check(ctx: &Context, package: Option<String>) -> Resul
 
     let code = result.status.code().unwrap_or(0);
     if code != 0 {
-        return Err(format!(
+        return error(format!(
             "Cargo command failed ({}): {}",
             result.status.code().unwrap_or(1),
             String::from_utf8_lossy(&result.stderr)
-        )
-        .into());
+        ));
     }
 
     let content = String::from_utf8_lossy(&result.stderr);
@@ -38,13 +37,14 @@ pub(crate) async fn cargo_check(ctx: &Context, package: Option<String>) -> Resul
     let content = content.trim();
 
     if content.is_empty() {
-        Ok("Check succeeded. No warnings or errors found.".to_owned())
+        Ok("Check succeeded. No warnings or errors found.".into())
     } else {
         Ok(indoc::formatdoc! {"
         ```
         {content}
         ```
-    "})
+    "}
+        .into())
     }
 }
 
@@ -79,7 +79,7 @@ mod tests {
 
         let result = cargo_check(&ctx, None).await.unwrap();
 
-        assert_eq!(result, indoc::indoc! {r#"
+        assert_eq!(result.into_content().unwrap(), indoc::indoc! {r#"
             ```
             warning: unused `std::result::Result` that must be used
              --> src/main.rs:2:5

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

@@ -157,30 +157,15 @@ pub(crate) async fn fs_modify_file(
         let after = if replace_using_regex {
             contents.replace_using_regexp(&string_to_replace, &new_string)?
         } else {
-            let (start_byte, mut end_byte) = contents
+            let (start_byte, end_byte) = contents
                 .find_pattern_range(&string_to_replace)
                 .ok_or("Cannot find pattern to replace")?;
 
-            // Check if pattern is followed by a newline
-            let followed_by_newline =
-                end_byte < contents.len() && contents.as_bytes()[end_byte] == b'\n';
-
-            // If followed by newline, consume it
-            if followed_by_newline {
-                end_byte += 1;
-            }
-
             // Replace the pattern with new string
             let mut new_content = String::new();
             new_content.push_str(&contents[..start_byte]);
             new_content.push_str(&new_string);
 
-            // If we consumed a newline but replacement doesn't end with one, add it
-            // back
-            if followed_by_newline && !new_string.ends_with('\n') {
-                new_content.push('\n');
-            }
-
             new_content.push_str(&contents[end_byte..]);
             new_content
         };
@@ -519,6 +504,73 @@ mod tests {
         assert_eq!(&fs::read_to_string(&absolute_file_path).unwrap(), result,);
     }
 
+    #[tokio::test]
+    #[test_log::test]
+    async fn test_issue_with_newlines_at_end() {
+        let string_to_replace = "use crossterm::style::Stylize as _;\nuse inquire::Confirm;\nuse \
+                                 jp_conversation::{Conversation, ConversationId, \
+                                 ConversationStream};\nuse \
+                                 jp_format::conversation::DetailsFmt;\nuse jp_printer::Printable \
+                                 as _;\n\nuse crate::{Output, cmd::Success, ctx::Ctx};\n";
+
+        let new_string = "use crossterm::style::Stylize as _;\nuse inquire::Confirm;\nuse \
+                          jp_conversation::{Conversation, ConversationId, \
+                          ConversationStream};\nuse jp_format::conversation::DetailsFmt;\n\nuse \
+                          crate::{Output, cmd::Success, ctx::Ctx};\n";
+
+        let source = indoc! {"
+            use crossterm::style::Stylize as _;
+            use inquire::Confirm;
+            use jp_conversation::{Conversation, ConversationId, ConversationStream};
+            use jp_format::conversation::DetailsFmt;
+            use jp_printer::Printable as _;
+
+            use crate::{Output, cmd::Success, ctx::Ctx};
+
+            #[derive(Debug, clap::Args)]
+            pub(crate) struct Rm {
+                /// Conversation IDs to remove."};
+
+        let result = indoc! {"
+            use crossterm::style::Stylize as _;
+            use inquire::Confirm;
+            use jp_conversation::{Conversation, ConversationId, ConversationStream};
+            use jp_format::conversation::DetailsFmt;
+
+            use crate::{Output, cmd::Success, ctx::Ctx};
+
+            #[derive(Debug, clap::Args)]
+            pub(crate) struct Rm {
+                /// Conversation IDs to remove."};
+
+        // Create root directory.
+        let temp_dir = tempdir().unwrap();
+        let root = temp_dir.path().to_path_buf();
+
+        // Create file to be modified.
+        let file_path = "test.txt";
+        let absolute_file_path = root.join(file_path);
+        fs::write(&absolute_file_path, source).unwrap();
+
+        let ctx = Context {
+            root,
+            format_parameters: false,
+        };
+
+        let _actual = fs_modify_file(
+            ctx,
+            &Map::new(),
+            file_path.to_owned(),
+            string_to_replace.to_owned(),
+            new_string.to_owned(),
+            false,
+        )
+        .await
+        .map_err(|e| e.to_string());
+
+        assert_eq!(&fs::read_to_string(&absolute_file_path).unwrap(), result);
+    }
+
     #[tokio::test]
     #[test_log::test]
     async fn test_modify_file_replace_with_regexp() {

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

@@ -21,14 +21,37 @@ pub(crate) async fn fs_read_file(
         .unwrap_or_default();
 
     let mut contents = std::fs::read_to_string(&absolute_path)?;
+    let lines = contents.split('\n').count();
+
+    if start_line.is_some_and(|v| v > end_line.unwrap_or(usize::MAX)) {
+        return error("`start_line` must be less than or equal to `end_line`.");
+    } else if start_line.is_some_and(|v| v == 0) {
+        return error("`start_line` must be greater than 0.");
+    } else if end_line.is_some_and(|v| v == 0) {
+        return error("`end_line` must be greater than 0.");
+    } else if start_line.is_some_and(|v| v > contents.lines().count()) {
+        return error(format!(
+            "`start_line` is greater than the number of lines in the file ({lines})."
+        ));
+    }
 
     if let Some(start_line) = start_line {
-        contents = contents.split('\n').skip(start_line).collect::<String>();
+        contents = contents
+            .split('\n')
+            .skip(start_line - 1)
+            .collect::<Vec<_>>()
+            .join("\n");
+
         contents.insert_str(0, &format!("... (starting from line #{start_line}) ...\n"));
     }
 
     if let Some(end_line) = end_line {
-        contents = contents.split('\n').take(end_line).collect::<String>();
+        contents = contents
+            .split('\n')
+            .take(end_line)
+            .collect::<Vec<_>>()
+            .join("\n");
+
         contents.push_str(&format!("\n... (truncated after line #{end_line}) ..."));
     }
 
@@ -39,3 +62,82 @@ pub(crate) async fn fs_read_file(
     "}
     .into())
 }
+
+#[cfg(test)]
+mod tests {
+    use jp_tool::Outcome;
+
+    use super::*;
+
+    #[tokio::test]
+    async fn test_fs_read_file() {
+        struct TestCase {
+            file_contents: String,
+            start_line: Option<usize>,
+            end_line: Option<usize>,
+            expected: String,
+        }
+
+        let cases = vec![
+            ("all content", TestCase {
+                file_contents: "foo\nbar\nbaz\n".to_owned(),
+                start_line: None,
+                end_line: None,
+                expected: "```txt\nfoo\nbar\nbaz\n\n```\n".to_owned(),
+            }),
+            ("start line", TestCase {
+                file_contents: "foo\nbar\nbaz\n".to_owned(),
+                start_line: Some(2),
+                end_line: None,
+                expected: "```txt\n... (starting from line #2) ...\nbar\nbaz\n\n```\n".to_owned(),
+            }),
+            ("end line", TestCase {
+                file_contents: "foo\nbar\nbaz\n".to_owned(),
+                start_line: None,
+                end_line: Some(2),
+                expected: "```txt\nfoo\nbar\n... (truncated after line #2) ...\n```\n".to_owned(),
+            }),
+            ("start and end line", TestCase {
+                file_contents: "foo\nbar\nbaz\n\n".to_owned(),
+                start_line: Some(2),
+                end_line: Some(2),
+                expected: "```txt\n... (starting from line #2) ...\nbar\n... (truncated after \
+                           line #2) ...\n```\n"
+                    .to_owned(),
+            }),
+        ];
+
+        for (
+            name,
+            TestCase {
+                file_contents,
+                start_line,
+                end_line,
+                expected,
+            },
+        ) in cases
+        {
+            let tmp = tempfile::tempdir().unwrap();
+            let path = tmp.path().join("file.txt");
+
+            std::fs::write(&path, file_contents).unwrap();
+
+            let result = fs_read_file(
+                tmp.path().to_owned(),
+                "file.txt".to_owned(),
+                start_line,
+                end_line,
+            )
+            .await
+            .unwrap();
+
+            let out = match result {
+                Outcome::Success { content } => content,
+                Outcome::Error { message, .. } => message,
+                Outcome::NeedsInput { .. } => String::new(),
+            };
+
+            assert_eq!(out, expected, "failed test case '{name}'");
+        }
+    }
+}

.config/jp/tools/src/fs/snapshots/modify_file_replace_word@replace_first_line_with_multiple_lines.snap

@@ -6,8 +6,9 @@ File modified successfully:
 ```diff
 --- test.txt
 +++ test.txt
-@@ -1 +1,2 @@
+@@ -1 +1,3 @@
 -hello world
 +hello
 +world
++
 ```

.config/jp/tools/src/lib.rs

@@ -15,7 +15,7 @@ type Result<T> = std::result::Result<T, Error>;
 
 pub async fn run(ctx: Context, t: Tool) -> Result<Outcome> {
     match t.name.as_str() {
-        s if s.starts_with("cargo_") => cargo::run(ctx, t).await.map(Into::into),
+        s if s.starts_with("cargo_") => cargo::run(ctx, t).await,
         s if s.starts_with("github_") => github::run(ctx, t).await.map(Into::into),
         s if s.starts_with("fs_") => fs::run(ctx, t).await,
         s if s.starts_with("web_") => web::run(ctx, t).await.map(Into::into),

deny.toml

@@ -1,6 +1,7 @@
 [advisories]
 ignore = [
     { id = "RUSTSEC-2024-0436", reason = "paste is unmaintained, but stable" },
+    { id = "RUSTSEC-2025-0141", reason = "The team considers version 1.3.3 a complete version of bincode that is not in need of any updates." },
 ]
 
 [licenses]