Address review feedback: improve error handling

- Remove unused thiserror dependency from workspace
- Add from_josh_err helper function for converting josh::JoshError
- Replace manual error mapping with from_josh_err + .context()
- Replace all bail! macro calls with explicit return Err(...)

This makes error handling clearer and avoids hidden control flow.

Co-authored-by: Vlad Ivanov <vlad-ivanov-name@users.noreply.github.com>

Author: github-actions[bot]

Cargo.toml

@@ -35,7 +35,6 @@ rs_tracing = { version = "1.1.0", features = ["rs_tracing"] }
 serde = { version = "1.0.228", features = ["std", "derive"] }
 serde_json = "1.0.145"
 serde_yaml = "0.9.34"
-thiserror = "2.0"
 toml = "0.9.8"
 tracing-subscriber = { version = "0.3.20", features = ["env-filter"] }
 tempfile = "3.23.0"

josh-cli/src/bin/josh.rs

@@ -8,6 +8,11 @@ use log::debug;
 use std::io::IsTerminal;
 use std::process::{Command as ProcessCommand, Stdio};
 
+/// Helper function to convert josh::JoshError to anyhow::Error
+fn from_josh_err(err: josh::JoshError) -> anyhow::Error {
+    anyhow::anyhow!("{}", err.0)
+}
+
 /// Spawn a git command directly to the terminal so users can see progress
 /// Falls back to captured output if not in a TTY environment
 fn spawn_git_command(
@@ -278,7 +283,8 @@ fn apply_josh_filtering(
 
     // Use josh API directly instead of calling josh-filter binary
     let filterobj = josh::filter::parse(filter)
-        .map_err(|e| anyhow::anyhow!("Failed to parse filter: {}", e.0))?;
+        .map_err(from_josh_err)
+        .context("Failed to parse filter")?;
 
     josh::cache_sled::sled_load(&repo_path.join(".git"))
         .context("Failed to load sled cache")?;
@@ -288,15 +294,18 @@ fn apply_josh_filtering(
             .with_backend(josh::cache_sled::SledCacheBackend::default())
             .with_backend(
                 josh::cache_notes::NotesCacheBackend::new(repo_path)
-                    .map_err(|e| anyhow::anyhow!("Failed to create NotesCacheBackend: {}", e.0))?,
+                    .map_err(from_josh_err)
+                    .context("Failed to create NotesCacheBackend")?,
             ),
     );
 
     // Open Josh transaction
     let transaction = josh::cache::TransactionContext::from_env(cache.clone())
-        .map_err(|e| anyhow::anyhow!("Failed TransactionContext::from_env: {}", e.0))?
+        .map_err(from_josh_err)
+        .context("Failed TransactionContext::from_env")?
         .open(None)
-        .map_err(|e| anyhow::anyhow!("Failed TransactionContext::open: {}", e.0))?;
+        .map_err(from_josh_err)
+        .context("Failed TransactionContext::open")?;
 
     let repo = transaction.repo();
 
@@ -356,7 +365,7 @@ fn apply_josh_filtering(
     )?;
 
     if exit_code != 0 {
-        anyhow::bail!("Failed to fetch filtered refs: exit code {}", exit_code);
+        return Err(anyhow::anyhow!("Failed to fetch filtered refs: exit code {}", exit_code));
     }
 
     Ok(())
@@ -449,11 +458,11 @@ fn handle_clone(args: &CloneArgs) -> Result<()> {
     )?;
 
     if exit_code != 0 {
-        anyhow::bail!(
+        return Err(anyhow::anyhow!(
             "Failed to checkout branch {}: exit code {}",
             default_branch,
             exit_code
-        );
+        ));
     }
 
     // Set up upstream tracking for the branch
@@ -469,11 +478,11 @@ fn handle_clone(args: &CloneArgs) -> Result<()> {
     )?;
 
     if exit_code != 0 {
-        anyhow::bail!(
+        return Err(anyhow::anyhow!(
             "Failed to set upstream for branch {}: exit code {}",
             default_branch,
             exit_code
-        );
+        ));
     }
 
     println!("Cloned repository to: {}", output_dir.display());
@@ -568,10 +577,10 @@ fn handle_fetch_internal(args: &FetchArgs, repo_path: &std::path::Path) -> Resul
     )?;
 
     if exit_code != 0 {
-        anyhow::bail!(
+        return Err(anyhow::anyhow!(
             "git fetch to josh/remotes failed with exit code: {}",
             exit_code
-        );
+        ));
     }
 
     // Set up remote HEAD reference using git ls-remote
@@ -648,7 +657,8 @@ fn handle_push(args: &PushArgs) -> Result<()> {
 
     // Parse the filter using Josh API
     let filter = josh::filter::parse(&filter_str)
-        .map_err(|e| anyhow::anyhow!("Failed to parse filter: {}", e.0))?;
+        .map_err(from_josh_err)
+        .context("Failed to parse filter")?;
 
     josh::cache_sled::sled_load(repo_path)
         .context("Failed to load sled cache")?;
@@ -657,15 +667,18 @@ fn handle_push(args: &PushArgs) -> Result<()> {
             .with_backend(josh::cache_sled::SledCacheBackend::default())
             .with_backend(
                 josh::cache_notes::NotesCacheBackend::new(repo_path)
-                    .map_err(|e| anyhow::anyhow!("Failed to create NotesCacheBackend: {}", e.0))?,
+                    .map_err(from_josh_err)
+                    .context("Failed to create NotesCacheBackend")?,
             ),
     );
 
     // Open Josh transaction
     let transaction = josh::cache::TransactionContext::from_env(cache.clone())
-        .map_err(|e| anyhow::anyhow!("Failed TransactionContext::from_env: {}", e.0))?
+        .map_err(from_josh_err)
+        .context("Failed TransactionContext::from_env")?
         .open(None)
-        .map_err(|e| anyhow::anyhow!("Failed TransactionContext::open: {}", e.0))?;
+        .map_err(from_josh_err)
+        .context("Failed TransactionContext::open")?;
 
     // Get the remote URL from josh-remote config
     let remote_url = config
@@ -786,7 +799,8 @@ fn handle_push(args: &PushArgs) -> Result<()> {
             None,         // reparent_orphans
             &mut changes, // change_ids
         )
-        .map_err(|e| format!("Failed to unapply filter: {}", e.0))?;
+        .map_err(from_josh_err)
+        .context("Failed to unapply filter")?;
 
         // Define variables needed for build_to_push
         let baseref = remote_ref.clone();
@@ -805,7 +819,8 @@ fn handle_push(args: &PushArgs) -> Result<()> {
             oid_to_push,
             old,
         )
-        .map_err(|e| anyhow::anyhow!("Failed to build to push: {}", e.0))?;
+        .map_err(from_josh_err)
+        .context("Failed to build to push")?;
 
         debug!("to_push: {:?}", to_push);
 
@@ -846,7 +861,7 @@ fn handle_push(args: &PushArgs) -> Result<()> {
             )?;
 
             if exit_code != 0 {
-                anyhow::bail!("git push failed for {}: exit code {}", refname, exit_code);
+                return Err(anyhow::anyhow!("git push failed for {}: exit code {}", refname, exit_code));
             }
 
             println!("Pushed {} to {}/{}", oid, args.remote, refname);
@@ -932,7 +947,7 @@ fn handle_remote_add_internal(args: &RemoteAddArgs, repo_path: &std::path::Path)
     )?;
 
     if exit_code != 0 {
-        anyhow::bail!("Failed to add git remote: exit code {}", exit_code);
+        return Err(anyhow::anyhow!("Failed to add git remote: exit code {}", exit_code));
     }
 
     // Set up namespace configuration for the remote
@@ -950,7 +965,7 @@ fn handle_remote_add_internal(args: &RemoteAddArgs, repo_path: &std::path::Path)
     )?;
 
     if exit_code != 0 {
-        anyhow::bail!("Failed to set remote uploadpack: exit code {}", exit_code);
+        return Err(anyhow::anyhow!("Failed to set remote uploadpack: exit code {}", exit_code));
     }
 
     println!(