Repository navigation
Carry a token refusal's OAuth error and Retry-After, and type device authorization's 429 - #972
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9df81bffb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Review summary at
|
5a8c49b to
8d2295a
Compare
77c59f5 to
de2b9e4
Compare
Dependency / rollout noteThis change has no server/API rollout dependency. It consumes the existing OAuth refusal contract—HTTP Its only ordering dependency is inside the SDK: it builds on the typed Go token-refusal errors introduced by #971, so #971 should merge first. The CLI already consumes these fields and retry semantics through an unpublished SDK commit; publishing this work in an SDK release lets the CLI return to a tagged dependency. |
…vice authorization's A refused refresh came back as a class and a status, and nothing else a caller could act on. bc3's abuse tracker answers every OAuth endpoint with a 429 and a Retry-After for up to a day once a client and address have failed often enough; the Go SDK reported that as an api_error with no wait, so the caller's only move was to try again into the block. Device authorization was worse: any non-2xx was "device authorization failed with status 429", with neither the server's reason nor the wait. basecamp.Error gains OAuthError, the RFC 6749 error code the endpoint named. The Exchanger and AuthManager refresh now type a 429 as rate_limit and carry OAuthError and RetryAfter on every class. RequestDeviceAuthorization reads a 4xx body's error and error_description into OAuthError and the message, types a 429 as rate_limit with its RetryAfter, and keeps everything else api_error. ParseRetryAfter exports the client's own §6 parser so the oauth package reads Retry-After the same way.
…nd the composed message A 429 whose body could not be read, or was over the cap, came back as an untyped read error from the Exchanger and as api_error from AuthManager, losing the rate limit and its wait. The body now only adds the OAuth error; the status and Retry-After classify the refusal whatever the body did. The device-authorization and Exchanger messages bounded the code and the description separately, so together they could reach twice the cap; the composed message is bounded too. The Retry-After test tolerance now applies only to the HTTP-date case.
de2b9e4 to
7e83bb2
Compare
Stacked on #971, which made the Go token-endpoint refusals typed; this builds on that typed error, and GitHub will retarget it to
mainwhen #971 merges.What
A token-endpoint or device-authorization refusal now carries what the server said, on
*basecamp.Error:OAuthErrorandOAuthErrorDescription: the RFC 6749erroranderror_description. Both are bounded the way the message is, and nothing else from the body is rendered (SPEC §9).RetryAfter: the wait fromRetry-After, parsed by the client's own §6 parser.ParseRetryAfterexports that parser so theoauthpackage reads the header the same way.rate_limitin the Exchanger and inAuthManagerrefresh. Rules 1 and 2 still come first, soinvalid_granton any status is stillauth_required. This is the same refinement Rust already makes.RequestDeviceAuthorizationgets the same treatment. A 4xx body'serroranderror_descriptiongo into the typed fields and the message. A 429 israte_limitwith itsRetryAfter, and everything else staysapi_errorwith its status. Nothing becomesauth_requiredthere, because the refusal is of the login being started, and "sign in again" doesn't fix it.Why
A Basecamp CLI user locked themselves out. Two containers held copies of one login. The second refresh revoked it, and a scheduled job then resent the revoked token every two minutes. bc3's abuse tracker escalated to a 4-hour block on every OAuth endpoint for that client and address, device authorization included. Through the SDK:
api_errorwith no wait, so the caller's only option was to try again into the block;device authorization failed with status 429, with no reason and no time.The CLI also detected
invalid_grantby matching"token error: invalid_grant"in the message. WithOAuthErrorit can match the code.Decisions
api_error, instead of failing at once.basecamp.Error, not a new error type. Callers already classify witherrors.As(*basecamp.Error). Extra fields keep that working and reach theAuthManagerpath, which can't importoauth.Testing
TestExchanger_Refresh_CarriesOAuthErrorAndRetryAfterandTestAuthManager_Refresh_CarriesOAuthErrorAndRetryAftergotapi_error/429withRetryAfter = 0and an emptyOAuthError.TestRequestDeviceAuthorization_CarriesOAuthErrorAndRetryAftergot the barestatus 429message. All pass now, along with the existing suites.TestExchanger_Refresh_BoundsOAuthErrorFields: a 10 KBerrororerror_descriptionis truncated in the typed fields as well.make checkpasses (macOS)Summary by cubic
A refused token refresh or device authorization in the Go SDK now carries the server's RFC 6749 error code, description, and Retry-After wait on
*basecamp.Error, so callers act on the server's own verdict instead of a bare status.rate_limitin the Exchanger andAuthManagerrefresh, matching Rust's classification.erroranderror_descriptioninto the typed fields and message; a 429 israte_limitwith its wait, everything else staysapi_error.ParseRetryAfterexports the client's Retry-After parser so theoauthpackage reads the header the same way.OAuthErrorandOAuthErrorDescriptionare bounded like the message; the composed message is bounded too.Written for commit 7e83bb2. Summary will update on new commits.