feat(workspace): implement worktree.rs (T-067)#67
Conversation
…st (T-067)
Async functions using tokio::process::Command:
- create_worktree: fetch origin + worktree add with conflict detection
- remove_worktree: forceful worktree removal
- list_worktrees: parse porcelain output, exclude main tree
- worktree_exists: simple path check
- build_worktree_path: path construction helper
Worktree dir: ~/.prism/workspaces/{repo}/worktrees/pr-{number}/
7 tests covering success, error, 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 a new public Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant Client
end
rect rgba(200,255,200,0.5)
participant Worktree
end
rect rgba(255,200,200,0.5)
participant Git
end
rect rgba(255,255,200,0.5)
participant FS
end
Client->>Worktree: create_worktree(repo_local_path, branch, pr_number, repo_name, base_dir)
Worktree->>FS: build_worktree_path() & check existence
alt path exists
Worktree-->>Client: Err(AppError::Workspace::AlreadyExists)
else
Worktree->>FS: create parent directories (tokio::fs)
Worktree->>Git: run_git("fetch", "origin", branch)
Git-->>Worktree: stdout / Err(AppError::Git)
Worktree->>Git: run_git("worktree", "add", path, "origin/branch")
Git-->>Worktree: stdout / Err(AppError::Git)
Worktree-->>Client: Ok(created PathBuf)
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 implements the All four concerns raised in the previous review pass have been addressed:
Notable implementation details:
Confidence Score: 5/5Safe to merge — all previous P1 concerns are resolved and only a minor diagnostic P2 remains. All four issues from the prior review round (blocking I/O, panic risk, path traversal, wrong return type) are fully addressed. The single remaining finding is a P2 cosmetic improvement to timeout error messages. No correctness, security, or data-integrity issues remain. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant create_worktree
participant build_worktree_path
participant tokio_fs
participant run_git
Caller->>create_worktree: (repo_path, branch, pr_number, repo_name, base_dir)
create_worktree->>build_worktree_path: validate repo_name, construct path
build_worktree_path-->>create_worktree: Ok(wt_path) or Err(Workspace)
create_worktree->>create_worktree: wt_path.exists()? → Err(Workspace) if true
create_worktree->>tokio_fs: create_dir_all(wt_path.parent())
tokio_fs-->>create_worktree: Ok(()) or Err(Workspace)
create_worktree->>run_git: git fetch origin -- branch
run_git-->>create_worktree: Ok(stdout) or Err(Git)
create_worktree->>run_git: git worktree add wt_path origin/branch
run_git-->>create_worktree: Ok(stdout) or Err(Git)
create_worktree-->>Caller: Ok(wt_path)
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src-tauri/src/workspace/worktree.rs
Line: 61-64
Comment:
**Timeout error message omits the git subcommand**
`cmd_label` is set from `args.first()`, which for all `git worktree *` calls is `"worktree"`. This means a timeout on `git worktree list` produces the message `"git worktree timed out after 120s"` instead of `"git worktree list timed out after 120s"`, making it harder to diagnose which operation hung when multiple worktree commands are in-flight.
The failure-path message already exposes the full args via `args_display` — making the timeout path consistent would close the gap:
```suggestion
let cmd_label = args
.iter()
.take(2)
.map(|s| s.to_string_lossy().into_owned())
.collect::<Vec<_>>()
.join(" ");
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (6): Last reviewed commit: "fix(workspace): explicit slash check in ..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src-tauri/src/workspace/worktree.rs (4)
77-82: Blockingstd::fs::create_dir_allin async context.
std::fs::create_dir_allis a blocking call that could briefly block the async runtime. For directory creation, the impact is typically negligible, but for consistency with the async pattern, consider usingtokio::fs::create_dir_all.♻️ Proposed async alternative
// Ensure parent directories exist if let Some(parent) = wt_path.parent() { - std::fs::create_dir_all(parent).map_err(|e| { + tokio::fs::create_dir_all(parent).await.map_err(|e| { AppError::Workspace(format!("failed to create worktree directory: {e}")) })?; }This requires adding
"fs"to tokio features in Cargo.toml.🤖 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 77 - 82, Replace the blocking std::fs::create_dir_all call with the async tokio::fs::create_dir_all and await it: where you currently call std::fs::create_dir_all(parent).map_err(...)? change to tokio::fs::create_dir_all(parent).await.map_err(|e| AppError::Workspace(format!("failed to create worktree directory: {e}"))); ensure the containing function is async (or spawn a blocking task if async conversion is undesirable) and add the "fs" feature to the tokio dependency in Cargo.toml so tokio::fs is available.
28-52: Consider adding a timeout for git operations.The
run_githelper has no timeout, which could cause operations likegit fetchto hang indefinitely on network issues. This could block async tasks and degrade user experience.⏱️ Proposed timeout implementation
+use std::time::Duration; +use tokio::time::timeout; + +const GIT_TIMEOUT: Duration = Duration::from_secs(120); + async fn run_git(args: &[&str], cwd: &Path) -> Result<String, AppError> { - let output = Command::new("git") + let output = timeout(GIT_TIMEOUT, Command::new("git") .args(args) .current_dir(cwd) - .output() - .await + .output()) + .await + .map_err(|_| AppError::Git(format!( + "git {} timed out after {}s", + args.first().unwrap_or(&""), + GIT_TIMEOUT.as_secs() + )))? .map_err(|e| { AppError::Git(format!( "failed to run git {}: {e}", args.first().unwrap_or(&"") )) })?;🤖 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 28 - 52, The run_git helper needs a timeout to avoid hanging; replace the direct Command::output().await with spawning the child (Command::spawn), then await child.wait_with_output() wrapped in tokio::time::timeout(Duration::from_secs(<N>), ...); if timeout occurs, call child.kill().await (handle errors) and return an AppError::Git indicating a git operation timeout; preserve the existing error mapping for spawn failures and preserve the current handling of non-success exit status and stdout/stderr logic in run_git.
17-26: Consider sanitizingrepo_nameto prevent path traversal.If
repo_namecontains..or/, it could cause the worktree to be created outside the intended directory structure. While the input likely comes from GitHub API data, defensive validation would prevent potential misuse.🛡️ Proposed sanitization
pub fn build_worktree_path(base_dir: &Path, repo_name: &str, pr_number: u32) -> PathBuf { + // Basic sanitization: reject path separators and traversal + debug_assert!( + !repo_name.contains('/') && !repo_name.contains('\\') && !repo_name.contains(".."), + "repo_name must not contain path separators or traversal sequences" + ); base_dir .join(repo_name) .join("worktrees") .join(format!("pr-{pr_number}")) }Alternatively, strip or replace problematic characters at runtime if the input source is not fully trusted.
🤖 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 17 - 26, build_worktree_path currently joins repo_name directly which allows path traversal (e.g., repo_name with ".." or "/" )—sanitize repo_name first by normalizing or rejecting unsafe input: remove or replace path-separators and dot-only segments (e.g., replace '/' and '\' with '_' and strip leading/trailing '.' or collapse sequences of ".."), or validate against a whitelist (alphanumeric, '-', '_', '.') and return an error for invalid names; then use the sanitized_repo_name when building the PathBuf in build_worktree_path so worktrees cannot be created outside the intended directory.
7-15: WrapPRismin backticks for clippy'sdoc_markdownlint.The doc comment should use backticks around
PRismper project conventions.📝 Proposed fix
-/// Returns the default base directory for `PRism` workspaces: `~/.prism/workspaces`. +/// Returns the default base directory for [`PRism`] workspaces: `~/.prism/workspaces`.Based on learnings: "For Rust doc comments, the name should be backtick-wrapped as
PRism. This is required for clippy's doc_markdown lint."🤖 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 7 - 15, Update the doc comment for the function default_base_dir to wrap PRism in backticks (i.e., change "PRism" to "`PRism`") so it satisfies clippy's doc_markdown lint; the comment immediately above the pub fn default_base_dir() is the location to edit.
🤖 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 77-82: Replace the blocking std::fs::create_dir_all call with the
async tokio::fs::create_dir_all and await it: where you currently call
std::fs::create_dir_all(parent).map_err(...)? change to
tokio::fs::create_dir_all(parent).await.map_err(|e|
AppError::Workspace(format!("failed to create worktree directory: {e}")));
ensure the containing function is async (or spawn a blocking task if async
conversion is undesirable) and add the "fs" feature to the tokio dependency in
Cargo.toml so tokio::fs is available.
- Around line 28-52: The run_git helper needs a timeout to avoid hanging;
replace the direct Command::output().await with spawning the child
(Command::spawn), then await child.wait_with_output() wrapped in
tokio::time::timeout(Duration::from_secs(<N>), ...); if timeout occurs, call
child.kill().await (handle errors) and return an AppError::Git indicating a git
operation timeout; preserve the existing error mapping for spawn failures and
preserve the current handling of non-success exit status and stdout/stderr logic
in run_git.
- Around line 17-26: build_worktree_path currently joins repo_name directly
which allows path traversal (e.g., repo_name with ".." or "/" )—sanitize
repo_name first by normalizing or rejecting unsafe input: remove or replace
path-separators and dot-only segments (e.g., replace '/' and '\' with '_' and
strip leading/trailing '.' or collapse sequences of ".."), or validate against a
whitelist (alphanumeric, '-', '_', '.') and return an error for invalid names;
then use the sanitized_repo_name when building the PathBuf in
build_worktree_path so worktrees cannot be created outside the intended
directory.
- Around line 7-15: Update the doc comment for the function default_base_dir to
wrap PRism in backticks (i.e., change "PRism" to "`PRism`") so it satisfies
clippy's doc_markdown lint; the comment immediately above the pub fn
default_base_dir() is the location to edit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b677243b-2b7f-41bb-9faf-425655173720
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
src-tauri/Cargo.tomlsrc-tauri/src/workspace/mod.rssrc-tauri/src/workspace/worktree.rs
![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}
…, Result types - run_git: add 120s timeout via tokio::time::timeout - build_worktree_path: validate repo_name against path traversal (/, \, ..) - default_base_dir: return Result instead of panicking on missing HOME - create_dir_all: use tokio::fs for async consistency - list_worktrees: return Vec<PathBuf> instead of Vec<String> - Add tokio "fs" feature
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 116-119: The call to run_git currently passes a UTF-8 lossy string
from wt_path.to_string_lossy(), corrupting non‑UTF8 paths; change run_git to
accept OS-string arguments (e.g., &[impl AsRef<OsStr>] or &[OsString]) instead
of &str and update its use of Command::args to forward those OsStrs directly,
then replace the callers (the worktree add invocation using wt_path and the
other call at the second location) to pass the Path/OsStr (e.g.,
wt_path.as_os_str() or the PathBuf itself) rather than wt_str so git receives
the exact OS bytes.
- Around line 35-42: The current repo_name validation uses string checks and
still allows values like "." or "C:" which are not a single normal path
component; change the validation to parse Path::new(&repo_name).components() and
require that it yields exactly one component and that component matches
Component::Normal(_), returning the same AppError::Workspace message on failure;
update the logic that currently checks repo_name.is_empty() ||
repo_name.contains('/')... to use this components-based check before calling
base_dir.join(repo_name) so only a single normal component is accepted.
- Around line 57-60: The timeout currently wraps Command::output(), which only
cancels waiting while leaving the spawned git child running; instead, spawn the
process, call child.kill_on_drop(true) on the returned Child, then wrap
child.wait_with_output() in the timeout so a timed-out operation will also
terminate the git process. Concretely, replace Command::new(...).output() with
Command::new(...).spawn(), call child.kill_on_drop(true) on the spawned Child,
and then use timeout(GIT_TIMEOUT, child.wait_with_output()) to obtain the
output.
🪄 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: bd36a206-27cd-4b33-bcdc-67173aca1941
📒 Files selected for processing (2)
src-tauri/Cargo.tomlsrc-tauri/src/workspace/worktree.rs
✅ Files skipped from review due to trivial changes (1)
- src-tauri/Cargo.toml
| let output = timeout( | ||
| GIT_TIMEOUT, | ||
| Command::new("git").args(args).current_dir(cwd).output(), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and check if it exists
find . -name "worktree.rs" -type fRepository: mpiton/prism
Length of output: 93
🏁 Script executed:
# Read the specific lines mentioned in the review
cat -n src-tauri/src/workspace/worktree.rs | sed -n '50,70p'Repository: mpiton/prism
Length of output: 895
🏁 Script executed:
# Check the entire file to understand context and search for timeout/Command patterns
wc -l src-tauri/src/workspace/worktree.rsRepository: mpiton/prism
Length of output: 95
🏁 Script executed:
# Search for kill_on_drop in the file
grep -n "kill_on_drop" src-tauri/src/workspace/worktree.rsRepository: mpiton/prism
Length of output: 38
🏁 Script executed:
# Check the entire run_git function to see full context
cat -n src-tauri/src/workspace/worktree.rs | sed -n '51,80p'Repository: mpiton/prism
Length of output: 1231
🏁 Script executed:
# Look for other similar git command patterns in the file
grep -n "Command::new" src-tauri/src/workspace/worktree.rsRepository: mpiton/prism
Length of output: 171
🏁 Script executed:
# Check Cargo.toml for tokio version
find . -name "Cargo.toml" -type f -exec grep -l "tokio" {} \; | head -5Repository: mpiton/prism
Length of output: 78
🏁 Script executed:
# Check tokio version in the Cargo.toml
grep -A 2 "^tokio" ./src-tauri/Cargo.tomlRepository: mpiton/prism
Length of output: 293
🏁 Script executed:
# Also check the line 181 context to see if there's a similar pattern
cat -n src-tauri/src/workspace/worktree.rs | sed -n '175,190p'Repository: mpiton/prism
Length of output: 638
🏁 Script executed:
# Search for timeout usage in the entire file to see the pattern
grep -n "timeout" src-tauri/src/workspace/worktree.rsRepository: mpiton/prism
Length of output: 179
🏁 Script executed:
# Check imports to understand what timeout function is being used
head -n 20 src-tauri/src/workspace/worktree.rsRepository: mpiton/prism
Length of output: 737
🏁 Script executed:
# Look for any issue tracking or comments about git processes
grep -r "kill_on_drop\|zombie\|process.*leak\|git.*timeout" src-tauri/src/workspace/ 2>/dev/nullRepository: mpiton/prism
Length of output: 38
🏁 Script executed:
# Check if there are any tests that might show the impact
grep -A 10 -B 5 "fn.*test" src-tauri/src/workspace/worktree.rs | head -40Repository: mpiton/prism
Length of output: 1365
🏁 Script executed:
# Verify the default behavior by checking tokio docs reference in comments
grep -n "kill_on_drop\|drop.*child\|spawn.*child" src-tauri/src/workspace/worktree.rsRepository: mpiton/prism
Length of output: 38
🏁 Script executed:
# Check if there are any related issues or TODOs in the codebase
rg -i "todo|fixme|bug|issue" src-tauri/src/workspace/worktree.rsRepository: mpiton/prism
Length of output: 38
🏁 Script executed:
# Look at what happens with the Command in the test function
cat -n src-tauri/src/workspace/worktree.rs | sed -n '179,195p'Repository: mpiton/prism
Length of output: 663
🏁 Script executed:
# Check the entire file for any cleanup or abort patterns
rg "abort|kill|terminate" src-tauri/src/workspace/worktree.rsRepository: mpiton/prism
Length of output: 38
🌐 Web query:
tokio::process::Child kill_on_drop default behavior timeout
💡 Result:
The default behavior of tokio::process::Child when dropped (with kill_on_drop at its default of false) is that the child process continues running independently, matching std::process::Child behavior. It does not get killed automatically. When kill_on_drop(true) is explicitly set on the Command before spawning, dropping the Child sends a kill signal (SIGKILL on Unix) synchronously in the Drop implementation. However, reaping the resulting zombie process cannot be awaited in Drop (to avoid blocking). The Tokio runtime makes a best-effort attempt to reap it later when another process Child is waited on, but there are no guarantees—zombies may persist if no further processes are spawned. There is no built-in timeout for the kill operation itself, as killing is synchronous. For graceful shutdown with timeout, users must implement it manually using Child::id with signals like SIGTERM followed by tokio::time::timeout on wait, or select! with kill.
Citations:
- 1: https://docs.rs/tokio/latest/tokio/process/struct.Child.html
- 2: https://docs.rs/tokio/latest/%20tokio/process/struct.Child.html
- 3: https://docs.rs/tokio/latest/tokio/process/struct.Command.html
- 4: tokio::process::Command leaves zombies when child future is dropped tokio-rs/tokio#2685
- 5: Gracefully clean up Child on drop instead of SIGKILL? tokio-rs/tokio#2504
Set kill_on_drop(true) to terminate timed-out git commands.
Wrapping Command::output() in timeout() only cancels the wait—the spawned child process continues running after drop by default. A timed-out fetch or worktree add can mutate repository state even after this function returns an error.
Suggested fix
- let output = timeout(
- GIT_TIMEOUT,
- Command::new("git").args(args).current_dir(cwd).output(),
- )
+ let mut cmd = Command::new("git");
+ cmd.kill_on_drop(true).args(args).current_dir(cwd);
+
+ let output = timeout(GIT_TIMEOUT, cmd.output())📝 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.
| let output = timeout( | |
| GIT_TIMEOUT, | |
| Command::new("git").args(args).current_dir(cwd).output(), | |
| ) | |
| let mut cmd = Command::new("git"); | |
| cmd.kill_on_drop(true).args(args).current_dir(cwd); | |
| let output = timeout(GIT_TIMEOUT, cmd.output()) |
🤖 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 57 - 60, The timeout
currently wraps Command::output(), which only cancels waiting while leaving the
spawned git child running; instead, spawn the process, call
child.kill_on_drop(true) on the returned Child, then wrap
child.wait_with_output() in the timeout so a timed-out operation will also
terminate the git process. Concretely, replace Command::new(...).output() with
Command::new(...).spawn(), call child.kill_on_drop(true) on the spawned Child,
and then use timeout(GIT_TIMEOUT, child.wait_with_output()) to obtain the
output.
|
Claude finished @mpiton's task in 4m 40s —— View job Fixed remaining review issues in worktree.rsAll open issues from the coderabbitai and greptile review rounds have been addressed. Commit: Changes made:
|
- Use Path::components() validation in build_worktree_path to also reject
single-dot names ("." resolves to CurDir, not Normal)
- Change run_git to accept &[OsString] so paths are passed verbatim to git
without lossy UTF-8 conversion (removes to_string_lossy() on wt_path)
- Add kill_on_drop(true) on Command so timed-out git processes are killed
- Add -- separator in git fetch to prevent branch names starting with -
being interpreted as flags
- Document parent-dir cleanup behavior in create_worktree doc comment
- Add test case for "." rejection in test_worktree_path_construction
Co-authored-by: Mathieu <mpiton@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src-tauri/src/workspace/worktree.rs (3)
231-259: Consider adding test case for"."repo_name.The validation tests at lines 254-258 verify rejection of
"..","owner/repo","a\\b", and empty strings, but don't test the"."case which (per the earlier validation issue) currently passes but shouldn't.// After fixing validation, add: assert!(build_worktree_path(&base, ".", 1).is_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 231 - 259, Add a test asserting that a repo_name of "." is rejected by build_worktree_path: call build_worktree_path(&base, ".", 1) and assert it returns an error; if validation logic lives in a helper (e.g., sanitize_repo_name or validate_repo_name) update that function to treat "." as invalid (same as "" and ".." and paths containing separators) so the new test fails until validation is fixed, then commit the added assertion to test_worktree_path_construction.
98-98: Consider async existence check for consistency.
wt_path.exists()is synchronous I/O in an async function. Whilestat()is typically fast, usingtokio::fs::try_exists()would maintain consistency with the async filesystem operations used elsewhere in this function (line 107).Suggested fix
- if wt_path.exists() { + if tokio::fs::try_exists(&wt_path).await.unwrap_or(false) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src-tauri/src/workspace/worktree.rs` at line 98, Replace the synchronous wt_path.exists() call with the async tokio helper: call tokio::fs::try_exists(&wt_path).await and handle the Result like other async FS ops in this function (e.g., propagate the error with ? or map to a bool consistent with surrounding error handling); update the conditional that currently uses wt_path.exists() to use the awaited boolean from try_exists instead.
160-164: Consider making this async for consistency.This is a synchronous function that may be called from async contexts. While
path.exists()is a quick syscall, an async version usingtokio::fs::try_exists()would align with the module's async patterns.🤖 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 160 - 164, The synchronous worktree_exists function should be converted to an async function: change pub fn worktree_exists(path: &Path) -> bool to pub async fn worktree_exists(path: &Path) -> std::io::Result<bool> (or -> bool if you prefer to map errors) and implement it using tokio::fs::try_exists(path).await; update any call sites to .await and import tokio::fs::try_exists; if you prefer to keep a bool API, map the Result to a bool (e.g., Ok(b) => b, Err(_) => false) but prefer surfacing the io::Result for callers that need error handling.
🤖 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 231-259: Add a test asserting that a repo_name of "." is rejected
by build_worktree_path: call build_worktree_path(&base, ".", 1) and assert it
returns an error; if validation logic lives in a helper (e.g.,
sanitize_repo_name or validate_repo_name) update that function to treat "." as
invalid (same as "" and ".." and paths containing separators) so the new test
fails until validation is fixed, then commit the added assertion to
test_worktree_path_construction.
- Line 98: Replace the synchronous wt_path.exists() call with the async tokio
helper: call tokio::fs::try_exists(&wt_path).await and handle the Result like
other async FS ops in this function (e.g., propagate the error with ? or map to
a bool consistent with surrounding error handling); update the conditional that
currently uses wt_path.exists() to use the awaited boolean from try_exists
instead.
- Around line 160-164: The synchronous worktree_exists function should be
converted to an async function: change pub fn worktree_exists(path: &Path) ->
bool to pub async fn worktree_exists(path: &Path) -> std::io::Result<bool> (or
-> bool if you prefer to map errors) and implement it using
tokio::fs::try_exists(path).await; update any call sites to .await and import
tokio::fs::try_exists; if you prefer to keep a bool API, map the Result to a
bool (e.g., Ok(b) => b, Err(_) => false) but prefer surfacing the io::Result for
callers that need error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9335577-a0cf-4925-98be-83ae51471c53
📒 Files selected for processing (1)
src-tauri/src/workspace/worktree.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src-tauri/src/workspace/worktree.rs (1)
118-123: Consider using async filesystem check for consistency.The function uses
tokio::fs::create_dir_all(line 127) butwt_path.exists()is a blocking call. For consistency with the async filesystem operations used elsewhere, consider usingtokio::fs::try_exists().♻️ Suggested change
- if wt_path.exists() { + 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 118 - 123, Replace the blocking wt_path.exists() check with the async tokio::fs::try_exists(wt_path).await call: call tokio::fs::try_exists with the same wt_path, handle the Result<bool, std::io::Error> (map the io::Error into your AppError::Workspace or propagate using ?), and use the returned bool to decide the existing-worktree error (same message) before calling tokio::fs::create_dir_all; ensure any necessary tokio::fs import is added and adjust control flow to await the async existence check.
🤖 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 37-40: The binding is failing rustfmt: reformat the matches!
expression that computes is_single_normal (the variables components and
is_single_normal in worktree.rs) so it conforms to rustfmt style; update the
multiline formatting around matches!(components.next(),
Some(std::path::Component::Normal(_))) && components.next().is_none() and then
run cargo fmt to apply the canonical formatting.
- Around line 134-143: The argument array passed to run_git (the slice created
with "fetch".into(), "origin".into(), "--".into(), branch.into()) is formatted
in a way that fails rustfmt; reformat the array to match rustfmt's preferred
layout (or simply run cargo fmt) so the call in worktree.rs that invokes
run_git(...) conforms to CI formatting rules; ensure the run_git call and its
argument slice remain unchanged functionally but are reformatted to satisfy
rustfmt.
---
Nitpick comments:
In `@src-tauri/src/workspace/worktree.rs`:
- Around line 118-123: Replace the blocking wt_path.exists() check with the
async tokio::fs::try_exists(wt_path).await call: call tokio::fs::try_exists with
the same wt_path, handle the Result<bool, std::io::Error> (map the io::Error
into your AppError::Workspace or propagate using ?), and use the returned bool
to decide the existing-worktree error (same message) before calling
tokio::fs::create_dir_all; ensure any necessary tokio::fs import is added and
adjust control flow to await the async existence check.
🪄 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: 98b5cb12-995d-4ae2-8fbf-10ba1f389922
📒 Files selected for processing (1)
src-tauri/src/workspace/worktree.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 37-43: The validation currently uses
Path::new(repo_name).components() and is_single_normal but misses explicit
forward-slash checks so inputs like "repo/" or "repo/." slip through; update the
check that currently tests repo_name.contains('\\') to also explicitly reject
repo_name.contains('/') (and any other path-separator-like characters you deem
necessary) so that the branch that returns an error when !is_single_normal ||
repo_name.contains('\\') becomes !is_single_normal || repo_name.contains('\\')
|| repo_name.contains('/'), keeping the rest of the logic (variables: repo_name,
is_single_normal, components) intact.
- Around line 177-180: The call to run_git inside list_worktrees currently uses
"--porcelain" and parses output with .lines(), which breaks on worktree paths
containing newlines; change the args to include "-z" (i.e., "--porcelain", "-z")
when invoking run_git and update the parsing logic to split on NUL bytes instead
of newline boundaries (use output.split('\0') and handle any trailing empty
element) when interpreting entries like the "worktree " and related records;
update any downstream matching that assumes line endings accordingly in the
list_worktrees function and the code that consumes its results.
🪄 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: 662f6919-05b9-48ca-bdac-edaff476618f
📒 Files selected for processing (1)
src-tauri/src/workspace/worktree.rs
…tree list
- build_worktree_path: add contains('/') check to reject "repo/" aliasing
- list_worktrees: use --porcelain -z with split('\0') for path-safe parsing
1 participant
Summary
worktree.rsmodule with async git worktree operations for PR workspacescreate_worktree: fetches remote branch then creates worktree at~/.prism/workspaces/{repo}/worktrees/pr-{number}/remove_worktree: forceful worktree removal viagit worktree remove --forcelist_worktrees: parsesgit worktree list --porcelainoutput, excludes main treeworktree_exists+build_worktree_path: path utilitiesprocessfeature to tokio fortokio::process::CommandTest plan
test_create_worktree_success— creates worktree, verifies path and.gitfiletest_create_worktree_branch_not_found— non-existent branch returnsAppError::Gittest_create_worktree_already_exists— duplicate creation returnsAppError::Workspacetest_remove_worktree— creates then removes, verifies directory gonetest_remove_worktree_not_found— non-existent path returnsAppError::Gittest_list_worktrees— empty list then one worktree after creationtest_worktree_path_construction— path format validationcargo clippy -- -D warningscleanSummary by cubic
Implements async Git worktree management for PR workspaces with create/remove/list under ~/.prism/workspaces/{repo}/worktrees/pr-{number}/. Adds NUL-delimited parsing and stricter repo name validation to meet T-067.
New Features
workspace::worktreewith:create_worktree,remove_worktree,list_worktrees,worktree_exists,build_worktree_path.create_worktreefetchesorigin/{branch}(guards flag-like names with--) and creates the worktree;list_worktreesparses--porcelain -z, skips the main tree, and returnsVec<PathBuf>.Bug Fixes
kill_on_dropto stop hung processes...,.), slashes (/,\), and trailing-slash aliasing;default_base_dirnow returnsResult.OsStringto pass non‑UTF‑8 paths verbatim.tokio::fs; enabledtokiofeaturesprocessandfs.Written for commit 85a009e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Chores
Tests