Clear only the credential GitHub rejected, and stop treating a plain 403 as one [patch] - #316
Merged
Merged
Conversation
…lain 403 as one [patch] MakeGitHubRequestAsync called OnAuthenticationFailure on any AuthorizationException, clearing the provider-level AccountId and Token even when the request had been sent with an owner's override token. One owner with an expired token took every other owner offline, while the token that actually failed stayed in the store and was rejected again every cycle. A rejected owner token now clears that owner's token only, so the owner falls back to the provider token, and the provider credentials are left alone. A rejected provider token still clears the provider credentials as before. A non-rate-limit 403 means the token may not make this one request, such as a workflow dispatch without actions: write. It is now reported as an error on the request and no longer erases any credentials. Fixes #297 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XkEHuDnqdEMrwMy3iu9Kyr
|
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.
Fixes #297
What was wrong
MakeGitHubRequestAsyncalways calledBuildProvider.OnAuthenticationFailure()on anAuthorizationExceptionor on a 403 that wasn't a rate limit. That cleared the provider-levelAccountIdandToken, whichever credential the request had actually used. This had three effects:HasValidCredentials(owner)stayed true and that owner was rejected again every cycle.actions: write, erased the global token.The fix
MakeGitHubRequestAsyncrecords which credential the request is sent with. If anAuthorizationExceptioncomes back for a request sent with an owner's override token, only that owner's token is cleared. That owner then falls back to the provider token, the provider credentials are left alone, and the status readsAuthFailedwith the owner's name.ProviderStatus.Error, "Forbidden: "), and the request returnsfalse. No credentials are touched.Tests
These run in
GitHubRequestOutcomeTestsagainst the in-memoryTokenStorage:ARejectedOwnerTokenClearsOnlyThatOwnersToken: the owner token is cleared and the provider token survives.APlain403LeavesTheCredentialsInPlace: both the provider and owner tokens survive a plain 403.ARejectedProviderTokenStillClearsTheProviderCredentials: the unchanged path, pinned down.APlain403ReportsFailurenow expectsErrorinstead ofAuthFailed.With
GitHub.csreverted tomain, three of these fail: the two new tests that cover changed behaviour, plus the updatedAPlain403ReportsFailure. The provider-token test passes either way by design. With the fix in place,dotnet testpasses 75 of 75.This is independent of #315 (#298). Both branch from
mainand touch different files.🤖 Generated with Claude Code
https://claude.ai/code/session_01XkEHuDnqdEMrwMy3iu9Kyr
Generated by Claude Code