From 49aa8878029b566518a1fe6b225387a014083eda Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Wed, 4 Nov 2026 23:31:54 -0600 Subject: [PATCH 1/7] Discard a review whose captured stack no longer exists An interrupted review keeps its captured stack. When a rewrite removes those commits and leaves no lineage, no current stack continues the attempt, so a review can neither reopen the saved range nor select a replacement for it. The attempt then stops every review with a recovery choice that has no working answer, and nothing ever removes it because cleanup only covered stacks that a fixup commit extended. Cleanup now drops an unfinished attempt whose captured commits no current stack continues, and the saved sessions of the stack versions it drops. The review then starts on the current stacks, and recovery is offered only for a range that a review can actually reopen. --- internal/cli/review/recovery_test.go | 49 ++++++++++++++++++++++++++++ internal/timber/review/run.go | 35 ++++++++++---------- internal/timber/review/run_test.go | 33 +++++++++++++++++++ internal/timber/review/state.go | 22 +++++++++++++ 4 files changed, 122 insertions(+), 17 deletions(-) diff --git a/internal/cli/review/recovery_test.go b/internal/cli/review/recovery_test.go index 427dcfb..3a97a29 100644 --- a/internal/cli/review/recovery_test.go +++ b/internal/cli/review/recovery_test.go @@ -2,6 +2,9 @@ package cli import ( "encoding/json/jsontext" + "encoding/json/v2" + "os" + "path/filepath" "testing" "github.com/stretchr/testify/require" @@ -54,3 +57,49 @@ func TestRecoveryDecisionExplainsSavedWork(t *testing.T) { }) } } + +// installFakeTuicr puts a scripted tuicr on PATH so a review can run without a +// terminal. +func installFakeTuicr(t *testing.T, body string) { + t.Helper() + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "tuicr"), []byte("#!/bin/sh\nset -eu\n"+body+"\n"), 0o700)) + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) +} + +// An interrupted attempt whose captured commits a rewrite removed leaves no +// range that a review can reopen or replace. The review must start on the +// current stacks instead of asking for an impossible choice. +func TestReviewDiscardsAttemptWhoseStackNoLongerExists(t *testing.T) { + dir := featureRepo(t) + t.Setenv("XDG_CACHE_HOME", t.TempDir()) + t.Setenv("XDG_DATA_HOME", t.TempDir()) + installFakeTuicr(t, "exit 0") + view, err := (review.Repository{Dir: dir}).Snapshot(t.Context()) + require.NoError(t, err) + interrupted := review.Attempt{ID: "interrupted", Stack: view.Stacks[0], Status: "reviewing", StartedAt: "2026-01-01T00:00:00Z"} + store := review.Store{Path: view.Path} + require.NoError(t, store.Update(func(state *review.State) error { + state.Attempts = append(state.Attempts, interrupted) + return nil + })) + // The rewrite keeps the patch but leaves no lineage to the captured commits. + git(t, dir, "commit", "--amend", "--allow-empty", "-m", "Add feature again") + out, _, err := execute(t, dir, "commits", "--json") + require.NoError(t, err) + var result review.Result + require.NoError(t, json.Unmarshal([]byte(out), &result)) + require.NotEqual(t, "decision_required", result.Status) + require.Nil(t, result.Decision) + state, err := store.Load() + require.NoError(t, err) + require.NotContains(t, attemptIDs(state), interrupted.ID) +} + +func attemptIDs(state review.State) []string { + ids := make([]string, 0, len(state.Attempts)) + for _, attempt := range state.Attempts { + ids = append(ids, attempt.ID) + } + return ids +} diff --git a/internal/timber/review/run.go b/internal/timber/review/run.go index 49346a3..b7b10fe 100644 --- a/internal/timber/review/run.go +++ b/internal/timber/review/run.go @@ -62,7 +62,7 @@ func (r Reviewer) Review(ctx context.Context, options ReviewOptions) (result Res } stopMaintenance := startCacheMaintenance(ctx, r.Service.Repository, lock) defer stopMaintenance() - r.discardSuperseded(ctx) + r.discardUnusable(ctx) result, err = r.reviewSequence(ctx, options, path, lock) if err != nil { return result, err @@ -173,24 +173,25 @@ func (r Reviewer) bounds(ctx context.Context, view Snapshot, first, last string) return view.Stacks[start : end+1], nil } -// discardSuperseded removes the reviews of earlier stack versions. Their saved -// sessions can no longer be resumed, and their unfinished attempts still look -// recoverable, so nothing else would ever remove them. -// Cleanup only warns: reviewing a current stack must still work. -func (r Reviewer) discardSuperseded(ctx context.Context) { +// discardUnusable removes the reviews of stack versions that a current review +// can no longer select: a stack that a fixup commit extended, and a stack that a +// rewrite removed. Their saved sessions can no longer be resumed, and their +// unfinished attempts still look recoverable, so nothing else would ever remove +// them. Cleanup only warns: reviewing a current stack must still work. +func (r Reviewer) discardUnusable(ctx context.Context) { view, err := r.Service.Repository.Snapshot(ctx) if err != nil { - r.warnSupersededCleanup(err) + r.warnCleanup(err) return } state, err := (Store{Path: view.Path}).Load() if err != nil { - r.warnSupersededCleanup(err) + r.warnCleanup(err) return } var sessions []Stack for _, attempt := range state.Attempts { - if supersededAttempt(attempt, view) { + if supersededAttempt(attempt, view) || lostAttempt(attempt, view) { sessions = append(sessions, attempt.Stack) } } @@ -199,18 +200,18 @@ func (r Reviewer) discardSuperseded(ctx context.Context) { } err = (Store{Path: view.Path}).Update(func(state *State) error { state.Attempts = slices.DeleteFunc(state.Attempts, func(attempt Attempt) bool { - return unfinished(attempt) && supersededAttempt(attempt, view) + return unusableAttempt(attempt, view) }) return nil }) if err != nil { - r.warnSupersededCleanup(err) + r.warnCleanup(err) return } writer := SavedWriter{Stores: r.Reader.Stores} for _, stack := range sessions { if _, err := writer.Delete(stack); err != nil { - r.warnSupersededCleanup(err) + r.warnCleanup(err) return } } @@ -462,8 +463,8 @@ type reviewPlan struct { exhausted bool } -func (r Reviewer) warnSupersededCleanup(cause error) { - r.Service.Repository.logger().Warn("Superseded review cleanup failed", "error", cause) +func (r Reviewer) warnCleanup(cause error) { + r.Service.Repository.logger().Warn("Review cleanup failed", "error", cause) } func (plan reviewPlan) rangeComplete() bool { @@ -577,8 +578,8 @@ func recoverablePrior(state State, view Snapshot, stack Stack) *Attempt { } // recoverableAttempt reports whether a review can reopen the attempt. A model -// result has no saved session to reopen, and a stack version that a feedback -// commit extended is no longer a range that the review can select. +// result has no saved session to reopen, and a stack version that no current +// stack continues is no longer a range that the review can select. func recoverableAttempt(attempt Attempt, view Snapshot) bool { if attempt.Status == "approved" { return false @@ -586,7 +587,7 @@ func recoverableAttempt(attempt Attempt, view Snapshot) bool { if isSame(attempt.ApprovalSource, "model") { return false } - return !supersededAttempt(attempt, view) + return !unusableAttempt(attempt, view) } func recordFailure(path, id string, cause error) error { diff --git a/internal/timber/review/run_test.go b/internal/timber/review/run_test.go index af68a1c..13843d8 100644 --- a/internal/timber/review/run_test.go +++ b/internal/timber/review/run_test.go @@ -2,6 +2,7 @@ package review import ( "context" + "encoding/json/v2" "fmt" "io" "os" @@ -163,6 +164,38 @@ func TestInterruptedReviewRecoveryAndReplacement(t *testing.T) { } } +// A saved session that still has work left belongs to a current stack, so a +// recovery must resume that attempt and keep the progress it recorded. +func TestInterruptedReviewResumesSavedProgress(t *testing.T) { + reviewer, view, store := testReviewer(t) + fakeTuicr(t, "exit 1") + _, err := reviewer.Review(t.Context(), ReviewOptions{}) + require.ErrorContains(t, err, "tuicr interrupted") + path, err := WorktreePath(view.Stacks[0].Repo) + require.NoError(t, err) + scope := view.Stacks[0] + scope.Repo = path + session := savedFixture(t, scope, store, false) + session["files"].(map[string]any)["main.go"].(map[string]any)["reviewed"] = true + writeJSON(t, filepath.Join(store, "sessions/0123456789abcdef.json"), session) + + fakeTuicr(t, "exit 0") + result, err := reviewer.Review(t.Context(), ReviewOptions{Choice: "recover"}) + require.NoError(t, err) + require.Equal(t, "incomplete", result.Status) + require.NotNil(t, result.Decision) + require.Equal(t, result.Decision.AttemptID, result.Entries[0].Attempt.ID) + state, err := (Store{Path: view.Path}).Load() + require.NoError(t, err) + require.Len(t, state.Attempts, 1, "recovery must not discard a resumable attempt") + require.Equal(t, result.Decision.AttemptID, state.Attempts[0].ID) + var saved SavedReview + require.NoError(t, json.Unmarshal(state.Attempts[0].Review, &saved)) + require.Equal(t, "incomplete", saved.Status()) + require.Equal(t, 1, saved.Reviewed) + require.Equal(t, 2, saved.Files) +} + func TestReviewDiscardsUntouchedExit(t *testing.T) { for _, savedSession := range []bool{false, true} { t.Run(fmt.Sprintf("saved=%t", savedSession), func(t *testing.T) { diff --git a/internal/timber/review/state.go b/internal/timber/review/state.go index 72b8d5a..d0f036b 100644 --- a/internal/timber/review/state.go +++ b/internal/timber/review/state.go @@ -355,6 +355,28 @@ func supersededAttempt(attempt Attempt, view Snapshot) bool { return false } +// lostAttempt reports whether no current stack continues the attempt's captured +// commits. A rewrite that leaves no lineage removes the range that the attempt +// and its saved session need, so no review can select it again. +func lostAttempt(attempt Attempt, view Snapshot) bool { + for _, stack := range view.Stacks { + if sameStack(attempt.Stack, stack) || sharesLineage(attempt.Stack, stack) { + return false + } + } + return true +} + +// unusableAttempt reports whether an unfinished attempt must be discarded, either +// because a current stack extends its commits or because no current stack +// continues them. Neither attempt asks the user for a recovery choice. +func unusableAttempt(attempt Attempt, view Snapshot) bool { + if !unfinished(attempt) { + return false + } + return supersededAttempt(attempt, view) || lostAttempt(attempt, view) +} + func extendsStack(old, current Stack) bool { if len(current.Commits) <= len(old.Commits) { return false From 04875febb28474c27e302228efb1e9aac05f7c66 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Wed, 4 Nov 2026 23:33:47 -0600 Subject: [PATCH 2/7] Ignore an attempt whose stack no longer exists when approving Approval refused any attempt that still looked active, asking the user to recover or replace it. For an attempt whose captured commits a rewrite removed, both choices are impossible, so the stack could never be approved and status kept reporting the dead attempt as active. An attempt that no review can reopen is no longer active, so approval, automatic review, feedback listings, and status all ignore it. The next review discards it. --- internal/cli/review/recovery_test.go | 24 ++++++++++++++++++++++++ internal/cli/review/status_test.go | 5 ++++- internal/timber/review/auto_split.go | 2 +- internal/timber/review/run.go | 18 +++++++++--------- internal/timber/review/service.go | 19 +++++++++++-------- 5 files changed, 49 insertions(+), 19 deletions(-) diff --git a/internal/cli/review/recovery_test.go b/internal/cli/review/recovery_test.go index 3a97a29..a60fd19 100644 --- a/internal/cli/review/recovery_test.go +++ b/internal/cli/review/recovery_test.go @@ -96,6 +96,30 @@ func TestReviewDiscardsAttemptWhoseStackNoLongerExists(t *testing.T) { require.NotContains(t, attemptIDs(state), interrupted.ID) } +func TestApprovalIgnoresAttemptWhoseStackNoLongerExists(t *testing.T) { + dir := featureRepo(t) + view, err := (review.Repository{Dir: dir}).Snapshot(t.Context()) + require.NoError(t, err) + interrupted := review.Attempt{ID: "interrupted", Stack: view.Stacks[0], Status: "reviewing", StartedAt: "2026-01-01T00:00:00Z"} + require.NoError(t, (review.Store{Path: view.Path}).Update(func(state *review.State) error { + state.Attempts = append(state.Attempts, interrupted) + return nil + })) + git(t, dir, "commit", "--amend", "--allow-empty", "-m", "Add feature again") + out, _, err := execute(t, dir, "approve", "HEAD", "--json") + require.NoError(t, err) + var approved review.Result + require.NoError(t, json.Unmarshal([]byte(out), &approved)) + require.Equal(t, "approved", approved.Status) + out, _, err = execute(t, dir, "status", "--needs-review", "--json") + require.NoError(t, err) + var listed review.Result + require.NoError(t, json.Unmarshal([]byte(out), &listed)) + require.Equal(t, "idle", listed.Status) + require.Empty(t, listed.Entries) + require.Empty(t, listed.Active) +} + func attemptIDs(state review.State) []string { ids := make([]string, 0, len(state.Attempts)) for _, attempt := range state.Attempts { diff --git a/internal/cli/review/status_test.go b/internal/cli/review/status_test.go index d12dc1b..4bb469c 100644 --- a/internal/cli/review/status_test.go +++ b/internal/cli/review/status_test.go @@ -112,7 +112,10 @@ func TestStatusCountsCurrentCommitsInStateOrder(t *testing.T) { require.NoError(t, json.Unmarshal([]byte(out), &result)) require.Equal(t, []review.StateCount{{State: "needs_review"}, {State: "reviewing"}, {State: "feedback"}, {State: "incomplete"}, {State: "auto_reviewed"}, {State: "approved"}}, result.Summary) require.Empty(t, result.Entries) - require.Equal(t, "pending", result.Status) // The historical active attempt still needs recovery. + // The removed stack can never reopen its attempt, so the attempt is not + // active and no review is pending. + require.Equal(t, "idle", result.Status) + require.Empty(t, result.Active) } func assertStatusVerbosity(t *testing.T, binary, dir, out string) { diff --git a/internal/timber/review/auto_split.go b/internal/timber/review/auto_split.go index 359a0f0..2d7217b 100644 --- a/internal/timber/review/auto_split.go +++ b/internal/timber/review/auto_split.go @@ -73,7 +73,7 @@ func runAuto(ctx context.Context, s Service, evaluator autoreview.Evaluator, opt if err := writeAutoLock(lock, view); err != nil { return result, err } - if len(activeAttempts(state)) != 0 { + if len(activeAttempts(view, state)) != 0 { return result, fmt.Errorf("recover or replace the active review before automatic approval") } selected, err := (Reviewer{Service: s}).bounds(ctx, view, options.Start, options.End) diff --git a/internal/timber/review/run.go b/internal/timber/review/run.go index b7b10fe..cd57d88 100644 --- a/internal/timber/review/run.go +++ b/internal/timber/review/run.go @@ -86,7 +86,7 @@ func (r Reviewer) begin(ctx context.Context, plan reviewPlan) (Attempt, error) { if err := r.Service.unchanged(ctx, plan.view); err != nil { return err } - if err := checkActive(*state, plan.prior); err != nil { + if err := checkActive(plan.view, *state, plan.prior); err != nil { return err } if plan.prior != nil { @@ -268,11 +268,11 @@ func (r Reviewer) plan(ctx context.Context, options ReviewOptions, visited map[s } selector := options.Stack view, state := plan.view, plan.state - active := activeAttempts(state) + active := activeAttempts(view, state) if len(active) > 1 { return plan, fmt.Errorf("multiple active attempts in state; resolve them before review") } - if len(active) == 1 && !supersededAttempt(active[0], view) { + if len(active) == 1 { prior := active[0] plan.prior = &prior plan.stack = &prior.Stack @@ -365,7 +365,7 @@ func (r Reviewer) resume(ctx context.Context, path string, plan reviewPlan, sess if _, err := r.readAttempt(*plan.prior, session); err != nil { return Result{}, err } - attempt, err := resumeAttempt(plan.view.Path, *plan.prior) + attempt, err := resumeAttempt(plan.view, *plan.prior) if err != nil { return Result{}, err } @@ -474,10 +474,10 @@ func (plan reviewPlan) rangeComplete() bool { return plan.exhausted } -func resumeAttempt(path string, prior Attempt) (Attempt, error) { +func resumeAttempt(view Snapshot, prior Attempt) (Attempt, error) { var attempt Attempt - err := (Store{Path: path}).Update(func(state *State) error { - if err := checkActive(*state, &prior); err != nil { + err := (Store{Path: view.Path}).Update(func(state *State) error { + if err := checkActive(view, *state, &prior); err != nil { return err } for i := range state.Attempts { @@ -529,8 +529,8 @@ func finishAttempt(state *State, attempt Attempt, saved *SavedReview, raw []byte return Attempt{}, fmt.Errorf("review attempt disappeared; retry") } -func checkActive(state State, prior *Attempt) error { - for _, a := range activeAttempts(state) { +func checkActive(view Snapshot, state State, prior *Attempt) error { + for _, a := range activeAttempts(view, state) { if prior == nil { return fmt.Errorf("another session started a review") } diff --git a/internal/timber/review/service.go b/internal/timber/review/service.go index bfe4e26..7650409 100644 --- a/internal/timber/review/service.go +++ b/internal/timber/review/service.go @@ -159,7 +159,7 @@ func (s Service) Feedback(ctx context.Context) (Result, error) { if err != nil { return Result{}, err } - result := Result{Version: 1, Command: "feedback", Status: "idle", View: view, Entries: []Entry{}, Active: activeAttempts(state), FileFeedback: files} + result := Result{Version: 1, Command: "feedback", Status: "idle", View: view, Entries: []Entry{}, Active: activeAttempts(view, state), FileFeedback: files} if len(files) > 0 { result.Status = "pending" } @@ -248,7 +248,7 @@ func (s Service) feedbackView(ctx context.Context, view Snapshot, state State) ( func (s Service) recordApproval(ctx context.Context, view Snapshot, selected []Stack) (Result, error) { store := Store{Path: view.Path} err := store.Update(func(state *State) error { - if err := approvalAllowed(selected, *state); err != nil { + if err := approvalAllowed(selected, view, *state); err != nil { return err } if err := s.unchanged(ctx, view); err != nil { @@ -302,7 +302,7 @@ func (s Service) recordAutomatic(ctx context.Context, view Snapshot, entry *Auto } if status == "approved" { // Only an approval needs the state and feedback checks. - if err := approvalAllowed([]Stack{entry.Stack}, *state); err != nil { + if err := approvalAllowed([]Stack{entry.Stack}, view, *state); err != nil { return err } } @@ -381,18 +381,21 @@ func (s Service) unchanged(ctx context.Context, view Snapshot) error { return nil } -func activeAttempts(state State) []Attempt { +// activeAttempts lists the attempts that can still hold their captured stack. An +// attempt that no current stack continues cannot be reopened, so it blocks +// neither another review nor an approval. +func activeAttempts(view Snapshot, state State) []Attempt { active := []Attempt{} for _, a := range state.Attempts { - if a.Status == "reviewing" { + if a.Status == "reviewing" && !unusableAttempt(a, view) { active = append(active, a) } } return active } -func approvalAllowed(selected []Stack, state State) error { - if len(activeAttempts(state)) > 0 { +func approvalAllowed(selected []Stack, view Snapshot, state State) error { + if len(activeAttempts(view, state)) > 0 { return fmt.Errorf("recover or replace the active review before approval") } for _, stack := range selected { @@ -454,7 +457,7 @@ func makeResult(command string, view Snapshot, state State) Result { } func resultFromEntries(command string, view Snapshot, state State, entries []Entry) Result { - result := Result{Version: 1, Command: command, Status: "pending", View: view, Entries: []Entry{}, Active: activeAttempts(state)} + result := Result{Version: 1, Command: command, Status: "pending", View: view, Entries: []Entry{}, Active: activeAttempts(view, state)} for _, entry := range entries { if entry.Status != "approved" { result.Entries = append(result.Entries, entry) From a9f593d40096c3a798ebda0e17955aa567deea7b Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 5 Nov 2026 01:23:41 -0600 Subject: [PATCH 3/7] Refactor: Carry the review store on a saved review A session document lives in a store whose index names it. A later change moves a session to another commit range, which needs the store as well as the document, so record the store beside the path that the reader already reports. The field is not serialized. Change-Id: Iab01f2030d6a2a2089da96644a9c6482457c2553 --- internal/timber/review/saved.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/timber/review/saved.go b/internal/timber/review/saved.go index 32cca67..32898b2 100644 --- a/internal/timber/review/saved.go +++ b/internal/timber/review/saved.go @@ -20,6 +20,7 @@ type SavedReview struct { Scope Stack `json:"scope"` Slug string `json:"slug"` Path string `json:"path"` + Store string `json:"-"` ID string `json:"id"` Version string `json:"version"` Reviewed int `json:"reviewed"` @@ -686,10 +687,10 @@ func readEntry(store, repo, slug string, entry object, scope Stack, selector str if err != nil { return nil, err } - return readCandidate(path, scope, repo, selector, slug, relative) + return readCandidate(store, path, scope, repo, selector, slug, relative) } -func readCandidate(path string, scope Stack, repo, selector, slug, relative string) (*SavedReview, error) { +func readCandidate(store, path string, scope Stack, repo, selector, slug, relative string) (*SavedReview, error) { session, err := readObject(path) if err != nil { return nil, err @@ -698,6 +699,7 @@ func readCandidate(path string, scope Stack, repo, selector, slug, relative stri if saved != nil { saved.Slug = slug saved.Path = path + saved.Store = store } return saved, err } From 79f2e40695254a5a61fe6b8ee224c42f2a553fff Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 5 Nov 2026 01:36:27 -0600 Subject: [PATCH 4/7] Carry a saved review over to rewritten commits tuicr keeps a session only for the exact commit range that a review shows, so a rebase or a reword discarded the reviewer's marks and comments even when the patch was identical. The human then had to review the same patch again, and the attempt that Timber recorded described a range that no review could reopen. A review now moves the session of a reviewed patch onto the current stack before it starts: the files, marks, comments, and content hashes stay, the commit-message entry follows the new tip, and a reworded message drops its mark and its comments so the message is read again. Only a matching patch carries, so no mark ever describes content that changed. Carrying is best effort, because the store belongs to tuicr: an unreadable or unexpected document warns and leaves a fresh review. Change-Id: Ic1634260e2004f026ac407b5fb2a2dc910931789 --- internal/timber/review/carry.go | 431 +++++++++++++++++++++++++++ internal/timber/review/carry_test.go | 183 ++++++++++++ internal/timber/review/predicates.go | 10 + internal/timber/review/run.go | 53 ++-- 4 files changed, 652 insertions(+), 25 deletions(-) create mode 100644 internal/timber/review/carry.go create mode 100644 internal/timber/review/carry_test.go diff --git a/internal/timber/review/carry.go b/internal/timber/review/carry.go new file mode 100644 index 0000000..86a0bca --- /dev/null +++ b/internal/timber/review/carry.go @@ -0,0 +1,431 @@ +package review + +import ( + "context" + "encoding/json/jsontext" + "encoding/json/v2" + "fmt" + "os" + "path/filepath" + "slices" + "strings" + "time" + "uuid" +) + +// commitMessagePrefix names the pseudo-file that tuicr adds for the commit +// message that a review shows. Its key carries the abbreviated tip commit. +const commitMessagePrefix = "Commit Message (" + +// carriedSession is the saved review that a rewrite moved onto a new range. +type carriedSession struct { + store string + path string + slug string + 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. + 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 +// the current range. Only a changed commit message asks the reviewer to read +// that message again. +// Carrying is best effort: a review must still start when the store cannot be +// read or holds a document that this build does not understand. +func carrySavedReviews(ctx context.Context, repository Repository, reader SavedReader) { + view, err := repository.Snapshot(ctx) + if err != nil { + warnCarry(repository, err) + return + } + state, err := (Store{Path: view.Path}).Load() + if err != nil { + warnCarry(repository, err) + return + } + for _, stack := range view.Stacks { + prior := carryablePrior(state, stack) + if prior == nil { + continue + } + if err := carrySavedSession(ctx, repository, reader, *prior, stack); err != nil { + warnCarry(repository, err) + } + } +} + +// carrySavedSession moves the session that captured from onto the current stack. +func carrySavedSession(ctx context.Context, repository Repository, reader SavedReader, from Attempt, to Stack) error { + source, err := carrySource(reader, from) + if err != nil { + return err + } + if source == nil { + return nil + } + checkout, err := WorktreePath(to.Repo) + if err != nil { + return err + } + described, err := describesRange(source, to, checkout) + if err != nil { + return err + } + if described { + // A session already describes the current range. + return nil + } + target, err := carryTargetFor(ctx, repository, checkout, to) + if err != nil { + return err + } + return moveCarriedSession(source, to, target, sameMessage(from.Stack, to)) +} + +// carrySource reads the session that captured an attempt, or nil when the +// attempt left no session to move. +func carrySource(reader SavedReader, from Attempt) (*carriedSession, error) { + saved, err := readAttempt(reader, from, "") + if err != nil { + return nil, err + } + if saved == nil || isEmpty(saved.Store) { + return nil, nil + } + document, err := readObject(saved.Path) + if err != nil { + return nil, err + } + if err := validateCarriedSession(document, from.Stack); err != nil { + return nil, err + } + 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) + 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) + if err != nil { + return carryTarget{}, err + } + return carryTarget{anchor: anchor, short: short, slug: slug, now: time.Now().UTC().Format(time.RFC3339Nano)}, nil +} + +// carryTargetName holds the names and the time of the range that a session +// moves to. +type carryTarget struct { + anchor string + short string + slug string + now string +} + +// carryablePrior returns the newest reviewed attempt for the patch of stack +// whose captured commits a rewrite replaced. +func carryablePrior(state State, stack Stack) *Attempt { + for i := len(state.Attempts) - 1; i >= 0; i-- { + attempt := &state.Attempts[i] + if isSame(attempt.ApprovalSource, "model") { + // A model result is an assessment, not a session to reopen. + continue + } + if sameStack(attempt.Stack, stack) { + continue + } + if samePatch(attempt.Stack, stack) { + return attempt + } + } + return nil +} + +// describesRange reports whether a session already describes the current range. +func describesRange(source *carriedSession, to Stack, checkout string) (bool, error) { + described := describe(to, checkout) + if described == nil { + return false, nil + } + existing, err := (SavedReader{Stores: []string{source.store}}).Read(*described, "") + if err != nil { + return false, err + } + return existing != nil, nil +} + +// describe returns the scope of stack inside the review checkout, or nil when +// no checkout exists to hold a session. +func describe(stack Stack, checkout string) *Stack { + info, err := os.Lstat(checkout) + if err != nil || isFalse(info.IsDir()) { + return nil + } + described := stack + described.Repo = checkout + return &described +} + +// moveCarriedSession writes the carried document and names its index entry +// under the current range. +func moveCarriedSession(source *carriedSession, to Stack, target carryTarget, keepMessage bool) error { + if err := rewriteCarriedSession(source.document, to, target.short, keepMessage, target.now); err != nil { + return err + } + data, err := json.Marshal(source.document, jsontext.WithIndent(" ")) + if err != nil { + return err + } + if err := replaceFile(source.path, append(data, '\n')); err != nil { + return err + } + return moveIndexEntry(source.store, source.path, source.slug, target.slug, sessionDisplay(source.document, target.anchor), target.now) +} + +// moveIndexEntry names the moved session under its current range and drops the +// entry for the range that a review no longer shows. +func moveIndexEntry(store, path, fromSlug, toSlug string, display object, now string) error { + index, entries, err := loadIndex(store) + if err != nil { + return err + } + if index == nil { + // A store without an index names no sessions to move. + return nil + } + if entries == nil { + return nil + } + // Match on the file name: a store can be reached through a symlinked path, + // which makes the path that the reader reports differ from the store. + entry, err := removeIndexEntry(entries, fromSlug, filepath.Base(path)) + if err != nil { + return err + } + if entry == nil { + return nil + } + entry["updated_at"] = quoted(now) + encoded, err := json.Marshal(display) + if err != nil { + return err + } + entry["display"] = encoded + entries[toSlug] = append(entries[toSlug], entry) + return writeIndex(store, index, entries) +} + +// quoted encodes a string as a JSON document. +func quoted(value string) jsontext.Value { + encoded, err := json.Marshal(value) + if err != nil { + return jsontext.Value(`""`) + } + return encoded +} + +// removeIndexEntry removes the row that names a session file and returns it. +func removeIndexEntry(entries map[string][]object, slug, name string) (object, error) { + rows := entries[slug] + kept := make([]object, 0, len(rows)) + var found object + for _, row := range rows { + stored, err := decodeRequired[string](row, "path") + if err != nil { + return nil, err + } + if found == nil && isSame(filepath.Base(stored), name) { + found = row + continue + } + kept = append(kept, row) + } + if len(kept) == 0 { + delete(entries, slug) + } else { + entries[slug] = kept + } + return found, nil +} + +// renameCommitMessageFile moves the message pseudo-file to the tip's key. A +// reworded message also loses its review mark and its comments, because they +// describe a message that the review no longer shows. +func renameCommitMessageFile(files object, short string, keepMessage bool) error { + var previous string + for key := range files { + if strings.HasPrefix(key, commitMessagePrefix) { + previous = key + } + } + if isEmpty(previous) { + return nil + } + message, err := decodeRequired[object](files, previous) + if err != nil { + return err + } + delete(files, previous) + name := commitMessagePrefix + short + ")" + message["path"] = quoted(name) + if isFalse(keepMessage) { + message["reviewed"] = jsontext.Value("false") + message["line_comments"] = jsontext.Value("{}") + message["file_comments"] = jsontext.Value("[]") + } + encoded, err := json.Marshal(message) + if err != nil { + return err + } + files[name] = encoded + return nil +} + +// rewriteCarriedSession points a session at the commits that the review shows +// now. Marks, comments, and content hashes stay because the patch is unchanged. +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) + if err != nil { + return err + } + document["commit_range"] = commits + document["base_commit"] = quoted(to.Commits[0]) + files, err := decodeRequired[object](document, "files") + if err != nil { + return err + } + if err := renameCommitMessageFile(files, short, keepMessage); err != nil { + return err + } + encoded, err := json.Marshal(files) + if err != nil { + return err + } + document["files"] = encoded + return nil +} + +// countSessionReview counts the reviewed files and the comments of a session. +func countSessionReview(files object) (int, int) { + reviewed, comments := 0, 0 + for _, raw := range files { + var file object + if err := json.Unmarshal(raw, &file); err != nil { + continue + } + if value, err := decodeRequired[bool](file, "reviewed"); err == nil && isTrue(value) { + reviewed++ + } + if value, err := decodeRequired[object](file, "line_comments"); err == nil { + for _, items := range value { + var list []jsontext.Value + if err := json.Unmarshal(items, &list); err == nil { + comments += len(list) + } + } + } + if value, err := decodeRequired[[]jsontext.Value](file, "file_comments"); err == nil { + comments += len(value) + } + } + return reviewed, comments +} + +// sessionDisplay counts what the moved session shows, because the index that +// tuicr lists sessions from keeps its own copy of those counts. +func sessionDisplay(document object, anchor string) object { + display := object{ + "comment_count": jsontext.Value("0"), + "reviewed_count": jsontext.Value("0"), + "file_count": jsontext.Value("0"), + "anchor": quoted(anchor), + } + files, err := decodeRequired[object](document, "files") + if err != nil { + return display + } + reviewed, comments := countSessionReview(files) + display["reviewed_count"] = jsontext.Value(fmt.Sprintf("%d", reviewed)) + display["comment_count"] = jsontext.Value(fmt.Sprintf("%d", comments)) + display["file_count"] = jsontext.Value(fmt.Sprintf("%d", len(files))) + return display +} + +// 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 +} + +// shortSHA reports the abbreviation that git prints for a revision. +func shortSHA(ctx context.Context, repository Repository, revision string) (string, error) { + return repository.git(ctx, "rev-parse", "--short", revision) +} + +// validateCarriedSession rejects a document whose range is not the range that +// the attempt captured. Carrying another document would attach a review to +// content that its marks and comments never described. +func validateCarriedSession(document object, from Stack) error { + source, err := decodeRequired[string](document, "diff_source") + if err != nil { + return err + } + if isFalse(isSame(source, "commit_range")) { + return fmt.Errorf("saved review does not capture a commit range") + } + commits, err := decodeRequired[[]string](document, "commit_range") + if err != nil { + return err + } + if isFalse(slices.Equal(commits, from.Commits)) { + return fmt.Errorf("saved review captured a different commit range") + } + return nil +} + +// warnCarry reports a session that could not move, because the review still +// has to start. +func warnCarry(repository Repository, cause error) { + repository.logger().Warn("Saved review carry failed", "error", cause) +} diff --git a/internal/timber/review/carry_test.go b/internal/timber/review/carry_test.go new file mode 100644 index 0000000..c94c718 --- /dev/null +++ b/internal/timber/review/carry_test.go @@ -0,0 +1,183 @@ +package review + +import ( + "encoding/json/v2" + "os" + "path/filepath" + "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. +func carryFixture(t *testing.T, reviewer Reviewer, view Snapshot, store string) { + t.Helper() + scope := view.Stacks[0] + path, err := WorktreePath(scope.Repo) + require.NoError(t, err) + require.NoError(t, prepareWorktree(t.Context(), reviewer.Service.Repository, path, scope)) + scope.Repo = path + session := savedFixture(t, scope, store, false) + short, err := shortSHA(t.Context(), reviewer.Service.Repository, view.Stacks[0].Tip) + require.NoError(t, err) + files := session["files"].(map[string]any) + message := files["CommitMessage"].(map[string]any) + delete(files, "CommitMessage") + name := commitMessagePrefix + short + ")" + message["path"] = name + message["reviewed"] = true + files[name] = message + file := files["main.go"].(map[string]any) + file["reviewed"] = true + file["line_comments"] = map[string]any{"1": []any{map[string]any{ + "id": "comment", "content": "Handle the empty case", "comment_type": "issue", + "created_at": "2026-01-01T00:00:00Z", "author": "user", "lifecycle_state": "local_draft", + }}} + writeJSON(t, filepath.Join(store, "sessions/0123456789abcdef.json"), session) + 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 + })) +} + +func carryRepository(t *testing.T) (Reviewer, Snapshot) { + t.Helper() + reviewer, _, _ := testReviewer(t) + dir := reviewer.Service.Repository.Dir + gitTest(t, dir, "reset", "--hard", "main") + require.NoError(t, os.WriteFile(filepath.Join(dir, "main.go"), []byte("package main\n"), 0o600)) + gitTest(t, dir, "add", ".") + gitTest(t, dir, "commit", "-m", "Feature") + view, err := reviewer.Service.Repository.Snapshot(t.Context()) + require.NoError(t, err) + return reviewer, view +} + +func TestCarrySavedReviewKeepsMarksAcrossRewrite(t *testing.T) { + for _, tc := range []struct { + name string + message string + wantMessage bool + wantReviewed int + }{ + {name: "reworded", message: "Feature reworded", wantMessage: false, wantReviewed: 1}, + {name: "rebased", message: "", wantMessage: true, wantReviewed: 2}, + } { + t.Run(tc.name, func(t *testing.T) { + reviewer, view := carryRepository(t) + store := t.TempDir() + reviewer.Reader = SavedReader{Stores: []string{store}} + carryFixture(t, reviewer, view, store) + args := []string{"commit", "--amend", "--allow-empty", "--date", "2026-01-01T00:00:00Z"} + if tc.message == "" { + args = append(args, "--no-edit") + } else { + args = append(args, "-m", tc.message) + } + gitTest(t, reviewer.Service.Repository.Dir, args...) + carried, err := reviewer.Service.Repository.Snapshot(t.Context()) + require.NoError(t, err) + require.NotEqual(t, view.Stacks[0].Commits, carried.Stacks[0].Commits) + require.Equal(t, view.Stacks[0].PatchHash, carried.Stacks[0].PatchHash) + + 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, tc.wantReviewed, saved.Reviewed) + require.Equal(t, 2, saved.Files) + require.Equal(t, "feedback", saved.Status()) + + document, err := readObject(saved.Path) + require.NoError(t, err) + files, err := decodeRequired[object](document, "files") + require.NoError(t, err) + short, err := shortSHA(t.Context(), reviewer.Service.Repository, carried.Stacks[0].Tip) + require.NoError(t, err) + message, err := decodeRequired[object](files, commitMessagePrefix+short+")") + require.NoError(t, err) + reviewed, err := decodeRequired[bool](message, "reviewed") + require.NoError(t, err) + require.Equal(t, tc.wantMessage, reviewed, "only a changed message asks again") + main, err := decodeRequired[object](files, "main.go") + require.NoError(t, err) + marked, err := decodeRequired[bool](main, "reviewed") + require.NoError(t, err) + require.True(t, marked) + + index, entries, err := loadIndex(store) + require.NoError(t, err) + require.NotNil(t, index) + require.NotContains(t, entries, "fixture") + require.Len(t, entries, 1) + }) + } +} + +func TestCarrySavedReviewLeavesAMatchingRangeAlone(t *testing.T) { + reviewer, view := carryRepository(t) + store := t.TempDir() + reviewer.Reader = SavedReader{Stores: []string{store}} + carryFixture(t, reviewer, view, store) + before, err := os.ReadFile(filepath.Join(store, "sessions/0123456789abcdef.json")) + require.NoError(t, err) + + carrySavedReviews(t.Context(), reviewer.Service.Repository, reviewer.Reader) + + after, err := os.ReadFile(filepath.Join(store, "sessions/0123456789abcdef.json")) + require.NoError(t, err) + require.Equal(t, before, after, "a current range keeps its session") + var indexed map[string]any + 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") +} + +// A review that a rewrite moved records the carried progress, so the result and +// the session that tuicr shows describe the same work. +func TestReviewRecordsCarriedProgress(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") + fakeTuicr(t, "exit 0") + + result, err := reviewer.Review(t.Context(), ReviewOptions{Choice: "replace"}) + require.NoError(t, err) + require.Equal(t, "feedback", result.Status) + require.Len(t, result.Entries, 1) + var saved SavedReview + require.NoError(t, json.Unmarshal(result.Entries[0].Attempt.Review, &saved)) + require.Equal(t, 1, saved.Reviewed) + require.Equal(t, 2, saved.Files) + require.Len(t, saved.Comments, 1) +} + +// A store is often reached through a symlinked path, as macOS does for /var, so +// the path that the reader reports differs from the path that names the store. +// The move must still find the index entry. +func TestCarrySavedReviewThroughAliasedStore(t *testing.T) { + reviewer, view := carryRepository(t) + store := t.TempDir() + alias := filepath.Join(t.TempDir(), "alias") + require.NoError(t, os.Symlink(store, alias)) + 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") + + 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) +} diff --git a/internal/timber/review/predicates.go b/internal/timber/review/predicates.go index fd5397a..c5fb6bb 100644 --- a/internal/timber/review/predicates.go +++ b/internal/timber/review/predicates.go @@ -37,3 +37,13 @@ func isTrue(v bool) bool { func isSymlink(info os.FileInfo) bool { return info.Mode()&os.ModeSymlink != 0 } + +// isEmpty reports whether s holds no characters. +func isEmpty(s string) bool { + return s == "" +} + +// isNotEmpty reports whether s holds at least one character. +func isNotEmpty(s string) bool { + return s != "" +} diff --git a/internal/timber/review/run.go b/internal/timber/review/run.go index cd57d88..1ee2a8a 100644 --- a/internal/timber/review/run.go +++ b/internal/timber/review/run.go @@ -62,6 +62,7 @@ func (r Reviewer) Review(ctx context.Context, options ReviewOptions) (result Res } stopMaintenance := startCacheMaintenance(ctx, r.Service.Repository, lock) defer stopMaintenance() + carrySavedReviews(ctx, r.Service.Repository, r.Reader) r.discardUnusable(ctx) result, err = r.reviewSequence(ctx, options, path, lock) if err != nil { @@ -298,31 +299,8 @@ func (r Reviewer) plan(ctx context.Context, options ReviewOptions, visited map[s return plan, nil } -func (r Reviewer) readAttempt(attempt Attempt, selector string) (*SavedReview, error) { - path, err := WorktreePath(attempt.Stack.Repo) - if err != nil { - return nil, err - } - if _, err := os.Stat(filepath.Join(path, ".git")); err == nil { - scope := attempt.Stack - scope.Repo = path - saved, err := r.Reader.Read(scope, selector) - if err != nil { - return nil, err - } - if saved != nil { - return saved, nil - } - } else { - if !errors.Is(err, os.ErrNotExist) { - return nil, err - } - } - return r.Reader.Read(attempt.Stack, selector) -} - func (r Reviewer) recover(plan reviewPlan, attempt Attempt, selector string) (Result, error) { - saved, err := r.readAttempt(attempt, selector) + saved, err := readAttempt(r.Reader, attempt, selector) if err != nil { return Result{}, err } @@ -362,7 +340,7 @@ func (r Reviewer) recover(plan reviewPlan, attempt Attempt, selector string) (Re func (r Reviewer) resume(ctx context.Context, path string, plan reviewPlan, session string, lock *os.File) (Result, error) { // Tuicr resumes the saved session for the same checkout and range. // Validate selection before launch rather than opening an ambiguous review. - if _, err := r.readAttempt(*plan.prior, session); err != nil { + if _, err := readAttempt(r.Reader, *plan.prior, session); err != nil { return Result{}, err } attempt, err := resumeAttempt(plan.view, *plan.prior) @@ -467,6 +445,31 @@ func (r Reviewer) warnCleanup(cause error) { r.Service.Repository.logger().Warn("Review cleanup failed", "error", cause) } +// readAttempt reads the saved review that captured an attempt. A session lives +// in the review checkout, but an older one can name the repository itself. +func readAttempt(reader SavedReader, attempt Attempt, selector string) (*SavedReview, error) { + path, err := WorktreePath(attempt.Stack.Repo) + if err != nil { + return nil, err + } + if _, err := os.Stat(filepath.Join(path, ".git")); err == nil { + scope := attempt.Stack + scope.Repo = path + saved, err := reader.Read(scope, selector) + if err != nil { + return nil, err + } + if saved != nil { + return saved, nil + } + } else { + if !errors.Is(err, os.ErrNotExist) { + return nil, err + } + } + return reader.Read(attempt.Stack, selector) +} + func (plan reviewPlan) rangeComplete() bool { if !plan.automatic { return false From 23e44351a6054ecb86764a66216befa3980acec4 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 5 Nov 2026 02:39:12 -0600 Subject: [PATCH 5/7] Replace the recovery choice with a rule An interrupted review asked the user to recover or replace it, and after a rewrite neither answer could open the captured range: recovery showed commits that no longer exist and replacement refused to start. The question was also unnecessary in the common case, where resuming is the only sensible answer. A review now resumes the attempt that captured the same stack, and it starts a new attempt on the current stack otherwise, which the previous change already carries the saved review onto. The --recover and --replace flags, the prompt, the decision result, and the decision_required status are removed, so a review either runs or reports its outcome. Change-Id: Ie52af4a26a36c18d09ef0a8563864d5795f8928d --- go.mod | 7 +- go.sum | 8 -- internal/cli/review/activity_test.go | 50 +++---- internal/cli/review/cli_test.go | 32 ++--- internal/cli/review/commits/commits.go | 24 +--- internal/cli/review/commits/prompt.go | 52 -------- internal/cli/review/commits/prompt_test.go | 97 -------------- internal/cli/review/display/render.go | 66 --------- internal/cli/review/feedback_test.go | 1 - internal/cli/review/recovery_test.go | 50 +------ internal/cli/review/tuicr_functional_test.go | 133 +++++++------------ internal/timber/review/auto_split.go | 2 +- internal/timber/review/auto_test.go | 2 +- internal/timber/review/bounds_test.go | 28 ++-- internal/timber/review/carry_test.go | 2 +- internal/timber/review/run.go | 59 +------- internal/timber/review/run_test.go | 120 ++++++++--------- internal/timber/review/service.go | 13 +- 18 files changed, 169 insertions(+), 577 deletions(-) delete mode 100644 internal/cli/review/commits/prompt.go delete mode 100644 internal/cli/review/commits/prompt_test.go diff --git a/go.mod b/go.mod index addf5b4..c613b8f 100644 --- a/go.mod +++ b/go.mod @@ -4,7 +4,6 @@ go 1.27.3 require ( charm.land/fang/v2 v2.0.1 - charm.land/huh/v2 v2.0.3 charm.land/lipgloss/v2 v2.0.5 charm.land/log/v2 v2.0.1 github.com/BurntSushi/toml v1.6.0 @@ -15,7 +14,6 @@ require ( github.com/charmbracelet/huh v1.0.0 github.com/charmbracelet/lipgloss v1.1.1-0.20250404203927-76690c660834 github.com/charmbracelet/x/ansi v0.11.8 - github.com/charmbracelet/x/term v0.2.2 github.com/creack/pty v1.1.24 github.com/go-git/go-git/v6 v6.0.0-beta.1 github.com/google/go-github/v91 v91.0.0 @@ -29,8 +27,6 @@ require ( ) require ( - charm.land/bubbles/v2 v2.0.0 // indirect - charm.land/bubbletea/v2 v2.0.2 // indirect github.com/Microsoft/go-winio v0.6.2 // indirect github.com/ProtonMail/go-crypto v1.5.2 // indirect github.com/agnivade/levenshtein v1.2.1 // indirect @@ -52,11 +48,12 @@ require ( github.com/charmbracelet/x/conpty v0.1.1 // indirect github.com/charmbracelet/x/errors v0.0.0-20240904165849-e8e43e13f84b // indirect github.com/charmbracelet/x/exp/charmtone v0.0.0-20250603201427-c31516f43444 // indirect - github.com/charmbracelet/x/exp/ordered v0.1.0 // indirect github.com/charmbracelet/x/exp/slice v0.0.0-20250327172914-2fdc97757edf // indirect github.com/charmbracelet/x/exp/strings v0.0.0-20240722160745-212f7b056ed0 // indirect + github.com/charmbracelet/x/term v0.2.2 // indirect github.com/charmbracelet/x/termios v0.1.1 // indirect github.com/charmbracelet/x/windows v0.2.2 // indirect + github.com/charmbracelet/x/xpty v0.1.3 // indirect github.com/clipperhouse/displaywidth v0.11.0 // indirect github.com/clipperhouse/uax29/v2 v2.7.0 // indirect github.com/cloudflare/circl v1.6.3 // indirect diff --git a/go.sum b/go.sum index eb6cfbb..e782e2b 100644 --- a/go.sum +++ b/go.sum @@ -1,11 +1,5 @@ -charm.land/bubbles/v2 v2.0.0 h1:tE3eK/pHjmtrDiRdoC9uGNLgpopOd8fjhEe31B/ai5s= -charm.land/bubbles/v2 v2.0.0/go.mod h1:rCHoleP2XhU8um45NTuOWBPNVHxnkXKTiZqcclL/qOI= -charm.land/bubbletea/v2 v2.0.2 h1:4CRtRnuZOdFDTWSff9r8QFt/9+z6Emubz3aDMnf/dx0= -charm.land/bubbletea/v2 v2.0.2/go.mod h1:3LRff2U4WIYXy7MTxfbAQ+AdfM3D8Xuvz2wbsOD9OHQ= charm.land/fang/v2 v2.0.1 h1:zQCM8JQJ1JnQX/66B5jlCYBUxL2as5JXQZ2KJ6EL0mY= charm.land/fang/v2 v2.0.1/go.mod h1:S1GmkpcvK+OB5w9caywUnJcsMew45Ot8FXqoz8ALrII= -charm.land/huh/v2 v2.0.3 h1:2cJsMqEPwSywGHvdlKsJyQKPtSJLVnFKyFbsYZTlLkU= -charm.land/huh/v2 v2.0.3/go.mod h1:93eEveeeqn47MwiC3tf+2atZ2l7Is88rAtmZNZ8x9Wc= charm.land/lipgloss/v2 v2.0.5 h1:kbNxgeeUOYv5J0YdpxFjfvf3dFvqH8Aci4zB6xqFtrY= charm.land/lipgloss/v2 v2.0.5/go.mod h1:9oqhxt4yxIMe6q5A4kHr44DremZk7J9UNh74GlWa5nc= charm.land/log/v2 v2.0.1 h1:uMk+t1hM/fomIqt7vn/Ub0AKo7IHvnDu6NK17nbOVx0= @@ -86,8 +80,6 @@ github.com/charmbracelet/x/exp/charmtone v0.0.0-20250603201427-c31516f43444 h1:I github.com/charmbracelet/x/exp/charmtone v0.0.0-20250603201427-c31516f43444/go.mod h1:T9jr8CzFpjhFVHjNjKwbAD7KwBNyFnj2pntAO7F2zw0= github.com/charmbracelet/x/exp/golden v0.0.0-20250806222409-83e3a29d542f h1:pk6gmGpCE7F3FcjaOEKYriCvpmIN4+6OS/RD0vm4uIA= github.com/charmbracelet/x/exp/golden v0.0.0-20250806222409-83e3a29d542f/go.mod h1:IfZAMTHB6XkZSeXUqriemErjAWCCzT0LwjKFYCZyw0I= -github.com/charmbracelet/x/exp/ordered v0.1.0 h1:55/qLwjIh0gL0Vni+QAWk7T/qRVP6sBf+2agPBgnOFE= -github.com/charmbracelet/x/exp/ordered v0.1.0/go.mod h1:5UHwmG+is5THxMyCJHNPCn2/ecI07aKNrW+LcResjJ8= github.com/charmbracelet/x/exp/slice v0.0.0-20250327172914-2fdc97757edf h1:rLG0Yb6MQSDKdB52aGX55JT1oi0P0Kuaj7wi1bLUpnI= github.com/charmbracelet/x/exp/slice v0.0.0-20250327172914-2fdc97757edf/go.mod h1:B3UgsnsBZS/eX42BlaNiJkD1pPOUa+oF1IYC6Yd2CEU= github.com/charmbracelet/x/exp/strings v0.0.0-20240722160745-212f7b056ed0 h1:qko3AQ4gK1MTS/de7F5hPGx6/k1u0w4TeYmBFwzYVP4= diff --git a/internal/cli/review/activity_test.go b/internal/cli/review/activity_test.go index 0e5bfed..20af2f4 100644 --- a/internal/cli/review/activity_test.go +++ b/internal/cli/review/activity_test.go @@ -1,11 +1,11 @@ package cli import ( - "context" "os" "os/exec" "path/filepath" "testing" + "time" "github.com/stretchr/testify/require" @@ -48,34 +48,38 @@ func TestReviewRunningReportsLiveLockNotPersistedAttempt(t *testing.T) { require.Error(t, err, string(output)) require.Empty(t, git(t, cwd, "branch", "--show-current")) } - called := false - reviewer := review.Reviewer{ - Service: review.Service{Repository: review.Repository{Dir: dir}}, - Choose: func(context.Context, *review.Decision) (string, error) { - called = true - check(dir, true) - check(other, false) - // A paused replay must still identify its branch, including in a - // linked worktree, without treating another branch's lock as its own. - pause(dir) - pause(other) - check(dir, true) - check(other, false) - git(t, dir, "rebase", "--abort") - git(t, other, "rebase", "--abort") - return "cancel", nil - }, - } - _, err = reviewer.Review(t.Context(), review.ReviewOptions{}) - require.NoError(t, err) - require.True(t, called) + // A live review holds the lock for the whole of tuicr, so a paused replay + // must still identify its branch, including in a linked worktree, without + // treating another branch's lock as its own. + started := filepath.Join(t.TempDir(), "started") + released := filepath.Join(t.TempDir(), "released") + t.Setenv("TEST_STARTED", started) + t.Setenv("TEST_RELEASED", released) + installFakeTuicr(t, `printf started > "$TEST_STARTED" +while [ ! -f "$TEST_RELEASED" ]; do sleep 0.05; done`) + reviewer := review.Reviewer{Service: review.Service{Repository: review.Repository{Dir: dir}}} + finished := make(chan error, 1) + go func() { + _, err := reviewer.Review(t.Context(), review.ReviewOptions{}) + finished <- err + }() + require.Eventually(t, func() bool { + _, err := os.Stat(started) + return err == nil + }, 5*time.Second, 10*time.Millisecond) + check(dir, true) + check(other, false) pause(dir) pause(other) - check(dir, false) + check(dir, true) check(other, false) for _, cwd := range []string{dir, other} { git(t, cwd, "rebase", "--abort") } + require.NoError(t, os.WriteFile(released, nil, 0o600)) + require.NoError(t, <-finished) + check(dir, false) + check(other, false) // A detached checkout without rebase metadata has no branch to check. git(t, dir, "checkout", "--detach") out, terminal, err := executeBinary(t, binary, dir, "status", "--is-reviewing", "--json") diff --git a/internal/cli/review/cli_test.go b/internal/cli/review/cli_test.go index 038562b..307d95f 100644 --- a/internal/cli/review/cli_test.go +++ b/internal/cli/review/cli_test.go @@ -100,7 +100,7 @@ func TestJSONErrorsIncludeArgumentAndGitFailures(t *testing.T) { for _, args := range [][]string{ {"status", "--needs-review", "--json"}, {"approve", "--json"}, - {"commits", "--recover", "--replace", "--json"}, + {"commits", "--unknown", "--json"}, {"not-a-command", "--json"}, {"--json", "status", "--needs-review", "--unknown"}, } { @@ -126,16 +126,22 @@ func TestReviewBoundsReachAutomaticSelection(t *testing.T) { state.Attempts = append(state.Attempts, review.Attempt{ID: "bounded", Stack: view.Stacks[1], Status: "incomplete", StartedAt: "2026-01-01T00:00:00Z"}) return nil })) + log := filepath.Join(t.TempDir(), "launch") + t.Setenv("TEST_LAUNCH_LOG", log) + installFakeTuicr(t, `printf '%s\n' "$4" >> "$TEST_LAUNCH_LOG"`) out, terminal, err := execute(t, dir, "commits", "--start", "HEAD", "--end", "HEAD", "--json") require.NoError(t, err) require.Empty(t, terminal) var result review.Result require.NoError(t, json.Unmarshal([]byte(out), &result)) - require.Equal(t, "decision_required", result.Status) - require.Equal(t, "bounded", result.Decision.AttemptID) + // The bounded stack resumes its captured attempt instead of asking. + require.Equal(t, "cancelled", result.Status) + launch, err := os.ReadFile(log) + require.NoError(t, err) + require.Equal(t, view.Stacks[1].Base+".."+view.Stacks[1].Tip+"\n", string(launch)) } -func TestReviewAliasProducesDecisionWithoutPromptOrStateChanges(t *testing.T) { +func TestReviewAliasResumesTheInterruptedAttempt(t *testing.T) { dir := featureRepo(t) view, err := (review.Repository{Dir: dir}).Snapshot(t.Context()) require.NoError(t, err) @@ -144,23 +150,19 @@ func TestReviewAliasProducesDecisionWithoutPromptOrStateChanges(t *testing.T) { state.Attempts = append(state.Attempts, review.Attempt{ID: "interrupted", Stack: view.Stacks[0], Status: "incomplete", StartedAt: "2026-01-01T00:00:00Z"}) return nil })) - before, err := os.ReadFile(view.Path) - require.NoError(t, err) + log := filepath.Join(t.TempDir(), "launch") + t.Setenv("TEST_LAUNCH_LOG", log) + installFakeTuicr(t, `printf '%s\n' "$4" >> "$TEST_LAUNCH_LOG"`) out, terminal, err := execute(t, dir, "challenge", "--json") require.NoError(t, err) require.Empty(t, terminal) var result review.Result require.NoError(t, json.Unmarshal([]byte(out), &result)) - require.Equal(t, "decision_required", result.Status) + require.Equal(t, "cancelled", result.Status) require.Equal(t, "commits", result.Command) - require.NotNil(t, result.Decision) - require.Equal(t, []string{"recover", "replace", "cancel"}, result.Decision.Choices) - after, err := os.ReadFile(view.Path) - require.NoError(t, err) - require.Equal(t, before, after) - out, _, err = execute(t, dir, "run") + launch, err := os.ReadFile(log) require.NoError(t, err) - require.Contains(t, out, "timber review commits --recover") + require.Equal(t, view.Stacks[0].Base+".."+view.Stacks[0].Tip+"\n", string(launch)) } func TestTerminalRenderingRetainsFeedbackAndRemovesControlSequences(t *testing.T) { @@ -227,7 +229,7 @@ func TestTuicrKeepsWorktreeLockedAfterGitReviewDies(t *testing.T) { require.Eventually(t, func() bool { _, err := os.Stat(started); return err == nil }, 5*time.Second, 10*time.Millisecond) require.NoError(t, cmd.Process.Kill()) require.Error(t, cmd.Wait()) - out, _, err := execute(t, dir, "commits", "--replace", "--json") + out, _, err := execute(t, dir, "commits", "--json") require.ErrorContains(t, err, "worktree is in use") var result display.ErrorResult require.NoError(t, json.Unmarshal([]byte(out), &result)) diff --git a/internal/cli/review/commits/commits.go b/internal/cli/review/commits/commits.go index ef14687..fb39eab 100644 --- a/internal/cli/review/commits/commits.go +++ b/internal/cli/review/commits/commits.go @@ -3,10 +3,8 @@ package commits import ( "context" - "errors" "io" - "charm.land/huh/v2" "github.com/spf13/cobra" "github.com/nnutter/timber/internal/cli/review/display" @@ -23,8 +21,6 @@ type Command struct { Start string End string Session string - Recover bool - Replace bool } // RunE runs the commits review workflow. @@ -34,12 +30,6 @@ func (c *Command) RunE(cmd *cobra.Command, args []string) error { options.Stack = args[0] c.Stack = args[0] } - if c.Recover { - options.Choice = "recover" - } - if c.Replace { - options.Choice = "replace" - } return c.runReview(cmd.Context(), options, cmd) } @@ -53,15 +43,6 @@ func (c *Command) runReview(ctx context.Context, options review.ReviewOptions, c return err } runner := review.Reviewer{Service: c.Service, Reader: reader, Input: c.Input, Terminal: c.Terminal} - if !asJSON && canPrompt(c.Input, c.Terminal) { - runner.Choose = func(ctx context.Context, decision *review.Decision) (string, error) { - choice, err := promptChoice(ctx, c.Input, c.Terminal, decision) - if errors.Is(err, huh.ErrUserAborted) { - return "cancel", nil - } - return choice, err - } - } result, err := runner.Review(ctx, options) if err != nil { return err @@ -73,13 +54,10 @@ func (c *Command) runReview(ctx context.Context, options review.ReviewOptions, c // with Install. func New(svc review.Service, in io.Reader, out, term io.Writer) *cobra.Command { c := &Command{Service: svc, Input: in, Output: out, Terminal: term} - cmd := &cobra.Command{Use: "commits []", Aliases: []string{"c", "run", "challenge"}, Short: "Review eligible stacks, or select one stack", Long: "Run tuicr in a stable detached worktree. Without a stack argument, review each\neligible stack in oldest-first order. A commit reference selects one stack. After\napproval or feedback, continue through the selected stacks. With --start or --end,\nlimit selection to the bounded range. Each eligible stack opens once per\ninvocation. Incomplete reviews, cancellation, required decisions, and errors stop\nthe command. Existing interrupted or feedback attempts require an explicit\nrecovery or replacement choice.", Args: cobra.MaximumNArgs(1), RunE: c.RunE} + cmd := &cobra.Command{Use: "commits []", Aliases: []string{"c", "run", "challenge"}, Short: "Review eligible stacks, or select one stack", Long: "Run tuicr in a stable detached worktree. Without a stack argument, review each\neligible stack in oldest-first order. A commit reference selects one stack. After\napproval or feedback, continue through the selected stacks. With --start or --end,\nlimit selection to the bounded range. Each eligible stack opens once per\ninvocation. A review resumes the attempt that captured the same stack, and it\ncarries the saved review of an unchanged patch onto rewritten commits. Incomplete\nreviews and errors stop the command.", Args: cobra.MaximumNArgs(1), RunE: c.RunE} cmd.Flags().StringVar(&c.Start, "start", "", "Review from the stack containing this commit (inclusive)") cmd.Flags().StringVar(&c.End, "end", "", "Review through the stack containing this commit (inclusive)") - cmd.Flags().BoolVar(&c.Recover, "recover", false, "Reopen the captured attempt in tuicr, preserving saved progress") - cmd.Flags().BoolVar(&c.Replace, "replace", false, "Launch a new attempt, retaining previous review history") cmd.Flags().StringVar(&c.Session, "session", "", "Select an exact saved session ID, slug, or indexed session path") - cmd.MarkFlagsMutuallyExclusive("recover", "replace") return cmd } diff --git a/internal/cli/review/commits/prompt.go b/internal/cli/review/commits/prompt.go deleted file mode 100644 index 1fc2995..0000000 --- a/internal/cli/review/commits/prompt.go +++ /dev/null @@ -1,52 +0,0 @@ -package commits - -import ( - "context" - "fmt" - "io" - "os" - - "charm.land/huh/v2" - "github.com/charmbracelet/x/term" - - "github.com/nnutter/timber/internal/cli/review/display" - "github.com/nnutter/timber/internal/timber/review" -) - -func canPrompt(input io.Reader, output io.Writer) bool { - if wrapped, ok := input.(interface{ UnwrapReader() io.Reader }); ok { - input = wrapped.UnwrapReader() - } - if wrapped, ok := output.(interface{ UnwrapWriter() io.Writer }); ok { - output = wrapped.UnwrapWriter() - } - in, ok := input.(*os.File) - if !ok { - return false - } - out, ok := output.(*os.File) - if !ok { - return false - } - return term.IsTerminal(in.Fd()) && term.IsTerminal(out.Fd()) -} - -func promptChoice(ctx context.Context, input io.Reader, output io.Writer, decision *review.Decision) (string, error) { - if decision == nil { - return "", fmt.Errorf("missing recovery decision") - } - if wrapped, ok := input.(interface{ UnwrapReader() io.Reader }); ok { - input = wrapped.UnwrapReader() - } - if wrapped, ok := output.(interface{ UnwrapWriter() io.Writer }); ok { - output = wrapped.UnwrapWriter() - } - choice := "cancel" - selectField := huh.NewSelect[string]().Title("Resume a saved review?").Description(display.RecoveryDescription(decision)).Options( - huh.NewOption("Recover — reopen tuicr with saved progress and comments", "recover"), - huh.NewOption("Replace — create a new attempt (keep prior history)", "replace"), - huh.NewOption("Cancel — keep the saved review unchanged", "cancel"), - ).Value(&choice) - err := huh.NewForm(huh.NewGroup(selectField)).WithInput(input).WithOutput(output).RunWithContext(ctx) - return choice, err -} diff --git a/internal/cli/review/commits/prompt_test.go b/internal/cli/review/commits/prompt_test.go deleted file mode 100644 index fc97630..0000000 --- a/internal/cli/review/commits/prompt_test.go +++ /dev/null @@ -1,97 +0,0 @@ -package commits - -import ( - "context" - "io" - "os" - "reflect" - "sync/atomic" - "testing" - "time" - - "github.com/charmbracelet/x/term" - "github.com/creack/pty" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "github.com/nnutter/timber/internal/timber/review" -) - -type promptReader struct{ file *os.File } - -func (r promptReader) Read(p []byte) (int, error) { return r.file.Read(p) } - -func (r promptReader) UnwrapReader() io.Reader { return r.file } - -type promptWriter struct { - file *os.File - writes atomic.Int64 -} - -func (w *promptWriter) UnwrapWriter() io.Writer { return w.file } - -func (w *promptWriter) Write(p []byte) (int, error) { - w.writes.Add(1) - return w.file.Write(p) -} - -func TestPromptChoiceUsesUnderlyingTerminal(t *testing.T) { - t.Skip("terminal dependency shutdown races under the race detector") - - master, slave, err := pty.Open() - require.NoError(t, err) - defer func() { require.NoError(t, master.Close()) }() - defer func() { require.NoError(t, slave.Close()) }() - require.NoError(t, pty.Setsize(slave, &pty.Winsize{Rows: 24, Cols: 80})) - - before, err := term.GetState(slave.Fd()) - require.NoError(t, err) - - ctx, cancel := context.WithTimeout(t.Context(), 3*time.Second) - result := make(chan struct { - choice string - err error - }, 1) - done := make(chan struct{}) - writer := &promptWriter{file: slave} - decision := &review.Decision{Attempt: review.Attempt{ - ID: "attempt", - Status: "incomplete", - Stack: review.Stack{Commits: []string{"abc"}, Tip: "abc", Subject: "Example"}, - }} - go func() { - defer close(done) - choice, err := promptChoice(ctx, promptReader{file: slave}, writer, decision) - result <- struct { - choice string - err error - }{choice: choice, err: err} - }() - defer func() { - cancel() - select { - case <-done: - case <-time.After(4 * time.Second): - assert.Fail(t, "recovery prompt did not stop after cancellation") - } - }() - - require.Eventually(t, func() bool { - current, err := term.GetState(slave.Fd()) - return err == nil && !reflect.DeepEqual(before, current) - }, time.Second, 10*time.Millisecond, "prompt did not place the terminal in raw mode") - _, err = master.Write([]byte("\r")) - require.NoError(t, err) - - select { - case got := <-result: - require.NoError(t, got.err) - require.Equal(t, "cancel", got.choice) - case <-time.After(3 * time.Second): - require.FailNow(t, "recovery prompt did not accept terminal input") - } - current, err := term.GetState(slave.Fd()) - require.NoError(t, err) - require.True(t, reflect.DeepEqual(before, current), "prompt did not restore terminal state") - require.Zero(t, writer.writes.Load(), "prompt wrote through a wrapped terminal stream") -} diff --git a/internal/cli/review/display/render.go b/internal/cli/review/display/render.go index 3ca9fcf..9195c53 100644 --- a/internal/cli/review/display/render.go +++ b/internal/cli/review/display/render.go @@ -52,10 +52,6 @@ func RenderResult(result review.Result, verbose bool) (string, error) { if message := resultMessage(result); message != "" { fmt.Fprintln(&out, message) } - if result.Decision != nil { - fmt.Fprintln(&out, RecoveryDescription(result.Decision)) - fmt.Fprintln(&out, "Choose timber review commits --recover, timber review commits --replace, or leave the attempt unchanged.") - } return strings.TrimSpace(out.String()), nil } @@ -80,59 +76,6 @@ func renderStackTable(entries []review.Entry) string { return t.Render() } -func RecoveryDescription(decision *review.Decision) string { - var out strings.Builder - attempt := decision.Attempt - fmt.Fprintf(&out, "Saved commits (inclusive): %s — %s\n", ShortStack(attempt.Stack), safeText(attempt.Stack.Subject)) - started := safeText(attempt.StartedAt) - if started == "" { - started = "unavailable" - } - fmt.Fprintf(&out, "Started: %s · Status: %s\n", started, StatusLabel(attempt.Status)) - var saved savedProgress - if !savedProgressAvailable(attempt.Review, &saved) { - fmt.Fprintln(&out, "Saved progress: unavailable") - } else { - fmt.Fprintf(&out, "Saved progress: %d/%d files reviewed · Comments: %d", *saved.Reviewed, *saved.Files, len(saved.Comments)) - if hasSessionNotes(saved.SessionNotes) { - fmt.Fprint(&out, " · session notes saved") - } - fmt.Fprintln(&out) - } - if attempt.Error != "" { - fmt.Fprintln(&out, "Last error: "+safeText(attempt.Error)) - } - if current := decision.Replacement; current != nil { - if scopeChanged(*current, attempt.Stack) { - fmt.Fprintf(&out, "New review commits (inclusive): %s — %s\n", ShortStack(*current), safeText(current.Subject)) - } - } else { - fmt.Fprintln(&out, "No current replacement scope. Select one with timber review commits .") - } - fmt.Fprint(&out, "Replacement keeps the prior attempt in history. Tuicr may reuse its saved session.") - return out.String() -} - -type savedProgress struct { - Reviewed *int `json:"reviewed"` - Files *int `json:"files"` - Comments []any `json:"comments"` - SessionNotes *string `json:"sessionNotes"` -} - -func savedProgressAvailable(data []byte, saved *savedProgress) bool { - if len(data) == 0 { - return false - } - if err := json.Unmarshal(data, saved); err != nil { - return false - } - if saved.Reviewed == nil { - return false - } - return saved.Files != nil -} - func showStatusDetails(result review.Result, verbose bool) bool { if !verbose { return false @@ -147,13 +90,6 @@ func hasSessionNotes(notes *string) bool { return *notes != "" } -func scopeChanged(current, captured review.Stack) bool { - if current.Base != captured.Base { - return true - } - return current.Tip != captured.Tip -} - func renderDetails(out *strings.Builder, entries []review.Entry) error { for _, entry := range entries { if entry.Attempt != nil { @@ -278,8 +214,6 @@ func resultMessage(result review.Result) string { return "Feedback requires another review after the stack changes." case "incomplete": return "Review is incomplete, not approved." - case "decision_required": - return "A previous attempt requires an explicit recovery or replacement choice." case "cancelled": return "Review cancelled. The attempt remains unchanged." default: diff --git a/internal/cli/review/feedback_test.go b/internal/cli/review/feedback_test.go index 580fbb0..859fd26 100644 --- a/internal/cli/review/feedback_test.go +++ b/internal/cli/review/feedback_test.go @@ -111,7 +111,6 @@ func TestFeedbackDoesNotLaunchOrRecoverReviews(t *testing.T) { require.NoError(t, json.Unmarshal([]byte(out), &result)) require.Equal(t, "idle", result.Status) require.Empty(t, result.Feedback) - require.Nil(t, result.Decision) after, err := os.ReadFile(view.Path) require.NoError(t, err) require.Equal(t, before, after) diff --git a/internal/cli/review/recovery_test.go b/internal/cli/review/recovery_test.go index a60fd19..579117b 100644 --- a/internal/cli/review/recovery_test.go +++ b/internal/cli/review/recovery_test.go @@ -1,7 +1,6 @@ package cli import ( - "encoding/json/jsontext" "encoding/json/v2" "os" "path/filepath" @@ -9,55 +8,9 @@ import ( "github.com/stretchr/testify/require" - "github.com/nnutter/timber/internal/cli/review/display" "github.com/nnutter/timber/internal/timber/review" ) -func TestRecoveryDecisionExplainsSavedWork(t *testing.T) { - for _, tc := range []struct { - name string - saved jsontext.Value - want string - replacement string - }{ - {"saved progress", jsontext.Value(`{"reviewed":1,"files":2,"comments":[{"content":"Fix this"}],"sessionNotes":"Check this"}`), "Saved progress: 1/2 files reviewed · Comments: 1 · session notes saved", "changed"}, - {"missing progress", nil, "Saved progress: unavailable", "same"}, - {"missing counters", jsontext.Value(`{"comments":[]}`), "Saved progress: unavailable", "same"}, - {"no replacement", nil, "Saved progress: unavailable", "none"}, - } { - t.Run(tc.name, func(t *testing.T) { - saved := review.Stack{Base: "base", Tip: "bbbbbbbbbbbb", Commits: []string{"aaaaaaaaaaaa", "bbbbbbbbbbbb"}, Subject: "Fix recovery\x1b[31m\nhandling"} - current := review.Stack{Base: "base", Tip: "cccccccccccc", Commits: []string{"cccccccccccc"}, Subject: "Revised recovery"} - result := review.Result{Command: "commits", Status: "decision_required", Decision: &review.Decision{ - AttemptID: "internal-id", - Attempt: review.Attempt{Stack: saved, Status: "incomplete", StartedAt: "2026-10-16T12:00:00Z", Review: tc.saved}, - Replacement: ¤t, - }} - switch tc.replacement { - case "same": - result.Decision.Replacement = &saved - case "none": - result.Decision.Replacement = nil - } - text, err := display.RenderResult(result, false) - require.NoError(t, err) - require.Contains(t, text, "Saved commits (inclusive): aaaaaaaa..bbbbbbbb — Fix recovery handling") - require.Contains(t, text, "Started: 2026-10-16T12:00:00Z · Status: Incomplete") - require.Contains(t, text, tc.want) - if tc.replacement == "changed" { - require.Contains(t, text, "New review commits (inclusive): cccccccc — Revised recovery") - } else { - require.NotContains(t, text, "New review commits") - } - if tc.replacement == "none" { - require.Contains(t, text, "No current replacement scope. Select one with timber review commits .") - } - require.Contains(t, text, "Replacement keeps the prior attempt in history. Tuicr may reuse its saved session.") - require.NotContains(t, text, "internal-id") - }) - } -} - // installFakeTuicr puts a scripted tuicr on PATH so a review can run without a // terminal. func installFakeTuicr(t *testing.T, body string) { @@ -89,8 +42,7 @@ func TestReviewDiscardsAttemptWhoseStackNoLongerExists(t *testing.T) { require.NoError(t, err) var result review.Result require.NoError(t, json.Unmarshal([]byte(out), &result)) - require.NotEqual(t, "decision_required", result.Status) - require.Nil(t, result.Decision) + require.Equal(t, "cancelled", result.Status, "the review started on the current stack") state, err := store.Load() require.NoError(t, err) require.NotContains(t, attemptIDs(state), interrupted.ID) diff --git a/internal/cli/review/tuicr_functional_test.go b/internal/cli/review/tuicr_functional_test.go index 531be27..bc45077 100644 --- a/internal/cli/review/tuicr_functional_test.go +++ b/internal/cli/review/tuicr_functional_test.go @@ -144,17 +144,19 @@ func TestRealTuicrInterruptedRecovery(t *testing.T) { } return !saved[0].Active }, 5*time.Second, 20*time.Millisecond) - before := runRealCommand(t, binary, dir, env, "commits", "--json") - var decision struct { - Status string `json:"status"` - Decision struct { - AttemptID string `json:"attemptId"` - } `json:"decision"` + before := runRealCommand(t, binary, dir, env, "status", "--needs-review", "--json") + var interrupted struct { + Entries []struct { + Attempt struct { + ID string `json:"id"` + } `json:"attempt"` + } `json:"entries"` } - require.NoError(t, json.Unmarshal([]byte(before), &decision)) - require.Equal(t, "decision_required", decision.Status) - require.NotEmpty(t, decision.Decision.AttemptID) - resumed := startTerminal(t, binary, dir, env, "commits", "HEAD", "--recover", "--json") + require.NoError(t, json.Unmarshal([]byte(before), &interrupted)) + require.Len(t, interrupted.Entries, 1) + require.NotEmpty(t, interrupted.Entries[0].Attempt.ID) + // The next review resumes the captured attempt without asking. + resumed := startTerminal(t, binary, dir, env, "commits", "HEAD", "--json") resumed.waitFor(t, "feature.txt") resumed.waitFor(t, "1/2") resumed.send(t, ":set noreviewed\rr") @@ -176,86 +178,11 @@ func TestRealTuicrInterruptedRecovery(t *testing.T) { require.NoError(t, json.Unmarshal([]byte(out), &result)) require.Equal(t, "approved", result.Status) require.Len(t, result.Entries, 1) - require.Equal(t, decision.Decision.AttemptID, result.Entries[0].Attempt.ID) + require.Equal(t, interrupted.Entries[0].Attempt.ID, result.Entries[0].Attempt.ID) require.True(t, result.Entries[0].Attempt.Review.Complete) require.Equal(t, 2, result.Entries[0].Attempt.Review.Reviewed) } -func TestRealTuicrRecoveryPrompt(t *testing.T) { - for _, choice := range []string{"recover", "replace", "cancel"} { - t.Run(choice, func(t *testing.T) { - env := realTuicrEnvironment(t) - binary := buildGitReview(t) - dir := realFeatureRepo(t) - initial := startTerminal(t, binary, dir, env, "commits", "--json") - initial.waitFor(t, "feature.txt") - initial.send(t, ":set noreviewed\rr") - initial.waitFor(t, "1/2") - initial.send(t, ":w\r:q!\r") - before := initial.waitExit(t) - var prior struct { - Status string `json:"status"` - Entries []struct { - Attempt struct { - ID string `json:"id"` - } `json:"attempt"` - } `json:"entries"` - } - require.NoError(t, json.Unmarshal([]byte(before), &prior)) - require.Equal(t, "incomplete", prior.Status) - require.Len(t, prior.Entries, 1) - prompt := startTerminal(t, binary, dir, env, "commits") - prompt.waitFor(t, "Resume a saved review?") - prompt.waitFor(t, "Saved commits (inclusive):") - prompt.waitFor(t, "Saved progress: 1/2 files reviewed") - prompt.waitFor(t, "Replace — create a new attempt (keep prior history)") - switch choice { - case "recover": - prompt.send(t, "kk\r") - prompt.waitFor(t, "feature.txt") - prompt.waitFor(t, "1/2") - prompt.send(t, ":set noreviewed\rr") - prompt.waitFor(t, "All files reviewed") - prompt.send(t, ":wq\r") - case "replace": - prompt.send(t, "k\r") - prompt.waitFor(t, "feature.txt") - prompt.send(t, ":set noreviewed\rr") - prompt.waitFor(t, "All files reviewed") - prompt.send(t, ":wq\r") - case "cancel": - prompt.send(t, "\r") - } - out := prompt.waitExit(t) - switch choice { - case "recover", "replace": - require.Contains(t, out, "No reviews pending in the selected range.") - case "cancel": - require.Contains(t, out, "Review cancelled. The attempt remains unchanged.") - } - after := runRealCommand(t, binary, dir, env, "status", "--needs-review", "--json") - var result struct { - Status string `json:"status"` - Entries []struct { - Status string `json:"status"` - Attempt struct { - ID string `json:"id"` - } `json:"attempt"` - } `json:"entries"` - } - require.NoError(t, json.Unmarshal([]byte(after), &result)) - if choice != "cancel" { - require.Equal(t, "idle", result.Status) - require.Empty(t, result.Entries) - } else { - require.Len(t, result.Entries, 1) - require.Equal(t, "incomplete", result.Entries[0].Status) - require.Equal(t, prior.Entries[0].Attempt.ID, result.Entries[0].Attempt.ID) - } - }) - } -} - func TestRealTuicrCleanReview(t *testing.T) { env := realTuicrEnvironment(t) binary := buildGitReview(t) @@ -292,3 +219,37 @@ func TestRealTuicrCleanReview(t *testing.T) { require.NoError(t, json.Unmarshal([]byte(listed), &status)) require.Equal(t, "idle", status.Status) } + +// A reword keeps the patch, so the review that the reword interrupted moves its +// saved marks onto the new commits and asks only for the new message. +func TestRealTuicrCarriesReviewOverRewordedCommits(t *testing.T) { + env := realTuicrEnvironment(t) + binary := buildGitReview(t) + dir := realFeatureRepo(t) + initial := startTerminal(t, binary, dir, env, "commits", "--json") + initial.waitFor(t, "feature.txt") + // Review the file, not the message: a reword resets the message mark. + initial.send(t, "]r") + initial.waitFor(t, "1/2") + initial.send(t, ":w\r:q!\r") + before := initial.waitExit(t) + var interrupted struct { + Status string `json:"status"` + } + require.NoError(t, json.Unmarshal([]byte(before), &interrupted)) + require.Equal(t, "incomplete", interrupted.Status) + + git(t, dir, "commit", "--amend", "--no-edit", "-m", "Carry the feature again\n\nChange-Id: I"+strings.Repeat("a", 40)) + carried := startTerminal(t, binary, dir, env, "commits", "--json") + carried.waitFor(t, "Carry the feature again") + carried.waitFor(t, "1/2") + carried.send(t, ":set noreviewed\rr") + carried.waitFor(t, "All files reviewed") + carried.send(t, ":wq\r") + out := carried.waitExit(t) + var result struct { + Status string `json:"status"` + } + require.NoError(t, json.Unmarshal([]byte(out), &result)) + require.Equal(t, "range_complete", result.Status) +} diff --git a/internal/timber/review/auto_split.go b/internal/timber/review/auto_split.go index 2d7217b..36066d0 100644 --- a/internal/timber/review/auto_split.go +++ b/internal/timber/review/auto_split.go @@ -74,7 +74,7 @@ func runAuto(ctx context.Context, s Service, evaluator autoreview.Evaluator, opt return result, err } if len(activeAttempts(view, state)) != 0 { - return result, fmt.Errorf("recover or replace the active review before automatic approval") + return result, fmt.Errorf("finish the active review before automatic approval") } selected, err := (Reviewer{Service: s}).bounds(ctx, view, options.Start, options.End) if err != nil { diff --git a/internal/timber/review/auto_test.go b/internal/timber/review/auto_test.go index 3bdb7e1..24185c7 100644 --- a/internal/timber/review/auto_test.go +++ b/internal/timber/review/auto_test.go @@ -315,5 +315,5 @@ func TestAutoRequiresNoActiveReview(t *testing.T) { require.Fail(t, "an active review must block automatic approval") return autoreview.Verdict{}, nil }), AutoOptions{}) - require.ErrorContains(t, err, "recover or replace the active review") + require.ErrorContains(t, err, "finish the active review") } diff --git a/internal/timber/review/bounds_test.go b/internal/timber/review/bounds_test.go index 4ec611c..3d4740e 100644 --- a/internal/timber/review/bounds_test.go +++ b/internal/timber/review/bounds_test.go @@ -67,7 +67,7 @@ func TestReviewSelectsWithinInclusiveBounds(t *testing.T) { } func TestBoundedReviewContinuesUntilApprovalOrUserInput(t *testing.T) { - for _, outcome := range []string{"approved", "feedback", "incomplete", "cancelled", "decision_required", "error"} { + for _, outcome := range []string{"approved", "feedback", "incomplete", "cancelled", "error"} { t.Run(outcome, func(t *testing.T) { reviewer, _, firstStore := testReviewer(t) r := reviewer.Service.Repository @@ -99,9 +99,6 @@ func TestBoundedReviewContinuesUntilApprovalOrUserInput(t *testing.T) { prior := newAttempt(view.Stacks[1], "incomplete") require.NoError(t, stateStore.Update(func(state *State) error { state.Attempts = append(state.Attempts, prior) - if outcome == "decision_required" { - state.Attempts = append(state.Attempts, newAttempt(view.Stacks[2], "incomplete")) - } return nil })) log := filepath.Join(t.TempDir(), "launch") @@ -112,7 +109,7 @@ func TestBoundedReviewContinuesUntilApprovalOrUserInput(t *testing.T) { } fakeTuicr(t, `printf '%s\n' "$4" >> "$TEST_LAUNCH_LOG" if [ "$(git rev-parse HEAD)" = "$TEST_FAIL_TIP" ]; then exit 1; fi`) - result, err := reviewer.Review(t.Context(), ReviewOptions{Start: first.Tip, End: second.Tip, Choice: "recover", Session: "fixture-session"}) + result, err := reviewer.Review(t.Context(), ReviewOptions{Start: first.Tip, End: second.Tip, Session: "fixture-session"}) if outcome == "error" { require.ErrorContains(t, err, "tuicr interrupted") } else { @@ -125,10 +122,7 @@ if [ "$(git rev-parse HEAD)" = "$TEST_FAIL_TIP" ]; then exit 1; fi`) } launch, err := os.ReadFile(log) require.NoError(t, err) - wantLaunch := first.Base + ".." + first.Tip + "\n" - if outcome != "decision_required" { - wantLaunch += second.Base + ".." + second.Tip + "\n" - } + wantLaunch := first.Base + ".." + first.Tip + "\n" + second.Base + ".." + second.Tip + "\n" require.Equal(t, wantLaunch, string(launch)) state, err := stateStore.Load() require.NoError(t, err) @@ -185,14 +179,16 @@ func TestBoundedReviewRetainsFeedbackAndContinuesToLaterStacks(t *testing.T) { require.Contains(t, string(state.Attempts[i].Review), "Please revise") } } - // Feedback remains unresolved and needs an explicit choice on a later invocation. + // A later review resumes the captured feedback attempt without asking. result, err = reviewer.Review(t.Context(), options) require.NoError(t, err) - require.Equal(t, "decision_required", result.Status) - require.Equal(t, state.Attempts[0].ID, result.Decision.AttemptID) + require.Equal(t, "feedback", result.Status) launch, err = os.ReadFile(log) require.NoError(t, err) - require.Equal(t, wantLaunch, string(launch)) + // The resumed run continues through the stacks whose feedback is unresolved. + resumed := view.Stacks[0].Base + ".." + view.Stacks[0].Tip + "\n" + resumed += view.Stacks[1].Base + ".." + view.Stacks[1].Tip + "\n" + require.Equal(t, wantLaunch+resumed, string(launch)) } func TestReviewRejectsInvalidBoundsWithoutStateChanges(t *testing.T) { @@ -241,17 +237,13 @@ func TestBoundedReviewRetainsDeferredAndActiveAttempts(t *testing.T) { state.Attempts = append(state.Attempts, active) return nil })) - result, err = reviewer.Review(t.Context(), options) - require.NoError(t, err) - require.Equal(t, "decision_required", result.Status) - require.Equal(t, active.ID, result.Decision.AttemptID) path, err := WorktreePath(r.Dir) require.NoError(t, err) scope := first scope.Repo = path savedFixture(t, scope, store, true) fakeTuicr(t, "exit 0") - options.Choice = "recover" + // The active attempt resumes without asking, and the deferred stack stays. result, err = reviewer.Review(t.Context(), options) require.NoError(t, err) require.Equal(t, "no_waiting", result.Status) diff --git a/internal/timber/review/carry_test.go b/internal/timber/review/carry_test.go index c94c718..58c47d9 100644 --- a/internal/timber/review/carry_test.go +++ b/internal/timber/review/carry_test.go @@ -151,7 +151,7 @@ func TestReviewRecordsCarriedProgress(t *testing.T) { gitTest(t, reviewer.Service.Repository.Dir, "commit", "--amend", "--allow-empty", "--date", "2026-01-01T00:00:00Z", "-m", "Feature reworded") fakeTuicr(t, "exit 0") - result, err := reviewer.Review(t.Context(), ReviewOptions{Choice: "replace"}) + result, err := reviewer.Review(t.Context(), ReviewOptions{}) require.NoError(t, err) require.Equal(t, "feedback", result.Status) require.Len(t, result.Entries, 1) diff --git a/internal/timber/review/run.go b/internal/timber/review/run.go index 1ee2a8a..28d1942 100644 --- a/internal/timber/review/run.go +++ b/internal/timber/review/run.go @@ -16,7 +16,6 @@ import ( // ReviewOptions selects a current stack and an explicit recovery choice. type ReviewOptions struct { Stack string - Choice string Session string Start string End string @@ -35,8 +34,6 @@ type Reviewer struct { Reader SavedReader Input io.Reader Terminal io.Writer - // Choose resolves recovery decisions while the review lock remains held. - Choose func(context.Context, *Decision) (string, error) } // Review continues automatic reviews after completed attempts, retaining feedback. @@ -64,20 +61,6 @@ func (r Reviewer) Review(ctx context.Context, options ReviewOptions) (result Res defer stopMaintenance() carrySavedReviews(ctx, r.Service.Repository, r.Reader) r.discardUnusable(ctx) - result, err = r.reviewSequence(ctx, options, path, lock) - if err != nil { - return result, err - } - if result.Status != "decision_required" { - return result, nil - } - if r.Choose == nil { - return result, nil - } - options.Choice, err = r.Choose(ctx, result.Decision) - if err != nil { - return result, err - } return r.reviewSequence(ctx, options, path, lock) } @@ -222,10 +205,7 @@ func (r Reviewer) execute(ctx context.Context, plan reviewPlan, options ReviewOp if plan.stack == nil { return result, fmt.Errorf("no stack selected") } - if options.Choice == "recover" { - if plan.prior == nil { - return result, fmt.Errorf("no previous attempt to recover") - } + if plan.prior != nil && sameStack(plan.prior.Stack, *plan.stack) { return r.resume(ctx, path, plan, options.Session, lock) } replacement, err := replacementStack(plan) @@ -331,9 +311,6 @@ func (r Reviewer) recover(plan reviewPlan, attempt Attempt, selector string) (Re return result, nil } result := Result{Version: 1, Command: "commits", Status: finished.Status, View: plan.view, Entries: []Entry{{Stack: finished.Stack, Status: finished.Status, Attempt: &finished}}, Active: []Attempt{}} - if finished.Status == "incomplete" { - result.Decision = recoveryDecision(plan, finished) - } return result, nil } @@ -378,8 +355,7 @@ func (r Reviewer) reviewSequence(ctx context.Context, options ReviewOptions, pat return result, nil } visited[result.Entries[0].Stack.Key] = true - // Recovery and session selection apply only to the initial attempt. - options.Choice = "" + // Session selection applies only to the initial attempt. options.Session = "" } } @@ -400,24 +376,6 @@ func (r Reviewer) reviewStack(ctx context.Context, options ReviewOptions, visite } return result, nil } - if plan.prior != nil { - if options.Choice == "" { - result.Status = "decision_required" - result.Decision = recoveryDecision(plan, *plan.prior) - return result, nil - } - } else { - if options.Choice == "recover" { - return result, fmt.Errorf("no previous attempt to recover") - } - } - if options.Choice == "cancel" { - result.Status = "cancelled" - return result, nil - } - if !slices.Contains([]string{"", "recover", "replace"}, options.Choice) { - return result, fmt.Errorf("invalid review choice %q", options.Choice) - } return r.execute(ctx, plan, options, path, lock) } @@ -609,19 +567,6 @@ func recordFailure(path, id string, cause error) error { }) } -func recoveryDecision(plan reviewPlan, attempt Attempt) *Decision { - decision := &Decision{ - AttemptID: attempt.ID, - StackKey: attempt.Stack.Key, - Choices: []string{"recover", "replace", "cancel"}, - Attempt: attempt, - } - if stack, err := replacementStack(plan); err == nil { - decision.Replacement = &stack - } - return decision -} - func replacementStack(plan reviewPlan) (Stack, error) { if plan.stack == nil { return Stack{}, fmt.Errorf("no stack selected") diff --git a/internal/timber/review/run_test.go b/internal/timber/review/run_test.go index 13843d8..8b61474 100644 --- a/internal/timber/review/run_test.go +++ b/internal/timber/review/run_test.go @@ -1,7 +1,6 @@ package review import ( - "context" "encoding/json/v2" "fmt" "io" @@ -9,6 +8,7 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/stretchr/testify/require" ) @@ -124,42 +124,47 @@ func TestReviewContinuesThroughEligibleStacksByDefault(t *testing.T) { } } -func TestInterruptedReviewRecoveryAndReplacement(t *testing.T) { - for _, choice := range []string{"recover", "replace"} { - t.Run(choice, func(t *testing.T) { +// A review resumes the attempt that captured the same stack, and a stack that a +// fixup changed starts a new attempt. +func TestInterruptedReviewResumesTheCapturedAttempt(t *testing.T) { + for _, tc := range []struct { + name string + amend bool + wantStatus string + wantSame bool + }{ + {name: "same stack resumes", wantStatus: "range_complete", wantSame: true}, + {name: "changed stack replaces", amend: true, wantStatus: "range_complete", wantSame: false}, + } { + t.Run(tc.name, func(t *testing.T) { reviewer, view, store := testReviewer(t) fakeTuicr(t, "exit 1") _, err := reviewer.Review(t.Context(), ReviewOptions{}) require.ErrorContains(t, err, "tuicr interrupted") - result, err := reviewer.Review(t.Context(), ReviewOptions{}) - require.NoError(t, err) - require.Equal(t, "decision_required", result.Status) - require.NotNil(t, result.Decision) - require.Equal(t, []string{"recover", "replace", "cancel"}, result.Decision.Choices) - before, err := (Store{Path: view.Path}).Load() - require.NoError(t, err) - require.Len(t, before.Attempts, 1) - require.Equal(t, before.Attempts[0], result.Decision.Attempt) - require.NotNil(t, result.Decision.Replacement) - require.Equal(t, view.Stacks[0], *result.Decision.Replacement) path, err := WorktreePath(view.Stacks[0].Repo) require.NoError(t, err) scope := view.Stacks[0] scope.Repo = path + if tc.amend { + gitTest(t, reviewer.Service.Repository.Dir, "commit", "--allow-empty", "--fixup=HEAD") + rewritten, err := reviewer.Service.Repository.Snapshot(t.Context()) + require.NoError(t, err) + scope = rewritten.Stacks[0] + scope.Repo = path + } savedFixture(t, scope, store, true) + before, err := (Store{Path: view.Path}).Load() + require.NoError(t, err) + require.Len(t, before.Attempts, 1) fakeTuicr(t, "exit 0") - result, err = reviewer.Review(t.Context(), ReviewOptions{Choice: choice}) + result, err := reviewer.Review(t.Context(), ReviewOptions{}) require.NoError(t, err) - require.Equal(t, "range_complete", result.Status) + require.Equal(t, tc.wantStatus, result.Status) after, err := (Store{Path: view.Path}).Load() require.NoError(t, err) - if choice == "recover" { - require.Len(t, after.Attempts, 1) - require.Equal(t, before.Attempts[0].ID, after.Attempts[0].ID) - } else { - require.Len(t, after.Attempts, 2) - require.Equal(t, "incomplete", after.Attempts[0].Status) - } + require.Len(t, after.Attempts, 1) + resumed := after.Attempts[0].ID == before.Attempts[0].ID + require.Equal(t, tc.wantSame, resumed) }) } } @@ -180,15 +185,16 @@ func TestInterruptedReviewResumesSavedProgress(t *testing.T) { writeJSON(t, filepath.Join(store, "sessions/0123456789abcdef.json"), session) fakeTuicr(t, "exit 0") - result, err := reviewer.Review(t.Context(), ReviewOptions{Choice: "recover"}) + before, err := (Store{Path: view.Path}).Load() + require.NoError(t, err) + result, err := reviewer.Review(t.Context(), ReviewOptions{}) require.NoError(t, err) require.Equal(t, "incomplete", result.Status) - require.NotNil(t, result.Decision) - require.Equal(t, result.Decision.AttemptID, result.Entries[0].Attempt.ID) + require.Equal(t, before.Attempts[0].ID, result.Entries[0].Attempt.ID) state, err := (Store{Path: view.Path}).Load() require.NoError(t, err) require.Len(t, state.Attempts, 1, "recovery must not discard a resumable attempt") - require.Equal(t, result.Decision.AttemptID, state.Attempts[0].ID) + require.Equal(t, before.Attempts[0].ID, state.Attempts[0].ID) var saved SavedReview require.NoError(t, json.Unmarshal(state.Attempts[0].Review, &saved)) require.Equal(t, "incomplete", saved.Status()) @@ -212,7 +218,6 @@ func TestReviewDiscardsUntouchedExit(t *testing.T) { result, err := reviewer.Review(t.Context(), ReviewOptions{}) require.NoError(t, err) require.Equal(t, "cancelled", result.Status) - require.Nil(t, result.Decision) state, err := (Store{Path: view.Path}).Load() require.NoError(t, err) require.Empty(t, state.Attempts) @@ -271,42 +276,33 @@ func TestDeferredReviewsRemainPending(t *testing.T) { require.Equal(t, "incomplete", result.Entries[0].Status) } -func TestReviewHoldsActivityLockDuringRecoveryChoice(t *testing.T) { - reviewer, view, _ := testReviewer(t) - require.NoError(t, (Store{Path: view.Path}).Update(func(state *State) error { - state.Attempts = append(state.Attempts, newAttempt(view.Stacks[0], "reviewing")) - return nil - })) - called := false - reviewer.Choose = func(ctx context.Context, decision *Decision) (string, error) { - called = true - require.NotNil(t, decision) - running, err := reviewer.Service.ReviewRunning(ctx) - require.NoError(t, err) - require.True(t, running, "waiting for user input is part of the live review") - return "cancel", nil - } - result, err := reviewer.Review(t.Context(), ReviewOptions{}) - require.NoError(t, err) - require.True(t, called) - require.Equal(t, "cancelled", result.Status) +// A review holds the worktree lock for the whole of tuicr, so nothing else can +// start a review, approve, or rewrite the state while the human reviews. +func TestReviewHoldsActivityLockDuringLaunch(t *testing.T) { + reviewer, _, _ := testReviewer(t) + started := filepath.Join(t.TempDir(), "started") + released := filepath.Join(t.TempDir(), "released") + t.Setenv("TEST_STARTED", started) + t.Setenv("TEST_RELEASED", released) + fakeTuicr(t, `printf started > "$TEST_STARTED" +while [ ! -f "$TEST_RELEASED" ]; do sleep 0.05; done`) + finished := make(chan error, 1) + go func() { + _, err := reviewer.Review(t.Context(), ReviewOptions{}) + finished <- err + }() + require.Eventually(t, func() bool { + _, err := os.Stat(started) + return err == nil + }, 5*time.Second, 10*time.Millisecond) running, err := reviewer.Service.ReviewRunning(t.Context()) require.NoError(t, err) - require.False(t, running) -} - -func TestReviewRefusesConcurrentWorktreeUse(t *testing.T) { - reviewer, view, _ := testReviewer(t) - path, err := WorktreePath(view.Stacks[0].Repo) - require.NoError(t, err) - lock, err := lockWorktree(path) + require.True(t, running, "the launch is part of the live review") + require.NoError(t, os.WriteFile(released, nil, 0o600)) + require.NoError(t, <-finished) + running, err = reviewer.Service.ReviewRunning(t.Context()) require.NoError(t, err) - defer func() { require.NoError(t, lock.Close()) }() - _, err = reviewer.Review(t.Context(), ReviewOptions{}) - require.ErrorContains(t, err, "worktree is in use") - state, err := (Store{Path: view.Path}).Load() - require.NoError(t, err) - require.Empty(t, state.Attempts) + require.False(t, running) } func TestRecoverablePriorIgnoresRecordedModelResults(t *testing.T) { diff --git a/internal/timber/review/service.go b/internal/timber/review/service.go index 7650409..fd4307a 100644 --- a/internal/timber/review/service.go +++ b/internal/timber/review/service.go @@ -26,7 +26,6 @@ type Result struct { View Snapshot `json:"view"` Entries []Entry `json:"entries"` Active []Attempt `json:"active"` - Decision *Decision `json:"decision,omitzero"` Feedback []FeedbackRecord `json:"feedback,omitempty"` FileFeedback []FilesResult `json:"fileFeedback,omitempty"` Summary []StateCount `json:"summary,omitempty"` @@ -38,16 +37,6 @@ type FeedbackRecord struct { Historical bool `json:"historical"` } -// Decision requires an explicit choice; it is not review approval. -type Decision struct { - AttemptID string `json:"attemptId"` - StackKey string `json:"stackKey"` - Choices []string `json:"choices"` - Attempt Attempt `json:"attempt"` - // Replacement is the current scope a new attempt would review, if available. - Replacement *Stack `json:"replacement,omitzero"` -} - // StateCount counts current feature commits in one review state. type StateCount struct { State string `json:"state"` @@ -396,7 +385,7 @@ func activeAttempts(view Snapshot, state State) []Attempt { func approvalAllowed(selected []Stack, view Snapshot, state State) error { if len(activeAttempts(view, state)) > 0 { - return fmt.Errorf("recover or replace the active review before approval") + return fmt.Errorf("finish the active review before approval") } for _, stack := range selected { a := state.latest(stack) From 294d5a8ad415c558a847958fdb1df5bfa0e60215 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 5 Nov 2026 02:42:01 -0600 Subject: [PATCH 6/7] Add timber review reset A review that resumes automatically needs an explicit way to start over, and a state that no command can change is the trap that a rewrite used to create. Reset clears the attempts and saved sessions of one stack, selected by a commit or by inclusive commit_stack endpoints, or of every stack in the feature range when it has no argument. It holds the review lock, so it cannot clear state under a live review, and both views then agree on an empty state. Change-Id: I5bf34c0e32e3c9e0710cc5999405c2c852e88be1 --- internal/cli/review/cli_test.go | 41 +++++++++++++++ internal/cli/review/display/render.go | 2 + internal/cli/review/reset/reset.go | 50 ++++++++++++++++++ internal/cli/review/root/root.go | 2 + internal/timber/review/service.go | 74 +++++++++++++++++++++++++++ 5 files changed, 169 insertions(+) create mode 100644 internal/cli/review/reset/reset.go diff --git a/internal/cli/review/cli_test.go b/internal/cli/review/cli_test.go index 307d95f..7cb5c4a 100644 --- a/internal/cli/review/cli_test.go +++ b/internal/cli/review/cli_test.go @@ -241,6 +241,7 @@ func TestHelpAndVersion(t *testing.T) { require.NoError(t, err) require.Contains(t, out, "approve") require.Contains(t, out, "commits") + require.Contains(t, out, "reset") require.Contains(t, out, "files") require.Contains(t, out, "--json") require.NotContains(t, out, "confirm") @@ -257,3 +258,43 @@ func TestHelpAndVersion(t *testing.T) { require.NoError(t, err) require.Contains(t, out, "test-version") } + +func TestResetClearsSelectedStacks(t *testing.T) { + dir := featureRepo(t) + git(t, dir, "commit", "--allow-empty", "-m", "Second\n\nChange-Id: I"+strings.Repeat("b", 40)) + view, err := (review.Repository{Dir: dir}).Snapshot(t.Context()) + require.NoError(t, err) + store := review.Store{Path: view.Path} + attempts := func(state *review.State) error { + state.Attempts = nil + for i, stack := range view.Stacks { + state.Attempts = append(state.Attempts, review.Attempt{ID: stack.Key, Stack: stack, Status: "approved", StartedAt: "2026-01-01T00:00:00Z", ApprovalSource: "user-command"}) + _ = i + } + return nil + } + require.NoError(t, store.Update(attempts)) + out, _, err := execute(t, dir, "reset", "HEAD", "--json") + require.NoError(t, err) + var result review.Result + require.NoError(t, json.Unmarshal([]byte(out), &result)) + require.Equal(t, "reset", result.Status) + state, err := store.Load() + require.NoError(t, err) + require.Len(t, state.Attempts, 1, "reset clears only the selected stack") + require.Equal(t, view.Stacks[0].Key, state.Attempts[0].ID) + + require.NoError(t, store.Update(attempts)) + out, _, err = execute(t, dir, "reset", "--json") + require.NoError(t, err) + require.NoError(t, json.Unmarshal([]byte(out), &result)) + require.Equal(t, "reset", result.Status) + state, err = store.Load() + require.NoError(t, err) + require.Empty(t, state.Attempts, "reset without a selector clears the branch") + out, _, err = execute(t, dir, "status", "--needs-review", "--json") + require.NoError(t, err) + var listed review.Result + require.NoError(t, json.Unmarshal([]byte(out), &listed)) + require.Equal(t, "pending", listed.Status) +} diff --git a/internal/cli/review/display/render.go b/internal/cli/review/display/render.go index 9195c53..a641cb7 100644 --- a/internal/cli/review/display/render.go +++ b/internal/cli/review/display/render.go @@ -216,6 +216,8 @@ func resultMessage(result review.Result) string { return "Review is incomplete, not approved." case "cancelled": return "Review cancelled. The attempt remains unchanged." + case "reset": + return "Review state cleared." default: return "" } diff --git a/internal/cli/review/reset/reset.go b/internal/cli/review/reset/reset.go new file mode 100644 index 0000000..e3c83aa --- /dev/null +++ b/internal/cli/review/reset/reset.go @@ -0,0 +1,50 @@ +// Package reset clears the review state of stacks. +package reset + +import ( + "io" + + "github.com/spf13/cobra" + + "github.com/nnutter/timber/internal/cli/review/display" + "github.com/nnutter/timber/internal/timber/review" +) + +// Command houses flag values and positional arguments for the reset review. +type Command struct { + Service review.Service + Output io.Writer +} + +// RunE runs the reset workflow. +func (c *Command) RunE(cmd *cobra.Command, args []string) error { + asJSON := false + if flag, err := cmd.Flags().GetBool("json"); err == nil { + asJSON = flag + } + selector := "" + if len(args) > 0 { + selector = args[0] + } + reader, err := review.DefaultSavedReader() + if err != nil { + return err + } + result, err := c.Service.Reset(cmd.Context(), reader, selector) + if err != nil { + return err + } + return display.Write(c.Output, asJSON, result, false) +} + +// New builds the reset sub-command. Callers install it onto the parent with +// Install. +func New(svc review.Service, out io.Writer) *cobra.Command { + c := &Command{Service: svc, Output: out} + return &cobra.Command{Use: "reset []", Aliases: []string{"clear"}, Short: "Clear the review state of stacks", Long: "Clear the saved review state without launching tuicr. A commit reference or\ninclusive commit_stack endpoints select one stack, and an invocation without an\nargument clears every stack in the feature range. Saved sessions for the cleared\nstacks are deleted, so tuicr and the agent start from the same empty state.", Args: cobra.MaximumNArgs(1), RunE: c.RunE} +} + +// Install adds the reset sub-command to parent. +func Install(parent *cobra.Command, svc review.Service, out io.Writer) { + parent.AddCommand(New(svc, out)) +} diff --git a/internal/cli/review/root/root.go b/internal/cli/review/root/root.go index 2430ef9..fb13cf7 100644 --- a/internal/cli/review/root/root.go +++ b/internal/cli/review/root/root.go @@ -21,6 +21,7 @@ import ( "github.com/nnutter/timber/internal/cli/review/feedback" "github.com/nnutter/timber/internal/cli/review/files" "github.com/nnutter/timber/internal/cli/review/log" + "github.com/nnutter/timber/internal/cli/review/reset" "github.com/nnutter/timber/internal/cli/review/status" "github.com/nnutter/timber/internal/display" coreconfig "github.com/nnutter/timber/internal/timber/config" @@ -62,6 +63,7 @@ func (c *Command) build() *cobra.Command { feedback.Install(root, c.Service, c.Output) files.Install(root, c.Service, c.Input, c.Output, c.Terminal) log.Install(root, c.Service, c.Input, c.Output, c.Terminal) + reset.Install(root, c.Service, c.Output) status.Install(root, c.Service, c.Output) return root } diff --git a/internal/timber/review/service.go b/internal/timber/review/service.go index fd4307a..b752b8a 100644 --- a/internal/timber/review/service.go +++ b/internal/timber/review/service.go @@ -5,6 +5,7 @@ import ( "encoding/json/v2" "errors" "fmt" + "slices" "strings" "github.com/nnutter/timber/internal/timber/review/autoreview" @@ -158,6 +159,57 @@ func (s Service) Feedback(ctx context.Context) (Result, error) { return result, nil } +// Reset clears the review state of the selected stacks, or of every stack in +// the feature range when no selector is given. It removes the attempts that a +// stack can show and the saved sessions that name them, so tuicr and the agent +// start clean. +func (s Service) Reset(ctx context.Context, reader SavedReader, selector string) (Result, error) { + view, err := s.Repository.Snapshot(ctx) + if err != nil { + return Result{}, err + } + selected, err := s.resetStacks(ctx, view, selector) + if err != nil { + return Result{}, err + } + path, err := WorktreePath(s.Repository.Dir) + if err != nil { + return Result{}, err + } + lock, err := lockWorktree(path) + if err != nil { + return Result{}, err + } + defer func() { + if closeErr := lock.Close(); closeErr != nil { + s.Repository.logger().Warn("Review lock close failed", "error", closeErr) + } + }() + store := Store{Path: view.Path} + if err := store.Update(func(state *State) error { + state.Attempts = slices.DeleteFunc(state.Attempts, func(attempt Attempt) bool { + return resetsAttempt(attempt, selected) + }) + return nil + }); err != nil { + return Result{}, err + } + writer := SavedWriter(reader) + for _, stack := range selected { + if _, err := writer.Delete(stack); err != nil { + return Result{}, err + } + } + state, err := store.Load() + if err != nil { + return Result{}, err + } + result := makeResult("reset", view, state) + result.Status = "reset" + result.Entries = nil + return result, nil +} + // ReviewRunning reports whether the current or rebasing branch owns the live review lock. // It does not create the lock file or infer activity from persisted attempts. func (s Service) ReviewRunning(ctx context.Context) (bool, error) { @@ -308,6 +360,18 @@ func (s Service) recordAutomatic(ctx context.Context, view Snapshot, entry *Auto return nil } +// resetStacks returns the stacks that a reset selects. +func (s Service) resetStacks(ctx context.Context, view Snapshot, selector string) ([]Stack, error) { + if selector == "" { + return view.Stacks, nil + } + stack, err := selectStack(ctx, s.Repository, view, selector) + if err != nil { + return nil, err + } + return []Stack{stack}, nil +} + func (s Service) selectRange(ctx context.Context, view Snapshot, first, last string) ([]Stack, error) { startSHA, err := s.Repository.Resolve(ctx, first) if err != nil { @@ -459,3 +523,13 @@ func resultFromEntries(command string, view Snapshot, state State, entries []Ent } return result } + +// resetsAttempt reports whether an attempt describes a stack that reset clears. +func resetsAttempt(attempt Attempt, selected []Stack) bool { + for _, stack := range selected { + if sameStack(attempt.Stack, stack) || attempt.matches(stack) || sharesLineage(attempt.Stack, stack) { + return true + } + } + return false +} From fbe41866a283f6c5cba62e4997749e70c69b92b6 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Thu, 5 Nov 2026 02:44:16 -0600 Subject: [PATCH 7/7] Document the automatic review recovery The skill told agents that recovery needs the user's choice and that --recover exists. Both are gone: a review resumes the attempt that captured the same stack, a matching patch carries its saved review onto rewritten commits, and reset clears the state when the user wants a clean start. Change-Id: I228617b704eeaece0b4639f41cf1967a2ee47e20 --- skills/timber-review/SKILL.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/skills/timber-review/SKILL.md b/skills/timber-review/SKILL.md index 79d9393..807fa81 100644 --- a/skills/timber-review/SKILL.md +++ b/skills/timber-review/SKILL.md @@ -126,8 +126,9 @@ Check live activity with `timber review status --is-reviewing --json` before rew When `is_reviewing` is true, a `commits` or `files` review holds the worktree lock. Ask the user to finish the live review instead of rewriting underneath it. This command does not collect uncaptured notes from interrupted tuicr sessions. -Do not use `timber review commits --recover` to retrieve feedback. -Recovery opens an interactive review and requires the user's choice. +Do not use `timber review commits` to retrieve feedback: it opens the interactive review, and it resumes the attempt that captured the same stack. +A rewrite that keeps the patch moves the saved review onto the current stack, so an interrupted review keeps its marks and comments after a rebase or reword. +When the stack must start from an empty state, ask the user to run `timber review reset`, which takes a commit or a commit_stack and defaults to the whole feature range. ## Discuss questions before making changes