From 8b57c53f88d50a6b45382321a7f0847aba7b3588 Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Tue, 31 Mar 2026 14:42:32 +0200 Subject: [PATCH 1/6] feat(workspace): classify git errors into user-friendly messages (T-095) Add classify_git_error() to parse git stderr patterns (branch not found, permission denied, already checked out, not a git repo) into specific, human-readable AppError messages. Handles both English and French git locales. Add repo path validation in create_worktree() before running git commands. Includes 8 new tests for error classification and edge cases. --- src-tauri/src/workspace/worktree.rs | 230 +++++++++++++++++++++++++++- 1 file changed, 224 insertions(+), 6 deletions(-) diff --git a/src-tauri/src/workspace/worktree.rs b/src-tauri/src/workspace/worktree.rs index 3b6c9d4..db0fe80 100644 --- a/src-tauri/src/workspace/worktree.rs +++ b/src-tauri/src/workspace/worktree.rs @@ -52,6 +52,67 @@ pub fn build_worktree_path( .join(format!("pr-{pr_number}"))) } +/// Classifies git stderr output into a specific, user-friendly [`AppError`]. +/// +/// Parses known error patterns from git output and returns a descriptive error +/// instead of raw stderr. Unknown patterns fall back to a generic git error. +fn classify_git_error(stderr: &str, args_display: &str) -> AppError { + let stderr_lower = stderr.to_lowercase(); + + // Branch not found (during fetch) — handles English and French git locales. + if stderr_lower.contains("couldn't find remote ref") + || stderr_lower.contains("remote ref does not exist") + || stderr_lower.contains("impossible de trouver la r\u{00e9}f\u{00e9}rence distante") + { + // Extract the ref name from the last word of the matching line. + let branch = stderr + .lines() + .find(|l| { + let low = l.to_lowercase(); + low.contains("remote ref") || low.contains("r\u{00e9}f\u{00e9}rence distante") + }) + .and_then(|l| l.rsplit_once(' ')) + .map_or("unknown", |(_, name)| name.trim()); + return AppError::Git(format!( + "Branch '{branch}' not found on remote. Verify the branch exists and try again." + )); + } + + // Permission denied — English and French + if stderr_lower.contains("permission denied") + || stderr_lower.contains("permission non accord") + || stderr_lower.contains("unable to access") + { + return AppError::Git(format!( + "Permission denied during git operation. Check file permissions. Details: {}", + stderr.trim() + )); + } + + // Worktree already checked out / locked — English and French + if stderr_lower.contains("is already checked out") + || stderr_lower.contains("already locked") + || stderr_lower.contains("est d\u{00e9}j\u{00e0}") + { + return AppError::Workspace(format!( + "Worktree is already in use by another working copy. Details: {}", + stderr.trim() + )); + } + + // Not a git repository — English and French + if stderr_lower.contains("not a git repository") + || stderr_lower.contains("pas un d\u{00e9}p\u{00f4}t git") + { + return AppError::Git( + "Path is not a valid git repository. Check the repository configuration.".into(), + ); + } + + // Default: generic git error with original stderr + AppError::Git(format!("git {} failed: {}", args_display, stderr.trim())) +} + /// Runs a git command in the given directory and returns stdout on success. /// /// Times out after [`GIT_TIMEOUT`] to prevent indefinite hangs on network operations. @@ -83,11 +144,7 @@ async fn run_git(args: &[OsString], cwd: &Path) -> Result { .map(|s| s.to_string_lossy()) .collect::>() .join(" "); - return Err(AppError::Git(format!( - "git {} failed: {}", - args_display, - stderr.trim() - ))); + return Err(classify_git_error(&stderr, &args_display)); } Ok(String::from_utf8_lossy(&output.stdout).into_owned()) @@ -112,6 +169,20 @@ pub async fn create_worktree( repo_name: &str, base_dir: &Path, ) -> Result { + // Validate that repo_local_path exists and is a git repository. + if !repo_local_path.exists() { + return Err(AppError::Git(format!( + "Repository path does not exist: {}", + repo_local_path.display() + ))); + } + if !repo_local_path.join(".git").exists() { + return Err(AppError::Git(format!( + "Path is not a valid git repository: {}", + repo_local_path.display() + ))); + } + let wt_path = build_worktree_path(base_dir, repo_name, pr_number)?; if wt_path.exists() { @@ -124,7 +195,14 @@ pub async fn create_worktree( // Ensure parent directories exist before running git commands. if let Some(parent) = wt_path.parent() { tokio::fs::create_dir_all(parent).await.map_err(|e| { - AppError::Workspace(format!("failed to create worktree directory: {e}")) + if e.kind() == std::io::ErrorKind::PermissionDenied { + AppError::Workspace(format!( + "Permission denied: cannot create worktree directory at {}. Check folder permissions.", + parent.display() + )) + } else { + AppError::Workspace(format!("failed to create worktree directory: {e}")) + } })?; } @@ -352,6 +430,11 @@ mod tests { matches!(err, AppError::Git(_)), "expected Git error, got: {err}" ); + let msg = err.to_string(); + assert!( + msg.contains("not found") && msg.contains("nonexistent-branch"), + "expected user-friendly branch-not-found message, got: {msg}" + ); } #[tokio::test] @@ -410,6 +493,141 @@ mod tests { ); } + #[test] + fn test_classify_git_error_branch_not_found() { + let stderr = "fatal: couldn't find remote ref nonexistent-branch\n"; + let err = classify_git_error(stderr, "fetch origin -- nonexistent-branch"); + assert!(matches!(err, AppError::Git(_)), "expected Git, got: {err}"); + let msg = err.to_string(); + assert!(msg.contains("not found"), "missing 'not found' in: {msg}"); + assert!( + msg.contains("nonexistent-branch"), + "missing branch name in: {msg}" + ); + } + + #[test] + fn test_classify_git_error_permission_denied() { + let stderr = "fatal: could not create work tree dir '/tmp/wt/pr-42': Permission denied\n"; + let err = classify_git_error(stderr, "worktree add /tmp/wt/pr-42 origin/main"); + assert!(matches!(err, AppError::Git(_)), "expected Git, got: {err}"); + let msg = err.to_string(); + assert!( + msg.to_lowercase().contains("permission denied"), + "missing 'permission denied' in: {msg}" + ); + } + + #[test] + fn test_classify_git_error_not_a_repo() { + let stderr = "fatal: not a git repository (or any of the parent directories): .git\n"; + let err = classify_git_error(stderr, "fetch origin -- main"); + assert!(matches!(err, AppError::Git(_)), "expected Git, got: {err}"); + let msg = err.to_string(); + assert!( + msg.contains("not a valid git repository"), + "missing 'not a valid git repository' in: {msg}" + ); + } + + #[test] + fn test_classify_git_error_already_checked_out() { + let stderr = "fatal: 'feature-42' is already checked out at '/path/to/other'\n"; + let err = classify_git_error(stderr, "worktree add /tmp/wt feature-42"); + assert!( + matches!(err, AppError::Workspace(_)), + "expected Workspace, got: {err}" + ); + assert!( + err.to_string().contains("already in use"), + "missing 'already in use' in: {err}" + ); + } + + #[test] + fn test_classify_git_error_generic_fallback() { + let stderr = "error: some unknown git problem\n"; + let err = classify_git_error(stderr, "status"); + assert!(matches!(err, AppError::Git(_)), "expected Git, got: {err}"); + let msg = err.to_string(); + assert!( + msg.contains("git status failed"), + "expected generic fallback, got: {msg}" + ); + } + + #[tokio::test] + async fn test_create_worktree_invalid_repo_path() { + let tmp = TempDir::new().unwrap(); + let fake_repo = tmp.path().join("not-a-repo"); + tokio::fs::create_dir_all(&fake_repo).await.unwrap(); + let base_dir = tmp.path().join("workspaces"); + + let result = create_worktree(&fake_repo, "main", 1, "test-repo", &base_dir).await; + assert!(result.is_err()); + + let err = result.unwrap_err(); + assert!( + matches!(err, AppError::Git(_)), + "expected Git error, got: {err}" + ); + assert!( + err.to_string().contains("not a valid git repository"), + "expected repo validation error, got: {err}" + ); + } + + #[tokio::test] + async fn test_create_worktree_repo_path_not_found() { + let tmp = TempDir::new().unwrap(); + let missing_path = tmp.path().join("does-not-exist"); + let base_dir = tmp.path().join("workspaces"); + + let result = create_worktree(&missing_path, "main", 1, "test-repo", &base_dir).await; + assert!(result.is_err()); + + let err = result.unwrap_err(); + assert!( + matches!(err, AppError::Git(_)), + "expected Git error, got: {err}" + ); + assert!( + err.to_string().contains("does not exist"), + "expected path-not-found error, got: {err}" + ); + } + + #[tokio::test] + async fn test_worktree_error_permission() { + use std::os::unix::fs::PermissionsExt; + + let (_tmp, local, base_dir) = setup_test_repo().await; + + // Create the worktrees dir, then make it non-writable so git cannot + // create the pr-42 subdirectory inside it. + let wt_parent = base_dir.join("test-repo").join("worktrees"); + tokio::fs::create_dir_all(&wt_parent).await.unwrap(); + + let mut perms = tokio::fs::metadata(&wt_parent).await.unwrap().permissions(); + perms.set_mode(0o555); // r-xr-xr-x — no write + tokio::fs::set_permissions(&wt_parent, perms.clone()) + .await + .unwrap(); + + let result = create_worktree(&local, "feature-42", 42, "test-repo", &base_dir).await; + + // Restore permissions for cleanup + perms.set_mode(0o755); + tokio::fs::set_permissions(&wt_parent, perms).await.unwrap(); + + assert!(result.is_err()); + let err_msg = result.unwrap_err().to_string(); + assert!( + err_msg.to_lowercase().contains("permission"), + "expected permission-related error, got: {err_msg}" + ); + } + #[tokio::test] async fn test_list_worktrees() { let (_tmp, local, base_dir) = setup_test_repo().await; From 967aa08045e01703cf219d7611148b7cd50afd3c Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Tue, 31 Mar 2026 14:54:20 +0200 Subject: [PATCH 2/6] fix(workspace): address PR review comments (T-095) - Remove "unable to access" from permission classifier (too broad, covers network/DNS/TLS errors, not just permissions) - Replace sync Path::exists() with tokio::fs::metadata() to distinguish NotFound from PermissionDenied in async context - Add #[cfg(unix)] guard to permission test for cross-platform compat --- src-tauri/src/workspace/worktree.rs | 63 +++++++++++++++++++++++------ 1 file changed, 50 insertions(+), 13 deletions(-) diff --git a/src-tauri/src/workspace/worktree.rs b/src-tauri/src/workspace/worktree.rs index db0fe80..d846cab 100644 --- a/src-tauri/src/workspace/worktree.rs +++ b/src-tauri/src/workspace/worktree.rs @@ -79,9 +79,7 @@ fn classify_git_error(stderr: &str, args_display: &str) -> AppError { } // Permission denied — English and French - if stderr_lower.contains("permission denied") - || stderr_lower.contains("permission non accord") - || stderr_lower.contains("unable to access") + if stderr_lower.contains("permission denied") || stderr_lower.contains("permission non accord") { return AppError::Git(format!( "Permission denied during git operation. Check file permissions. Details: {}", @@ -170,17 +168,55 @@ pub async fn create_worktree( base_dir: &Path, ) -> Result { // Validate that repo_local_path exists and is a git repository. - if !repo_local_path.exists() { - return Err(AppError::Git(format!( - "Repository path does not exist: {}", - repo_local_path.display() - ))); + // Use tokio::fs::metadata to distinguish NotFound from PermissionDenied. + match tokio::fs::metadata(repo_local_path).await { + Ok(meta) if meta.is_dir() => {} + Ok(_) => { + return Err(AppError::Git(format!( + "Repository path is not a directory: {}", + repo_local_path.display() + ))); + } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + return Err(AppError::Git(format!( + "Repository path does not exist: {}", + repo_local_path.display() + ))); + } + Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => { + return Err(AppError::Git(format!( + "Permission denied while accessing repository path: {}", + repo_local_path.display() + ))); + } + Err(e) => { + return Err(AppError::Git(format!( + "Failed to access repository path {}: {e}", + repo_local_path.display() + ))); + } } - if !repo_local_path.join(".git").exists() { - return Err(AppError::Git(format!( - "Path is not a valid git repository: {}", - repo_local_path.display() - ))); + + match tokio::fs::metadata(repo_local_path.join(".git")).await { + Ok(_) => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + return Err(AppError::Git(format!( + "Path is not a valid git repository: {}", + repo_local_path.display() + ))); + } + Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => { + return Err(AppError::Git(format!( + "Permission denied while accessing git metadata in {}", + repo_local_path.display() + ))); + } + Err(e) => { + return Err(AppError::Git(format!( + "Failed to inspect repository {}: {e}", + repo_local_path.display() + ))); + } } let wt_path = build_worktree_path(base_dir, repo_name, pr_number)?; @@ -597,6 +633,7 @@ mod tests { ); } + #[cfg(unix)] #[tokio::test] async fn test_worktree_error_permission() { use std::os::unix::fs::PermissionsExt; From 7427012ee892deb58ba3489e86af702804d6374a Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Tue, 31 Mar 2026 15:05:11 +0200 Subject: [PATCH 3/6] fix(workspace): use git rev-parse for repo validation (T-095) Replace .git metadata check with git rev-parse --git-dir to correctly accept both regular and bare repositories. --- src-tauri/src/workspace/worktree.rs | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/src-tauri/src/workspace/worktree.rs b/src-tauri/src/workspace/worktree.rs index d846cab..a19ab80 100644 --- a/src-tauri/src/workspace/worktree.rs +++ b/src-tauri/src/workspace/worktree.rs @@ -197,27 +197,16 @@ pub async fn create_worktree( } } - match tokio::fs::metadata(repo_local_path.join(".git")).await { - Ok(_) => {} - Err(e) if e.kind() == std::io::ErrorKind::NotFound => { - return Err(AppError::Git(format!( + // Use `git rev-parse --git-dir` to validate the repo. This works for both + // regular and bare repositories (bare repos have no `.git` subdirectory). + run_git(&["rev-parse".into(), "--git-dir".into()], repo_local_path) + .await + .map_err(|_| { + AppError::Git(format!( "Path is not a valid git repository: {}", repo_local_path.display() - ))); - } - Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => { - return Err(AppError::Git(format!( - "Permission denied while accessing git metadata in {}", - repo_local_path.display() - ))); - } - Err(e) => { - return Err(AppError::Git(format!( - "Failed to inspect repository {}: {e}", - repo_local_path.display() - ))); - } - } + )) + })?; let wt_path = build_worktree_path(base_dir, repo_name, pr_number)?; From 1e29bee27e81f100e8f4c6b85a62d12a07b0c94c Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Tue, 31 Mar 2026 15:18:46 +0200 Subject: [PATCH 4/6] fix(workspace): sanitize error messages and propagate classified errors (T-095) - Remove raw stderr from user-facing error messages, log it via tracing instead to prevent leaking filesystem paths and URLs to the frontend - Propagate classified error from run_git for rev-parse instead of masking all failures as "not a valid git repository" - Fix French locale detection for "not a git repository" pattern --- src-tauri/src/workspace/worktree.rs | 43 ++++++++++++++++------------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/src-tauri/src/workspace/worktree.rs b/src-tauri/src/workspace/worktree.rs index a19ab80..ce4bb2c 100644 --- a/src-tauri/src/workspace/worktree.rs +++ b/src-tauri/src/workspace/worktree.rs @@ -4,6 +4,7 @@ use std::time::Duration; use tokio::process::Command; use tokio::time::timeout; +use tracing::warn; use crate::error::AppError; @@ -81,10 +82,10 @@ fn classify_git_error(stderr: &str, args_display: &str) -> AppError { // Permission denied — English and French if stderr_lower.contains("permission denied") || stderr_lower.contains("permission non accord") { - return AppError::Git(format!( - "Permission denied during git operation. Check file permissions. Details: {}", - stderr.trim() - )); + warn!(stderr = stderr.trim(), "git permission denied"); + return AppError::Git( + "Permission denied during git operation. Check file and directory permissions.".into(), + ); } // Worktree already checked out / locked — English and French @@ -92,23 +93,28 @@ fn classify_git_error(stderr: &str, args_display: &str) -> AppError { || stderr_lower.contains("already locked") || stderr_lower.contains("est d\u{00e9}j\u{00e0}") { - return AppError::Workspace(format!( - "Worktree is already in use by another working copy. Details: {}", - stderr.trim() - )); + warn!(stderr = stderr.trim(), "git worktree already in use"); + return AppError::Workspace("Worktree is already in use by another working copy.".into()); } // Not a git repository — English and French if stderr_lower.contains("not a git repository") - || stderr_lower.contains("pas un d\u{00e9}p\u{00f4}t git") + || stderr_lower.contains("d\u{00e9}p\u{00f4}t git") { return AppError::Git( "Path is not a valid git repository. Check the repository configuration.".into(), ); } - // Default: generic git error with original stderr - AppError::Git(format!("git {} failed: {}", args_display, stderr.trim())) + // Default: generic git error — log stderr for debugging, keep user message clean. + warn!( + command = args_display, + stderr = stderr.trim(), + "git command failed" + ); + AppError::Git(format!( + "git {args_display} failed. Check the logs for details." + )) } /// Runs a git command in the given directory and returns stdout on success. @@ -199,14 +205,8 @@ pub async fn create_worktree( // Use `git rev-parse --git-dir` to validate the repo. This works for both // regular and bare repositories (bare repos have no `.git` subdirectory). - run_git(&["rev-parse".into(), "--git-dir".into()], repo_local_path) - .await - .map_err(|_| { - AppError::Git(format!( - "Path is not a valid git repository: {}", - repo_local_path.display() - )) - })?; + // Let the classified error from run_git propagate (timeout, permission, etc.). + run_git(&["rev-parse".into(), "--git-dir".into()], repo_local_path).await?; let wt_path = build_worktree_path(base_dir, repo_name, pr_number)?; @@ -579,6 +579,11 @@ mod tests { msg.contains("git status failed"), "expected generic fallback, got: {msg}" ); + // Ensure raw stderr is NOT leaked in the user-facing message + assert!( + !msg.contains("some unknown git problem"), + "raw stderr should not appear in user-facing message: {msg}" + ); } #[tokio::test] From 49f40ccf90ccbd177176dd66397e49ceb1842841 Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Tue, 31 Mar 2026 15:28:24 +0200 Subject: [PATCH 5/6] fix(workspace): narrow French not-a-repo matcher (T-095) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace broad "dépôt git" pattern with specific "n'est un dépôt git" and "pas un dépôt git" to avoid misclassifying unrelated git errors. --- src-tauri/src/workspace/worktree.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src-tauri/src/workspace/worktree.rs b/src-tauri/src/workspace/worktree.rs index ce4bb2c..9e9f590 100644 --- a/src-tauri/src/workspace/worktree.rs +++ b/src-tauri/src/workspace/worktree.rs @@ -99,7 +99,8 @@ fn classify_git_error(stderr: &str, args_display: &str) -> AppError { // Not a git repository — English and French if stderr_lower.contains("not a git repository") - || stderr_lower.contains("d\u{00e9}p\u{00f4}t git") + || stderr_lower.contains("n'est un d\u{00e9}p\u{00f4}t git") + || stderr_lower.contains("pas un d\u{00e9}p\u{00f4}t git") { return AppError::Git( "Path is not a valid git repository. Check the repository configuration.".into(), From 0a119abccdef6e57996840744bbe2548ab20f32e Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Tue, 31 Mar 2026 15:58:30 +0200 Subject: [PATCH 6/6] fix(workspace): test create_dir_all permission branch correctly (T-095) Make repo_root (not worktrees/) read-only so create_dir_all actually fails with PermissionDenied instead of being a no-op. --- src-tauri/src/workspace/worktree.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src-tauri/src/workspace/worktree.rs b/src-tauri/src/workspace/worktree.rs index 9e9f590..b953bd9 100644 --- a/src-tauri/src/workspace/worktree.rs +++ b/src-tauri/src/workspace/worktree.rs @@ -635,14 +635,15 @@ mod tests { let (_tmp, local, base_dir) = setup_test_repo().await; - // Create the worktrees dir, then make it non-writable so git cannot - // create the pr-42 subdirectory inside it. - let wt_parent = base_dir.join("test-repo").join("worktrees"); - tokio::fs::create_dir_all(&wt_parent).await.unwrap(); + // Create only the repo-level dir (not worktrees/) and make it + // non-writable so create_dir_all in create_worktree hits PermissionDenied + // when trying to create the worktrees/ subdirectory. + let repo_root = base_dir.join("test-repo"); + tokio::fs::create_dir_all(&repo_root).await.unwrap(); - let mut perms = tokio::fs::metadata(&wt_parent).await.unwrap().permissions(); + let mut perms = tokio::fs::metadata(&repo_root).await.unwrap().permissions(); perms.set_mode(0o555); // r-xr-xr-x — no write - tokio::fs::set_permissions(&wt_parent, perms.clone()) + tokio::fs::set_permissions(&repo_root, perms.clone()) .await .unwrap(); @@ -650,7 +651,7 @@ mod tests { // Restore permissions for cleanup perms.set_mode(0o755); - tokio::fs::set_permissions(&wt_parent, perms).await.unwrap(); + tokio::fs::set_permissions(&repo_root, perms).await.unwrap(); assert!(result.is_err()); let err_msg = result.unwrap_err().to_string();