Conversation
github-actions
Bot
added
commands
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. |
There was a problem hiding this comment.
🟡 Changes recommended
Custom OAuth client mismatches can delete valid credentials, and profiled SDK failures recommend a login command that repairs the wrong credential.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Improves OAuth refresh failures by classifying rejected grants as authentication errors and providing login guidance.
Changes:
- Deletes rejected refresh credentials and returns
auth_required. - Adds default and profile-aware login hints.
- Expands refresh and 401 regression coverage.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
internal/output/errors.go |
Adds default authentication hints. |
internal/commands/projects.go |
Applies hints to converted SDK errors. |
internal/commands/projects_test.go |
Tests 401 JSON error hints. |
internal/auth/device_test.go |
Updates legacy-refresh assertions. |
internal/auth/auth.go |
Handles rejected refresh grants and profile hints. |
internal/auth/auth_test.go |
Adds refresh failure and metadata-preservation tests. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![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: 49e88e35c1
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
Credential deletion remains vulnerable to a cross-process race, and profile names are not shell-quoted in generated commands.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41d3e185a4
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
A 5xx invalid_grant can delete valid credentials, and some profile-scoped auth failures still emit a login command targeting the wrong credential.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/auth/auth.go:170
- The new profile-aware helper is still bypassed by auth errors returned from helper paths used during refresh. For example, a malformed stored token endpoint returns
output.ErrAuthfromrequireSecureOAuthEndpoint;Refresh/AccessTokenpass it through, andApp.withAuthRemedydeliberately does not rewrite it because it has no SDK cause. Under profilework, users are still told to run barebasecamp auth login, which repairs the wrong credential key. Normalize all auth errors leaving the manager tom.LoginHint()(and cover this refresh path), rather than only directm.errAuthcall sites.
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5dc62b082a
ℹ️ 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".
There was a problem hiding this comment.
🔵 Needs a closer look
Wrapped error context is lost, and some profiled refresh failures still emit an incorrect bare-login remedy.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
internal/auth/auth.go:170
errAuthis not applied to every auth failure returned byrefreshLocked. In particular, the errors fromrequireSecureOAuthEndpointandresolveClientCredentialsare returned directly; both can beoutput.ErrAuthvalues with the generic login hint and no SDK cause, soApp.withAuthRemedydeliberately skips them. Consequentlybasecamp auth refresh -P workcan still tell users to run a bare login that stores credentials under the wrong key. Route those refresh-path auth errors through the manager's profile-aware remedy as well (using a specific environment fix where appropriate).
internal/output/errors.go:40- Prioritizing the nested CLI error also discards context added by outer wrappers. For example,
resolvePersonIDswraps a resolver API failure asresolving %q: %w; that resolver error is an*output.Errorwhose cause is an SDK error. Previously the SDK branch rendered the full wrapped message, but this branch now returns only the inner SDK message. Preserveerr.Error()for a genuinely wrapped CLI error while retaining the inner CLI code, hint, status, and retryability.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22cde06e3b
ℹ️ 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".
There was a problem hiding this comment.
🔵 Needs a closer look
Some profile-mode auth failures still receive the wrong remedy, and two tests depend on ambient environment state.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
internal/auth/auth.go:170
- The profile-aware constructor is still bypassed by auth errors returned indirectly from
refreshLocked. For example,requireSecureOAuthEndpointatauth.go:352returnsoutput.ErrAuth, andApp.withAuthRemedyonly rewrites errors carrying abasecamp.Error; thereforeauth refresh -P workwith a malformed stored token endpoint still suggests barebasecamp auth login, which may repair a different credential. Route these credential-manager auth failures through the profile-aware remedy as well.
internal/auth/auth_test.go:1919 - This test is environment-dependent:
m.IsAuthenticated()returns true whenever an inheritedBASECAMP_TOKENis present, even after the stored credential is deleted. Clear the variable before exercising the refresh so the regression test reliably verifies the store state.
internal/commands/projects_test.go:204 - This test depends on the caller not having
BASECAMP_TOKENset.App.withAuthRemedychecks the process environment, so an inherited token changes the rendered hint to the environment-token remedy and makes the asserted generic hint fail. Clear the variable here, as the following profile test already does.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f18362701
ℹ️ 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".
A refresh the server refuses with invalid_grant means the refresh token is expired, revoked, or reused. The CLI surfaced it as api_error (exit 7) carrying the raw "token error: invalid_grant" string, and left the dead credential in the store, so every later command re-tried the same refresh. Basecamp's abuse tracker bans the client and address after a handful of invalid_grant failures, which turned one expired session into a lockout. Now the refusal is auth_required (exit 3) with the login to run, and the credential is deleted on the spot. Only invalid_grant deletes: invalid_request, network faults, and 5xx keep the credential and their existing classes. Detection matches the SDK's untyped error message; the coupling is named beside the constant. Every auth_required error also carries a remedy now. A 401 from the API arrives without a hint, so output.AsError and convertSDKError default one; the auth errors the credential manager raises name the active profile, since a bare login would store the new credential under the base URL instead. A regression test pins that a refresh preserves user_id, user_email, scope, and resource across the token rotation.
Review of the invalid_grant handling raised two ways an unconditional delete could discard a live credential: a Launchpad refresh sends whichever client the environment names, and the server answers invalid_grant for a token issued to another client too; and two processes can refresh at once, so the one refused for reusing the old token would delete the rotated credential the other had just saved. Only a BC5 credential is forgotten now — its client is the fixed public one — and only while the store still holds the refresh token that was refused. The generic "Run: basecamp auth login" hint was not actionable under an active profile (the login would store the credential under the base URL) or under BASECAMP_TOKEN (no login changes what requests send). The app boundary now replaces the generic hint with the profile-aware login command, or names the environment token. For that to survive rendering, output.AsError prefers an *Error already in the chain over the SDK error it wraps, which is also what every command's hand-built error expects.
|
Rebased onto main after #712 landed. Two conflicts, both additive: the new tests at the end of projects_test.go and the two early checks at the top of AsError in errors.go (the queued-gate verdict from #712 now runs first, then this PR's CLI-error verdict). No behaviour change; the suite on the rebased head fails only the TTY-detection tests that also fail on main in a non-terminal shell. |
There was a problem hiding this comment.
🟡 Changes recommended
Some profile-scoped refresh failures still emit a bare login command that targets the wrong credential key.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
There was a problem hiding this comment.
🟡 Changes recommended
The refresh hint wrapper does not replace the already-populated default hint, breaking profile-aware remedies and their new tests.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
The profile-aware login remedy was attached at chosen sites inside the refresh, so the failures it did not name — an unsafe stored token endpoint, a half-configured OAuth client, a lane client that could not be built — reached the reader with the bare "basecamp auth login", which under an active profile stores the new credential where the failing command never looks. The refresh now has one exit, and every auth-class error crossing it without a remedy is given the profile's login.
There was a problem hiding this comment.
🟢 Approved
The refresh handling, credential safeguards, error conversion, and relevant edge cases are comprehensively covered.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2af0b1d4f2
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. 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". |
…n over A Launchpad refresh sends whichever OAuth client the environment names, and the server answers invalid_grant for a token issued to a different client as well as for a dead grant. The credential is already kept in that case, but the message still said the session had expired, sending the reader to a new login when unsetting or correcting the client variables would have kept the one they had. The Launchpad message now names both readings.
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
There was a problem hiding this comment.
🔵 Needs a closer look
A new regression test can fail when the ambient BASECAMP_TOKEN is set.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/commands/projects_test.go:231
- This test inherits
BASECAMP_TOKENfrom the developer or CI environment. If it is non-empty,App.withAuthRemedyintentionally replaces the expected login hint with the environment-token remedy, so the assertion at line 249 fails. Clear the variable here as the neighboring profile test does.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
|
@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 threads: 3 resolved (3 fixed, 0 declined).
The Race Detection failure on 5180eac was |
What
invalid_grant(refresh token expired, revoked, or reused) is now anauth_requirederror (exit 3) that says what to run, instead of a genericapi_error(exit 7) carrying the rawtoken error: invalid_grant - …string.invalid_grantfailures in 48 hours, and until now every later command re-tried the same dead refresh token, so one expired session could turn into a lockout. Onlyinvalid_grantdeletes:invalid_request, network faults and 5xx keep the credential and their existing error classes.auth_requirederror that reaches the user carries a remedy.output.AsErrorandconvertSDKErrornow default an empty hint toRun: basecamp auth login(a 401 from the API arrives with none), and every auth error the credential manager raises names the active profile:Run: basecamp auth login -P <profile>.auth refreshpreservesuser_id,user_email,scopeandresourceacross a token rotation.The
invalid_grantdetection matches the SDK's untyped error message (token error: invalid_grant); the SDK's exchanger returns token-endpoint errors as plainfmt.Errorfvalues, so the OAuth error code is recoverable only from the text. That coupling is named in a comment next to the constant; a re-pin that types the error can replace the match.Why
Smoke-test finding "Refresh with a rejected refresh token exits 7 api_error instead of 3 auth_required": after a refresh token is revoked server-side,
basecamp auth refreshand every ordinary command failed with exit 7, no hint, and the credential left on disk to fail again next time. Scripts keyed on exit 3 to trigger a re-login never saw it, and a person saw an OAuth wire string with nothing to do about it.Before / After
Before:
After:
A 401 on an ordinary command:
Borrowed from Codex
Codex maps refresh failures to plain sentences ("Your access token could not be refreshed because your refresh token has expired. Please log out and sign in again."); the wording here does the same and adds the exact command, profile included.
Testing
internal/auth:TestRefresh_InvalidGrantForgetsTheCredential,TestRefresh_InvalidGrantWithoutDescription,TestRefresh_InvalidRequestKeepsTheCredential,TestAccessToken_InvalidGrantHintsTheProfile,TestRefresh_PreservesIdentityAndBinding(httptest token endpoints; no network).internal/commands:TestProjectsList401CarriesTheLoginHint(401 transport, hint asserted on the converted error and in the rendered JSON envelope).TestRefreshLocked_LegacyBC3RequiresReauthnow asserts the typed code and hint instead of the old inline "re-authenticate" wording.make fmt-check vet lint,go test -tags dev ./internal/...,make check-surface check-skill-drift,make check.Summary by cubic
Refresh refused with
invalid_grantis now anauth_requirederror (exit 3) that says the session expired and what to run, instead of a genericapi_error(exit 7); the refused credential is deleted on the spot so later commands stop re-trying it and one expired session can't accumulate into a Basecamp abuse-tracker lockout. The delete only touches a BC5 credential while the store still holds the exact refused token, so a concurrent rotation's fresh credential is kept and the refresh succeeds by proxy — all other refusals keep the credential and their existing error classes.Bug Fixes
Run: basecamp auth login -P <profile>), or aBASECAMP_TOKENnote when set; stored-credential commands that ignore the env token keep their profile-named hint.user_id,user_email,scope, andresource.BASECAMP_OAUTH_CLIENT_ID/SECRETnaming a different client — instead of declaring the session over.invalid_grantdetection matches the SDK's untyped error message, documented in a comment so a later typed SDK error can replace the string match.Written for commit d2693fc. Summary will update on new commits.