Skip to content

fix(review): survive transient GitHub server errors - #711

Merged
piekstra merged 2 commits into
mainfrom
fix/github-transient-retry
Nov 3, 2026
Merged

fix(review): survive transient GitHub server errors#711
piekstra merged 2 commits into
mainfrom
fix/github-transient-retry

Conversation

@piekstra

@piekstra piekstra commented Nov 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

cr review made one attempt at every GitHub request, so one 5xx anywhere in a run failed it before anything was reviewed. On 2026-11-03 that happened at least 8 times, and a rerun with no change worked each time.

Reads now retry. Writes are protected by the existing marker reconciliation. A run that still fails says so on stdout, on stderr and in its exit code.

Root cause

Every provider error was already classified ErrRetryable, and nothing acted on that inside a run. It only chose the exit code (5). A fake server answering 502 then 200 got 1 request and this error: WhoAmI: gitprovider: retryable upstream error: github: status 502.

Recorded failures on 2026-11-03, from cr's own output:

Operation Statuses
GetPR 503
GetFileAtRef 500, 502, 504
GetDiff 502
ListInlineThreads 504

What is retried

Request Client retry Bound
All reads (REST and GraphQL queries) on 5xx, 429, rate-limit 403, connection failures, retryable GraphQL errors Yes, in one place: retrier in internal/gitprovider/github/retry.go 6 attempts; 1, 2, 4, 8, 16 s with equal jitter; 5 min total
GitHub App installation lookup and token mint Yes, even when a write triggered the refresh Same
git fetch on 5xx, 429 and connection failures Yes, in internal/gitexec 4 attempts; 2, 4, 8 s with equal jitter
Review, inline comment, issue comment, thread reply, resolve thread No. See below Outbox: 3 sends
Accept invitation No, and it is not an outbox action 1 attempt
401, 403 without a rate-limit signal, 404, 409, 422, certificate failures No 1 attempt

retrySafeOperation is an allowlist, so an operation added later gets one attempt until someone lists it.

Retry-After is honored in both forms. Secondary rate limits follow GitHub's documented order: Retry-After, then X-RateLimit-Reset when the quota is exhausted, then one minute when neither header is present. The existing code already treated 429 and GraphQL RATE_LIMITED as retryable, but mapped a rate-limit 403 to ErrPermission, which the outbox records as a terminal auth failure. Those 403s are now retryable. A requested wait over 2 minutes fails at once and names the wait.

How writes avoid duplicates

The client never re-sends a write, because a write can land and still return 5xx. The posting phase already embeds an action marker in every body and reconciles against the PR before dispatching, which is what made a manual rerun safe. In-run recovery reuses it (internal/outbox/write_retry.go):

  1. Wait at least 12 s, then 24 s. GitHub documents that it terminates API requests after 10 s, so the original request should have finished before the check.
  2. Re-read threads, comments and reviews, and look for the action's marker.
  3. Marker present: record the action as posted and send nothing.
  4. Marker absent: send again, up to 3 sends in total.
  5. Re-read failed: leave the action pending. Nothing is sent unverified.

Once one action has used up its sends, later actions in that run get one send each, so an outage cannot multiply the wait. Whatever remains stays pending, the run exits 5, and the next cr review reconciles it. Thread checkpoints still leave failures to the final outbox, which reconciles first.

Remaining risk: a write GitHub applies after both the settle wait and the marker check would be sent again. GitHub's own limit makes that unlikely, and a manual rerun carries the same risk today.

Reconciliation had a gap this depends on. GraphQL names an app slug where REST and the posting identity say slug[bot], so a GitHub App never recognized its own thread comments. A resend would have duplicated them. The thread queries now request __typename and normalize Bot logins.

What a failed run prints

A wrapper filtering for Status: saw nothing because a run that returned an error rendered nothing to stdout. Only stderr had the error, and the exit code was correct (5).

Stream Before After
stdout, text empty Status: error, Exit code: 5, Message: ... status 502 (gave up after 6 attempts)
stdout, --json empty the same record, "status": "error"
stderr the error the error
exit code 5 5

An incomplete run now names its cause and next step in place of completed with outcome incomplete. --dry-run output is unchanged.

Retries appear as event=info progress records: op="retry_read", op="retry_post" (with phase=waiting|reconciled|resending|abandoned) and op="retry_fetch". Paths in the cause are redacted the same way as error lines.

404

A 404 still fails at once. Two were on record:

Failure Finding
GetDiff 404 on 2026-11-03 GetPR succeeded immediately before it. The head had been pushed 9 minutes earlier and had already returned a 502 and a 504. The same diff returned 142 KB under another credential 10 s later, and a rerun worked. A freshly pushed head would not explain this, and the response body is redacted, so I cannot identify a replication lag.
GetPR 404 on 2026-11-02 The review identity had no permission on that private repository. That is a real 404, and retrying would only delay the error.

Tests

All offline against fake servers, with waits injected. go test ./..., golangci-lint run (0 issues) and go mod tidy are clean locally.

Layer Coverage
github/retry_test.go 502 then 200 for each 5xx; connection reset; GraphQL 5xx and error envelope; backoff sequence; give-up message; elapsed budget; Retry-After in both forms; rate-limit 403 and 429; refused long waits; 404 and other 4xx fail fast; every write sends once; every operation classified; cancel during a wait; context deadline; certificate failure; app token mint; bot login
outbox/write_retry_test.go Landed write is not re-sent, for review, rollup and reply; unlanded write is re-sent; failed read blocks the re-send; exhaustion then reconciliation by the next run; bundled inline comments; Retry-After; outage bound; cancel
noleak/github_transient_test.go The real command tree and real client: every read fails once and the review completes; PR reads stay at 502 and the run exits 5 with Status: error in text and JSON; landed review gives 1 POST; unlanded review gives 2 POSTs and 1 kept; failed check gives 1 POST and exit 5
gitexec, workbench, reviewcmd, app, progress Fetch retry and give-up; ref names cannot trigger a retry; cause kept in the chain; rendering; runtime wiring

Mutation checks, each reverted afterwards: letting the client retry SubmitReview produced 6 POSTs and failed the suite. Dropping the marker check produced duplicates and failed the suite. Dropping bot normalization failed its test. Reusing one GitHub App JWT across retries failed with a 401. Leaving fetch arguments in the matched output retried on a ref name. Omitting WriteRetry from one outbox.Post failed the architecture test.

GitLab reads are not retried here. GitLab documents no processing limit, so a GitLab write that fails retryably gets one send and stays pending for the next cr review to reconcile. The settle wait comes from ProviderCaps.WriteSettle: 12 seconds for GitHub, none for GitLab.

@piekstra
fix(review): survive transient GitHub server errors
83016d7 
cr made one attempt at every GitHub request, so a single 5xx anywhere in a
run failed it before anything was reviewed. Reads now retry 5xx, 429 and
connection failures with jittered backoff and honor Retry-After. Writes are
not retried by the client, because a write can land and still return 5xx;
the outbox re-reads the PR for the write's marker and resends only if it is
absent. Failed runs now print a Status line on stdout as well as the error
on stderr and a non-zero exit code.

Twapp-Session: 59d912f3-982f-402e-8bd8-3e31269a28b0
piekstra-dev
piekstra-dev reviewed Nov 3, 2026
View reviewed changes

@piekstra-dev piekstra-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated PR Review

Reviewed commit: 83016d70d4a5
Profile: reviewer - Posting as: piekstra-dev

Summary

Reviewer Findings
documentation:docs 0
documentation:drift 0
go:implementation-tests 2
policies:conventions 0
privacy:captured-data 0
structure:repo-health 1
architecture:system 2
go:implementation 0
security:code-auditor 0
design:interface 0
go:implementation-tests (2 findings)

Minor - internal/gitexec/client.go:41

The comment on commandError says PR-controlled ref names cannot decide whether a fetch failure is transient. Only the argument list is kept out of the match. The matcher still runs on git's stderr, and git echoes the requested ref there, for example couldn't find remote ref refs/heads/<name>. Git ref names can contain ssl_read, gnutls_ or early-eof-style substrings, and the gnutls_ and ssl_read alternatives need no spaces. A PR branch or ref with such a name that does not exist would be classified as transient. It would then be retried 4 times (about 14s of sleeping) and surfaced as ErrRetryable (exit 5) instead of a plain permanent failure.

The test transient words only in a ref name does not catch this. Its fake git prints fatal: couldn't find remote ref without the ref, so it only proves that args are excluded.

Fix: anchor the patterns to git and curl diagnostic prefixes, such as ^(fatal|error): lines and curl \d+. Alternatively, strip the fetch's ref arguments from detail before matching. Also make the fake git echo the ref name, as real git does, so the test fails if ref text can trigger a retry.

Minor - internal/gitprovider/github/retry_test.go:310

TestWritesAreNotRetriedByTheClient only exercises a 502. This PR also changes how writes are classified. restJSONAttempt now routes every non-2xx write response through responseError, so a rate-limit 403 on a write becomes ErrRetryable carrying a RetryAfterError. Before this change it was ErrPermission, which the outbox records as a terminal auth failure (exit 4 instead of 5). The outbox tests inject a synthetic RetryAfterError, and the read-side tests cover the 403 rate-limit mapping through GetPR, WhoAmI and GetDiff. Nothing runs a write through the real client with a 403 secondary-limit response. If someone switched the write path back to mapWriteHTTPStatus, or mapWriteHTTPStatus took over 403 handling, every test would still pass. The result would be a terminal auth failure for a run that only needed to wait.

Fix: add a case to this table, or a sibling test. The server should answer a write such as SubmitReview or PostIssueComment with 403 plus Retry-After (and one variant with a "secondary rate limit" message and no headers). Assert ErrRetryable, not ErrPermission, that gitprovider.RetryAfterHint returns the wait, and that the server saw exactly one request.

structure:repo-health (1 finding)

Minor - internal/gateio/gateio.go:569

Invariant: every outbox.Post call site must receive the same recovery policy, or a write path silently loses in-run recovery. This diff threads WriteRetry by hand into four separate outbox.Options{...} literals (three in gateio.go: repair, approval override, retry-posts; one in reviewrun.continueRun), duplicating the Store/Provider/Limiter/Now/WriteRetry copy each time. A future Post call site that omits the field compiles fine and falls back to a single attempt with no error, and no test covers that omission. Impact: inconsistent write behaviour between repair, override and normal posting paths that is hard to notice. Fix: add one small helper (e.g. outboxOptions(opts Options) outbox.Options in gateio, reused by reviewrun) so the field is wired in one place, or add a test that asserts each Post path passes a non-nil WriteRetry.

architecture:system (2 findings)

Major - internal/outbox/write_retry.go:18

The 12 s settle wait is a GitHub-specific guarantee placed in the host-agnostic outbox, and it also governs GitLab writes, where nothing establishes that it holds.

Scenario: buildReviewRunner in internal/app/runtime.go passes outbox.DefaultWriteRetry(...) for every provider, and gitlab/http.go maps every 5xx to ErrRetryable. A POST to a merge request's notes endpoint gets a 502 or 504 from a proxy, or a connection reset, while the GitLab backend is still processing it. Self-managed proxy and application timeouts are set by the operator, and nothing in this repository bounds them near 10 s. recoverWrite waits about 12-15 s, finds no marker, and sends again, and it can do the same again about 24-30 s later. The late first write then lands too, so the merge request carries a duplicate review note, inline discussion or reply.

This contradicts the safety argument in docs/architecture.md and docs/review-lifecycle.md, which rest on GitHub's 10-second processing limit (write_retry.go:15 says the same). The PR's stated remaining risk covers only GitHub. outbox/write_retry_test.go has no GitLab case. Before this PR GitLab writes got one send per run, so in-run resend is a new double-execution path for that host.

Fix: make the policy per host. Either enable WriteRetry only for providers with a documented processing limit, leaving GitLab failures pending for the next cr review to reconcile, or take the settle value from the provider (for example through ProviderCaps) with a GitLab value justified by its timeouts. Also state GitLab's behaviour in docs/review-lifecycle.md.

Minor - internal/gitprovider/github/app_auth.go:247

The new retry budget can outlast the single JWT it reuses, which turns a transient GitHub failure into an auth failure.

Scenario: a long run needs an installation token refresh during a GitHub incident. refreshLocked signs one JWT, with a 9-minute gitHubAppJWTLifetime, and uses it for both the installation lookup and the token mint. Each step now goes through retry.repeat, with a 5-minute defaultRetryMaxElapsed checked only before each sleep, and each attempt can take up to the 2-minute defaultHTTPTimeout. If the lookup spends 5 minutes on one-minute rate-limit waits or stalled connections, and the mint then retries for 4 more, the JWT expires and GitHub answers 401.

mapHTTPStatus classifies that as ErrAuth, and repeat stops. A read then fails the run with the auth exit code (3), telling the user to fix credentials that are fine. If a write triggered the refresh, classifyProviderError returns failureAuth and Post calls markFailedTerminal. That action is no longer pending, so the next cr review does not retry it, and the PR's promise that whatever remains stays pending and reconciles on the next run does not hold for it.

Fix: sign a fresh JWT per attempt, by passing a token function into doAppREST and calling a.jwt() inside the repeat closure. Alternatively, cap the combined lookup and mint elapsed time below the JWT lifetime.

Reviewer Coverage

Complete coverage details are retained in the local coverage.json artifact.

  • documentation:docs — complete (broad); scope: 3 files; examples: docs/architecture.md, docs/development.md, docs/review-lifecycle.md; body-inspected: 3 files; examples: docs/architecture.md, docs/development.md, docs/review-lifecycle.md; skipped: none; constraints: Verified retrySafeOperation, retry.go, write_retry.go and progress field names (attempt/max_attempts/delay_ms, retry_read/retry_post/retry_fetch) exist; did not verify the 'error' field key or phase values exhaustively.
  • documentation:drift — complete (constrained); scope: 20 files; examples: internal/app/progress.go, internal/app/runtime.go, internal/app/runtime_selection.go, internal/cmd/reviewcmd/reviewcmd.go, internal/gateio/gateio.go; 15 omitted; body-inspected: 20 files; examples: internal/app/progress.go, internal/app/runtime.go, internal/app/runtime_selection.go, internal/cmd/reviewcmd/reviewcmd.go, internal/gateio/gateio.go; 15 omitted; skipped: none; context (not coverage): 4 files; examples: README.md, docs/architecture.md, docs/development.md, docs/review-lifecycle.md; constraints: Compared the PR's doc edits (docs/architecture.md, docs/development.md, docs/review-lifecycle.md) against the code. README.md was searched for stale statements and none were found that the change makes false. Read the diffs for the reviewcmd, progress, reviewrun and view files in full. For the other assigned files I checked only the retry constants and the doc claims about them, not every function body.
  • go:implementation-tests — complete (broad); scope: 9 files; examples: internal/cmd/noleak/github_transient_test.go, internal/gitexec/client.go, internal/gitexec/client_test.go, internal/gitprovider/github/app_auth.go, internal/gitprovider/github/http.go; 4 omitted; body-inspected: 9 files; examples: internal/cmd/noleak/github_transient_test.go, internal/gitexec/client.go, internal/gitexec/client_test.go, internal/gitprovider/github/app_auth.go, internal/gitprovider/github/http.go; 4 omitted; skipped: none; constraints: Did not read the outbox call site that applies the 'later actions get one send each' rule, because it is outside the assigned scope. Did not run the test suite or linters. Read all three assigned test files in full. Did not run the tests or mutate the code to check that they fail. Read the full bodies of retry.go, write_retry.go and the production diffs for gitexec, http.go and app_auth.go. For the test files I read the gitexec tests in full and only skimmed the list of test names for retry_tes…
  • policies:conventions — complete (broad); scope: 8 files; examples: docs/architecture.md, docs/development.md, docs/review-lifecycle.md, internal/cmd/noleak/github_transient_test.go, internal/cmd/noleak/noleak_test.go; 3 omitted; body-inspected: 8 files; examples: docs/architecture.md, docs/development.md, docs/review-lifecycle.md, internal/cmd/noleak/github_transient_test.go, internal/cmd/noleak/noleak_test.go; 3 omitted; skipped: none; constraints: Shared cli-common docs were not available in context, so conformance to them was not checked beyond repo-local docs. The github_transient_test.go file was only partly read (first ~60 lines of 249).
  • privacy:captured-data — complete (constrained); scope: 35 files; examples: docs/architecture.md, docs/development.md, docs/review-lifecycle.md, internal/app/progress.go, internal/app/runtime.go; 30 omitted; body-inspected: 35 files; examples: docs/architecture.md, docs/development.md, docs/review-lifecycle.md, internal/app/progress.go, internal/app/runtime.go; 30 omitted; skipped: none; constraints: Review was a targeted scan of added literals (IDs, numbers, emails, URLs, logins, names) in the diff plus the commit message, PR body and the new testdata file, rather than a line-by-line read of every test body. Test literals found use obvious placeholders (octocat, reviewer-user, cr-reviewer, o/r, x/y, github.example) and small sequential IDs; no repository fixture policy was found to compare against.
  • structure:repo-health — complete (broad); scope: 6 files; examples: docs/development.md, internal/cmd/reviewcmd/reviewcmd.go, internal/gateio/gateio.go, internal/outbox/outbox.go, internal/outbox/write_retry.go; 1 omitted; body-inspected: 6 files; examples: docs/development.md, internal/cmd/reviewcmd/reviewcmd.go, internal/gateio/gateio.go, internal/outbox/outbox.go, internal/outbox/write_retry.go; 1 omitted; skipped: none; constraints: Reviewed diffs of in-scope files plus write_retry.go body; did not run tests or read the github/gitexec files outside assignment scope.
  • architecture:system — complete (broad); scope: 5 files; examples: internal/gitexec/client.go, internal/gitprovider/github/app_auth.go, internal/gitprovider/github/http.go, internal/gitprovider/github/retry.go, internal/outbox/write_retry.go; body-inspected: 5 files; examples: internal/gitexec/client.go, internal/gitprovider/github/app_auth.go, internal/gitprovider/github/http.go, internal/gitprovider/github/retry.go, internal/outbox/write_retry.go; skipped: none; context (not coverage): 7 files; examples: docs/architecture.md, docs/review-lifecycle.md, internal/app/runtime.go, internal/gitprovider/github/client.go, internal/gitprovider/gitlab/http.go; 2 omitted; constraints: GitLab server timeouts were not measured; the GitLab finding rests on the absence of any documented limit in this repository, and on the settle value being justified only by GitHub's limit. Static review only: no tests or fake-server runs were executed.
  • go:implementation — complete (broad); scope: 5 files; examples: internal/gitexec/client.go, internal/gitprovider/errors.go, internal/gitprovider/github/http.go, internal/gitprovider/github/retry.go, internal/outbox/write_retry.go; body-inspected: 5 files; examples: internal/gitexec/client.go, internal/gitprovider/errors.go, internal/gitprovider/github/http.go, internal/gitprovider/github/retry.go, internal/outbox/write_retry.go; skipped: none; constraints: Did not read callers of the new outbox recovery code or the wiring of the retry policy into the client. Static read of the changed files and diffs only; tests were not run and the accompanying test files were not read.
  • security:code-auditor — complete (broad); scope: 5 files; examples: internal/gitprovider/errors.go, internal/gitprovider/github/app_auth.go, internal/gitprovider/github/http.go, internal/gitprovider/github/retry.go, internal/outbox/write_retry.go; body-inspected: 5 files; examples: internal/gitprovider/errors.go, internal/gitprovider/github/app_auth.go, internal/gitprovider/github/http.go, internal/gitprovider/github/retry.go, internal/outbox/write_retry.go; skipped: none; constraints: Reviewed the diff and full contents of the assigned files; did not execute tests or the code.
  • design:interface — complete (broad); scope: 5 files; examples: internal/app/progress.go, internal/cmd/reviewcmd/reviewcmd.go, internal/progress/progress.go, internal/reviewrun/reviewrun.go, internal/view/review.go; body-inspected: 5 files; examples: internal/app/progress.go, internal/cmd/reviewcmd/reviewcmd.go, internal/progress/progress.go, internal/reviewrun/reviewrun.go, internal/view/review.go; skipped: none; constraints: Did not run tests or the CLI; judged the stdout/JSON failure record from source and docs/review-lifecycle.md, which documents it. Reviewed only the five assigned files' diffs plus nearby code in reviewcmd.go, view/review.go and docs; did not read the retry or outbox implementations (outside scope).

0 PR discussion threads considered. 0 summarized; 0 resolved.

Completed in 5m 02s | $2.89 | claude-sonnet-5-5, claude-opus-5-5 | cr 0.10.365
Field Value
Model claude-sonnet-5-5, claude-opus-5-5
Reviewers documentation:docs, documentation:drift, go:implementation-tests, policies:conventions, privacy:captured-data, structure:repo-health, architecture:system, go:implementation, security:code-auditor, design:interface
Engine claude_cli · claude-sonnet-5-5, claude-opus-5-5
Reviewed by cr · piekstra-dev
Duration 5m 02s wall · 9m 25s compute
Cost $2.89
Tokens 136 in / 47.3k out

Per-workstream usage

  • orchestrator-selection — claude-sonnet-5-5
    • In: 6
    • Out: 3.1k
    • Cache read: 23.3k
    • Cache create: 17.2k
    • Cost: $0.10
    • Duration: 23s
  • documentation:docs — claude-sonnet-5-5
    • In: 12
    • Out: 1.8k
    • Cache read: 71.7k
    • Cache create: 13.4k
    • Cost: $0.08
    • Duration: 34s
  • documentation:drift — claude-sonnet-5-5
    • In: 14
    • Out: 2.9k
    • Cache read: 123.8k
    • Cache create: 23.5k
    • Cost: $0.14
    • Duration: 37s
  • go:implementation-tests — claude-sonnet-5-5
    • In: 22
    • Out: 6.3k
    • Cache read: 363.8k
    • Cache create: 71.1k
    • Cost: $0.55
    • Duration: 1m 13s
  • policies:conventions — claude-sonnet-5-5
    • In: 10
    • Out: 1.5k
    • Cache read: 60.9k
    • Cache create: 17.4k
    • Cost: $0.09
    • Duration: 13s
  • privacy:captured-data — claude-sonnet-5-5
    • In: 8
    • Out: 1.8k
    • Cache read: 44.6k
    • Cache create: 15.7k
    • Cost: $0.09
    • Duration: 25s
  • structure:repo-health — claude-sonnet-5-5
    • In: 10
    • Out: 2.0k
    • Cache read: 79.9k
    • Cache create: 27.1k
    • Cost: $0.14
    • Duration: 23s
  • architecture:system — claude-opus-5-5
    • In: 20
    • Out: 22.0k
    • Cache read: 444.9k
    • Cache create: 85.5k
    • Cost: $1.21
    • Duration: 3m 51s
  • go:implementation — claude-sonnet-5-5
    • In: 8
    • Out: 2.0k
    • Cache read: 51.3k
    • Cache create: 25.0k
    • Cost: $0.12
    • Duration: 22s
  • security:code-auditor — claude-sonnet-5-5
    • In: 8
    • Out: 1.1k
    • Cache read: 49.1k
    • Cache create: 22.2k
    • Cost: $0.10
    • Duration: 25s
  • design:interface — claude-sonnet-5-5
    • In: 12
    • Out: 2.3k
    • Cache read: 90.6k
    • Cache create: 21.9k
    • Cost: $0.12
    • Duration: 40s
  • orchestrator-rollup — claude-sonnet-5-5
    • In: 6
    • Out: 491
    • Cache read: 66.5k
    • Cache create: 6.8k
    • Cost: $0.14
    • Duration: 13s

Comment thread internal/outbox/write_retry.go Resolved
Comment thread internal/gitexec/client.go Resolved
Comment thread internal/gitprovider/github/app_auth.go Resolved
Comment thread internal/gateio/gateio.go Resolved
Comment thread internal/gitprovider/github/retry_test.go Resolved
@piekstra
fix(review): take the write settle wait from the provider and harden …
c4245c9 
…retries

Declare the settle wait through ProviderCaps.WriteSettle. GitHub documents a
10-second processing limit, so it declares 12 seconds. GitLab documents none, so a
failed GitLab write gets one send and stays pending for the next run.

Sign a fresh GitHub App JWT for each attempt, because retries can outlast one
JWT's lifetime. Strip the fetch arguments from git's output before matching it, so
a ref name cannot decide that a failure is transient. Add tests for rate-limited
writes through the real client and for every outbox.Post path naming WriteRetry.

Twapp-Session: 59d912f3-982f-402e-8bd8-3e31269a28b0
piekstra-dev
piekstra-dev approved these changes Nov 3, 2026
View reviewed changes

@piekstra-dev piekstra-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated PR Review

Reviewed commit: c4245c9f7663
Profile: reviewer - Posting as: piekstra-dev

Summary

Reviewer Findings
documentation:docs 0
documentation:drift 0
go:implementation-tests 0
policies:conventions 0
privacy:captured-data 0
structure:repo-health 0
architecture:system 0
go:implementation 0
security:code-auditor 0
design:interface 0

Reviewer Coverage

Complete coverage details are retained in the local coverage.json artifact.

  • documentation:docs — complete (constrained); scope: 3 files; examples: docs/architecture.md, docs/development.md, docs/review-lifecycle.md; body-inspected: 3 files; examples: docs/architecture.md, docs/development.md, docs/review-lifecycle.md; skipped: none; constraints: Verified ProviderCaps.WriteSettle, retry.go and write_retry.go exist and match the docs; did not exhaustively verify progress field keys or phase values.
  • documentation:drift — complete (constrained); scope: 21 files; examples: internal/app/progress.go, internal/app/runtime.go, internal/app/runtime_selection.go, internal/cmd/reviewcmd/reviewcmd.go, internal/gateio/gateio.go; 16 omitted; body-inspected: 21 files; examples: internal/app/progress.go, internal/app/runtime.go, internal/app/runtime_selection.go, internal/cmd/reviewcmd/reviewcmd.go, internal/gateio/gateio.go; 16 omitted; skipped: none; context (not coverage): 4 files; examples: README.md, docs/architecture.md, docs/development.md, docs/review-lifecycle.md; constraints: Compared the PR's doc edits (docs/architecture.md, docs/development.md, docs/review-lifecycle.md) with the code, including the new ProviderCaps.WriteSettle behavior (GitHub 12s, GitLab none). No stale README text found. Read the diffs for the reviewcmd, progress, types.go and related files in full. For the other assigned files I checked only the retry constants and the doc claims about them, not every function body.
  • go:implementation-tests — complete (constrained); scope: 11 files; examples: internal/architecture/outbox_write_retry_test.go, internal/cmd/noleak/github_transient_test.go, internal/gitexec/client.go, internal/gitexec/client_test.go, internal/gitprovider/github/app_auth.go; 6 omitted; body-inspected: 11 files; examples: internal/architecture/outbox_write_retry_test.go, internal/cmd/noleak/github_transient_test.go, internal/gitexec/client.go, internal/gitexec/client_test.go, internal/gitprovider/github/app_auth.go; 6 omitted; skipped: none; context (not coverage): 1 file; examples: internal/outbox/outbox.go; constraints: For retry_test.go, write_retry_test.go and gitexec/client_test.go I read the changed hunks at this head (JWT, rate-limited-write and WriteSettle tests) plus the earlier full reads. I read github_transient_test.go, retry.go and http.go in full at the earlier head 83016d7. git diff shows them unchanged at c4245c9, so I did not re-read them at this head. Static review only. I did not run the tests, the linter or any mutation checks. The previously raised issues (ref-name matching, rate-l…
  • policies:conventions — complete (constrained); scope: 8 files; examples: docs/architecture.md, docs/development.md, docs/review-lifecycle.md, internal/cmd/noleak/github_transient_test.go, internal/cmd/noleak/noleak_test.go; 3 omitted; body-inspected: 8 files; examples: docs/architecture.md, docs/development.md, docs/review-lifecycle.md, internal/cmd/noleak/github_transient_test.go, internal/cmd/noleak/noleak_test.go; 3 omitted; skipped: none; constraints: Only docs, noleak_test.go and the reviewcmd diffs were read in full; github_transient_test.go and reviewcmd_test.go were skimmed. Shared cli-common docs were not available in context, so conformance to them was not checked beyond repo-local docs.
  • privacy:captured-data — complete (constrained); scope: 38 files; examples: docs/architecture.md, docs/development.md, docs/review-lifecycle.md, internal/app/progress.go, internal/app/runtime.go; 33 omitted; body-inspected: 38 files; examples: docs/architecture.md, docs/development.md, docs/review-lifecycle.md, internal/app/progress.go, internal/app/runtime.go; 33 omitted; skipped: none; constraints: Review was a targeted scan of added literals (IDs, numbers, emails, URLs, logins, names) in the diff plus commit messages and PR body, not a line-by-line read of every test body. Test literals found use obvious placeholders (octocat, reviewer-user, cr-reviewer, o/r, x/y, github.example) and small sequential IDs; no repository fixture policy was found to compare against.
  • structure:repo-health — complete (constrained); scope: 6 files; examples: docs/development.md, internal/cmd/reviewcmd/reviewcmd.go, internal/gateio/gateio.go, internal/outbox/outbox.go, internal/outbox/write_retry.go; 1 omitted; body-inspected: 6 files; examples: docs/development.md, internal/cmd/reviewcmd/reviewcmd.go, internal/gateio/gateio.go, internal/outbox/outbox.go, internal/outbox/write_retry.go; 1 omitted; skipped: none; constraints: Reviewed diffs of in-scope files (docs/development.md, reviewcmd.go, gateio.go, outbox.go, write_retry.go, reviewrun.go); out-of-scope files such as the architecture test and GitHub client were not read, and no tests were run.
  • architecture:system — complete (constrained); scope: 5 files; examples: internal/gitexec/client.go, internal/gitprovider/github/app_auth.go, internal/gitprovider/github/http.go, internal/gitprovider/github/retry.go, internal/outbox/write_retry.go; body-inspected: 5 files; examples: internal/gitexec/client.go, internal/gitprovider/github/app_auth.go, internal/gitprovider/github/http.go, internal/gitprovider/github/retry.go, internal/outbox/write_retry.go; skipped: none; context (not coverage): 5 files; examples: docs/architecture.md, docs/review-lifecycle.md, internal/gitprovider/github/client.go, internal/gitprovider/types.go, internal/outbox/outbox.go; constraints: Because of that, the new JWT-per-attempt and settle-capability tests were read but not run; retry.go and http.go are unchanged since the previous review of this PR. Tests passed for internal/gitexec and internal/architecture only. internal/gitprovider/github did not build (cgo: clang module cache path contains a space). internal/outbox failed one test on go list parsing of the same path. Neither points at the PR.
  • go:implementation — complete (constrained); scope: 5 files; examples: internal/gitexec/client.go, internal/gitprovider/errors.go, internal/gitprovider/github/http.go, internal/gitprovider/github/retry.go, internal/outbox/write_retry.go; body-inspected: 5 files; examples: internal/gitexec/client.go, internal/gitprovider/errors.go, internal/gitprovider/github/http.go, internal/gitprovider/github/retry.go, internal/outbox/write_retry.go; skipped: none; constraints: Did not read callers of the outbox recovery code, the provider capability wiring, or the app-auth retry closure outside the assigned files. Static read of the changed files and diffs only; tests were not run and test files were not read.
  • security:code-auditor — complete (constrained); scope: 5 files; examples: internal/gitprovider/errors.go, internal/gitprovider/github/app_auth.go, internal/gitprovider/github/http.go, internal/gitprovider/github/retry.go, internal/outbox/write_retry.go; body-inspected: 5 files; examples: internal/gitprovider/errors.go, internal/gitprovider/github/app_auth.go, internal/gitprovider/github/http.go, internal/gitprovider/github/retry.go, internal/outbox/write_retry.go; skipped: none; constraints: Static read-only review of the diff and file bodies; tests and code were not executed.
  • design:interface — complete (constrained); scope: 5 files; examples: internal/app/progress.go, internal/cmd/reviewcmd/reviewcmd.go, internal/progress/progress.go, internal/reviewrun/reviewrun.go, internal/view/review.go; body-inspected: 5 files; examples: internal/app/progress.go, internal/cmd/reviewcmd/reviewcmd.go, internal/progress/progress.go, internal/reviewrun/reviewrun.go, internal/view/review.go; skipped: none; constraints: Did not run tests or the CLI; judged the stdout/JSON failure record from source and docs/review-lifecycle.md, which documents it. Reviewed only the five assigned files' diffs plus nearby code and docs; did not read the retry, outbox or capability implementations (outside scope).

0 PR discussion threads considered. 0 summarized; 0 resolved.

Completed in 1m 57s | $3.98 | claude-sonnet-5-5, claude-opus-5-5 | cr 0.10.365
Field Value
Model claude-sonnet-5-5, claude-opus-5-5
Reviewers documentation:docs, documentation:drift, go:implementation-tests, policies:conventions, privacy:captured-data, structure:repo-health, architecture:system, go:implementation, security:code-auditor, design:interface
Engine claude_cli · claude-sonnet-5-5, claude-opus-5-5
Reviewed by cr · piekstra-dev
Duration 1m 57s wall · 4m 43s compute
Cost $3.98
Tokens 98 in / 18.2k out

Per-workstream usage

  • documentation:docs — claude-sonnet-5-5
    • In: 8
    • Out: 1.1k
    • Cache read: 95.0k
    • Cache create: 13.4k
    • Cost: $0.15
    • Duration: 20s
  • documentation:drift — claude-sonnet-5-5
    • In: 8
    • Out: 1.6k
    • Cache read: 137.9k
    • Cache create: 15.4k
    • Cost: $0.23
    • Duration: 25s
  • go:implementation-tests — claude-sonnet-5-5
    • In: 16
    • Out: 3.9k
    • Cache read: 741.6k
    • Cache create: 30.4k
    • Cost: $0.62
    • Duration: 50s
  • policies:conventions — claude-sonnet-5-5
    • In: 8
    • Out: 1.3k
    • Cache read: 108.7k
    • Cache create: 12.2k
    • Cost: $0.16
    • Duration: 12s
  • privacy:captured-data — claude-sonnet-5-5
    • In: 8
    • Out: 1.7k
    • Cache read: 109.2k
    • Cache create: 16.4k
    • Cost: $0.18
    • Duration: 16s
  • structure:repo-health — claude-sonnet-5-5
    • In: 8
    • Out: 1.2k
    • Cache read: 154.7k
    • Cache create: 19.1k
    • Cost: $0.24
    • Duration: 22s
  • architecture:system — claude-opus-5-5
    • In: 12
    • Out: 3.5k
    • Cache read: 620.6k
    • Cache create: 26.4k
    • Cost: $1.62
    • Duration: 59s
  • go:implementation — claude-sonnet-5-5
    • In: 8
    • Out: 1.4k
    • Cache read: 145.7k
    • Cache create: 17.8k
    • Cost: $0.22
    • Duration: 23s
  • security:code-auditor — claude-sonnet-5-5
    • In: 8
    • Out: 1.0k
    • Cache read: 132.9k
    • Cache create: 16.3k
    • Cost: $0.19
    • Duration: 26s
  • design:interface — claude-sonnet-5-5
    • In: 8
    • Out: 1.1k
    • Cache read: 129.3k
    • Cache create: 13.5k
    • Cost: $0.20
    • Duration: 19s
  • orchestrator-rollup — claude-sonnet-5-5
    • In: 6
    • Out: 317
    • Cache read: 83.9k
    • Cache create: 3.7k
    • Cost: $0.17
    • Duration: 6s

@piekstra
piekstra merged commit 4bd02e8 into main Nov 3, 2026
12 checks passed
@piekstra
piekstra deleted the fix/github-transient-retry branch November 3, 2026 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@piekstra-dev piekstra-dev piekstra-dev approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@piekstra @piekstra-dev