From 7b79f4b83cb490dadea46fd50b971eb1066b1ee6 Mon Sep 17 00:00:00 2001 From: Jason Allum <21417+jallum@users.noreply.github.com> Date: Sat, 30 May 2026 08:31:50 -0400 Subject: [PATCH 1/2] Add recursive close (--recursive/-r) to cascade-close a subtree (beadwork-ep5) bw close --recursive (-r) closes the issue and every descendant, leaf-up, in a single commit. Already-closed members are skipped (an already-closed root is tolerated so a re-run mops up stragglers); the root keeps its --reason while descendants record 'closed with parent '. External issues that become unblocked are surfaced and deduped; internal unblocks are suppressed. New Store.CloseSubtree returns SubtreeCloseResult{Closed, Skipped, Unblocked}. The non-recursive path and its CloseResult JSON shape are unchanged. --- cmd/bw/close.go | 100 +++++++++++++-- cmd/bw/close_test.go | 92 ++++++++++++++ cmd/bw/command.go | 4 +- cmd/bw/helpers.go | 1 + internal/issue/subtree_close.go | 107 ++++++++++++++++ internal/issue/subtree_close_test.go | 177 +++++++++++++++++++++++++++ 6 files changed, 470 insertions(+), 11 deletions(-) create mode 100644 internal/issue/subtree_close.go create mode 100644 internal/issue/subtree_close_test.go diff --git a/cmd/bw/close.go b/cmd/bw/close.go index 4c184541..f8809466 100644 --- a/cmd/bw/close.go +++ b/cmd/bw/close.go @@ -10,23 +10,26 @@ import ( ) type CloseArgs struct { - ID string - Reason string - JSON bool + ID string + Reason string + Recursive bool + JSON bool } func parseCloseArgs(raw []string) (CloseArgs, error) { - if len(raw) == 0 { - return CloseArgs{}, fmt.Errorf("usage: bw close [--reason ]") - } - a, err := ParseArgs(raw[1:], []string{"--reason"}, []string{"--json"}) + a, err := ParseArgs(raw, []string{"--reason"}, []string{"--recursive", "--json"}) if err != nil { return CloseArgs{}, err } + id := a.PosFirst() + if id == "" { + return CloseArgs{}, fmt.Errorf("usage: bw close [--reason ] [--recursive]") + } return CloseArgs{ - ID: raw[0], - Reason: a.String("--reason"), - JSON: a.JSON(), + ID: id, + Reason: a.String("--reason"), + Recursive: a.Bool("--recursive"), + JSON: a.JSON(), }, nil } @@ -36,6 +39,10 @@ func cmdClose(store *issue.Store, args []string, w Writer, _ *config.Config) (*c return nil, err } + if ca.Recursive { + return cmdCloseRecursive(store, ca, w) + } + var iss *issue.Issue var unblocked []*issue.Issue @@ -90,6 +97,79 @@ func cmdClose(store *issue.Store, args []string, w Writer, _ *config.Config) (*c return nil, nil } +// cmdCloseRecursive closes an issue and its entire subtree in a single commit. +func cmdCloseRecursive(store *issue.Store, ca CloseArgs, w Writer) (*config.Config, error) { + var result *issue.SubtreeCloseResult + + err := commitWithRetry(store, commitMaxRetries, func() (string, error) { + var cerr error + result, cerr = store.CloseSubtree(ca.ID, ca.Reason) + if cerr != nil { + return "", cerr + } + if len(result.Closed) == 0 { + return "", fmt.Errorf("nothing to close: %s and its subtree are already closed", ca.ID) + } + + intent := "" + for i, iss := range result.Closed { + if i > 0 { + intent += "\n" + } + intent += fmt.Sprintf("close %s", iss.ID) + if iss.CloseReason != "" { + intent += fmt.Sprintf(" reason=%q", iss.CloseReason) + } + } + for _, u := range result.Unblocked { + intent += fmt.Sprintf("\nunblocked %s", u.ID) + } + return intent, nil + }) + if err != nil { + return nil, err + } + + if ca.JSON { + if result.Closed == nil { + result.Closed = []*issue.Issue{} + } + if result.Skipped == nil { + result.Skipped = []*issue.Issue{} + } + if result.Unblocked == nil { + result.Unblocked = []*issue.Issue{} + } + fprintJSON(w, result) + return nil, nil + } + + fmt.Fprintf(w, "closed %d issue(s) under {id:%s}:\n", len(result.Closed), ca.ID) + w.Push(2) + for _, iss := range result.Closed { + fmt.Fprintf(w, "{id:%s}: ~~%s~~\n", iss.ID, md.Escape(iss.Title)) + } + w.Pop() + if len(result.Skipped) > 0 { + fmt.Fprintf(w, "\n%d already closed, skipped.\n", len(result.Skipped)) + } + if len(result.Unblocked) > 0 { + fmt.Fprintln(w) + w.Push(2) + for _, u := range result.Unblocked { + fmt.Fprintf(w, "unblocked {id:%s}: %s\n", u.ID, md.Escape(u.Title)) + } + w.Pop() + fmt.Fprintln(w) + if len(result.Unblocked) == 1 { + fmt.Fprintf(w, "Next: `bw start %s` to begin, or `bw ready` for all options.\n", result.Unblocked[0].ID) + } else { + fmt.Fprintln(w, "Next: `bw ready` to see available work.") + } + } + return nil, nil +} + type ReopenArgs struct { ID string JSON bool diff --git a/cmd/bw/close_test.go b/cmd/bw/close_test.go index 7d3a6864..657bdd4c 100644 --- a/cmd/bw/close_test.go +++ b/cmd/bw/close_test.go @@ -82,6 +82,98 @@ func TestCmdCloseNotFound(t *testing.T) { } } +func TestCmdCloseRecursive(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + root, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"}) + c, _ := env.Store.Create("Child", issue.CreateOpts{Parent: root.ID}) + gc, _ := env.Store.Create("Grandchild", issue.CreateOpts{Parent: c.ID}) + env.Repo.Commit("setup") + + var buf bytes.Buffer + _, err := cmdClose(env.Store, []string{root.ID, "--recursive"}, PlainWriter(&buf), nil) + if err != nil { + t.Fatalf("cmdClose --recursive: %v", err) + } + if !strings.Contains(buf.String(), "closed 3 issue") { + t.Errorf("output = %q", buf.String()) + } + + for _, id := range []string{root.ID, c.ID, gc.ID} { + got, _ := env.Store.Get(id) + if got.Status != "closed" { + t.Errorf("%s status = %q, want closed", id, got.Status) + } + } +} + +func TestCmdCloseRecursiveAlias(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + root, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"}) + c, _ := env.Store.Create("Child", issue.CreateOpts{Parent: root.ID}) + env.Repo.Commit("setup") + + var buf bytes.Buffer + if _, err := cmdClose(env.Store, []string{root.ID, "-r"}, PlainWriter(&buf), nil); err != nil { + t.Fatalf("cmdClose -r: %v", err) + } + got, _ := env.Store.Get(c.ID) + if got.Status != "closed" { + t.Errorf("child status = %q, want closed", got.Status) + } +} + +func TestCmdCloseRecursiveSkipsAlreadyClosed(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + root, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"}) + c, _ := env.Store.Create("Child", issue.CreateOpts{Parent: root.ID}) + env.Store.Close(c.ID, "") + env.Repo.Commit("setup") + + var buf bytes.Buffer + if _, err := cmdClose(env.Store, []string{root.ID, "-r"}, PlainWriter(&buf), nil); err != nil { + t.Fatalf("cmdClose -r: %v", err) + } + if !strings.Contains(buf.String(), "already closed, skipped") { + t.Errorf("output = %q", buf.String()) + } + rootGot, _ := env.Store.Get(root.ID) + if rootGot.Status != "closed" { + t.Errorf("root status = %q, want closed", rootGot.Status) + } +} + +func TestCmdCloseRecursiveJSON(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + root, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"}) + env.Store.Create("Child", issue.CreateOpts{Parent: root.ID}) + env.Repo.Commit("setup") + + var buf bytes.Buffer + _, err := cmdClose(env.Store, []string{root.ID, "-r", "--json"}, PlainWriter(&buf), nil) + if err != nil { + t.Fatalf("cmdClose -r --json: %v", err) + } + + var got issue.SubtreeCloseResult + if err := json.Unmarshal(buf.Bytes(), &got); err != nil { + t.Fatalf("JSON parse: %v", err) + } + if len(got.Closed) != 2 { + t.Errorf("closed = %d, want 2", len(got.Closed)) + } + if got.Skipped == nil || got.Unblocked == nil { + t.Error("skipped/unblocked should be [] not null") + } +} + func TestCmdReopenBasic(t *testing.T) { env := testutil.NewEnv(t) defer env.Cleanup() diff --git a/cmd/bw/command.go b/cmd/bw/command.go index 23dddc0d..8547b157 100644 --- a/cmd/bw/command.go +++ b/cmd/bw/command.go @@ -173,17 +173,19 @@ var commands = []Command{ { Name: "close", Summary: "Close an issue", - Description: "Close an issue. Optionally provide a reason.", + Description: "Close an issue. Optionally provide a reason.\nWith --recursive, also close the issue's entire subtree (all descendants).", Positionals: []Positional{ {Name: "", Required: true, Help: "Issue ID"}, }, Flags: []Flag{ {Long: "--reason", Value: "REASON", Help: "Closing reason"}, + {Long: "--recursive", Short: "-r", Help: "Also close all descendants (the whole subtree)"}, {Long: "--json", Help: "Output as JSON"}, }, Examples: []Example{ {Cmd: "bw close bw-a3f8"}, {Cmd: "bw close bw-a3f8 --reason duplicate"}, + {Cmd: "bw close bw-a3f8 -r"}, }, NeedsStore: true, Run: cmdClose, diff --git a/cmd/bw/helpers.go b/cmd/bw/helpers.go index 70b22d45..6e0e6b4d 100644 --- a/cmd/bw/helpers.go +++ b/cmd/bw/helpers.go @@ -103,6 +103,7 @@ var aliases = map[string]string{ "-l": "--labels", "-g": "--grep", "-y": "--yes", + "-r": "--recursive", } // Args holds parsed command-line arguments separated into boolean flags, diff --git a/internal/issue/subtree_close.go b/internal/issue/subtree_close.go new file mode 100644 index 00000000..31243cae --- /dev/null +++ b/internal/issue/subtree_close.go @@ -0,0 +1,107 @@ +package issue + +import "fmt" + +// SubtreeCloseResult reports the outcome of a recursive close: the issues +// newly closed (leaf-up order), those skipped because they were already +// closed, and the external issues that became unblocked as a result. +type SubtreeCloseResult struct { + Closed []*Issue `json:"closed"` + Skipped []*Issue `json:"skipped"` + Unblocked []*Issue `json:"unblocked"` +} + +// CloseSubtree closes id and every descendant under it. Members that are +// already closed are skipped rather than erroring, and an already-closed +// root is tolerated so a re-run mops up any stragglers. The root records the +// given reason verbatim; each descendant records an auto-generated reason +// noting the cascade. Issues outside the subtree that become fully unblocked +// are returned, deduplicated, in close order. +// +// Traversal is built from all issues (including closed ones) so the walk can +// pass through an already-closed node to reach still-open descendants beneath +// it. Closing proceeds leaf-up: a parent is closed only after its children. +func (s *Store) CloseSubtree(id, reason string) (*SubtreeCloseResult, error) { + id, err := s.resolveID(id) + if err != nil { + return nil, err + } + + all, err := s.List(Filter{}) + if err != nil { + return nil, err + } + childrenOf := make(map[string][]string) + for _, iss := range all { + if iss.Parent != "" { + childrenOf[iss.Parent] = append(childrenOf[iss.Parent], iss.ID) + } + } + + subtree := buildSubtreeSet(id, childrenOf) + ordered := subtreePostOrder(id, childrenOf) + + result := &SubtreeCloseResult{} + for _, memberID := range ordered { + iss, err := s.readIssue(memberID) + if err != nil { + continue + } + if iss.Status == "closed" { + result.Skipped = append(result.Skipped, iss) + continue + } + + memberReason := reason + if memberID != id { + memberReason = fmt.Sprintf("closed with parent %s", id) + } + if err := s.moveStatus(memberID, iss.Status, "closed"); err != nil { + return nil, err + } + now := s.nowRFC3339() + iss.Status = "closed" + iss.ClosedAt = now + iss.CloseReason = memberReason + iss.UpdatedAt = now + if err := s.writeIssue(iss); err != nil { + return nil, err + } + result.Closed = append(result.Closed, iss) + } + + // With the whole subtree closed, surface only external unblocks: issues + // outside the subtree whose blockers are now all closed. Internal + // unblocks are noise since those issues are closed too. + seen := make(map[string]bool) + for _, closed := range result.Closed { + unblocked, err := s.NewlyUnblocked(closed.ID) + if err != nil { + return nil, err + } + for _, u := range unblocked { + if subtree[u.ID] || seen[u.ID] { + continue + } + seen[u.ID] = true + result.Unblocked = append(result.Unblocked, u) + } + } + + return result, nil +} + +// subtreePostOrder returns the subtree rooted at rootID in post-order, so +// every node appears after all of its descendants (root last). +func subtreePostOrder(rootID string, children map[string][]string) []string { + var out []string + var walk func(id string) + walk = func(id string) { + for _, c := range children[id] { + walk(c) + } + out = append(out, id) + } + walk(rootID) + return out +} diff --git a/internal/issue/subtree_close_test.go b/internal/issue/subtree_close_test.go new file mode 100644 index 00000000..55247aa8 --- /dev/null +++ b/internal/issue/subtree_close_test.go @@ -0,0 +1,177 @@ +package issue_test + +import ( + "testing" + + "github.com/jallum/beadwork/internal/issue" + "github.com/jallum/beadwork/internal/testutil" +) + +// child creates an issue parented to parentID. +func child(t *testing.T, env *testutil.Env, title, parentID string) *issue.Issue { + t.Helper() + iss, err := env.Store.Create(title, issue.CreateOpts{Parent: parentID}) + if err != nil { + t.Fatalf("create %q: %v", title, err) + } + return iss +} + +func TestCloseSubtreeNested(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + root, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"}) + c := child(t, env, "Child", root.ID) + gc := child(t, env, "Grandchild", c.ID) + env.CommitIntent("setup") + + result, err := env.Store.CloseSubtree(root.ID, "shipping it") + if err != nil { + t.Fatalf("CloseSubtree: %v", err) + } + + if len(result.Closed) != 3 { + t.Fatalf("closed %d issues, want 3", len(result.Closed)) + } + // Leaf-up order: grandchild, child, root. + wantOrder := []string{gc.ID, c.ID, root.ID} + for i, id := range wantOrder { + if result.Closed[i].ID != id { + t.Errorf("closed[%d] = %s, want %s", i, result.Closed[i].ID, id) + } + } + + for _, id := range wantOrder { + got, _ := env.Store.Get(id) + if got.Status != "closed" { + t.Errorf("%s status = %q, want closed", id, got.Status) + } + } + + rootIss, _ := env.Store.Get(root.ID) + if rootIss.CloseReason != "shipping it" { + t.Errorf("root reason = %q, want %q", rootIss.CloseReason, "shipping it") + } + wantNote := "closed with parent " + root.ID + for _, id := range []string{c.ID, gc.ID} { + got, _ := env.Store.Get(id) + if got.CloseReason != wantNote { + t.Errorf("%s reason = %q, want %q", id, got.CloseReason, wantNote) + } + } +} + +func TestCloseSubtreeSkipsAlreadyClosed(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + root, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"}) + c1 := child(t, env, "Child 1", root.ID) + c2 := child(t, env, "Child 2", root.ID) + env.CommitIntent("setup") + + if _, err := env.Store.Close(c1.ID, ""); err != nil { + t.Fatalf("pre-close: %v", err) + } + env.CommitIntent("pre-close") + + result, err := env.Store.CloseSubtree(root.ID, "") + if err != nil { + t.Fatalf("CloseSubtree: %v", err) + } + + if len(result.Skipped) != 1 || result.Skipped[0].ID != c1.ID { + t.Errorf("skipped = %v, want [%s]", idsOf(result.Skipped), c1.ID) + } + if len(result.Closed) != 2 { + t.Fatalf("closed %d, want 2 (%s, %s)", len(result.Closed), c2.ID, root.ID) + } + for _, id := range []string{c2.ID, root.ID} { + got, _ := env.Store.Get(id) + if got.Status != "closed" { + t.Errorf("%s status = %q, want closed", id, got.Status) + } + } +} + +func TestCloseSubtreeAlreadyClosedRoot(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + root, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"}) + c := child(t, env, "Open child under closed root", root.ID) + env.CommitIntent("setup") + + if _, err := env.Store.Close(root.ID, ""); err != nil { + t.Fatalf("pre-close root: %v", err) + } + env.CommitIntent("pre-close root") + + // Re-running against an already-closed root should mop up the open child. + result, err := env.Store.CloseSubtree(root.ID, "") + if err != nil { + t.Fatalf("CloseSubtree: %v", err) + } + if len(result.Closed) != 1 || result.Closed[0].ID != c.ID { + t.Errorf("closed = %v, want [%s]", idsOf(result.Closed), c.ID) + } + if len(result.Skipped) != 1 || result.Skipped[0].ID != root.ID { + t.Errorf("skipped = %v, want [%s]", idsOf(result.Skipped), root.ID) + } +} + +func TestCloseSubtreeExternalUnblockSurfaced(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + root, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"}) + c := child(t, env, "Child blocker", root.ID) + outsider, _ := env.Store.Create("Outside, blocked by child", issue.CreateOpts{}) + // child blocks outsider; outsider lives outside the subtree. + if err := env.Store.Link(c.ID, outsider.ID); err != nil { + t.Fatalf("link: %v", err) + } + env.CommitIntent("setup") + + result, err := env.Store.CloseSubtree(root.ID, "") + if err != nil { + t.Fatalf("CloseSubtree: %v", err) + } + + if len(result.Unblocked) != 1 || result.Unblocked[0].ID != outsider.ID { + t.Errorf("unblocked = %v, want [%s]", idsOf(result.Unblocked), outsider.ID) + } +} + +func TestCloseSubtreeInternalUnblockSuppressed(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + root, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"}) + a := child(t, env, "A blocks B", root.ID) + b := child(t, env, "B blocked by A", root.ID) + if err := env.Store.Link(a.ID, b.ID); err != nil { + t.Fatalf("link: %v", err) + } + env.CommitIntent("setup") + + result, err := env.Store.CloseSubtree(root.ID, "") + if err != nil { + t.Fatalf("CloseSubtree: %v", err) + } + + // B is inside the subtree and gets closed too, so it must not appear as + // "unblocked" — that would be noise. + if len(result.Unblocked) != 0 { + t.Errorf("unblocked = %v, want none", idsOf(result.Unblocked)) + } +} + +func idsOf(issues []*issue.Issue) []string { + ids := make([]string, len(issues)) + for i, iss := range issues { + ids[i] = iss.ID + } + return ids +} From 5e2320ccb9035c070f8a0616eed605de8d940309 Mon Sep 17 00:00:00 2001 From: Jason Allum <21417+jallum@users.noreply.github.com> Date: Sun, 31 May 2026 07:35:06 -0400 Subject: [PATCH 2/2] Add recursive delete (--recursive/-r) to cascade-delete a subtree Mirrors the recursive close in this PR for delete, per review feedback: delete an issue and its entire subtree (all descendants) leaf-up in a single commit (one "delete " intent line per removed issue, reusing the existing delete verb for deterministic replay). - Store: DeleteSubtree + DeleteSubtreePreview + SubtreeDeleteResult in internal/issue/subtree_delete.go, reusing buildSubtreeSet/subtreePostOrder. External dependents that become fully unblocked are surfaced and deduped; internal (in-subtree) unblocks are suppressed as noise. - CLI: delete grows --recursive/-r. Like the non-recursive path it previews by default; --force commits. - Tests: store-level (nested leaf-up order, deletes open+closed members, pass-through closed node, external unblock surfaced, internal unblock suppressed, still-blocked dependent not surfaced, preview mutates nothing) and CLI (preview->force, -r alias, --json shape). Follow-up to beadwork-ep5. --- cmd/bw/command.go | 4 +- cmd/bw/delete.go | 110 ++++++++++++-- cmd/bw/delete_test.go | 78 ++++++++++ internal/issue/subtree_delete.go | 139 ++++++++++++++++++ internal/issue/subtree_delete_test.go | 197 ++++++++++++++++++++++++++ 5 files changed, 519 insertions(+), 9 deletions(-) create mode 100644 internal/issue/subtree_delete.go create mode 100644 internal/issue/subtree_delete_test.go diff --git a/cmd/bw/command.go b/cmd/bw/command.go index 8547b157..c323708a 100644 --- a/cmd/bw/command.go +++ b/cmd/bw/command.go @@ -260,17 +260,19 @@ var commands = []Command{ { Name: "delete", Summary: "Delete an issue", - Description: "Permanently delete an issue and clean up references.\nWithout --force, shows a preview of what would be affected.", + Description: "Permanently delete an issue and clean up references.\nWithout --force, shows a preview of what would be affected.\nWith --recursive, also delete the issue's entire subtree (all descendants).", Positionals: []Positional{ {Name: "", Required: true, Help: "Issue ID"}, }, Flags: []Flag{ + {Long: "--recursive", Short: "-r", Help: "Also delete all descendants (the whole subtree)"}, {Long: "--force", Help: "Actually delete (default: preview only)"}, {Long: "--json", Help: "Output as JSON"}, }, Examples: []Example{ {Cmd: "bw delete bw-a3f8", Help: "Preview deletion"}, {Cmd: "bw delete bw-a3f8 --force", Help: "Delete permanently"}, + {Cmd: "bw delete bw-a3f8 -r --force", Help: "Delete the whole subtree"}, }, NeedsStore: true, Run: cmdDelete, diff --git a/cmd/bw/delete.go b/cmd/bw/delete.go index 3e58836a..e577a1e0 100644 --- a/cmd/bw/delete.go +++ b/cmd/bw/delete.go @@ -2,33 +2,37 @@ package main import ( "fmt" + "strings" "github.com/jallum/beadwork/internal/config" "github.com/jallum/beadwork/internal/issue" + "github.com/jallum/beadwork/internal/md" ) type DeleteArgs struct { - ID string - Force bool - JSON bool + ID string + Recursive bool + Force bool + JSON bool } func parseDeleteArgs(raw []string) (DeleteArgs, error) { if len(raw) == 0 { - return DeleteArgs{}, fmt.Errorf("usage: bw delete [--force] [--json]") + return DeleteArgs{}, fmt.Errorf("usage: bw delete [--recursive] [--force] [--json]") } a, err := ParseArgs(raw[1:], nil, - []string{"--force", "--json"}, + []string{"--recursive", "--force", "--json"}, ) if err != nil { return DeleteArgs{}, err } return DeleteArgs{ - ID: raw[0], - Force: a.Bool("--force"), - JSON: a.JSON(), + ID: raw[0], + Recursive: a.Bool("--recursive"), + Force: a.Bool("--force"), + JSON: a.JSON(), }, nil } @@ -38,6 +42,10 @@ func cmdDelete(store *issue.Store, args []string, w Writer, _ *config.Config) (* return nil, err } + if da.Recursive { + return cmdDeleteRecursive(store, da, w) + } + if !da.Force { // Preview mode plan, err := store.DeletePreview(da.ID) @@ -100,3 +108,89 @@ func cmdDelete(store *issue.Store, args []string, w Writer, _ *config.Config) (* } return nil, nil } + +// cmdDeleteRecursive deletes an issue and its entire subtree. Like the +// non-recursive path it previews by default; --force commits the removal in a +// single commit (one `delete ` intent line per removed issue, leaf-up). +func cmdDeleteRecursive(store *issue.Store, da DeleteArgs, w Writer) (*config.Config, error) { + if !da.Force { + plan, err := store.DeleteSubtreePreview(da.ID) + if err != nil { + return nil, err + } + if da.JSON { + fprintJSON(w, plan) + return nil, nil + } + fmt.Fprintln(w, sectionHeader(w, "DELETE PREVIEW")) + fmt.Fprintf(w, "\nIssues to delete (subtree): %d\n", len(plan.Deleted)) + w.Push(2) + for _, iss := range plan.Deleted { + fmt.Fprintf(w, "%s: %s\n", w.Style(iss.ID, Cyan), iss.Title) + } + w.Pop() + if len(plan.Unblocked) > 0 { + fmt.Fprintf(w, "\nWould unblock: %d\n", len(plan.Unblocked)) + w.Push(2) + for _, u := range plan.Unblocked { + fmt.Fprintf(w, "%s: %s\n", w.Style(u.ID, Cyan), u.Title) + } + w.Pop() + } + fmt.Fprintf(w, "\nTo proceed: %s\n", w.Style(fmt.Sprintf("bw delete %s --recursive --force", da.ID), Dim)) + return nil, nil + } + + var result *issue.SubtreeDeleteResult + err := commitWithRetry(store, commitMaxRetries, func() (string, error) { + var derr error + result, derr = store.DeleteSubtree(da.ID) + if derr != nil { + return "", derr + } + if len(result.Deleted) == 0 { + return "", fmt.Errorf("nothing to delete: %s", da.ID) + } + lines := make([]string, len(result.Deleted)) + for i, iss := range result.Deleted { + lines[i] = fmt.Sprintf("delete %s", iss.ID) + } + return strings.Join(lines, "\n"), nil + }) + if err != nil { + return nil, err + } + + if da.JSON { + if result.Deleted == nil { + result.Deleted = []*issue.Issue{} + } + if result.Unblocked == nil { + result.Unblocked = []*issue.Issue{} + } + fprintJSON(w, result) + return nil, nil + } + + fmt.Fprintf(w, "deleted %d issue(s) under {id:%s}:\n", len(result.Deleted), da.ID) + w.Push(2) + for _, iss := range result.Deleted { + fmt.Fprintf(w, "{id:%s}: ~~%s~~\n", iss.ID, md.Escape(iss.Title)) + } + w.Pop() + if len(result.Unblocked) > 0 { + fmt.Fprintln(w) + w.Push(2) + for _, u := range result.Unblocked { + fmt.Fprintf(w, "unblocked {id:%s}: %s\n", u.ID, md.Escape(u.Title)) + } + w.Pop() + fmt.Fprintln(w) + if len(result.Unblocked) == 1 { + fmt.Fprintf(w, "Next: `bw start %s` to begin, or `bw ready` for all options.\n", result.Unblocked[0].ID) + } else { + fmt.Fprintln(w, "Next: `bw ready` to see available work.") + } + } + return nil, nil +} diff --git a/cmd/bw/delete_test.go b/cmd/bw/delete_test.go index 92f7f7a1..21c596f2 100644 --- a/cmd/bw/delete_test.go +++ b/cmd/bw/delete_test.go @@ -106,3 +106,81 @@ func TestCmdDeleteNonexistent(t *testing.T) { t.Error("expected error for nonexistent issue") } } + +func TestCmdDeleteRecursivePreviewThenForce(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + root, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"}) + c, _ := env.Store.Create("Child", issue.CreateOpts{Parent: root.ID}) + gc, _ := env.Store.Create("Grandchild", issue.CreateOpts{Parent: c.ID}) + env.Repo.Commit("setup") + + // Preview (no --force): mentions the subtree, mutates nothing. + var preview bytes.Buffer + if _, err := cmdDelete(env.Store, []string{root.ID, "--recursive"}, PlainWriter(&preview), nil); err != nil { + t.Fatalf("cmdDelete --recursive (preview): %v", err) + } + if !strings.Contains(preview.String(), "subtree") { + t.Errorf("preview should describe the subtree, got: %q", preview.String()) + } + if _, err := env.Store.Get(gc.ID); err != nil { + t.Error("preview must not delete anything") + } + + // Execute with --force. + var run bytes.Buffer + if _, err := cmdDelete(env.Store, []string{root.ID, "--recursive", "--force"}, PlainWriter(&run), nil); err != nil { + t.Fatalf("cmdDelete --recursive --force: %v", err) + } + if !strings.Contains(run.String(), "deleted 3 issue") { + t.Errorf("output = %q", run.String()) + } + for _, id := range []string{root.ID, c.ID, gc.ID} { + if _, err := env.Store.Get(id); err == nil { + t.Errorf("%s should be gone after recursive delete", id) + } + } +} + +func TestCmdDeleteRecursiveAlias(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + root, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"}) + c, _ := env.Store.Create("Child", issue.CreateOpts{Parent: root.ID}) + env.Repo.Commit("setup") + + // -r aliases --recursive; with --force it should remove the whole subtree. + var buf bytes.Buffer + if _, err := cmdDelete(env.Store, []string{root.ID, "-r", "--force"}, PlainWriter(&buf), nil); err != nil { + t.Fatalf("cmdDelete -r --force: %v", err) + } + if _, err := env.Store.Get(c.ID); err == nil { + t.Error("child should be gone after -r --force") + } +} + +func TestCmdDeleteRecursiveForceJSON(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + root, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"}) + env.Store.Create("Child", issue.CreateOpts{Parent: root.ID}) + env.Repo.Commit("setup") + + var buf bytes.Buffer + if _, err := cmdDelete(env.Store, []string{root.ID, "-r", "--force", "--json"}, PlainWriter(&buf), nil); err != nil { + t.Fatalf("cmdDelete -r --force --json: %v", err) + } + var got issue.SubtreeDeleteResult + if err := json.Unmarshal(buf.Bytes(), &got); err != nil { + t.Fatalf("JSON parse: %v", err) + } + if len(got.Deleted) != 2 { + t.Errorf("deleted = %d, want 2", len(got.Deleted)) + } + if got.Unblocked == nil { + t.Error("unblocked should be [] not null") + } +} diff --git a/internal/issue/subtree_delete.go b/internal/issue/subtree_delete.go new file mode 100644 index 00000000..cee7592b --- /dev/null +++ b/internal/issue/subtree_delete.go @@ -0,0 +1,139 @@ +package issue + +import "sort" + +// SubtreeDeleteResult reports the outcome of a recursive delete: the issues +// removed (leaf-up order, descendants before parents) and the external issues +// that became fully unblocked once the subtree was gone. +type SubtreeDeleteResult struct { + Deleted []*Issue `json:"deleted"` + Unblocked []*Issue `json:"unblocked"` +} + +// subtreeDeletePlan computes, without mutating anything, the leaf-up delete +// order for the subtree rooted at id and the external dependents — issues +// outside the subtree that a subtree member blocks. Those externals are the +// candidates that may become unblocked once the subtree is removed. +// +// Like CloseSubtree, the traversal is built from all issues (including closed +// ones) so it can pass through a closed node to reach descendants beneath it. +func (s *Store) subtreeDeletePlan(id string) (ordered, extDeps []string, err error) { + id, err = s.resolveID(id) + if err != nil { + return nil, nil, err + } + + all, err := s.List(Filter{}) + if err != nil { + return nil, nil, err + } + childrenOf := make(map[string][]string) + for _, iss := range all { + if iss.Parent != "" { + childrenOf[iss.Parent] = append(childrenOf[iss.Parent], iss.ID) + } + } + + subtree := buildSubtreeSet(id, childrenOf) + ordered = subtreePostOrder(id, childrenOf) + + seen := make(map[string]bool) + for _, memberID := range ordered { + iss, err := s.readIssue(memberID) + if err != nil { + continue + } + for _, blockedID := range iss.Blocks { + if subtree[blockedID] || seen[blockedID] { + continue + } + seen[blockedID] = true + extDeps = append(extDeps, blockedID) + } + } + sort.Strings(extDeps) + return ordered, extDeps, nil +} + +// DeleteSubtreePreview returns the subtree members that a recursive delete of +// id would remove (leaf-up order) and the external dependents that would become +// fully unblocked. It does not mutate anything. +func (s *Store) DeleteSubtreePreview(id string) (*SubtreeDeleteResult, error) { + ordered, extDeps, err := s.subtreeDeletePlan(id) + if err != nil { + return nil, err + } + + result := &SubtreeDeleteResult{} + for _, memberID := range ordered { + if iss, err := s.readIssue(memberID); err == nil { + result.Deleted = append(result.Deleted, iss) + } + } + // Predict which external dependents lose their last open blocker. The + // subtree members are still live here, so exclude them when judging + // whether the dependent's remaining blockers are all resolved. + subtree := make(map[string]bool, len(ordered)) + for _, m := range ordered { + subtree[m] = true + } + for _, depID := range extDeps { + iss, err := s.readIssue(depID) + if err != nil || iss.Status == "closed" { + continue + } + if remainingBlockersResolved(s, iss.BlockedBy, subtree) { + result.Unblocked = append(result.Unblocked, iss) + } + } + return result, nil +} + +// DeleteSubtree deletes id and every descendant under it, leaf-up (a parent is +// removed only after its children). Edges to issues outside the subtree are +// severed (mirroring Delete: dependents lose the blocker, external children are +// orphaned). External issues that become fully unblocked are returned, +// deduplicated, in delete order. The removal is permanent. +func (s *Store) DeleteSubtree(id string) (*SubtreeDeleteResult, error) { + ordered, extDeps, err := s.subtreeDeletePlan(id) + if err != nil { + return nil, err + } + + result := &SubtreeDeleteResult{} + for _, memberID := range ordered { + iss, err := s.Delete(memberID) + if err != nil { + return nil, err + } + result.Deleted = append(result.Deleted, iss) + } + + // With the subtree gone and its edges severed, surface external dependents + // whose remaining blockers are now all closed. + for _, depID := range extDeps { + iss, err := s.readIssue(depID) + if err != nil || iss.Status == "closed" { + continue + } + if allResolved(s, iss.BlockedBy) { + result.Unblocked = append(result.Unblocked, iss) + } + } + return result, nil +} + +// remainingBlockersResolved reports whether every blocker outside the excluded +// set is closed. Used to predict unblocking while the excluded (about-to-be- +// deleted) issues are still live. +func remainingBlockersResolved(s *Store, blockerIDs []string, excluded map[string]bool) bool { + for _, id := range blockerIDs { + if excluded[id] { + continue + } + if !s.IsClosed(id) { + return false + } + } + return true +} diff --git a/internal/issue/subtree_delete_test.go b/internal/issue/subtree_delete_test.go new file mode 100644 index 00000000..b286cd43 --- /dev/null +++ b/internal/issue/subtree_delete_test.go @@ -0,0 +1,197 @@ +package issue_test + +import ( + "testing" + + "github.com/jallum/beadwork/internal/issue" + "github.com/jallum/beadwork/internal/testutil" +) + +func TestDeleteSubtreeNested(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + root, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"}) + c := child(t, env, "Child", root.ID) + gc := child(t, env, "Grandchild", c.ID) + env.CommitIntent("setup") + + result, err := env.Store.DeleteSubtree(root.ID) + if err != nil { + t.Fatalf("DeleteSubtree: %v", err) + } + + if len(result.Deleted) != 3 { + t.Fatalf("deleted %d issues, want 3", len(result.Deleted)) + } + // Leaf-up order: grandchild, child, root. + wantOrder := []string{gc.ID, c.ID, root.ID} + for i, id := range wantOrder { + if result.Deleted[i].ID != id { + t.Errorf("deleted[%d] = %s, want %s", i, result.Deleted[i].ID, id) + } + } + + // All three must be gone from the live tree. + for _, id := range wantOrder { + if _, err := env.Store.Get(id); err == nil { + t.Errorf("%s should no longer resolve after delete", id) + } + } +} + +func TestDeleteSubtreeDeletesClosedAndOpenMembers(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + root, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"}) + open := child(t, env, "Open child", root.ID) + closed := child(t, env, "Closed child", root.ID) + env.Store.Close(closed.ID, "") + env.CommitIntent("setup") + + result, err := env.Store.DeleteSubtree(root.ID) + if err != nil { + t.Fatalf("DeleteSubtree: %v", err) + } + // Unlike archive/close, delete removes members regardless of status. + if len(result.Deleted) != 3 { + t.Fatalf("deleted %d, want 3 (root, %s, %s)", len(result.Deleted), open.ID, closed.ID) + } + for _, id := range []string{root.ID, open.ID, closed.ID} { + if _, err := env.Store.Get(id); err == nil { + t.Errorf("%s should be gone", id) + } + } +} + +func TestDeleteSubtreeThroughClosedNodeReachesOpenDescendant(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + root, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"}) + mid := child(t, env, "Closed mid", root.ID) + leaf := child(t, env, "Open leaf", mid.ID) + env.Store.Close(mid.ID, "") + env.CommitIntent("setup") + + result, err := env.Store.DeleteSubtree(root.ID) + if err != nil { + t.Fatalf("DeleteSubtree: %v", err) + } + // The walk must pass through the closed mid to reach the open leaf. + if len(result.Deleted) != 3 { + t.Fatalf("deleted %d, want 3", len(result.Deleted)) + } + if _, err := env.Store.Get(leaf.ID); err == nil { + t.Error("open leaf under a closed mid should be deleted") + } +} + +func TestDeleteSubtreeExternalUnblockSurfaced(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + root, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"}) + c := child(t, env, "Child blocker", root.ID) + outsider, _ := env.Store.Create("Outside, blocked by child", issue.CreateOpts{}) + // child blocks outsider; outsider lives outside the subtree. + if err := env.Store.Link(c.ID, outsider.ID); err != nil { + t.Fatalf("link: %v", err) + } + env.CommitIntent("setup") + + result, err := env.Store.DeleteSubtree(root.ID) + if err != nil { + t.Fatalf("DeleteSubtree: %v", err) + } + + if len(result.Unblocked) != 1 || result.Unblocked[0].ID != outsider.ID { + t.Errorf("unblocked = %v, want [%s]", idsOf(result.Unblocked), outsider.ID) + } + // The edge must actually be gone from the surviving outsider. + o, _ := env.Store.Get(outsider.ID) + if len(o.BlockedBy) != 0 { + t.Errorf("outsider.BlockedBy = %v, want empty after subtree delete", o.BlockedBy) + } +} + +func TestDeleteSubtreeInternalUnblockSuppressed(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + root, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"}) + a := child(t, env, "A blocks B", root.ID) + b := child(t, env, "B blocked by A", root.ID) + if err := env.Store.Link(a.ID, b.ID); err != nil { + t.Fatalf("link: %v", err) + } + env.CommitIntent("setup") + + result, err := env.Store.DeleteSubtree(root.ID) + if err != nil { + t.Fatalf("DeleteSubtree: %v", err) + } + + // B is inside the subtree and gets deleted too, so it must not appear as + // "unblocked" — that would be noise (and a dangling reference). + if len(result.Unblocked) != 0 { + t.Errorf("unblocked = %v, want none", idsOf(result.Unblocked)) + } +} + +func TestDeleteSubtreeExternalDependentStillBlockedNotSurfaced(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + root, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"}) + c := child(t, env, "Child blocker", root.ID) + otherBlocker, _ := env.Store.Create("Other open blocker", issue.CreateOpts{}) + outsider, _ := env.Store.Create("Outside, two blockers", issue.CreateOpts{}) + env.Store.Link(c.ID, outsider.ID) + env.Store.Link(otherBlocker.ID, outsider.ID) + env.CommitIntent("setup") + + result, err := env.Store.DeleteSubtree(root.ID) + if err != nil { + t.Fatalf("DeleteSubtree: %v", err) + } + // outsider still has an open blocker outside the subtree, so deleting the + // subtree must not report it as unblocked. + if len(result.Unblocked) != 0 { + t.Errorf("unblocked = %v, want none (other blocker still open)", idsOf(result.Unblocked)) + } +} + +func TestDeleteSubtreePreviewMutatesNothing(t *testing.T) { + env := testutil.NewEnv(t) + defer env.Cleanup() + + root, _ := env.Store.Create("Epic", issue.CreateOpts{Type: "epic"}) + c := child(t, env, "Child", root.ID) + gc := child(t, env, "Grandchild", c.ID) + outsider, _ := env.Store.Create("Outside, blocked by child", issue.CreateOpts{}) + env.Store.Link(c.ID, outsider.ID) + env.CommitIntent("setup") + + plan, err := env.Store.DeleteSubtreePreview(root.ID) + if err != nil { + t.Fatalf("DeleteSubtreePreview: %v", err) + } + if len(plan.Deleted) != 3 { + t.Errorf("preview Deleted = %v, want 3 members", idsOf(plan.Deleted)) + } + if len(plan.Unblocked) != 1 || plan.Unblocked[0].ID != outsider.ID { + t.Errorf("preview Unblocked = %v, want [%s]", idsOf(plan.Unblocked), outsider.ID) + } + // Nothing should have been mutated. + for _, id := range []string{root.ID, c.ID, gc.ID, outsider.ID} { + if _, err := env.Store.Get(id); err != nil { + t.Errorf("preview must not delete %s: %v", id, err) + } + } + o, _ := env.Store.Get(outsider.ID) + if len(o.BlockedBy) != 1 { + t.Errorf("preview must not sever edges; outsider.BlockedBy = %v", o.BlockedBy) + } +}