From 256490f966c19c4c34ed43ba16c49fd4cf2a54af Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Fri, 6 Nov 2026 15:15:39 -0600 Subject: [PATCH 1/2] Fix: Carry a saved session under the name tuicr gives it tuicr finds a saved session by the name that it gives the session, so a carried session must keep that name. The carry renamed the session to a name that only timber used, so tuicr did not find the carried session and wrote a second session for the same range. The review then found two sessions for one range and stopped. The carry keeps the repository part of the name, names the range from the oldest commit to the newest, and stores the range and the base commit the way tuicr stores them. A range of more than one commit carries now, because the captured range is compared as a set of commits. Change-Id: Icfe9e542dce76fcaec12d12c00ab2a051554db18 --- internal/timber/review/carry.go | 80 ++++++------ internal/timber/review/carry_test.go | 187 ++++++++++++++++++++++++++- internal/timber/review/predicates.go | 5 - 3 files changed, 227 insertions(+), 45 deletions(-) diff --git a/internal/timber/review/carry.go b/internal/timber/review/carry.go index 86a0bca..4567fc6 100644 --- a/internal/timber/review/carry.go +++ b/internal/timber/review/carry.go @@ -25,13 +25,10 @@ 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 @@ -39,15 +36,6 @@ func anchorFor(ctx context.Context, repository Repository, checkout string, stac 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,10 +113,11 @@ 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 } @@ -136,10 +125,11 @@ func carryTargetFor(ctx context.Context, repository Repository, checkout string, 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 diff --git a/internal/timber/review/carry_test.go b/internal/timber/review/carry_test.go index 58c47d9..03e81d8 100644 --- a/internal/timber/review/carry_test.go +++ b/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,7 +37,12 @@ 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) @@ -42,6 +50,29 @@ func carryFixture(t *testing.T, reviewer Reviewer, view Snapshot, store string) })) } +// 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)) } diff --git a/internal/timber/review/predicates.go b/internal/timber/review/predicates.go index c5fb6bb..2be6efb 100644 --- a/internal/timber/review/predicates.go +++ b/internal/timber/review/predicates.go @@ -42,8 +42,3 @@ func isSymlink(info os.FileInfo) bool { func isEmpty(s string) bool { return s == "" } - -// isNotEmpty reports whether s holds at least one character. -func isNotEmpty(s string) bool { - return s != "" -} From f4c88f9118ed7182629a59591f069237635abee1 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Fri, 6 Nov 2026 15:16:07 -0600 Subject: [PATCH 2/2] Fix: Continue a review with the last recorded session A store can hold two sessions for one range: a carried session and the session that tuicr wrote. The reader refused to choose between them, so a review stopped and every retry stopped the same way. The reader continues with the session that the index recorded last, because that is the session that tuicr reopens. A selector still names an exact session. The file review store follows the same rule. Change-Id: I27b447cc8186590e99f54e9dc4a98cefe531cad9 --- internal/timber/review/files.go | 16 +++---- internal/timber/review/files_test.go | 14 +++--- internal/timber/review/saved.go | 64 +++++++++++++++++++++++++--- internal/timber/review/saved_test.go | 17 +++++--- 4 files changed, 82 insertions(+), 29 deletions(-) diff --git a/internal/timber/review/files.go b/internal/timber/review/files.go index 84d1b6d..512962e 100644 --- a/internal/timber/review/files.go +++ b/internal/timber/review/files.go @@ -354,25 +354,23 @@ func readFileStore(store, repo, diffSource, selector string) (*SavedReview, stri if err != nil { return nil, "", err } - candidates := map[string]*SavedReview{} + candidates := []sessionCandidate{} for slug, rows := range entries { for _, entry := range rows { - saved, sessionPath, err := readFileEntry(store, repo, slug, entry, diffSource, selector) + saved, _, err := readFileEntry(store, repo, slug, entry, diffSource, selector) if err != nil { return nil, "", err } if saved != nil { - candidates[sessionPath] = saved + candidates = append(candidates, sessionCandidate{saved: saved, updatedAt: entryUpdatedAt(entry)}) } } } - if len(candidates) > 1 { - return nil, "", fmt.Errorf("ambiguous saved reviews; select an exact session with --session") - } - for sessionPath, saved := range candidates { - return saved, sessionPath, nil + if len(candidates) == 0 { + return nil, "", nil } - return nil, "", nil + newest := newestCandidate(candidates) + return newest, newest.Path, nil } func readFileEntry(store, repo, slug string, entry object, diffSource, selector string) (*SavedReview, string, error) { diff --git a/internal/timber/review/files_test.go b/internal/timber/review/files_test.go index 9a8f813..8bcb70a 100644 --- a/internal/timber/review/files_test.go +++ b/internal/timber/review/files_test.go @@ -90,7 +90,7 @@ func TestReadFileOutcomes(t *testing.T) { } } -func TestReadFileAmbiguousRequiresSession(t *testing.T) { +func TestReadFileChoosesTheLastRecordedSession(t *testing.T) { r := testRepo(t) gitTest(t, r.Dir, "commit", "--allow-empty", "-m", "Feature") repo, err := r.git(t.Context(), "rev-parse", "--show-toplevel") @@ -106,12 +106,14 @@ func TestReadFileAmbiguousRequiresSession(t *testing.T) { } writeJSON(t, filepath.Join(store, "sessions/fedcba9876543210.json"), session) writeJSON(t, filepath.Join(store, "index.json"), map[string]any{"version": "2.0", "entries": map[string]any{"fixture": []any{ - map[string]any{"kind": map[string]any{"type": "local"}, "canonical_repo_path": repo, "path": "sessions/0123456789abcdef.json"}, - map[string]any{"kind": map[string]any{"type": "local"}, "canonical_repo_path": repo, "path": "sessions/fedcba9876543210.json"}, + map[string]any{"kind": map[string]any{"type": "local"}, "canonical_repo_path": repo, "path": "sessions/0123456789abcdef.json", "updated_at": "2026-01-01T00:00:00Z"}, + map[string]any{"kind": map[string]any{"type": "local"}, "canonical_repo_path": repo, "path": "sessions/fedcba9876543210.json", "updated_at": "2026-02-02T00:00:00Z"}, }}}) - _, _, err = (SavedReader{Stores: []string{store}}).ReadFile(repo, "working_tree", "") - require.ErrorContains(t, err, "ambiguous") - saved, _, err := (SavedReader{Stores: []string{store}}).ReadFile(repo, "working_tree", "second-session") + saved, _, err := (SavedReader{Stores: []string{store}}).ReadFile(repo, "working_tree", "") + require.NoError(t, err) + require.NotNil(t, saved) + require.Equal(t, "second-session", saved.ID, "the last recorded session continues the review") + saved, _, err = (SavedReader{Stores: []string{store}}).ReadFile(repo, "working_tree", "second-session") require.NoError(t, err) require.NotNil(t, saved) require.Equal(t, "second-session", saved.ID) diff --git a/internal/timber/review/saved.go b/internal/timber/review/saved.go index 32898b2..92c342b 100644 --- a/internal/timber/review/saved.go +++ b/internal/timber/review/saved.go @@ -12,6 +12,7 @@ import ( "runtime" "slices" "strings" + "time" ) // SavedReview contains validated progress and all annotation scopes. @@ -636,7 +637,7 @@ func readStore(store string, scope Stack, selector string) (*SavedReview, error) if err != nil { return nil, err } - candidates := map[string]*SavedReview{} + candidates := []sessionCandidate{} for slug, rows := range entries { for _, entry := range rows { saved, err := readEntry(store, repo, slug, entry, scope, selector) @@ -644,17 +645,66 @@ func readStore(store string, scope Stack, selector string) (*SavedReview, error) return nil, err } if saved != nil { - candidates[saved.Path] = saved + candidates = append(candidates, sessionCandidate{saved: saved, updatedAt: entryUpdatedAt(entry)}) } } } - if len(candidates) > 1 { - return nil, fmt.Errorf("ambiguous saved reviews; select an exact session with --session") + if len(candidates) == 0 { + return nil, nil } - for _, saved := range candidates { - return saved, nil + return newestCandidate(candidates), nil +} + +// sessionCandidate is a saved session that describes a scope, with the time +// that the index recorded for it. +type sessionCandidate struct { + saved *SavedReview + updatedAt time.Time +} + +// newestCandidate returns the session that the index recorded last. Several +// sessions can describe one range: a carried session and the session that +// tuicr wrote name the same commits. The last one is the session that tuicr +// reopens, so it is the session that a review continues. +func newestCandidate(candidates []sessionCandidate) *SavedReview { + newest := candidates[0] + for _, candidate := range candidates[1:] { + if isLater(candidate, newest) { + newest = candidate + } } - return nil, nil + return newest.saved +} + +// isLater reports whether a candidate is later than another one. The path +// breaks a tie, so that every run makes the same choice. +func isLater(candidate, other sessionCandidate) bool { + if candidate.updatedAt.After(other.updatedAt) { + return true + } + return isTied(candidate, other) && isLess(other.saved.Path, candidate.saved.Path) +} + +// isTied reports whether two candidates were recorded at the same time. +func isTied(candidate, other sessionCandidate) bool { + return isSame(candidate.updatedAt, other.updatedAt) +} + +// entryUpdatedAt reports the time that the index recorded for a session. A +// session without a time sorts as the oldest one. +func entryUpdatedAt(entry object) time.Time { + if _, ok := entry["updated_at"]; !ok { + return time.Time{} + } + value, err := decodeRequired[string](entry, "updated_at") + if err != nil { + return time.Time{} + } + updated, err := time.Parse(time.RFC3339, value) + if err != nil { + return time.Time{} + } + return updated } func readEntry(store, repo, slug string, entry object, scope Stack, selector string) (*SavedReview, error) { diff --git a/internal/timber/review/saved_test.go b/internal/timber/review/saved_test.go index 0f15100..28d1297 100644 --- a/internal/timber/review/saved_test.go +++ b/internal/timber/review/saved_test.go @@ -143,20 +143,23 @@ func TestSavedReviewScopeAndDiscovery(t *testing.T) { _, err = reader.Read(view.Stacks[0], "") require.Error(t, err) - // Slugs are discovery metadata; two matching full sessions require selection. + // Several sessions can describe one range. The one that the index recorded + // last is the session that a review continues. session = savedFixture(t, view.Stacks[0], store, true) session["id"] = "second-session" writeJSON(t, filepath.Join(store, "sessions/fedcba9876543210.json"), session) writeJSON(t, filepath.Join(store, "index.json"), map[string]any{"version": "2.0", "entries": map[string]any{"fixture": []any{ - map[string]any{"kind": map[string]any{"type": "local"}, "canonical_repo_path": r.Dir, "path": "sessions/0123456789abcdef.json"}, - map[string]any{"kind": map[string]any{"type": "local"}, "canonical_repo_path": r.Dir, "path": "sessions/fedcba9876543210.json"}, + map[string]any{"kind": map[string]any{"type": "local"}, "canonical_repo_path": r.Dir, "path": "sessions/0123456789abcdef.json", "updated_at": "2026-01-01T00:00:00Z"}, + map[string]any{"kind": map[string]any{"type": "local"}, "canonical_repo_path": r.Dir, "path": "sessions/fedcba9876543210.json", "updated_at": "2026-02-02T00:00:00Z"}, }}}) - _, err = reader.Read(view.Stacks[0], "") - require.ErrorContains(t, err, "ambiguous") - saved, err = reader.Read(view.Stacks[0], "second-session") + saved, err = reader.Read(view.Stacks[0], "") + require.NoError(t, err) + require.NotNil(t, saved) + require.Equal(t, "second-session", saved.ID, "the last recorded session continues the review") + saved, err = reader.Read(view.Stacks[0], "fixture-session") require.NoError(t, err) require.NotNil(t, saved) - require.Equal(t, "second-session", saved.ID) + require.Equal(t, "fixture-session", saved.ID, "a selector names an exact session") } func savedSession(t *testing.T, store string, stack Stack, name string) string {