From 27be52ab8b7a4c87de786449dc60f72fa7419b43 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Mon, 19 Oct 2026 07:06:44 -0500 Subject: [PATCH 01/22] Refactor: Format upstream imports and struct fields Keep formatter corrections separate so later method moves and helper extractions contain no unrelated whitespace or import grouping changes. --- internal/cli/recovery_test.go | 3 ++- internal/review/git.go | 16 ++++++++-------- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/internal/cli/recovery_test.go b/internal/cli/recovery_test.go index 4dac43d..92f9397 100644 --- a/internal/cli/recovery_test.go +++ b/internal/cli/recovery_test.go @@ -4,8 +4,9 @@ import ( "encoding/json/jsontext" "testing" - "github.com/nnutter/git-review/internal/review" "github.com/stretchr/testify/require" + + "github.com/nnutter/git-review/internal/review" ) func TestRecoveryDecisionExplainsSavedWork(t *testing.T) { diff --git a/internal/review/git.go b/internal/review/git.go index 1cb443d..289f1b1 100644 --- a/internal/review/git.go +++ b/internal/review/git.go @@ -20,14 +20,14 @@ import ( // reviewable content. Attached fixup! commits carry no message, so patch // identity alone decides their review status. type Stack struct { - Repo string `json:"repo"` - Commits []string `json:"commits"` - Base string `json:"base"` - Tip string `json:"tip"` - Subject string `json:"subject"` - Key string `json:"key"` - ChangeID string `json:"changeId,omitempty"` - PatchHash string `json:"patchHash,omitempty"` + Repo string `json:"repo"` + Commits []string `json:"commits"` + Base string `json:"base"` + Tip string `json:"tip"` + Subject string `json:"subject"` + Key string `json:"key"` + ChangeID string `json:"changeId,omitempty"` + PatchHash string `json:"patchHash,omitempty"` AmendCount int `json:"amendCount,omitzero"` } From fbec85c6f2aff23e807d9033c0245b54e1888081 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Mon, 19 Oct 2026 07:07:52 -0500 Subject: [PATCH 02/22] Refactor: Relocate and order upstream declarations Place methods beside their receiver types in the order Constable requires. Keep these moves separate from predicate and complexity repairs. --- internal/cli/cli.go | 51 ++++++++++++++++++++++++++++ internal/cli/log.go | 58 -------------------------------- internal/review/git.go | 68 +++++++++++++++++++------------------- internal/review/service.go | 64 +++++++++++++++++------------------ 4 files changed, 117 insertions(+), 124 deletions(-) delete mode 100644 internal/cli/log.go diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 0b62471..af914e2 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -8,6 +8,7 @@ import ( "fmt" "io" "os" + "os/exec" "strings" "syscall" @@ -100,6 +101,56 @@ func (a *application) command() *cobra.Command { return root } +func (a *application) logCommand() *cobra.Command { + var patch, ignoreAllSpace, stat, difft, hunk bool + cmd := &cobra.Command{ + Use: "log", + Short: "Show commits after the upstream in oldest-first order", + Long: "Run git log @{u}..HEAD --reverse with optional patches, diff statistics, and whitespace filtering.\nUse --difft for Difftastic or --hunk for hunk pager. Both imply --patch.\nOutput uses Git's format and pager, not git-review's review state.", + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, _ []string) error { + if a.json { + return fmt.Errorf("log does not support --json") + } + var args []string + if hunk { + args = append(args, "-c", "core.pager=hunk pager") + } + if difft { + args = append(args, "-c", "diff.external=difft") + } + args = append(args, "log", "@{u}..HEAD", "--reverse") + if difft { + args = append(args, "--ext-diff") + } + if patch || difft || hunk { + args = append(args, "-p") + } + if ignoreAllSpace { + args = append(args, "-w") + } + if stat { + args = append(args, "--stat") + } + git := exec.CommandContext(cmd.Context(), "git", args...) + git.Dir = a.service.Repository.Dir + git.Stdin = a.input + git.Stdout = a.output + git.Stderr = a.terminal + if err := git.Run(); err != nil { + return fmt.Errorf("git log: %w", err) + } + return nil + }, + } + cmd.Flags().BoolVarP(&patch, "patch", "p", false, "Show the patch for each commit") + cmd.Flags().BoolVarP(&ignoreAllSpace, "ignore-all-space", "w", false, "Ignore whitespace changes in patches") + cmd.Flags().BoolVar(&stat, "stat", false, "Show diff statistics for each commit") + cmd.Flags().BoolVar(&difft, "difft", false, "Show patches with difft (requires difft on PATH)") + cmd.Flags().BoolVar(&hunk, "hunk", false, "Show patches with hunk pager (requires hunk on PATH)") + return cmd +} + func (a *application) runCommand() *cobra.Command { var recoverReview, replaceReview bool var session, start, end string diff --git a/internal/cli/log.go b/internal/cli/log.go deleted file mode 100644 index d6ac059..0000000 --- a/internal/cli/log.go +++ /dev/null @@ -1,58 +0,0 @@ -package cli - -import ( - "fmt" - "os/exec" - - "github.com/spf13/cobra" -) - -func (a *application) logCommand() *cobra.Command { - var patch, ignoreAllSpace, stat, difft, hunk bool - cmd := &cobra.Command{ - Use: "log", - Short: "Show commits after the upstream in oldest-first order", - Long: "Run git log @{u}..HEAD --reverse with optional patches, diff statistics, and whitespace filtering.\nUse --difft for Difftastic or --hunk for hunk pager. Both imply --patch.\nOutput uses Git's format and pager, not git-review's review state.", - Args: cobra.NoArgs, - RunE: func(cmd *cobra.Command, _ []string) error { - if a.json { - return fmt.Errorf("log does not support --json") - } - var args []string - if hunk { - args = append(args, "-c", "core.pager=hunk pager") - } - if difft { - args = append(args, "-c", "diff.external=difft") - } - args = append(args, "log", "@{u}..HEAD", "--reverse") - if difft { - args = append(args, "--ext-diff") - } - if patch || difft || hunk { - args = append(args, "-p") - } - if ignoreAllSpace { - args = append(args, "-w") - } - if stat { - args = append(args, "--stat") - } - git := exec.CommandContext(cmd.Context(), "git", args...) - git.Dir = a.service.Repository.Dir - git.Stdin = a.input - git.Stdout = a.output - git.Stderr = a.terminal - if err := git.Run(); err != nil { - return fmt.Errorf("git log: %w", err) - } - return nil - }, - } - cmd.Flags().BoolVarP(&patch, "patch", "p", false, "Show the patch for each commit") - cmd.Flags().BoolVarP(&ignoreAllSpace, "ignore-all-space", "w", false, "Ignore whitespace changes in patches") - cmd.Flags().BoolVar(&stat, "stat", false, "Show diff statistics for each commit") - cmd.Flags().BoolVar(&difft, "difft", false, "Show patches with difft (requires difft on PATH)") - cmd.Flags().BoolVar(&hunk, "hunk", false, "Show patches with hunk pager (requires hunk on PATH)") - return cmd -} diff --git a/internal/review/git.go b/internal/review/git.go index 289f1b1..caedeaf 100644 --- a/internal/review/git.go +++ b/internal/review/git.go @@ -149,40 +149,6 @@ func (r Repository) Snapshot(ctx context.Context) (Snapshot, error) { return view, nil } -// reviewBranch identifies the branch whose review lock applies during a replay. -// Other review commands still require an attached HEAD through branchView. -func (r Repository) reviewBranch(ctx context.Context) (string, error) { - branch, err := r.git(ctx, "symbolic-ref", "--quiet", "HEAD") - if err == nil { - return branch, nil - } - if exit, ok := errors.AsType[*exec.ExitError](err); !ok || exit.ExitCode() != 1 { - return "", err - } - for _, backend := range []string{"rebase-merge", "rebase-apply"} { - path, pathErr := r.git(ctx, "rev-parse", "--path-format=absolute", "--git-path", backend+"/head-name") - if pathErr != nil { - return "", pathErr - } - data, readErr := os.ReadFile(path) - if errors.Is(readErr, os.ErrNotExist) { - continue - } - if readErr != nil { - return "", readErr - } - branch = strings.TrimSpace(string(data)) - if !strings.HasPrefix(branch, "refs/heads/") { - return "", fmt.Errorf("rebase does not identify a local branch") - } - if _, err := r.git(ctx, "check-ref-format", branch); err != nil { - return "", err - } - return branch, nil - } - return "", err -} - func (r Repository) branchView(ctx context.Context) (Snapshot, string, error) { var view Snapshot repo, err := r.git(ctx, "rev-parse", "--show-toplevel") @@ -286,6 +252,40 @@ func (r Repository) patchHash(ctx context.Context, base, tip string) (string, er return "v2:" + fields[0], nil } +// reviewBranch identifies the branch whose review lock applies during a replay. +// Other review commands still require an attached HEAD through branchView. +func (r Repository) reviewBranch(ctx context.Context) (string, error) { + branch, err := r.git(ctx, "symbolic-ref", "--quiet", "HEAD") + if err == nil { + return branch, nil + } + if exit, ok := errors.AsType[*exec.ExitError](err); !ok || exit.ExitCode() != 1 { + return "", err + } + for _, backend := range []string{"rebase-merge", "rebase-apply"} { + path, pathErr := r.git(ctx, "rev-parse", "--path-format=absolute", "--git-path", backend+"/head-name") + if pathErr != nil { + return "", pathErr + } + data, readErr := os.ReadFile(path) + if errors.Is(readErr, os.ErrNotExist) { + continue + } + if readErr != nil { + return "", readErr + } + branch = strings.TrimSpace(string(data)) + if !strings.HasPrefix(branch, "refs/heads/") { + return "", fmt.Errorf("rebase does not identify a local branch") + } + if _, err := r.git(ctx, "check-ref-format", branch); err != nil { + return "", err + } + return branch, nil + } + return "", err +} + func (r Repository) stacks(ctx context.Context, repo, base, history string) ([]Stack, error) { stacks := []Stack{} if history == "" { diff --git a/internal/review/service.go b/internal/review/service.go index db2af6b..b532b0c 100644 --- a/internal/review/service.go +++ b/internal/review/service.go @@ -49,6 +49,26 @@ type StateCount struct { Count int `json:"count"` } +func countStates(view Snapshot, state State) []StateCount { + counts := []StateCount{ + {State: "needs_review"}, + {State: "reviewing"}, + {State: "feedback"}, + {State: "incomplete"}, + {State: "approved"}, + } + for _, stack := range view.Stacks { + status := entryFor(state, stack).Status + for i := range counts { + if counts[i].State == status { + counts[i].Count += len(stack.Commits) + break + } + } + } + return counts +} + // Service coordinates the current branch with its durable review history. type Service struct{ Repository Repository } @@ -156,38 +176,6 @@ func (s Service) Status(ctx context.Context, needsReview bool) (Result, error) { return result, nil } -func (s Service) snapshotState(ctx context.Context) (Snapshot, State, error) { - view, err := s.Repository.Snapshot(ctx) - if err != nil { - return Snapshot{}, State{}, err - } - state, err := (Store{Path: view.Path}).Load() - if err != nil { - return Snapshot{}, State{}, err - } - return view, state, nil -} - -func countStates(view Snapshot, state State) []StateCount { - counts := []StateCount{ - {State: "needs_review"}, - {State: "reviewing"}, - {State: "feedback"}, - {State: "incomplete"}, - {State: "approved"}, - } - for _, stack := range view.Stacks { - status := entryFor(state, stack).Status - for i := range counts { - if counts[i].State == status { - counts[i].Count += len(stack.Commits) - break - } - } - } - return counts -} - func (s Service) selectRange(ctx context.Context, view Snapshot, first, last string) ([]Stack, error) { startSHA, err := s.Repository.Resolve(ctx, first) if err != nil { @@ -218,6 +206,18 @@ func (s Service) selectRange(ctx context.Context, view Snapshot, first, last str return view.Stacks[start : end+1], nil } +func (s Service) snapshotState(ctx context.Context) (Snapshot, State, error) { + view, err := s.Repository.Snapshot(ctx) + if err != nil { + return Snapshot{}, State{}, err + } + state, err := (Store{Path: view.Path}).Load() + if err != nil { + return Snapshot{}, State{}, err + } + return view, state, nil +} + func (s Service) unchanged(ctx context.Context, view Snapshot) error { fresh, err := s.Repository.Snapshot(ctx) if err != nil { From c6d0ec849f2e89e54c73f5cd5d383d15d7da042c Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Mon, 19 Oct 2026 07:08:27 -0500 Subject: [PATCH 03/22] Refactor: Extract the saved-progress type Name the saved-progress structure so a future recovery-progress check can accept it as an argument. Keep this preparation separate from the later check extraction, as requested in review. --- internal/cli/render.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/internal/cli/render.go b/internal/cli/render.go index ec2f333..1e8b3a8 100644 --- a/internal/cli/render.go +++ b/internal/cli/render.go @@ -85,12 +85,7 @@ func recoveryDescription(decision *review.Decision) string { started = "unavailable" } fmt.Fprintf(&out, "Started: %s · Status: %s\n", started, statusLabel(attempt.Status)) - var saved struct { - Reviewed *int `json:"reviewed"` - Files *int `json:"files"` - Comments []any `json:"comments"` - SessionNotes *string `json:"sessionNotes"` - } + var saved savedProgress if len(attempt.Review) == 0 || json.Unmarshal(attempt.Review, &saved) != nil || saved.Reviewed == nil || saved.Files == nil { fmt.Fprintln(&out, "Saved progress: unavailable") } else { @@ -114,6 +109,13 @@ func recoveryDescription(decision *review.Decision) string { return out.String() } +type savedProgress struct { + Reviewed *int `json:"reviewed"` + Files *int `json:"files"` + Comments []any `json:"comments"` + SessionNotes *string `json:"sessionNotes"` +} + func renderDetails(out *strings.Builder, entries []review.Entry) error { for _, entry := range entries { if entry.Attempt != nil { From 34931332daf6067f4b14170ffddb35b287006451 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Mon, 19 Oct 2026 07:09:18 -0500 Subject: [PATCH 04/22] Refactor: Extract upstream decision predicates Extract named predicate functions for compound conditions to satisfy Constable. --- internal/cli/cli.go | 12 +++++++++++- internal/cli/render.go | 42 ++++++++++++++++++++++++++++++++++++++---- internal/review/git.go | 21 +++++++++++++++++++-- 3 files changed, 68 insertions(+), 7 deletions(-) diff --git a/internal/cli/cli.go b/internal/cli/cli.go index af914e2..b6fe5db 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -123,7 +123,7 @@ func (a *application) logCommand() *cobra.Command { if difft { args = append(args, "--ext-diff") } - if patch || difft || hunk { + if wantsPatch(patch, difft, hunk) { args = append(args, "-p") } if ignoreAllSpace { @@ -238,6 +238,16 @@ func promptChoice(ctx context.Context, input io.Reader, output io.Writer, decisi return choice, err } +func wantsPatch(patch, difft, hunk bool) bool { + if patch { + return true + } + if difft { + return true + } + return hunk +} + // Determine error output even if Cobra fails before it parses persistent flags. func wantsJSON(args []string) bool { enabled := false diff --git a/internal/cli/render.go b/internal/cli/render.go index 1e8b3a8..27adb6e 100644 --- a/internal/cli/render.go +++ b/internal/cli/render.go @@ -33,7 +33,7 @@ func render(output io.Writer, result review.Result, verbose bool) error { func renderResult(result review.Result, verbose bool) (string, error) { var out strings.Builder - if result.Command == "status" && verbose { + if showStatusDetails(result, verbose) { if err := renderDetails(&out, result.Entries); err != nil { return "", err } @@ -86,11 +86,11 @@ func recoveryDescription(decision *review.Decision) string { } fmt.Fprintf(&out, "Started: %s · Status: %s\n", started, statusLabel(attempt.Status)) var saved savedProgress - if len(attempt.Review) == 0 || json.Unmarshal(attempt.Review, &saved) != nil || saved.Reviewed == nil || saved.Files == nil { + 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 saved.SessionNotes != nil && *saved.SessionNotes != "" { + if hasSessionNotes(saved.SessionNotes) { fmt.Fprint(&out, " · session notes saved") } fmt.Fprintln(&out) @@ -99,7 +99,7 @@ func recoveryDescription(decision *review.Decision) string { fmt.Fprintln(&out, "Last error: "+safeText(attempt.Error)) } if current := decision.Replacement; current != nil { - if current.Base != attempt.Stack.Base || current.Tip != attempt.Stack.Tip { + if scopeChanged(*current, attempt.Stack) { fmt.Fprintf(&out, "New review commits (inclusive): %s — %s\n", shortStack(*current), safeText(current.Subject)) } } else { @@ -116,6 +116,40 @@ type savedProgress struct { 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 + } + return result.Command == "status" +} + +func hasSessionNotes(notes *string) bool { + if notes == nil { + return false + } + 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 { diff --git a/internal/review/git.go b/internal/review/git.go index caedeaf..5f01218 100644 --- a/internal/review/git.go +++ b/internal/review/git.go @@ -259,7 +259,7 @@ func (r Repository) reviewBranch(ctx context.Context) (string, error) { if err == nil { return branch, nil } - if exit, ok := errors.AsType[*exec.ExitError](err); !ok || exit.ExitCode() != 1 { + if !detachedHEADExit(err) { return "", err } for _, backend := range []string{"rebase-merge", "rebase-apply"} { @@ -327,6 +327,23 @@ func (r Repository) stacks(ctx context.Context, repo, base, history string) ([]S return stacks, nil } +func detachedHEADExit(err error) bool { + exit, ok := errors.AsType[*exec.ExitError](err) + if !ok { + return false + } + return exit.ExitCode() == 1 +} + +func changesCommitMessage(kind string) bool { + switch kind { + case "amend!", "reword!": + return true + default: + return false + } +} + func tryAppendFeedback(stacks []Stack, targets map[string][]int, sha, kind, subject string) bool { matches := targets[subject] if len(matches) != 1 { @@ -339,7 +356,7 @@ func tryAppendFeedback(stacks []Stack, targets map[string][]int, sha, kind, subj last.Commits = append(last.Commits, sha) last.Tip = sha last.Key = last.Commits[0] + ".." + sha - if kind == "amend!" || kind == "reword!" { + if changesCommitMessage(kind) { last.AmendCount++ } return true From 011486ec2ecc38e1b8e8fbb300670ae26b2220e0 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Mon, 19 Oct 2026 07:09:48 -0500 Subject: [PATCH 05/22] Refactor: Move orphan-stack counts into test cases Store the expected first-stack commit count in each test case so the assertion no longer depends on the test case name. --- internal/review/git_test.go | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/internal/review/git_test.go b/internal/review/git_test.go index 19df8a3..c9db9b4 100644 --- a/internal/review/git_test.go +++ b/internal/review/git_test.go @@ -72,17 +72,18 @@ func TestSnapshotGroupsNativeFixupsTargetingStackTip(t *testing.T) { func TestSnapshotTreatsOrphanFeedbackAsNormalCommit(t *testing.T) { cases := []struct { - name string - subjects []string - want []string + name string + subjects []string + want []string + firstCommits int }{ - {"non-adjacent fixup", []string{"One", "Two", "fixup! One"}, []string{"One", "Two", "fixup! One"}}, - {"non-adjacent amend", []string{"One", "Two", "amend! One"}, []string{"One", "Two", "amend! One"}}, - {"non-adjacent reword", []string{"One", "Two", "reword! One"}, []string{"One", "Two", "reword! One"}}, - {"ambiguous subject", []string{"One", "One", "fixup! One"}, []string{"One", "One", "fixup! One"}}, - {"missing target", []string{"One", "fixup! fixup! One"}, []string{"One", "fixup! fixup! One"}}, - {"forward reference", []string{"One", "fixup! Two", "Three"}, []string{"One", "fixup! Two", "Three"}}, - {"non-adjacent nested fixup", []string{"One", "fixup! One", "Two", "fixup! fixup! One"}, []string{"One", "Two", "fixup! fixup! One"}}, + {"non-adjacent fixup", []string{"One", "Two", "fixup! One"}, []string{"One", "Two", "fixup! One"}, 1}, + {"non-adjacent amend", []string{"One", "Two", "amend! One"}, []string{"One", "Two", "amend! One"}, 1}, + {"non-adjacent reword", []string{"One", "Two", "reword! One"}, []string{"One", "Two", "reword! One"}, 1}, + {"ambiguous subject", []string{"One", "One", "fixup! One"}, []string{"One", "One", "fixup! One"}, 1}, + {"missing target", []string{"One", "fixup! fixup! One"}, []string{"One", "fixup! fixup! One"}, 1}, + {"forward reference", []string{"One", "fixup! Two", "Three"}, []string{"One", "fixup! Two", "Three"}, 1}, + {"non-adjacent nested fixup", []string{"One", "fixup! One", "Two", "fixup! fixup! One"}, []string{"One", "Two", "fixup! fixup! One"}, 2}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -95,11 +96,11 @@ func TestSnapshotTreatsOrphanFeedbackAsNormalCommit(t *testing.T) { require.Len(t, view.Stacks, len(tc.want)) for i, subject := range tc.want { require.Equal(t, subject, view.Stacks[i].Subject) - if i == 0 && tc.name == "non-adjacent nested fixup" { - require.Len(t, view.Stacks[i].Commits, 2) - } else { - require.Len(t, view.Stacks[i].Commits, 1) + count := 1 + if i == 0 { + count = tc.firstCommits } + require.Len(t, view.Stacks[i].Commits, count) } }) } From 6ded496971385305d2ad55e44bd01e179ad56f47 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Mon, 19 Oct 2026 07:10:16 -0500 Subject: [PATCH 06/22] Refactor: Extract stack-table rendering Extract stack-table rendering into a helper to reduce complexity in `renderResult`. --- internal/cli/render.go | 40 ++++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/internal/cli/render.go b/internal/cli/render.go index 27adb6e..6016bdc 100644 --- a/internal/cli/render.go +++ b/internal/cli/render.go @@ -39,24 +39,7 @@ func renderResult(result review.Result, verbose bool) (string, error) { } } if len(result.Entries) > 0 { - rows := make([][]string, 0, len(result.Entries)) - for _, entry := range result.Entries { - label := statusLabel(entry.Status) - if entry.Previous != nil { - if entry.Status == "needs_review" { - label += " (changed)" - } - } - rows = append(rows, []string{label, shortStack(entry.Stack), safeText(entry.Stack.Subject)}) - } - t := table.New().Headers("STATUS", "COMMITS (inclusive)", "SUBJECT").Rows(rows...).Border(lipgloss.RoundedBorder()).StyleFunc(func(row, _ int) lipgloss.Style { - style := lipgloss.NewStyle().Padding(0, 1) - if row == table.HeaderRow { - return style.Bold(true).Foreground(lipgloss.Color("12")) - } - return style - }) - fmt.Fprintln(&out, t.Render()) + fmt.Fprintln(&out, renderStackTable(result.Entries)) } for _, attempt := range result.Active { fmt.Fprintf(&out, "Active attempt: %s %s\n", safeText(attempt.ID), shortStack(attempt.Stack)) @@ -76,6 +59,27 @@ func renderResult(result review.Result, verbose bool) (string, error) { return strings.TrimSpace(out.String()), nil } +func renderStackTable(entries []review.Entry) string { + rows := make([][]string, 0, len(entries)) + for _, entry := range entries { + label := statusLabel(entry.Status) + if entry.Previous != nil { + if entry.Status == "needs_review" { + label += " (changed)" + } + } + rows = append(rows, []string{label, shortStack(entry.Stack), safeText(entry.Stack.Subject)}) + } + t := table.New().Headers("STATUS", "COMMITS (inclusive)", "SUBJECT").Rows(rows...).Border(lipgloss.RoundedBorder()).StyleFunc(func(row, _ int) lipgloss.Style { + style := lipgloss.NewStyle().Padding(0, 1) + if row == table.HeaderRow { + return style.Bold(true).Foreground(lipgloss.Color("12")) + } + return style + }) + return t.Render() +} + func recoveryDescription(decision *review.Decision) string { var out strings.Builder attempt := decision.Attempt From 3e414b1348099caf11ac7f41aee37bef5391c1aa Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Mon, 19 Oct 2026 07:10:45 -0500 Subject: [PATCH 07/22] Refactor: Extract status-verbosity assertions Extract the existing verbosity assertions into one helper to simplify the status-count integration test. --- internal/cli/status_test.go | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/internal/cli/status_test.go b/internal/cli/status_test.go index 4f1a621..6493abd 100644 --- a/internal/cli/status_test.go +++ b/internal/cli/status_test.go @@ -100,6 +100,18 @@ func TestStatusCountsCurrentCommitsInStateOrder(t *testing.T) { require.NoError(t, err) require.NoError(t, json.Unmarshal([]byte(out), &result)) require.Equal(t, []review.StateCount{{State: "needs_review", Count: 5}, {State: "reviewing", Count: 1}, {State: "feedback", Count: 1}, {State: "incomplete", Count: 1}, {State: "approved", Count: 0}}, result.Summary) + assertStatusVerbosity(t, binary, dir, out) + git(t, dir, "reset", "--hard", "main") + out, _, err = executeBinary(t, binary, dir, "status", "--json") + require.NoError(t, err) + require.NoError(t, json.Unmarshal([]byte(out), &result)) + require.Equal(t, []review.StateCount{{State: "needs_review"}, {State: "reviewing"}, {State: "feedback"}, {State: "incomplete"}, {State: "approved"}}, result.Summary) + require.Empty(t, result.Entries) + require.Equal(t, "pending", result.Status) // The historical active attempt still needs recovery. +} + +func assertStatusVerbosity(t *testing.T, binary, dir, out string) { + t.Helper() for _, flag := range []string{"", "-v", "--verbose"} { args := []string{"status"} if flag != "" { @@ -122,13 +134,6 @@ func TestStatusCountsCurrentCommitsInStateOrder(t *testing.T) { require.NoError(t, err) require.JSONEq(t, out, jsonOut) } - git(t, dir, "reset", "--hard", "main") - out, _, err = executeBinary(t, binary, dir, "status", "--json") - require.NoError(t, err) - require.NoError(t, json.Unmarshal([]byte(out), &result)) - require.Equal(t, []review.StateCount{{State: "needs_review"}, {State: "reviewing"}, {State: "feedback"}, {State: "incomplete"}, {State: "approved"}}, result.Summary) - require.Empty(t, result.Entries) - require.Equal(t, "pending", result.Status) // The historical active attempt still needs recovery. } func TestStatusWithoutReviewHistory(t *testing.T) { From b7e576c2bbbc777d9b36a67a9395949302b3c60e Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Mon, 19 Oct 2026 07:11:23 -0500 Subject: [PATCH 08/22] Refactor: Extract review-state count assertions Extract the existing assertion loop into a helper to reduce complexity in the stack-rewrite integration test. --- internal/review/patch_status_test.go | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/internal/review/patch_status_test.go b/internal/review/patch_status_test.go index e3ee209..7db6456 100644 --- a/internal/review/patch_status_test.go +++ b/internal/review/patch_status_test.go @@ -137,13 +137,7 @@ func TestReviewStatusAfterStackRewrite(t *testing.T) { } statusResult, err = service.Status(t.Context(), false) require.NoError(t, err) - for _, count := range statusResult.Summary { - want := 0 - if count.State == tc.want { - want = len(result.View.Stacks[0].Commits) - } - require.Equal(t, want, count.Count, "state %s", count.State) - } + assertStateCounts(t, statusResult.Summary, tc.want, len(result.View.Stacks[0].Commits)) after, err := os.ReadFile(store.Path) require.NoError(t, err) require.Equal(t, before, after, "status matching must not rewrite captured scopes") @@ -151,6 +145,17 @@ func TestReviewStatusAfterStackRewrite(t *testing.T) { } } +func assertStateCounts(t *testing.T, counts []StateCount, status string, commits int) { + t.Helper() + for _, count := range counts { + want := 0 + if count.State == status { + want = commits + } + require.Equal(t, want, count.Count, "state %s", count.State) + } +} + // A rebase can change blob identities and hunk offsets without changing the // reviewed patch. Python indentation must still distinguish rewritten content. func TestApprovalAfterTouchedFileRebase(t *testing.T) { From f7ef4f1e027857fedfc464cd888159fa0d911a4b Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Mon, 19 Oct 2026 00:00:03 -0500 Subject: [PATCH 09/22] Preserve the real-tuicr test opt-in during Git isolation Git isolation removed `GIT_REVIEW_REAL_TUICR`, so `mise run functional-tests` silently skipped real-tuicr tests even when explicitly enabled. Successful runs therefore gave false confidence without testing real-tuicr. Preserve this opt-in while clearing inherited Git repository settings so explicitly enabled real-tuicr tests execute. --- internal/cli/tuicr_functional_test.go | 4 ++-- internal/testenv/git.go | 4 ++++ internal/testenv/git_test.go | 16 ++++++++++++++++ 3 files changed, 22 insertions(+), 2 deletions(-) create mode 100644 internal/testenv/git_test.go diff --git a/internal/cli/tuicr_functional_test.go b/internal/cli/tuicr_functional_test.go index e944e1c..0879067 100644 --- a/internal/cli/tuicr_functional_test.go +++ b/internal/cli/tuicr_functional_test.go @@ -105,7 +105,7 @@ func TestRealTuicrFeedback(t *testing.T) { require.Equal(t, "Handle the empty case", saved.Comments[0].Content) require.Equal(t, "feature.txt", saved.Comments[0].Path) require.Equal(t, "line", saved.Comments[0].Location) - listed := runRealCommand(t, binary, dir, env, "status", "--needs-review") + listed := runRealCommand(t, binary, dir, env, "status", "--needs-review", "--verbose") require.Contains(t, listed, "Feedback") require.Contains(t, listed, "feature.txt:1: Handle the empty case") } @@ -228,7 +228,7 @@ func TestRealTuicrRecoveryPrompt(t *testing.T) { out := prompt.waitExit(t) switch choice { case "recover", "replace": - require.Contains(t, out, "Approved. Approval applies only to the recorded commits.") + require.Contains(t, out, "No reviews pending in the selected range.") case "cancel": require.Contains(t, out, "Review cancelled. The attempt remains unchanged.") } diff --git a/internal/testenv/git.go b/internal/testenv/git.go index db51895..c872969 100644 --- a/internal/testenv/git.go +++ b/internal/testenv/git.go @@ -23,6 +23,10 @@ func Run(m *testing.M) { func isolateGit() error { for _, entry := range os.Environ() { key, _, _ := strings.Cut(entry, "=") + // This is a test-suite opt-in, not a Git repository setting. + if key == "GIT_REVIEW_REAL_TUICR" { + continue + } if strings.HasPrefix(key, "GIT_") { if err := os.Unsetenv(key); err != nil { return err diff --git a/internal/testenv/git_test.go b/internal/testenv/git_test.go new file mode 100644 index 0000000..30eb2e4 --- /dev/null +++ b/internal/testenv/git_test.go @@ -0,0 +1,16 @@ +package testenv + +import ( + "os" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestIsolationPreservesFunctionalTestOptIn(t *testing.T) { + t.Setenv("GIT_REVIEW_REAL_TUICR", "1") + t.Setenv("GIT_DIR", "/not-the-test-repository") + require.NoError(t, isolateGit()) + require.Equal(t, "1", os.Getenv("GIT_REVIEW_REAL_TUICR")) + require.Empty(t, os.Getenv("GIT_DIR")) +} From 568000fbff84aa61beff62a745babacd74591ab6 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Sun, 18 Oct 2026 21:47:53 -0500 Subject: [PATCH 10/22] Consolidate live-review check into status --is-reviewing flag. Route live-review checks through status so callers share one command and one versioned JSON response format. --- README.md | 10 +++--- internal/cli/activity_test.go | 32 ++++++++++++++--- internal/cli/cli.go | 36 ++++++++++--------- internal/cli/cli_test.go | 6 ++-- pi/extensions/git-review/index.ts | 9 +++-- pi/extensions/git-review/support.ts | 18 ++++++++++ .../tests/git-review-support.test.ts | 22 +++++++++++- .../git-review/tests/git-review.test.ts | 30 +++++++++------- skills/git-review/SKILL.md | 3 ++ 9 files changed, 120 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index 26b0caf..0e94e55 100644 --- a/README.md +++ b/README.md @@ -151,15 +151,17 @@ Current and historical feedback remain available in the result. ### Check live review activity ```sh -git-review running --json +git-review status --is-reviewing --json ``` -Successful output is `{"version":1,"running":true}` or `{"version":1,"running":false}`. -The command reports whether the current branch owns the live review worktree lock without changing review state or creating files. +Successful output is `{"version":1,"command":"status","is_reviewing":true}` or `{"version":1,"command":"status","is_reviewing":false}`. +Without `--json`, the command prints `Is reviewing: true` or `Is reviewing: false`. +The flag reports whether the current branch owns the live review worktree lock without changing review state or creating files. A running automatic review holds this lock across all selected stacks. A persisted `reviewing` attempt alone does not indicate a running process. -A review on another branch returns `running: false`, including when that branch is in a linked worktree. +A review on another branch returns `is_reviewing: false`, including when that branch is in a linked worktree. The lock remains repository-wide to prevent concurrent reviews from using the shared review checkout. +The flag cannot be combined with `--needs-review` or `--verbose`. Command failures exit with status 1 and emit the standard versioned JSON error object. ### Collect saved feedback diff --git a/internal/cli/activity_test.go b/internal/cli/activity_test.go index f2ffc28..7f62428 100644 --- a/internal/cli/activity_test.go +++ b/internal/cli/activity_test.go @@ -19,12 +19,12 @@ func TestReviewRunningReportsLiveLockNotPersistedAttempt(t *testing.T) { require.NoError(t, err) check := func(cwd string, running bool) { t.Helper() - out, terminal, err := executeBinary(t, binary, cwd, "running", "--json") + out, terminal, err := executeBinary(t, binary, cwd, "status", "--is-reviewing", "--json") require.NoError(t, err) require.Empty(t, terminal) - want := `{"version":1,"running":false}` + want := `{"version":1,"command":"status","is_reviewing":false}` if running { - want = `{"version":1,"running":true}` + want = `{"version":1,"command":"status","is_reviewing":true}` } require.JSONEq(t, want, out) } @@ -78,15 +78,37 @@ func TestReviewRunningReportsLiveLockNotPersistedAttempt(t *testing.T) { } // A detached checkout without rebase metadata has no branch to check. git(t, dir, "checkout", "--detach") - out, terminal, err := executeBinary(t, binary, dir, "running", "--json") + out, terminal, err := executeBinary(t, binary, dir, "status", "--is-reviewing", "--json") require.Error(t, err) require.Empty(t, terminal) require.Contains(t, out, `"status":"error"`) } func TestReviewRunningOutsideRepository(t *testing.T) { - out, terminal, err := execute(t, t.TempDir(), "running", "--json") + out, terminal, err := execute(t, t.TempDir(), "status", "--is-reviewing", "--json") require.Error(t, err) require.Empty(t, terminal) require.Contains(t, out, `"status":"error"`) } + +func TestStatusIsReviewingTerminalAndConflicts(t *testing.T) { + binary := buildGitReview(t) + dir := featureRepo(t) + out, terminal, err := executeBinary(t, binary, dir, "status", "--is-reviewing") + require.NoError(t, err) + require.Empty(t, terminal) + require.Contains(t, out, "Is reviewing: false") + out, terminal, err = executeBinary(t, binary, dir, "status", "--is-reviewing", "--json") + require.NoError(t, err) + require.Empty(t, terminal) + require.Contains(t, out, `"is_reviewing"`) + for _, args := range [][]string{ + {"status", "--is-reviewing", "--needs-review", "--json"}, + {"status", "--is-reviewing", "--verbose", "--json"}, + } { + out, terminal, err := executeBinary(t, binary, dir, args...) + require.Error(t, err) + require.Empty(t, terminal) + require.Contains(t, out, `"status":"error"`) + } +} diff --git a/internal/cli/cli.go b/internal/cli/cli.go index b6fe5db..c33ce8e 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -73,8 +73,23 @@ func (a *application) command() *cobra.Command { } return a.write(result) }} - var onlyNeedsReview bool + var onlyNeedsReview, isReviewing bool status := &cobra.Command{Use: "status", Short: "Show all feature stacks and their review states", Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { + if isReviewing { + reviewing, err := a.service.ReviewRunning(cmd.Context()) + if err != nil { + return err + } + if a.json { + return writeJSON(a.output, struct { + Version int `json:"version"` + Command string `json:"command"` + IsReviewing bool `json:"is_reviewing"` + }{Version: 1, Command: "status", IsReviewing: reviewing}) + } + _, err = fmt.Fprintf(a.output, "Is reviewing: %t\n", reviewing) + return err + } result, err := a.service.Status(cmd.Context(), onlyNeedsReview) if err != nil { return err @@ -83,21 +98,10 @@ func (a *application) command() *cobra.Command { }} status.Flags().BoolVarP(&a.verbose, "verbose", "v", false, "Show saved review details before the table") status.Flags().BoolVar(&onlyNeedsReview, "needs-review", false, "Show unresolved stacks and active review attempts") - activity := &cobra.Command{Use: "running", Short: "Check whether a review is running for the current branch", Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { - running, err := a.service.ReviewRunning(cmd.Context()) - if err != nil { - return err - } - if a.json { - return writeJSON(a.output, struct { - Version int `json:"version"` - Running bool `json:"running"` - }{Version: 1, Running: running}) - } - _, err = fmt.Fprintf(a.output, "Review running: %t\n", running) - return err - }} - root.AddCommand(feedback, approve, status, activity, a.runCommand(), a.logCommand()) + status.Flags().BoolVar(&isReviewing, "is-reviewing", false, "Check whether a review is running for the current branch") + status.MarkFlagsMutuallyExclusive("needs-review", "is-reviewing") + status.MarkFlagsMutuallyExclusive("verbose", "is-reviewing") + root.AddCommand(feedback, approve, status, a.runCommand(), a.logCommand()) return root } diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 9095b56..5d2fb27 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -237,15 +237,17 @@ func TestHelpAndVersion(t *testing.T) { require.NoError(t, err) require.Contains(t, out, "approve") require.Contains(t, out, "run") - require.Contains(t, out, "running") require.Contains(t, out, "--json") require.NotContains(t, out, "confirm") require.NotContains(t, out, "challenge") - for _, name := range []string{"needs-review", "review", "review-running"} { + for _, name := range []string{"needs-review", "review", "review-running", "running"} { out, _, err = execute(t, t.TempDir(), name) require.ErrorContains(t, err, "unknown command") require.Empty(t, out) } + out, _, err = execute(t, t.TempDir(), "status", "--help") + require.NoError(t, err) + require.Contains(t, out, "is-reviewing") out, _, err = execute(t, t.TempDir(), "--version") require.NoError(t, err) require.Contains(t, out, "test-version") diff --git a/pi/extensions/git-review/index.ts b/pi/extensions/git-review/index.ts index 792e984..764334b 100644 --- a/pi/extensions/git-review/index.ts +++ b/pi/extensions/git-review/index.ts @@ -23,6 +23,7 @@ import { MISE_AFTER_ON_STOP_EVENT, MISE_COORDINATES_EVENT, parseGitReviewStatus, + parseIsReviewing, GIT_REVIEW_READY_EVENT, GIT_REVIEW_REQUEST_STATUS_EVENT, GIT_REVIEW_STATUS_EVENT, @@ -86,15 +87,13 @@ export default function (pi: ExtensionAPI) { return; } if (aborted || status.feedback === lastNudgedFeedback) return; - const activity = await pi.exec("git-review", ["running", "--json"], { + const activity = await pi.exec("git-review", ["status", "--is-reviewing", "--json"], { cwd: ctx.cwd, timeout: STATUS_TIMEOUT_MS, }); if (activity.killed || activity.code !== 0) return; - const parsed: unknown = JSON.parse(activity.stdout); - if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) return; - const { version, running } = parsed as { version?: unknown; running?: unknown }; - if (version !== 1 || running !== false || epoch !== generation) return; + const reviewing = parseIsReviewing(activity.stdout); + if (reviewing !== false || epoch !== generation) return; if (idleOnly && (!ctx.isIdle() || !idleNoticesAllowed)) return; lastNudgedFeedback = status.feedback; nudge(ctx, status.feedback); diff --git a/pi/extensions/git-review/support.ts b/pi/extensions/git-review/support.ts index d3facc7..ff07629 100644 --- a/pi/extensions/git-review/support.ts +++ b/pi/extensions/git-review/support.ts @@ -79,6 +79,24 @@ export function parseGitReviewStatus(stdout: string): GitReviewStatus | undefine }; } +/** + * Parse successful `git-review status --is-reviewing --json` stdout. + * Returns the reviewing flag, or undefined for unsupported results. + */ +export function parseIsReviewing(stdout: string): boolean | undefined { + let parsed: unknown; + try { + parsed = JSON.parse(stdout); + } catch { + return undefined; + } + if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) return undefined; + const result = parsed as { version?: unknown; command?: unknown; is_reviewing?: unknown }; + if (result.version !== 1 || result.command !== "status") return undefined; + if (typeof result.is_reviewing !== "boolean") return undefined; + return result.is_reviewing; +} + /** Accept only the status object git-review publishes. */ export function reviewLineFromEvent(data: unknown): string | undefined { if (data === null || typeof data !== "object") return undefined; diff --git a/pi/extensions/git-review/tests/git-review-support.test.ts b/pi/extensions/git-review/tests/git-review-support.test.ts index 559fe7d..76e1bc1 100644 --- a/pi/extensions/git-review/tests/git-review-support.test.ts +++ b/pi/extensions/git-review/tests/git-review-support.test.ts @@ -1,6 +1,6 @@ import assert from "node:assert/strict"; import test from "node:test"; -import { parseGitReviewStatus } from "../support.ts"; +import { parseGitReviewStatus, parseIsReviewing } from "../support.ts"; const summary = [ { state: "needs_review", count: 0 }, @@ -34,3 +34,23 @@ test("status parser rejects unsupported envelopes and invalid summaries", () => } assert.equal(parseGitReviewStatus("not json"), undefined); }); + +test("is-reviewing parser reads status flag", () => { + assert.equal(parseIsReviewing(JSON.stringify({ version: 1, command: "status", is_reviewing: true })), true); + assert.equal(parseIsReviewing(JSON.stringify({ version: 1, command: "status", is_reviewing: false })), false); +}); + +test("is-reviewing parser rejects unsupported envelopes", () => { + for (const invalid of [ + null, + [], + { version: 2, command: "status", is_reviewing: false }, + { version: 1, command: "running", is_reviewing: false }, + { version: 1, command: "status" }, + { version: 1, command: "status", is_reviewing: "false" }, + { version: 1, running: false }, + ]) { + assert.equal(parseIsReviewing(JSON.stringify(invalid)), undefined); + } + assert.equal(parseIsReviewing("not json"), undefined); +}); diff --git a/pi/extensions/git-review/tests/git-review.test.ts b/pi/extensions/git-review/tests/git-review.test.ts index cfe79e4..11b295b 100644 --- a/pi/extensions/git-review/tests/git-review.test.ts +++ b/pi/extensions/git-review/tests/git-review.test.ts @@ -12,7 +12,7 @@ function harness(mode: ExtensionContext["mode"] = "tui") { const messages: string[] = []; const published: unknown[] = []; let feedback = 1; - let activity = { code: 0, killed: false, stdout: '{"version":1,"running":true}' }; + let activity = { code: 0, killed: false, stdout: '{"version":1,"command":"status","is_reviewing":true}' }; let idle = true; let activityGate: Promise | undefined; let activityCalls = 0; @@ -23,9 +23,10 @@ function harness(mode: ExtensionContext["mode"] = "tui") { exec: async (command: string, args: string[]) => { if (command === "git") return { code: 0, killed: false, stdout: "true" }; assert.equal(command, "git-review"); - if (args[0] === "running") { + if (args[0] === "status" && args[1] === "--is-reviewing") { activityCalls++; await activityGate; + assert.deepEqual(args, ["status", "--is-reviewing", "--json"]); return activity; } assert.deepEqual(args, ["status", "--needs-review", "--json"]); @@ -65,7 +66,7 @@ for (const coordinated of [false, true]) { await settle(); assert.equal(h.messages.length, 0); assert.equal(h.published.length, 1, "UI still receives review counts"); - h.setActivity({ code: 0, killed: false, stdout: '{"version":1,"running":false}' }); + h.setActivity({ code: 0, killed: false, stdout: '{"version":1,"command":"status","is_reviewing":false}' }); await settle(); assert.equal(h.messages.length, 1, "suppression does not consume the notice"); await settle(); @@ -89,7 +90,7 @@ for (const mode of ["tui", "rpc"] as const) { t.mock.timers.tick(5_000); await setImmediate(); assert.equal(h.messages.length, 0); - h.setActivity({ code: 0, killed: false, stdout: '{"version":1,"running":false}' }); + h.setActivity({ code: 0, killed: false, stdout: '{"version":1,"command":"status","is_reviewing":false}' }); h.setIdle(false); t.mock.timers.tick(5_000); await setImmediate(); @@ -116,7 +117,7 @@ test("idle checks wait for the coordinated on-stop hook", async t => { await h.emit("session_start"); h.events.emit(MISE_COORDINATES_EVENT); await h.emit("agent_start"); - h.setActivity({ code: 0, killed: false, stdout: '{"version":1,"running":false}' }); + h.setActivity({ code: 0, killed: false, stdout: '{"version":1,"command":"status","is_reviewing":false}' }); // Pi can report idle while another extension is still finishing on-stop. await h.emit("agent_settled"); t.mock.timers.tick(5_000); @@ -133,7 +134,7 @@ test("non-interactive sessions do not start idle polling", async t => { for (const mode of ["print", "json"] as const) { const h = harness(mode); await h.emit("session_start"); - h.setActivity({ code: 0, killed: false, stdout: '{"version":1,"running":false}' }); + h.setActivity({ code: 0, killed: false, stdout: '{"version":1,"command":"status","is_reviewing":false}' }); t.mock.timers.tick(10_000); await setImmediate(); assert.equal(h.activityCalls(), 0); @@ -146,7 +147,7 @@ test("slow idle checks do not overlap or notify after shutdown", async t => { t.mock.timers.enable({ apis: ["setInterval"] }); const h = harness(); await h.emit("session_start"); - h.setActivity({ code: 0, killed: false, stdout: '{"version":1,"running":false}' }); + h.setActivity({ code: 0, killed: false, stdout: '{"version":1,"command":"status","is_reviewing":false}' }); let release!: () => void; h.setActivityGate(new Promise(resolve => { release = resolve; })); t.mock.timers.tick(5_000); @@ -167,13 +168,13 @@ test("idle notices respect abort and recheck whether the agent is idle", async t t.after(() => h.emit("session_shutdown")); await h.emit("session_start"); await h.emit("agent_settled", { aborted: true }); - h.setActivity({ code: 0, killed: false, stdout: '{"version":1,"running":false}' }); + h.setActivity({ code: 0, killed: false, stdout: '{"version":1,"command":"status","is_reviewing":false}' }); t.mock.timers.tick(5_000); await setImmediate(); assert.equal(h.messages.length, 0, "abort must not restart the agent through polling"); - h.setActivity({ code: 0, killed: false, stdout: '{"version":1,"running":true}' }); + h.setActivity({ code: 0, killed: false, stdout: '{"version":1,"command":"status","is_reviewing":true}' }); await h.emit("agent_settled"); - h.setActivity({ code: 0, killed: false, stdout: '{"version":1,"running":false}' }); + h.setActivity({ code: 0, killed: false, stdout: '{"version":1,"command":"status","is_reviewing":false}' }); let release!: () => void; h.setActivityGate(new Promise(resolve => { release = resolve; })); t.mock.timers.tick(5_000); @@ -192,16 +193,19 @@ test("activity query failures do not authorize a feedback notice", async () => { const h = harness(); for (const result of [ { code: 1, killed: false, stdout: '{"version":1,"status":"error"}' }, - { code: 0, killed: true, stdout: '{"version":1,"running":false}' }, + { code: 0, killed: true, stdout: '{"version":1,"command":"status","is_reviewing":false}' }, { code: 0, killed: false, stdout: 'not json' }, - { code: 0, killed: false, stdout: '{"version":2,"running":false}' }, + { code: 0, killed: false, stdout: '{"version":2,"command":"status","is_reviewing":false}' }, { code: 0, killed: false, stdout: '{"version":1}' }, + { code: 0, killed: false, stdout: '{"version":1,"command":"status"}' }, + { code: 0, killed: false, stdout: '{"version":1,"command":"status","is_reviewing":"false"}' }, + { code: 0, killed: false, stdout: '{"version":1,"running":false}' }, ]) { h.setActivity(result); await h.emit("agent_settled"); assert.equal(h.messages.length, 0); } - h.setActivity({ code: 0, killed: false, stdout: '{"version":1,"running":false}' }); + h.setActivity({ code: 0, killed: false, stdout: '{"version":1,"command":"status","is_reviewing":false}' }); await h.emit("agent_settled", { aborted: true }); assert.equal(h.messages.length, 0); await h.emit("agent_settled"); diff --git a/skills/git-review/SKILL.md b/skills/git-review/SKILL.md index 2271ac0..216e8d5 100644 --- a/skills/git-review/SKILL.md +++ b/skills/git-review/SKILL.md @@ -83,6 +83,9 @@ Do not silently lose selected feedback or address the same attempt twice. If `active` contains an attempt, pause before rewriting history. Ask the user to finish or resolve the active review. +Check live activity with `git-review status --is-reviewing --json` before rewriting history. +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 `git-review run --recover` to retrieve feedback. Recovery opens an interactive review and requires the user's choice. From 18713b716287f9891d10f27e5197494322fb42e2 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Sun, 18 Oct 2026 21:48:33 -0500 Subject: [PATCH 11/22] Rename run command to commits to distinguish stack and files reviews Use `commits` in documentation and result JSON to distinguish stack reviews from upcoming file reviews. Keep `run` as an alias so existing scripts continue to work. --- README.md | 22 ++++++++++++---------- internal/cli/cli.go | 2 +- internal/cli/cli_test.go | 16 ++++++++-------- internal/cli/recovery_test.go | 4 ++-- internal/cli/render.go | 4 ++-- internal/cli/tuicr_functional_test.go | 14 +++++++------- internal/review/run.go | 10 +++++----- pi/extensions/git-review/index.ts | 5 +++-- pi/extensions/git-review/support.ts | 3 ++- skills/git-review/SKILL.md | 4 ++-- 10 files changed, 44 insertions(+), 40 deletions(-) diff --git a/README.md b/README.md index 0e94e55..5d0fbba 100644 --- a/README.md +++ b/README.md @@ -197,11 +197,13 @@ Active attempts remain visible in `active`, but collecting feedback does not res ### Review a stack ```sh -git-review run -git-review run HEAD -git-review run abc123..def456 +git-review commits +git-review commits HEAD +git-review commits abc123..def456 ``` +`run` remains as a deprecated alias for `commits`. + Without an argument, git-review reviews each eligible, non-deferred stack in oldest-first order, once per invocation. Approval or feedback does not stop this sequence. A commit reference selects only its containing stack. @@ -235,9 +237,9 @@ An untouched review leaves no attempt in git-review's history and does not requi ### Review a range of stacks ```sh -git-review run --start abc123 -git-review run --start abc123 --end def456 --json -git-review run --end def456 +git-review commits --start abc123 +git-review commits --start abc123 --end def456 --json +git-review commits --end def456 ``` Each invocation reviews eligible stacks within the inclusive bounds in oldest-first order. @@ -266,9 +268,9 @@ In an interactive terminal, git-review prompts to recover, replace, or cancel. With JSON output or redirected streams, it returns `decision_required` without a prompt. ```sh -git-review run --recover -git-review run --replace -git-review run --recover --session +git-review commits --recover +git-review commits --replace +git-review commits --recover --session ``` Recovery reopens tuicr at the previous attempt's exact captured range and preserves its attempt ID. @@ -279,7 +281,7 @@ Neither operation proceeds while the review worktree is held by a live git-revie `--session` accepts an exact saved session ID, index slug, or indexed `sessions/.json` path. It resolves ambiguous saved sessions and is not an arbitrary filesystem path. -If an interrupted original disappeared and no lineage identifies its replacement, use `git-review run --replace`. +If an interrupted original disappeared and no lineage identifies its replacement, use `git-review commits --replace`. ### Record explicit user approval diff --git a/internal/cli/cli.go b/internal/cli/cli.go index c33ce8e..81277bf 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -158,7 +158,7 @@ func (a *application) logCommand() *cobra.Command { func (a *application) runCommand() *cobra.Command { var recoverReview, replaceReview bool var session, start, end string - cmd := &cobra.Command{Use: "run []", Aliases: []string{"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: func(cmd *cobra.Command, args []string) error { + cmd := &cobra.Command{Use: "commits []", Aliases: []string{"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: func(cmd *cobra.Command, args []string) error { options := review.ReviewOptions{Session: session, Start: start, End: end} if len(args) > 0 { options.Stack = args[0] diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 5d2fb27..7d607ec 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -98,7 +98,7 @@ func TestJSONErrorsIncludeArgumentAndGitFailures(t *testing.T) { for _, args := range [][]string{ {"status", "--needs-review", "--json"}, {"approve", "--json"}, - {"run", "--recover", "--replace", "--json"}, + {"commits", "--recover", "--replace", "--json"}, {"not-a-command", "--json"}, {"--json", "status", "--needs-review", "--unknown"}, } { @@ -124,7 +124,7 @@ 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 })) - out, terminal, err := execute(t, dir, "run", "--start", "HEAD", "--end", "HEAD", "--json") + out, terminal, err := execute(t, dir, "commits", "--start", "HEAD", "--end", "HEAD", "--json") require.NoError(t, err) require.Empty(t, terminal) var result review.Result @@ -150,7 +150,7 @@ func TestReviewAliasProducesDecisionWithoutPromptOrStateChanges(t *testing.T) { var result review.Result require.NoError(t, json.Unmarshal([]byte(out), &result)) require.Equal(t, "decision_required", result.Status) - require.Equal(t, "run", result.Command) + 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) @@ -158,7 +158,7 @@ func TestReviewAliasProducesDecisionWithoutPromptOrStateChanges(t *testing.T) { require.Equal(t, before, after) out, _, err = execute(t, dir, "run") require.NoError(t, err) - require.Contains(t, out, "git-review run --recover") + require.Contains(t, out, "git-review commits --recover") } func TestTerminalRenderingRetainsFeedbackAndRemovesControlSequences(t *testing.T) { @@ -183,7 +183,7 @@ func TestReviewKeepsTuicrOutputOffJSONStdout(t *testing.T) { bin := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(bin, "tuicr"), []byte("#!/bin/sh\nprintf 'tuicr terminal output\\n' >&2\nprintf 'exported feedback\\n'\n"), 0o700)) t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) - out, terminal, err := execute(t, dir, "run", "--json") + out, terminal, err := execute(t, dir, "commits", "--json") require.NoError(t, err) require.Equal(t, "tuicr terminal output\n", terminal) var result review.Result @@ -209,7 +209,7 @@ func TestTuicrKeepsWorktreeLockedAfterGitReviewDies(t *testing.T) { script := "#!/bin/sh\nset -eu\nprintf started > \"$TEST_STARTED\"\nwhile [ ! -f \"$TEST_EXIT\" ]; do sleep 0.05; done\nprintf finished > \"$TEST_STARTED\"\n" require.NoError(t, os.WriteFile(filepath.Join(bin, "tuicr"), []byte(script), 0o700)) t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) - cmd := exec.CommandContext(t.Context(), binary, "run", "--json") + cmd := exec.CommandContext(t.Context(), binary, "commits", "--json") cmd.Dir = dir require.NoError(t, cmd.Start()) defer func() { @@ -225,7 +225,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, "run", "--replace", "--json") + out, _, err := execute(t, dir, "commits", "--replace", "--json") require.ErrorContains(t, err, "worktree is in use") var result errorResult require.NoError(t, json.Unmarshal([]byte(out), &result)) @@ -236,7 +236,7 @@ func TestHelpAndVersion(t *testing.T) { out, _, err := execute(t, t.TempDir(), "--help") require.NoError(t, err) require.Contains(t, out, "approve") - require.Contains(t, out, "run") + require.Contains(t, out, "commits") require.Contains(t, out, "--json") require.NotContains(t, out, "confirm") require.NotContains(t, out, "challenge") diff --git a/internal/cli/recovery_test.go b/internal/cli/recovery_test.go index 92f9397..e28ab79 100644 --- a/internal/cli/recovery_test.go +++ b/internal/cli/recovery_test.go @@ -24,7 +24,7 @@ func TestRecoveryDecisionExplainsSavedWork(t *testing.T) { 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: "run", Status: "decision_required", Decision: &review.Decision{ + 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, @@ -46,7 +46,7 @@ func TestRecoveryDecisionExplainsSavedWork(t *testing.T) { require.NotContains(t, text, "New review commits") } if tc.replacement == "none" { - require.Contains(t, text, "No current replacement scope. Select one with git-review run .") + require.Contains(t, text, "No current replacement scope. Select one with git-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") diff --git a/internal/cli/render.go b/internal/cli/render.go index 6016bdc..00027e9 100644 --- a/internal/cli/render.go +++ b/internal/cli/render.go @@ -54,7 +54,7 @@ func renderResult(result review.Result, verbose bool) (string, error) { } if result.Decision != nil { fmt.Fprintln(&out, recoveryDescription(result.Decision)) - fmt.Fprintln(&out, "Choose git-review run --recover, git-review run --replace, or leave the attempt unchanged.") + fmt.Fprintln(&out, "Choose git-review commits --recover, git-review commits --replace, or leave the attempt unchanged.") } return strings.TrimSpace(out.String()), nil } @@ -107,7 +107,7 @@ func recoveryDescription(decision *review.Decision) string { 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 git-review run .") + fmt.Fprintln(&out, "No current replacement scope. Select one with git-review commits .") } fmt.Fprint(&out, "Replacement keeps the prior attempt in history. Tuicr may reuse its saved session.") return out.String() diff --git a/internal/cli/tuicr_functional_test.go b/internal/cli/tuicr_functional_test.go index 0879067..83dba4f 100644 --- a/internal/cli/tuicr_functional_test.go +++ b/internal/cli/tuicr_functional_test.go @@ -67,7 +67,7 @@ func TestRealTuicrFeedback(t *testing.T) { env := realTuicrEnvironment(t) binary := buildGitReview(t) dir := realFeatureRepo(t) - session := startTerminal(t, binary, dir, env, "run", "--json") + session := startTerminal(t, binary, dir, env, "commits", "--json") session.waitFor(t, "feature.txt") list := runRealCommand(t, "tuicr", dir, env, "review", "list", "--all") var sessions []struct { @@ -114,7 +114,7 @@ func TestRealTuicrInterruptedRecovery(t *testing.T) { env := realTuicrEnvironment(t) binary := buildGitReview(t) dir := realFeatureRepo(t) - session := startTerminal(t, binary, dir, env, "run", "--json") + session := startTerminal(t, binary, dir, env, "commits", "--json") session.waitFor(t, "feature.txt") session.send(t, ":set noreviewed\rr") session.waitFor(t, "1/2") @@ -143,7 +143,7 @@ func TestRealTuicrInterruptedRecovery(t *testing.T) { } return !saved[0].Active }, 5*time.Second, 20*time.Millisecond) - before := runRealCommand(t, binary, dir, env, "run", "--json") + before := runRealCommand(t, binary, dir, env, "commits", "--json") var decision struct { Status string `json:"status"` Decision struct { @@ -153,7 +153,7 @@ func TestRealTuicrInterruptedRecovery(t *testing.T) { 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, "run", "HEAD", "--recover", "--json") + resumed := startTerminal(t, binary, dir, env, "commits", "HEAD", "--recover", "--json") resumed.waitFor(t, "feature.txt") resumed.waitFor(t, "1/2") resumed.send(t, ":set noreviewed\rr") @@ -186,7 +186,7 @@ func TestRealTuicrRecoveryPrompt(t *testing.T) { env := realTuicrEnvironment(t) binary := buildGitReview(t) dir := realFeatureRepo(t) - initial := startTerminal(t, binary, dir, env, "run", "--json") + initial := startTerminal(t, binary, dir, env, "commits", "--json") initial.waitFor(t, "feature.txt") initial.send(t, ":set noreviewed\rr") initial.waitFor(t, "1/2") @@ -203,7 +203,7 @@ func TestRealTuicrRecoveryPrompt(t *testing.T) { 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, "run") + 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") @@ -259,7 +259,7 @@ func TestRealTuicrCleanReview(t *testing.T) { env := realTuicrEnvironment(t) binary := buildGitReview(t) dir := realFeatureRepo(t) - session := startTerminal(t, binary, dir, env, "run", "HEAD", "--json") + session := startTerminal(t, binary, dir, env, "commits", "HEAD", "--json") session.waitFor(t, "feature.txt") session.send(t, ":set noreviewed\r") session.send(t, "rr") diff --git a/internal/review/run.go b/internal/review/run.go index 9ce7998..e9cd744 100644 --- a/internal/review/run.go +++ b/internal/review/run.go @@ -208,7 +208,7 @@ func (r Reviewer) launch(ctx context.Context, path string, stack Stack, lock *os // Keep the worktree locked if git-review dies while tuicr is still running. cmd.ExtraFiles = []*os.File{lock} if err := cmd.Run(); err != nil { - return fmt.Errorf("tuicr interrupted or failed; use git-review run to recover or replace: %w", err) + return fmt.Errorf("tuicr interrupted or failed; use git-review commits to recover or replace: %w", err) } return nil } @@ -300,11 +300,11 @@ func (r Reviewer) recover(plan reviewPlan, attempt Attempt, selector string) (Re if err != nil { return Result{}, err } - result := makeResult("run", plan.view, state) + result := makeResult("commits", plan.view, state) result.Status = "cancelled" return result, nil } - result := Result{Version: 1, Command: "run", Status: finished.Status, View: plan.view, Entries: []Entry{{Stack: finished.Stack, Status: finished.Status, Attempt: &finished}}, Active: []Attempt{}} + 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) } @@ -363,7 +363,7 @@ func (r Reviewer) reviewStack(ctx context.Context, options ReviewOptions, visite if err != nil { return Result{}, err } - result := makeResult("run", plan.view, plan.state) + result := makeResult("commits", plan.view, plan.state) if plan.stack == nil { if plan.rangeComplete() { result.Status = "range_complete" @@ -549,7 +549,7 @@ func replacementStack(plan reviewPlan) (Stack, error) { return stack, nil } } - return Stack{}, fmt.Errorf("interrupted stack is no longer current; select its replacement with git-review run ") + return Stack{}, fmt.Errorf("interrupted stack is no longer current; select its replacement with git-review commits ") } func (options ReviewOptions) hasBounds() bool { diff --git a/pi/extensions/git-review/index.ts b/pi/extensions/git-review/index.ts index 764334b..ef53995 100644 --- a/pi/extensions/git-review/index.ts +++ b/pi/extensions/git-review/index.ts @@ -8,8 +8,9 @@ * Recap (and anything else) can ask for a fresh post with * `git-review:request-status`. The request object may set `refresh(cwd)`. * - * On settle, when feedback exists and no review process is running, the extension tells - * the model to use the git-review skill. The same count is not sent again + * On settle, when `commits` feedback exists and no `commits` or `files` review + * is running (`status --is-reviewing`), the extension tells the model to use + * the git-review skill. The same count is not sent again * until it changes, so an unanswered notice does not loop. That notice * runs from `mise-hooks:after-on-stop` when mise-hooks is loaded, so it * follows the mise on-stop hook. Otherwise it runs on `agent_settled`. diff --git a/pi/extensions/git-review/support.ts b/pi/extensions/git-review/support.ts index ff07629..92c2b96 100644 --- a/pi/extensions/git-review/support.ts +++ b/pi/extensions/git-review/support.ts @@ -2,7 +2,8 @@ * Pure helpers for the git-review extension. * * No Pi imports: node:test can load this file outside the Pi process. - * `git-review status --needs-review --json` prints a result with a summary of { state, count }. + * `git-review status --needs-review --json` prints `commits` stacks with a summary of { state, count }. + * `git-review status --is-reviewing --json` prints whether a `commits` or `files` review holds the lock. * Failures print a versioned object and exit non-zero. Callers check * the exit code before parsing. */ diff --git a/skills/git-review/SKILL.md b/skills/git-review/SKILL.md index 216e8d5..48a5bf2 100644 --- a/skills/git-review/SKILL.md +++ b/skills/git-review/SKILL.md @@ -87,7 +87,7 @@ Check live activity with `git-review status --is-reviewing --json` before rewrit 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 `git-review run --recover` to retrieve feedback. +Do not use `git-review commits --recover` to retrieve feedback. Recovery opens an interactive review and requires the user's choice. ## Prepare the changes @@ -200,4 +200,4 @@ Keep backup branches until the user requests their removal. Remove temporary fix branches only after checking their commits are reachable from the feature branch. Leave recovery branches intact when integration fails. -Tell the user that the changes are ready for re-review with `git-review run`. +Tell the user that the changes are ready for re-review with `git-review commits`. From d58a4f1bbecf70f07d64c3df40fd2678a0680b4e Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Sun, 18 Oct 2026 21:49:53 -0500 Subject: [PATCH 12/22] Add files command for working-tree reviews Support reviews of tracked and untracked working-tree files because commit stack reviews cannot cover changes without commits. Distinguish file results from commit results so automation addresses file feedback with new commits rather than fixups. Hold the shared review lock so status checks detect either kind of live review. --- README.md | 39 +++ internal/cli/cli.go | 37 ++- internal/cli/cli_test.go | 1 + internal/cli/files_functional_test.go | 75 +++++ internal/cli/files_test.go | 143 +++++++++ internal/cli/render.go | 63 ++++ internal/review/files.go | 424 ++++++++++++++++++++++++++ internal/review/files_test.go | 170 +++++++++++ internal/review/saved.go | 16 + pi/extensions/git-review/index.ts | 6 +- pi/extensions/git-review/support.ts | 2 + skills/git-review/SKILL.md | 29 +- 12 files changed, 999 insertions(+), 6 deletions(-) create mode 100644 internal/cli/files_functional_test.go create mode 100644 internal/cli/files_test.go create mode 100644 internal/review/files.go create mode 100644 internal/review/files_test.go diff --git a/README.md b/README.md index 5d0fbba..2ec83ec 100644 --- a/README.md +++ b/README.md @@ -283,6 +283,43 @@ Neither operation proceeds while the review worktree is held by a live git-revie It resolves ambiguous saved sessions and is not an arbitrary filesystem path. If an interrupted original disappeared and no lineage identifies its replacement, use `git-review commits --replace`. +### Review working-tree files + +```sh +git-review files +git-review files +git-review files --untracked +git-review files --untracked --json +``` + +Without a path, git-review runs `tuicr tui --all-files` to review every tracked file. +With a path, it runs `tuicr tui --file ` for that file or directory. +With `--untracked` and no path, it runs `tuicr tui --file .` to include untracked files in the current directory. +`--untracked` cannot be combined with an explicit path. +Use `--session` to select an exact saved session when several file reviews match the same repository. + +File reviews run in the feature checkout, not the detached review worktree, and 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. +`approve` applies only to commit stacks, not file reviews. + +Successful `files` output uses `"command":"files"` to distinguish it from `"command":"commits"`. +It contains the reviewed repository, requested path, `allFiles` and `untracked` flags, and the saved file review with comments and session notes. +Saved sessions are matched to tuicr's canonical file or directory scope, not just the Git repository root. +File comment paths are relative to the Git repository root, including reviews started from a subdirectory. +Statuses mirror stack reviews without recovery decisions: + +| Saved result | Status | +| --- | --- | +| Every file reviewed, without comments or session notes | `approved` | +| Any comments or session notes, including drafts | `feedback` | +| No reviewed files, comments, or session notes, including no saved session | `cancelled` (no feedback saved) | +| Some reviewed files, but incomplete and without notes | `incomplete` | + +Address `files` feedback as one or more new commits, logically organized and sized. +Do not use `fixup!`, `amend!`, or `reword!` commits for file feedback. +Address `commits` feedback with native Git fixup commits inserted immediately after their target stacks instead. + ### Record explicit user approval ```sh @@ -305,6 +342,8 @@ All commands accept `--json`, before or after the subcommand. Each command produces one JSON value followed by a newline on stdout. Successful `status` output is an ordered array of state counts. Other successful commands produce a versioned JSON object. +`commits` results use `"command":"commits"` with stack entries and attempts. +`files` results use `"command":"files"` with the file scope and saved file review. JSON and terminal output consume the same typed result. JSON retains full commit IDs, attempts, saved comments, session notes, and optional previous-version lineage. Tuicr renders its interactive view through the controlling terminal, with diagnostics on stderr, not the JSON stream. diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 81277bf..107be52 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -101,10 +101,25 @@ func (a *application) command() *cobra.Command { status.Flags().BoolVar(&isReviewing, "is-reviewing", false, "Check whether a review is running for the current branch") status.MarkFlagsMutuallyExclusive("needs-review", "is-reviewing") status.MarkFlagsMutuallyExclusive("verbose", "is-reviewing") - root.AddCommand(feedback, approve, status, a.runCommand(), a.logCommand()) + root.AddCommand(feedback, approve, status, a.runCommand(), a.filesCommand(), a.logCommand()) return root } +func (a *application) filesCommand() *cobra.Command { + var untracked bool + var session string + cmd := &cobra.Command{Use: "files []", Short: "Review working-tree files with tuicr", Long: "Review working-tree files with tuicr. Without a path, review every tracked\nfile with tuicr tui --all-files. With a path, review that file or directory\nwith tuicr tui --file. With --untracked and no path, review untracked files\nin the current directory with tuicr tui --file . Address file feedback as new commits,\nlogically organized, not fixup commits.", Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + options := review.FilesOptions{Session: session, Untracked: untracked} + if len(args) > 0 { + options.Path = args[0] + } + return a.runFiles(cmd.Context(), options) + }} + cmd.Flags().BoolVar(&untracked, "untracked", false, "Review untracked files in the current directory (defaults path to .)") + cmd.Flags().StringVar(&session, "session", "", "Select an exact saved session ID, slug, or indexed session path") + return cmd +} + func (a *application) logCommand() *cobra.Command { var patch, ignoreAllSpace, stat, difft, hunk bool cmd := &cobra.Command{ @@ -180,6 +195,19 @@ func (a *application) runCommand() *cobra.Command { return cmd } +func (a *application) runFiles(ctx context.Context, options review.FilesOptions) error { + reader, err := review.DefaultSavedReader() + if err != nil { + return err + } + reviewer := review.FileReviewer{Service: a.service, Reader: reader, Input: a.input, Terminal: a.terminal} + result, err := reviewer.Review(ctx, options) + if err != nil { + return err + } + return a.writeFiles(result) +} + func (a *application) runReview(ctx context.Context, options review.ReviewOptions) error { reader, err := review.DefaultSavedReader() if err != nil { @@ -209,6 +237,13 @@ func (a *application) write(result review.Result) error { return render(a.output, result, a.verbose) } +func (a *application) writeFiles(result review.FilesResult) error { + if a.json { + return writeJSON(a.output, result) + } + return renderFiles(a.output, result) +} + type errorResult struct { Version int `json:"version"` Command string `json:"command"` diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 7d607ec..02e9556 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -237,6 +237,7 @@ func TestHelpAndVersion(t *testing.T) { require.NoError(t, err) require.Contains(t, out, "approve") require.Contains(t, out, "commits") + require.Contains(t, out, "files") require.Contains(t, out, "--json") require.NotContains(t, out, "confirm") require.NotContains(t, out, "challenge") diff --git a/internal/cli/files_functional_test.go b/internal/cli/files_functional_test.go new file mode 100644 index 0000000..9b4153b --- /dev/null +++ b/internal/cli/files_functional_test.go @@ -0,0 +1,75 @@ +package cli + +import ( + "encoding/json/v2" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/nnutter/git-review/internal/review" +) + +func TestRealTuicrFileScopes(t *testing.T) { + env := realTuicrEnvironment(t) + binary := buildGitReview(t) + for _, tc := range []struct { + name string + args []string + cwd string + previous bool + }{ + {name: "nested file", args: []string{"src/nested.txt"}}, + {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}, + {name: "tracked from subdirectory", cwd: "src", previous: true}, + {name: "symlink to nested file", args: []string{"linked.txt"}, previous: true}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := realFeatureRepo(t) + require.NoError(t, os.Mkdir(filepath.Join(dir, "src"), 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "src", "nested.txt"), []byte("Nested file to review\n"), 0o600)) + require.NoError(t, os.Symlink(filepath.Join("src", "nested.txt"), filepath.Join(dir, "linked.txt"))) + git(t, dir, "add", "src/nested.txt") + git(t, dir, "commit", "--amend", "--no-edit") + if tc.previous { + reviewRealFile(t, binary, dir, env, "feature.txt", "Prior root feedback", "feature.txt") + } + 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"]) + }) + } +} + +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...) + session := startTerminal(t, binary, dir, env, command...) + session.waitFor(t, file) + list := runRealCommand(t, "tuicr", dir, env, "review", "list", "--all") + var sessions []struct { + Path string `json:"path"` + Active bool `json:"active"` + } + require.NoError(t, json.Unmarshal([]byte(list), &sessions)) + var active []string + for _, saved := range sessions { + if saved.Active { + active = append(active, saved.Path) + } + } + require.Len(t, active, 1) + runRealCommand(t, "tuicr", dir, env, "review", "add", "--session", active[0], "--target-file", file, "--line", "1", feedback) + session.send(t, ":wq\r") + var result review.FilesResult + require.NoError(t, json.Unmarshal([]byte(session.waitExit(t)), &result)) + require.Equal(t, "feedback", result.Status) + require.Equal(t, active[0], result.Session) + require.NotNil(t, result.Review) + return result +} diff --git a/internal/cli/files_test.go b/internal/cli/files_test.go new file mode 100644 index 0000000..f819dba --- /dev/null +++ b/internal/cli/files_test.go @@ -0,0 +1,143 @@ +package cli + +import ( + "encoding/json/v2" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/nnutter/git-review/internal/review" +) + +func TestFilesCommandArgMapping(t *testing.T) { + dir := featureRepo(t) + require.NoError(t, os.WriteFile(filepath.Join(dir, "file.txt"), []byte("File to review\n"), 0o600)) + t.Setenv("XDG_CACHE_HOME", t.TempDir()) + t.Setenv("XDG_DATA_HOME", t.TempDir()) + bin := t.TempDir() + log := filepath.Join(t.TempDir(), "launch") + t.Setenv("TEST_LAUNCH_LOG", log) + require.NoError(t, os.WriteFile(filepath.Join(bin, "tuicr"), []byte("#!/bin/sh\nprintf '%s\\n' \"$@\" > \"$TEST_LAUNCH_LOG\"\n"), 0o700)) + t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) + for _, tc := range []struct { + name string + args []string + wantArgs string + }{ + {"all files", []string{"files", "--json"}, "tui\n--no-update-check\n--stdout\n--all-files"}, + {"single file", []string{"files", "file.txt", "--json"}, "tui\n--no-update-check\n--stdout\n--file\nfile.txt"}, + {"untracked defaults to dot", []string{"files", "--untracked", "--json"}, "tui\n--no-update-check\n--stdout\n--file\n."}, + } { + t.Run(tc.name, func(t *testing.T) { + _ = os.Remove(log) + out, terminal, err := execute(t, dir, tc.args...) + require.NoError(t, err) + require.Empty(t, terminal) + var result review.FilesResult + require.NoError(t, json.Unmarshal([]byte(out), &result)) + require.Equal(t, 1, result.Version) + require.Equal(t, "files", result.Command) + require.Equal(t, "cancelled", result.Status) + launch, err := os.ReadFile(log) + require.NoError(t, err) + require.Equal(t, tc.wantArgs+"\n", string(launch)) + }) + } +} + +func TestFilesCommandDistinguishesCommits(t *testing.T) { + dir := featureRepo(t) + t.Setenv("XDG_CACHE_HOME", t.TempDir()) + t.Setenv("XDG_DATA_HOME", t.TempDir()) + bin := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(bin, "tuicr"), []byte("#!/bin/sh\nexit 0\n"), 0o700)) + t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) + commitsOut, _, err := execute(t, dir, "commits", "--json") + require.NoError(t, err) + var commits review.Result + require.NoError(t, json.Unmarshal([]byte(commitsOut), &commits)) + require.Equal(t, "commits", commits.Command) + filesOut, _, err := execute(t, dir, "files", "--json") + require.NoError(t, err) + var files review.FilesResult + require.NoError(t, json.Unmarshal([]byte(filesOut), &files)) + require.Equal(t, "files", files.Command) + require.NotEqual(t, commits.Command, files.Command) +} + +func TestFilesCommandTerminalAndErrors(t *testing.T) { + dir := featureRepo(t) + t.Setenv("XDG_CACHE_HOME", t.TempDir()) + t.Setenv("XDG_DATA_HOME", t.TempDir()) + bin := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(bin, "tuicr"), []byte("#!/bin/sh\nexit 0\n"), 0o700)) + t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) + out, terminal, err := execute(t, dir, "files") + require.NoError(t, err) + require.Empty(t, terminal) + require.Contains(t, out, "Files review:") + require.Contains(t, out, "Review cancelled.") + require.Contains(t, out, "No file feedback was saved.") + for _, args := range [][]string{ + {"files", "a", "b", "--json"}, + {"files", "--untracked", "file.txt", "--json"}, + {"files", "--unknown", "--json"}, + } { + out, terminal, err := execute(t, dir, args...) + require.Error(t, err) + require.Empty(t, terminal) + require.Contains(t, out, `"status":"error"`) + require.Contains(t, out, `"command":"files"`) + } +} + +func TestFilesKeepsTuicrOutputOffJSON(t *testing.T) { + dir := featureRepo(t) + t.Setenv("XDG_CACHE_HOME", t.TempDir()) + t.Setenv("XDG_DATA_HOME", t.TempDir()) + bin := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(bin, "tuicr"), []byte("#!/bin/sh\nprintf 'tuicr terminal output\\n' >&2\nprintf 'exported feedback\\n'\n"), 0o700)) + t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) + out, terminal, err := execute(t, dir, "files", "--json") + require.NoError(t, err) + require.Equal(t, "tuicr terminal output\n", terminal) + var result review.FilesResult + require.NoError(t, json.Unmarshal([]byte(out), &result)) + require.Equal(t, "cancelled", result.Status) + require.NotContains(t, out, "tuicr terminal output") + require.NotContains(t, out, "exported feedback") +} + +func TestFilesHoldsWorktreeLock(t *testing.T) { + dir := featureRepo(t) + t.Setenv("XDG_CACHE_HOME", t.TempDir()) + t.Setenv("XDG_DATA_HOME", t.TempDir()) + bin := t.TempDir() + started := filepath.Join(bin, "started") + require.NoError(t, os.WriteFile(filepath.Join(bin, "tuicr"), []byte("#!/bin/sh\nprintf started > \""+started+"\"\nsleep 3\n"), 0o700)) + t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) + // Launch files in background via binary to check lock held during TUI. + binary := buildGitReview(t) + t.Setenv("PATH", filepath.Dir(binary)+string(os.PathListSeparator)+os.Getenv("PATH")) + done := make(chan struct{}) + go func() { + defer close(done) + out, _, _ := executeBinary(t, binary, dir, "files", "--json") + if strings.Contains(out, `"status":"cancelled"`) { + return + } + }() + require.Eventually(t, func() bool { + _, err := os.Stat(started) + return err == nil + }, 5_000_000_000, 10_000_000) + // While files holds the repository-wide lock, status --is-reviewing + // reports true, just as it does during a commits review. + out, _, err := executeBinary(t, binary, dir, "status", "--is-reviewing", "--json") + require.NoError(t, err) + require.Contains(t, out, `"is_reviewing":true`) + <-done +} diff --git a/internal/cli/render.go b/internal/cli/render.go index 00027e9..264d838 100644 --- a/internal/cli/render.go +++ b/internal/cli/render.go @@ -303,3 +303,66 @@ func statusLabel(status string) string { return safeText(status) } } + +func renderFiles(output io.Writer, result review.FilesResult) error { + text, err := renderFilesResult(result) + if err != nil { + return err + } + _, err = fmt.Fprintln(colorprofile.NewWriter(output, os.Environ()), text) + return err +} + +func renderFilesResult(result review.FilesResult) (string, error) { + var out strings.Builder + scope := result.Repo + if result.AllFiles { + fmt.Fprintf(&out, "Files review: all tracked files in %s\n", safeText(scope)) + } else if result.Path != "" { + fmt.Fprintf(&out, "Files review: %s in %s\n", safeText(result.Path), safeText(scope)) + } else { + fmt.Fprintf(&out, "Files review: %s\n", safeText(scope)) + } + if result.Session != "" { + fmt.Fprintf(&out, "Session: %s\n", safeText(result.Session)) + } + if result.Review != nil { + fmt.Fprintf(&out, "Review %s: %d/%d files reviewed\n", safeText(result.Review.ID), result.Review.Reviewed, result.Review.Files) + for _, comment := range result.Review.Comments { + content, _ := comment["content"].(string) + path, _ := comment["path"].(string) + line, _ := comment["stored_line"].(string) + location, _ := comment["location"].(string) + label := location + if path != "" { + label = path + } + if line != "" { + label += ":" + line + } + fmt.Fprintf(&out, " %s: %s\n", safeText(label), safeText(content)) + } + if hasSessionNotes(result.Review.SessionNotes) { + fmt.Fprintln(&out, " Session notes: "+safeText(*result.Review.SessionNotes)) + } + } + if message := filesResultMessage(result); message != "" { + fmt.Fprintln(&out, message) + } + return strings.TrimSpace(out.String()), nil +} + +func filesResultMessage(result review.FilesResult) string { + switch result.Status { + case "approved": + return "Approved. Address any follow-up as new commits." + case "feedback": + return "Feedback requires new commits, logically organized. Do not use fixup commits." + case "incomplete": + return "Review is incomplete, not approved." + case "cancelled": + return "Review cancelled. No file feedback was saved." + default: + return "" + } +} diff --git a/internal/review/files.go b/internal/review/files.go new file mode 100644 index 0000000..7869bd9 --- /dev/null +++ b/internal/review/files.go @@ -0,0 +1,424 @@ +package review + +import ( + "context" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" +) + +// FilesOptions selects a working-tree file review. +type FilesOptions struct { + Path string + Untracked bool + Session string +} + +// FileReview contains validated progress for a file review session. +type FileReview struct { + ID string `json:"id"` + Version string `json:"version"` + Reviewed int `json:"reviewed"` + Files int `json:"files"` + Complete bool `json:"complete"` + Comments []map[string]any `json:"comments"` + SessionNotes *string `json:"sessionNotes"` +} + +// Status never equates a missing or partially reviewed session with approval. +func (r *FileReview) Status() string { + if r == nil { + return "incomplete" + } + if len(r.Comments) > 0 { + return "feedback" + } + if r.SessionNotes != nil { + if *r.SessionNotes != "" { + return "feedback" + } + } + if r.Complete { + return "approved" + } + return "incomplete" +} + +// FilesResult is the shared input to JSON encoding and terminal rendering +// for file reviews. Command is always "files" to distinguish it from +// "commits" stack reviews. +type FilesResult struct { + Version int `json:"version"` + Command string `json:"command"` + Status string `json:"status"` + Repo string `json:"repo"` + Path string `json:"path,omitempty"` + AllFiles bool `json:"allFiles,omitzero"` + Untracked bool `json:"untracked,omitzero"` + Review *FileReview `json:"review,omitempty"` + Session string `json:"session,omitempty"` +} + +// FileReviewer runs tuicr for working-tree files, keeping its terminal output +// separate from results. +type FileReviewer struct { + Service Service + Reader SavedReader + Input io.Reader + Terminal io.Writer +} + +// 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 conflictingFileScope(options) { + return result, fmt.Errorf("--untracked cannot be combined with an explicit path") + } + repo, err := r.Service.Repository.git(ctx, "rev-parse", "--show-toplevel") + if err != nil { + return result, err + } + lock, err := r.lockReview(ctx) + if err != nil { + return result, err + } + defer func() { + // Close releases the repository-wide lock so status --is-reviewing + // reports false after either commits or files reviews exit. + _ = lock.Close() + }() + + allFiles := reviewAllFiles(options) + filePath := options.Path + if reviewUntrackedDirectory(options) { + filePath = "." + } + diffSource := "working_tree" + args := []string{"tui", "--no-update-check", "--stdout"} + if allFiles { + args = append(args, "--all-files") + diffSource = "pristine" + } else { + args = append(args, "--file", filePath) + } + if err := r.launch(ctx, args, lock); err != nil { + return result, err + } + saved, sessionPath, err := r.readReview(filePath, diffSource, options.Session) + if err != nil { + return result, err + } + result = FilesResult{ + Version: 1, + Command: "files", + Repo: repo, + Path: options.Path, + AllFiles: allFiles, + Untracked: options.Untracked, + Session: sessionPath, + } + if reviewUntrackedDirectory(options) { + result.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 + cmd.Stdin = r.Input + // --stdout bypasses tuicr's clipboard confirmation. The TUI uses /dev/tty; + // git-review reads persisted feedback instead of the exported markdown. + cmd.Stdout = io.Discard + cmd.Stderr = r.Terminal + // Keep the worktree locked if git-review dies while tuicr is still running. + cmd.ExtraFiles = []*os.File{lock} + if err := cmd.Run(); err != nil { + return fmt.Errorf("tuicr interrupted or failed; use git-review files to retry: %w", err) + } + return nil +} + +func (r FileReviewer) lockReview(ctx context.Context) (*os.File, error) { + branch, err := r.Service.Repository.reviewBranch(ctx) + if err != nil { + return nil, err + } + path, err := WorktreePath(r.Service.Repository.Dir) + if err != nil { + return nil, err + } + lock, err := lockWorktree(path) + if err != nil { + return nil, err + } + if err := lock.Truncate(0); err != nil { + _ = lock.Close() + return nil, err + } + if _, err := lock.WriteString(branch); err != nil { + _ = lock.Close() + return nil, err + } + return lock, nil +} + +func (r FileReviewer) readReview(path, diffSource, selector string) (*SavedReview, string, error) { + root, err := fileSessionRoot(r.Service.Repository.Dir, path) + if err != nil { + return nil, "", err + } + return r.Reader.ReadFile(root, diffSource, selector) +} + +// 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) { + if path == "" { + path = "." + } + if !filepath.IsAbs(path) { + path = filepath.Join(dir, path) + } + path, err := filepath.Abs(path) + if err != nil { + return "", err + } + path, err = filepath.EvalSymlinks(path) + if err != nil { + return "", err + } + info, err := os.Stat(path) + if err != nil { + return "", err + } + if !info.IsDir() { + path = filepath.Dir(path) + } + return path, nil +} + +func fileReviewResult(result FilesResult, saved *SavedReview) (FilesResult, error) { + if saved == nil || untouchedFileReview(saved) { + result.Status = "cancelled" + return result, nil + } + if err := makeFileCommentsRepoRelative(saved, result.Repo); err != nil { + return result, err + } + review := &FileReview{ + ID: saved.ID, + Version: saved.Version, + Reviewed: saved.Reviewed, + Files: saved.Files, + Complete: saved.Complete, + Comments: saved.Comments, + SessionNotes: saved.SessionNotes, + } + result.Status = review.Status() + result.Review = review + return result, nil +} + +func makeFileCommentsRepoRelative(saved *SavedReview, repo string) error { + for _, comment := range saved.Comments { + path, _ := comment["path"].(string) + if path == "" { + continue + } + relative, err := filepath.Rel(repo, filepath.Join(saved.Scope.Repo, filepath.FromSlash(path))) + if err != nil { + return err + } + comment["path"] = filepath.ToSlash(relative) + } + return nil +} + +func conflictingFileScope(options FilesOptions) bool { + if !options.Untracked { + return false + } + return options.Path != "" +} + +func reviewAllFiles(options FilesOptions) bool { + if options.Untracked { + return false + } + return options.Path == "" +} + +func reviewUntrackedDirectory(options FilesOptions) bool { + if !options.Untracked { + return false + } + return options.Path == "" +} + +func untouchedFileReview(saved *SavedReview) bool { + if saved.Status() != "incomplete" { + return false + } + return saved.Reviewed == 0 +} + +func readFileStore(store, repo, diffSource, selector string) (*SavedReview, string, error) { + entries, err := readIndex(store) + if err != nil { + return nil, "", err + } + if entries == nil { + return nil, "", nil + } + repo, err = filepath.EvalSymlinks(repo) + if err != nil { + return nil, "", err + } + candidates := map[string]*SavedReview{} + for slug, rows := range entries { + for _, entry := range rows { + saved, sessionPath, err := readFileEntry(store, repo, slug, entry, diffSource, selector) + if err != nil { + return nil, "", err + } + if saved != nil { + candidates[sessionPath] = saved + } + } + } + if len(candidates) > 1 { + return nil, "", fmt.Errorf("ambiguous saved reviews; select an exact session with --session") + } + for sessionPath, saved := range candidates { + return saved, sessionPath, nil + } + return nil, "", nil +} + +func readFileEntry(store, repo, slug string, entry object, diffSource, selector string) (*SavedReview, string, error) { + kind, err := decodeRequired[object](entry, "kind") + if err != nil { + return nil, "", err + } + entryType, err := decodeRequired[string](kind, "type") + if err != nil { + return nil, "", err + } + if entryType != "local" { + return nil, "", nil + } + canonical, err := decodeRequired[string](entry, "canonical_repo_path") + if err != nil { + return nil, "", err + } + if filepath.Clean(canonical) != repo { + return nil, "", nil + } + 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 + } + return readFileCandidate(path, repo, diffSource, selector, slug, relative) +} + +func readFileCandidate(path, repo, diffSource, selector, slug, relative string) (*SavedReview, string, error) { + session, err := readObject(path) + if err != nil { + return nil, "", err + } + saved, err := matchFileSession(session, repo, diffSource, selector, slug, relative) + if err != nil { + return nil, "", err + } + if saved != nil { + saved.Slug = slug + saved.Path = path + return saved, path, nil + } + return nil, "", nil +} + +func matchFileSession(session object, repo, diffSource, selector, slug, relative string) (*SavedReview, error) { + source, err := decodeRequired[string](session, "diff_source") + if err != nil { + return nil, err + } + if source != diffSource { + return nil, nil + } + selected, err := matchesSelector(session, selector, slug, relative) + if err != nil { + return nil, err + } + if !selected { + return nil, nil + } + if err := validateSessionRepo(session, repo); err != nil { + return nil, err + } + return decodeFileSession(session, repo) +} + +func decodeFileSessionIdentity(session object, repo string) (*SavedReview, error) { + version, err := decodeRequired[string](session, "version") + if err != nil { + return nil, err + } + if version != "1.3" { + return nil, fmt.Errorf("unsupported saved-session version %q", version) + } + if raw, ok := session["pr_session_key"]; ok { + if string(raw) != "null" { + return nil, fmt.Errorf("expected local file session") + } + } + id, err := decodeRequired[string](session, "id") + if err != nil { + return nil, err + } + if id == "" { + return nil, fmt.Errorf("missing session ID") + } + return &SavedReview{Scope: Stack{Repo: repo}, ID: id, Version: version}, nil +} + +func decodeFileSession(session object, repo string) (*SavedReview, error) { + saved, err := decodeFileSessionIdentity(session, repo) + if err != nil { + return nil, err + } + files, err := decodeRequired[object](session, "files") + if err != nil { + return nil, err + } + collector := commentCollector{comments: []map[string]any{}, ids: map[string]string{}} + if raw, ok := session["review_comments"]; ok { + if err := collector.add(raw, "review", "", ""); err != nil { + return nil, err + } + } + reviewed, err := collector.collectFiles(files) + if err != nil { + return nil, err + } + notes, err := sessionNotes(session) + if err != nil { + return nil, err + } + saved.Reviewed = reviewed + saved.Files = len(files) + saved.Complete = len(files) > 0 && allReviewed(reviewed, len(files)) + saved.Comments = collector.comments + saved.SessionNotes = notes + return saved, nil +} diff --git a/internal/review/files_test.go b/internal/review/files_test.go new file mode 100644 index 0000000..6897c21 --- /dev/null +++ b/internal/review/files_test.go @@ -0,0 +1,170 @@ +package review + +import ( + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +func fileFixture(t *testing.T, repo, diffSource, base string, store string, reviewed bool, sessionFile string) map[string]any { + t.Helper() + require.NoError(t, os.MkdirAll(filepath.Join(store, "sessions"), 0o700)) + session := map[string]any{ + "version": "1.3", "id": "file-session", "repo_path": repo, + "base_commit": base, "diff_source": diffSource, "commit_range": nil, + "files": map[string]any{ + "file.txt": map[string]any{"path": "file.txt", "reviewed": reviewed, "file_comments": []any{}, "line_comments": map[string]any{}}, + }, + } + index := map[string]any{"version": "2.0", "entries": map[string]any{"fixture": []any{map[string]any{ + "kind": map[string]any{"type": "local"}, "canonical_repo_path": repo, "path": sessionFile, + }}}} + writeJSON(t, filepath.Join(store, "index.json"), index) + writeJSON(t, filepath.Join(store, sessionFile), session) + return session +} + +func TestReadFilePristineAndWorkingTree(t *testing.T) { + r := testRepo(t) + gitTest(t, r.Dir, "commit", "--allow-empty", "-m", "Feature") + repo, err := r.git(t.Context(), "rev-parse", "--show-toplevel") + require.NoError(t, err) + for _, tc := range []struct { + name string + diffSource string + base string + }{ + {"pristine", "pristine", "pristine:abc:def"}, + {"working_tree", "working_tree", "file"}, + } { + t.Run(tc.name, func(t *testing.T) { + store := t.TempDir() + fileFixture(t, repo, tc.diffSource, tc.base, store, true, "sessions/0123456789abcdef.json") + saved, sessionPath, err := (SavedReader{Stores: []string{store}}).ReadFile(repo, tc.diffSource, "") + require.NoError(t, err) + require.NotNil(t, saved) + require.Equal(t, "file-session", saved.ID) + require.Equal(t, "approved", saved.Status()) + require.NotEmpty(t, sessionPath) + // Wrong diff source finds nothing. + missing, _, err := (SavedReader{Stores: []string{store}}).ReadFile(repo, "commit_range", "") + require.NoError(t, err) + require.Nil(t, missing) + }) + } +} + +func TestReadFileOutcomes(t *testing.T) { + r := testRepo(t) + gitTest(t, r.Dir, "commit", "--allow-empty", "-m", "Feature") + repo, err := r.git(t.Context(), "rev-parse", "--show-toplevel") + require.NoError(t, err) + for _, tc := range []struct { + name string + mutate func(map[string]any) + status string + }{ + {"approved", func(map[string]any) {}, "approved"}, + {"feedback comment", func(s map[string]any) { + s["review_comments"] = []any{map[string]any{"id": "c1", "content": "Fix this", "comment_type": "none"}} + }, "feedback"}, + {"feedback notes", func(s map[string]any) { s["session_notes"] = "Revise" }, "feedback"}, + {"incomplete", func(s map[string]any) { + s["files"].(map[string]any)["file.txt"].(map[string]any)["reviewed"] = false + }, "incomplete"}, + } { + t.Run(tc.name, func(t *testing.T) { + store := t.TempDir() + session := fileFixture(t, repo, "pristine", "pristine:abc:def", store, true, "sessions/0123456789abcdef.json") + tc.mutate(session) + writeJSON(t, filepath.Join(store, "sessions/0123456789abcdef.json"), session) + saved, _, err := (SavedReader{Stores: []string{store}}).ReadFile(repo, "pristine", "") + require.NoError(t, err) + require.NotNil(t, saved) + require.Equal(t, tc.status, saved.Status()) + }) + } +} + +func TestReadFileAmbiguousRequiresSession(t *testing.T) { + r := testRepo(t) + gitTest(t, r.Dir, "commit", "--allow-empty", "-m", "Feature") + repo, err := r.git(t.Context(), "rev-parse", "--show-toplevel") + require.NoError(t, err) + store := t.TempDir() + fileFixture(t, repo, "working_tree", "file", store, true, "sessions/0123456789abcdef.json") + session := map[string]any{ + "version": "1.3", "id": "second-session", "repo_path": repo, + "base_commit": "file", "diff_source": "working_tree", "commit_range": nil, + "files": map[string]any{ + "other.txt": map[string]any{"path": "other.txt", "reviewed": true, "file_comments": []any{}, "line_comments": map[string]any{}}, + }, + } + writeJSON(t, filepath.Join(store, "sessions/fedcba9876543210.json"), session) + writeJSON(t, filepath.Join(store, "index.json"), map[string]any{"version": "2.0", "entries": map[string]any{"fixture": []any{ + map[string]any{"kind": map[string]any{"type": "local"}, "canonical_repo_path": repo, "path": "sessions/0123456789abcdef.json"}, + map[string]any{"kind": map[string]any{"type": "local"}, "canonical_repo_path": repo, "path": "sessions/fedcba9876543210.json"}, + }}}) + _, _, err = (SavedReader{Stores: []string{store}}).ReadFile(repo, "working_tree", "") + require.ErrorContains(t, err, "ambiguous") + saved, _, err := (SavedReader{Stores: []string{store}}).ReadFile(repo, "working_tree", "second-session") + require.NoError(t, err) + require.NotNil(t, saved) + require.Equal(t, "second-session", saved.ID) +} + +func TestFilesReviewArgMappingAndLock(t *testing.T) { + reviewer, _, _ := testReviewer(t) + r := reviewer.Service.Repository + require.NoError(t, os.WriteFile(filepath.Join(r.Dir, "file.txt"), []byte("File to review\n"), 0o600)) + repo, err := r.git(t.Context(), "rev-parse", "--show-toplevel") + require.NoError(t, err) + // Untracked with explicit path is an error without launching tuicr. + _, err = FileReviewer{Service: reviewer.Service, Reader: reviewer.Reader, Input: strings.NewReader(""), Terminal: io.Discard}.Review(t.Context(), FilesOptions{Path: "file.txt", Untracked: true}) + require.ErrorContains(t, err, "--untracked") + for _, tc := range []struct { + name string + options FilesOptions + wantArgs string + diffSource string + base string + }{ + {"all files", FilesOptions{}, "--all-files", "pristine", "pristine:abc:def"}, + {"single file", FilesOptions{Path: "file.txt"}, "--file\nfile.txt", "working_tree", "file"}, + {"untracked defaults to dot", FilesOptions{Untracked: true}, "--file\n.", "working_tree", "file"}, + } { + t.Run(tc.name, func(t *testing.T) { + log := filepath.Join(t.TempDir(), "launch") + t.Setenv("TEST_LAUNCH_LOG", log) + fakeTuicr(t, `printf '%s\n' "$@" > "$TEST_LAUNCH_LOG"`) + fileStore := t.TempDir() + fileFixture(t, repo, tc.diffSource, tc.base, fileStore, true, "sessions/0123456789abcdef.json") + fr := FileReviewer{Service: reviewer.Service, Reader: SavedReader{Stores: []string{fileStore}}, Input: strings.NewReader(""), Terminal: io.Discard} + result, err := fr.Review(t.Context(), tc.options) + require.NoError(t, err) + require.Equal(t, "files", result.Command) + require.Equal(t, "approved", result.Status) + require.NotNil(t, result.Review) + launch, err := os.ReadFile(log) + require.NoError(t, err) + require.Contains(t, string(launch), "tui\n--no-update-check\n--stdout") + require.Contains(t, string(launch), tc.wantArgs) + }) + } +} + +func TestFilesReviewCancelledWithoutSession(t *testing.T) { + reviewer, _, _ := testReviewer(t) + fakeTuicr(t, "exit 0") + store := t.TempDir() + fr := FileReviewer{Service: reviewer.Service, Reader: SavedReader{Stores: []string{store}}, Input: strings.NewReader(""), Terminal: io.Discard} + result, err := fr.Review(t.Context(), FilesOptions{}) + require.NoError(t, err) + require.Equal(t, "files", result.Command) + require.Equal(t, "cancelled", result.Status) + require.Nil(t, result.Review) +} diff --git a/internal/review/saved.go b/internal/review/saved.go index 500b2aa..5b2b77d 100644 --- a/internal/review/saved.go +++ b/internal/review/saved.go @@ -67,6 +67,22 @@ func (r SavedReader) Read(scope Stack, selector string) (*SavedReview, error) { return nil, nil } +// ReadFile returns the saved file review for a repository and diff source. +// It returns nil only when no matching session exists. Corruption is an error. +// Diff sources are "pristine" for --all-files and "working_tree" for --file. +func (r SavedReader) ReadFile(repo, diffSource, selector string) (*SavedReview, string, error) { + for _, store := range r.Stores { + saved, sessionPath, err := readFileStore(store, repo, diffSource, selector) + if err != nil { + return nil, "", err + } + if saved != nil { + return saved, sessionPath, nil + } + } + return nil, "", nil +} + // DefaultSavedReader uses tuicr's platform store, then the other platform path. func DefaultSavedReader() (SavedReader, error) { home, err := os.UserHomeDir() diff --git a/pi/extensions/git-review/index.ts b/pi/extensions/git-review/index.ts index ef53995..e251519 100644 --- a/pi/extensions/git-review/index.ts +++ b/pi/extensions/git-review/index.ts @@ -2,8 +2,10 @@ * git-review extension. * * Reads `git-review status --needs-review --json` for the current Git feature branch and - * publishes the counts on `git-review:status`. Outside a Git repository it - * publishes nothing. + * publishes the `commits` counts on `git-review:status`. Outside a Git repository it + * publishes nothing. `files` reviews use `"command":"files"` and are addressed + * as new commits, not fixups; they hold the same worktree lock so + * `status --is-reviewing` defers notices during either review. * * Recap (and anything else) can ask for a fresh post with * `git-review:request-status`. The request object may set `refresh(cwd)`. diff --git a/pi/extensions/git-review/support.ts b/pi/extensions/git-review/support.ts index 92c2b96..fdbeccb 100644 --- a/pi/extensions/git-review/support.ts +++ b/pi/extensions/git-review/support.ts @@ -4,6 +4,8 @@ * No Pi imports: node:test can load this file outside the Pi process. * `git-review status --needs-review --json` prints `commits` stacks with a summary of { state, count }. * `git-review status --is-reviewing --json` prints whether a `commits` or `files` review holds the lock. + * `git-review commits --json` returns `"command":"commits"` for fixup workflow. + * `git-review files --json` returns `"command":"files"` for new-commit workflow. * Failures print a versioned object and exit non-zero. Callers check * the exit code before parsing. */ diff --git a/skills/git-review/SKILL.md b/skills/git-review/SKILL.md index 48a5bf2..aeb3e59 100644 --- a/skills/git-review/SKILL.md +++ b/skills/git-review/SKILL.md @@ -1,7 +1,7 @@ --- name: git-review -description: Retrieve saved git-review feedback, address comments, and rewrite Git history into adjacent commit stacks. Use when the user asks to address git-review feedback, insert fixup commits, or prepare amendments for re-review. -compatibility: Requires Git and git-review on PATH, a feature branch with a configured upstream, and permission to rewrite local history. +description: Retrieve saved git-review feedback, address comments, and prepare commits for re-review. Use when the user asks to address git-review feedback, insert fixup commits, or create new commits for file reviews. +compatibility: Requires Git and git-review on PATH, a feature branch with a configured upstream for commit reviews, and permission to change history for fixups. --- # Address git-review feedback @@ -44,6 +44,19 @@ The summary counts the full feature range even when entries are filtered. Use `git-review feedback --json` for saved comments and session notes. Counts do not identify feedback targets or authorize approval. +## Distinguish commits and files output + +`git-review commits --json` reviews commit stacks and returns `"command":"commits"`. +Address its feedback with fixup commits inserted immediately after their target stacks. + +`git-review files [] --json` reviews working-tree files and returns `"command":"files"`. +It runs `tuicr tui --all-files` without a path, `tuicr tui --file ` with a path, and `tuicr tui --file .` with `--untracked` and no path. +Address its feedback as one or more new commits, logically organized and sized. +Do not use `fixup!`, `amend!`, or `reword!` commits for file feedback. + +Check the result's `command` before choosing a workflow. +Do not infer the workflow from file paths or comment text alone. + ## Retrieve feedback Run this command from the feature checkout: @@ -185,6 +198,16 @@ Do not use `fixup`, `squash`, or `reword` todo actions to consume these commits Git reword fixups normally have an `amend!` subject. Git generates nested feedback subjects when the target is an existing feedback commit. +## Create new commits for file feedback + +File reviews do not use stacks, fixups, or history rewrites. +Make fixes in the feature checkout on the feature branch. +Create one or more new commits with normal messages, logically organized and sized. +Do not use `--fixup`, `--fixup=amend:`, or `--fixup=reword:` for file feedback. +Do not rebase or reorder existing commits to insert file fixes. +Test the final history with the repository's required checks. +Record the new commit IDs for the report. + ## Check and report 1. Check that the checkout uses the feature branch after integration. @@ -200,4 +223,4 @@ Keep backup branches until the user requests their removal. Remove temporary fix branches only after checking their commits are reachable from the feature branch. Leave recovery branches intact when integration fails. -Tell the user that the changes are ready for re-review with `git-review commits`. +Tell the user that the changes are ready for re-review with `git-review commits` for stack feedback or `git-review files` for file feedback. From ae69c03bbb6f63fe4a7e438832a7430a1d8c4cd1 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Sun, 18 Oct 2026 21:51:18 -0500 Subject: [PATCH 13/22] Refactor: Move prompt helpers into their own file Separate interactive prompting from command assembly to prepare for moving command implementations into independent packages. --- internal/cli/cli.go | 27 --------------------------- internal/cli/prompt.go | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 27 deletions(-) create mode 100644 internal/cli/prompt.go diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 107be52..c95f8c9 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -14,7 +14,6 @@ import ( "charm.land/huh/v2" "github.com/charmbracelet/fang" - "github.com/charmbracelet/x/term" "github.com/spf13/cobra" "github.com/nnutter/git-review/internal/review" @@ -251,32 +250,6 @@ type errorResult struct { Error string `json:"error"` } -func canPrompt(input io.Reader, output io.Writer) bool { - 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") - } - choice := "cancel" - selectField := huh.NewSelect[string]().Title("Resume a saved review?").Description(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 -} - func wantsPatch(patch, difft, hunk bool) bool { if patch { return true diff --git a/internal/cli/prompt.go b/internal/cli/prompt.go new file mode 100644 index 0000000..171900b --- /dev/null +++ b/internal/cli/prompt.go @@ -0,0 +1,39 @@ +package cli + +import ( + "context" + "fmt" + "io" + "os" + + "charm.land/huh/v2" + "github.com/charmbracelet/x/term" + + "github.com/nnutter/git-review/internal/review" +) + +func canPrompt(input io.Reader, output io.Writer) bool { + 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") + } + choice := "cancel" + selectField := huh.NewSelect[string]().Title("Resume a saved review?").Description(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 +} From 5141512f32745e2b68d8d7aca9d02a2eebfc829f Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Sun, 18 Oct 2026 21:52:34 -0500 Subject: [PATCH 14/22] Refactor: Move rendering and JSON helpers to display package Move the existing helpers so upcoming command packages can reuse them without importing the code that assembles commands. --- internal/cli/cli.go | 50 +++++------------------------ internal/cli/cli_test.go | 5 +-- internal/cli/feedback_test.go | 3 +- internal/cli/prompt.go | 3 +- internal/cli/recovery_test.go | 3 +- internal/cli/status_test.go | 15 +++++---- internal/display/json.go | 43 +++++++++++++++++++++++++ internal/{cli => display}/render.go | 34 ++++++++++---------- 8 files changed, 85 insertions(+), 71 deletions(-) create mode 100644 internal/display/json.go rename internal/{cli => display}/render.go (91%) diff --git a/internal/cli/cli.go b/internal/cli/cli.go index c95f8c9..bc3cda9 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -3,25 +3,24 @@ package cli import ( "context" - "encoding/json/v2" "errors" "fmt" "io" "os" "os/exec" - "strings" "syscall" "charm.land/huh/v2" "github.com/charmbracelet/fang" "github.com/spf13/cobra" + "github.com/nnutter/git-review/internal/display" "github.com/nnutter/git-review/internal/review" ) // Execute runs a command with explicit process streams and checkout location. func Execute(ctx context.Context, args []string, dir string, input io.Reader, output, terminal io.Writer, version string) error { - app := application{json: wantsJSON(args), service: review.Service{Repository: review.Repository{Dir: dir}}, input: input, output: output, terminal: terminal} + app := application{json: display.WantsJSON(args), service: review.Service{Repository: review.Repository{Dir: dir}}, input: input, output: output, terminal: terminal} root := app.command() root.SetArgs(args) root.SetIn(input) @@ -33,7 +32,7 @@ func Execute(ctx context.Context, args []string, dir string, input io.Reader, ou if failed == nil { failed = root } - app.outputError = writeJSON(output, errorResult{Version: 1, Command: failed.Name(), Status: "error", Error: err.Error()}) + app.outputError = display.WriteJSON(output, display.ErrorResult{Version: 1, Command: failed.Name(), Status: "error", Error: err.Error()}) return } fang.DefaultErrorHandler(w, styles, err) @@ -80,7 +79,7 @@ func (a *application) command() *cobra.Command { return err } if a.json { - return writeJSON(a.output, struct { + return display.WriteJSON(a.output, struct { Version int `json:"version"` Command string `json:"command"` IsReviewing bool `json:"is_reviewing"` @@ -231,23 +230,16 @@ func (a *application) runReview(ctx context.Context, options review.ReviewOption func (a *application) write(result review.Result) error { if a.json { - return writeJSON(a.output, result) + return display.WriteJSON(a.output, result) } - return render(a.output, result, a.verbose) + return display.Render(a.output, result, a.verbose) } func (a *application) writeFiles(result review.FilesResult) error { if a.json { - return writeJSON(a.output, result) + return display.WriteJSON(a.output, result) } - return renderFiles(a.output, result) -} - -type errorResult struct { - Version int `json:"version"` - Command string `json:"command"` - Status string `json:"status"` - Error string `json:"error"` + return display.RenderFiles(a.output, result) } func wantsPatch(patch, difft, hunk bool) bool { @@ -259,29 +251,3 @@ func wantsPatch(patch, difft, hunk bool) bool { } return hunk } - -// Determine error output even if Cobra fails before it parses persistent flags. -func wantsJSON(args []string) bool { - enabled := false - for _, arg := range args { - if arg == "--" { - break - } - switch arg { - case "--json", "--json=true": - enabled = true - case "--json=false": - enabled = false - } - } - return enabled -} - -func writeJSON(output io.Writer, value any) error { - data, err := json.Marshal(value) - if err != nil { - return err - } - _, err = fmt.Fprintln(output, strings.TrimSpace(string(data))) - return err -} diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 02e9556..0c60bcb 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/require" + "github.com/nnutter/git-review/internal/display" "github.com/nnutter/git-review/internal/review" ) @@ -108,7 +109,7 @@ func TestJSONErrorsIncludeArgumentAndGitFailures(t *testing.T) { require.Equal(t, 1, exit.ExitCode()) require.Empty(t, terminal) require.Equal(t, 1, strings.Count(out, "\n")) - var result errorResult + var result display.ErrorResult require.NoError(t, json.Unmarshal([]byte(out), &result)) require.Equal(t, "error", result.Status) require.NotEmpty(t, result.Error) @@ -227,7 +228,7 @@ func TestTuicrKeepsWorktreeLockedAfterGitReviewDies(t *testing.T) { require.Error(t, cmd.Wait()) out, _, err := execute(t, dir, "commits", "--replace", "--json") require.ErrorContains(t, err, "worktree is in use") - var result errorResult + var result display.ErrorResult require.NoError(t, json.Unmarshal([]byte(out), &result)) require.Equal(t, "error", result.Status) } diff --git a/internal/cli/feedback_test.go b/internal/cli/feedback_test.go index 3f6d2b7..349d79c 100644 --- a/internal/cli/feedback_test.go +++ b/internal/cli/feedback_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/require" + "github.com/nnutter/git-review/internal/display" "github.com/nnutter/git-review/internal/review" ) @@ -123,7 +124,7 @@ func TestFeedbackDoesNotLaunchOrRecoverReviews(t *testing.T) { out, terminal, err := execute(t, dir, "feedback", "--json") require.ErrorContains(t, err, "invalid stored review no-notes") require.Empty(t, terminal) - var failed errorResult + var failed display.ErrorResult require.NoError(t, json.Unmarshal([]byte(out), &failed)) require.Equal(t, "feedback", failed.Command) require.Equal(t, "error", failed.Status) diff --git a/internal/cli/prompt.go b/internal/cli/prompt.go index 171900b..1c388fa 100644 --- a/internal/cli/prompt.go +++ b/internal/cli/prompt.go @@ -9,6 +9,7 @@ import ( "charm.land/huh/v2" "github.com/charmbracelet/x/term" + "github.com/nnutter/git-review/internal/display" "github.com/nnutter/git-review/internal/review" ) @@ -29,7 +30,7 @@ func promptChoice(ctx context.Context, input io.Reader, output io.Writer, decisi return "", fmt.Errorf("missing recovery decision") } choice := "cancel" - selectField := huh.NewSelect[string]().Title("Resume a saved review?").Description(recoveryDescription(decision)).Options( + 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"), diff --git a/internal/cli/recovery_test.go b/internal/cli/recovery_test.go index e28ab79..8d492cd 100644 --- a/internal/cli/recovery_test.go +++ b/internal/cli/recovery_test.go @@ -6,6 +6,7 @@ import ( "github.com/stretchr/testify/require" + "github.com/nnutter/git-review/internal/display" "github.com/nnutter/git-review/internal/review" ) @@ -35,7 +36,7 @@ func TestRecoveryDecisionExplainsSavedWork(t *testing.T) { case "none": result.Decision.Replacement = nil } - text, err := renderResult(result, false) + 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") diff --git a/internal/cli/status_test.go b/internal/cli/status_test.go index 6493abd..e046e7e 100644 --- a/internal/cli/status_test.go +++ b/internal/cli/status_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/require" + "github.com/nnutter/git-review/internal/display" "github.com/nnutter/git-review/internal/review" ) @@ -66,17 +67,17 @@ func TestStatusCountsCurrentCommitsInStateOrder(t *testing.T) { require.NotContains(t, out, "Saved review details") require.Contains(t, out, "Active attempt: reviewing") for i, stack := range view.Stacks { - require.Contains(t, out, shortStack(stack)) + require.Contains(t, out, display.ShortStack(stack)) require.Contains(t, out, stack.Subject) - require.Contains(t, out, statusLabel(want[i].State)) + require.Contains(t, out, display.StatusLabel(want[i].State)) } filtered, terminal, err := executeBinary(t, binary, dir, "status", "--needs-review") require.NoError(t, err) require.Empty(t, terminal) for _, stack := range view.Stacks[:4] { - require.Contains(t, filtered, shortStack(stack)) + require.Contains(t, filtered, display.ShortStack(stack)) } - require.NotContains(t, filtered, shortStack(view.Stacks[4])) + require.NotContains(t, filtered, display.ShortStack(view.Stacks[4])) filtered, _, err = executeBinary(t, binary, dir, "status", "--needs-review", "--json") require.NoError(t, err) var result review.Result @@ -180,7 +181,7 @@ func TestStatusOutsideGitRepository(t *testing.T) { require.Equal(t, 1, exit.ExitCode()) if len(args) == 2 { require.Empty(t, terminal) - var failed errorResult + var failed display.ErrorResult require.NoError(t, json.Unmarshal([]byte(out), &failed)) require.Equal(t, "status", failed.Command) require.Equal(t, "error", failed.Status) @@ -202,7 +203,7 @@ func TestStatusJSONErrors(t *testing.T) { out, terminal, err := execute(t, dir, args...) require.Error(t, err) require.Empty(t, terminal) - var failed errorResult + var failed display.ErrorResult require.NoError(t, json.Unmarshal([]byte(out), &failed)) require.Equal(t, "status", failed.Command) require.Equal(t, "error", failed.Status) @@ -211,7 +212,7 @@ func TestStatusJSONErrors(t *testing.T) { out, terminal, err := execute(t, dir, "status", "--json") require.Error(t, err) require.Empty(t, terminal) - var failed errorResult + var failed display.ErrorResult require.NoError(t, json.Unmarshal([]byte(out), &failed)) require.Equal(t, "status", failed.Command) require.Equal(t, "error", failed.Status) diff --git a/internal/display/json.go b/internal/display/json.go new file mode 100644 index 0000000..14799ec --- /dev/null +++ b/internal/display/json.go @@ -0,0 +1,43 @@ +// Package display presents CLI helpers used by every git-review subcommand. +package display + +import ( + "encoding/json/v2" + "fmt" + "io" + "strings" +) + +// ErrorResult is the versioned JSON object for command failures. +type ErrorResult struct { + Version int `json:"version"` + Command string `json:"command"` + Status string `json:"status"` + Error string `json:"error"` +} + +// Determine error output even if Cobra fails before it parses persistent flags. +func WantsJSON(args []string) bool { + enabled := false + for _, arg := range args { + if arg == "--" { + break + } + switch arg { + case "--json", "--json=true": + enabled = true + case "--json=false": + enabled = false + } + } + return enabled +} + +func WriteJSON(output io.Writer, value any) error { + data, err := json.Marshal(value) + if err != nil { + return err + } + _, err = fmt.Fprintln(output, strings.TrimSpace(string(data))) + return err +} diff --git a/internal/cli/render.go b/internal/display/render.go similarity index 91% rename from internal/cli/render.go rename to internal/display/render.go index 264d838..948b6e6 100644 --- a/internal/cli/render.go +++ b/internal/display/render.go @@ -1,4 +1,4 @@ -package cli +package display import ( "encoding/json/v2" @@ -16,13 +16,13 @@ import ( "github.com/nnutter/git-review/internal/review" ) -func render(output io.Writer, result review.Result, verbose bool) error { +func Render(output io.Writer, result review.Result, verbose bool) error { var text string var err error if result.Command == "feedback" { text, err = renderFeedbackResult(result) } else { - text, err = renderResult(result, verbose) + text, err = RenderResult(result, verbose) } if err != nil { return err @@ -31,7 +31,7 @@ func render(output io.Writer, result review.Result, verbose bool) error { return err } -func renderResult(result review.Result, verbose bool) (string, error) { +func RenderResult(result review.Result, verbose bool) (string, error) { var out strings.Builder if showStatusDetails(result, verbose) { if err := renderDetails(&out, result.Entries); err != nil { @@ -42,7 +42,7 @@ func renderResult(result review.Result, verbose bool) (string, error) { fmt.Fprintln(&out, renderStackTable(result.Entries)) } for _, attempt := range result.Active { - fmt.Fprintf(&out, "Active attempt: %s %s\n", safeText(attempt.ID), shortStack(attempt.Stack)) + fmt.Fprintf(&out, "Active attempt: %s %s\n", safeText(attempt.ID), ShortStack(attempt.Stack)) } if result.Command != "status" { if err := renderDetails(&out, result.Entries); err != nil { @@ -53,7 +53,7 @@ func renderResult(result review.Result, verbose bool) (string, error) { fmt.Fprintln(&out, message) } if result.Decision != nil { - fmt.Fprintln(&out, recoveryDescription(result.Decision)) + fmt.Fprintln(&out, RecoveryDescription(result.Decision)) fmt.Fprintln(&out, "Choose git-review commits --recover, git-review commits --replace, or leave the attempt unchanged.") } return strings.TrimSpace(out.String()), nil @@ -62,13 +62,13 @@ func renderResult(result review.Result, verbose bool) (string, error) { func renderStackTable(entries []review.Entry) string { rows := make([][]string, 0, len(entries)) for _, entry := range entries { - label := statusLabel(entry.Status) + label := StatusLabel(entry.Status) if entry.Previous != nil { if entry.Status == "needs_review" { label += " (changed)" } } - rows = append(rows, []string{label, shortStack(entry.Stack), safeText(entry.Stack.Subject)}) + rows = append(rows, []string{label, ShortStack(entry.Stack), safeText(entry.Stack.Subject)}) } t := table.New().Headers("STATUS", "COMMITS (inclusive)", "SUBJECT").Rows(rows...).Border(lipgloss.RoundedBorder()).StyleFunc(func(row, _ int) lipgloss.Style { style := lipgloss.NewStyle().Padding(0, 1) @@ -80,15 +80,15 @@ func renderStackTable(entries []review.Entry) string { return t.Render() } -func recoveryDescription(decision *review.Decision) string { +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)) + 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)) + 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") @@ -104,7 +104,7 @@ func recoveryDescription(decision *review.Decision) string { } 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)) + 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 git-review commits .") @@ -176,7 +176,7 @@ func renderFeedbackResult(result review.Result) (string, error) { fmt.Fprintln(&out, "Target: "+safeText(result.View.Target)) fmt.Fprintln(&out, "State: "+safeText(result.View.Path)) for _, attempt := range result.Active { - fmt.Fprintf(&out, "Active attempt: %s %s\n", safeText(attempt.ID), shortStack(attempt.Stack)) + fmt.Fprintf(&out, "Active attempt: %s %s\n", safeText(attempt.ID), ShortStack(attempt.Stack)) } if err := renderFeedback(&out, result.Feedback); err != nil { return "", err @@ -195,7 +195,7 @@ func renderFeedback(out *strings.Builder, records []review.FeedbackRecord) error if record.Historical { label = "Historical feedback" } - fmt.Fprintf(out, "%s: %s %s %s\n", label, safeText(record.Attempt.ID), shortStack(record.Attempt.Stack), safeText(record.Attempt.Stack.Subject)) + fmt.Fprintf(out, "%s: %s %s %s\n", label, safeText(record.Attempt.ID), ShortStack(record.Attempt.Stack), safeText(record.Attempt.Stack.Subject)) if record.Attempt.Response != "" { fmt.Fprintln(out, " Response: "+safeText(record.Attempt.Response)) } @@ -272,7 +272,7 @@ func safeText(value string) string { }, ansi.Strip(value)) } -func shortStack(stack review.Stack) string { +func ShortStack(stack review.Stack) string { first := stack.Commits[0] if len(first) > 8 { first = first[:8] @@ -287,7 +287,7 @@ func shortStack(stack review.Stack) string { return first } -func statusLabel(status string) string { +func StatusLabel(status string) string { switch status { case "needs_review": return "Needs review" @@ -304,7 +304,7 @@ func statusLabel(status string) string { } } -func renderFiles(output io.Writer, result review.FilesResult) error { +func RenderFiles(output io.Writer, result review.FilesResult) error { text, err := renderFilesResult(result) if err != nil { return err From 3f7a647bc21e204613368dd023aca431148ef518 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Sun, 18 Oct 2026 21:53:49 -0500 Subject: [PATCH 15/22] Refactor: Extract output selection helpers for reuse Replace the application methods `write` and `writeFiles` with reusable helpers and convert all callers so later command package moves need only relocate code. --- internal/cli/cli.go | 24 +++++------------------- internal/display/write.go | 23 +++++++++++++++++++++++ 2 files changed, 28 insertions(+), 19 deletions(-) create mode 100644 internal/display/write.go diff --git a/internal/cli/cli.go b/internal/cli/cli.go index bc3cda9..7793c5d 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -58,7 +58,7 @@ func (a *application) command() *cobra.Command { if err != nil { return err } - return a.write(result) + return display.Write(a.output, a.json, result, a.verbose) }} approve := &cobra.Command{Use: "approve []", Aliases: []string{"confirm"}, Short: "Record explicit user approval of complete stacks", Long: "Record user approval without launching tuicr. Endpoints are inclusive and\nmust include complete stacks, including all attached feedback commits.", Args: cobra.RangeArgs(1, 2), RunE: func(cmd *cobra.Command, args []string) error { last := "" @@ -69,7 +69,7 @@ func (a *application) command() *cobra.Command { if err != nil { return err } - return a.write(result) + return display.Write(a.output, a.json, result, a.verbose) }} var onlyNeedsReview, isReviewing bool status := &cobra.Command{Use: "status", Short: "Show all feature stacks and their review states", Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { @@ -92,7 +92,7 @@ func (a *application) command() *cobra.Command { if err != nil { return err } - return a.write(result) + return display.Write(a.output, a.json, result, a.verbose) }} status.Flags().BoolVarP(&a.verbose, "verbose", "v", false, "Show saved review details before the table") status.Flags().BoolVar(&onlyNeedsReview, "needs-review", false, "Show unresolved stacks and active review attempts") @@ -203,7 +203,7 @@ func (a *application) runFiles(ctx context.Context, options review.FilesOptions) if err != nil { return err } - return a.writeFiles(result) + return display.WriteFiles(a.output, a.json, result) } func (a *application) runReview(ctx context.Context, options review.ReviewOptions) error { @@ -225,21 +225,7 @@ func (a *application) runReview(ctx context.Context, options review.ReviewOption if err != nil { return err } - return a.write(result) -} - -func (a *application) write(result review.Result) error { - if a.json { - return display.WriteJSON(a.output, result) - } - return display.Render(a.output, result, a.verbose) -} - -func (a *application) writeFiles(result review.FilesResult) error { - if a.json { - return display.WriteJSON(a.output, result) - } - return display.RenderFiles(a.output, result) + return display.Write(a.output, a.json, result, a.verbose) } func wantsPatch(patch, difft, hunk bool) bool { diff --git a/internal/display/write.go b/internal/display/write.go new file mode 100644 index 0000000..e92f79b --- /dev/null +++ b/internal/display/write.go @@ -0,0 +1,23 @@ +package display + +import ( + "io" + + "github.com/nnutter/git-review/internal/review" +) + +// Write emits a stack result as JSON or terminal text. +func Write(w io.Writer, asJSON bool, result review.Result, verbose bool) error { + if asJSON { + return WriteJSON(w, result) + } + return Render(w, result, verbose) +} + +// WriteFiles emits a file result as JSON or terminal text. +func WriteFiles(w io.Writer, asJSON bool, result review.FilesResult) error { + if asJSON { + return WriteJSON(w, result) + } + return RenderFiles(w, result) +} From 8fb3dcc9df17bef87d53cfdf9e750e2a0dd6058c Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Sun, 18 Oct 2026 21:55:09 -0500 Subject: [PATCH 16/22] Refactor: Move commits command into its own package Separate the stack-review flags, execution, and interactive prompts from root command assembly so the root only installs the command. --- internal/cli/cli.go | 52 +--------------- internal/cli/commits/commits.go | 89 ++++++++++++++++++++++++++++ internal/cli/{ => commits}/prompt.go | 2 +- 3 files changed, 93 insertions(+), 50 deletions(-) create mode 100644 internal/cli/commits/commits.go rename internal/cli/{ => commits}/prompt.go (98%) diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 7793c5d..de811c5 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -10,10 +10,10 @@ import ( "os/exec" "syscall" - "charm.land/huh/v2" "github.com/charmbracelet/fang" "github.com/spf13/cobra" + "github.com/nnutter/git-review/internal/cli/commits" "github.com/nnutter/git-review/internal/display" "github.com/nnutter/git-review/internal/review" ) @@ -99,7 +99,8 @@ func (a *application) command() *cobra.Command { status.Flags().BoolVar(&isReviewing, "is-reviewing", false, "Check whether a review is running for the current branch") status.MarkFlagsMutuallyExclusive("needs-review", "is-reviewing") status.MarkFlagsMutuallyExclusive("verbose", "is-reviewing") - root.AddCommand(feedback, approve, status, a.runCommand(), a.filesCommand(), a.logCommand()) + root.AddCommand(feedback, approve, status, a.filesCommand(), a.logCommand()) + commits.Install(root, a.service, a.input, a.output, a.terminal) return root } @@ -168,31 +169,6 @@ func (a *application) logCommand() *cobra.Command { return cmd } -func (a *application) runCommand() *cobra.Command { - var recoverReview, replaceReview bool - var session, start, end string - cmd := &cobra.Command{Use: "commits []", Aliases: []string{"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: func(cmd *cobra.Command, args []string) error { - options := review.ReviewOptions{Session: session, Start: start, End: end} - if len(args) > 0 { - options.Stack = args[0] - } - if recoverReview { - options.Choice = "recover" - } - if replaceReview { - options.Choice = "replace" - } - return a.runReview(cmd.Context(), options) - }} - cmd.Flags().StringVar(&start, "start", "", "Review from the stack containing this commit (inclusive)") - cmd.Flags().StringVar(&end, "end", "", "Review through the stack containing this commit (inclusive)") - cmd.Flags().BoolVar(&recoverReview, "recover", false, "Reopen the captured attempt in tuicr, preserving saved progress") - cmd.Flags().BoolVar(&replaceReview, "replace", false, "Launch a new attempt, retaining previous review history") - cmd.Flags().StringVar(&session, "session", "", "Select an exact saved session ID, slug, or indexed session path") - cmd.MarkFlagsMutuallyExclusive("recover", "replace") - return cmd -} - func (a *application) runFiles(ctx context.Context, options review.FilesOptions) error { reader, err := review.DefaultSavedReader() if err != nil { @@ -206,28 +182,6 @@ func (a *application) runFiles(ctx context.Context, options review.FilesOptions) return display.WriteFiles(a.output, a.json, result) } -func (a *application) runReview(ctx context.Context, options review.ReviewOptions) error { - reader, err := review.DefaultSavedReader() - if err != nil { - return err - } - runner := review.Reviewer{Service: a.service, Reader: reader, Input: a.input, Terminal: a.terminal} - if !a.json && canPrompt(a.input, a.terminal) { - runner.Choose = func(ctx context.Context, decision *review.Decision) (string, error) { - choice, err := promptChoice(ctx, a.input, a.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 - } - return display.Write(a.output, a.json, result, a.verbose) -} - func wantsPatch(patch, difft, hunk bool) bool { if patch { return true diff --git a/internal/cli/commits/commits.go b/internal/cli/commits/commits.go new file mode 100644 index 0000000..f6bd6f9 --- /dev/null +++ b/internal/cli/commits/commits.go @@ -0,0 +1,89 @@ +// Package commits reviews feature-branch commit stacks with tuicr. +package commits + +import ( + "context" + "errors" + "io" + + "charm.land/huh/v2" + "github.com/spf13/cobra" + + "github.com/nnutter/git-review/internal/display" + "github.com/nnutter/git-review/internal/review" +) + +// Command houses flag values and positional arguments for the commits review. +type Command struct { + Service review.Service + Input io.Reader + Output io.Writer + Terminal io.Writer + Stack string + Start string + End string + Session string + Recover bool + Replace bool +} + +// New builds the commits sub-command. Callers install it onto the parent +// 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{"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.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 +} + +// Install adds the commits sub-command to parent. +func Install(parent *cobra.Command, svc review.Service, in io.Reader, out, term io.Writer) { + parent.AddCommand(New(svc, in, out, term)) +} + +// RunE runs the commits review workflow. +func (c *Command) RunE(cmd *cobra.Command, args []string) error { + options := review.ReviewOptions{Session: c.Session, Start: c.Start, End: c.End} + if len(args) > 0 { + 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) +} + +func (c *Command) runReview(ctx context.Context, options review.ReviewOptions, cmd *cobra.Command) error { + asJSON := false + if flag, err := cmd.Root().PersistentFlags().GetBool("json"); err == nil { + asJSON = flag + } + reader, err := review.DefaultSavedReader() + if err != nil { + 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 + } + return display.Write(c.Output, asJSON, result, false) +} diff --git a/internal/cli/prompt.go b/internal/cli/commits/prompt.go similarity index 98% rename from internal/cli/prompt.go rename to internal/cli/commits/prompt.go index 1c388fa..c857442 100644 --- a/internal/cli/prompt.go +++ b/internal/cli/commits/prompt.go @@ -1,4 +1,4 @@ -package cli +package commits import ( "context" From 1567cfb3372cf79d3e7ebaa51b67a8dc4ede6dfd Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Sun, 18 Oct 2026 23:51:47 -0500 Subject: [PATCH 17/22] Refactor: Extract files command to its own package Move command setup, flag values, and execution into the files package so the root command only installs the files command. --- internal/cli/cli.go | 32 ++------------------ internal/cli/files/files.go | 60 +++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 29 deletions(-) create mode 100644 internal/cli/files/files.go diff --git a/internal/cli/cli.go b/internal/cli/cli.go index de811c5..715ef7f 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -14,6 +14,7 @@ import ( "github.com/spf13/cobra" "github.com/nnutter/git-review/internal/cli/commits" + "github.com/nnutter/git-review/internal/cli/files" "github.com/nnutter/git-review/internal/display" "github.com/nnutter/git-review/internal/review" ) @@ -99,26 +100,12 @@ func (a *application) command() *cobra.Command { status.Flags().BoolVar(&isReviewing, "is-reviewing", false, "Check whether a review is running for the current branch") status.MarkFlagsMutuallyExclusive("needs-review", "is-reviewing") status.MarkFlagsMutuallyExclusive("verbose", "is-reviewing") - root.AddCommand(feedback, approve, status, a.filesCommand(), a.logCommand()) + root.AddCommand(feedback, approve, status, a.logCommand()) commits.Install(root, a.service, a.input, a.output, a.terminal) + files.Install(root, a.service, a.input, a.output, a.terminal) return root } -func (a *application) filesCommand() *cobra.Command { - var untracked bool - var session string - cmd := &cobra.Command{Use: "files []", Short: "Review working-tree files with tuicr", Long: "Review working-tree files with tuicr. Without a path, review every tracked\nfile with tuicr tui --all-files. With a path, review that file or directory\nwith tuicr tui --file. With --untracked and no path, review untracked files\nin the current directory with tuicr tui --file . Address file feedback as new commits,\nlogically organized, not fixup commits.", Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - options := review.FilesOptions{Session: session, Untracked: untracked} - if len(args) > 0 { - options.Path = args[0] - } - return a.runFiles(cmd.Context(), options) - }} - cmd.Flags().BoolVar(&untracked, "untracked", false, "Review untracked files in the current directory (defaults path to .)") - cmd.Flags().StringVar(&session, "session", "", "Select an exact saved session ID, slug, or indexed session path") - return cmd -} - func (a *application) logCommand() *cobra.Command { var patch, ignoreAllSpace, stat, difft, hunk bool cmd := &cobra.Command{ @@ -169,19 +156,6 @@ func (a *application) logCommand() *cobra.Command { return cmd } -func (a *application) runFiles(ctx context.Context, options review.FilesOptions) error { - reader, err := review.DefaultSavedReader() - if err != nil { - return err - } - reviewer := review.FileReviewer{Service: a.service, Reader: reader, Input: a.input, Terminal: a.terminal} - result, err := reviewer.Review(ctx, options) - if err != nil { - return err - } - return display.WriteFiles(a.output, a.json, result) -} - func wantsPatch(patch, difft, hunk bool) bool { if patch { return true diff --git a/internal/cli/files/files.go b/internal/cli/files/files.go new file mode 100644 index 0000000..4d8aad8 --- /dev/null +++ b/internal/cli/files/files.go @@ -0,0 +1,60 @@ +// Package files reviews working-tree files with tuicr. +package files + +import ( + "io" + + "github.com/spf13/cobra" + + "github.com/nnutter/git-review/internal/display" + "github.com/nnutter/git-review/internal/review" +) + +// Command houses flag values and positional arguments for the files review. +type Command struct { + Service review.Service + Input io.Reader + Output io.Writer + Terminal io.Writer + Path string + Untracked bool + Session string +} + +// New builds the files sub-command. Callers install it onto the parent +// 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: "files []", Short: "Review working-tree files with tuicr", Long: "Review working-tree files with tuicr. Without a path, review every tracked\nfile with tuicr tui --all-files. With a path, review that file or directory\nwith tuicr tui --file. With --untracked and no path, review untracked files\nin the current directory with tuicr tui --file . Address file feedback as new commits,\nlogically organized, not fixup commits.", Args: cobra.MaximumNArgs(1), RunE: c.RunE} + cmd.Flags().BoolVar(&c.Untracked, "untracked", false, "Review untracked files in the current directory (defaults path to .)") + cmd.Flags().StringVar(&c.Session, "session", "", "Select an exact saved session ID, slug, or indexed session path") + return cmd +} + +// Install adds the files sub-command to parent. +func Install(parent *cobra.Command, svc review.Service, in io.Reader, out, term io.Writer) { + parent.AddCommand(New(svc, in, out, term)) +} + +// RunE runs the files review workflow. +func (c *Command) RunE(cmd *cobra.Command, args []string) error { + options := review.FilesOptions{Session: c.Session, Untracked: c.Untracked} + if len(args) > 0 { + options.Path = args[0] + c.Path = args[0] + } + asJSON := false + if flag, err := cmd.Root().PersistentFlags().GetBool("json"); err == nil { + asJSON = flag + } + reader, err := review.DefaultSavedReader() + if err != nil { + return err + } + reviewer := review.FileReviewer{Service: c.Service, Reader: reader, Input: c.Input, Terminal: c.Terminal} + result, err := reviewer.Review(cmd.Context(), options) + if err != nil { + return err + } + return display.WriteFiles(c.Output, asJSON, result) +} From 7fceb0fb52a68c1fddea933dfcc6be50b7506e46 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Sun, 18 Oct 2026 23:52:32 -0500 Subject: [PATCH 18/22] Refactor: Extract status command into its own package Let the status command own its flags and execution separately from the code that assembles the root command. --- internal/cli/cli.go | 32 ++--------------- internal/cli/status/status.go | 67 +++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 29 deletions(-) create mode 100644 internal/cli/status/status.go diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 715ef7f..326944f 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -15,6 +15,7 @@ import ( "github.com/nnutter/git-review/internal/cli/commits" "github.com/nnutter/git-review/internal/cli/files" + "github.com/nnutter/git-review/internal/cli/status" "github.com/nnutter/git-review/internal/display" "github.com/nnutter/git-review/internal/review" ) @@ -72,37 +73,10 @@ func (a *application) command() *cobra.Command { } return display.Write(a.output, a.json, result, a.verbose) }} - var onlyNeedsReview, isReviewing bool - status := &cobra.Command{Use: "status", Short: "Show all feature stacks and their review states", Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { - if isReviewing { - reviewing, err := a.service.ReviewRunning(cmd.Context()) - if err != nil { - return err - } - if a.json { - return display.WriteJSON(a.output, struct { - Version int `json:"version"` - Command string `json:"command"` - IsReviewing bool `json:"is_reviewing"` - }{Version: 1, Command: "status", IsReviewing: reviewing}) - } - _, err = fmt.Fprintf(a.output, "Is reviewing: %t\n", reviewing) - return err - } - result, err := a.service.Status(cmd.Context(), onlyNeedsReview) - if err != nil { - return err - } - return display.Write(a.output, a.json, result, a.verbose) - }} - status.Flags().BoolVarP(&a.verbose, "verbose", "v", false, "Show saved review details before the table") - status.Flags().BoolVar(&onlyNeedsReview, "needs-review", false, "Show unresolved stacks and active review attempts") - status.Flags().BoolVar(&isReviewing, "is-reviewing", false, "Check whether a review is running for the current branch") - status.MarkFlagsMutuallyExclusive("needs-review", "is-reviewing") - status.MarkFlagsMutuallyExclusive("verbose", "is-reviewing") - root.AddCommand(feedback, approve, status, a.logCommand()) + root.AddCommand(feedback, approve, a.logCommand()) commits.Install(root, a.service, a.input, a.output, a.terminal) files.Install(root, a.service, a.input, a.output, a.terminal) + status.Install(root, a.service, a.output) return root } diff --git a/internal/cli/status/status.go b/internal/cli/status/status.go new file mode 100644 index 0000000..222f798 --- /dev/null +++ b/internal/cli/status/status.go @@ -0,0 +1,67 @@ +// Package status shows feature stacks and live review activity. +package status + +import ( + "fmt" + "io" + + "github.com/spf13/cobra" + + "github.com/nnutter/git-review/internal/display" + "github.com/nnutter/git-review/internal/review" +) + +// Command houses flag values for the status review. +type Command struct { + Service review.Service + Output io.Writer + NeedsReview bool + IsReviewing bool + Verbose bool +} + +// New builds the status 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} + cmd := &cobra.Command{Use: "status", Short: "Show all feature stacks and their review states", Args: cobra.NoArgs, RunE: c.RunE} + cmd.Flags().BoolVarP(&c.Verbose, "verbose", "v", false, "Show saved review details before the table") + cmd.Flags().BoolVar(&c.NeedsReview, "needs-review", false, "Show unresolved stacks and active review attempts") + cmd.Flags().BoolVar(&c.IsReviewing, "is-reviewing", false, "Check whether a review is running for the current branch") + cmd.MarkFlagsMutuallyExclusive("needs-review", "is-reviewing") + cmd.MarkFlagsMutuallyExclusive("verbose", "is-reviewing") + return cmd +} + +// Install adds the status sub-command to parent. +func Install(parent *cobra.Command, svc review.Service, out io.Writer) { + parent.AddCommand(New(svc, out)) +} + +// RunE runs the status workflow. +func (c *Command) RunE(cmd *cobra.Command, _ []string) error { + asJSON := false + if flag, err := cmd.Root().PersistentFlags().GetBool("json"); err == nil { + asJSON = flag + } + if c.IsReviewing { + reviewing, err := c.Service.ReviewRunning(cmd.Context()) + if err != nil { + return err + } + if asJSON { + return display.WriteJSON(c.Output, struct { + Version int `json:"version"` + Command string `json:"command"` + IsReviewing bool `json:"is_reviewing"` + }{Version: 1, Command: "status", IsReviewing: reviewing}) + } + _, err = fmt.Fprintf(c.Output, "Is reviewing: %t\n", reviewing) + return err + } + result, err := c.Service.Status(cmd.Context(), c.NeedsReview) + if err != nil { + return err + } + return display.Write(c.Output, asJSON, result, c.Verbose) +} From eaf6767d572342c2021a58063a800b4bf505383b Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Sun, 18 Oct 2026 23:53:15 -0500 Subject: [PATCH 19/22] Refactor: Extract feedback command into its own package Give the feedback command ownership of its setup and output so it no longer depends on methods that assemble the root command. --- internal/cli/cli.go | 11 +++----- internal/cli/feedback/feedback.go | 43 +++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 8 deletions(-) create mode 100644 internal/cli/feedback/feedback.go diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 326944f..a25df60 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -14,6 +14,7 @@ import ( "github.com/spf13/cobra" "github.com/nnutter/git-review/internal/cli/commits" + "github.com/nnutter/git-review/internal/cli/feedback" "github.com/nnutter/git-review/internal/cli/files" "github.com/nnutter/git-review/internal/cli/status" "github.com/nnutter/git-review/internal/display" @@ -55,13 +56,6 @@ type application struct { func (a *application) command() *cobra.Command { root := &cobra.Command{Use: "git-review", Short: "Review feature-branch commit stacks with tuicr", Long: "git-review tracks review of exact commit stacks against the configured Git upstream.\nApproval never transfers to rewritten commits or added fixups."} root.PersistentFlags().BoolVar(&a.json, "json", a.json, "Emit a versioned JSON result without interactive prompts") - feedback := &cobra.Command{Use: "feedback", Short: "Read current and historical saved feedback without launching tuicr", Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { - result, err := a.service.Feedback(cmd.Context()) - if err != nil { - return err - } - return display.Write(a.output, a.json, result, a.verbose) - }} approve := &cobra.Command{Use: "approve []", Aliases: []string{"confirm"}, Short: "Record explicit user approval of complete stacks", Long: "Record user approval without launching tuicr. Endpoints are inclusive and\nmust include complete stacks, including all attached feedback commits.", Args: cobra.RangeArgs(1, 2), RunE: func(cmd *cobra.Command, args []string) error { last := "" if len(args) == 2 { @@ -73,8 +67,9 @@ func (a *application) command() *cobra.Command { } return display.Write(a.output, a.json, result, a.verbose) }} - root.AddCommand(feedback, approve, a.logCommand()) + root.AddCommand(approve, a.logCommand()) commits.Install(root, a.service, a.input, a.output, a.terminal) + feedback.Install(root, a.service, a.output) files.Install(root, a.service, a.input, a.output, a.terminal) status.Install(root, a.service, a.output) return root diff --git a/internal/cli/feedback/feedback.go b/internal/cli/feedback/feedback.go new file mode 100644 index 0000000..54599ed --- /dev/null +++ b/internal/cli/feedback/feedback.go @@ -0,0 +1,43 @@ +// Package feedback reads saved feedback without launching tuicr. +package feedback + +import ( + "io" + + "github.com/spf13/cobra" + + "github.com/nnutter/git-review/internal/display" + "github.com/nnutter/git-review/internal/review" +) + +// Command houses no flags for the feedback review. +type Command struct { + Service review.Service + Output io.Writer +} + +// New builds the feedback 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} + cmd := &cobra.Command{Use: "feedback", Short: "Read current and historical saved feedback without launching tuicr", Args: cobra.NoArgs, RunE: c.RunE} + return cmd +} + +// Install adds the feedback sub-command to parent. +func Install(parent *cobra.Command, svc review.Service, out io.Writer) { + parent.AddCommand(New(svc, out)) +} + +// RunE runs the feedback workflow. +func (c *Command) RunE(cmd *cobra.Command, _ []string) error { + asJSON := false + if flag, err := cmd.Root().PersistentFlags().GetBool("json"); err == nil { + asJSON = flag + } + result, err := c.Service.Feedback(cmd.Context()) + if err != nil { + return err + } + return display.Write(c.Output, asJSON, result, false) +} From 3bcd67078eaf2c13b12206ccbc2083640d0e3a85 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Sun, 18 Oct 2026 23:54:45 -0500 Subject: [PATCH 20/22] Refactor: Extract approve command into its own package Give approval setup and execution independent ownership with explicit service and output arguments. Remove the unused application verbosity field now that its final caller moved, so this commit stays lint-clean. --- internal/cli/approve/approve.go | 49 +++++++++++++++++++++++++++++++++ internal/cli/cli.go | 16 ++--------- 2 files changed, 52 insertions(+), 13 deletions(-) create mode 100644 internal/cli/approve/approve.go diff --git a/internal/cli/approve/approve.go b/internal/cli/approve/approve.go new file mode 100644 index 0000000..e0c2678 --- /dev/null +++ b/internal/cli/approve/approve.go @@ -0,0 +1,49 @@ +// Package approve records explicit user approval of complete stacks. +package approve + +import ( + "io" + + "github.com/spf13/cobra" + + "github.com/nnutter/git-review/internal/display" + "github.com/nnutter/git-review/internal/review" +) + +// Command houses positional arguments for the approval review. +type Command struct { + Service review.Service + Output io.Writer + First string + Last string +} + +// New builds the approve 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} + cmd := &cobra.Command{Use: "approve []", Aliases: []string{"confirm"}, Short: "Record explicit user approval of complete stacks", Long: "Record user approval without launching tuicr. Endpoints are inclusive and\nmust include complete stacks, including all attached feedback commits.", Args: cobra.RangeArgs(1, 2), RunE: c.RunE} + return cmd +} + +// Install adds the approve sub-command to parent. +func Install(parent *cobra.Command, svc review.Service, out io.Writer) { + parent.AddCommand(New(svc, out)) +} + +// RunE runs the approval workflow. +func (c *Command) RunE(cmd *cobra.Command, args []string) error { + asJSON := false + if flag, err := cmd.Root().PersistentFlags().GetBool("json"); err == nil { + asJSON = flag + } + c.First = args[0] + if len(args) == 2 { + c.Last = args[1] + } + result, err := c.Service.Approve(cmd.Context(), c.First, c.Last) + if err != nil { + return err + } + return display.Write(c.Output, asJSON, result, false) +} diff --git a/internal/cli/cli.go b/internal/cli/cli.go index a25df60..8d4c30d 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -13,6 +13,7 @@ import ( "github.com/charmbracelet/fang" "github.com/spf13/cobra" + "github.com/nnutter/git-review/internal/cli/approve" "github.com/nnutter/git-review/internal/cli/commits" "github.com/nnutter/git-review/internal/cli/feedback" "github.com/nnutter/git-review/internal/cli/files" @@ -45,7 +46,6 @@ func Execute(ctx context.Context, args []string, dir string, input io.Reader, ou type application struct { json bool - verbose bool service review.Service input io.Reader output io.Writer @@ -56,20 +56,10 @@ type application struct { func (a *application) command() *cobra.Command { root := &cobra.Command{Use: "git-review", Short: "Review feature-branch commit stacks with tuicr", Long: "git-review tracks review of exact commit stacks against the configured Git upstream.\nApproval never transfers to rewritten commits or added fixups."} root.PersistentFlags().BoolVar(&a.json, "json", a.json, "Emit a versioned JSON result without interactive prompts") - approve := &cobra.Command{Use: "approve []", Aliases: []string{"confirm"}, Short: "Record explicit user approval of complete stacks", Long: "Record user approval without launching tuicr. Endpoints are inclusive and\nmust include complete stacks, including all attached feedback commits.", Args: cobra.RangeArgs(1, 2), RunE: func(cmd *cobra.Command, args []string) error { - last := "" - if len(args) == 2 { - last = args[1] - } - result, err := a.service.Approve(cmd.Context(), args[0], last) - if err != nil { - return err - } - return display.Write(a.output, a.json, result, a.verbose) - }} - root.AddCommand(approve, a.logCommand()) + root.AddCommand(a.logCommand()) commits.Install(root, a.service, a.input, a.output, a.terminal) feedback.Install(root, a.service, a.output) + approve.Install(root, a.service, a.output) files.Install(root, a.service, a.input, a.output, a.terminal) status.Install(root, a.service, a.output) return root From eb720d63ed65c261ff4291d3f03214c80782a908 Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Sun, 18 Oct 2026 23:55:45 -0500 Subject: [PATCH 21/22] Refactor: Extract log command into its own package Separate the log command's flags and Git invocation from root command assembly. Keep its patch-selection function in the same package to avoid a later lint repair. --- internal/cli/cli.go | 65 +-------------------------- internal/cli/log/log.go | 99 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 63 deletions(-) create mode 100644 internal/cli/log/log.go diff --git a/internal/cli/cli.go b/internal/cli/cli.go index 8d4c30d..f2a6b05 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -4,10 +4,8 @@ package cli import ( "context" "errors" - "fmt" "io" "os" - "os/exec" "syscall" "github.com/charmbracelet/fang" @@ -17,6 +15,7 @@ import ( "github.com/nnutter/git-review/internal/cli/commits" "github.com/nnutter/git-review/internal/cli/feedback" "github.com/nnutter/git-review/internal/cli/files" + "github.com/nnutter/git-review/internal/cli/log" "github.com/nnutter/git-review/internal/cli/status" "github.com/nnutter/git-review/internal/display" "github.com/nnutter/git-review/internal/review" @@ -56,71 +55,11 @@ type application struct { func (a *application) command() *cobra.Command { root := &cobra.Command{Use: "git-review", Short: "Review feature-branch commit stacks with tuicr", Long: "git-review tracks review of exact commit stacks against the configured Git upstream.\nApproval never transfers to rewritten commits or added fixups."} root.PersistentFlags().BoolVar(&a.json, "json", a.json, "Emit a versioned JSON result without interactive prompts") - root.AddCommand(a.logCommand()) commits.Install(root, a.service, a.input, a.output, a.terminal) feedback.Install(root, a.service, a.output) approve.Install(root, a.service, a.output) files.Install(root, a.service, a.input, a.output, a.terminal) + log.Install(root, a.service, a.input, a.output, a.terminal) status.Install(root, a.service, a.output) return root } - -func (a *application) logCommand() *cobra.Command { - var patch, ignoreAllSpace, stat, difft, hunk bool - cmd := &cobra.Command{ - Use: "log", - Short: "Show commits after the upstream in oldest-first order", - Long: "Run git log @{u}..HEAD --reverse with optional patches, diff statistics, and whitespace filtering.\nUse --difft for Difftastic or --hunk for hunk pager. Both imply --patch.\nOutput uses Git's format and pager, not git-review's review state.", - Args: cobra.NoArgs, - RunE: func(cmd *cobra.Command, _ []string) error { - if a.json { - return fmt.Errorf("log does not support --json") - } - var args []string - if hunk { - args = append(args, "-c", "core.pager=hunk pager") - } - if difft { - args = append(args, "-c", "diff.external=difft") - } - args = append(args, "log", "@{u}..HEAD", "--reverse") - if difft { - args = append(args, "--ext-diff") - } - if wantsPatch(patch, difft, hunk) { - args = append(args, "-p") - } - if ignoreAllSpace { - args = append(args, "-w") - } - if stat { - args = append(args, "--stat") - } - git := exec.CommandContext(cmd.Context(), "git", args...) - git.Dir = a.service.Repository.Dir - git.Stdin = a.input - git.Stdout = a.output - git.Stderr = a.terminal - if err := git.Run(); err != nil { - return fmt.Errorf("git log: %w", err) - } - return nil - }, - } - cmd.Flags().BoolVarP(&patch, "patch", "p", false, "Show the patch for each commit") - cmd.Flags().BoolVarP(&ignoreAllSpace, "ignore-all-space", "w", false, "Ignore whitespace changes in patches") - cmd.Flags().BoolVar(&stat, "stat", false, "Show diff statistics for each commit") - cmd.Flags().BoolVar(&difft, "difft", false, "Show patches with difft (requires difft on PATH)") - cmd.Flags().BoolVar(&hunk, "hunk", false, "Show patches with hunk pager (requires hunk on PATH)") - return cmd -} - -func wantsPatch(patch, difft, hunk bool) bool { - if patch { - return true - } - if difft { - return true - } - return hunk -} diff --git a/internal/cli/log/log.go b/internal/cli/log/log.go new file mode 100644 index 0000000..e9204f8 --- /dev/null +++ b/internal/cli/log/log.go @@ -0,0 +1,99 @@ +// Package log shows commits after the upstream in oldest-first order. +package log + +import ( + "fmt" + "io" + "os/exec" + + "github.com/spf13/cobra" + + "github.com/nnutter/git-review/internal/review" +) + +// Command houses flag values for the log review. +type Command struct { + Service review.Service + Input io.Reader + Output io.Writer + Terminal io.Writer + Patch bool + IgnoreAllSpace bool + Stat bool + Difft bool + Hunk bool +} + +// New builds the log sub-command. Callers install it onto the parent +// 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: "log", + Short: "Show commits after the upstream in oldest-first order", + Long: "Run git log @{u}..HEAD --reverse with optional patches, diff statistics, and whitespace filtering.\nUse --difft for Difftastic or --hunk for hunk pager. Both imply --patch.\nOutput uses Git's format and pager, not git-review's review state.", + Args: cobra.NoArgs, + RunE: c.RunE, + } + cmd.Flags().BoolVarP(&c.Patch, "patch", "p", false, "Show the patch for each commit") + cmd.Flags().BoolVarP(&c.IgnoreAllSpace, "ignore-all-space", "w", false, "Ignore whitespace changes in patches") + cmd.Flags().BoolVar(&c.Stat, "stat", false, "Show diff statistics for each commit") + cmd.Flags().BoolVar(&c.Difft, "difft", false, "Show patches with difft (requires difft on PATH)") + cmd.Flags().BoolVar(&c.Hunk, "hunk", false, "Show patches with hunk pager (requires hunk on PATH)") + return cmd +} + +// Install adds the log sub-command to parent. +func Install(parent *cobra.Command, svc review.Service, in io.Reader, out, term io.Writer) { + parent.AddCommand(New(svc, in, out, term)) +} + +// RunE runs the log workflow. +func (c *Command) RunE(cmd *cobra.Command, _ []string) error { + asJSON := false + if flag, err := cmd.Root().PersistentFlags().GetBool("json"); err == nil { + asJSON = flag + } + if asJSON { + return fmt.Errorf("log does not support --json") + } + var args []string + if c.Hunk { + args = append(args, "-c", "core.pager=hunk pager") + } + if c.Difft { + args = append(args, "-c", "diff.external=difft") + } + args = append(args, "log", "@{u}..HEAD", "--reverse") + if c.Difft { + args = append(args, "--ext-diff") + } + if wantsPatch(c.Patch, c.Difft, c.Hunk) { + args = append(args, "-p") + } + if c.IgnoreAllSpace { + args = append(args, "-w") + } + if c.Stat { + args = append(args, "--stat") + } + git := exec.CommandContext(cmd.Context(), "git", args...) + git.Dir = c.Service.Repository.Dir + git.Stdin = c.Input + git.Stdout = c.Output + git.Stderr = c.Terminal + if err := git.Run(); err != nil { + return fmt.Errorf("git log: %w", err) + } + return nil +} + +func wantsPatch(patch, difft, hunk bool) bool { + if patch { + return true + } + if difft { + return true + } + return hunk +} From 1ccfc939888464aeadaa44b7a60c6e6ae6c29edc Mon Sep 17 00:00:00 2001 From: Nathan Nutter Date: Sun, 18 Oct 2026 23:56:27 -0500 Subject: [PATCH 22/22] Refactor: Extract root command assembly into cli/root package Separate command construction from execution so callers can assemble the command tree without running it. Keep `cli.Execute` as the entry point for existing callers. --- internal/cli/cli.go | 56 ++------------------------- internal/cli/root/root.go | 79 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 53 deletions(-) create mode 100644 internal/cli/root/root.go diff --git a/internal/cli/cli.go b/internal/cli/cli.go index f2a6b05..266b7e3 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -3,63 +3,13 @@ package cli import ( "context" - "errors" "io" - "os" - "syscall" - "github.com/charmbracelet/fang" - "github.com/spf13/cobra" - - "github.com/nnutter/git-review/internal/cli/approve" - "github.com/nnutter/git-review/internal/cli/commits" - "github.com/nnutter/git-review/internal/cli/feedback" - "github.com/nnutter/git-review/internal/cli/files" - "github.com/nnutter/git-review/internal/cli/log" - "github.com/nnutter/git-review/internal/cli/status" - "github.com/nnutter/git-review/internal/display" - "github.com/nnutter/git-review/internal/review" + "github.com/nnutter/git-review/internal/cli/root" ) // Execute runs a command with explicit process streams and checkout location. +// It delegates to the assembled root tree so existing callers keep working. func Execute(ctx context.Context, args []string, dir string, input io.Reader, output, terminal io.Writer, version string) error { - app := application{json: display.WantsJSON(args), service: review.Service{Repository: review.Repository{Dir: dir}}, input: input, output: output, terminal: terminal} - root := app.command() - root.SetArgs(args) - root.SetIn(input) - root.SetOut(output) - root.SetErr(terminal) - err := fang.Execute(ctx, root, fang.WithVersion(version), fang.WithoutManpage(), fang.WithNotifySignal(os.Interrupt, syscall.SIGTERM), fang.WithErrorHandler(func(w io.Writer, styles fang.Styles, err error) { - if app.json { - failed, _, _ := root.Find(args) - if failed == nil { - failed = root - } - app.outputError = display.WriteJSON(output, display.ErrorResult{Version: 1, Command: failed.Name(), Status: "error", Error: err.Error()}) - return - } - fang.DefaultErrorHandler(w, styles, err) - })) - return errors.Join(err, app.outputError) -} - -type application struct { - json bool - service review.Service - input io.Reader - output io.Writer - terminal io.Writer - outputError error -} - -func (a *application) command() *cobra.Command { - root := &cobra.Command{Use: "git-review", Short: "Review feature-branch commit stacks with tuicr", Long: "git-review tracks review of exact commit stacks against the configured Git upstream.\nApproval never transfers to rewritten commits or added fixups."} - root.PersistentFlags().BoolVar(&a.json, "json", a.json, "Emit a versioned JSON result without interactive prompts") - commits.Install(root, a.service, a.input, a.output, a.terminal) - feedback.Install(root, a.service, a.output) - approve.Install(root, a.service, a.output) - files.Install(root, a.service, a.input, a.output, a.terminal) - log.Install(root, a.service, a.input, a.output, a.terminal) - status.Install(root, a.service, a.output) - return root + return root.Execute(ctx, args, dir, input, output, terminal, version) } diff --git a/internal/cli/root/root.go b/internal/cli/root/root.go new file mode 100644 index 0000000..9b5eed4 --- /dev/null +++ b/internal/cli/root/root.go @@ -0,0 +1,79 @@ +// Package root assembles the git-review command tree. +package root + +import ( + "context" + "errors" + "io" + "os" + "syscall" + + "github.com/charmbracelet/fang" + "github.com/spf13/cobra" + + "github.com/nnutter/git-review/internal/cli/approve" + "github.com/nnutter/git-review/internal/cli/commits" + "github.com/nnutter/git-review/internal/cli/feedback" + "github.com/nnutter/git-review/internal/cli/files" + "github.com/nnutter/git-review/internal/cli/log" + "github.com/nnutter/git-review/internal/cli/status" + "github.com/nnutter/git-review/internal/display" + "github.com/nnutter/git-review/internal/review" +) + +// Command houses setup state for the root command. +type Command struct { + Service review.Service + Input io.Reader + Output io.Writer + Terminal io.Writer + JSON bool + outputError error +} + +// New builds the root command and installs every sub-command onto it. +func New(svc review.Service, in io.Reader, out, term io.Writer, jsonDef bool) *cobra.Command { + c := &Command{Service: svc, Input: in, Output: out, Terminal: term, JSON: jsonDef} + return c.build() +} + +// Install adds the assembled tree to parent. It exists so every command +// package shares the same constructor shape; the binary uses Execute. +func Install(parent *cobra.Command, svc review.Service, in io.Reader, out, term io.Writer, jsonDef bool) { + parent.AddCommand(New(svc, in, out, term, jsonDef)) +} + +// Execute runs a command with explicit process streams and checkout location. +func Execute(ctx context.Context, args []string, dir string, input io.Reader, output, terminal io.Writer, version string) error { + svc := review.Service{Repository: review.Repository{Dir: dir}} + c := &Command{Service: svc, Input: input, Output: output, Terminal: terminal, JSON: display.WantsJSON(args)} + root := c.build() + root.SetArgs(args) + root.SetIn(input) + root.SetOut(output) + root.SetErr(terminal) + err := fang.Execute(ctx, root, fang.WithVersion(version), fang.WithoutManpage(), fang.WithNotifySignal(os.Interrupt, syscall.SIGTERM), fang.WithErrorHandler(func(w io.Writer, styles fang.Styles, err error) { + if c.JSON { + failed, _, _ := root.Find(args) + if failed == nil { + failed = root + } + c.outputError = display.WriteJSON(output, display.ErrorResult{Version: 1, Command: failed.Name(), Status: "error", Error: err.Error()}) + return + } + fang.DefaultErrorHandler(w, styles, err) + })) + return errors.Join(err, c.outputError) +} + +func (c *Command) build() *cobra.Command { + root := &cobra.Command{Use: "git-review", Short: "Review feature-branch commit stacks with tuicr", Long: "git-review tracks review of exact commit stacks against the configured Git upstream.\nApproval never transfers to rewritten commits or added fixups."} + root.PersistentFlags().BoolVar(&c.JSON, "json", c.JSON, "Emit a versioned JSON result without interactive prompts") + approve.Install(root, c.Service, c.Output) + commits.Install(root, c.Service, c.Input, c.Output, c.Terminal) + 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) + status.Install(root, c.Service, c.Output) + return root +}