From 31e2b8716d141effd750f4f7301e778b0a6b8138 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Wed, 21 Oct 2026 14:54:01 -0500 Subject: [PATCH 1/5] Refactor: Extract checkout discovery for reuse in file feedback Separate branch and shared state path discovery from upstream resolution to prepare for file feedback retrieval without a configured upstream. --- internal/review/git.go | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/internal/review/git.go b/internal/review/git.go index 5f01218..fc95428 100644 --- a/internal/review/git.go +++ b/internal/review/git.go @@ -150,26 +150,15 @@ func (r Repository) Snapshot(ctx context.Context) (Snapshot, error) { } func (r Repository) branchView(ctx context.Context) (Snapshot, string, error) { - var view Snapshot - repo, err := r.git(ctx, "rev-parse", "--show-toplevel") - if err != nil { - return view, "", err - } - branch, err := r.git(ctx, "symbolic-ref", "--quiet", "--short", "HEAD") - if err != nil { - return view, "", fmt.Errorf("review requires a branch, not detached HEAD: %w", err) - } - common, err := r.git(ctx, "rev-parse", "--path-format=absolute", "--git-common-dir") + view, repo, err := r.checkoutView(ctx) if err != nil { return view, "", err } target, err := r.git(ctx, "rev-parse", "--symbolic-full-name", "@{upstream}") if err != nil { - return view, "", fmt.Errorf("branch %s requires a configured Git upstream: %w", branch, err) + return view, "", fmt.Errorf("branch %s requires a configured Git upstream: %w", view.Branch, err) } - view.Branch = branch view.Target = target - view.Path = filepath.Join(common, "tuicr-reviews", fmt.Sprintf("%x.json", sha256.Sum256([]byte(branch)))) return view, repo, nil } @@ -191,6 +180,25 @@ func (r Repository) changeID(ctx context.Context, sha string, ids map[string]boo return id, nil } +func (r Repository) checkoutView(ctx context.Context) (Snapshot, string, error) { + var view Snapshot + repo, err := r.git(ctx, "rev-parse", "--show-toplevel") + if err != nil { + return view, "", err + } + branch, err := r.git(ctx, "symbolic-ref", "--quiet", "--short", "HEAD") + if err != nil { + return view, "", fmt.Errorf("review requires a branch, not detached HEAD: %w", err) + } + common, err := r.git(ctx, "rev-parse", "--path-format=absolute", "--git-common-dir") + if err != nil { + return view, "", err + } + view.Branch = branch + view.Path = filepath.Join(common, "tuicr-reviews", fmt.Sprintf("%x.json", sha256.Sum256([]byte(branch)))) + return view, repo, nil +} + func (r Repository) git(ctx context.Context, args ...string) (string, error) { out, err := r.gitOutput(ctx, args...) return strings.TrimSpace(out), err From 6c7b15c00c589bfb3e2a4823d61e4f758e87a81d Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Wed, 21 Oct 2026 15:02:29 -0500 Subject: [PATCH 2/5] Include saved file-review notes in feedback Read file-review notes from tuicr so feedback does not omit sessions that have no git-review commit attempt. Keep file feedback separate and label it so agents do not treat repository-scoped notes as commit-stack feedback that requires fixup commits. File notes remain relevant across branch changes until resolved or removed in tuicr. Retrieving them without commit attempts must not require an upstream branch or linear history. --- README.md | 21 +++-- internal/cli/files_functional_test.go | 46 +++++++++- internal/display/render.go | 8 ++ internal/review/file_feedback.go | 122 ++++++++++++++++++++++++++ internal/review/file_feedback_test.go | 55 ++++++++++++ internal/review/saved.go | 37 ++++++++ internal/review/service.go | 98 +++++++++++++++------ skills/git-review/SKILL.md | 14 ++- 8 files changed, 360 insertions(+), 41 deletions(-) create mode 100644 internal/review/file_feedback.go create mode 100644 internal/review/file_feedback_test.go diff --git a/README.md b/README.md index f5bdb54..be7d65a 100644 --- a/README.md +++ b/README.md @@ -171,9 +171,9 @@ git-review feedback git-review feedback --json ``` -This read-only command returns notes persisted by git-review, including reviews completed outside an agent session. +This read-only command returns commit-review notes persisted by git-review and file-review notes from tuicr, including existing saved sessions. It does not launch tuicr, recover an attempt, or change either review store. -Interrupted sessions whose notes were not captured by git-review still require explicit interactive recovery. +Interrupted commit-review sessions whose notes were not captured by git-review still require explicit interactive recovery. Feedback results use the versioned result envelope, with `"command":"feedback"`. The optional `feedback` array contains records with these fields: @@ -189,9 +189,16 @@ Superseded attempts, changed or removed stacks, and addressed attempts remain hi The command does not infer that historical feedback needs another fix or that a changed stack resolved it. Malformed stored review data is an error rather than silently omitted feedback. -The result is `pending` when current saved feedback exists and `idle` otherwise. +The optional `fileFeedback` array contains file-review results with `"command":"files"`, the saved session path, comments, and session notes. +It includes local sessions in the feature checkout and its annotation directories, not commit-review sessions in the detached checkout. +These records are repository-scoped and remain visible after branch changes while their tuicr sessions contain notes. +They do not imply current stack feedback or authorize fixup commits. +Address them as new commits, logically organized and sized. +Resolve or remove obsolete notes in tuicr when they no longer apply. + +The result is `pending` when current commit feedback or any saved file feedback exists and `idle` otherwise. An `idle` result can still include historical records. -The `feedback` field is omitted when no saved notes exist, and `entries` is empty. +Each feedback array is omitted when it has no records, and `entries` is empty. Active attempts remain visible in `active`, but collecting feedback does not resume them. ### Review a stack @@ -305,7 +312,8 @@ Use `--session` to select an exact saved session when several file reviews match File reviews run in the feature checkout, not the detached review worktree. `--all`, explicit paths, and `--untracked` do not require a configured upstream. They hold the same repository-wide worktree lock as `commits` reviews, so `status --is-reviewing` reports true during either review. -They do not create git-review attempts and do not affect `status` counts or `feedback` records. +They do not create git-review attempts or affect `status` counts. +Retrieve their comments and session notes with `git-review feedback`, in the separate `fileFeedback` JSON array or the terminal “File feedback” section. `approve` applies only to commit stacks, not file reviews. Successful `files` output uses `"command":"files"` to distinguish it from `"command":"commits"`. @@ -368,7 +376,8 @@ Successful results other than `status` contain: | `entries` | Unresolved stacks, or the stacks selected by approval/review | | `active` | Persisted active attempts | | `decision` | Required explicit choice, when present | -| `feedback` | Current and historical saved feedback records for the feedback command, when notes exist | +| `feedback` | Current and historical commit-review feedback records, when notes exist | +| `fileFeedback` | Saved file-review results from tuicr, when notes exist | Needs-review outcomes are `pending` or `idle`. Review outcomes include `approved`, `feedback`, `incomplete`, `decision_required`, `cancelled`, `idle`, `range_complete`, and `no_waiting`. diff --git a/internal/cli/files_functional_test.go b/internal/cli/files_functional_test.go index 988997b..316fa7c 100644 --- a/internal/cli/files_functional_test.go +++ b/internal/cli/files_functional_test.go @@ -15,12 +15,14 @@ func TestRealTuicrFileScopes(t *testing.T) { env := realTuicrEnvironment(t) binary := buildGitReview(t) for _, tc := range []struct { - name string - args []string - cwd string - previous bool + name string + args []string + cwd string + previous bool + noUpstream bool }{ {name: "nested file", args: []string{"src/nested.txt"}}, + {name: "annotation without upstream", args: []string{"src/nested.txt"}, noUpstream: true}, {name: "nested file after root review", args: []string{"src/nested.txt"}, previous: true}, {name: "nested directory", args: []string{"src"}, previous: true}, {name: "untracked from subdirectory", args: []string{"--untracked"}, cwd: "src", previous: true}, @@ -37,11 +39,15 @@ func TestRealTuicrFileScopes(t *testing.T) { if tc.previous { reviewRealFile(t, binary, dir, env, "feature.txt", "Prior root feedback", "feature.txt") } + if tc.noUpstream { + git(t, dir, "branch", "--unset-upstream") + } result := reviewRealFile(t, binary, filepath.Join(dir, tc.cwd), env, "nested.txt", "Nested feedback", tc.args...) require.Equal(t, dir, result.Repo) require.Len(t, result.Review.Comments, 1) require.Equal(t, "Nested feedback", result.Review.Comments[0]["content"]) require.Equal(t, "src/nested.txt", result.Review.Comments[0]["path"]) + checkRealFileFeedback(t, binary, filepath.Join(dir, tc.cwd), env, result) }) } } @@ -88,10 +94,42 @@ func TestRealTuicrBranchFiles(t *testing.T) { require.Len(t, result.Review.Comments, 1) require.Equal(t, "Branch feedback", result.Review.Comments[0]["content"]) require.Equal(t, "feature.txt", result.Review.Comments[0]["path"]) + git(t, dir, "commit", "--allow-empty", "-m", "Advance branch after file review") + checkRealFileFeedback(t, binary, filepath.Join(dir, tc.cwd), env, result) }) } } +func checkRealFileFeedback(t *testing.T, binary, dir string, env []string, reviewed review.FilesResult) { + t.Helper() + before, err := os.ReadFile(reviewed.Session) + require.NoError(t, err) + out := runRealCommand(t, binary, dir, env, "feedback", "--json") + var result struct { + Status string `json:"status"` + Feedback []review.FeedbackRecord `json:"feedback"` + FileFeedback []review.FilesResult `json:"fileFeedback"` + } + require.NoError(t, json.Unmarshal([]byte(out), &result)) + require.Equal(t, "pending", result.Status) + require.Empty(t, result.Feedback, "file feedback must not become a stack attempt") + var found bool + for _, file := range result.FileFeedback { + if file.Session == reviewed.Session { + found = true + require.Equal(t, "files", file.Command) + require.Equal(t, reviewed.Review, file.Review) + } + } + require.True(t, found, "feedback must include the saved file review") + terminal := runRealCommand(t, binary, dir, env, "feedback") + require.Contains(t, terminal, "File feedback:") + require.Contains(t, terminal, reviewed.Review.Comments[0]["content"]) + after, err := os.ReadFile(reviewed.Session) + require.NoError(t, err) + require.Equal(t, before, after, "feedback must not change the tuicr session") +} + func reviewRealFile(t *testing.T, binary, dir string, env []string, file, feedback string, args ...string) review.FilesResult { t.Helper() command := append([]string{"files", "--json"}, args...) diff --git a/internal/display/render.go b/internal/display/render.go index 948b6e6..c0c4b1c 100644 --- a/internal/display/render.go +++ b/internal/display/render.go @@ -181,6 +181,14 @@ func renderFeedbackResult(result review.Result) (string, error) { if err := renderFeedback(&out, result.Feedback); err != nil { return "", err } + for _, file := range result.FileFeedback { + fmt.Fprintln(&out, "File feedback:") + text, err := renderFilesResult(file) + if err != nil { + return "", err + } + fmt.Fprintln(&out, text) + } if result.Status == "idle" { fmt.Fprintln(&out, "No current saved feedback.") } else { diff --git a/internal/review/file_feedback.go b/internal/review/file_feedback.go new file mode 100644 index 0000000..b5e9724 --- /dev/null +++ b/internal/review/file_feedback.go @@ -0,0 +1,122 @@ +package review + +import ( + "fmt" + "path/filepath" + "strings" +) + +func readFileFeedbackEntry(store, repo string, entry object) (*FilesResult, error) { + canonical, relativeScope, err := localFileFeedbackScope(repo, entry) + if err != nil { + return nil, err + } + if canonical == "" { + return nil, nil + } + session, path, err := readFileFeedbackSession(store, entry) + if err != nil { + return nil, err + } + source, err := fileFeedbackSource(session, relativeScope) + if err != nil { + return nil, err + } + if source == "" { + return nil, nil + } + if err := validateSessionRepo(session, filepath.Clean(canonical)); err != nil { + return nil, err + } + saved, err := decodeFileFeedbackSession(session, canonical, source) + if err != nil { + return nil, err + } + if saved.Status() != "feedback" { + return nil, nil + } + result := FilesResult{Version: 1, Command: "files", Repo: repo, Session: path, AllFiles: source == "pristine"} + if relativeScope != "." { + result.Path = filepath.ToSlash(relativeScope) + } + result, err = fileReviewResult(result, saved) + return &result, err +} + +func localFileFeedbackScope(repo string, entry object) (string, string, error) { + kind, err := decodeRequired[object](entry, "kind") + if err != nil { + return "", "", err + } + entryType, err := decodeRequired[string](kind, "type") + if err != nil { + return "", "", err + } + if entryType != "local" { + return "", "", nil + } + canonical, err := decodeRequired[string](entry, "canonical_repo_path") + if err != nil { + return "", "", err + } + relativeScope, err := filepath.Rel(repo, canonical) + if err != nil { + return "", "", err + } + if relativeScope == ".." || strings.HasPrefix(relativeScope, ".."+string(filepath.Separator)) { + return "", "", nil + } + return canonical, relativeScope, nil +} + +func readFileFeedbackSession(store string, entry object) (object, string, error) { + relative, err := decodeRequired[string](entry, "path") + if err != nil { + return nil, "", err + } + if !sessionPathPattern.MatchString(relative) { + return nil, "", fmt.Errorf("unsafe session path %q", relative) + } + path, err := safeSessionPath(store, relative) + if err != nil { + return nil, "", err + } + session, err := readObject(path) + return session, path, err +} + +func fileFeedbackSource(session object, relativeScope string) (string, error) { + if raw, ok := session["pr_session_key"]; ok { + if string(raw) != "null" { + return "", nil + } + } + source, err := decodeRequired[string](session, "diff_source") + if err != nil { + return "", err + } + switch source { + case "pristine", "working_tree": + return source, nil + case "commit_range": + // Branch diffs launched by files always use the feature repository root. + if relativeScope == "." { + return source, nil + } + } + return "", nil +} + +func decodeFileFeedbackSession(session object, canonical, source string) (*SavedReview, error) { + if source != "commit_range" { + return decodeFileSession(session, canonical) + } + commits, err := decodeRequired[[]string](session, "commit_range") + if err != nil { + return nil, err + } + if err := validateCommits(commits); err != nil { + return nil, err + } + return decodeSession(session, Stack{Repo: canonical, Commits: commits}) +} diff --git a/internal/review/file_feedback_test.go b/internal/review/file_feedback_test.go new file mode 100644 index 0000000..4229b47 --- /dev/null +++ b/internal/review/file_feedback_test.go @@ -0,0 +1,55 @@ +package review + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestFileFeedbackDiscoversNotesWithoutChangingSessions(t *testing.T) { + r := testRepo(t) + store := t.TempDir() + path := filepath.Join(store, "sessions/0123456789abcdef.json") + session := fileFixture(t, r.Dir, "working_tree", "file", store, false, "sessions/0123456789abcdef.json") + session["session_notes"] = "Check the public API" + writeJSON(t, path, session) + before, err := os.ReadFile(path) + require.NoError(t, err) + reader := SavedReader{Stores: []string{store, store}} + feedback, err := reader.FileFeedback(r.Dir) + require.NoError(t, err) + require.Len(t, feedback, 1, "duplicate index discovery must not repeat feedback") + require.Equal(t, "feedback", feedback[0].Status) + require.Empty(t, feedback[0].Review.Comments) + require.Equal(t, "Check the public API", *feedback[0].Review.SessionNotes) + after, err := os.ReadFile(path) + require.NoError(t, err) + require.Equal(t, before, after) + + // A different repository must not inherit this annotation, even if its + // directory name starts with the same prefix. + other := r.Dir + "-other" + require.NoError(t, os.Mkdir(other, 0o700)) + t.Cleanup(func() { _ = os.Remove(other) }) + otherStore := t.TempDir() + otherSession := fileFixture(t, other, "working_tree", "file", otherStore, false, "sessions/0123456789abcdef.json") + otherSession["session_notes"] = "Unrelated feedback" + writeJSON(t, filepath.Join(otherStore, "sessions/0123456789abcdef.json"), otherSession) + feedback, err = (SavedReader{Stores: []string{otherStore}}).FileFeedback(r.Dir) + require.NoError(t, err) + require.Empty(t, feedback) + + delete(session, "session_notes") + writeJSON(t, path, session) + feedback, err = reader.FileFeedback(r.Dir) + require.NoError(t, err) + require.Empty(t, feedback, "incomplete reviews without notes are not feedback") + + // Corrupt notes must be reported, not mistaken for absent feedback. + session["session_notes"] = 42 + writeJSON(t, path, session) + _, err = reader.FileFeedback(r.Dir) + require.ErrorContains(t, err, "session_notes") +} diff --git a/internal/review/saved.go b/internal/review/saved.go index 5b2b77d..31dc0c4 100644 --- a/internal/review/saved.go +++ b/internal/review/saved.go @@ -1,6 +1,7 @@ package review import ( + "cmp" "encoding/json/jsontext" "encoding/json/v2" "errors" @@ -50,6 +51,42 @@ func (s *SavedReview) Status() string { // SavedReader reads indexed sessions without modifying the tuicr store. type SavedReader struct{ Stores []string } +// FileFeedback reads local sessions in the feature checkout and its annotation +// directories. Commit reviews use a separate detached checkout, not this scope. +// Sessions remain available after branch changes and are not stack approvals. +func (r SavedReader) FileFeedback(repo string) ([]FilesResult, error) { + repo, err := filepath.EvalSymlinks(repo) + if err != nil { + return nil, err + } + var results []FilesResult + seen := map[string]bool{} + for _, store := range r.Stores { + entries, err := readIndex(store) + if err != nil { + return nil, err + } + for _, rows := range entries { + for _, entry := range rows { + result, err := readFileFeedbackEntry(store, repo, entry) + if err != nil { + return nil, err + } + if result == nil { + continue + } + if seen[result.Session] { + continue + } + seen[result.Session] = true + results = append(results, *result) + } + } + } + slices.SortFunc(results, func(a, b FilesResult) int { return cmp.Compare(a.Session, b.Session) }) + return results, nil +} + // Read returns nil only when no matching session exists. Corruption is an error. func (r SavedReader) Read(scope Stack, selector string) (*SavedReview, error) { if err := validateCommits(scope.Commits); err != nil { diff --git a/internal/review/service.go b/internal/review/service.go index b532b0c..bc5c4f1 100644 --- a/internal/review/service.go +++ b/internal/review/service.go @@ -16,15 +16,16 @@ type Entry struct { // Result is the shared input to JSON encoding and terminal rendering. type Result struct { - Version int `json:"version"` - Command string `json:"command"` - Status string `json:"status"` - View Snapshot `json:"view"` - Entries []Entry `json:"entries"` - Active []Attempt `json:"active"` - Decision *Decision `json:"decision,omitzero"` - Feedback []FeedbackRecord `json:"feedback,omitempty"` - Summary []StateCount `json:"summary,omitempty"` + Version int `json:"version"` + Command string `json:"command"` + Status string `json:"status"` + 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"` } // FeedbackRecord retains captured scope and marks obsolete or addressed notes. @@ -69,6 +70,27 @@ func countStates(view Snapshot, state State) []StateCount { return counts } +func appendCommitFeedback(result *Result, state State) error { + for _, attempt := range state.Attempts { + if len(attempt.Review) == 0 { + continue + } + var saved SavedReview + if err := json.Unmarshal(attempt.Review, &saved); err != nil { + return fmt.Errorf("invalid stored review %s: %w", attempt.ID, err) + } + if saved.Status() != "feedback" { + continue + } + historical := !currentAttempt(result.View, state, attempt) + result.Feedback = append(result.Feedback, FeedbackRecord{Attempt: attempt, Historical: historical}) + if !historical { + result.Status = "pending" + } + } + return nil +} + // Service coordinates the current branch with its durable review history. type Service struct{ Repository Repository } @@ -113,9 +135,9 @@ func (s Service) Approve(ctx context.Context, first, last string) (Result, error return result, nil } -// Feedback reads every persisted attempt with notes without changing either store. +// Feedback reads saved commit and file feedback without changing either store. func (s Service) Feedback(ctx context.Context) (Result, error) { - view, err := s.Repository.Snapshot(ctx) + view, repo, err := s.Repository.checkoutView(ctx) if err != nil { return Result{}, err } @@ -123,23 +145,24 @@ 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)} - for _, attempt := range state.Attempts { - if len(attempt.Review) == 0 { - continue - } - var saved SavedReview - if err := json.Unmarshal(attempt.Review, &saved); err != nil { - return Result{}, fmt.Errorf("invalid stored review %s: %w", attempt.ID, err) - } - if saved.Status() != "feedback" { - continue - } - historical := !currentAttempt(view, state, attempt) - result.Feedback = append(result.Feedback, FeedbackRecord{Attempt: attempt, Historical: historical}) - if !historical { - result.Status = "pending" - } + view, err = s.feedbackView(ctx, view, state) + if err != nil { + return Result{}, err + } + reader, err := DefaultSavedReader() + if err != nil { + return Result{}, err + } + files, err := reader.FileFeedback(repo) + if err != nil { + return Result{}, err + } + result := Result{Version: 1, Command: "feedback", Status: "idle", View: view, Entries: []Entry{}, Active: activeAttempts(state), FileFeedback: files} + if len(files) > 0 { + result.Status = "pending" + } + if err := appendCommitFeedback(&result, state); err != nil { + return Result{}, err } return result, nil } @@ -176,6 +199,25 @@ func (s Service) Status(ctx context.Context, needsReview bool) (Result, error) { return result, nil } +// File annotation does not require an upstream or linear commit history. +// Preserve stack validation when there are commit-review attempts to classify. +func (s Service) feedbackView(ctx context.Context, view Snapshot, state State) (Snapshot, error) { + if len(state.Attempts) > 0 { + return s.Repository.Snapshot(ctx) + } + head, err := s.Repository.Resolve(ctx, "HEAD") + if err != nil { + return Snapshot{}, err + } + target, err := s.Repository.git(ctx, "for-each-ref", "--format=%(upstream)", "refs/heads/"+view.Branch) + if err != nil { + return Snapshot{}, err + } + view.Head = head + view.Target = target + return view, 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 { diff --git a/skills/git-review/SKILL.md b/skills/git-review/SKILL.md index cdc9ede..dad2ea7 100644 --- a/skills/git-review/SKILL.md +++ b/skills/git-review/SKILL.md @@ -75,11 +75,19 @@ Inspect the JSON result, not only the exit code. Stop on command errors or an unsupported output version. The supported output version is `1`. -Read the `feedback` array rather than `entries`. -The array is absent when no saved notes exist. +Read both `feedback` and `fileFeedback`, not `entries`. +Each array is absent when it has no records. +The `feedback` array contains commit-review feedback. Each record contains an `attempt` and a `historical` boolean. -For each selected attempt, read: +The `fileFeedback` array contains saved file reviews with `"command":"files"`. +Read each result’s `review.comments` and `review.sessionNotes`. +These repository-scoped sessions can predate the current branch state. +Check whether their notes still apply before changing code. +Address file feedback as new commits, not `fixup!`, `amend!`, or `reword!` commits. +Do not treat file feedback as a commit-review attempt or stack approval. + +For each selected commit-review attempt, read: - `id`: identifies the feedback attempt. - `stack`: contains the captured commits, tip, subject, and optional Change-Id. From 3be0fd3669afe5c1cce6eb77db8a00e167f35e36 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Wed, 21 Oct 2026 15:05:07 -0500 Subject: [PATCH 3/5] Fix macOS test failures by canonicalizing fixture paths On macOS, temporary fixture paths use `/var`, while Git and the saved-session reader use the equivalent `/private/var` path. Canonicalize fixture repository directories and the real-tuicr home so saved paths and session assertions match the production contract. --- internal/cli/cli_test.go | 3 ++- internal/cli/tuicr_functional_test.go | 3 ++- internal/review/git_test.go | 4 +++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 0c60bcb..e55aee9 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -28,7 +28,8 @@ func git(t *testing.T, dir string, args ...string) string { func featureRepo(t *testing.T) string { t.Helper() - dir := t.TempDir() + dir, err := filepath.EvalSymlinks(t.TempDir()) + require.NoError(t, err) git(t, dir, "init", "-b", "main") git(t, dir, "config", "user.name", "Test") git(t, dir, "config", "user.email", "test@example.com") diff --git a/internal/cli/tuicr_functional_test.go b/internal/cli/tuicr_functional_test.go index 83dba4f..531be27 100644 --- a/internal/cli/tuicr_functional_test.go +++ b/internal/cli/tuicr_functional_test.go @@ -21,7 +21,8 @@ func realTuicrEnvironment(t *testing.T) []string { } _, err := exec.LookPath("tuicr") require.NoError(t, err, "functional tests require tuicr on PATH") - home := t.TempDir() + home, err := filepath.EvalSymlinks(t.TempDir()) + require.NoError(t, err) overrides := map[string]string{ "HOME": home, "XDG_DATA_HOME": filepath.Join(home, "data"), "XDG_CACHE_HOME": filepath.Join(home, "cache"), "XDG_CONFIG_HOME": filepath.Join(home, "config"), "TERM": "xterm-256color", "COLORTERM": "truecolor", "GIT_CONFIG_GLOBAL": os.DevNull, "GIT_CONFIG_NOSYSTEM": "1", diff --git a/internal/review/git_test.go b/internal/review/git_test.go index c9db9b4..5019279 100644 --- a/internal/review/git_test.go +++ b/internal/review/git_test.go @@ -3,6 +3,7 @@ package review import ( "fmt" "os/exec" + "path/filepath" "strings" "testing" @@ -20,7 +21,8 @@ func gitTest(t *testing.T, dir string, args ...string) string { func testRepo(t *testing.T) Repository { t.Helper() - dir := t.TempDir() + dir, err := filepath.EvalSymlinks(t.TempDir()) + require.NoError(t, err) gitTest(t, dir, "init", "-b", "main") gitTest(t, dir, "config", "user.name", "Test") gitTest(t, dir, "config", "user.email", "test@example.com") From 931a82ec12c08456ddf44a7c132f6e6ea84bd6a5 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Wed, 21 Oct 2026 15:47:31 -0500 Subject: [PATCH 4/5] Refactor: Reorder FileReviewer methods Keep this refactor separate because upstream merge `7c3162d` already includes the code. Combining it with the original feature commit would rewrite history outside the current feature range. --- internal/review/files.go | 88 ++++++++++++++++++++-------------------- 1 file changed, 44 insertions(+), 44 deletions(-) diff --git a/internal/review/files.go b/internal/review/files.go index 5227a50..8a8e324 100644 --- a/internal/review/files.go +++ b/internal/review/files.go @@ -133,50 +133,6 @@ func (r FileReviewer) Review(ctx context.Context, options FilesOptions) (FilesRe return fileReviewResult(result, saved) } -// reviewBranchDiff uses Git's upstream merge-base without imposing the linear -// history and stack-placement rules required by commit reviews. -func (r FileReviewer) reviewBranchDiff(ctx context.Context, repo, selector string, lock *os.File) (FilesResult, error) { - result := FilesResult{Version: 1, Command: "files", Repo: repo} - repository := r.Service.Repository - view, _, err := repository.branchView(ctx) - if err != nil { - return result, err - } - head, err := repository.Resolve(ctx, "HEAD") - if err != nil { - return result, err - } - base, err := repository.git(ctx, "merge-base", "--all", view.Target, head) - if err != nil { - return result, err - } - if strings.Contains(base, "\n") { - return result, fmt.Errorf("review requires one merge-base with upstream") - } - history, err := repository.git(ctx, "rev-list", "--reverse", base+".."+head) - if err != nil { - return result, err - } - if history == "" { - return fileReviewResult(result, nil) - } - // Run at the root so invocation from a subdirectory keeps the same scope - // and saved-session identity. - r.Service.Repository.Dir = repo - if err := r.launch(ctx, []string{"tui", "--no-update-check", "--stdout", "--revisions", base + ".." + head}, lock); err != nil { - return result, err - } - scope := Stack{Repo: repo, Base: base, Tip: head, Commits: strings.Split(history, "\n")} - saved, err := r.Reader.Read(scope, selector) - if err != nil { - return result, err - } - if saved != nil { - result.Session = saved.Path - } - return fileReviewResult(result, saved) -} - func (r FileReviewer) launch(ctx context.Context, args []string, lock *os.File) error { cmd := exec.CommandContext(ctx, "tuicr", args...) cmd.Dir = r.Service.Repository.Dir @@ -225,6 +181,50 @@ func (r FileReviewer) readReview(path, diffSource, selector string) (*SavedRevie return r.Reader.ReadFile(root, diffSource, selector) } +// reviewBranchDiff uses Git's upstream merge-base without imposing the linear +// history and stack-placement rules required by commit reviews. +func (r FileReviewer) reviewBranchDiff(ctx context.Context, repo, selector string, lock *os.File) (FilesResult, error) { + result := FilesResult{Version: 1, Command: "files", Repo: repo} + repository := r.Service.Repository + view, _, err := repository.branchView(ctx) + if err != nil { + return result, err + } + head, err := repository.Resolve(ctx, "HEAD") + if err != nil { + return result, err + } + base, err := repository.git(ctx, "merge-base", "--all", view.Target, head) + if err != nil { + return result, err + } + if strings.Contains(base, "\n") { + return result, fmt.Errorf("review requires one merge-base with upstream") + } + history, err := repository.git(ctx, "rev-list", "--reverse", base+".."+head) + if err != nil { + return result, err + } + if history == "" { + return fileReviewResult(result, nil) + } + // Run at the root so invocation from a subdirectory keeps the same scope + // and saved-session identity. + r.Service.Repository.Dir = repo + if err := r.launch(ctx, []string{"tui", "--no-update-check", "--stdout", "--revisions", base + ".." + head}, lock); err != nil { + return result, err + } + scope := Stack{Repo: repo, Base: base, Tip: head, Commits: strings.Split(history, "\n")} + saved, err := r.Reader.Read(scope, selector) + if err != nil { + return result, err + } + if saved != nil { + result.Session = saved.Path + } + return fileReviewResult(result, saved) +} + // Tuicr keys --file sessions by the canonical parent of a file or by the // selected directory. --all-files uses the canonical current directory. func fileSessionRoot(dir, path string) (string, error) { From d23f145b0fdddaa581171044fc7684e69d80105d Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Wed, 21 Oct 2026 15:48:39 -0500 Subject: [PATCH 5/5] Refactor: Extract FileReviewer file-scope validation and selection Keep this as a standalone baseline refactor because the original files command changes merged upstream in 7c3162d. No target remains on the current branch, so this avoids rewriting merged history. --- internal/review/files.go | 39 +++++++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/internal/review/files.go b/internal/review/files.go index 8a8e324..7377674 100644 --- a/internal/review/files.go +++ b/internal/review/files.go @@ -75,11 +75,8 @@ type FileReviewer struct { // Review launches tuicr for working-tree files and reads its saved session. func (r FileReviewer) Review(ctx context.Context, options FilesOptions) (FilesResult, error) { var result FilesResult - if options.All && (options.Untracked || options.Path != "") { - return result, fmt.Errorf("--all cannot be combined with --untracked or an explicit path") - } - if conflictingFileScope(options) { - return result, fmt.Errorf("--untracked cannot be combined with an explicit path") + if err := validateFileScope(options); err != nil { + return result, err } repo, err := r.Service.Repository.git(ctx, "rev-parse", "--show-toplevel") if err != nil { @@ -95,7 +92,7 @@ func (r FileReviewer) Review(ctx context.Context, options FilesOptions) (FilesRe _ = lock.Close() }() - if !options.All && !options.Untracked && options.Path == "" { + if reviewBranchFiles(options) { return r.reviewBranchDiff(ctx, repo, options.Session, lock) } allFiles := options.All @@ -289,6 +286,36 @@ func makeFileCommentsRepoRelative(saved *SavedReview, repo string) error { return nil } +func validateFileScope(options FilesOptions) error { + if conflictingAllFileScope(options) { + return fmt.Errorf("--all cannot be combined with --untracked or an explicit path") + } + if conflictingFileScope(options) { + return fmt.Errorf("--untracked cannot be combined with an explicit path") + } + return nil +} + +func conflictingAllFileScope(options FilesOptions) bool { + if !options.All { + return false + } + if options.Untracked { + return true + } + return options.Path != "" +} + +func reviewBranchFiles(options FilesOptions) bool { + if options.All { + return false + } + if options.Untracked { + return false + } + return options.Path == "" +} + func conflictingFileScope(options FilesOptions) bool { if !options.Untracked { return false