From ab3f0e3de3ad3c58952533b0ca9b7e099cc99165 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 19 Mar 2026 17:07:11 +0000 Subject: [PATCH 1/3] Initial plan From ce4da1be6720c75c61c2fc562e26273e8ac42e1e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 19 Mar 2026 17:16:47 +0000 Subject: [PATCH 2/3] feat: add git progress feedback and review commencement message to stderr Co-authored-by: richardthombs <1511599+richardthombs@users.noreply.github.com> --- cmd/prr/checkout.go | 2 +- cmd/prr/checkout_helpers.go | 6 ++++-- cmd/prr/checkout_test.go | 5 +++-- cmd/prr/diff.go | 2 +- cmd/prr/diff_bundle_test.go | 5 +++-- cmd/prr/pwd.go | 2 +- cmd/prr/review.go | 3 ++- cmd/prr/review_render_test.go | 23 ++++++++++++----------- internal/git/git.go | 25 +++++++++++++++++++++---- 9 files changed, 48 insertions(+), 25 deletions(-) diff --git a/cmd/prr/checkout.go b/cmd/prr/checkout.go index 64d5e1b..3867eef 100644 --- a/cmd/prr/checkout.go +++ b/cmd/prr/checkout.go @@ -72,7 +72,7 @@ var checkoutCmd = &cobra.Command{ } prRef = provider.EnrichPRRef(context.Background(), prRef, prEnricherFactory(), warnf) - service := mirrorServiceFactory() + service := mirrorServiceFactory(cmd.ErrOrStderr()) commonOpts := git.EnsureOptions{ Verbose: verbose || whatIf, WhatIf: whatIf, diff --git a/cmd/prr/checkout_helpers.go b/cmd/prr/checkout_helpers.go index fbc75e2..02e7b25 100644 --- a/cmd/prr/checkout_helpers.go +++ b/cmd/prr/checkout_helpers.go @@ -1,14 +1,16 @@ package main import ( + "io" + apperrors "github.com/richardthombs/prr/internal/errors" "github.com/richardthombs/prr/internal/git" "github.com/richardthombs/prr/internal/provider" "github.com/spf13/cobra" ) -var mirrorServiceFactory = func() *git.Service { - return git.NewService(git.NewExecRunner()) +var mirrorServiceFactory = func(stderr io.Writer) *git.Service { + return git.NewService(git.NewExecRunnerWithStderr(stderr)) } var prEnricherFactory = func() provider.CLIRunner { diff --git a/cmd/prr/checkout_test.go b/cmd/prr/checkout_test.go index f23c921..12ecfcc 100644 --- a/cmd/prr/checkout_test.go +++ b/cmd/prr/checkout_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "io" "strings" "testing" @@ -59,7 +60,7 @@ func TestCheckoutEmitsPipelineEquivalentPayload(t *testing.T) { return "", nil }}, t.TempDir()) - mirrorServiceFactory = func() *git.Service { + mirrorServiceFactory = func(_ io.Writer) *git.Service { return service } @@ -118,7 +119,7 @@ func TestCheckoutWhatIfSkipsExternalExecution(t *testing.T) { }} service := git.NewServiceWithCacheDir(runner, t.TempDir()) - mirrorServiceFactory = func() *git.Service { + mirrorServiceFactory = func(_ io.Writer) *git.Service { return service } diff --git a/cmd/prr/diff.go b/cmd/prr/diff.go index 1f4cb40..36e7234 100644 --- a/cmd/prr/diff.go +++ b/cmd/prr/diff.go @@ -50,7 +50,7 @@ var diffCmd = &cobra.Command{ return apperrors.WrapConfig("worktree directory is required; provide --work-dir or stdin JSON with workDir", nil) } - service := mirrorServiceFactory() + service := mirrorServiceFactory(cmd.ErrOrStderr()) diffOutput, err := service.DiffContributionWithOptions(context.Background(), resolvedWorkDir, strings.TrimSpace(input.BaseRef), git.EnsureOptions{ Verbose: verbose || whatIf, WhatIf: whatIf, diff --git a/cmd/prr/diff_bundle_test.go b/cmd/prr/diff_bundle_test.go index 2c86ccb..ef79918 100644 --- a/cmd/prr/diff_bundle_test.go +++ b/cmd/prr/diff_bundle_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "io" "strings" "testing" @@ -36,7 +37,7 @@ func TestDiffCommandEmitsJSONFromStdinWorkDir(t *testing.T) { } }}, t.TempDir()) - mirrorServiceFactory = func() *git.Service { return service } + mirrorServiceFactory = func(_ io.Writer) *git.Service { return service } stdin := bytes.NewBufferString(`{"prId":12,"repoUrl":"https://github.com/acme/repo","remote":"origin","provider":"github","bareDir":"/tmp/bare","mergeRef":"refs/prr/pull/12/merge","workDir":"/tmp/work/12"}`) stdout := &bytes.Buffer{} @@ -78,7 +79,7 @@ func TestDiffCommandWhatIfLogsAndSkipsExecution(t *testing.T) { return "", nil }} service := git.NewServiceWithCacheDir(runner, t.TempDir()) - mirrorServiceFactory = func() *git.Service { return service } + mirrorServiceFactory = func(_ io.Writer) *git.Service { return service } stdin := bytes.NewBufferString(`{"workDir":"/tmp/work/99"}`) stdout := &bytes.Buffer{} diff --git a/cmd/prr/pwd.go b/cmd/prr/pwd.go index 297cf7d..a4ed120 100644 --- a/cmd/prr/pwd.go +++ b/cmd/prr/pwd.go @@ -44,7 +44,7 @@ var pwdCmd = &cobra.Command{ return err } - service := mirrorServiceFactory() + service := mirrorServiceFactory(cmd.ErrOrStderr()) bareDir, err := service.ResolveMirrorDir(prRef.RepoURL) if err != nil { diff --git a/cmd/prr/review.go b/cmd/prr/review.go index a5f074a..3dab3f9 100644 --- a/cmd/prr/review.go +++ b/cmd/prr/review.go @@ -118,7 +118,7 @@ var reviewCmd = &cobra.Command{ prRef = provider.EnrichPRRef(context.Background(), prRef, prEnricherFactory(), warnf) } - service := mirrorServiceFactory() + service := mirrorServiceFactory(cmd.ErrOrStderr()) commonOpts := git.EnsureOptions{ Verbose: verbose || whatIf, WhatIf: whatIf, @@ -221,6 +221,7 @@ var reviewCmd = &cobra.Command{ return err } + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "[prr] starting PR review...\n") reviewOutput, err := reviewEngineFactory().Review(context.Background(), engine.ReviewInput{ Bundle: bundlePayload, WorkDir: workDir, diff --git a/cmd/prr/review_render_test.go b/cmd/prr/review_render_test.go index 84c86e0..1110212 100644 --- a/cmd/prr/review_render_test.go +++ b/cmd/prr/review_render_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "errors" + "io" "strings" "testing" @@ -44,7 +45,7 @@ func TestReviewCommandEmitsStructuredJSONAndKeepsDiagnosticsOffStdout(t *testing } }}, t.TempDir()) - mirrorServiceFactory = func() *git.Service { return service } + mirrorServiceFactory = func(_ io.Writer) *git.Service { return service } issueRunnerFactory = func() provider.CLIRunner { return stubRunner{runFunc: func(_ context.Context, name string, args ...string) (string, error) { if name != "gh" { @@ -79,8 +80,8 @@ func TestReviewCommandEmitsStructuredJSONAndKeepsDiagnosticsOffStdout(t *testing t.Fatalf("expected review command to succeed, got %v", err) } - if strings.TrimSpace(stderr.String()) != "" { - t.Fatalf("expected no diagnostics on stderr without verbose/what-if, got %q", stderr.String()) + if !strings.Contains(stderr.String(), "[prr] starting PR review...") { + t.Fatalf("expected review commencement message on stderr, got %q", stderr.String()) } var payload map[string]any @@ -133,7 +134,7 @@ func TestReviewCommandWhatIfVerbosePrintsCommandsToStderr(t *testing.T) { return "", nil }} service := git.NewServiceWithCacheDir(runner, t.TempDir()) - mirrorServiceFactory = func() *git.Service { return service } + mirrorServiceFactory = func(_ io.Writer) *git.Service { return service } issueRunnerFactory = func() provider.CLIRunner { return stubRunner{runFunc: func(_ context.Context, _ string, _ ...string) (string, error) { return "[]", nil }} } @@ -188,7 +189,7 @@ func TestReviewCommandAcceptsPRURLArgument(t *testing.T) { return "", nil } }}, t.TempDir()) - mirrorServiceFactory = func() *git.Service { return service } + mirrorServiceFactory = func(_ io.Writer) *git.Service { return service } issueRunnerFactory = func() provider.CLIRunner { return stubRunner{runFunc: func(_ context.Context, _ string, _ ...string) (string, error) { return "[]", nil }} } @@ -245,7 +246,7 @@ func TestReviewCommandAcceptsPipedCheckoutJSONWithoutArgs(t *testing.T) { return "", nil } }}, t.TempDir()) - mirrorServiceFactory = func() *git.Service { return service } + mirrorServiceFactory = func(_ io.Writer) *git.Service { return service } issueRunnerFactory = func() provider.CLIRunner { return stubRunner{runFunc: func(_ context.Context, _ string, _ ...string) (string, error) { return "[]", nil }} } @@ -309,7 +310,7 @@ func TestReviewCommandBypassesSetupWithAuthoritativeCheckoutJSON(t *testing.T) { }} service := git.NewServiceWithCacheDir(runner, t.TempDir()) - mirrorServiceFactory = func() *git.Service { return service } + mirrorServiceFactory = func(_ io.Writer) *git.Service { return service } issueRunnerFactory = func() provider.CLIRunner { return stubRunner{runFunc: func(_ context.Context, _ string, _ ...string) (string, error) { return "[]", nil }} } @@ -367,7 +368,7 @@ func TestReviewCommandPassesModelFlagToEngine(t *testing.T) { return "", nil } }}, t.TempDir()) - mirrorServiceFactory = func() *git.Service { return service } + mirrorServiceFactory = func(_ io.Writer) *git.Service { return service } issueRunnerFactory = func() provider.CLIRunner { return stubRunner{runFunc: func(_ context.Context, _ string, _ ...string) (string, error) { return "[]", nil }} } @@ -419,7 +420,7 @@ func TestReviewCommandClassifiesEngineFailures(t *testing.T) { return "", nil } }}, t.TempDir()) - mirrorServiceFactory = func() *git.Service { return service } + mirrorServiceFactory = func(_ io.Writer) *git.Service { return service } issueRunnerFactory = func() provider.CLIRunner { return stubRunner{runFunc: func(_ context.Context, _ string, _ ...string) (string, error) { return "[]", nil }} } @@ -460,7 +461,7 @@ func TestReviewCommandEmitsDeterministicJSONShape(t *testing.T) { t.Fatalf("expected no external command execution in what-if mode") return "", nil }} - mirrorServiceFactory = func() *git.Service { return git.NewServiceWithCacheDir(runner, t.TempDir()) } + mirrorServiceFactory = func(_ io.Writer) *git.Service { return git.NewServiceWithCacheDir(runner, t.TempDir()) } issueRunnerFactory = func() provider.CLIRunner { return stubRunner{runFunc: func(_ context.Context, _ string, _ ...string) (string, error) { return "[]", nil }} } @@ -528,7 +529,7 @@ func TestReviewCommandEmitsDeterministicMarkdown(t *testing.T) { return "", nil } }}, t.TempDir()) - mirrorServiceFactory = func() *git.Service { return service } + mirrorServiceFactory = func(_ io.Writer) *git.Service { return service } issueRunnerFactory = func() provider.CLIRunner { return stubRunner{runFunc: func(_ context.Context, _ string, _ ...string) (string, error) { return `{"data":{"repository":{"pullRequest":{"closingIssuesReferences":{"nodes":[{"number":13,"url":"https://github.com/acme/repo/issues/13","title":"Issue title","body":"Issue body","state":"OPEN","labels":{"nodes":[{"name":"bug"}]}}]}}}}}`, nil diff --git a/internal/git/git.go b/internal/git/git.go index 827fb31..c1e37a7 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "fmt" + "io" "os/exec" "strings" ) @@ -12,24 +13,40 @@ type Runner interface { Run(ctx context.Context, name string, args ...string) (string, error) } -type ExecRunner struct{} +// ExecRunner executes real git commands. When Stderr is non-nil, git's stderr +// output (e.g. clone/fetch progress) is forwarded to that writer in addition +// to being captured for inclusion in any error messages. +type ExecRunner struct { + Stderr io.Writer +} func NewExecRunner() Runner { return ExecRunner{} } +// NewExecRunnerWithStderr returns an ExecRunner that forwards git stderr output +// to the provided writer. This allows callers to surface clone and fetch progress +// to the user without interfering with stdout. +func NewExecRunnerWithStderr(stderr io.Writer) Runner { + return ExecRunner{Stderr: stderr} +} + func (r ExecRunner) Run(ctx context.Context, name string, args ...string) (string, error) { cmd := exec.CommandContext(ctx, name, args...) var stdout bytes.Buffer - var stderr bytes.Buffer + var stderrBuf bytes.Buffer cmd.Stdout = &stdout - cmd.Stderr = &stderr + if r.Stderr != nil { + cmd.Stderr = io.MultiWriter(&stderrBuf, r.Stderr) + } else { + cmd.Stderr = &stderrBuf + } err := cmd.Run() if err != nil { - combined := strings.TrimSpace(strings.Join([]string{stderr.String(), stdout.String()}, "\n")) + combined := strings.TrimSpace(strings.Join([]string{stderrBuf.String(), stdout.String()}, "\n")) trimmed := strings.TrimSpace(combined) if trimmed == "" { return "", err From 642c199d674802f0ebbfe86815faf915c50a68c9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 19 Mar 2026 20:04:08 +0000 Subject: [PATCH 3/3] fix: add --progress flag to git clone/fetch to force progress output on non-TTY stderr Co-authored-by: richardthombs <1511599+richardthombs@users.noreply.github.com> --- cmd/prr/checkout_test.go | 4 ++-- internal/git/mirror.go | 12 ++++++------ internal/git/mirror_test.go | 8 ++++---- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/cmd/prr/checkout_test.go b/cmd/prr/checkout_test.go index 12ecfcc..06dba0b 100644 --- a/cmd/prr/checkout_test.go +++ b/cmd/prr/checkout_test.go @@ -52,7 +52,7 @@ func TestCheckoutEmitsPipelineEquivalentPayload(t *testing.T) { joined := strings.Join(args, " ") if !strings.Contains(joined, "clone --mirror") && - !strings.Contains(joined, "fetch origin pull/987654321/merge:refs/prr/pull/987654321/merge") && + !strings.Contains(joined, "fetch --progress origin pull/987654321/merge:refs/prr/pull/987654321/merge") && !strings.Contains(joined, "worktree add --detach") { t.Fatalf("unexpected git invocation %q", joined) } @@ -138,7 +138,7 @@ func TestCheckoutWhatIfSkipsExternalExecution(t *testing.T) { if !strings.Contains(stderrText, "exec: git clone --mirror") { t.Fatalf("expected mirror command preview in stderr, got %q", stderrText) } - if !strings.Contains(stderrText, "exec: git -C") || !strings.Contains(stderrText, "fetch origin pull/123456789/merge:refs/prr/pull/123456789/merge") { + if !strings.Contains(stderrText, "exec: git -C") || !strings.Contains(stderrText, "fetch --progress origin pull/123456789/merge:refs/prr/pull/123456789/merge") { t.Fatalf("expected fetch command preview in stderr, got %q", stderrText) } if !strings.Contains(stderrText, "worktree add --detach") && !strings.Contains(stderrText, "reset --hard") { diff --git a/internal/git/mirror.go b/internal/git/mirror.go index 41f4be4..b61e87d 100644 --- a/internal/git/mirror.go +++ b/internal/git/mirror.go @@ -54,7 +54,7 @@ func (s *Service) EnsureMirrorWithOptions(ctx context.Context, repoURL string, o if opts.WhatIf { if _, statErr := os.Stat(bareDir); statErr == nil { - _, runErr := s.runCommand(ctx, opts, "git", "-C", bareDir, "remote", "update", "--prune") + _, runErr := s.runCommand(ctx, opts, "git", "-C", bareDir, "fetch", "--all", "--prune", "--progress") if runErr != nil { return "", runErr } @@ -64,7 +64,7 @@ func (s *Service) EnsureMirrorWithOptions(ctx context.Context, repoURL string, o return "", apperrors.WrapRuntime("failed to inspect bare mirror path", statErr) } - _, runErr := s.runCommand(ctx, opts, "git", "clone", "--mirror", strings.TrimSpace(repoURL), bareDir) + _, runErr := s.runCommand(ctx, opts, "git", "clone", "--mirror", "--progress", strings.TrimSpace(repoURL), bareDir) if runErr != nil { return "", runErr } @@ -78,7 +78,7 @@ func (s *Service) EnsureMirrorWithOptions(ctx context.Context, repoURL string, o err = s.withRepoLock(bareDir, opts, func() error { if _, statErr := os.Stat(bareDir); statErr == nil { - _, runErr := s.runCommand(ctx, opts, "git", "-C", bareDir, "remote", "update", "--prune") + _, runErr := s.runCommand(ctx, opts, "git", "-C", bareDir, "fetch", "--all", "--prune", "--progress") if runErr != nil { return apperrors.WrapRuntime("failed to update bare mirror", runErr) } @@ -88,7 +88,7 @@ func (s *Service) EnsureMirrorWithOptions(ctx context.Context, repoURL string, o return apperrors.WrapRuntime("failed to inspect bare mirror path", statErr) } - _, runErr := s.runCommand(ctx, opts, "git", "clone", "--mirror", strings.TrimSpace(repoURL), bareDir) + _, runErr := s.runCommand(ctx, opts, "git", "clone", "--mirror", "--progress", strings.TrimSpace(repoURL), bareDir) if runErr != nil { return apperrors.WrapRuntime("failed to create bare mirror", runErr) } @@ -137,7 +137,7 @@ func (s *Service) FetchPRMergeRefWithOptions(ctx context.Context, bareDir, remot sourceRef := "pull/" + strconv.Itoa(prID) + "/merge" destination := sourceRef + ":" + mergeRef - _, err := s.runCommand(ctx, opts, "git", "-C", trimmedBareDir, "fetch", trimmedRemote, destination) + _, err := s.runCommand(ctx, opts, "git", "-C", trimmedBareDir, "fetch", "--progress", trimmedRemote, destination) if err != nil { return "", apperrors.WrapProvider("failed to fetch PR merge ref", err) } @@ -295,7 +295,7 @@ func (s *Service) FetchPRHeadRef(ctx context.Context, bareDir, remote string, pr sourceRef := "pull/" + strconv.Itoa(prID) + "/head" destination := sourceRef + ":" + headRef - _, err := s.runCommand(ctx, opts, "git", "-C", trimmedBareDir, "fetch", trimmedRemote, destination) + _, err := s.runCommand(ctx, opts, "git", "-C", trimmedBareDir, "fetch", "--progress", trimmedRemote, destination) if err != nil { return "", apperrors.WrapProvider("failed to fetch PR head ref", err) } diff --git a/internal/git/mirror_test.go b/internal/git/mirror_test.go index 90e4218..0551996 100644 --- a/internal/git/mirror_test.go +++ b/internal/git/mirror_test.go @@ -161,8 +161,8 @@ func TestEnsureMirrorUsesUpdateForExistingMirror(t *testing.T) { } command := strings.Join(runner.commands[0], " ") - if !strings.Contains(command, "git -C "+bareDir+" remote update --prune") { - t.Fatalf("expected remote update command, got %q", command) + if !strings.Contains(command, "git -C "+bareDir+" fetch --all --prune --progress") { + t.Fatalf("expected fetch --all --prune --progress command, got %q", command) } } @@ -184,7 +184,7 @@ func TestFetchPRMergeRefUsesPRRNamespaceDestination(t *testing.T) { } command := strings.Join(runner.commands[0], " ") - if !strings.Contains(command, "fetch origin pull/42/merge:refs/prr/pull/42/merge") { + if !strings.Contains(command, "fetch --progress origin pull/42/merge:refs/prr/pull/42/merge") { t.Fatalf("expected fetch destination to target PRR namespace, got %q", command) } } @@ -349,7 +349,7 @@ t.Fatalf("expected one fetch command, got %d", len(runner.commands)) } command := strings.Join(runner.commands[0], " ") -if !strings.Contains(command, "fetch origin pull/42/head:refs/prr/pull/42/head") { +if !strings.Contains(command, "fetch --progress origin pull/42/head:refs/prr/pull/42/head") { t.Fatalf("expected fetch destination to target PRR namespace, got %q", command) } }