tasks: a stopped run names the work its worker already committed

docs/changes/unreleased/1710-stopped-run-committed-work.md

@@ -0,0 +1,8 @@
+---
+kind: fixed
+title: a stopped run names the work its worker already committed
+pr: 1710
+surface: [chat, engine, docs]
+invalidates:
+  - "Stopping a run whose worker had already committed its work said `stopped · it had changed nothing` and named no branch, although the branch held the commits. The stop receipt and row now count work committed on the task branch since its base as well as uncommitted edits, and name the branch; a run that truly changed nothing still says so."
+---

internal/manual/chat/worker-harness.md

@@ -651,6 +651,11 @@ conversation and the run's page say where the work is and what to do with it:
 stopped · its work so far is kept on <branch> and did not go into <folder> · merge that branch to bring it in, or delete it to drop it
 ```
 
+**Work the worker already committed counts too.** The stopped row lists files
+changed since the run began, together with any pending edits committed as it
+stops. A clean working copy does not mean the run changed nothing: its existing
+commits stay on the named branch.
+
 A run stopped before it changed anything says `stopped · it had changed nothing` and
 names no branch. The next `/task` after a stop starts a fresh run; it never picks the
 stopped work back up, and the stopped run stays readable from the side list.

internal/session/land_run_tree.go

@@ -59,24 +59,10 @@ func LandRunTree(dir, base, title, model string) (branch string, changed []strin
 	}
 	headMoved := false
 	if base != "" {
-		out, err := git(dir, "diff", "--name-only", "-z", base, "HEAD")
+		changed, headMoved, err = runCommittedPaths(dir, base)
 		if err != nil {
 			return "", nil, "", err
 		}
-		changed = append(changed, gitNULPaths(out)...)
-		head := runTreeHead(dir)
-		if head == "" {
-			return "", nil, "", errors.New("read the run's branch head after landing")
-		}
-		headMoved = head != base
-		if headMoved && len(changed) == 0 {
-			// The net path list measures this tree since base, not the
-			// rewrite's private range; include an outside merged branch too.
-			changed, err = runTouchedPaths(dir, base, head)
-			if err != nil {
-				return "", nil, "", err
-			}
-		}
 	} else {
 		changed = saved
 	}
@@ -89,6 +75,29 @@ func LandRunTree(dir, base, title, model string) (branch string, changed []strin
 	return branch, changed, "", nil
 }
 
+// runCommittedPaths reads a run's work from its original base, because a
+// worker's own commits no longer appear in the working copy's status. A stop
+// and a landing owe the same account of those files, including commits whose
+// edits later cancelled themselves out.
+func runCommittedPaths(dir, base string) (changed []string, headMoved bool, err error) {
+	out, err := git(dir, "diff", "--name-only", "-z", base, "HEAD")
+	if err != nil {
+		return nil, false, err
+	}
+	changed = gitNULPaths(out)
+	head := runTreeHead(dir)
+	if head == "" {
+		return nil, false, errors.New("read the run's branch head")
+	}
+	headMoved = head != base
+	if headMoved && len(changed) == 0 {
+		// The net path list measures this tree since base, not the
+		// rewrite's private range; include an outside merged branch too.
+		changed, err = runTouchedPaths(dir, base, head)
+	}
+	return changed, headMoved, err
+}
+
 // runTouchedPaths names files in non-merge commits when committed work later
 // cancels itself out. It is sorted and unique, while an empty-commit run still
 // lands on its branch with a count of zero files.

internal/session/stopped_run_commits_test.go

@@ -0,0 +1,107 @@
+package session
+
+import (
+	"os"
+	"path/filepath"
+	"reflect"
+	"sort"
+	"strings"
+	"testing"
+)
+
+// A worker's own commits are clean in its index when the person stops it.
+// The stop still owes the person the branch holding those files, rather than
+// sending them away with a sentence saying the run had changed nothing.
+func TestAStoppedRunNamesWorkTheWorkerAlreadyCommitted(t *testing.T) {
+	for _, pending := range []bool{false, true} {
+		name := "committed"
+		if pending {
+			name = "committed_and_pending"
+		}
+		t.Run(name, func(t *testing.T) {
+			agent, double, conversation, dir := stoppableBeltRun(t, 71)
+			double.mu.Lock()
+			workspace := double.spec.Workspace
+			double.mu.Unlock()
+			mustGit(t, workspace, "add", "half.txt")
+			mustGit(t, workspace, "-c", "user.name=Worker", "-c", "[email protected]", "commit", "-m", "first FAQ item")
+			want := []string{"half.txt"}
+			if pending {
+				runCommitFile(t, workspace, "committed.txt", "another FAQ item")
+				for path, body := range map[string]string{"half.txt": "half made\nmore work\n", "pending.txt": "not yet committed\n"} {
+					if err := os.WriteFile(filepath.Join(workspace, path), []byte(body), 0o644); err != nil {
+						t.Fatal(err)
+					}
+				}
+				want = []string{"committed.txt", "half.txt", "pending.txt"}
+			}
+			before := gitOut(t, conversation, "rev-parse", "HEAD")
+			if _, err := agent.Cancel(CancelTask + ":71"); err != nil {
+				t.Fatal(err)
+			}
+			beltRunWaitFor(t, "the stopped run to settle", func() bool {
+				agent.beltMu.Lock()
+				defer agent.beltMu.Unlock()
+				return agent.beltRun == nil
+			})
+			rows := agent.graph().runRows(71)
+			if len(rows) != 1 || rows[0].Branch == "" || !rows[0].Stopped || rows[0].Merge != mergeAborted {
+				t.Fatalf("a stopped run with committed work names no kept branch: %+v", rows)
+			}
+			changed := append([]string(nil), rows[0].Changed...)
+			sort.Strings(changed)
+			if !reflect.DeepEqual(changed, want) {
+				t.Fatalf("the stopped row lists %v, want each committed and pending file once: %v", changed, want)
+			}
+			where := "stopped · its work so far is kept on " + rows[0].Branch + " and did not go into " + canonicalPath(conversation) +
+				" · merge that branch to bring it in, or delete it to drop it"
+			if said := strings.Join(beltRunNotes(t, dir, "71"), "\n"); !strings.Contains(said, where) || strings.Contains(said, "changed nothing") {
+				t.Fatalf("the stopped page does not name the worker's committed work:\n%s", said)
+			}
+			if got := conversationNotes(agent, where); got != 1 {
+				t.Fatalf("the stop receipt names the committed work %d times, want once", got)
+			}
+			for _, path := range want {
+				if out, err := git(conversation, "show", rows[0].Branch+":"+path); err != nil || out == "" {
+					t.Fatalf("the kept branch does not hold %s: %q, %v", path, out, err)
+				}
+				if _, err := os.Stat(filepath.Join(conversation, path)); !os.IsNotExist(err) {
+					t.Fatalf("the stopped work appeared in the person's checkout: %s, %v", path, err)
+				}
+			}
+			if after := gitOut(t, conversation, "rev-parse", "HEAD"); after != before {
+				t.Fatalf("stopping moved the person's checkout from %s to %s", before, after)
+			}
+		})
+	}
+}
+
+// A stop uses the same history rule as a landing: commits that cancelled
+// their file changes still leave work on a branch the person can inspect.
+func TestAStoppedRunNamesCommittedThenRevertedWork(t *testing.T) {
+	agent, double, conversation, dir := stoppableBeltRun(t, 71)
+	double.mu.Lock()
+	workspace := double.spec.Workspace
+	double.mu.Unlock()
+	mustGit(t, workspace, "add", "half.txt")
+	mustGit(t, workspace, "-c", "user.name=Worker", "-c", "[email protected]", "commit", "-m", "first FAQ item")
+	mustGit(t, workspace, "-c", "user.name=Worker", "-c", "[email protected]", "revert", "--no-edit", "HEAD")
+	if _, err := agent.Cancel(CancelTask + ":71"); err != nil {
+		t.Fatal(err)
+	}
+	beltRunWaitFor(t, "the stopped run to settle", func() bool {
+		agent.beltMu.Lock()
+		defer agent.beltMu.Unlock()
+		return agent.beltRun == nil
+	})
+	rows := agent.graph().runRows(71)
+	if len(rows) != 1 || rows[0].Branch == "" || !reflect.DeepEqual(rows[0].Changed, []string{"half.txt"}) {
+		t.Fatalf("a stopped run hid its committed then reverted work: %+v", rows)
+	}
+	if said := strings.Join(beltRunNotes(t, dir, "71"), "\n"); strings.Contains(said, "changed nothing") || !strings.Contains(said, "kept on "+rows[0].Branch) {
+		t.Fatalf("the stopped page hides the retained history:\n%s", said)
+	}
+	if commits := strings.Fields(gitOut(t, conversation, "rev-list", "HEAD.."+rows[0].Branch)); len(commits) != 2 {
+		t.Fatalf("the kept branch lost the worker's commits: %v", commits)
+	}
+}

internal/session/stoprun.go

@@ -349,7 +349,19 @@ func (a *Agent) settleStoppedBeltRun(run *beltRun, why string, cut []string) {
 			merge, branch = mergeKept, run.folder.Branch
 		}
 	} else {
-		merge, changed = keptWork(run.tree, run.title, nil, a.signsGitWork())
+		// THE WORKER'S OWN COMMITS ARE WORK TOO. The index may be clean when
+		// the stop arrives, so read the branch against its original base before
+		// keeping the pending edits and giving its working copy back.
+		if run.tree.checkBase != "" {
+			var err error
+			changed, _, err = runCommittedPaths(run.workspace, run.tree.checkBase)
+			if err != nil {
+				if g := a.graph(); g != nil {
+					g.planNote("the stopped run's committed work could not be read: " + err.Error())
+				}
+			}
+		}
+		merge, changed = keptWork(run.tree, run.title, changed, a.signsGitWork())
 		if merge != mergeInPlace {
 			report += " · " + beltStoppedWhere(run.tree.branch, run.ground, changed)
 		}