From 256268b7ab52c337416037d645c3b0651e6178d0 Mon Sep 17 00:00:00 2001 From: tdwd <111124579+tdwd@users.noreply.github.com> Date: Fri, 18 Sep 2026 12:38:30 +0200 Subject: [PATCH 1/7] Move worktree creation into its own file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AddWorktree and ErrNoCommits leave gitx.go for worktree.go, and their tests follow into worktree_test.go. No line of the function body changes. Size did not force this: gitx.go was 155 lines, under the limit. The category did. What stays in gitx.go is running git at all and the questions asked of a repository before Deck commits to anything — where its root is, what it has checked out, whether it has a commit yet. Making a worktree is a different job, and RemoveWorktree needs somewhere to land that is not the file holding the rev-parse wrappers. ErrNoCommits travels because AddWorktree is its only producer. ErrNotARepo stays, because RepoRoot returns it too. --- internal/gitx/gitx.go | 41 ++------------- internal/gitx/gitx_test.go | 80 ------------------------------ internal/gitx/worktree.go | 48 ++++++++++++++++++ internal/gitx/worktree_test.go | 91 ++++++++++++++++++++++++++++++++++ 4 files changed, 144 insertions(+), 116 deletions(-) create mode 100644 internal/gitx/worktree.go create mode 100644 internal/gitx/worktree_test.go diff --git a/internal/gitx/gitx.go b/internal/gitx/gitx.go index 3d580a5..f274916 100644 --- a/internal/gitx/gitx.go +++ b/internal/gitx/gitx.go @@ -4,6 +4,11 @@ // guaranteed to agree with the user's own `git worktree list`. package gitx +// Running git at all, and the questions asked of a repository before Deck +// commits to anything: where its root is, what it has checked out, whether it +// has a commit yet, and whether a directory is a project or a shelf of them. +// The worktree a session gets is worktree.go. + import ( "bytes" "errors" @@ -17,11 +22,6 @@ import ( // ErrNotARepo is returned when a path is not inside a git working tree. var ErrNotARepo = errors.New("not a git repository") -// ErrNoCommits is returned when a repository has an unborn HEAD — freshly -// initialised, nothing committed. There is no commit for a worktree to check -// out, so an isolated session is impossible until the first commit exists. -var ErrNoCommits = errors.New("repository has no commits yet") - // run executes git in dir and returns trimmed stdout. git writes its // diagnostics to stderr, so the error carries them verbatim — a wrapped // "exit status 128" on its own tells the user nothing. @@ -122,34 +122,3 @@ func HoldsRepos(dir string) bool { func HeadCommit(dir string) (string, error) { return run(dir, "rev-parse", "HEAD") } - -// AddWorktree creates branch at the current HEAD of repo and checks it out -// into dest. dest must not exist; git refuses to reuse a populated directory -// and we do not try to talk it round. -func AddWorktree(repo, dest, branch string) error { - // A project need not be a repository — it may be a directory that only - // collects them. Say that plainly rather than letting the unborn-HEAD - // check below report "no commits yet", which is true of a non-repository - // and tells the user nothing about what is actually wrong. - if _, err := RepoRoot(repo); err != nil { - // Wrapping prepends the sentinel's own text, so this half must not - // repeat it or the message reads "not a git repository: X is not a - // git repository". - return fmt.Errorf("%w: %s has no branch to work from — open the session in the project directory instead", - ErrNotARepo, filepath.Base(repo)) - } - // Check for an unborn HEAD next. Without this the caller gets git's own - // "fatal: invalid reference: HEAD", which is accurate and tells a user - // nothing about what to do next. - if !HasCommits(repo) { - return fmt.Errorf("%w: commit something first, or open the session in the project directory", ErrNoCommits) - } - if err := os.MkdirAll(filepath.Dir(dest), 0o755); err != nil { - return fmt.Errorf("create worktree parent: %w", err) - } - if _, err := os.Stat(dest); err == nil { - return fmt.Errorf("worktree path already exists: %s", dest) - } - _, err := run(repo, "worktree", "add", "-b", branch, dest, "HEAD") - return err -} diff --git a/internal/gitx/gitx_test.go b/internal/gitx/gitx_test.go index b2aa958..64b40f6 100644 --- a/internal/gitx/gitx_test.go +++ b/internal/gitx/gitx_test.go @@ -50,46 +50,6 @@ func TestHeadBranch(t *testing.T) { } } -// TestAddWorktree covers what a session needs: a checkout of its own, on its -// own branch. -// -// There is no removal half. Deck deliberately leaves a worktree on disk -// when a session closes, because it may hold uncommitted work — so there is no -// remove function to test, and adding one for the test's sake would be code -// with no caller. -func TestAddWorktree(t *testing.T) { - repo := testRepo(t) - dest := filepath.Join(t.TempDir(), "worktrees", "scheming-hawk-jhgk") - const branch = "session/scheming-hawk-jhgk" - - if err := AddWorktree(repo, dest, branch); err != nil { - t.Fatalf("AddWorktree: %v", err) - } - if _, err := os.Stat(filepath.Join(dest, ".git")); err != nil { - t.Fatalf("worktree has no .git: %v", err) - } - got, err := HeadBranch(dest) - if err != nil { - t.Fatalf("HeadBranch in worktree: %v", err) - } - if got != branch { - t.Errorf("worktree branch = %q, want %q", got, branch) - } -} - -// TestAddWorktreeRefusesExistingPath matters because silently reusing a -// populated directory would put an agent to work in someone else's tree. -func TestAddWorktreeRefusesExistingPath(t *testing.T) { - repo := testRepo(t) - dest := filepath.Join(t.TempDir(), "taken") - if err := os.MkdirAll(dest, 0o755); err != nil { - t.Fatal(err) - } - if err := AddWorktree(repo, dest, "session/x"); err == nil { - t.Fatal("AddWorktree overwrote an existing path") - } -} - // TestRunErrorCarriesGitStderr keeps the diagnostics: a bare "exit status 128" // tells the user nothing about what git objected to. func TestRunErrorCarriesGitStderr(t *testing.T) { @@ -103,52 +63,12 @@ func TestRunErrorCarriesGitStderr(t *testing.T) { } } -// TestAddWorktreeOnUnbornHead covers a freshly `git init`-ed repository. git's -// own message is "fatal: invalid reference: HEAD", which is accurate and tells -// a user nothing about what to do next. -func TestAddWorktreeOnUnbornHead(t *testing.T) { - // gittest.Repo initialises without committing, which is the unborn HEAD - // this test is about. - dir := gittest.Repo(t) - - if HasCommits(dir) { - t.Fatal("a repository with no commits reported HasCommits") - } - - err := AddWorktree(dir, filepath.Join(t.TempDir(), "wt"), "session/x") - if !errors.Is(err, ErrNoCommits) { - t.Fatalf("error = %v, want ErrNoCommits", err) - } - if !strings.Contains(err.Error(), "project directory") { - t.Errorf("error does not suggest the way out: %v", err) - } -} - func TestHasCommits(t *testing.T) { if !HasCommits(testRepo(t)) { t.Error("a repository with a commit reported no commits") } } -// TestAddWorktreeOnNonRepo covers a project that is a collector of -// repositories rather than one itself. Without the explicit check the -// unborn-HEAD branch reports "no commits yet", which is true of a -// non-repository and explains nothing. -func TestAddWorktreeOnNonRepo(t *testing.T) { - plain := t.TempDir() - - err := AddWorktree(plain, filepath.Join(t.TempDir(), "wt"), "session/x") - if !errors.Is(err, ErrNotARepo) { - t.Fatalf("error = %v, want ErrNotARepo", err) - } - if strings.Contains(err.Error(), "no commits") { - t.Errorf("a non-repository was reported as having no commits: %v", err) - } - if !strings.Contains(err.Error(), "project directory") { - t.Errorf("error does not suggest the way out: %v", err) - } -} - // TestHoldsRepos covers the collector check that lets `deck` seed a directory // which is not itself a repository but coordinates several that are. func TestHoldsRepos(t *testing.T) { diff --git a/internal/gitx/worktree.go b/internal/gitx/worktree.go new file mode 100644 index 0000000..b923aa6 --- /dev/null +++ b/internal/gitx/worktree.go @@ -0,0 +1,48 @@ +package gitx + +// The worktree an isolated session works in: creating one on a branch of its +// own, and refusing when the project has nothing to branch from or the path is +// already taken. + +import ( + "errors" + "fmt" + "os" + "path/filepath" +) + +// ErrNoCommits is returned when a repository has an unborn HEAD — freshly +// initialised, nothing committed. There is no commit for a worktree to check +// out, so an isolated session is impossible until the first commit exists. +var ErrNoCommits = errors.New("repository has no commits yet") + +// AddWorktree creates branch at the current HEAD of repo and checks it out +// into dest. dest must not exist; git refuses to reuse a populated directory +// and we do not try to talk it round. +func AddWorktree(repo, dest, branch string) error { + // A project need not be a repository — it may be a directory that only + // collects them. Say that plainly rather than letting the unborn-HEAD + // check below report "no commits yet", which is true of a non-repository + // and tells the user nothing about what is actually wrong. + if _, err := RepoRoot(repo); err != nil { + // Wrapping prepends the sentinel's own text, so this half must not + // repeat it or the message reads "not a git repository: X is not a + // git repository". + return fmt.Errorf("%w: %s has no branch to work from — open the session in the project directory instead", + ErrNotARepo, filepath.Base(repo)) + } + // Check for an unborn HEAD next. Without this the caller gets git's own + // "fatal: invalid reference: HEAD", which is accurate and tells a user + // nothing about what to do next. + if !HasCommits(repo) { + return fmt.Errorf("%w: commit something first, or open the session in the project directory", ErrNoCommits) + } + if err := os.MkdirAll(filepath.Dir(dest), 0o755); err != nil { + return fmt.Errorf("create worktree parent: %w", err) + } + if _, err := os.Stat(dest); err == nil { + return fmt.Errorf("worktree path already exists: %s", dest) + } + _, err := run(repo, "worktree", "add", "-b", branch, dest, "HEAD") + return err +} diff --git a/internal/gitx/worktree_test.go b/internal/gitx/worktree_test.go new file mode 100644 index 0000000..8157c10 --- /dev/null +++ b/internal/gitx/worktree_test.go @@ -0,0 +1,91 @@ +package gitx + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/tripledownab/deck/internal/gittest" +) + +// TestAddWorktree covers what a session needs: a checkout of its own, on its +// own branch. +// +// There is no removal half. Deck deliberately leaves a worktree on disk +// when a session closes, because it may hold uncommitted work — so there is no +// remove function to test, and adding one for the test's sake would be code +// with no caller. +func TestAddWorktree(t *testing.T) { + repo := testRepo(t) + dest := filepath.Join(t.TempDir(), "worktrees", "scheming-hawk-jhgk") + const branch = "session/scheming-hawk-jhgk" + + if err := AddWorktree(repo, dest, branch); err != nil { + t.Fatalf("AddWorktree: %v", err) + } + if _, err := os.Stat(filepath.Join(dest, ".git")); err != nil { + t.Fatalf("worktree has no .git: %v", err) + } + got, err := HeadBranch(dest) + if err != nil { + t.Fatalf("HeadBranch in worktree: %v", err) + } + if got != branch { + t.Errorf("worktree branch = %q, want %q", got, branch) + } +} + +// TestAddWorktreeRefusesExistingPath matters because silently reusing a +// populated directory would put an agent to work in someone else's tree. +func TestAddWorktreeRefusesExistingPath(t *testing.T) { + repo := testRepo(t) + dest := filepath.Join(t.TempDir(), "taken") + if err := os.MkdirAll(dest, 0o755); err != nil { + t.Fatal(err) + } + if err := AddWorktree(repo, dest, "session/x"); err == nil { + t.Fatal("AddWorktree overwrote an existing path") + } +} + +// TestAddWorktreeOnUnbornHead covers a freshly `git init`-ed repository. git's +// own message is "fatal: invalid reference: HEAD", which is accurate and tells +// a user nothing about what to do next. +func TestAddWorktreeOnUnbornHead(t *testing.T) { + // gittest.Repo initialises without committing, which is the unborn HEAD + // this test is about. + dir := gittest.Repo(t) + + if HasCommits(dir) { + t.Fatal("a repository with no commits reported HasCommits") + } + + err := AddWorktree(dir, filepath.Join(t.TempDir(), "wt"), "session/x") + if !errors.Is(err, ErrNoCommits) { + t.Fatalf("error = %v, want ErrNoCommits", err) + } + if !strings.Contains(err.Error(), "project directory") { + t.Errorf("error does not suggest the way out: %v", err) + } +} + +// TestAddWorktreeOnNonRepo covers a project that is a collector of +// repositories rather than one itself. Without the explicit check the +// unborn-HEAD branch reports "no commits yet", which is true of a +// non-repository and explains nothing. +func TestAddWorktreeOnNonRepo(t *testing.T) { + plain := t.TempDir() + + err := AddWorktree(plain, filepath.Join(t.TempDir(), "wt"), "session/x") + if !errors.Is(err, ErrNotARepo) { + t.Fatalf("error = %v, want ErrNotARepo", err) + } + if strings.Contains(err.Error(), "no commits") { + t.Errorf("a non-repository was reported as having no commits: %v", err) + } + if !strings.Contains(err.Error(), "project directory") { + t.Errorf("error does not suggest the way out: %v", err) + } +} From f8a5b8842b9b7e9525ec0960ebd3691da62c754e Mon Sep 17 00:00:00 2001 From: tdwd <111124579+tdwd@users.noreply.github.com> Date: Fri, 18 Sep 2026 12:38:39 +0200 Subject: [PATCH 2/7] Correct three claims that measurement disproves Each described behaviour the code or git does not have, and each was found by running something rather than by reading it again. AddWorktree's doc said git refuses to reuse a populated directory. Running `git worktree add -b feat HEAD` exits 0, so git accepts an empty one. The os.Stat check in AddWorktree is Deck's own rule and it is the stricter one: it refuses any existing path. TestRunErrorCarriesGitStderr asserted the error mentions the branch name. run formats its message as "git : ", so the args already carry the branch. Replacing the stderr read with err.Error() left the test passing on "git worktree add -b main ...: exit status 255". It now asserts the message is not the bare exit status, which is the assertion that fails when the passthrough is removed. TestHoldsRepos ended with an if whose body was a bare return, as the last statement of the function. Both branches ended the test, so the one-level property it named was asserted by nothing. HoldsRepos does stop at one level. The test now builds a repository two levels down and checks that its grandparent is refused. --- internal/gitx/gitx_test.go | 25 ++++++++++++++++++++----- internal/gitx/worktree.go | 8 ++++++-- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/internal/gitx/gitx_test.go b/internal/gitx/gitx_test.go index 64b40f6..c60ad3e 100644 --- a/internal/gitx/gitx_test.go +++ b/internal/gitx/gitx_test.go @@ -52,14 +52,23 @@ func TestHeadBranch(t *testing.T) { // TestRunErrorCarriesGitStderr keeps the diagnostics: a bare "exit status 128" // tells the user nothing about what git objected to. +// +// It asserts on git's own words rather than on the branch name, because run +// formats the message as "git : " and the args already contain +// the branch. The earlier assertion was that the message mentioned "main", +// which stayed green with the stderr read replaced by err.Error() — measured, +// not reasoned about. The negative assertion is the discriminating one. func TestRunErrorCarriesGitStderr(t *testing.T) { repo := testRepo(t) err := AddWorktree(repo, filepath.Join(t.TempDir(), "wt"), "main") if err == nil { t.Fatal("creating a branch that already exists succeeded") } - if !strings.Contains(err.Error(), "main") { - t.Errorf("error does not mention the branch: %v", err) + if strings.Contains(err.Error(), "exit status") { + t.Errorf("error fell back to the exit code: %v", err) + } + if !strings.Contains(err.Error(), "already exists") { + t.Errorf("error does not carry git's complaint: %v", err) } } @@ -85,9 +94,15 @@ func TestHoldsRepos(t *testing.T) { t.Error("a directory holding a repository was not recognised") } - // One level only: a grandparent is not a collector, or $HOME would be. - if HoldsRepos(filepath.Dir(collector)) == HoldsRepos(collector) { - return // sibling temp dirs may coincidentally contain repos; not asserting + // One level only: a grandparent is not a collector, or $HOME would be. The + // tree is built here rather than read from filepath.Dir(collector), whose + // contents the test does not control. + deep := t.TempDir() + if err := os.MkdirAll(filepath.Join(deep, "mid", "repo", ".git"), 0o755); err != nil { + t.Fatal(err) + } + if HoldsRepos(deep) { + t.Error("a repository two levels down made its grandparent a collector") } } diff --git a/internal/gitx/worktree.go b/internal/gitx/worktree.go index b923aa6..cf27c87 100644 --- a/internal/gitx/worktree.go +++ b/internal/gitx/worktree.go @@ -17,8 +17,12 @@ import ( var ErrNoCommits = errors.New("repository has no commits yet") // AddWorktree creates branch at the current HEAD of repo and checks it out -// into dest. dest must not exist; git refuses to reuse a populated directory -// and we do not try to talk it round. +// into dest. +// +// dest must not exist, and that is Deck's rule rather than git's. `git worktree +// add` accepts an existing empty directory, so the os.Stat below is the +// stricter check: it refuses any existing path. Reusing one would put a session +// to work in a tree that something else already owns. func AddWorktree(repo, dest, branch string) error { // A project need not be a repository — it may be a directory that only // collects them. Say that plainly rather than letting the unborn-HEAD From 2af0790c237f94463b6c6e61a598940531195c37 Mon Sep 17 00:00:00 2001 From: tdwd <111124579+tdwd@users.noreply.github.com> Date: Fri, 18 Sep 2026 12:46:13 +0200 Subject: [PATCH 3/7] Add worktree and branch removal to gitx RemoveWorktree runs `git worktree remove` and DeleteBranch runs `git branch -d`. Neither forces, which is the whole design: git's own refusals are the answer rather than an obstacle to work around. Every claim in the doc comments was measured against a real repository rather than read off the manual. - A tree holding modified or untracked files is refused. Untracked is the case that catches people: an agent that wrote a file and never added it leaves the tree dirty, with nothing in `git diff` to show for it. - `git branch -d` refuses a branch a worktree still has checked out, which is why the order is worktree first and branch second. The reverse fails on every session rather than on the ones worth refusing. - `git branch -d` measures merged-ness against the branch's upstream, or against HEAD when there is none. `worktree add -b` off a local HEAD sets none, so for a session branch it is HEAD. - A directory the user already deleted is not an error. git drops the registration and reports success, which is what lets a delete finish instead of stranding the session behind a tree that is already gone. Each test was checked by stubbing both functions to return nil. All five fail, so none of them is green on the fixture alone. Both functions are exported with no production caller. The consumer is the x modal in the next commit on this branch. If that does not land, this commit comes out with it. --- internal/gitx/worktree.go | 39 ++++++++- internal/gitx/worktree_test.go | 144 +++++++++++++++++++++++++++++++-- 2 files changed, 176 insertions(+), 7 deletions(-) diff --git a/internal/gitx/worktree.go b/internal/gitx/worktree.go index cf27c87..c246f8b 100644 --- a/internal/gitx/worktree.go +++ b/internal/gitx/worktree.go @@ -1,8 +1,8 @@ package gitx // The worktree an isolated session works in: creating one on a branch of its -// own, and refusing when the project has nothing to branch from or the path is -// already taken. +// own, removing both when the session is deleted, and the refusals on either +// side that Deck reports rather than forces. import ( "errors" @@ -50,3 +50,38 @@ func AddWorktree(repo, dest, branch string) error { _, err := run(repo, "worktree", "add", "-b", branch, dest, "HEAD") return err } + +// RemoveWorktree removes dest from repo. It never forces. +// +// git refuses a tree holding modified or untracked files, and that refusal is +// the answer rather than an obstacle to work around: the tree is where the +// session's work is. Untracked counts, which is the case that catches people — +// an agent that wrote a file and never added it leaves the tree dirty by this +// test. git's message names the path and says --force exists, so the user has +// the way out without Deck offering it as a button. +// +// A dest the user already deleted by hand is not an error. git drops the +// administrative entry and reports success. +func RemoveWorktree(repo, dest string) error { + _, err := run(repo, "worktree", "remove", dest) + return err +} + +// DeleteBranch deletes branch from repo. It never forces. +// +// Call it after RemoveWorktree, never before: git refuses to delete a branch +// that a worktree still has checked out, so the reverse order fails on every +// session rather than on the ones worth refusing. +// +// -d refuses a branch whose commits are not merged. git measures against the +// branch's upstream, or against HEAD when there is none, and `worktree add -b` +// off a local HEAD sets none — so for a session branch it is HEAD, unless the +// user has configured git to set tracking on every new branch. +// +// That refusal is the case where the session goes and its work stays, so the +// caller keeps the branch and says so. A session that committed nothing sits on +// an ancestor of HEAD and deletes without complaint. +func DeleteBranch(repo, branch string) error { + _, err := run(repo, "branch", "-d", branch) + return err +} diff --git a/internal/gitx/worktree_test.go b/internal/gitx/worktree_test.go index 8157c10..ad50c4d 100644 --- a/internal/gitx/worktree_test.go +++ b/internal/gitx/worktree_test.go @@ -10,13 +10,24 @@ import ( "github.com/tripledownab/deck/internal/gittest" ) +// session is a repository with one committed file and an isolated session's +// worktree checked out of it, which is where every removal case starts. +// +// The file is committed rather than left absent because the refusal tests need +// something tracked to modify. +func session(t *testing.T) (repo, dest, branch string) { + t.Helper() + repo, _ = gittest.RepoWith(t, "a.txt", "one\n") + dest = filepath.Join(t.TempDir(), "wt") + branch = "session/scheming-hawk-jhgk" + if err := AddWorktree(repo, dest, branch); err != nil { + t.Fatalf("AddWorktree: %v", err) + } + return repo, dest, branch +} + // TestAddWorktree covers what a session needs: a checkout of its own, on its // own branch. -// -// There is no removal half. Deck deliberately leaves a worktree on disk -// when a session closes, because it may hold uncommitted work — so there is no -// remove function to test, and adding one for the test's sake would be code -// with no caller. func TestAddWorktree(t *testing.T) { repo := testRepo(t) dest := filepath.Join(t.TempDir(), "worktrees", "scheming-hawk-jhgk") @@ -89,3 +100,126 @@ func TestAddWorktreeOnNonRepo(t *testing.T) { t.Errorf("error does not suggest the way out: %v", err) } } + +// TestRemoveWorktreeAndBranch is the path a deleted session takes when its work +// is committed and merged: the tree goes, the registration goes, the branch +// goes. +func TestRemoveWorktreeAndBranch(t *testing.T) { + repo, dest, branch := session(t) + + if err := RemoveWorktree(repo, dest); err != nil { + t.Fatalf("RemoveWorktree: %v", err) + } + if _, err := os.Stat(dest); !os.IsNotExist(err) { + t.Errorf("worktree directory survived: stat err = %v", err) + } + // The directory going is not the same as git forgetting it. A stale entry + // keeps the path reserved and shows up in the user's own worktree list. + if list := gittest.Run(t, repo, "worktree", "list"); strings.Contains(list, dest) { + t.Errorf("git still lists the worktree:\n%s", list) + } + + if err := DeleteBranch(repo, branch); err != nil { + t.Fatalf("DeleteBranch: %v", err) + } + if out := gittest.Run(t, repo, "branch", "--list", branch); out != "" { + t.Errorf("branch --list = %q, want it gone", out) + } +} + +// TestRemoveWorktreeRefusesWork is the promise that nothing is forced. Both +// cases are the same refusal from git, and the untracked one is here because it +// is the one that surprises: an agent that wrote a file and never added it has +// left the tree dirty, with nothing in `git diff` to show for it. +func TestRemoveWorktreeRefusesWork(t *testing.T) { + for _, tc := range []struct { + name string + file string + }{ + {"modified tracked file", "a.txt"}, + {"untracked file only", "scratch.txt"}, + } { + t.Run(tc.name, func(t *testing.T) { + repo, dest, _ := session(t) + if err := os.WriteFile(filepath.Join(dest, tc.file), []byte("work\n"), 0o644); err != nil { + t.Fatal(err) + } + + err := RemoveWorktree(repo, dest) + if err == nil { + t.Fatal("RemoveWorktree deleted a tree holding work") + } + if _, statErr := os.Stat(dest); statErr != nil { + t.Errorf("a refused removal still took the directory: %v", statErr) + } + // Not asserting that the message names dest: run formats "git + // : " and dest is an argument, so that would hold + // with the stderr dropped. See TestRunErrorCarriesGitStderr. + if strings.Contains(err.Error(), "exit status") { + t.Errorf("refusal does not say what git objected to: %v", err) + } + }) + } +} + +// TestDeleteBranchRefusesUnmergedWork is the worktree-is-the-gate rule seen from +// the branch side. A session that committed and never merged leaves a clean tree +// that removes fine, and then the branch is the only copy of the work. +func TestDeleteBranchRefusesUnmergedWork(t *testing.T) { + repo, dest, branch := session(t) + if err := os.WriteFile(filepath.Join(dest, "b.txt"), []byte("work\n"), 0o644); err != nil { + t.Fatal(err) + } + gittest.Run(t, dest, "add", "b.txt") + gittest.Run(t, dest, "commit", "-q", "-m", "session work") + + if err := RemoveWorktree(repo, dest); err != nil { + t.Fatalf("a committed session left a dirty tree: %v", err) + } + + err := DeleteBranch(repo, branch) + if err == nil { + t.Fatal("DeleteBranch dropped the only copy of unmerged work") + } + // The branch name is already an argument, so asserting on it would hold with + // the stderr dropped. "not fully merged" is what only git can have said, and + // it is the sentence the caller turns into the notice. + if !strings.Contains(err.Error(), "not fully merged") { + t.Errorf("refusal does not say why the branch was kept: %v", err) + } + if out := gittest.Run(t, repo, "branch", "--list", branch); out == "" { + t.Error("the branch is gone after a refused delete") + } +} + +// TestDeleteBranchRefusesWhileCheckedOut pins the ordering DeleteBranch's doc +// states. Called before RemoveWorktree it fails on every session, merged or not, +// so getting the order wrong would look like a broken delete rather than a rule. +func TestDeleteBranchRefusesWhileCheckedOut(t *testing.T) { + repo, _, branch := session(t) + + if err := DeleteBranch(repo, branch); err == nil { + t.Fatal("DeleteBranch deleted a branch a worktree had checked out") + } + if out := gittest.Run(t, repo, "branch", "--list", branch); out == "" { + t.Error("the branch is gone after a refused delete") + } +} + +// TestRemoveWorktreeAfterTheDirectoryIsGone covers the user who deleted the +// tree by hand. git drops the registration and reports success, which is what +// lets a delete finish instead of stranding the session behind a tree that is +// already gone. +func TestRemoveWorktreeAfterTheDirectoryIsGone(t *testing.T) { + repo, dest, _ := session(t) + if err := os.RemoveAll(dest); err != nil { + t.Fatal(err) + } + + if err := RemoveWorktree(repo, dest); err != nil { + t.Fatalf("RemoveWorktree on an absent tree: %v", err) + } + if list := gittest.Run(t, repo, "worktree", "list"); strings.Contains(list, dest) { + t.Errorf("git still lists the removed worktree:\n%s", list) + } +} From 05f70376f9aaddd7d27dab9855d8c2618489d08e Mon Sep 17 00:00:00 2001 From: tdwd <111124579+tdwd@users.noreply.github.com> Date: Fri, 18 Sep 2026 14:16:59 +0200 Subject: [PATCH 4/7] Rename connectFrom to pickerSubject The field holds the session the open picker is about, captured because a modal outlives the keystroke that opened it. That reason is not specific to connecting, and the next commit opens a second picker that needs the same capture for the same reason. Rename only. No behaviour changes. --- internal/ui/connections.go | 4 ++-- internal/ui/model.go | 7 ++++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/internal/ui/connections.go b/internal/ui/connections.go index d7af16f..75de4c2 100644 --- a/internal/ui/connections.go +++ b/internal/ui/connections.go @@ -67,14 +67,14 @@ func (m Model) openConnectPicker(sess *store.Session) (tea.Model, tea.Cmd) { m.notice = "no sessions on another project to connect to" return m, nil } - m.connectFrom = sess.ID + m.pickerSubject = sess.ID m.picker = newPicker(pickConnect, "Connect "+sessionLabel(*sess)+" to", rows, "") return m, nil } // connectPicked links or unlinks the chosen session and persists the result. func (m Model) connectPicked(otherID string) (tea.Model, tea.Cmd) { - sess := m.state.Session(m.connectFrom) + sess := m.state.Session(m.pickerSubject) other := m.state.Session(otherID) if sess == nil || other == nil { return m, nil diff --git a/internal/ui/model.go b/internal/ui/model.go index ad76f9e..313bafa 100644 --- a/internal/ui/model.go +++ b/internal/ui/model.go @@ -55,14 +55,15 @@ type Model struct { picker *picker showHelp bool - // connectFrom is the session the open connect picker is linking from. + // pickerSubject is the session the open picker is about — today the one a + // connect is linking from. // // Captured when the picker opens rather than resolved on the commit key, // for the same reason picker.restore is: the modal outlives the keystroke // that opened it, and the dashboard and the session view resolve "the - // selected session" by different rules. Re-reading it would connect + // selected session" by different rules. Re-reading it would act on // whichever session the other screen happens to be pointing at. - connectFrom string + pickerSubject string // notice is a one-line message in the footer; fault is a failure the user // must see, and it outranks the notice. From de582db3782565b5833e338bc37dcaff2c539ad5 Mon Sep 17 00:00:00 2001 From: tdwd <111124579+tdwd@users.noreply.github.com> Date: Fri, 18 Sep 2026 14:23:26 +0200 Subject: [PATCH 5/7] Extract stopRunner from stopCurrent Stopping an agent and releasing what it held in the coordinator always happen together, and the pair was written out twice. The session-delete path would have made it three. A claim held by a process that has exited is worse than no claim, which is why the two belong in one call rather than side by side at every call site. No behaviour change. stopCurrent still reports "session is not running" before stopping anything, and still tells the coordinator only when there was a runner to stop. --- internal/ui/runner.go | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/internal/ui/runner.go b/internal/ui/runner.go index 4613278..a62d2f7 100644 --- a/internal/ui/runner.go +++ b/internal/ui/runner.go @@ -57,18 +57,29 @@ func (m *Model) stopCurrent() { if sess == nil { return } - r, ok := m.runners[sess.ID] - if !ok { + if _, ok := m.runners[sess.ID]; !ok { m.notice = "session is not running" return } - r.Stop() - delete(m.runners, sess.ID) - m.releaseCoord(sess.ID) + m.stopRunner(sess.ID) m.attached = false m.notice = "stopped " + sess.Name } +// stopRunner stops a session's agent and frees what it held in the +// coordinator. +// +// The two always happen together, which is why they are one call: a claim held +// by a process that has exited is worse than no claim. Stopping a session that +// was not running is not an error, so the coordinator is told either way. +func (m *Model) stopRunner(sessionID string) { + if r, ok := m.runners[sessionID]; ok { + r.Stop() + delete(m.runners, sessionID) + } + m.releaseCoord(sessionID) +} + // releaseCoord drops a session from the coordinator, freeing its claims. A // claim held by a process that has exited is worse than no claim: the next // agent believes someone is working there. @@ -80,7 +91,7 @@ func (m *Model) releaseCoord(sessionID string) { // sweepExited releases sessions whose agent stopped without being told to. // -// stopCurrent and closeSelectedFromDashboard cover the deliberate paths, but +// stopCurrent and stopRunner cover the deliberate paths, but // an agent can also leave on its own — /exit, a crash, the process being // killed. Nothing calls back into the UI when that happens, so without this // sweep the coordinator keeps listing a dead session and holding its claims, From 9d218c4f1b00c53dfc9f7f9198fa5940db0d193c Mon Sep 17 00:00:00 2001 From: tdwd <111124579+tdwd@users.noreply.github.com> Date: Fri, 18 Sep 2026 14:24:01 +0200 Subject: [PATCH 6/7] Ask what to do with the worktree when a session ends MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit x closed the session outright, so the key that keeps a worktree and the key that would remove one were the same key with no step in between. It now opens a modal holding both outcomes. Close is the row the cursor starts on. It is the reversible one, and a modal that opens on the destructive row turns a confirmation into a trap. The Delete row carries what the session changed — "3 files changed, 41 insertions(+)", or "no files changed" — rather than a warning about what is about to go. A confirmation that states a fact can be answered. One that states a warning can only be believed or dismissed. A measurement that fails says so in place of the figure, because a row reading "no files changed" because git errored would state a fact it does not have. Nothing is forced. git refuses a worktree holding modified or untracked files and the session then stays, so the user can open it again, commit, and delete it after. The worktree is the gate and the branch is best effort: a clean tree whose branch holds unmerged commits removes fine, `branch -d` then refuses, and the notice names the branch that was kept. A session that ran in the project directory skips the modal and closes as before. It has no worktree and no branch of its own, so Delete would have nothing to remove. deleteSession guards that itself rather than trusting its caller, because such a session's Dir is the project's own checkout. The delete tests drive real git rather than a stub. The refusals are the whole feature, and a fake that refuses when told to would prove only that it was told. --- README.md | 2 +- internal/ui/app_test.go | 92 ++++++++++++++++-- internal/ui/closing.go | 138 ++++++++++++++++++++++---- internal/ui/closing_test.go | 188 ++++++++++++++++++++++++++++++++++++ internal/ui/format.go | 21 ++++ internal/ui/keyroutes.go | 2 +- internal/ui/keys.go | 2 +- internal/ui/picker.go | 1 + internal/ui/pickerctl.go | 43 +++++++++ 9 files changed, 458 insertions(+), 31 deletions(-) create mode 100644 internal/ui/closing_test.go diff --git a/README.md b/README.md index 0e3e42e..334d963 100644 --- a/README.md +++ b/README.md @@ -114,7 +114,7 @@ the arrows keep working inside `claude`. | `n` | new session | | `a` | add project | | `e` | rename project | -| `x` | close session | +| `x` | end session — close and keep the worktree, or delete both | | `c` | connect this session to one on another project | | `t` | theme picker | | `?` | help | diff --git a/internal/ui/app_test.go b/internal/ui/app_test.go index d648c70..5f955f1 100644 --- a/internal/ui/app_test.go +++ b/internal/ui/app_test.go @@ -4,6 +4,7 @@ import ( "strings" "testing" + tea "github.com/charmbracelet/bubbletea" "github.com/tripledownab/deck/internal/store" ) @@ -175,6 +176,9 @@ func TestCloseIgnoresProjectsColumn(t *testing.T) { refused, _ := m.dashboardKey(typed("x")) after := refused.(Model) + if after.picker != nil { + t.Error("x opened the end-session modal from the projects list") + } if n := len(after.state.Sessions); n != 2 { t.Errorf("sessions = %d after x on the projects list, want both kept", n) } @@ -182,22 +186,94 @@ func TestCloseIgnoresProjectsColumn(t *testing.T) { t.Error("the refused key said nothing about why") } - // The same key still closes with the session list focused, so what refused - // above was the guard and not a close that had stopped working. + // The same key still asks with the session list focused, so what refused + // above was the guard and not a route that had stopped working. after.focusContent() - closed, _ := after.dashboardKey(typed("x")) + asked, _ := after.dashboardKey(typed("x")) + if asked.(Model).picker == nil { + t.Error("x did not open the modal with the session list focused") + } +} + +// TestEndModalClosesAndKeepsTheWorktree drives the whole route: the key, the +// modal, and the default row. +// +// Close is the row the cursor starts on, so enter without moving is the +// reversible outcome. Naming the directory is the whole promise of keeping it — +// a notice that only said "closed" would leave the work somewhere the user +// cannot find. +func TestEndModalClosesAndKeepsTheWorktree(t *testing.T) { + t.Setenv("XDG_STATE_HOME", t.TempDir()) + m := modelWith(2) + m.focusContent() + + opened, _ := m.dashboardKey(typed("x")) + asked := opened.(Model) + if asked.picker == nil { + t.Fatal("x did not open the end-session modal") + } + if got := asked.picker.selected(); got != endClose { + t.Errorf("the modal opened on %q, want the reversible row %q", got, endClose) + } + + closed, _ := asked.pickerKey(tea.KeyMsg{Type: tea.KeyEnter}) done := closed.(Model) + if done.picker != nil { + t.Error("the modal stayed up after a choice") + } if n := len(done.state.Sessions); n != 1 { - t.Errorf("sessions = %d after x with the session list focused, want 1", n) + t.Errorf("sessions = %d after choosing Close, want 1", n) } - // Naming the directory is the whole promise of keeping the worktree: a - // notice that only said "closed" would leave the work somewhere the user - // cannot find. Asserting the text, not just that there is some. if !strings.Contains(done.notice, "/worktrees/sess") { t.Errorf("notice = %q, want it to name where the worktree was kept", done.notice) } } +// TestEndModalEscapeKeepsTheSession is the reason the modal exists. x alone +// used to end a session outright, so the key had no step at which the user +// could change their mind. +func TestEndModalEscapeKeepsTheSession(t *testing.T) { + t.Setenv("XDG_STATE_HOME", t.TempDir()) + m := modelWith(2) + m.focusContent() + + opened, _ := m.dashboardKey(typed("x")) + away, _ := opened.(Model).pickerKey(tea.KeyMsg{Type: tea.KeyEsc}) + after := away.(Model) + + if after.picker != nil { + t.Error("esc left the modal up") + } + if n := len(after.state.Sessions); n != 2 { + t.Errorf("sessions = %d after esc, want both kept", n) + } +} + +// TestEndSkipsTheModalWithoutAWorktree covers a session that ran in the project +// directory. It has no worktree and no branch of its own, so Delete would have +// nothing to remove and a modal offering it would be offering nothing. +func TestEndSkipsTheModalWithoutAWorktree(t *testing.T) { + t.Setenv("XDG_STATE_HOME", t.TempDir()) + st := &store.State{} + p := st.AddProject(store.Project{Name: "demo", Path: "/demo"}) + st.AddSession(store.Session{ProjectID: p.ID, Name: "inplace", Title: "in place", Dir: "/demo"}) + m := New(st, "bash", nil) + m.focusContent() + + ended, _ := m.dashboardKey(typed("x")) + after := ended.(Model) + + if after.picker != nil { + t.Fatal("a session with no worktree was asked what to do with one") + } + if n := len(after.state.Sessions); n != 0 { + t.Errorf("sessions = %d, want the session closed outright", n) + } + if strings.Contains(after.notice, "worktree") { + t.Errorf("notice = %q, want no mention of a worktree it never had", after.notice) + } +} + // TestClosingTheLastSessionReleasesFocus keeps colContent meaning "a session is // selected", which is what focusedSession's refusal depends on. // @@ -212,7 +288,7 @@ func TestClosingTheLastSessionReleasesFocus(t *testing.T) { t.Fatal("could not focus the session list") } - m.closeSelectedFromDashboard() + m.closeSession(*m.focusedSession()) if m.focus != colProjects { t.Errorf("focus = %v after closing the last session, want the projects column", m.focus) diff --git a/internal/ui/closing.go b/internal/ui/closing.go index 722f7c9..c8974fa 100644 --- a/internal/ui/closing.go +++ b/internal/ui/closing.go @@ -1,47 +1,145 @@ package ui -// Ending a session: stopping its agent, forgetting the record, and what is -// deliberately left on disk behind it. +// Ending a session, which is two outcomes rather than one: closing forgets the +// record and keeps the worktree, deleting removes the worktree and the branch +// behind it. The modal that asks which is pickerctl.go. -// closeSelectedFromDashboard stops a session's agent and forgets it. +import ( + tea "github.com/charmbracelet/bubbletea" + "github.com/tripledownab/deck/internal/gitx" + "github.com/tripledownab/deck/internal/store" +) + +// endSelectedFromDashboard is x on the dashboard. +// +// Only an isolated session is asked the question. One that ran in the project +// directory has no worktree and no branch of its own, so there is nothing for +// Delete to remove and a modal offering it would be offering nothing. +func (m Model) endSelectedFromDashboard() (tea.Model, tea.Cmd) { + sess := m.focusedSession() + if sess == nil { + return m, nil + } + if !sess.Isolated { + m.closeSession(*sess) + return m, nil + } + return m.openEndSessionPicker(sess) +} + +// endPicked carries out the choice the modal collected. +// +// It resolves the session from pickerSubject rather than from the cursor: the +// figure on the Delete row was measured for that session when the modal opened, +// and a confirmation that states a fact has to act on the thing it measured. +func (m Model) endPicked(choice string) (tea.Model, tea.Cmd) { + sess := m.state.Session(m.pickerSubject) + if sess == nil { + return m, nil + } + if choice == endDelete { + m.deleteSession(*sess) + } else { + m.closeSession(*sess) + } + return m, nil +} + +// closeSession stops a session's agent and forgets it. // // The worktree is left on disk on purpose. It may hold uncommitted work, and // deleting a branch's only checkout to tidy a list is not a trade Deck // gets to make silently. The notice says where it went. -func (m *Model) closeSelectedFromDashboard() { - p := m.currentProject() - selected := m.focusedSession() - if p == nil || selected == nil { +func (m *Model) closeSession(sess store.Session) { + m.stopRunner(sess.ID) + if !m.forget(sess) { + return + } + if sess.Isolated { + m.notice = "closed " + sess.Name + " — worktree kept at " + sess.Dir + } else { + m.notice = "closed " + sess.Name + } +} + +// deleteSession closes a session and removes the worktree and branch behind it. +// +// Nothing is forced. git refuses a worktree holding modified or untracked +// files, and that refusal is the answer: the session stays, so the user can +// open it again, commit the work, and delete it after. +// +// The worktree is the gate and the branch is best effort. A clean tree whose +// branch holds unmerged commits removes fine and then `branch -d` refuses, +// which is the right outcome — the session goes and the work stays — so the +// notice names the branch that was kept. +func (m *Model) deleteSession(sess store.Session) { + p := m.state.Project(sess.ProjectID) + // Guarded here and not only where the modal opens. A non-isolated session's + // Dir is the project directory itself, so a delete that reached this would + // aim git at the user's own checkout. + if p == nil || !sess.Isolated { + m.notice = sess.Name + " has no worktree of its own to delete" + return + } + + // The agent stops before the tree is touched, because git counts a file it + // is still writing and cannot be asked to wait. A refusal below therefore + // leaves the session in place with its agent stopped, which Deck already + // treats as a restart rather than a dead end. + m.stopRunner(sess.ID) + if err := gitx.RemoveWorktree(p.Path, sess.Dir); err != nil { + m.fault = err + return + } + + kept := "" + if sess.Branch != "" { + if err := gitx.DeleteBranch(p.Path, sess.Branch); err != nil { + kept = sess.Branch + } + } + + if !m.forget(sess) { return } - sess := *selected - if r, ok := m.runners[sess.ID]; ok { - r.Stop() - delete(m.runners, sess.ID) + m.notice = "deleted " + sess.Name + if kept != "" { + m.notice += " — branch " + kept + " kept, it holds unmerged commits" } - m.releaseCoord(sess.ID) +} + +// forget drops the record and saves, then puts the lists and the cursor back in +// a consistent state. It reports whether the save succeeded. +// +// Callers do their disk work first and call this last, so a worktree that +// refused to go still has a row naming it. The reverse order would leave an +// orphan the user can no longer see, retry or delete through Deck. +// +// This is not itself atomic, and the comment says so rather than implying +// otherwise. RemoveSession runs before Save, so a failed Save leaves the +// session gone from memory and still in state.json, and it returns at the next +// launch. Putting it back is not possible from here: RemoveSession also drops +// the links this session held, and the copy passed in does not carry them. +// Making the pair atomic belongs in store, not in a caller working around it. +func (m *Model) forget(sess store.Session) bool { // RemoveSession drops the links this session held, so the coordinator has // to be told: releaseCoord frees claims and the inbox, but peers is set // wholesale and outlives an agent exiting on purpose. m.state.RemoveSession(sess.ID) if err := m.state.Save(); err != nil { m.fault = err - return + return false } m.syncConnections() m.rebuildRows() - left := m.state.SessionsFor(p.ID) + left := m.state.SessionsFor(sess.ProjectID) m.listIx = clamp(m.listIx, 0, max(len(left)-1, 0)) - // Closing the last one leaves the cursor in a column with nothing in it — + // Ending the last one leaves the cursor in a column with nothing in it — // the state focusContent refuses to create, reached from the other side. // focusedSession's promise that a refusal says why rests on this: with no // sessions there is nothing for it to name, so it would refuse in silence. if len(left) == 0 { m.focus = colProjects } - if sess.Isolated { - m.notice = "closed " + sess.Name + " — worktree kept at " + sess.Dir - } else { - m.notice = "closed " + sess.Name - } + return true } diff --git a/internal/ui/closing_test.go b/internal/ui/closing_test.go new file mode 100644 index 0000000..0395b6a --- /dev/null +++ b/internal/ui/closing_test.go @@ -0,0 +1,188 @@ +package ui + +// Deleting a session against a real repository. The refusals are the point of +// the feature, so they are driven through git rather than a stub: a fake that +// refuses when told to would prove only that it was told. + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/tripledownab/deck/internal/gittest" + "github.com/tripledownab/deck/internal/gitx" + "github.com/tripledownab/deck/internal/store" +) + +// deckWithWorktree builds a model holding one project that is a real +// repository and one isolated session with a real worktree checked out of it. +func deckWithWorktree(t *testing.T) (Model, store.Session) { + t.Helper() + t.Setenv("XDG_STATE_HOME", t.TempDir()) + + repo, head := gittest.RepoWith(t, "a.txt", "one\n") + dest := filepath.Join(t.TempDir(), "wt") + const branch = "session/quiet-otter-abcd" + if err := gitx.AddWorktree(repo, dest, branch); err != nil { + t.Fatalf("AddWorktree: %v", err) + } + + st := &store.State{} + p := st.AddProject(store.Project{Name: "demo", Path: repo}) + sess := st.AddSession(store.Session{ + ProjectID: p.ID, + Name: "quiet-otter-abcd", + Title: "work", + Branch: branch, + BaseRef: head, + Dir: dest, + Isolated: true, + }) + m := New(st, "bash", nil) + m.focusContent() + return m, *sess +} + +// TestDeleteRemovesTheWorktreeAndBranch is the outcome the feature exists for: +// a session that changed nothing leaves nothing behind. +func TestDeleteRemovesTheWorktreeAndBranch(t *testing.T) { + m, sess := deckWithWorktree(t) + repo := m.state.Project(sess.ProjectID).Path + + m.deleteSession(sess) + + if m.fault != nil { + t.Fatalf("deleting a clean session reported: %v", m.fault) + } + if _, err := os.Stat(sess.Dir); !os.IsNotExist(err) { + t.Errorf("the worktree survived: stat err = %v", err) + } + if out := gittest.Run(t, repo, "branch", "--list", sess.Branch); out != "" { + t.Errorf("branch --list = %q, want the branch gone", out) + } + if n := len(m.state.Sessions); n != 0 { + t.Errorf("sessions = %d, want the record forgotten", n) + } +} + +// TestDeleteRefusesADirtyWorktree is the promise that nothing is forced. The +// file is never added, because untracked is the case that catches people: an +// agent that wrote one and stopped has left work with nothing in `git diff` to +// show for it. +func TestDeleteRefusesADirtyWorktree(t *testing.T) { + m, sess := deckWithWorktree(t) + if err := os.WriteFile(filepath.Join(sess.Dir, "scratch.txt"), []byte("work\n"), 0o644); err != nil { + t.Fatal(err) + } + + m.deleteSession(sess) + + if m.fault == nil { + t.Fatal("deleting a tree holding work reported nothing") + } + if _, err := os.Stat(sess.Dir); err != nil { + t.Errorf("a refused delete still took the directory: %v", err) + } + // The session has to survive its own refused delete. A record dropped over + // a surviving worktree is an orphan the user can no longer see or retry. + if n := len(m.state.Sessions); n != 1 { + t.Errorf("sessions = %d after a refused delete, want the session kept", n) + } +} + +// TestDeleteKeepsAnUnmergedBranch is the worktree-is-the-gate rule. A session +// that committed leaves a clean tree, so the tree goes and the branch is then +// the only copy of the work. git refuses it, the session still goes, and the +// notice is the only place the user learns where the work is. +func TestDeleteKeepsAnUnmergedBranch(t *testing.T) { + m, sess := deckWithWorktree(t) + repo := m.state.Project(sess.ProjectID).Path + if err := os.WriteFile(filepath.Join(sess.Dir, "b.txt"), []byte("work\n"), 0o644); err != nil { + t.Fatal(err) + } + gittest.Run(t, sess.Dir, "add", "b.txt") + gittest.Run(t, sess.Dir, "commit", "-q", "-m", "session work") + + m.deleteSession(sess) + + if m.fault != nil { + t.Fatalf("a committed session was refused: %v", m.fault) + } + if _, err := os.Stat(sess.Dir); !os.IsNotExist(err) { + t.Errorf("the worktree survived: stat err = %v", err) + } + if out := gittest.Run(t, repo, "branch", "--list", sess.Branch); out == "" { + t.Error("the branch holding the only copy of the work was deleted") + } + if n := len(m.state.Sessions); n != 0 { + t.Errorf("sessions = %d, want the record forgotten", n) + } + if !strings.Contains(m.notice, sess.Branch) { + t.Errorf("notice = %q, want it to name the branch that was kept", m.notice) + } +} + +// TestDeleteRefusesASessionWithoutAWorktree guards the case the modal is not +// supposed to reach. A non-isolated session's Dir is the project directory, so +// a delete that got here would aim git at the user's own checkout. +func TestDeleteRefusesASessionWithoutAWorktree(t *testing.T) { + t.Setenv("XDG_STATE_HOME", t.TempDir()) + repo, _ := gittest.RepoWith(t, "a.txt", "one\n") + + st := &store.State{} + p := st.AddProject(store.Project{Name: "demo", Path: repo}) + sess := st.AddSession(store.Session{ + ProjectID: p.ID, Name: "inplace", Title: "in place", Dir: repo, + }) + m := New(st, "bash", nil) + + m.deleteSession(*sess) + + if _, err := os.Stat(filepath.Join(repo, "a.txt")); err != nil { + t.Fatalf("the project's own checkout was touched: %v", err) + } + if n := len(m.state.Sessions); n != 1 { + t.Errorf("sessions = %d, want the session kept", n) + } + if m.notice == "" { + t.Error("the refusal said nothing about why") + } +} + +// TestWorkSummaryIsOneLine keeps the Delete row answerable. gitx.Work.Stat is +// one line per changed file followed by the total, and only the total fits. +func TestWorkSummaryIsOneLine(t *testing.T) { + _, sess := deckWithWorktree(t) + for _, name := range []string{"b.txt", "c.txt"} { + if err := os.WriteFile(filepath.Join(sess.Dir, name), []byte("work\n"), 0o644); err != nil { + t.Fatal(err) + } + } + + got := workSummary(sess) + + if strings.Contains(got, "\n") { + t.Errorf("summary spans lines, so the row cannot hold it:\n%s", got) + } + if !strings.Contains(got, "2 files changed") { + t.Errorf("summary = %q, want the total across both new files", got) + } +} + +// TestWorkSummaryReportsAFailedMeasurement covers a session with no recorded +// base. Answering "no files changed" there would be a confirmation stating a +// fact it does not have, which is worse than one admitting it could not look. +func TestWorkSummaryReportsAFailedMeasurement(t *testing.T) { + _, sess := deckWithWorktree(t) + sess.BaseRef = "" + + got := workSummary(sess) + + if strings.Contains(got, "no files changed") { + t.Errorf("summary = %q, want it to admit the measurement failed", got) + } + if !strings.Contains(got, "could not measure") { + t.Errorf("summary = %q, want it to say the measurement failed", got) + } +} diff --git a/internal/ui/format.go b/internal/ui/format.go index cb7bc2b..4162fa8 100644 --- a/internal/ui/format.go +++ b/internal/ui/format.go @@ -6,7 +6,9 @@ package ui import ( "fmt" "path/filepath" + "strings" + "github.com/tripledownab/deck/internal/gitx" "github.com/tripledownab/deck/internal/store" ) @@ -56,3 +58,22 @@ func projectLabel(state *store.State, projectID string) string { } return filepath.Base(p.Path) } + +// workSummary is the one line a modal row can hold about what a session +// changed: "3 files changed, 41 insertions(+)", or "no files changed". +// +// gitx.Work.Stat is one line per changed file followed by that total, and the +// total is the last of them. Diff substitutes "no files changed" for an empty +// stat, so there is always a line to take. +// +// A failed measurement is reported in place of the figure. A row that said "no +// files changed" because git errored would be a confirmation stating a fact it +// does not have, which is worse than one admitting it could not look. +func workSummary(sess store.Session) string { + w, err := gitx.Diff(sess.Dir, sess.BaseRef) + if err != nil { + return "could not measure what it changed: " + err.Error() + } + lines := strings.Split(strings.TrimSpace(w.Stat), "\n") + return strings.TrimSpace(lines[len(lines)-1]) +} diff --git a/internal/ui/keyroutes.go b/internal/ui/keyroutes.go index d32d852..f3db165 100644 --- a/internal/ui/keyroutes.go +++ b/internal/ui/keyroutes.go @@ -39,7 +39,7 @@ func (m Model) dashboardKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { case key.Matches(msg, m.keys.Theme): return m.openThemePicker() case key.Matches(msg, m.keys.Delete): - m.closeSelectedFromDashboard() + return m.endSelectedFromDashboard() case key.Matches(msg, m.keys.Enter): return m.openFromDashboard() } diff --git a/internal/ui/keys.go b/internal/ui/keys.go index 722f576..ba2c0d9 100644 --- a/internal/ui/keys.go +++ b/internal/ui/keys.go @@ -64,7 +64,7 @@ func defaultKeys() keyMap { NewSession: key.NewBinding(key.WithKeys("n"), key.WithHelp("n", "new session")), AddProject: key.NewBinding(key.WithKeys("a"), key.WithHelp("a", "add project")), Rename: key.NewBinding(key.WithKeys("e"), key.WithHelp("e", "rename project")), - Delete: key.NewBinding(key.WithKeys("x"), key.WithHelp("x", "close session")), + Delete: key.NewBinding(key.WithKeys("x"), key.WithHelp("x", "end session")), Connect: key.NewBinding(key.WithKeys("c"), key.WithHelp("c", "connect to a session elsewhere")), Theme: key.NewBinding(key.WithKeys("t"), key.WithHelp("t", "theme")), diff --git a/internal/ui/picker.go b/internal/ui/picker.go index a2bdb8e..71e57a0 100644 --- a/internal/ui/picker.go +++ b/internal/ui/picker.go @@ -18,6 +18,7 @@ const ( pickTheme pickerKind = iota pickProject pickConnect + pickEndSession ) // picker is a modal list: a title, rows, a cursor. diff --git a/internal/ui/pickerctl.go b/internal/ui/pickerctl.go index 18aaed6..06eefbb 100644 --- a/internal/ui/pickerctl.go +++ b/internal/ui/pickerctl.go @@ -8,6 +8,15 @@ import ( "fmt" tea "github.com/charmbracelet/bubbletea" + "github.com/tripledownab/deck/internal/store" +) + +// The two rows of the end-session modal. They are ids rather than labels +// because the label is what the user reads and the id is what endPicked +// branches on, and a row renamed for clarity must not change what it does. +const ( + endClose = "close" + endDelete = "delete" ) // openThemePicker opens the theme list, positioned on the active theme. @@ -32,6 +41,25 @@ func (m Model) openFieldPicker() (tea.Model, tea.Cmd) { return m, nil } +// openEndSessionPicker asks what ending a session should do with the worktree +// behind it. +// +// Close is the row the cursor starts on. It is the reversible outcome, and a +// modal that opens on the destructive one turns a confirmation into a trap. +// +// The Delete row carries what that session changed rather than a warning about +// what it is about to lose. A confirmation that states a fact can be answered; +// one that states a warning can only be believed or dismissed. +func (m Model) openEndSessionPicker(sess *store.Session) (tea.Model, tea.Cmd) { + rows := []pickerRow{ + {id: endClose, label: "Close", desc: "keep the worktree at " + sess.Dir}, + {id: endDelete, label: "Delete", desc: "remove the worktree and branch — " + workSummary(*sess)}, + } + m.pickerSubject = sess.ID + m.picker = newPicker(pickEndSession, "End "+sessionLabel(*sess), rows, endClose) + return m, nil +} + // pickerKey drives the theme picker. // // The palette is applied as the cursor moves, so the whole frame behind the @@ -56,6 +84,21 @@ func (m Model) pickerKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return m, nil } + // Ending previews nothing, and esc is the way out of a question rather than + // a way to undo an answer. The modal exists so the destructive row costs a + // second, deliberate keystroke. + if m.picker.kind == pickEndSession { + switch { + case cancel: + m.picker = nil + case commit: + choice := m.picker.selected() + m.picker = nil + return m.endPicked(choice) + } + return m, nil + } + // A field picker floats over the open form and only writes back into it. // Nothing is previewed and nothing is persisted, so cancel is simply // closing it. From 7b296a22ef723cc5611e6722a5d6b9b21d8cf6b7 Mon Sep 17 00:00:00 2001 From: tdwd <111124579+tdwd@users.noreply.github.com> Date: Fri, 18 Sep 2026 14:34:57 +0200 Subject: [PATCH 7/7] Document ending a session, and close backlog entry 14 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds "Ending a session is two outcomes" to the architecture doc: the two rows, why Close is the default, and the three rules the code has to keep — nothing is forced, the worktree is the gate and the branch is best effort, and a session in the project directory has nothing to delete. It also says plainly that forget is not atomic. RemoveSession runs before Save, so a failed save leaves the session gone from memory and still in state.json. Making the pair atomic belongs in store, and saying so is better than a doc that implies the guarantee. Four stale claims the change exposed, each checked against the code: - The dead-code paragraph named RemoveWorktree and DeleteBranch as helpers gitx does not carry, and prescribed adding one back with the feature that needs it. That is exactly what happened, so the paragraph now records it. IsDirty, AheadBehind and HeadSubject are still absent. - The file map named picker_open.go, which was renamed to pickerctl.go. - "One list widget, two jobs" counted two kinds. There are four. - README's key table said x closes a session. It asks now. --- docs/architecture.md | 72 +++++++++++++++++++++++++++++++++++------ docs/backlog.md | 77 +++++++++++--------------------------------- internal/ui/model.go | 4 +-- 3 files changed, 83 insertions(+), 70 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index f77d594..ad8212d 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -56,7 +56,7 @@ see its siblings. ``` main.go flags, state load, cwd registration, teardown after Run internal/naming scheming-hawk-jhgk names, branch names, slugs -internal/gitx repo root, branch, worktree add +internal/gitx repo root, branch, worktree add and remove internal/termquery answers terminal queries for harnesses with no terminal internal/store projects + sessions, atomic JSON persistence: store.go the document, load and save @@ -90,6 +90,8 @@ internal/ui the Bubble Tea program, split by job: keys.go the binding table; ptykeys.go encodes to bytes selection.go cursor and list navigation actions.go forms; sessions.go and projects.go do the work + closing.go ending a session: close keeps the worktree, + delete removes it and the branch runner.go agent lifecycle; agentargs.go builds its argv dashboard.go + projectlist.go / projectdetail.go / help.go session.go + chrome.go / sidebar.go / status.go / pane.go @@ -97,7 +99,7 @@ internal/ui the Bubble Tea program, split by job: formproject.go build the forms, formfields.go the pieces, forminput.go keys browser.go the directory explorer; dirlist.go lists it - picker.go the list modal; picker_open.go opens it + picker.go the list modal; pickerctl.go opens and drives it theme.go + palette.go / styles.go internal/gittest throwaway git repositories for tests, in one place because four packages had grown their own and they had drifted @@ -106,8 +108,12 @@ probe/ debug harness: renders frames without a human present `gitx` holds only what something calls. It briefly carried `IsDirty`, `AheadBehind`, `HeadSubject`, `RemoveWorktree` and `DeleteBranch` because a git -wrapper "should" have them; all five had tests and no caller, which is how -dead code passes review. If a feature needs one, add it back with the feature. +wrapper "should" have them; all five had tests and no caller, which is how dead +code passes review. If a feature needs one, add it back with the feature. + +`RemoveWorktree` and `DeleteBranch` came back that way, with the `x` modal that +calls them. The other three are still absent, and having a test is still not +having a caller. ### Relationship to cathode @@ -541,13 +547,21 @@ screen and one keystroke from the right directory, so an error belongs there. A pick that survives opens the project form with the path prefilled and focus on the description. -### One list widget, two jobs (`ui/picker.go`) +### One list widget, four jobs (`ui/picker.go`) + +`picker` is a modal list with a `pickerKind`. It serves the theme list, any form +field marked `pickable` — currently the project field — the connect list, and +the question `x` asks about a worktree. The kinds behave differently on purpose. +The theme picker previews by applying and persists on commit. A field picker +floats over the open form, previews nothing, and only writes the chosen value +back into the field. Connecting and ending both act on commit alone, because a +link is a change to the store and ending a session is not reversible, so +previewing either as the cursor moves would do the thing being asked about. -`picker` is a modal list with a `pickerKind`. It serves the theme list and any -form field marked `pickable` — currently the project field. The kinds behave -differently on purpose: the theme picker previews by applying and persists on -commit, while a field picker floats over the open form, previews nothing, and -only writes the chosen value back into the field. +`pickerSubject` holds the session an open picker is about, captured when it +opens rather than read back on the commit key. The modal outlives the keystroke +that opened it, and the dashboard and the session view resolve "the selected +session" by different rules. The form's own choice field renders options as a row of chips and falls back to a one-at-a-time cycler when they overflow. That is right for two working-copy @@ -674,6 +688,44 @@ switching to the project directory is one `tab` away. Check `gitx.HasCommits` before anything else that assumes a resolvable HEAD — git's own "fatal: invalid reference: HEAD" is accurate and useless. +### Ending a session is two outcomes (`ui/closing.go`) + +`x` opens a modal rather than acting. **Close** forgets the record and leaves +the worktree on disk. **Delete** removes the worktree and the branch. Close is +the row the cursor starts on, because it is the reversible one and a modal that +opens on the destructive row turns a confirmation into a trap. + +The Delete row carries what the session changed — "3 files changed, 41 +insertions(+)" — measured by `gitx.Diff` against `Session.BaseRef`. A +confirmation that states a fact can be answered. One that states a warning can +only be believed or dismissed. A failed measurement says so in place of the +figure, because a row reading "no files changed" because git errored would state +a fact it does not have. + +**Nothing is forced.** `git worktree remove` refuses a tree holding modified or +untracked files and `git branch -d` refuses unmerged commits, and both refusals +are the answer rather than an obstacle. Untracked counts, which is the case that +surprises: an agent that wrote a file and never added it leaves the tree dirty +with nothing in `git diff` to show for it. The session stays, so the way out is +to open it again, commit the work, and delete it after. + +**The worktree is the gate, the branch is best effort.** A clean tree whose +branch holds unmerged commits removes fine, and `branch -d` then refuses. The +session goes and the work stays, which is the right outcome, so the notice names +the branch that was kept. + +**A session that ran in the project directory has nothing to delete.** Its `Dir` +is the project's own checkout, so it skips the modal and closes outright. +`deleteSession` guards that itself rather than trusting its caller, because the +cost of reaching it with one is git aimed at the user's own tree. + +The disk work comes first and `forget` comes last, so a worktree that refused to +go still has a row naming it. `forget` is not itself atomic: `RemoveSession` +runs before `Save`, so a failed save leaves the session gone from memory and +still in `state.json`, where it returns at the next launch. Putting it back is +not possible from there, because `RemoveSession` also drops the links the +session held. Making the pair atomic belongs in `store`. + ### An exited agent is a restart, not a dead end `landOn` drops the attachment when the cursor moves to a session with no diff --git a/docs/backlog.md b/docs/backlog.md index 722ab33..a88ee48 100644 --- a/docs/backlog.md +++ b/docs/backlog.md @@ -236,64 +236,25 @@ and three decisions around it. Numbered 13 rather than reusing 12: that number already names the public-tree notice, and a closed entry should not change meaning. -## 14. Deleting a session, and the worktree it leaves - -Closing forgets the record and keeps the worktree, which is the right default -and currently the only one. Nothing in Deck removes what it keeps, so thirty -closed **isolated** sessions are thirty trees under -`$XDG_STATE_HOME/deck/worktrees`, thirty `session/*` branches, and thirty -entries in the project's `git worktree list`. The only way out today is git by -hand. A session that ran in the project directory leaves nothing behind, which -is the distinction the fourth decision below turns on. - -`x` opens a modal with both outcomes rather than growing a second key. -**Close** keeps the worktree and is the default. **Delete** removes the worktree -and the branch. The Delete row carries `gitx.Diff` against `BaseRef` — "3 files -changed, 41 insertions" or "no files changed" — because a confirmation that -states a fact is answerable and one that states a warning is not. Deck already -measures exactly this for `analyse`, so the figure costs nothing new. - -Four decisions the code has to keep. - -**The disk work comes first, and the record is forgotten only if it succeeded.** -A dropped record over a surviving worktree is an orphan the user can no longer -see, retry or name. The order is: stop the runner, unregister from the -coordinator, remove the worktree, delete the branch, then `RemoveSession` and -`Save`. - -**Nothing is forced.** `git worktree remove` refuses a dirty tree and `git -branch -d` refuses unmerged commits. Both refusals are the answer, reported with -the path. A "delete anyway" row is the one affordance the dirty case cannot -afford, and the way out — commit it, or remove it by hand — fits in the notice. -Add force only if that refusal proves to be a real obstacle. - -**The worktree is the gate, the branch is best effort.** A clean worktree whose -branch holds unmerged commits removes fine, and then `branch -d` refuses. The -session is gone and the work is not, which is the right outcome, so the notice -says the branch was kept and why. - -**A non-isolated session has no worktree and no branch.** Its `Dir` is the -project directory itself. It skips the modal and closes as it does today, and -the delete path must never be reachable with one. `coord.workOf` already refuses -one for the neighbouring reason — a shared project directory holds everyone's -edits at once, so its changes cannot be told apart — and the same fact rules out -deleting anything on such a session's behalf. - -The guard that came out of reading the handler shipped ahead of this, because it -was small and needed nothing from the modal. `x` fired whatever column had -focus, while `dashboardSession` resolves through `listIx`, so pressing it on the -projects list closed that project's newest session — a row the keyboard was not -driving. The row was never invisible: `cursorMarker` keeps a dimmed cursor on -the unfocused column deliberately. What was missing was the focus, and -`sectionLeft` already scoped ←/→ by it for the stated reason — the focused -column is drawn with an accent border, and a key that reaches across makes that -border a lie. `focusedSession` is where the rule lives now, and `c` goes through -it too. It matters to this entry because the modal must open on the session the -user pointed at, not on whichever one `listIx` happens to hold. - -Deliberately not this: `x` on the projects column meaning "remove project". What -happens to that project's sessions and their worktrees deserves its own answer, -not one reached in passing inside a session delete. +## ~~14. Deleting a session, and the worktree it leaves~~ — done 2026-09-18 + +Shipped as **Ending a session is two outcomes** in `docs/architecture.md`, with +`gitx.RemoveWorktree` and `gitx.DeleteBranch` underneath it. + +`x` opens a modal holding both outcomes rather than growing a second key. Close +is the default because it is the reversible one. The Delete row carries +`gitx.Diff` against `BaseRef`, so the confirmation states a fact instead of a +warning. + +All four decisions this entry was waiting on held once they met real git, and +one of them turned out wider than written. `git worktree remove` refuses a tree +holding **untracked** files, not only modified ones, so an agent that wrote a +file and never added it is enough to stop a delete. That is the right refusal, +and it is a second way to reach it that this entry did not name. + +Deliberately still not this: `x` on the projects column meaning "remove +project". What happens to that project's sessions and their worktrees deserves +its own answer, not one reached in passing inside a session delete. ## ~~12. A public-repo notice a cloner will meet~~ — done 2026-08-28 diff --git a/internal/ui/model.go b/internal/ui/model.go index 313bafa..bb9a62e 100644 --- a/internal/ui/model.go +++ b/internal/ui/model.go @@ -55,8 +55,8 @@ type Model struct { picker *picker showHelp bool - // pickerSubject is the session the open picker is about — today the one a - // connect is linking from. + // pickerSubject is the session the open picker is about: the one a connect + // is linking from, or the one the end-session modal will close or delete. // // Captured when the picker opens rather than resolved on the commit key, // for the same reason picker.restore is: the modal outlives the keystroke