feat(workspace): classify git errors into user-friendly messages (T-095)#93
Merged
feat(workspace): classify git errors into user-friendly messages (T-095)#93
Conversation
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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds git stderr classification and stronger repo-path validation in the worktree module: introduces Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant WorktreeModule
participant Filesystem
participant GitProcess
Caller->>WorktreeModule: create_worktree(repo_local_path, ...)
WorktreeModule->>Filesystem: tokio::fs::metadata(repo_local_path)
alt stat error (NotFound / PermissionDenied / other)
Filesystem-->>WorktreeModule: error
WorktreeModule-->>Caller: AppError::Workspace / AppError::Git
else stat ok
WorktreeModule->>GitProcess: git rev-parse --git-dir
alt rev-parse fails
GitProcess-->>WorktreeModule: stderr
WorktreeModule->>WorktreeModule: classify_git_error(stderr, args_display)
WorktreeModule-->>Caller: mapped AppError
else rev-parse ok
WorktreeModule->>GitProcess: git fetch / git worktree add ...
GitProcess-->>WorktreeModule: stdout, stderr, exit_code
alt exit_code == 0
WorktreeModule-->>Caller: success
else
WorktreeModule->>WorktreeModule: classify_git_error(stderr, args_display)
WorktreeModule-->>Caller: mapped AppError
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR improves the Key changes:
Two minor P2 issues found:
Confidence Score: 5/5Safe to merge; all remaining findings are P2 with no blocking defects on the happy path. Both findings are P2: the broad French "est déjà" pattern is an edge-case misclassification affecting only French-locale users in non-worktree error scenarios, and the root-user test guard is a CI reliability nit. Core logic — async validation, error classification, and test coverage — is solid and the previous P1 concerns have been fully resolved. src-tauri/src/workspace/worktree.rs — minor French locale pattern and CI test robustness Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src-tauri/src/workspace/worktree.rs
Line: 92-98
Comment:
**`est déjà` French pattern is overly broad**
The string `"est déjà"` means "is already" in French and appears in many unrelated git messages, not just worktree conflicts. Any French git error containing those two words would be misclassified as "Worktree is already in use by another working copy." — for example:
```
fatal: La branche 'main' est déjà fusionnée.
# → incorrectly classified as Workspace("Worktree is already in use…")
```
The English side uses the specific phrases `"is already checked out"` and `"already locked"`. The French equivalent of "is already checked out" is `"est déjà extrait"` and of "already locked" is `"est déjà verrouillé"`. Using these tighter substrings avoids false-positive matches:
```suggestion
if stderr_lower.contains("is already checked out")
|| stderr_lower.contains("already locked")
|| stderr_lower.contains("est d\u{00e9}j\u{00e0} extrait")
|| stderr_lower.contains("est d\u{00e9}j\u{00e0} verrouill\u{00e9}")
{
warn!(stderr = stderr.trim(), "git worktree already in use");
return AppError::Workspace("Worktree is already in use by another working copy.".into());
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src-tauri/src/workspace/worktree.rs
Line: 631-662
Comment:
**Permission test spuriously passes (and then fails) when run as root**
Many CI environments run tests inside containers where the process is `root`. Because `root` ignores filesystem permission bits, `create_dir_all` will succeed on a `0o555` directory and `create_worktree` will return `Ok(...)`, causing `assert!(result.is_err())` on line 656 to panic and the test to fail with a misleading message.
A standard guard for this class of test is to skip it when running as root:
```rust
#[cfg(unix)]
#[tokio::test]
async fn test_worktree_error_permission() {
use std::os::unix::fs::PermissionsExt;
// Root bypasses permission checks; skip to avoid a spurious failure in CI.
if unsafe { libc::getuid() } == 0 {
return;
}
// … rest of the test unchanged …
}
```
Alternatively, use the `nix` crate's `Uid::current().is_root()` if `libc` is not already a dependency. Without this guard, the test is liable to fail in Docker-based CI pipelines.
How can I resolve this? If you propose a fix, please make it concise.Reviews (6): Last reviewed commit: "fix(workspace): test create_dir_all perm..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src-tauri/src/workspace/worktree.rs">
<violation number="1" location="src-tauri/src/workspace/worktree.rs:84">
P2: `"unable to access"` is too broad for permission detection and can misclassify network/auth git failures as filesystem permission errors.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/workspace/worktree.rs`:
- Around line 600-628: The test function test_worktree_error_permission uses
std::os::unix::fs::PermissionsExt and must be gated to Unix-only targets; add a
platform guard by placing #[cfg(unix)] immediately above the #[tokio::test]
attribute on the test_worktree_error_permission function so the test (and its
use of PermissionsExt) is only compiled and run on Unix platforms.
- Around line 81-89: The permission-error classifier in run_git() is too
broad—remove the check for the string "unable to access" from the condition that
builds AppError::Git("Permission denied...") so only true filesystem permission
phrases remain (e.g., "permission denied", "permission non accord"); update the
conditional that currently checks stderr_lower.contains("unable to access") to
no longer include that clause and ensure run_git() instead falls through to the
generic error path for network/TLS/authentication failures so those produce an
appropriate non-permission error message.
- Around line 172-184: The current preflight uses Path::exists() which collapses
PermissionDenied into false; replace the two exists() checks with
std::fs::metadata calls (or repo_local_path.metadata() and
repo_local_path.join(".git").metadata()) and pattern-match the io::ErrorKind to
preserve NotFound vs PermissionDenied: when metadata() returns Ok proceed, when
Err(kind==NotFound) return the existing AppError::Git "does not exist"/"not a
valid git repository" messages, and when Err(kind==PermissionDenied) return a
distinct error (or map to AppError::Git with a PermissionDenied-specific
message) so unreadable repos are not reported as missing; update the logic
around repo_local_path and repo_local_path.join(".git") accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4072c966-f16b-471f-a33f-8bdf6aaf5a3e
📒 Files selected for processing (1)
src-tauri/src/workspace/worktree.rs
- 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
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src-tauri/src/workspace/worktree.rs (1)
532-593: Add one French fixture to lock in the locale support.Lines 63-65, 82, 93, and 103 add French-language classifiers, but this new test block only exercises English stderr. A small French sample for each mapped class would keep those branches from regressing silently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/workspace/worktree.rs` around lines 532 - 593, Add French-language stderr fixtures and corresponding assertions alongside the existing English tests to ensure locale support doesn't regress: for classify_git_error add tests mirroring test_classify_git_error_branch_not_found, test_classify_git_error_permission_denied, test_classify_git_error_not_a_repo, test_classify_git_error_already_checked_out and test_classify_git_error_generic_fallback but using French stderr samples (e.g. "introuvable", "Permission refusée", "n'est pas un dépôt git", "est déjà extrait", etc.), call classify_git_error with the same command strings, assert the same AppError variants (AppError::Git or AppError::Workspace) and the same substring expectations on err.to_string() to lock in French classifier behavior for the functions/classify_git_error and the test names (e.g. append _fr to each test).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/workspace/worktree.rs`:
- Around line 200-206: The current preflight in worktree.rs rejects repositories
by checking tokio::fs::metadata(repo_local_path.join(".git")), which incorrectly
fails for bare repositories; replace that filesystem-only check with a Git-aware
validation: run "git rev-parse --git-dir" (e.g. via tokio::process::Command with
"-C" repo_local_path) and treat a successful exit as a valid repo, or
alternatively accept a repo root that contains bare-repo markers (HEAD plus
objects/refs) instead of requiring a .git subdirectory; update the logic that
currently matches on tokio::fs::metadata and use the git-aware check
(referencing repo_local_path and the metadata-matching branch) so bare
repositories are allowed.
---
Nitpick comments:
In `@src-tauri/src/workspace/worktree.rs`:
- Around line 532-593: Add French-language stderr fixtures and corresponding
assertions alongside the existing English tests to ensure locale support doesn't
regress: for classify_git_error add tests mirroring
test_classify_git_error_branch_not_found,
test_classify_git_error_permission_denied, test_classify_git_error_not_a_repo,
test_classify_git_error_already_checked_out and
test_classify_git_error_generic_fallback but using French stderr samples (e.g.
"introuvable", "Permission refusée", "n'est pas un dépôt git", "est déjà
extrait", etc.), call classify_git_error with the same command strings, assert
the same AppError variants (AppError::Git or AppError::Workspace) and the same
substring expectations on err.to_string() to lock in French classifier behavior
for the functions/classify_git_error and the test names (e.g. append _fr to each
test).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f6b44a3-1caa-4310-a335-cda4f7f00ac2
📒 Files selected for processing (1)
src-tauri/src/workspace/worktree.rs
Replace .git metadata check with git rev-parse --git-dir to correctly accept both regular and bare repositories.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src-tauri/src/workspace/worktree.rs">
<violation number="1" location="src-tauri/src/workspace/worktree.rs:204">
P2: This `map_err(|_| ...)` masks all `git rev-parse` failures as "not a valid git repository", hiding specific errors like permission denied. Propagate the original `run_git` error instead.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src-tauri/src/workspace/worktree.rs (1)
632-647: This test never exercises the newcreate_dir_allpermission mapping.
wt_parentalready exists beforecreate_worktree()runs, socreate_dir_all(parent)is a no-op even afterchmod 0555; the failure is deferred togit worktree addinstead. That means the explicitPermissionDeniedbranch on Lines 223-230 can regress without this test failing. Maketest-repo/non-writable and leaveworktrees/absent so the permission error happens atcreate_dir_all().🧪 Adjust the fixture so it hits the intended branch
- // 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(); + // Create the repo dir, then make it non-writable so create_dir_all() + // cannot create the `worktrees` child. + let repo_dir = base_dir.join("test-repo"); + tokio::fs::create_dir_all(&repo_dir).await.unwrap(); + + let mut perms = tokio::fs::metadata(&repo_dir).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_dir, 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(); + tokio::fs::set_permissions(&repo_dir, perms).await.unwrap();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/workspace/worktree.rs` around lines 632 - 647, The test currently creates worktrees/ before calling create_worktree so create_dir_all is a no-op; change the fixture to make the test-repo directory non-writable (set permissions on base_dir.join("test-repo") to 0o555) and ensure the worktrees/ directory does NOT exist so that create_dir_all(&wt_parent) inside create_worktree triggers the PermissionDenied branch (lines handling PermissionDenied). Keep the call to create_worktree(&local, "feature-42", 42, "test-repo", &base_dir).await, then restore permissions on test-repo to writable for cleanup; reference wt_parent, base_dir, create_worktree, and the PermissionDenied branch when making edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/workspace/worktree.rs`:
- Around line 84-87: The code in worktree.rs currently interpolates raw git
stderr into AppError::Git (e.g., the return using AppError::Git(format!(...
stderr.trim() ...))) which can leak sensitive repo URLs and paths to the
frontend via workspace_open; instead, log the raw stderr locally (e.g., using
the existing logger) and return a sanitized, generic error string in
AppError::Git such as "Permission denied during git operation; check repository
and file permissions" (omit stderr content), applying the same change to the
other similar returns (the AppError::Git occurrences at the other noted
locations); keep AppError::Git as the error variant but remove any raw stderr
interpolation so only backend logs contain the detailed git output.
- Around line 202-209: The current map_err on run_git(...) throws away the
original error; change it to inspect the error returned by run_git (capture it
as e) and only convert to AppError::Git with the repo_local_path when the error
indicates the specific "not a git repository" condition (e.g., inspect an error
variant or stderr/message containing "not a git repository"); otherwise return
or propagate the original error unchanged. Update the call around
run_git(&["rev-parse"...], repo_local_path).await to match on the captured error
and only produce AppError::Git(...) for the specific case, leaving other errors
(timeouts, git missing, permission issues) intact.
---
Nitpick comments:
In `@src-tauri/src/workspace/worktree.rs`:
- Around line 632-647: The test currently creates worktrees/ before calling
create_worktree so create_dir_all is a no-op; change the fixture to make the
test-repo directory non-writable (set permissions on base_dir.join("test-repo")
to 0o555) and ensure the worktrees/ directory does NOT exist so that
create_dir_all(&wt_parent) inside create_worktree triggers the PermissionDenied
branch (lines handling PermissionDenied). Keep the call to
create_worktree(&local, "feature-42", 42, "test-repo", &base_dir).await, then
restore permissions on test-repo to writable for cleanup; reference wt_parent,
base_dir, create_worktree, and the PermissionDenied branch when making edits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 510322b3-57e8-4fe6-b626-d5b4e2b8b410
📒 Files selected for processing (1)
src-tauri/src/workspace/worktree.rs
src-tauri/src/workspace/worktree.rs
Outdated
Comment on lines
+202
to
+209
| 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() | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
Don’t collapse every rev-parse failure into “not a valid git repository”.
This map_err(|_| ...) discards the classifier you just added. A timeout, an unreadable .git, or even “git not found” will all be reported as a bad repository path, which sends users to the wrong remediation. Let run_git() propagate other failures unchanged, and only rewrite the specific not-a-repo case if you still want the path in the message.
🔎 Minimal fix
- 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()
- ))
- })?;
+ run_git(&["rev-parse".into(), "--git-dir".into()], repo_local_path).await?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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() | |
| )) | |
| })?; | |
| run_git(&["rev-parse".into(), "--git-dir".into()], repo_local_path).await?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src-tauri/src/workspace/worktree.rs` around lines 202 - 209, The current
map_err on run_git(...) throws away the original error; change it to inspect the
error returned by run_git (capture it as e) and only convert to AppError::Git
with the repo_local_path when the error indicates the specific "not a git
repository" condition (e.g., inspect an error variant or stderr/message
containing "not a git repository"); otherwise return or propagate the original
error unchanged. Update the call around run_git(&["rev-parse"...],
repo_local_path).await to match on the captured error and only produce
AppError::Git(...) for the specific case, leaving other errors (timeouts, git
missing, permission issues) intact.
…rs (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
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src-tauri/src/workspace/worktree.rs (1)
213-218: Consider using async metadata check for consistency.This synchronous
wt_path.exists()call is inconsistent with the asynctokio::fs::metadataapproach used forrepo_local_pathvalidation. While unlikely to cause issues in practice (subsequent operations would fail), using async file operations throughout would be more consistent with the PR's approach.♻️ Optional refactor for consistency
- if wt_path.exists() { - return Err(AppError::Workspace(format!( - "worktree already exists at {}", - wt_path.display() - ))); - } + if tokio::fs::try_exists(&wt_path).await.unwrap_or(false) { + return Err(AppError::Workspace(format!( + "worktree already exists at {}", + wt_path.display() + ))); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/workspace/worktree.rs` around lines 213 - 218, Replace the synchronous wt_path.exists() call with an async tokio::fs::metadata check to match the repo_local_path validation: call tokio::fs::metadata(&wt_path).await and treat Ok(_) as "worktree exists" (return the same AppError::Workspace), treat an Err whose kind is NotFound as "does not exist" and continue, and map any other Err to an appropriate AppError::Workspace with the error details; update the surrounding async function to await this call and keep references to wt_path and AppError::Workspace intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src-tauri/src/workspace/worktree.rs`:
- Around line 213-218: Replace the synchronous wt_path.exists() call with an
async tokio::fs::metadata check to match the repo_local_path validation: call
tokio::fs::metadata(&wt_path).await and treat Ok(_) as "worktree exists" (return
the same AppError::Workspace), treat an Err whose kind is NotFound as "does not
exist" and continue, and map any other Err to an appropriate AppError::Workspace
with the error details; update the surrounding async function to await this call
and keep references to wt_path and AppError::Workspace intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 383c1f97-8e56-4e7e-b051-098e51fb3536
📒 Files selected for processing (1)
src-tauri/src/workspace/worktree.rs
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src-tauri/src/workspace/worktree.rs">
<violation number="1" location="src-tauri/src/workspace/worktree.rs:102">
P2: The French not-a-repository matcher is too broad and can misclassify unrelated git errors as local repo-path errors.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src-tauri/src/workspace/worktree.rs (2)
522-588: Add a French fixture for the localized matcher paths.The classifier now relies on exact French substrings, but every unit test here is English-only. A couple of French regression cases would keep that support from regressing silently.
🇫🇷 Suggested regression cases
+ #[test] + fn test_classify_git_error_branch_not_found_french() { + let stderr = "fatal: impossible de trouver la référence distante nonexistent-branch\n"; + let err = classify_git_error(stderr, "fetch origin -- nonexistent-branch"); + 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_not_a_repo_french() { + let stderr = "fatal: ce n'est pas un dépôt git (ou un des répertoires parents): .git\n"; + let err = classify_git_error(stderr, "rev-parse --git-dir"); + assert!( + err.to_string().contains("not a valid git repository"), + "missing repo validation message: {err}" + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/workspace/worktree.rs` around lines 522 - 588, Tests exercise classify_git_error only with English stderr messages, but the classifier also matches exact French substrings and needs regression coverage; add one or two unit tests (e.g., test_classify_git_error_branch_not_found_fr and test_classify_git_error_permission_denied_fr) that pass French stderr strings into classify_git_error (using the same command strings as the English tests) and assert the same AppError variant and user-facing message fragments (translated equivalents like "introuvable" or "permission refusée" / "permission denied" in lowercased checks), and ensure the generic fallback test still verifies that raw French stderr is not leaked in the user-facing message. Ensure you reference classify_git_error and the existing test names when adding the new fixtures so they live next to the corresponding English tests.
63-79: Mirror thewarn!logging in these early-return classifier branches.These two paths sanitize the user-facing message, but they are also the only classified branches that currently drop the backend stderr/context entirely.
🪵 Minimal follow-up
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") { + warn!( + command = args_display, + stderr = stderr.trim(), + "git branch not found" + ); // Extract the ref name from the last word of the matching line. let branch = stderr .lines() @@ if stderr_lower.contains("not a git repository") || 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") { + warn!( + command = args_display, + stderr = stderr.trim(), + "git repository validation failed" + ); return AppError::Git( "Path is not a valid git repository. Check the repository configuration.".into(), ); }Also applies to: 100-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/workspace/worktree.rs` around lines 63 - 79, The early-return branch in worktree.rs that matches stderr_lower ("couldn't find remote ref" / "remote ref does not exist" / "impossible de trouver la référence distante") returns an AppError::Git without logging the underlying stderr; update this path to call warn! with the raw stderr (and the extracted branch name) before returning so the backend context is preserved (mirror the existing warn! pattern used elsewhere), and make the analogous change to the other similar classifier branch (around the 100-107 area) so both branches log warn!(target: ... or similar) including stderr and branch to aid debugging while still returning the sanitized AppError::Git.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src-tauri/src/workspace/worktree.rs`:
- Around line 640-647: The test sets up wt_parent ahead of calling
create_worktree(), so create_dir_all(&parent) in create_worktree() is a no-op
and the new permission-handling branch is never executed; change the test to
ensure the directory does not exist before calling create_worktree() (or create
a parent directory and make the parent of wt_parent readonly) so that
create_dir_all(parent) in create_worktree() actually runs and fails due to
permissions—target symbols: wt_parent, create_worktree(),
create_dir_all(parent), and the pre-git permission handling branch referenced
around the git worktree add path.
---
Nitpick comments:
In `@src-tauri/src/workspace/worktree.rs`:
- Around line 522-588: Tests exercise classify_git_error only with English
stderr messages, but the classifier also matches exact French substrings and
needs regression coverage; add one or two unit tests (e.g.,
test_classify_git_error_branch_not_found_fr and
test_classify_git_error_permission_denied_fr) that pass French stderr strings
into classify_git_error (using the same command strings as the English tests)
and assert the same AppError variant and user-facing message fragments
(translated equivalents like "introuvable" or "permission refusée" / "permission
denied" in lowercased checks), and ensure the generic fallback test still
verifies that raw French stderr is not leaked in the user-facing message. Ensure
you reference classify_git_error and the existing test names when adding the new
fixtures so they live next to the corresponding English tests.
- Around line 63-79: The early-return branch in worktree.rs that matches
stderr_lower ("couldn't find remote ref" / "remote ref does not exist" /
"impossible de trouver la référence distante") returns an AppError::Git without
logging the underlying stderr; update this path to call warn! with the raw
stderr (and the extracted branch name) before returning so the backend context
is preserved (mirror the existing warn! pattern used elsewhere), and make the
analogous change to the other similar classifier branch (around the 100-107
area) so both branches log warn!(target: ... or similar) including stderr and
branch to aid debugging while still returning the sanitized AppError::Git.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1d78f29-e1d3-4252-956e-d02cf4e4ea95
📒 Files selected for processing (1)
src-tauri/src/workspace/worktree.rs
Make repo_root (not worktrees/) read-only so create_dir_all actually fails with PermissionDenied instead of being a no-op.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
classify_git_error()function that parses git stderr patterns (branch not found, permission denied, worktree already checked out, not a git repository) into specific, human-readableAppErrormessagescreate_worktree()(existence check +.gitcheck) before running any git commandscreate_dir_allerror to explicitly mention permission denied when applicableTest plan
classify_git_error(branch not found, permission denied, not a repo, already checked out, generic fallback)create_worktreevalidation (invalid repo path, missing repo path)test_create_worktree_branch_not_foundto verify user-friendly message content-D warningsSummary by cubic
Converts git errors into clear, actionable messages, validates repos with
git rev-parse, and hides raw stderr from the UI. Improves worktree creation reliability and satisfies Linear T-095.New Features
classify_git_error()to map common git failures (missing branch, permission denied, already checked out/locked, not a repo) to specificAppErrormessages in English and French.repo_local_pathwithtokio::fs::metadata()andgit rev-parse --git-dirto support regular and bare repos.Bug Fixes
tracing. Classified errors now propagate fromrun_git(includingrev-parse).Path::exists()withtokio::fs::metadata(), added a Unix-only guard, narrowed French "not a repo" matching, and fixed the permission test to make the parent dir read-only socreate_dir_allhits PermissionDenied.Written for commit 0a119ab. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests