fix: prevent removing current worktree #33
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds centralized worktree-managed detection and runtime path guards to the remove flow: resolve absolute target and cwd, ensure the target is not the current worktree (or a nested path), return a new CannotRemoveCurrentWorktree error when appropriate, and add tests for that behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RemoveCmd as Remove Command
participant PathChecker as Path Validator
participant Executor as Command Executor
User->>RemoveCmd: request remove <targetPath> (cwd provided)
RemoveCmd->>RemoveCmd: resolve absTargetPath, resolve absCwd
RemoveCmd->>PathChecker: isPathWithin(absCwd, absTargetPath)?
alt target is equal or nested inside cwd
PathChecker-->>RemoveCmd: true
RemoveCmd->>User: return CannotRemoveCurrentWorktree error
else target is outside cwd
PathChecker-->>RemoveCmd: false
RemoveCmd->>Executor: run git worktree remove / shell removal
Executor-->>User: command result (success/failure)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
cmd/wtp/**📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (2)📚 Learning: 2025-10-07T15:56:11.510ZApplied to files:
📚 Learning: 2025-10-07T15:56:11.510ZApplied to files:
🧬 Code graph analysis (2)cmd/wtp/worktree_managed.go (1)
cmd/wtp/remove.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR adds safety checks to prevent users from accidentally removing the worktree that contains their current working directory. It introduces validation logic that detects when the target worktree path contains the current directory and provides helpful error messaging with guidance on how to switch worktrees before retrying the removal.
- Adds
isPathWithinhelper function to detect if current directory is within the target worktree - Creates
CannotRemoveCurrentWorktreeerror function with user-friendly guidance - Extends test coverage for the new validation logic and error handling
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cmd/wtp/remove.go | Adds current directory validation logic and path comparison helper |
| internal/errors/errors.go | Introduces new error function for current worktree removal attempts |
| internal/errors/errors_test.go | Adds unit test for the new error function |
| cmd/wtp/remove_test.go | Adds comprehensive test coverage for current worktree removal prevention |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return errors.DirectoryAccessFailed("access current", cwd, err) | ||
| } | ||
|
|
||
| if isPathWithin(absTargetPath, absCwd) { |
There was a problem hiding this comment.
The function call arguments are reversed. Based on the function logic, isPathWithin(basePath, targetPath) should be called as isPathWithin(absCwd, absTargetPath) to check if the current working directory is within the target worktree path.
| if isPathWithin(absTargetPath, absCwd) { | |
| if isPathWithin(absCwd, absTargetPath) { |
Copilot uses AI. Check for mistakes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/wtp/remove.go(3 hunks)cmd/wtp/remove_test.go(2 hunks)internal/errors/errors.go(1 hunks)internal/errors/errors_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Run gofmt and goimports; keep imports grouped and organized; local import prefix follows module path github.com/satococoa/wtp
Adhere to golangci-lint rules configured for the project (vet, staticcheck, gosec, mnd, lll=120, etc.)
Errors should be wrapped with context and must not be ignored
Use snake_case for Go filenames; document exported identifiers when non-trivial
Files:
internal/errors/errors.gocmd/wtp/remove_test.gointernal/errors/errors_test.gocmd/wtp/remove.go
internal/{git,config,hooks,command,errors,io}/**
📄 CodeRabbit inference engine (AGENTS.md)
Place internal packages within internal/{git,config,hooks,command,errors,io}
Files:
internal/errors/errors.gointernal/errors/errors_test.go
cmd/wtp/**
📄 CodeRabbit inference engine (AGENTS.md)
cmd/wtp/**: CLI entrypoint and commands are implemented under cmd/wtp
Update CLI help text to reflect user-facing changes
Command behavior: wtp cd prints only the absolute worktree path with no side effects
Command behavior: wtp completion generates pure completion scripts via urfave/cli
Command behavior: wtp hook emits shell functions that intercept wtp cd; wtp shell-init combines completion and hook output
Files:
cmd/wtp/remove_test.gocmd/wtp/remove.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Unit tests live alongside packages and use *_test.go naming
Prefer table-driven tests and mock git operations in unit tests
Files:
cmd/wtp/remove_test.gointernal/errors/errors_test.go
🧠 Learnings (2)
📚 Learning: 2025-10-07T15:56:11.510Z
Learnt from: CR
PR: satococoa/wtp#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T15:56:11.510Z
Learning: Applies to cmd/wtp/completion.go : Use getWorktreeNameFromPath() to resolve worktree display names consistently across completion, errors, and parsing
Applied to files:
cmd/wtp/remove.go
📚 Learning: 2025-10-07T15:56:11.510Z
Learnt from: CR
PR: satococoa/wtp#0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T15:56:11.510Z
Learning: Applies to cmd/wtp/** : Command behavior: wtp cd prints only the absolute worktree path with no side effects
Applied to files:
cmd/wtp/remove.go
🧬 Code graph analysis (3)
cmd/wtp/remove_test.go (1)
internal/command/types.go (2)
Result(11-15)Command(4-8)
internal/errors/errors_test.go (1)
internal/errors/errors.go (1)
CannotRemoveCurrentWorktree(168-173)
cmd/wtp/remove.go (1)
internal/errors/errors.go (3)
WorktreeRemovalFailed(133-166)DirectoryAccessFailed(240-264)CannotRemoveCurrentWorktree(168-173)
🪛 GitHub Actions: CI
cmd/wtp/remove_test.go
[error] 432-432: Command failed: go test -race -coverprofile=coverage.out -covermode=atomic ./...; Test failure in TestRemoveCommand_FailsWhenRemovingCurrentWorktree/exact_worktree_path: expected error to contain "cannot remove worktree 'feature/foo'" but got "worktree 'feature/foo' not found\n\nNo worktrees found.\n\nTip: Run 'wtp list' to see all worktrees"
[error] 432-432: Command failed: go test -race -coverprofile=coverage.out -covermode=atomic ./...; Test failure in TestRemoveCommand_FailsWhenRemovingCurrentWorktree/nested_directory_inside_worktree: expected error to contain "cannot remove worktree 'feature/foo'" but got "worktree 'feature/foo' not found\n\nNo worktrees found.\n\nTip: Run 'wtp list' to see all worktrees"
🪛 GitHub Actions: E2E Tests
cmd/wtp/remove_test.go
[error] 432-432: TestRemoveCommand_FailsWhenRemovingCurrentWorktree (exact_worktree_path) failed: expected error message to contain "cannot remove worktree 'feature/foo'" but got: "worktree 'feature/foo' not found\n\nNo worktrees found.\n\nTip: Run 'wtp list' to see all worktrees"
[error] 432-432: TestRemoveCommand_FailsWhenRemovingCurrentWorktree (nested_directory_inside_worktree) failed: expected error message to contain "cannot remove worktree 'feature/foo'" but got: "worktree 'feature/foo' not found\n\nNo worktrees found.\n\nTip: Run 'wtp list' to see all worktrees"
🔇 Additional comments (6)
internal/errors/errors_test.go (1)
214-221: LGTM!The test structure follows the established patterns in the file and appropriately validates that the error message includes all critical information: the worktree name, current location, and helpful guidance.
internal/errors/errors.go (1)
168-173: LGTM!The error message is clear, informative, and provides actionable guidance. The tip includes specific commands (
wtp cd @andwtp cd <another-worktree>) that help users resolve the issue immediately.cmd/wtp/remove_test.go (1)
6-6: LGTM!The
fmtimport is correctly added to support the test's use offmt.Sprintffor constructing mock worktree list output.cmd/wtp/remove.go (3)
136-136: LGTM!The parameter name
cwdclearly indicates it represents the current working directory, making the function signature more self-documenting.
156-168: LGTM!The path validation logic correctly prevents removing the current worktree by:
- Computing absolute paths for both the target worktree and current directory
- Checking if the current directory is within the target worktree path
- Returning an appropriate error with helpful guidance
This covers both exact path matches and nested directories within the worktree.
206-221: LGTM!The
isPathWithinhelper correctly determines path containment usingfilepath.Rel:
- Returns
truewhentargetPathequals or is withinbasePath- Returns
falsewhentargetPathis outsidebasePath(relative path starts with..)- Safely returns
falseon errorsThe logic properly handles the use case where we need to check if the current directory (
absCwd) is within the worktree being removed (absTargetPath).
|
@codex fix tests. |
|
Summary
Testing
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/wtp/remove_test.go (1)
393-441: Good fix and coverage for current-worktree guard.Path now aligns with default base_dir (/repo/worktrees), resolving the earlier “worktree not found” mismatch; table covers exact and nested cwd cases and asserts no remove command runs.
🧹 Nitpick comments (1)
cmd/wtp/remove.go (1)
206-221: ClarifyisPathWithinsemantics.Add a brief comment/docstring noting it returns true when targetPath is equal to or within basePath; prevents future call-site mixups.
-func isPathWithin(basePath, targetPath string) bool { +// isPathWithin returns true if targetPath is the same as, or a descendant of, basePath. +func isPathWithin(basePath, targetPath string) bool {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/wtp/remove.go(3 hunks)cmd/wtp/remove_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Run gofmt and goimports; keep imports grouped and organized; local import prefix follows module path github.com/satococoa/wtp
Adhere to golangci-lint rules configured for the project (vet, staticcheck, gosec, mnd, lll=120, etc.)
Errors should be wrapped with context and must not be ignored
Use snake_case for Go filenames; document exported identifiers when non-trivial
Files:
cmd/wtp/remove.gocmd/wtp/remove_test.go
cmd/wtp/**
📄 CodeRabbit inference engine (AGENTS.md)
cmd/wtp/**: CLI entrypoint and commands are implemented under cmd/wtp
Update CLI help text to reflect user-facing changes
Command behavior: wtp cd prints only the absolute worktree path with no side effects
Command behavior: wtp completion generates pure completion scripts via urfave/cli
Command behavior: wtp hook emits shell functions that intercept wtp cd; wtp shell-init combines completion and hook output
Files:
cmd/wtp/remove.gocmd/wtp/remove_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Unit tests live alongside packages and use *_test.go naming
Prefer table-driven tests and mock git operations in unit tests
Files:
cmd/wtp/remove_test.go
🧬 Code graph analysis (2)
cmd/wtp/remove.go (1)
internal/errors/errors.go (3)
WorktreeRemovalFailed(133-166)DirectoryAccessFailed(240-264)CannotRemoveCurrentWorktree(168-173)
cmd/wtp/remove_test.go (1)
internal/command/types.go (2)
Result(11-15)Command(4-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests (macos-latest)
🔇 Additional comments (2)
cmd/wtp/remove_test.go (1)
6-6: Import addition is appropriate.fmt is needed for Sprintf below; no issues.
cmd/wtp/remove.go (1)
132-139: Passing cwd improves testability and clarity.Decouples path resolution from execution; nice.
satococoa
deleted the
codex/design-wtp-remove-behavior-for-current-wotktree
branch
1 participant
Summary
wtp removefrom deleting the worktree that contains the current directoryTesting
https://chatgpt.com/codex/tasks/task_e_68f390c09fa88327957ca9a1b618abec
Summary by CodeRabbit