fix: detect mid-push auth rotation and abort cleanly#463
Merged
tomasz-tomczyk merged 1 commit intomainfrom May 5, 2026
Merged
Conversation
When `gh`'s auth token rotated or expired partway through `crit push`,
the per-item gh-api loop kept firing — each subsequent call returned
HTTP 401, and the user got a wall of generic transport errors with no
clear recovery path. The successful and failed posts were also
interleaved enough to make a clean retry risky.
This change introduces an `errGHAuthFailed` sentinel and an
`isGHAuthFailure` parser that recognises gh's canonical 401 surfaces
("HTTP 401", "HTTP/2 401", "Bad credentials"). The four push-loop
gh-api callers (`createGHReview`, `postGHReply`, `patchGHComment`,
`deleteGHComment`) wrap their failures with the sentinel when 401 is
detected; the inner push helpers (`postPushReplies`, `pushEditedBodies`,
`pushDeletedComments`) check for it via `errors.Is` and break out of
their loops on first sight. `runPushLive` then short-circuits the
remaining steps, prints
Pushed K of N comments before auth failed.
Run 'gh auth refresh' then re-run 'crit push' to post the rest.
to stderr, and exits non-zero. The K-of-N totals cover postable
top-level comments, replies, edits and pending deletes attempted before
the abort.
Dedup safety on retry is preserved by the existing review-file
contract: only items where `github_id == 0` are considered candidates
for posting (`collectNewRepliesForPush`, `bucketCommentsForPush`), and
`PendingGitHubDeletes` is only drained for IDs that returned a
non-auth-failure status. So after `gh auth refresh` a re-run of
`crit push` posts only the (N-K) items that hadn't landed — never a
duplicate of one that did.
`runPushLive` now returns an exit code instead of calling `os.Exit`
directly, so the new test can drive it end-to-end without terminating
the test process.
Closes #452
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (66.98%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #463 +/- ##
==========================================
+ Coverage 68.72% 69.04% +0.31%
==========================================
Files 36 36
Lines 10634 10698 +64
==========================================
+ Hits 7308 7386 +78
+ Misses 2764 2748 -16
- Partials 562 564 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
crit pushnow detects HTTP 401 fromgh apicalls and aborts the rest of the push loop instead of spamming per-item errors. Each per-call failure is wrapped witherrGHAuthFailedso the loop can short-circuit viaerrors.Is.Pushed K of N comments before auth failed. Run 'gh auth refresh' then re-run 'crit push' to post the rest.and exits non-zero.runPushLivewas refactored fromos.Exit(1)to returning an int so the new test can drive it without terminating the process; only call site isrunPush.gh's canonical error-line shapes (gh: HTTP 401:andHTTP/x 401at line start) rather than loose substring match — avoids false-positives when comment bodies in the PR happen to contain "HTTP 401" or "Bad credentials".gh auth refreshonly retries comments wheregithub_id == 0(and items still in the pending-deletes queue) — no double-post of already-pushed work.Review
Test plan
push_auth_rotation_test.go: stubsghwith a fake binary that returns 201 thengh: HTTP 401: Bad credentials. Asserts non-zero exit, K-of-N message in stderr, fake gh invoked exactly twice (loop aborted), only the first reply gotgithub_idset. Then re-runs with healthy fake — only the un-pushed replies are retried, already-pushedgithub_idis unchanged.github_auth_test.go: table-driven coverage of the matcher — positives (gh: HTTP 401:, allHTTP/x 401include shapes) and negatives (HTTP 401mid-line,Bad credentialsmid-line, 200/404 status lines, indented variants).go test -race -count=1 ./...green.Closes #452
🤖 Generated with Claude Code