fix(review): survive transient GitHub server errors

docs/architecture.md

@@ -102,6 +102,16 @@ This keeps markers, retries, reconciliation, idempotency, and resume behavior in
 one place. New commands such as `cr respond` should produce planned thread
 actions and let the reviewplan/ledger/outbox flow perform provider writes.
 
+The GitHub client retries only reads, in one place: `retrySafeOperation` in
+`internal/gitprovider/github/retry.go` lists what may be re-sent, and everything
+else, including any operation added later, gets one attempt. A write can land at
+GitHub and still return 5xx, so a blind retry would post twice. Writes are retried
+by the outbox, which waits for GitHub's 10-second processing limit to pass, re-reads
+the PR, and sends again only when the action's marker is absent. If that read fails,
+the write stays pending rather than being sent unverified. The wait comes from the
+provider's `ProviderCaps.WriteSettle`. A provider that declares none, as GitLab does
+because nothing bounds its proxy and application timeouts, gets no resend in the run.
+
 Accepting a repository invitation is the one exception. It changes the posting
 identity's account, not a pull request, so it has no marker, no run, and
 nothing to reconcile, and accepting an invitation that is already gone fails

docs/development.md

@@ -78,6 +78,11 @@ Review progress includes instant catalog-resolution, final selection, and
 per-reviewer assignment records. These expose IDs, winning provenance,
 selection reasoning, and assigned paths without logging prompt bodies.
 
+Retries during a review are `event=info` records with `op="retry_read"`,
+`op="retry_post"` or `op="retry_fetch"`. They carry `attempt`, `max_attempts`,
+`delay_ms` where a wait is scheduled, and the cause in `error`; `retry_post` adds the
+recovery `phase` (`waiting`, `reconciled`, `resending` or `abandoned`).
+
 Current command coverage is `benchmark run`, `benchmark select`,
 `benchmark compare`, `data prune`, `data purge`, `config clear`, `me`, and
 `sessions delete`.

docs/review-lifecycle.md

@@ -110,6 +110,18 @@ not reached the PR.
 - Outbox reconciliation checks markers before every dispatch, so a process
   crash after a provider write but before the local update does not duplicate
   the reply or final review.
+- GitHub reads and `git fetch` retry 5xx, 429 and connection failures with
+  jittered backoff and honor `Retry-After`. A 404 fails immediately.
+- A write that fails retryably during the final outbox is checked against the PR
+  after a settle wait, marked posted if its marker is there, and otherwise resent,
+  up to three sends. Once one action uses up its sends, later actions in that run
+  get one send each, so an outage cannot multiply the wait. Whatever is left stays
+  pending, the run exits 5, and the next `cr review` reconciles it. Duplicates remain
+  possible only if GitHub applies a write after the settle wait and the marker check.
+  GitLab declares no processing limit, so a GitLab write that fails retryably gets one
+  send and stays pending for the next `cr review` to reconcile.
+- A run that fails renders `Status: error` and the cause on stdout, or the same
+  record as JSON, as well as the error on stderr and a non-zero exit code.
 - Thread replies are ordered before resolutions, and a resolution is not sent
   until its reply is posted or reconciled.
 - Completed summary markers on the PR outrank stale local fallback state.

internal/app/progress.go

@@ -1,10 +1,56 @@
 package app
 
-import "github.com/open-cli-collective/codereview-cli/internal/progress"
+import (
+	"strconv"
+	"time"
+
+	"github.com/open-cli-collective/codereview-cli/internal/gitexec"
+	githubprovider "github.com/open-cli-collective/codereview-cli/internal/gitprovider/github"
+	"github.com/open-cli-collective/codereview-cli/internal/outbox"
+	"github.com/open-cli-collective/codereview-cli/internal/progress"
+)
 
 func endProgressSpan(span *progress.Span, err error) error {
 	if span != nil {
 		_ = span.End(err)
 	}
 	return err
 }
+
+// githubRetryProgress reports each retried GitHub read, so a slow run shows why.
+func githubRetryProgress(logger *progress.Logger, command string) func(githubprovider.RetryEvent) {
+	return func(event githubprovider.RetryEvent) {
+		logger.InfoReason(command, "retry_read", string(event.Op), event.Reason,
+			progress.Field{Key: "attempt", Value: strconv.Itoa(event.Attempt)},
+			progress.Field{Key: "max_attempts", Value: strconv.Itoa(event.MaxAttempts)},
+			progress.Field{Key: "delay_ms", Value: strconv.FormatInt(event.Delay.Milliseconds(), 10)},
+		)
+	}
+}
+
+// gitFetchRetryProgress reports each retried git fetch.
+func gitFetchRetryProgress(logger *progress.Logger, command string) func(gitexec.RetryEvent) {
+	return func(event gitexec.RetryEvent) {
+		logger.InfoReason(command, "retry_fetch", "workbench", event.Reason,
+			progress.Field{Key: "attempt", Value: strconv.Itoa(event.Attempt)},
+			progress.Field{Key: "max_attempts", Value: strconv.Itoa(event.MaxAttempts)},
+			progress.Field{Key: "delay_ms", Value: strconv.FormatInt(event.Delay.Milliseconds(), 10)},
+		)
+	}
+}
+
+// writeRetryProgress reports each step of recovering a failed GitHub write.
+func writeRetryProgress(logger *progress.Logger, command string) func(outbox.WriteRetryEvent) {
+	return func(event outbox.WriteRetryEvent) {
+		fields := []progress.Field{
+			{Key: "phase", Value: string(event.Phase)},
+			{Key: "action", Value: event.ActionID},
+			{Key: "attempt", Value: strconv.Itoa(event.Attempt)},
+			{Key: "max_attempts", Value: strconv.Itoa(event.MaxAttempts)},
+		}
+		if event.Delay > time.Duration(0) {
+			fields = append(fields, progress.Field{Key: "delay_ms", Value: strconv.FormatInt(event.Delay.Milliseconds(), 10)})
+		}
+		logger.InfoReason(command, "retry_post", string(event.Kind), event.Reason, fields...)
+	}
+}

internal/app/runtime.go

@@ -119,22 +119,29 @@ type Dependencies struct {
 	CheckAgentSources      func(context.Context, []string, io.Writer)
 }
 
+// gitCommandFactory returns the injected factory, or the production one reporting
+// fetch retries to onRetry.
+func (d Dependencies) gitCommandFactory(onRetry func(gitexec.RetryEvent)) func(string, gitexec.TokenSource, string) (func(context.Context, string, ...string) ([]byte, error), error) {
+	if d.NewGitCommand != nil {
+		return d.NewGitCommand
+	}
+	return func(host string, tokens gitexec.TokenSource, username string) (func(context.Context, string, ...string) ([]byte, error), error) {
+		client, err := gitexec.NewWithUsername(host, tokens, username)
+		if err != nil {
+			return nil, err
+		}
+		client.OnRetry = onRetry
+		return client.Run, nil
+	}
+}
+
 func (d Dependencies) withDefaults() Dependencies {
 	if d.NewGitProvider == nil {
 		d.NewGitProvider = gitproviders.New
 	}
 	if d.ResolvePostingIdentity == nil {
 		d.ResolvePostingIdentity = resolvePostingIdentity
 	}
-	if d.NewGitCommand == nil {
-		d.NewGitCommand = func(host string, tokens gitexec.TokenSource, username string) (func(context.Context, string, ...string) ([]byte, error), error) {
-			client, err := gitexec.NewWithUsername(host, tokens, username)
-			if err != nil {
-				return nil, err
-			}
-			return client.Run, nil
-		}
-	}
 	if d.ResolveRepoRoot == nil {
 		d.ResolveRepoRoot = appruntime.ResolveRepoRoot
 	}
@@ -175,12 +182,12 @@ func Open(ctx context.Context, req OpenRequest) (Runtime, error) {
 		cleanup()
 		return Runtime{}, err
 	}
-	repoProvider, repoCredential, err := deps.NewGitProvider(profile.Git, repoProviderStore, gitProviderOptions(profile, profile.Git, req.PRRef))
+	repoProvider, repoCredential, err := deps.NewGitProvider(profile.Git, repoProviderStore, gitProviderOptions(profile, profile.Git, req.PRRef, githubRetryProgress(req.Progress, command)))
 	if err != nil {
 		cleanup()
 		return Runtime{}, err
 	}
-	gitCommand, err := deps.NewGitCommand(profile.Git.Host, repositoryTokenSource(repoProvider, repoCredential), gitproviders.GitBasicAuthUsername(profile.Git.ProviderKind()))
+	gitCommand, err := deps.gitCommandFactory(gitFetchRetryProgress(req.Progress, command))(profile.Git.Host, repositoryTokenSource(repoProvider, repoCredential), gitproviders.GitBasicAuthUsername(profile.Git.ProviderKind()))
 	if err != nil {
 		cleanup()
 		return Runtime{}, err
@@ -192,7 +199,7 @@ func Open(ctx context.Context, req OpenRequest) (Runtime, error) {
 		cleanup()
 		return Runtime{}, err
 	}
-	postingProvider, credential, err := deps.NewGitProvider(postingGit, postingProviderStore, gitProviderOptions(profile, postingGit, req.PRRef))
+	postingProvider, credential, err := deps.NewGitProvider(postingGit, postingProviderStore, gitProviderOptions(profile, postingGit, req.PRRef, githubRetryProgress(req.Progress, command)))
 	if err != nil {
 		cleanup()
 		return Runtime{}, err
@@ -374,11 +381,12 @@ func normalizeRuntimeProfile(profile config.Profile) config.Profile {
 	}).Profiles["runtime"]
 }
 
-func gitProviderOptions(profile config.Profile, git config.GitConfig, ref gitprovider.PRRef) gitproviders.Options {
+func gitProviderOptions(profile config.Profile, git config.GitConfig, ref gitprovider.PRRef, onRetry func(githubprovider.RetryEvent)) gitproviders.Options {
 	opts := gitproviders.Options{}
 	if git.ProviderKind() != config.GitProviderGitHub {
 		return opts
 	}
+	opts.GitHub.Retry.OnRetry = onRetry
 	if installationID := config.PinnedGitHubAppInstallationIDForGit(profile, git); installationID != "" {
 		opts.GitHub.InstallationID = installationID
 		return opts
@@ -409,6 +417,7 @@ func runtimeLayout() (statepaths.Layout, error) {
 func buildReviewRunner(ledgerStore *ledger.Store, repoProvider gitprovider.GitProvider, postingProvider gitprovider.GitProvider, adapter llm.Adapter, profile config.Profile, limiter outbox.Limiter, layout statepaths.Layout, warnings io.Writer, logger *progress.Logger, dispatcher *hookDispatcher, req OpenRequest, command string, gitCommand func(context.Context, string, ...string) ([]byte, error), resolveRepoRoot func(context.Context) (string, error)) reviewRunner {
 	taskProgress := newPipelineTaskProgress(logger, command, dispatcher)
 	liveProvider := runtimeProvider{read: repoProvider, write: postingProvider}
+	writeRetry := outbox.DefaultWriteRetry(writeRetryProgress(logger, command))
 	pipelineOpts := pipeline.Options{
 		Provider:            repoProvider,
 		Adapter:             adapter,
@@ -450,6 +459,7 @@ func buildReviewRunner(ledgerStore *ledger.Store, repoProvider gitprovider.GitPr
 			Provider:                liveProvider,
 			Planner:                 withProgressPlanner(logger, dispatcher, livePlanner{opts: pipelineOpts}),
 			Limiter:                 limiter,
+			WriteRetry:              writeRetry,
 			Layout:                  layout,
 			StaleHeartbeatThreshold: 10 * time.Minute,
 			Warnings:                warnings,
@@ -464,6 +474,7 @@ func buildReviewRunner(ledgerStore *ledger.Store, repoProvider gitprovider.GitPr
 			Provider:     liveProvider,
 			Adapter:      adapter,
 			Limiter:      limiter,
+			WriteRetry:   writeRetry,
 			Layout:       layout,
 			TaskProgress: taskProgress,
 			NewActionID:  pipelineOpts.NewActionID,

internal/app/runtime_selection.go

@@ -34,12 +34,12 @@ func OpenSelection(ctx context.Context, req SelectionOpenRequest) (SelectionRunt
 	}
 	// Selection-only paths read with the profile git credential while keeping
 	// PR-scoped GitHub App installation lookup aligned with review runs.
-	provider, credential, err := deps.NewGitProvider(profile.Git, gitStore, gitProviderOptions(profile, profile.Git, req.PRRef))
+	provider, credential, err := deps.NewGitProvider(profile.Git, gitStore, gitProviderOptions(profile, profile.Git, req.PRRef, nil))
 	if err != nil {
 		cleanup()
 		return SelectionRuntime{}, err
 	}
-	gitCommand, err := deps.NewGitCommand(profile.Git.Host, repositoryTokenSource(provider, credential), gitproviders.GitBasicAuthUsername(profile.Git.ProviderKind()))
+	gitCommand, err := deps.gitCommandFactory(nil)(profile.Git.Host, repositoryTokenSource(provider, credential), gitproviders.GitBasicAuthUsername(profile.Git.ProviderKind()))
 	if err != nil {
 		cleanup()
 		return SelectionRuntime{}, err

internal/app/runtime_test.go

@@ -33,6 +33,7 @@ import (
 	"github.com/open-cli-collective/codereview-cli/internal/modelcatalog"
 	"github.com/open-cli-collective/codereview-cli/internal/outbox"
 	"github.com/open-cli-collective/codereview-cli/internal/pipeline"
+	"github.com/open-cli-collective/codereview-cli/internal/progress"
 	"github.com/open-cli-collective/codereview-cli/internal/reporoot"
 	"github.com/open-cli-collective/codereview-cli/internal/review"
 	"github.com/open-cli-collective/codereview-cli/internal/reviewplan"
@@ -1430,3 +1431,80 @@ func pipelineRequest(ref gitprovider.PRRef, pr gitprovider.PR, profile config.Pr
 		PostingIdentity: identity,
 	}
 }
+
+func TestOpenReportsGitHubRetriesThroughProgressAndEnablesWriteRecovery(t *testing.T) {
+	provider := &gitprovider.Fake{}
+	identity := gitprovider.Identity{Login: "review-bot", ID: "bot-id"}
+	var captured []gitproviders.Options
+	deps := testDependencies(t,
+		func(_ config.GitConfig, _ credentials.Reader, opts gitproviders.Options) (gitprovider.GitProvider, gitprovider.Credential, error) {
+			captured = append(captured, opts)
+			return provider, gitprovider.Credential{Type: "pat", Token: "token"}, nil
+		},
+		func(context.Context, gitprovider.GitProvider, gitprovider.Credential, credentials.Reader, config.Profile) (gitprovider.Identity, error) {
+			return identity, nil
+		},
+		func(config.LLMConfig, credentials.Reader) (llm.Adapter, error) {
+			return &llm.FakeAdapter{}, nil
+		},
+	)
+	cfg := testConfig()
+	var out bytes.Buffer
+	runtime, err := Open(context.Background(), OpenRequest{
+		Config:       cfg,
+		Profile:      cfg.Profiles["home"],
+		ProfileName:  "home",
+		Command:      "review",
+		Progress:     progress.New(&out, false, nil),
+		PRRef:        gitprovider.PRRef{Host: "github.com", Owner: "o", Repo: "r", Number: 1},
+		Dependencies: deps,
+	})
+	if err != nil {
+		t.Fatalf("Open: %v", err)
+	}
+	defer runtime.Cleanup()
+
+	if len(captured) != 2 {
+		t.Fatalf("providers built = %d, want read and posting", len(captured))
+	}
+	for i, opts := range captured {
+		if opts.GitHub.Retry.OnRetry == nil {
+			t.Fatalf("provider %d has no retry observer", i)
+		}
+	}
+	captured[0].GitHub.Retry.OnRetry(githubprovider.RetryEvent{Op: gitprovider.OperationGetPR, Attempt: 2, MaxAttempts: 6, Delay: 3 * time.Second, Reason: "github: status 502"})
+	line := out.String()
+	for _, want := range []string{`event=info`, `command="review"`, `op="retry_read"`, `target="GetPR"`, `attempt="2"`, `max_attempts="6"`, `delay_ms="3000"`, `error="github: status 502"`} {
+		if !strings.Contains(line, want) {
+			t.Fatalf("progress = %q, want %q", line, want)
+		}
+	}
+
+	runner, ok := runtime.Runner.(reviewRunner)
+	if !ok {
+		t.Fatalf("runner = %T", runtime.Runner)
+	}
+	if runner.live.WriteRetry == nil || runner.respond.WriteRetry == nil {
+		t.Fatalf("write recovery live = %v respond = %v, want both enabled", runner.live.WriteRetry, runner.respond.WriteRetry)
+	}
+	out.Reset()
+	runner.live.WriteRetry.OnEvent(outbox.WriteRetryEvent{Phase: outbox.WriteRetryWaiting, ActionID: "action-1", Kind: ledger.PlannedActionSubmitReview, Attempt: 1, MaxAttempts: 3, Delay: 12 * time.Second, Reason: "github: status 504"})
+	for _, want := range []string{`op="retry_post"`, `target="submit_review"`, `phase="waiting"`, `action="action-1"`, `delay_ms="12000"`, `error="github: status 504"`} {
+		if !strings.Contains(out.String(), want) {
+			t.Fatalf("progress = %q, want %q", out.String(), want)
+		}
+	}
+}
+
+func TestGitFetchRetryProgress(t *testing.T) {
+	var out bytes.Buffer
+	gitFetchRetryProgress(progress.New(&out, false, nil), "review")(gitexec.RetryEvent{Attempt: 1, MaxAttempts: 4, Delay: time.Second, Reason: "git fetch https://x/y: returned error: 502"})
+	for _, want := range []string{`op="retry_fetch"`, `attempt="1"`, `delay_ms="1000"`, `[path]`, `returned error: 502`} {
+		if !strings.Contains(out.String(), want) {
+			t.Fatalf("progress = %q, want %q", out.String(), want)
+		}
+	}
+	if strings.Contains(out.String(), "https://x/y") {
+		t.Fatalf("progress = %q, want the path redacted", out.String())
+	}
+}