diff --git a/README.md b/README.md index 26b0caf..2ec83ec 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 @@ -195,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. @@ -233,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. @@ -264,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. @@ -277,7 +281,44 @@ 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`. + +### 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 @@ -301,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/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/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 0b62471..266b7e3 100644 --- a/internal/cli/cli.go +++ b/internal/cli/cli.go @@ -3,212 +3,13 @@ package cli import ( "context" - "encoding/json/v2" - "errors" - "fmt" "io" - "os" - "strings" - "syscall" - "charm.land/huh/v2" - "github.com/charmbracelet/fang" - "github.com/charmbracelet/x/term" - "github.com/spf13/cobra" - - "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: 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 = writeJSON(output, 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 - verbose 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") - 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 a.write(result) - }} - 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 a.write(result) - }} - var onlyNeedsReview 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 { - result, err := a.service.Status(cmd.Context(), onlyNeedsReview) - if err != nil { - return err - } - return a.write(result) - }} - 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()) - return root -} - -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 { - 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) 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 a.write(result) -} - -func (a *application) write(result review.Result) error { - if a.json { - return writeJSON(a.output, result) - } - return render(a.output, result, a.verbose) -} - -type errorResult struct { - Version int `json:"version"` - Command string `json:"command"` - Status string `json:"status"` - 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 -} - -// 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 + return root.Execute(ctx, args, dir, input, output, terminal, version) } diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 9095b56..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" ) @@ -98,7 +99,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"}, } { @@ -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) @@ -124,7 +125,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 +151,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 +159,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 +184,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 +210,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,9 +226,9 @@ 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 + var result display.ErrorResult require.NoError(t, json.Unmarshal([]byte(out), &result)) require.Equal(t, "error", result.Status) } @@ -236,16 +237,19 @@ 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, "running") + 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") - 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/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/commits/prompt.go b/internal/cli/commits/prompt.go new file mode 100644 index 0000000..c857442 --- /dev/null +++ b/internal/cli/commits/prompt.go @@ -0,0 +1,40 @@ +package commits + +import ( + "context" + "fmt" + "io" + "os" + + "charm.land/huh/v2" + "github.com/charmbracelet/x/term" + + "github.com/nnutter/git-review/internal/display" + "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(display.RecoveryDescription(decision)).Options( + huh.NewOption("Recover — reopen tuicr with saved progress and comments", "recover"), + huh.NewOption("Replace — create a new attempt (keep prior history)", "replace"), + huh.NewOption("Cancel — keep the saved review unchanged", "cancel"), + ).Value(&choice) + err := huh.NewForm(huh.NewGroup(selectField)).WithInput(input).WithOutput(output).RunWithContext(ctx) + return choice, err +} diff --git a/internal/cli/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) +} 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/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) +} 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/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/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 +} diff --git a/internal/cli/recovery_test.go b/internal/cli/recovery_test.go index 4dac43d..8d492cd 100644 --- a/internal/cli/recovery_test.go +++ b/internal/cli/recovery_test.go @@ -4,8 +4,10 @@ import ( "encoding/json/jsontext" "testing" - "github.com/nnutter/git-review/internal/review" "github.com/stretchr/testify/require" + + "github.com/nnutter/git-review/internal/display" + "github.com/nnutter/git-review/internal/review" ) func TestRecoveryDecisionExplainsSavedWork(t *testing.T) { @@ -23,7 +25,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, @@ -34,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") @@ -45,7 +47,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/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 +} 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) +} diff --git a/internal/cli/status_test.go b/internal/cli/status_test.go index 4f1a621..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 @@ -100,6 +101,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 +135,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) { @@ -175,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) @@ -197,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) @@ -206,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/cli/tuicr_functional_test.go b/internal/cli/tuicr_functional_test.go index e944e1c..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 { @@ -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") } @@ -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") @@ -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.") } @@ -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/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 56% rename from internal/cli/render.go rename to internal/display/render.go index ec2f333..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,35 +31,18 @@ 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 result.Command == "status" && verbose { + if showStatusDetails(result, verbose) { if err := renderDetails(&out, result.Entries); err != nil { return "", err } } 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)) + 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 { @@ -70,32 +53,48 @@ 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, "Choose git-review run --recover, git-review run --replace, or leave the attempt unchanged.") + 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 } -func recoveryDescription(decision *review.Decision) string { +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 - 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)) - var saved struct { - Reviewed *int `json:"reviewed"` - Files *int `json:"files"` - Comments []any `json:"comments"` - SessionNotes *string `json:"sessionNotes"` - } - if len(attempt.Review) == 0 || json.Unmarshal(attempt.Review, &saved) != nil || saved.Reviewed == nil || saved.Files == nil { + fmt.Fprintf(&out, "Started: %s · Status: %s\n", started, StatusLabel(attempt.Status)) + var saved savedProgress + if !savedProgressAvailable(attempt.Review, &saved) { fmt.Fprintln(&out, "Saved progress: unavailable") } else { fmt.Fprintf(&out, "Saved progress: %d/%d files reviewed · Comments: %d", *saved.Reviewed, *saved.Files, len(saved.Comments)) - if saved.SessionNotes != nil && *saved.SessionNotes != "" { + if hasSessionNotes(saved.SessionNotes) { fmt.Fprint(&out, " · session notes saved") } fmt.Fprintln(&out) @@ -104,16 +103,57 @@ 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 { - fmt.Fprintf(&out, "New review commits (inclusive): %s — %s\n", shortStack(*current), safeText(current.Subject)) + if scopeChanged(*current, attempt.Stack) { + fmt.Fprintf(&out, "New review commits (inclusive): %s — %s\n", ShortStack(*current), safeText(current.Subject)) } } else { - fmt.Fprintln(&out, "No current replacement scope. Select one with 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() } +type savedProgress struct { + Reviewed *int `json:"reviewed"` + Files *int `json:"files"` + Comments []any `json:"comments"` + SessionNotes *string `json:"sessionNotes"` +} + +func savedProgressAvailable(data []byte, saved *savedProgress) bool { + if len(data) == 0 { + return false + } + if err := json.Unmarshal(data, saved); err != nil { + return false + } + if saved.Reviewed == nil { + return false + } + return saved.Files != nil +} + +func showStatusDetails(result review.Result, verbose bool) bool { + if !verbose { + return false + } + 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 { @@ -136,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 @@ -155,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)) } @@ -232,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] @@ -247,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" @@ -263,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/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) +} 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/git.go b/internal/review/git.go index 1cb443d..5f01218 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"` } @@ -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 !detachedHEADExit(err) { + 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 == "" { @@ -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 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) } }) } 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) { 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/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/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 { 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")) +} diff --git a/pi/extensions/git-review/index.ts b/pi/extensions/git-review/index.ts index 792e984..e251519 100644 --- a/pi/extensions/git-review/index.ts +++ b/pi/extensions/git-review/index.ts @@ -2,14 +2,17 @@ * 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)`. * - * 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`. @@ -23,6 +26,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 +90,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..fdbeccb 100644 --- a/pi/extensions/git-review/support.ts +++ b/pi/extensions/git-review/support.ts @@ -2,7 +2,10 @@ * 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. + * `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. */ @@ -79,6 +82,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..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: @@ -83,8 +96,11 @@ 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. +Do not use `git-review commits --recover` to retrieve feedback. Recovery opens an interactive review and requires the user's choice. ## Prepare the changes @@ -182,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. @@ -197,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 run`. +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.