feat(clean): add interactive worktree cleanup command#89
feat(clean): add interactive worktree cleanup command#89fdarian wants to merge 11 commits intosatococoa:mainfrom
Conversation
Add GitStatusPorcelain and GitLogNotPushed commands to support the new clean command's validation logic.
Add wtp clean command with interactive checkbox UI using charmbracelet/huh. Worktrees are pre-selected if they are: - Fully merged into the main branch - Have no uncommitted changes - Have no unpushed commits Supports both arrow keys and vim keys (j/k) for navigation.
Add tests for validation logic, option building, and command execution. Covers safe/unsafe worktree detection and edge cases.
Validate worktrees concurrently using goroutines instead of sequentially. Also hoists detectMainBranch out of the per-worktree loop so it's called once rather than N times.
Align option labels into three columns (name, status, note) with dynamic padding based on the longest worktree name. Add a column header via the MultiSelect Description field.
Add 4-space left padding to the column header to account for the huh MultiSelect bullet prefix (> • / ○).
📝 WalkthroughWalkthroughAdds a new interactive Changes
Sequence DiagramsequenceDiagram
participant User
participant CleanCmd as Clean Command
participant Executor as Git Executor
participant Validator as Validator
participant Form as Interactive Form
participant Remover as Remover
User->>CleanCmd: run `wtp clean`
CleanCmd->>Executor: git worktree list
Executor-->>CleanCmd: worktrees + main path
CleanCmd->>Validator: load config, filter managed worktrees
Validator-->>CleanCmd: managed worktrees
CleanCmd->>Validator: validate worktrees (parallel)
par Checks per worktree
Validator->>Executor: git merge-base / main branch
Validator->>Executor: git status --porcelain (porcelain)
Validator->>Executor: git log (not pushed)
end
Validator-->>CleanCmd: statuses (safe/unsafe + reasons)
CleanCmd->>Form: build multi-select options (safe pre-selected)
Form-->>CleanCmd: selected worktrees
CleanCmd->>Remover: remove selected
par Removals
Remover->>Executor: git worktree remove [--force]
Executor-->>Remover: result
end
Remover-->>User: report successes/failures
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@cmd/wtp/clean_test.go`:
- Around line 761-790: The mockCleanCommandExecutor.matchCommand currently
treats any command with arg0 == "rev-list" the same, so two different rev-list
calls from checkPushStatus (origin/<branch>..<branch> vs <main>..<branch>) both
return m.results["rev-list"] and never m.results["ahead-cmd"]; update
matchCommand to inspect cmd.Args (use strings.Contains or join) to distinguish
the two rev-list invocations and return m.results["ahead-cmd"] for the rev-list
that computes the ahead count (the origin/<branch>..<branch> or the specific
pattern used by checkPushStatus), leaving the other rev-list to return
m.results["rev-list"]; reference function names matchCommand and checkPushStatus
and map keys "rev-list" and "ahead-cmd".
- Around line 279-342: The test TestBuildCleanOptions is missing assertions for
pre-selected counts and cannot inspect huh.Option[string] selected state because
huh.Option only exposes Selected(bool) setter; update buildCleanOptions to
return a testable representation (e.g., change its return type or add a helper
like cleanOptionsWithState containing options []huh.Option[string] and Selected
[]bool or a slice of struct{ Opt huh.Option[string]; Selected bool }) so the
selection flag is produced alongside each option, update buildCleanOptions (and
any callers) to populate that Selected value based on
worktreeCleanStatus.isSafe, and then add assertions in TestBuildCleanOptions to
compare tt.expectedSelected and tt.expectedUnselected against the returned
Selected flags while still verifying len(opts.options) and option keys;
alternatively, if you prefer minimal change, introduce a thin wrapper type or
factory used by buildCleanOptions that records selection state for tests and
assert against that wrapper in the test.
In `@cmd/wtp/clean.go`:
- Around line 120-138: The function getWorktreesForClean currently dereferences
result.Results[0].Output without validating result or its Results slice; update
getWorktreesForClean to first check that result is non-nil and
len(result.Results) > 0 and return a descriptive error (e.g.,
errors.GitCommandFailed("git worktree list", "no results returned") or similar)
if the slice is empty or result is nil, then safely use result.Results[0].Output
to call parseWorktreesFromOutput; reference getWorktreesForClean and
result.Results[0].Output when making the change.
- Around line 305-322: In checkCleanStatus, don't ignore the error from
executor.Execute and avoid dereferencing a nil result; mirror the safe pattern
used in checkMergeStatus by capturing the error return from executor.Execute,
return (or set s.isClean=false) when an error occurs, and check result != nil
before accessing result.Results; update the function (checkCleanStatus) to
handle the error and nil result cases so it never calls len(result.Results) or
accesses result.Results[0] when result is nil.
- Around line 281-303: The checkMergeStatus function currently discards the
error returned by executor.Execute and then accesses result.Results which can
panic if result is nil; update checkMergeStatus to capture and check the error
from executor.Execute (called with mergeBaseCmd) and only access result.Results
when err == nil and result != nil, setting s.isMerged/s.isSafe and appending
reasons when an error occurs or when the command indicates unmerged; apply the
same guard-and-error-handling pattern to the other affected methods:
checkCleanStatus, checkPushStatus, and getWorktreesForClean so they validate the
returned error and result before dereferencing result.
- Around line 324-366: In checkPushStatus, don't ignore errors or assume
executor.Execute returns a non-nil result: for both calls to executor.Execute
inside checkPushStatus (the pushCheckCmd and aheadCmd invocations) capture the
returned error and check that result != nil and len(result.Results) > 0 before
accessing result.Results[0].Output; if error != nil or result is nil/empty,
return early (or treat as no output) to avoid a nil dereference, and keep
existing logic that sets s.isPushed/s.isSafe and appends reasons only when valid
output is present.
- Around line 256-279: The detectMainBranch function currently prefers "master"
when both exist and uses git rev-parse on the literal "main"; change it to
prefer "main" first and to determine the default branch by executing and parsing
the output of "git symbolic-ref refs/remotes/origin/HEAD" (via the executor) to
extract the branch name (strip "refs/remotes/origin/"), falling back to checking
explicit "main" and then "master" with rev-parse if symbolic-ref fails; update
the commands built in detectMainBranch and the return logic so it returns the
parsed default branch when available, otherwise "main" or "master" as fallbacks.
In `@go.mod`:
- Line 64: The go.mod entry for github.com/charmbracelet/huh is marked as
indirect but the package is imported directly (see cmd/wtp/clean.go where huh is
used); run `go mod tidy` to update go.mod so huh appears in the direct require
block (or manually move the require for github.com/charmbracelet/huh v0.8.0 into
the main require list) and then commit the updated go.mod and go.sum.
🧹 Nitpick comments (4)
cmd/wtp/clean.go (1)
20-29: Consider consolidatingreasonandreasonsfields.
worktreeCleanStatuscarries bothreason stringandreasons []string, wherereasonis always derived fromreasonsviabuildReason(). Keeping both creates a risk of them getting out of sync ifbuildReason()is forgotten. Consider computing the display string on demand (e.g., a method) instead of storing it.cmd/wtp/clean_test.go (2)
668-686:os.Chdirin test mutates process-global state.Lines 671-673 use
os.Chdirwhich affects the entire process. This is fragile if tests are ever run in parallel. Consider using thecleanGetwdoverride (already defined inclean.goline 32) to inject the test directory instead.Suggested approach
func TestCleanCommand_NotInGitRepo(t *testing.T) { - // Create a temporary directory that is not a git repo tempDir := t.TempDir() - oldDir, _ := os.Getwd() - defer func() { _ = os.Chdir(oldDir) }() - err := os.Chdir(tempDir) - assert.NoError(t, err) + originalGetwd := cleanGetwd + cleanGetwd = func() (string, error) { return tempDir, nil } + defer func() { cleanGetwd = originalGetwd }()
725-732:callCountfield is unused.
mockCleanCommandExecutor.callCountis incremented on line 750 but never read or asserted in any test. Either remove it or add assertions that verify call counts where it matters.internal/command/builders.go (1)
90-104: Remove or integrate unused command builders.
GitStatusPorcelainandGitLogNotPushedare defined but never called. ThecheckCleanStatus(line 306) andcheckPushStatus(line 333) functions incmd/wtp/clean.gobuild their commands inline instead:
checkCleanStatususesWorkDirfield withgit status --porcelain(vsGitStatusPorcelain's-Cflag approach)checkPushStatususesgit rev-list --count origin/<branch>..<branch>(vsGitLogNotPushed'sgit log ... --not --remotes)Either integrate these builders into the respective functions or remove them.
| func TestBuildCleanOptions(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| statuses []worktreeCleanStatus | ||
| expectedOptions int | ||
| expectedSelected int | ||
| expectedUnselected int | ||
| }{ | ||
| { | ||
| name: "all safe worktrees - all pre-selected", | ||
| statuses: []worktreeCleanStatus{ | ||
| {worktree: git.Worktree{Path: "/test/repo/.worktrees/feature1"}, isSafe: true, reason: "safe"}, | ||
| {worktree: git.Worktree{Path: "/test/repo/.worktrees/feature2"}, isSafe: true, reason: "safe"}, | ||
| }, | ||
| expectedOptions: 2, | ||
| expectedSelected: 2, | ||
| expectedUnselected: 0, | ||
| }, | ||
| { | ||
| name: "mix of safe and unsafe - only safe pre-selected", | ||
| statuses: []worktreeCleanStatus{ | ||
| {worktree: git.Worktree{Path: "/test/repo/.worktrees/safe"}, isSafe: true, reason: "safe"}, | ||
| {worktree: git.Worktree{Path: "/test/repo/.worktrees/unsafe"}, isSafe: false, reason: "unsafe"}, | ||
| }, | ||
| expectedOptions: 2, | ||
| expectedSelected: 1, | ||
| expectedUnselected: 1, | ||
| }, | ||
| { | ||
| name: "all unsafe worktrees - none pre-selected", | ||
| statuses: []worktreeCleanStatus{ | ||
| {worktree: git.Worktree{Path: "/test/repo/.worktrees/unsafe1"}, isSafe: false, reason: "unsafe"}, | ||
| {worktree: git.Worktree{Path: "/test/repo/.worktrees/unsafe2"}, isSafe: false, reason: "unsafe"}, | ||
| }, | ||
| expectedOptions: 2, | ||
| expectedSelected: 0, | ||
| expectedUnselected: 2, | ||
| }, | ||
| { | ||
| name: "empty statuses", | ||
| statuses: []worktreeCleanStatus{}, | ||
| expectedOptions: 0, | ||
| expectedSelected: 0, | ||
| expectedUnselected: 0, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| cfg := &config.Config{ | ||
| Defaults: config.Defaults{BaseDir: ".worktrees"}, | ||
| } | ||
|
|
||
| opts := buildCleanOptions(tt.statuses, cfg, "/test/repo") | ||
|
|
||
| assert.Equal(t, tt.expectedOptions, len(opts.options)) | ||
|
|
||
| // Verify options were created | ||
| for _, opt := range opts.options { | ||
| assert.NotEmpty(t, opt.Key) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the exact test code at lines 279-342
cd /tmp && git ls-files | grep -E "cmd/wtp/clean_test\.go"Repository: satococoa/wtp
Length of output: 125
🏁 Script executed:
# Read the test file to verify the claims
cat -n cmd/wtp/clean_test.go | sed -n '279,342p'Repository: satococoa/wtp
Length of output: 2441
🏁 Script executed:
# Check the buildCleanOptions function signature and return type
rg -A 10 "func buildCleanOptions" cmd/wtp/Repository: satococoa/wtp
Length of output: 691
🏁 Script executed:
# Search for the worktreeCleanOptions struct definition to understand its structure
rg -B 3 -A 10 "type.*CleanOptions|type.*Option struct" cmd/wtp/Repository: satococoa/wtp
Length of output: 39
🏁 Script executed:
# Check if huh is being imported and what version
rg "github.com/charmbracelet/huh|import.*huh" cmd/wtp/Repository: satococoa/wtp
Length of output: 159
🏁 Script executed:
# Get the complete buildCleanOptions function implementation
rg -A 30 "func buildCleanOptions" cmd/wtp/clean.goRepository: satococoa/wtp
Length of output: 1086
🏁 Script executed:
# Search for cleanOptions struct definition
rg -B 2 -A 10 "type cleanOptions struct" cmd/wtp/clean.goRepository: satococoa/wtp
Length of output: 498
🏁 Script executed:
# Check go.mod for huh version
cat go.mod | grep huhRepository: satococoa/wtp
Length of output: 105
🏁 Script executed:
# Search for how huh.Option is used in the codebase - particularly checking if Selected is a field
rg "\.Selected|huh\.Option" cmd/wtp/ -A 2Repository: satococoa/wtp
Length of output: 692
🏁 Script executed:
# Let's check if there's documentation or code showing how to access Selected state from huh.Option
# First check if there's any test in huh itself or examples showing this
web_searchRepository: satococoa/wtp
Length of output: 107
🌐 Web query:
charmbracelet huh Option v0.8.0 Selected field getter
💡 Result:
In github.com/charmbracelet/huh v0.8.0, Option[T] only has Selected(selected bool) Option[T] (a setter)—there is no “selected” getter on Option. The way to read what’s selected is via the field value (binding/accessor), or (more generically) GetValue(). [1]
- Select: read the chosen value via your bound variable (
Value(&v)) ors.GetValue()(type-assert toT). [1] - MultiSelect: read the chosen values via your bound slice (
Value(&vs)) orm.GetValue()(type-assert to[]T). [1]
Example:
s := huh.NewSelect[string]().
Options(huh.NewOptions("A", "B", "C")...)
// ...after s.Run() or form.Run():
v := s.GetValue().(string)
m := huh.NewMultiSelect[string]().
Options(huh.NewOptions("A", "B", "C")...)
// ...after m.Run() or form.Run():
vs := m.GetValue().([]string)If you meant “what option is the cursor on right now”, use Hovered() on Select/MultiSelect. [1]
Sources: pkg.go.dev API docs for huh (includes Option.Selected, Select.GetValue, MultiSelect.GetValue, Hovered). [1]
🏁 Script executed:
# Also check the actual huh library if it's in go.mod to understand the Option structure
# Let's look for any existing tests that check Selected state
rg "\.Selected\(\)" cmd/wtp/clean_test.goRepository: satococoa/wtp
Length of output: 39
🏁 Script executed:
# Check if there are any other tests that verify selected/unselected states
rg -B 3 -A 3 "isSafe.*Selected|Selected.*isSafe" cmd/wtp/Repository: satococoa/wtp
Length of output: 39
🏁 Script executed:
# Let's verify what properties/methods are available on huh.Option
# by checking how it's used in the actual code
rg "option\." cmd/wtp/clean.goRepository: satococoa/wtp
Length of output: 90
🏁 Script executed:
# Check if there are any other ways the test validates selected state
rg -B 5 -A 5 "expectedSelected|expectedUnselected" cmd/wtp/clean_test.goRepository: satococoa/wtp
Length of output: 1667
Add missing assertions for pre-selection counts — the suggested fix won't compile with huh v0.8.0.
The test correctly defines expectedSelected and expectedUnselected test cases but never asserts on them (lines 334–339 only check option count and key presence). The pre-selection logic is untested.
However, the suggested fix cannot work as written: huh.Option[string] in v0.8.0 has no getter for the selected state—Selected(bool) is only a setter method. To verify selected state, you must either:
- Refactor
buildCleanOptionsto return selection state in a testable form (e.g., a struct containing both the option and selection flag) - Use reflection to inspect the internal state (not recommended)
- Mock or wrap
huh.Optionwith a custom type that tracks selection
Add the missing assertions once a verifiable approach is chosen.
🤖 Prompt for AI Agents
In `@cmd/wtp/clean_test.go` around lines 279 - 342, The test TestBuildCleanOptions
is missing assertions for pre-selected counts and cannot inspect
huh.Option[string] selected state because huh.Option only exposes Selected(bool)
setter; update buildCleanOptions to return a testable representation (e.g.,
change its return type or add a helper like cleanOptionsWithState containing
options []huh.Option[string] and Selected []bool or a slice of struct{ Opt
huh.Option[string]; Selected bool }) so the selection flag is produced alongside
each option, update buildCleanOptions (and any callers) to populate that
Selected value based on worktreeCleanStatus.isSafe, and then add assertions in
TestBuildCleanOptions to compare tt.expectedSelected and tt.expectedUnselected
against the returned Selected flags while still verifying len(opts.options) and
option keys; alternatively, if you prefer minimal change, introduce a thin
wrapper type or factory used by buildCleanOptions that records selection state
for tests and assert against that wrapper in the test.
There was a problem hiding this comment.
Acknowledged. The pre-selection test assertions cannot be added as suggested because huh.Option v0.8.0 has no getter for the selected state—Selected(bool) is only a setter. This requires a design decision on how to make the selection state testable. Potential approaches:
- Refactor
buildCleanOptionsto return selection state alongside options (e.g., a struct with both option and boolean flag) - Use reflection to inspect internal state (not recommended)
- Mock or wrap
huh.Optionwith a custom type
A follow-up issue can be opened to address this pending resolution.
There was a problem hiding this comment.
@fdarian, your analysis is correct—this is indeed a structural limitation that requires a design decision rather than a quick fix in this PR.
Opening a follow-up issue is a good approach to track this technical debt. Would you like me to create a GitHub issue with the context (the missing assertions, the huh.Option limitation, and the three potential approaches you outlined)? I can include a reference back to this PR and comment for continuity.
🧠 Learnings used
Learnt from: CR
Repo: satococoa/wtp PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-11T14:56:49.178Z
Learning: Run `go tool task test` for running tests
Prevent nil pointer dereferences when executor.Execute returns nil result or empty Results slice in getWorktreesForClean, checkMergeStatus, checkCleanStatus, and checkPushStatus.
The previous logic using rev-parse --abbrev-ref would skip "main" when it existed, incorrectly falling through to check for "master". Simplify to just check branch existence with --verify --quiet.
The mock now checks for "origin/" prefix in args to differentiate between push-check and ahead-count rev-list commands.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/wtp/clean.go`:
- Around line 305-322: When executor.Execute in
worktreeCleanStatus.checkCleanStatus fails or returns nil/empty results, don't
return silently; instead mark the worktree as dirty and unsafe by setting
s.isClean = false, s.isSafe = false and append an explanatory reason (e.g., "git
status failed" plus any error text if available) so failures are fail-safe.
Apply the same principle in worktreeCleanStatus.checkPushStatus: when
executor.Execute (the push-check command) errors or yields no usable result, set
s.isPushed = false, s.isSafe = false and append a reason (e.g., "push check
failed" with error details if present). Ensure you use the existing
executor.Execute call sites and result handling in checkCleanStatus and
checkPushStatus to capture and log the error or nil-result case before
returning.
- Around line 228-236: The code calls
executor.Execute([]command.Command{removeCmd}) and immediately dereferences
result.Results, which can panic if result is nil; add a nil-guard like the other
helpers (checkMergeStatus/checkCleanStatus/checkPushStatus): after execErr
check, verify result != nil before accessing result.Results, and if result is
nil write a failure message to w (including name and that result was nil) and
continue; reference the executor.Execute call, the result variable, and
result.Results to locate the change.
🧹 Nitpick comments (3)
cmd/wtp/clean_test.go (3)
735-760: Mock executor only processescommands[0], silently ignoring batched commands.If
cleanCommandWithExecutoror any caller passes multiple commands in a singleExecutecall, only the first command's result is returned. This is fine if the production code always passes single-command slices (which appears to be the case here), but it's a subtle assumption worth documenting in the mock.
810-829:TestCleanCommand_EmptyWorktreeListduplicatesTestCleanCommand_NoManagedWorktrees.Both tests set up a mock with only the main worktree and assert "No managed worktrees found". They test the same code path. Consider removing one or differentiating them (e.g., test with a truly empty worktree list output).
669-687:TestCleanCommand_NotInGitRepochanges the process working directory, which is shared state.
os.Chdiraffects all goroutines. Ifgo testruns tests in parallel (viat.Parallel()), this could cause flaky failures in other tests. Currently not.Parallel()calls exist, but this is fragile. Consider using thet.Chdirhelper available in Go 1.24 which automatically restores the directory and is test-safe.♻️ Proposed fix using Go 1.24's `t.Chdir`
func TestCleanCommand_NotInGitRepo(t *testing.T) { - // Create a temporary directory that is not a git repo tempDir := t.TempDir() - oldDir, _ := os.Getwd() - defer func() { _ = os.Chdir(oldDir) }() - err := os.Chdir(tempDir) - assert.NoError(t, err) + t.Chdir(tempDir) app := &cli.Command{
| result, execErr := executor.Execute([]command.Command{removeCmd}) | ||
| if execErr != nil { | ||
| _, _ = fmt.Fprintf(w, "Failed to remove '%s': %v\n", name, execErr) | ||
| continue | ||
| } | ||
| if len(result.Results) > 0 && result.Results[0].Error != nil { | ||
| _, _ = fmt.Fprintf(w, "Failed to remove '%s': %v\n", name, result.Results[0].Error) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Missing nil check on result before accessing result.Results.
If executor.Execute returns (nil, nil), line 233 will panic with a nil pointer dereference. The same nil-guard pattern used in checkMergeStatus, checkCleanStatus, and checkPushStatus should be applied here.
🐛 Proposed fix
result, execErr := executor.Execute([]command.Command{removeCmd})
if execErr != nil {
_, _ = fmt.Fprintf(w, "Failed to remove '%s': %v\n", name, execErr)
continue
}
- if len(result.Results) > 0 && result.Results[0].Error != nil {
+ if result == nil || (len(result.Results) > 0 && result.Results[0].Error != nil) {
+ errMsg := "unknown error"
+ if result != nil && len(result.Results) > 0 && result.Results[0].Error != nil {
+ errMsg = result.Results[0].Error.Error()
+ }
- _, _ = fmt.Fprintf(w, "Failed to remove '%s': %v\n", name, result.Results[0].Error)
+ _, _ = fmt.Fprintf(w, "Failed to remove '%s': %s\n", name, errMsg)
continue
}🤖 Prompt for AI Agents
In `@cmd/wtp/clean.go` around lines 228 - 236, The code calls
executor.Execute([]command.Command{removeCmd}) and immediately dereferences
result.Results, which can panic if result is nil; add a nil-guard like the other
helpers (checkMergeStatus/checkCleanStatus/checkPushStatus): after execErr
check, verify result != nil before accessing result.Results, and if result is
nil write a failure message to w (including name and that result was nil) and
continue; reference the executor.Execute call, the result variable, and
result.Results to locate the change.
| func (s *worktreeCleanStatus) checkCleanStatus(wt git.Worktree, executor command.Executor) { | ||
| statusCmd := command.Command{ | ||
| Name: "git", | ||
| Args: []string{"status", "--porcelain"}, | ||
| WorkDir: wt.Path, | ||
| } | ||
| result, err := executor.Execute([]command.Command{statusCmd}) | ||
| if err != nil || result == nil || len(result.Results) == 0 { | ||
| return | ||
| } | ||
|
|
||
| output := strings.TrimSpace(result.Results[0].Output) | ||
| if output != "" { | ||
| s.isClean = false | ||
| s.isSafe = false | ||
| s.reasons = append(s.reasons, "uncommitted changes") | ||
| } | ||
| } |
There was a problem hiding this comment.
Silent failure in checkCleanStatus defaults to "clean", risking data loss.
If executor.Execute returns an error or nil result (line 312-313), the method returns early without marking the worktree as dirty. Since isClean defaults to true, a failed git status check will leave the worktree marked as safe for removal — potentially deleting a worktree with uncommitted changes.
The same concern applies to checkPushStatus (lines 337-340), where a failure defaults to "pushed".
🛡️ Proposed fix — fail-safe: treat errors as unsafe
func (s *worktreeCleanStatus) checkCleanStatus(wt git.Worktree, executor command.Executor) {
statusCmd := command.Command{
Name: "git",
Args: []string{"status", "--porcelain"},
WorkDir: wt.Path,
}
result, err := executor.Execute([]command.Command{statusCmd})
if err != nil || result == nil || len(result.Results) == 0 {
+ s.isClean = false
+ s.isSafe = false
+ s.reasons = append(s.reasons, "uncommitted changes (check failed)")
return
}🤖 Prompt for AI Agents
In `@cmd/wtp/clean.go` around lines 305 - 322, When executor.Execute in
worktreeCleanStatus.checkCleanStatus fails or returns nil/empty results, don't
return silently; instead mark the worktree as dirty and unsafe by setting
s.isClean = false, s.isSafe = false and append an explanatory reason (e.g., "git
status failed" plus any error text if available) so failures are fail-safe.
Apply the same principle in worktreeCleanStatus.checkPushStatus: when
executor.Execute (the push-check command) errors or yields no usable result, set
s.isPushed = false, s.isSafe = false and append a reason (e.g., "push check
failed" with error details if present). Ensure you use the existing
executor.Execute call sites and result handling in checkCleanStatus and
checkPushStatus to capture and log the error or nil-result case before
returning.
1 participant
Closes #88
Summary
wtp cleancommand for interactively reviewing and removing stale worktreesFlow
Validation Logic
A worktree is safe when all three checks pass:
unmergedordetached HEADgit status --porcelainis emptyuncommitted changesunpushed commits (N ahead)Safe worktrees are pre-selected for removal. Unsafe worktrees are shown but not pre-selected — the user can still choose to remove them.
The
--forceflag is passed through togit worktree removefor worktrees with uncommitted changes.Changes
cmd/wtp/clean.gocmd/wtp/clean_test.gocmd/wtp/app.gointernal/command/builders.gogo.mod/go.sumcharmbracelet/huhdependency for interactive formsTest Plan
wtp cleanin repo with mixed safe/unsafe worktrees--forceremoves worktrees with uncommitted changesSummary by CodeRabbit
New Features
Tests