Refactor Errors to use thiserror and anyhow

Cargo.toml

@@ -34,6 +34,8 @@ nix = { version = "0.31", features = ["signal"] }
 dirs = "6.0.0"
 shell-words = "1.1.1"
 chrono = { version = "0.4.42", default-features = false, features = ["clock", "std"] }
+thiserror = "2.0.18"
+anyhow = "1.0.102"
 
 [dev-dependencies]
 tempfile = "3.27.0"

src/allowlist.rs

@@ -5,48 +5,51 @@ use std::fs;
 use std::path::{Path, PathBuf};
 
 /// Returns the path to the active allowlist.toml.
-fn allowlist_path() -> Result<PathBuf, String> {
+fn allowlist_path() -> anyhow::Result<PathBuf> {
     active_allowlist_path()
 }
 
 /// Load the allowlist from the active allowlist path.
 /// If the file does not exist, return an empty allowlist.
-pub fn load_allowlist() -> Result<Allowlist, String> {
+pub fn load_allowlist() -> anyhow::Result<Allowlist> {
     let path = allowlist_path()?;
     let dela_dir = active_dela_config_dir()?;
 
     // Check if the dela config directory exists
     if !dela_dir.exists() {
-        return Err("Dela is not initialized. Please run 'dela init' first.".to_string());
+        return Err(anyhow::anyhow!(
+            "Dela is not initialized. Please run 'dela init' first."
+        ));
     }
 
     // If allowlist file doesn't exist but the config directory does, return empty allowlist
     if !path.exists() {
         return Ok(Allowlist::default());
     }
 
-    let contents =
-        fs::read_to_string(&path).map_err(|e| format!("Failed to read allowlist file: {}", e))?;
+    let contents = fs::read_to_string(&path)
+        .map_err(|e| anyhow::anyhow!("Failed to read allowlist file: {}", e))?;
 
     match toml::from_str::<Allowlist>(&contents) {
         Ok(allowlist) => Ok(allowlist),
-        Err(e) => Err(format!("Failed to parse allowlist TOML: {}", e)),
+        Err(e) => Err(anyhow::anyhow!("Failed to parse allowlist TOML: {}", e)),
     }
 }
 
 /// Save the allowlist to ~/.config/dela/allowlist.toml.
-pub fn save_allowlist(allowlist: &Allowlist) -> Result<(), String> {
+pub fn save_allowlist(allowlist: &Allowlist) -> anyhow::Result<()> {
     let path = preferred_allowlist_path()?;
 
     // Create the config directory if it doesn't exist
     if let Some(parent) = path.parent() {
         fs::create_dir_all(parent)
-            .map_err(|e| format!("Failed to create dela config directory: {}", e))?;
+            .map_err(|e| anyhow::anyhow!("Failed to create dela config directory: {}", e))?;
     }
 
     let toml = toml::to_string_pretty(&allowlist)
-        .map_err(|e| format!("Failed to serialize allowlist: {}", e))?;
-    fs::write(&path, toml).map_err(|e| format!("Failed to create allowlist file: {}", e))?;
+        .map_err(|e| anyhow::anyhow!("Failed to serialize allowlist: {}", e))?;
+    fs::write(&path, toml)
+        .map_err(|e| anyhow::anyhow!("Failed to create allowlist file: {}", e))?;
     Ok(())
 }
 
@@ -126,7 +129,7 @@ pub fn allowlist_entry_for_task(task: &Task, scope: AllowScope) -> AllowlistEntr
 /// Check if a given task is explicitly allowed or denied by the allowlist
 /// Returns (explicitly_allowed, explicitly_denied) - both false means not found in allowlist
 /// This is the core allowlist checking logic without any prompting or persistence
-pub fn is_task_allowed(task: &Task) -> Result<(bool, bool), String> {
+pub fn is_task_allowed(task: &Task) -> anyhow::Result<(bool, bool)> {
     // Only proceed with allowlist operations if dela is initialized
     let allowlist = load_allowlist()?;
     Ok(match evaluate_task_against_allowlist(task, &allowlist) {
@@ -138,7 +141,7 @@ pub fn is_task_allowed(task: &Task) -> Result<(bool, bool), String> {
 
 /// Check if a given task is allowed, based on the loaded allowlist
 /// If the task is not in the allowlist, prompt the user for a decision
-pub fn check_task_allowed(task: &Task) -> Result<bool, String> {
+pub fn check_task_allowed(task: &Task) -> anyhow::Result<bool> {
     // Check if task is explicitly allowed or denied
     let (explicitly_allowed, explicitly_denied) = is_task_allowed(task)?;
 
@@ -183,7 +186,7 @@ pub fn check_task_allowed(task: &Task) -> Result<bool, String> {
 }
 
 /// Check if a given task is allowed with a specific scope, without prompting
-pub fn check_task_allowed_with_scope(task: &Task, scope: AllowScope) -> Result<bool, String> {
+pub fn check_task_allowed_with_scope(task: &Task, scope: AllowScope) -> anyhow::Result<bool> {
     // Only proceed with allowlist operations if dela is initialized
     let mut allowlist = load_allowlist()?;
 

src/commands/allow_command.rs

@@ -2,23 +2,28 @@ use crate::allowlist;
 use crate::config::preferred_allowlist_path;
 use crate::task_discovery;
 use crate::types::AllowScope;
+#[allow(unused_imports)]
+use anyhow::Context;
 use std::env;
 
-pub fn execute(task_with_args: &str, allow: Option<u8>) -> Result<(), String> {
+pub fn execute(task_with_args: &str, allow: Option<u8>) -> anyhow::Result<()> {
     let task_name = task_with_args
         .split_whitespace()
         .next()
-        .ok_or_else(|| "No task name provided".to_string())?;
+        .context("No task name provided")?;
 
-    let current_dir =
-        env::current_dir().map_err(|e| format!("Failed to get current directory: {}", e))?;
+    let current_dir = env::current_dir()
+        .map_err(|e| anyhow::anyhow!("Failed to get current directory: {}", e))?;
     let discovered = task_discovery::discover_tasks(&current_dir);
 
     // Find all tasks with the given name (both original and disambiguated)
     let matching_tasks = task_discovery::get_matching_tasks(&discovered, task_name);
 
     match matching_tasks.len() {
-        0 => Err(format!("dela: command or task not found: {}", task_name)),
+        0 => Err(anyhow::anyhow!(
+            "dela: command or task not found: {}",
+            task_name
+        )),
         1 => {
             // Single task found, check allowlist
             let task = matching_tasks[0];
@@ -40,13 +45,13 @@ pub fn execute(task_with_args: &str, allow: Option<u8>) -> Result<(), String> {
                     }
                     5 => {
                         eprintln!("Task '{}' was denied by the allowlist.", task.name);
-                        Err(format!(
+                        Err(anyhow::anyhow!(
                             "Dela task '{}' was denied by the {}",
                             task.name,
                             preferred_allowlist_path()?.display()
                         ))
                     }
-                    _ => Err(format!(
+                    _ => Err(anyhow::anyhow!(
                         "Invalid allow choice {}. Please use a number between 2 and 5.",
                         choice
                     )),
@@ -55,7 +60,7 @@ pub fn execute(task_with_args: &str, allow: Option<u8>) -> Result<(), String> {
                 // Otherwise, use the interactive prompt
                 if !allowlist::check_task_allowed(task)? {
                     eprintln!("Task '{}' was denied by the allowlist.", task.name);
-                    return Err(format!(
+                    return Err(anyhow::anyhow!(
                         "Dela task '{}' was denied by the {}",
                         task.name,
                         preferred_allowlist_path()?.display()
@@ -68,7 +73,10 @@ pub fn execute(task_with_args: &str, allow: Option<u8>) -> Result<(), String> {
             // Multiple tasks found, print error and list them
             let error_msg = task_discovery::format_ambiguous_task_error(task_name, &matching_tasks);
             eprintln!("{}", error_msg);
-            Err(format!("Multiple tasks named '{}' found", task_name))
+            Err(anyhow::anyhow!(
+                "Multiple tasks named '{}' found",
+                task_name
+            ))
         }
     }
 }
@@ -189,7 +197,7 @@ test: ## Running tests
             let result = execute("test", None);
             assert!(result.is_err(), "Should fail when task is denied");
             assert_eq!(
-                result.unwrap_err(),
+                result.unwrap_err().to_string(),
                 format!(
                     "Dela task 'test' was denied by the {}",
                     preferred_allowlist_path_for(home_dir.path()).display()
@@ -209,7 +217,7 @@ test: ## Running tests
         let result = execute("nonexistent", None);
         assert!(result.is_err(), "Should fail when no task found");
         assert_eq!(
-            result.unwrap_err(),
+            result.unwrap_err().to_string(),
             "dela: command or task not found: nonexistent"
         );
 
@@ -242,7 +250,7 @@ test: ## Running tests
         let result = execute("test", Some(5));
         assert!(result.is_err(), "Should fail with allow=5");
         assert_eq!(
-            result.unwrap_err(),
+            result.unwrap_err().to_string(),
             format!(
                 "Dela task 'test' was denied by the {}",
                 preferred_allowlist_path_for(home_dir.path()).display()
@@ -253,15 +261,15 @@ test: ## Running tests
         let result = execute("test", Some(1));
         assert!(result.is_err(), "Should fail with allow=1");
         assert_eq!(
-            result.unwrap_err(),
+            result.unwrap_err().to_string(),
             "Invalid allow choice 1. Please use a number between 2 and 5."
         );
 
         // Test with out of range allow option
         let result = execute("test", Some(6));
         assert!(result.is_err(), "Should fail with allow=6");
         assert_eq!(
-            result.unwrap_err(),
+            result.unwrap_err().to_string(),
             "Invalid allow choice 6. Please use a number between 2 and 5."
         );
 
@@ -300,7 +308,7 @@ test: ## Running tests
             "Should fail when the dela config directory doesn't exist"
         );
         assert_eq!(
-            result.unwrap_err(),
+            result.unwrap_err().to_string(),
             "Dela is not initialized. Please run 'dela init' first."
         );
 
@@ -329,7 +337,7 @@ test: ## Running tests
         let result = execute("test --verbose --coverage", Some(5));
         assert!(result.is_err(), "Should fail with allow=5 and arguments");
         assert_eq!(
-            result.unwrap_err(),
+            result.unwrap_err().to_string(),
             format!(
                 "Dela task 'test' was denied by the {}",
                 preferred_allowlist_path_for(home_dir.path()).display()
@@ -370,6 +378,7 @@ test: ## Running tests
         assert!(
             result
                 .unwrap_err()
+                .to_string()
                 .contains("Multiple tasks named 'test' found"),
             "Error should mention multiple tasks"
         );

src/commands/configure_shell.rs

@@ -1,4 +1,6 @@
 use crate::environment::get_current_shell;
+#[allow(unused_imports)]
+use anyhow::Context;
 
 const ZSH_CONFIG: &str = include_str!("../../resources/zsh.sh");
 const BASH_CONFIG: &str = include_str!("../../resources/bash.sh");
@@ -15,12 +17,12 @@ enum Shell {
 }
 
 impl Shell {
-    fn from_path(path: &str) -> Result<Shell, String> {
+    fn from_path(path: &str) -> anyhow::Result<Shell> {
         let shell_path = std::path::PathBuf::from(path);
         let shell_name = shell_path
             .file_name()
             .and_then(|name| name.to_str())
-            .ok_or_else(|| "Invalid shell path".to_string())?;
+            .context("Invalid shell path")?;
 
         match shell_name {
             "zsh" => Ok(Shell::Zsh),
@@ -32,9 +34,9 @@ impl Shell {
     }
 }
 
-pub fn execute() -> Result<(), String> {
+pub fn execute() -> anyhow::Result<()> {
     // Get the current shell from environment
-    let shell = get_current_shell().ok_or("SHELL environment variable not set".to_string())?;
+    let shell = get_current_shell().context("SHELL environment variable not set")?;
 
     // Parse the shell type
     let shell_type = Shell::from_path(&shell)?;
@@ -57,7 +59,7 @@ pub fn execute() -> Result<(), String> {
             print!("{}", PWSH_CONFIG);
             Ok(())
         }
-        Shell::Unknown(name) => Err(format!("Unsupported shell: {}", name)),
+        Shell::Unknown(name) => Err(anyhow::anyhow!("Unsupported shell: {}", name)),
     }
 }
 
@@ -114,7 +116,10 @@ mod tests {
         setup_test_env("/bin/unknown");
         let result = execute();
         assert!(result.is_err());
-        assert_eq!(result.unwrap_err(), "Unsupported shell: unknown");
+        assert_eq!(
+            result.unwrap_err().to_string(),
+            "Unsupported shell: unknown"
+        );
         reset_to_real_environment();
     }
 
@@ -136,7 +141,10 @@ mod tests {
 
         let result = execute();
         assert!(result.is_err());
-        assert_eq!(result.unwrap_err(), "SHELL environment variable not set");
+        assert_eq!(
+            result.unwrap_err().to_string(),
+            "SHELL environment variable not set"
+        );
         reset_to_real_environment();
     }
 }