[Feat] Unified worktree protection policy for destructive operations#64
[Feat] Unified worktree protection policy for destructive operations#64
Conversation
Add ProtectionPolicy struct with IsProtected/Reason methods as a single predicate for bare, main-branch, and root-worktree protection. Wire into Remove, Promote, and Prune to replace scattered inline checks. - Unify main branch resolution via resolveBaseBranchWithPolicy - Add --all/-a flag to wt rm (skips protected, removes the rest) - Contextual error messages: bare repository / main worktree / main branch - Bare-layout HEAD fallback for non-main/master default branches Signed-off-by: samzong <[email protected]>
There was a problem hiding this comment.
Code Review
This pull request introduces a --all flag for the worktree remove command and implements a centralized ProtectionPolicy to prevent the accidental removal or promotion of critical worktrees, such as the main branch or bare repository. The feedback focuses on the safety of the ProtectionPolicy implementation; specifically, the NewProtectionPolicy function and its helpers currently swallow errors when resolving the repository root or main branch. It is recommended to update these methods to return and handle errors to ensure that bulk operations do not proceed with an incomplete or incorrect protection state.
| return nil, err | ||
| } | ||
|
|
||
| pp := wtClient.NewProtectionPolicy() |
There was a problem hiding this comment.
The NewProtectionPolicy function can fail if it cannot resolve the repository root or the main branch. This error should be handled to prevent proceeding with an incomplete protection policy, which could lead to accidental removal of protected worktrees.
pp, err := wtClient.NewProtectionPolicy()
if err != nil {
return nil, err
}| repoDir := repoDirForGit(root) | ||
| isBare := repoDir != root | ||
|
|
||
| pp := c.NewProtectionPolicy() |
| for _, wt := range worktrees { | ||
| if wt.Branch == "main" || wt.Branch == "master" { | ||
| return wt.Path, nil | ||
| mainBranch := c.resolvedMainBranch() |
| pp := c.NewProtectionPolicy() | ||
| if pp.IsProtected(wtInfo) { |
There was a problem hiding this comment.
| return report, errors.New("worktree is in detached HEAD state, cannot promote") | ||
| } | ||
|
|
||
| pp := c.NewProtectionPolicy() |
| func (c *Client) NewProtectionPolicy() ProtectionPolicy { | ||
| var p ProtectionPolicy | ||
| root, err := c.GetWorktreeRoot() | ||
| if err != nil { | ||
| return p | ||
| } | ||
| p.RootPath = root | ||
| repoDir := repoDirForGit(root) | ||
| isBareLayout := repoDir != root | ||
| branch, err := c.resolveBaseBranchWithPolicy(repoDir, "", isBareLayout) | ||
| if err != nil { | ||
| return p | ||
| } | ||
| p.MainBranch = localBranchName(branch) | ||
| return p | ||
| } |
There was a problem hiding this comment.
The NewProtectionPolicy function currently swallows errors from GetWorktreeRoot and resolveBaseBranchWithPolicy. This is a significant safety risk: if the main branch cannot be resolved (e.g., due to a git error), the policy will have an empty MainBranch, and IsProtected will return false for the main branch worktree. This could lead to accidental deletion of the main branch during bulk operations like rm --all. The function should return an error so callers can abort safely.
| func (c *Client) NewProtectionPolicy() ProtectionPolicy { | |
| var p ProtectionPolicy | |
| root, err := c.GetWorktreeRoot() | |
| if err != nil { | |
| return p | |
| } | |
| p.RootPath = root | |
| repoDir := repoDirForGit(root) | |
| isBareLayout := repoDir != root | |
| branch, err := c.resolveBaseBranchWithPolicy(repoDir, "", isBareLayout) | |
| if err != nil { | |
| return p | |
| } | |
| p.MainBranch = localBranchName(branch) | |
| return p | |
| } | |
| func (c *Client) NewProtectionPolicy() (ProtectionPolicy, error) { | |
| var p ProtectionPolicy | |
| root, err := c.GetWorktreeRoot() | |
| if err != nil { | |
| return p, fmt.Errorf("failed to get worktree root: %w", err) | |
| } | |
| p.RootPath = root | |
| repoDir := repoDirForGit(root) | |
| isBareLayout := repoDir != root | |
| branch, err := c.resolveBaseBranchWithPolicy(repoDir, "", isBareLayout) | |
| if err != nil { | |
| return p, fmt.Errorf("failed to resolve main branch: %w", err) | |
| } | |
| p.MainBranch = localBranchName(branch) | |
| return p, nil | |
| } |
| func (c *Client) IsProtectedWorktree(wt Info) bool { | ||
| return c.NewProtectionPolicy().IsProtected(wt) | ||
| } | ||
|
|
||
| func (c *Client) resolvedMainBranch() string { | ||
| return c.NewProtectionPolicy().MainBranch | ||
| } |
There was a problem hiding this comment.
Update these helper methods to propagate errors from NewProtectionPolicy.
func (c *Client) IsProtectedWorktree(wt Info) (bool, error) {
pp, err := c.NewProtectionPolicy()
if err != nil {
return false, err
}
return pp.IsProtected(wt), nil
}
func (c *Client) resolvedMainBranch() (string, error) {
pp, err := c.NewProtectionPolicy()
if err != nil {
return "", err
}
return pp.MainBranch, nil
}| } | ||
|
|
||
| for _, wt := range worktrees { | ||
| protected := client.IsProtectedWorktree(wt) |
Return error from NewProtectionPolicy, IsProtectedWorktree, and resolvedMainBranch instead of silently returning empty policy. Propagate errors in all callers (prepareRemove, Promote, Prune, resolveAllRemovableWorktrees) so destructive operations abort when protection state cannot be determined. Signed-off-by: samzong <[email protected]>
Fix 7 new lint violations from #63 and #64: - lll: break long lines in worktree.go, worktree_test.go, prune.go, resource.go - perfsprint: use errors.New instead of fmt.Errorf for static strings in prune.go - gocritic/unlambda: simplify exec.Command wrapper in worktree_test.go Signed-off-by: samzong <[email protected]>
* fix: resolve lint issues introduced by protection and cache PRs Fix 7 new lint violations from #63 and #64: - lll: break long lines in worktree.go, worktree_test.go, prune.go, resource.go - perfsprint: use errors.New instead of fmt.Errorf for static strings in prune.go - gocritic/unlambda: simplify exec.Command wrapper in worktree_test.go Signed-off-by: samzong <[email protected]> * fix(wt): derive worktree name from -b when no name given - Allow `gmc wt add -b <branch>` without positional args; worktree name defaults to the base branch value. - Split the `-b/--base` flag variable between `wt add` and `wt dup` so dup's "main" default no longer pollutes add at init time. ## Considered and deferred - cmd/worktree.go [BOT-NIT]: wtBaseBranch could be renamed to wtAddBase for symmetry with wtDupBase / wtPruneBase. Deferred — not a behavior concern. - cmd/worktree.go [BOT-SCOPE]: No cmd-level test for the new zero-arg + -b path. Deferred for a follow-up test-only PR. Signed-off-by: samzong <[email protected]> --------- Signed-off-by: samzong <[email protected]>
What's changed?
ProtectionPolicystruct (IsProtected/Reason) as a single predicate for worktree protectionRemove,Promote, andPrune— replaces scattered inline bare/main checksgetDefaultBranch,findMainWorktreePath, and prune all go throughresolveBaseBranchWithPolicy--all/-aflag togmc wt rmthat removes all non-protected worktreesWhy
--allis a common workflow need when cleaning up after parallel development sessions