Fix continuing a review with the previous session

internal/timber/review/carry.go

@@ -25,29 +25,17 @@ type carriedSession struct {
 	document object
 }
 
-// anchorFor reports the branch name of the review checkout, or its abbreviated
-// commit when the checkout is detached.
-func anchorFor(ctx context.Context, repository Repository, checkout string, stack Stack) (string, error) {
-	if branch, err := branchName(ctx, repository, checkout); err == nil && isNotEmpty(branch) {
-		return branch, nil
-	}
-	// A review checkout is detached at the commit that the review shows.
+// anchorFor reports the abbreviated commit that anchors a session name. The
+// review checkout is detached at the tip when tuicr opens a range, so tuicr
+// names the session after the tip and not after a branch.
+func anchorFor(ctx context.Context, repository Repository, stack Stack) (string, error) {
 	short, err := shortSHA(ctx, repository, stack.Tip)
 	if err != nil {
 		return "", err
 	}
 	return "~" + short, nil
 }
 
-// branchName reports the branch that the review checkout has checked out.
-func branchName(ctx context.Context, repository Repository, checkout string) (string, error) {
-	if _, err := os.Lstat(checkout); err != nil {
-		return "", err
-	}
-	repository.Dir = checkout
-	return repository.git(ctx, "symbolic-ref", "--quiet", "--short", "HEAD")
-}
-
 // carrySavedReviews moves the saved session of a reviewed patch onto the stack
 // that a rewrite moved it to. A rewrite that keeps the patch keeps every file,
 // hunk, and comment that the session describes, so tuicr reopens the review at
@@ -98,7 +86,7 @@ func carrySavedSession(ctx context.Context, repository Repository, reader SavedR
 		// A session already describes the current range.
 		return nil
 	}
-	target, err := carryTargetFor(ctx, repository, checkout, to)
+	target, err := carryTargetFor(ctx, repository, to, projectOf(source.slug))
 	if err != nil {
 		return err
 	}
@@ -125,21 +113,23 @@ func carrySource(reader SavedReader, from Attempt) (*carriedSession, error) {
 	return &carriedSession{store: saved.Store, path: saved.Path, slug: saved.Slug, document: document}, nil
 }
 
-// carryTarget names the session and the message key that the current stack
-// needs.
-func carryTargetFor(ctx context.Context, repository Repository, checkout string, to Stack) (carryTarget, error) {
-	anchor, err := anchorFor(ctx, repository, checkout, to)
+// carryTargetFor names the session and the message key that the current stack
+// needs. The name must be the name that tuicr gives the session, because
+// tuicr finds a session by that name.
+func carryTargetFor(ctx context.Context, repository Repository, to Stack, project string) (carryTarget, error) {
+	anchor, err := anchorFor(ctx, repository, to)
 	if err != nil {
 		return carryTarget{}, err
 	}
 	short, err := shortSHA(ctx, repository, to.Tip)
 	if err != nil {
 		return carryTarget{}, err
 	}
-	slug, err := sessionSlug(ctx, repository, checkout, to, anchor)
+	first, err := shortSHA(ctx, repository, to.Commits[0])
 	if err != nil {
 		return carryTarget{}, err
 	}
+	slug := sessionSlug(project, anchor, first, short)
 	return carryTarget{anchor: anchor, short: short, slug: slug, now: time.Now().UTC().Format(time.RFC3339Nano)}, nil
 }
 
@@ -313,16 +303,21 @@ func renameCommitMessageFile(files object, short string, keepMessage bool) error
 
 // rewriteCarriedSession points a session at the commits that the review shows
 // now. Marks, comments, and content hashes stay because the patch is unchanged.
+// The range and the base commit follow the layout that tuicr stores them in,
+// because tuicr derives the name of the session from them.
 func rewriteCarriedSession(document object, to Stack, short string, keepMessage bool, now string) error {
 	document["id"] = quoted(uuid.New().String())
 	document["created_at"] = quoted(now)
 	document["updated_at"] = quoted(now)
-	commits, err := json.Marshal(to.Commits)
+	commits, err := json.Marshal(newestFirst(to.Commits))
 	if err != nil {
 		return err
 	}
 	document["commit_range"] = commits
-	document["base_commit"] = quoted(to.Commits[0])
+	document["base_commit"] = quoted(to.Tip)
+	// The review checkout is detached when tuicr opens the range, so the
+	// session names the tip and not a branch.
+	document["branch_name"] = jsontext.Value("null")
 	files, err := decodeRequired[object](document, "files")
 	if err != nil {
 		return err
@@ -338,6 +333,14 @@ func rewriteCarriedSession(document object, to Stack, short string, keepMessage
 	return nil
 }
 
+// newestFirst reports the commits from the newest to the oldest. tuicr stores
+// a commit range in that order.
+func newestFirst(commits []string) []string {
+	ordered := slices.Clone(commits)
+	slices.Reverse(ordered)
+	return ordered
+}
+
 // countSessionReview counts the reviewed files and the comments of a session.
 func countSessionReview(files object) (int, int) {
 	reviewed, comments := 0, 0
@@ -385,17 +388,22 @@ func sessionDisplay(document object, anchor string) object {
 }
 
 // sessionSlug names the session that tuicr keeps for a stack in its review
-// checkout.
-func sessionSlug(ctx context.Context, repository Repository, checkout string, stack Stack, anchor string) (string, error) {
-	first, err := shortSHA(ctx, repository, stack.Commits[0])
-	if err != nil {
-		return "", err
-	}
-	tip, err := shortSHA(ctx, repository, stack.Tip)
-	if err != nil {
-		return "", err
-	}
-	return fmt.Sprintf("%s@%s/commits/%s..%s", filepath.Base(checkout), anchor, first, tip), nil
+// checkout. tuicr finds a session by this name, so the name holds the
+// repository that the origin remote names, the anchor of the checkout, and
+// the range that the session shows.
+func sessionSlug(project, anchor, first, last string) string {
+	return fmt.Sprintf("%s@%s/commits/%s..%s", project, anchor, first, last)
+}
+
+// projectOf reports the repository part of a session name. The part names the
+// repository that the origin remote names, and it is the same for every
+// session of that repository.
+func projectOf(slug string) string {
+	project, _, found := strings.Cut(slug, "@")
+	if !found {
+		return slug
+	}
+	return project
 }
 
 // shortSHA reports the abbreviation that git prints for a revision.
@@ -418,7 +426,7 @@ func validateCarriedSession(document object, from Stack) error {
 	if err != nil {
 		return err
 	}
-	if isFalse(slices.Equal(commits, from.Commits)) {
+	if isFalse(sameCommitSet(commits, from.Commits)) {
 		return fmt.Errorf("saved review captured a different commit range")
 	}
 	return nil

internal/timber/review/carry_test.go

@@ -1,16 +1,19 @@
 package review
 
 import (
+	"encoding/json/jsontext"
 	"encoding/json/v2"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 )
 
 // carryFixture writes a saved session for the current stack with one of its two
-// files reviewed and one line comment.
+// files reviewed and one line comment. The session and the name that the
+// store holds follow the layout that tuicr writes.
 func carryFixture(t *testing.T, reviewer Reviewer, view Snapshot, store string) {
 	t.Helper()
 	scope := view.Stacks[0]
@@ -34,14 +37,42 @@ func carryFixture(t *testing.T, reviewer Reviewer, view Snapshot, store string)
 		"id": "comment", "content": "Handle the empty case", "comment_type": "issue",
 		"created_at": "2026-01-01T00:00:00Z", "author": "user", "lifecycle_state": "local_draft",
 	}}}
+	// tuicr stores the range from the newest commit to the oldest, and it
+	// names the session after the repository, the anchor, and the range.
+	session["commit_range"] = newestFirst(scope.Commits)
+	session["base_commit"] = scope.Tip
 	writeJSON(t, filepath.Join(store, "sessions/0123456789abcdef.json"), session)
+	renameIndexKey(t, store, "fixture", tuicrName(t, reviewer.Service.Repository, scope))
 	require.NoError(t, (Store{Path: view.Path}).Update(func(state *State) error {
 		attempt := newAttempt(view.Stacks[0], "incomplete")
 		state.Attempts = append(state.Attempts, attempt)
 		return nil
 	}))
 }
 
+// tuicrName reports the name that tuicr gives a session of a range.
+func tuicrName(t *testing.T, repository Repository, scope Stack) string {
+	t.Helper()
+	first, err := shortSHA(t.Context(), repository, scope.Commits[0])
+	require.NoError(t, err)
+	tip, err := shortSHA(t.Context(), repository, scope.Tip)
+	require.NoError(t, err)
+	return "nnutter/timber@~" + tip + "/commits/" + first + ".." + tip
+}
+
+// renameIndexKey names a session in the store under a new name.
+func renameIndexKey(t *testing.T, store, from, to string) {
+	t.Helper()
+	index, entries, err := loadIndex(store)
+	require.NoError(t, err)
+	require.NotNil(t, index)
+	rows, ok := entries[from]
+	require.True(t, ok, "the store names the session")
+	delete(entries, from)
+	entries[to] = rows
+	require.NoError(t, writeIndex(store, index, entries))
+}
+
 func carryRepository(t *testing.T) (Reviewer, Snapshot) {
 	t.Helper()
 	reviewer, _, _ := testReviewer(t)
@@ -115,8 +146,9 @@ func TestCarrySavedReviewKeepsMarksAcrossRewrite(t *testing.T) {
 			index, entries, err := loadIndex(store)
 			require.NoError(t, err)
 			require.NotNil(t, index)
-			require.NotContains(t, entries, "fixture")
 			require.Len(t, entries, 1)
+			require.Contains(t, entries, tuicrName(t, reviewer.Service.Repository, carried.Stacks[0]),
+				"the session keeps the name that tuicr gives it")
 		})
 	}
 }
@@ -138,7 +170,7 @@ func TestCarrySavedReviewLeavesAMatchingRangeAlone(t *testing.T) {
 	raw, err := os.ReadFile(filepath.Join(store, "index.json"))
 	require.NoError(t, err)
 	require.NoError(t, json.Unmarshal(raw, &indexed))
-	require.Contains(t, indexed["entries"], "fixture")
+	require.Contains(t, indexed["entries"], tuicrName(t, reviewer.Service.Repository, view.Stacks[0]))
 }
 
 // A review that a rewrite moved records the carried progress, so the result and
@@ -173,11 +205,158 @@ func TestCarrySavedReviewThroughAliasedStore(t *testing.T) {
 	reviewer.Reader = SavedReader{Stores: []string{alias}}
 	carryFixture(t, reviewer, view, store)
 	gitTest(t, reviewer.Service.Repository.Dir, "commit", "--amend", "--allow-empty", "--date", "2026-01-01T00:00:00Z", "-m", "Feature reworded")
+	carried, err := reviewer.Service.Repository.Snapshot(t.Context())
+	require.NoError(t, err)
 
 	carrySavedReviews(t.Context(), reviewer.Service.Repository, reviewer.Reader)
 
 	_, entries, err := loadIndex(alias)
 	require.NoError(t, err)
-	require.NotContains(t, entries, "fixture")
 	require.Len(t, entries, 1)
+	require.Contains(t, entries, tuicrName(t, reviewer.Service.Repository, carried.Stacks[0]))
+}
+
+// tuicr derives the name of a session from the range and the base commit that
+// the session stores. A carried session must store them the way that tuicr
+// stores them, or tuicr names the session under another name.
+func TestCarrySavedReviewStoresTheRangeTheWayTuicrStoresIt(t *testing.T) {
+	reviewer, view := carryRepository(t)
+	store := t.TempDir()
+	reviewer.Reader = SavedReader{Stores: []string{store}}
+	carryFixture(t, reviewer, view, store)
+	gitTest(t, reviewer.Service.Repository.Dir, "commit", "--amend", "--allow-empty", "--date", "2026-01-01T00:00:00Z", "-m", "Feature reworded")
+	carried, err := reviewer.Service.Repository.Snapshot(t.Context())
+	require.NoError(t, err)
+
+	carrySavedReviews(t.Context(), reviewer.Service.Repository, reviewer.Reader)
+
+	scope := carried.Stacks[0]
+	checkout, err := WorktreePath(scope.Repo)
+	require.NoError(t, err)
+	scope.Repo = checkout
+	saved, err := (SavedReader{Stores: []string{store}}).Read(scope, "")
+	require.NoError(t, err)
+	require.NotNil(t, saved)
+
+	document, err := readObject(saved.Path)
+	require.NoError(t, err)
+	commits, err := decodeRequired[[]string](document, "commit_range")
+	require.NoError(t, err)
+	require.Equal(t, newestFirst(scope.Commits), commits, "tuicr stores the range from the newest commit")
+	base, err := decodeRequired[string](document, "base_commit")
+	require.NoError(t, err)
+	require.Equal(t, scope.Tip, base, "the base commit is the tip of the range")
+}
+
+// A range of more than one commit carries, because tuicr stores the range
+// from the newest commit to the oldest.
+func TestCarrySavedReviewCarriesARangeOfSeveralCommits(t *testing.T) {
+	reviewer, _, _ := testReviewer(t)
+	dir := reviewer.Service.Repository.Dir
+	gitTest(t, dir, "reset", "--hard", "main")
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "first.go"), []byte("package main\n"), 0o600))
+	gitTest(t, dir, "add", ".")
+	gitTest(t, dir, "commit", "-m", "Add feature\n\nChange-Id: I"+strings.Repeat("a", 40))
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "second.go"), []byte("package main\n"), 0o600))
+	gitTest(t, dir, "add", ".")
+	gitTest(t, dir, "commit", "-m", "fixup! Add feature")
+	view, err := reviewer.Service.Repository.Snapshot(t.Context())
+	require.NoError(t, err)
+	require.Len(t, view.Stacks, 1)
+	require.Len(t, view.Stacks[0].Commits, 2)
+
+	store := t.TempDir()
+	reviewer.Reader = SavedReader{Stores: []string{store}}
+	carryFixture(t, reviewer, view, store)
+	gitTest(t, dir, "commit", "--amend", "--no-edit", "--date", "2026-01-01T00:00:00Z")
+	carried, err := reviewer.Service.Repository.Snapshot(t.Context())
+	require.NoError(t, err)
+
+	carrySavedReviews(t.Context(), reviewer.Service.Repository, reviewer.Reader)
+
+	scope := carried.Stacks[0]
+	checkout, err := WorktreePath(scope.Repo)
+	require.NoError(t, err)
+	scope.Repo = checkout
+	saved, err := (SavedReader{Stores: []string{store}}).Read(scope, "")
+	require.NoError(t, err)
+	require.NotNil(t, saved, "the review moved onto the rewritten commits")
+	require.Equal(t, 2, saved.Reviewed)
+
+	_, entries, err := loadIndex(store)
+	require.NoError(t, err)
+	require.Contains(t, entries, tuicrName(t, reviewer.Service.Repository, carried.Stacks[0]))
+}
+
+// A squashed commit rewrites the range of a review. The carried session must
+// keep the name that tuicr gives a session, because tuicr finds a session by
+// that name and writes a second session for a range that it cannot find.
+func TestReviewContinuesAfterASquashedCommit(t *testing.T) {
+	reviewer, view := carryRepository(t)
+	store := t.TempDir()
+	reviewer.Reader = SavedReader{Stores: []string{store}}
+	carryFixture(t, reviewer, view, store)
+	gitTest(t, reviewer.Service.Repository.Dir, "commit", "--amend", "--allow-empty", "--date", "2026-01-01T00:00:00Z", "-m", "Feature reworded")
+	carried, err := reviewer.Service.Repository.Snapshot(t.Context())
+	require.NoError(t, err)
+
+	carrySavedReviews(t.Context(), reviewer.Service.Repository, reviewer.Reader)
+
+	_, entries, err := loadIndex(store)
+	require.NoError(t, err)
+	require.Len(t, entries, 1, "the store names one session for the range")
+	require.Contains(t, entries, tuicrName(t, reviewer.Service.Repository, carried.Stacks[0]),
+		"tuicr finds the carried session under its own name")
+}
+
+// A store can hold two sessions for one range: a carried session and the
+// session that tuicr wrote. The review continues with the session that the
+// index recorded last, because that is the session that tuicr reopens.
+func TestReviewContinuesWithTheLastRecordedSession(t *testing.T) {
+	reviewer, view := carryRepository(t)
+	store := t.TempDir()
+	reviewer.Reader = SavedReader{Stores: []string{store}}
+	carryFixture(t, reviewer, view, store)
+	addSecondSession(t, store, view, "2026-02-02T00:00:00Z")
+	fakeTuicr(t, "exit 0")
+
+	result, err := reviewer.Review(t.Context(), ReviewOptions{})
+	require.NoError(t, err, "the review does not stop on a range with two sessions")
+	require.Equal(t, "range_complete", result.Status)
+	state, err := (Store{Path: view.Path}).Load()
+	require.NoError(t, err)
+	recorded := state.Attempts[len(state.Attempts)-1]
+	var saved SavedReview
+	require.NoError(t, json.Unmarshal(recorded.Review, &saved))
+	require.Equal(t, "second-session", saved.ID, "the last recorded session continues the review")
+}
+
+// addSecondSession writes a session for the same range under a later time.
+func addSecondSession(t *testing.T, store string, view Snapshot, updatedAt string) {
+	t.Helper()
+	scope := view.Stacks[0]
+	checkout, err := WorktreePath(scope.Repo)
+	require.NoError(t, err)
+	session := map[string]any{
+		"version": "1.3", "id": "second-session", "repo_path": checkout,
+		"base_commit": scope.Tip, "diff_source": "commit_range", "commit_range": newestFirst(scope.Commits),
+		"files": map[string]any{
+			"main.go": map[string]any{"path": "main.go", "reviewed": true, "file_comments": []any{}, "line_comments": map[string]any{}},
+		},
+	}
+	writeJSON(t, filepath.Join(store, "sessions/fedcba9876543210.json"), session)
+	index, entries, err := loadIndex(store)
+	require.NoError(t, err)
+	require.NotNil(t, index)
+	entry := object{
+		"kind":                jsontext.Value(`{"type": "local"}`),
+		"canonical_repo_path": jsontext.Value(`"` + checkout + `"`),
+		"path":                jsontext.Value(`"sessions/fedcba9876543210.json"`),
+		"updated_at":          jsontext.Value(`"` + updatedAt + `"`),
+		"display":             jsontext.Value(`{"reviewed_count": 1, "comment_count": 0}`),
+	}
+	for slug, rows := range entries {
+		entries[slug] = append(rows, entry)
+	}
+	require.NoError(t, writeIndex(store, index, entries))
 }