From f208a1bc10c7b1c69c0157fce5be574912d550e4 Mon Sep 17 00:00:00 2001 From: Saurav Panda Date: Tue, 18 Aug 2026 16:07:28 -0700 Subject: [PATCH] Harden destructive worktree workflows --- README.md | 4 ++ cmd/list.go | 18 ++++++- cmd/push.go | 26 +++++++++- cmd/rm.go | 14 ++++-- cmd/safety_hardening_test.go | 94 ++++++++++++++++++++++++++++++++++++ cmd/sync.go | 25 ++++++++-- 6 files changed, 171 insertions(+), 10 deletions(-) create mode 100644 cmd/safety_hardening_test.go diff --git a/README.md b/README.md index 3409691..0ba3636 100644 --- a/README.md +++ b/README.md @@ -112,8 +112,12 @@ bonsai push feat/search bonsai push --pr bonsai push --web bonsai push --pr --remove +bonsai push --pr --remove --yes ``` +`--remove` asks before deleting the worktree. Add `--yes` only when that +removal has already been approved in an automated workflow. + ### `bonsai clean` Open an interactive picker for merged, stale, or otherwise removable worktrees. diff --git a/cmd/list.go b/cmd/list.go index 35b3192..e8e7639 100644 --- a/cmd/list.go +++ b/cmd/list.go @@ -105,6 +105,7 @@ func runList(cmd *cobra.Command, args []string) error { wt.PRURL = pr.URL wt.PRNumber = pr.Number wt.PRHeadOID = pr.HeadRefOID + reconcileMergedPRStatus(wt) } else if errors.Is(err, github.ErrNoPR) { wt.PRStatus = "none" } else { @@ -134,7 +135,7 @@ func runList(cmd *cobra.Command, args []string) error { if filterNoPR { var filtered []*git.Worktree for _, wt := range worktrees { - if wt.IsMain || wt.PRStatus == "none" || wt.PRStatus == "unknown" { + if matchesNoPRFilter(wt) { filtered = append(filtered, wt) } } @@ -155,6 +156,21 @@ func runList(cmd *cobra.Command, args []string) error { return nil } +func matchesNoPRFilter(wt *git.Worktree) bool { + return wt.IsMain || wt.PRStatus == "none" +} + +func reconcileMergedPRStatus(wt *git.Worktree) { + // Squash and rebase merges do not make the original branch commits + // ancestors of the base branch. If GitHub confirms that this exact HEAD was + // the merged PR head, it is not unpushed work and should not get a warning. + if wt.PRStatus == "merged" && wt.PRHeadOID != "" && wt.HEAD == wt.PRHeadOID { + wt.HasUnpushed = false + wt.UnpushedCommits = 0 + wt.UnpushedKnown = true + } +} + // jsonWorktree is the JSON shape emitted by `bonsai list --json`. type jsonWorktree struct { Number int `json:"number,omitempty"` diff --git a/cmd/push.go b/cmd/push.go index e1bba71..a3d6e90 100644 --- a/cmd/push.go +++ b/cmd/push.go @@ -23,7 +23,8 @@ If a path or branch name is provided, bonsai will find the matching worktree. If omitted, the current working directory is used. After pushing, you can optionally open a GitHub PR via gh CLI, -and then remove the worktree.`, +and then remove the worktree. Removal requires confirmation unless --yes is +provided together with --remove.`, Args: cobra.MaximumNArgs(1), RunE: runPush, } @@ -33,6 +34,7 @@ func init() { pushCmd.Flags().Bool("pr", false, "open a PR after pushing") pushCmd.Flags().Bool("web", false, "open PR creation in browser (implies --pr)") pushCmd.Flags().BoolP("remove", "r", false, "remove worktree after push/PR") + pushCmd.Flags().BoolP("yes", "y", false, "confirm worktree removal without prompting (requires --remove)") pushCmd.Flags().Bool("dry-run", false, "show what would happen without doing it") } @@ -45,7 +47,11 @@ func runPush(cmd *cobra.Command, args []string) error { openPR, _ := cmd.Flags().GetBool("pr") web, _ := cmd.Flags().GetBool("web") remove, _ := cmd.Flags().GetBool("remove") + autoYes, _ := cmd.Flags().GetBool("yes") dryRun, _ := cmd.Flags().GetBool("dry-run") + if autoYes && !remove { + return fmt.Errorf("--yes requires --remove") + } if web { openPR = true } @@ -126,6 +132,17 @@ func runPush(cmd *cobra.Command, args []string) error { // Remove if remove { + approved, quit := approvePushRemoval(autoYes, func() (bool, bool) { + return confirmOrQuit(fmt.Sprintf(" Remove worktree %s? [y/N/q] ", wt.Path)) + }) + if !approved { + if quit { + fmt.Println(" removal cancelled; worktree kept") + } else { + fmt.Println(" worktree kept") + } + return nil + } if wt.HasUnpushed { // After push, re-check — but we just pushed, so should be clear. // Re-enrich to be safe. @@ -141,6 +158,13 @@ func runPush(cmd *cobra.Command, args []string) error { return nil } +func approvePushRemoval(autoYes bool, prompt func() (yes bool, quit bool)) (yes bool, quit bool) { + if autoYes { + return true, false + } + return prompt() +} + // extractTicket returns the first ticket ID found in branch using the configured // regexp pattern. Returns "" if pattern is empty or no match. func extractTicket(branch, pattern string) string { diff --git a/cmd/rm.go b/cmd/rm.go index 5513b81..f483bc4 100644 --- a/cmd/rm.go +++ b/cmd/rm.go @@ -54,9 +54,9 @@ func runRm(cmd *cobra.Command, args []string) error { var targets []*git.Worktree seen := map[int]bool{} for _, a := range args { - n, err := strconv.Atoi(strings.TrimSpace(a)) - if err != nil || n < 1 || n > len(added) { - return fmt.Errorf("invalid worktree number %q (valid range: 1–%d)", a, len(added)) + n, err := parseWorktreeNumber(a, len(added)) + if err != nil { + return err } if seen[n] { continue @@ -114,3 +114,11 @@ func runRm(cmd *cobra.Command, args []string) error { } return nil } + +func parseWorktreeNumber(value string, total int) (int, error) { + n, err := strconv.Atoi(strings.TrimSpace(value)) + if err != nil || n < 1 || n > total { + return 0, fmt.Errorf("invalid worktree number %q (valid range: 1-%d)", value, total) + } + return n, nil +} diff --git a/cmd/safety_hardening_test.go b/cmd/safety_hardening_test.go new file mode 100644 index 0000000..7cf3853 --- /dev/null +++ b/cmd/safety_hardening_test.go @@ -0,0 +1,94 @@ +package cmd + +import ( + "strings" + "testing" + + "github.com/sauravpanda/bonsai/internal/git" +) + +func TestMatchesNoPRFilterExcludesUnknownStatus(t *testing.T) { + tests := []struct { + name string + wt *git.Worktree + want bool + }{ + {name: "main", wt: &git.Worktree{IsMain: true, PRStatus: "unknown"}, want: true}, + {name: "no PR", wt: &git.Worktree{PRStatus: "none"}, want: true}, + {name: "unknown", wt: &git.Worktree{PRStatus: "unknown"}, want: false}, + {name: "open", wt: &git.Worktree{PRStatus: "open"}, want: false}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if got := matchesNoPRFilter(test.wt); got != test.want { + t.Fatalf("matchesNoPRFilter() = %t, want %t", got, test.want) + } + }) + } +} + +func TestReconcileMergedPRStatusClearsFalseUnpushedWarning(t *testing.T) { + wt := &git.Worktree{ + HEAD: "abc123", + PRStatus: "merged", + PRHeadOID: "abc123", + HasUnpushed: true, + UnpushedCommits: 3, + } + reconcileMergedPRStatus(wt) + if wt.HasUnpushed || wt.UnpushedCommits != 0 || !wt.UnpushedKnown { + t.Fatalf("matching merged PR head was not reconciled: %+v", wt) + } +} + +func TestReconcileMergedPRStatusKeepsCommitsAfterPRHead(t *testing.T) { + wt := &git.Worktree{ + HEAD: "new-work", + PRStatus: "merged", + PRHeadOID: "merged-head", + HasUnpushed: true, + UnpushedCommits: 1, + } + reconcileMergedPRStatus(wt) + if !wt.HasUnpushed || wt.UnpushedCommits != 1 { + t.Fatalf("post-merge work was incorrectly cleared: %+v", wt) + } +} + +func TestParseWorktreeNumberUsesPortableRange(t *testing.T) { + if got, err := parseWorktreeNumber("2", 3); err != nil || got != 2 { + t.Fatalf("parseWorktreeNumber() = %d, %v", got, err) + } + _, err := parseWorktreeNumber("4", 3) + if err == nil || !strings.Contains(err.Error(), "1-3") { + t.Fatalf("expected ASCII range in error, got %v", err) + } + if strings.Contains(err.Error(), "–") { + t.Fatalf("error contains a non-ASCII en dash: %v", err) + } +} + +func TestApprovePushRemoval(t *testing.T) { + prompted := false + yes, quit := approvePushRemoval(true, func() (bool, bool) { + prompted = true + return false, false + }) + if !yes || quit || prompted { + t.Fatalf("--yes should approve without prompting: yes=%t quit=%t prompted=%t", yes, quit, prompted) + } + + yes, quit = approvePushRemoval(false, func() (bool, bool) { + return false, true + }) + if yes || !quit { + t.Fatalf("manual cancellation was not preserved: yes=%t quit=%t", yes, quit) + } +} + +func TestAbortFailedSyncReportsAbortFailure(t *testing.T) { + err := abortFailedSync(t.TempDir(), false) + if err == nil || !strings.Contains(err.Error(), "git rebase --abort") { + t.Fatalf("expected actionable abort failure, got %v", err) + } +} diff --git a/cmd/sync.go b/cmd/sync.go index 499bc0e..6442894 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -122,11 +122,10 @@ func runSync(cmd *cobra.Command, args []string) error { fmt.Printf(" %s\n", line) } } - // Abort rebase on conflict so the worktree is not left in a broken state. - if !useMerge { - exec.Command("git", "-C", wt.Path, "rebase", "--abort").Run() //nolint:errcheck - } else { - exec.Command("git", "-C", wt.Path, "merge", "--abort").Run() //nolint:errcheck + // Abort on conflict so the worktree is not left in a broken state. + if abortErr := abortFailedSync(wt.Path, useMerge); abortErr != nil { + fmt.Fprintf(os.Stderr, " %s\n", + warnStyle.Render("warning: could not abort cleanly: "+abortErr.Error())) } failed++ } else { @@ -148,3 +147,19 @@ func runSync(cmd *cobra.Command, args []string) error { return nil } + +func abortFailedSync(path string, useMerge bool) error { + operation := "rebase" + if useMerge { + operation = "merge" + } + out, err := exec.Command("git", "-C", path, operation, "--abort").CombinedOutput() + if err == nil { + return nil + } + detail := strings.TrimSpace(string(out)) + if detail == "" { + return fmt.Errorf("git %s --abort: %w", operation, err) + } + return fmt.Errorf("git %s --abort: %w: %s", operation, err, detail) +}