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
Revocation can use the wrong profile security policy, storage failures are misreported, and failed-revocation messaging understates refresh-token exposure.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds server-side OAuth credential revocation to logout, profile deletion, and the new auth revoke command.
Changes:
- Implements RFC 7009 revocation and issuer persistence.
- Adds revoke-aware CLI output and failure handling.
- Expands unit tests, command surface, skill docs, and smoke 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 |
|---|---|
skills/basecamp/SKILL.md |
Documents auth revoke. |
internal/commands/profile.go |
Revokes credentials during profile deletion. |
internal/commands/profile_test.go |
Tests profile deletion revocation. |
internal/commands/auth.go |
Adds revoke command and logout reporting. |
internal/commands/auth_logout_test.go |
Tests logout and revoke output. |
internal/auth/revoke.go |
Implements credential revocation workflows. |
internal/auth/revoke_test.go |
Tests revocation behavior and failures. |
internal/auth/keyring.go |
Persists authorization-server issuer. |
internal/auth/device_test.go |
Extends mock authorization server. |
internal/auth/auth.go |
Records issuer during login. |
internal/auth/auth_test.go |
Updates logout tests. |
e2e/smoke/smoke_lifecycle.bats |
Marks revoke outside smoke scope. |
.surface |
Registers the new CLI surface. |
Review details
Suppressed comments (1)
internal/auth/revoke.go:113
Store.Loadcan fail because credentials are malformed or because the backend failed, not only because the key is absent (keyring.go:137-147). Mapping every such error toErrNoCredentialmakesauth revokereport successful “Not logged in” while a credential may remain and no revocation was attempted. Distinguish a genuinely missing key and propagate other load failures.
creds, err := m.store.Load(credKey)
if err != nil {
return ErrNoCredential
- Files reviewed: 13/13 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.
`auth logout` and `profile delete` only deleted the local copy, so the refresh token and its access tokens stayed live on the server for anyone who had captured them — the smoke test noted that logout never revokes, even though the in-house authorization server advertises an RFC 7009 revocation endpoint for the public basecamp-cli client. Both now revoke first and delete regardless: the refresh token (which revokes its whole family) and then the access token are POSTed to the revocation endpoint named by the issuer's metadata, each request bounded to ten seconds, and a failure is reported rather than fatal — the access token expires within the hour on its own. Launchpad has no revocation endpoint and an imported personal access token is the operator's, not the CLI's, so those are forgotten locally and the summary says so. Logging out with nothing stored is "Not logged in", exit 0. `auth revoke` is the explicit verb for "kill this token": it revokes and then removes the credential, but unlike logout it keeps a credential it could not revoke so the revocation can be retried, and refuses Launchpad and imported tokens with the reason. The SDK's discovery drops revocation_endpoint, so the metadata fetch is a one-field GET here. Logins record the issuer on the credential; earlier credentials derive it from the token endpoint's origin, which Basecamp mounts under the issuer.
Review findings on the revoke-on-logout change: The revocation for `profile delete` rode the BC5 lane of the ACTIVE configuration, whose address policy derives from the active base URL — not the deleted profile's. Deleting a loopback development profile while production is active had the production lane refuse the loopback issuer, after which the credential was discarded unrevoked. The lane is now anchored on the deleted profile's own base URL (the operator's configuration, never a server-named URL), falling back to the active lane when they agree. A stored blob that is not a credential was deleted best-effort on the way to "Not logged in", hiding a failed delete. Store.Load now marks that case with ErrInvalidCredentials; the logout clears it and reports a failure to, and a missing key is reported without a delete. "it expires within the hour" was true of the access token only: an unrevoked refresh token stays valid until it is revoked, so the copy says so.
![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: 5253b75b3d
ℹ️ 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-store failures are misreported as absent credentials, and the command catalog omits the new revoke action.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
internal/auth/revoke.go:122
- This also maps every credential-store read failure to
ErrNoCredential. For example, an unavailable keyring can makeauth revokeclaimNot logged ineven though the credential remains and may still be valid; propagate non-missing load errors and reserveErrNoCredentialfor an actual absent entry.
creds, err := m.store.Load(credKey)
if err != nil {
return ErrNoCredential
- Files reviewed: 13/13 changed files
- Comments generated: 2
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
Second review round on revoke-on-logout: A store that could not be read — a locked keyring, an unreadable credentials file — was reported as "Not logged in", leaving a credential that may well be there live on both sides. Store.Load now marks a genuine miss with ErrNoCredential (go-keyring's ErrNotFound, or the file backend's one not-found phrase) and every other failure propagates; logout and revoke fail loudly on it instead. `auth logout` and `auth revoke` refuse while BASECAMP_TOKEN is set: the stored credential is not the session in use, and revoking it would kill an unrelated refresh family for good. A failed revocation now says what it left usable — the whole family when the refresh token was not accepted, only the access token after it was — instead of a fixed claim about the refresh token, and `auth revoke` keeps the failure's taxonomy so a 5xx or a transport failure stays retryable while a refusal does not. `revoke` joins the auth entry of the static command catalog.
There was a problem hiding this comment.
🟢 Approved
The revocation flow is complete, security-conscious, and thoroughly covered by focused tests.
Review details
- Files reviewed: 14/14 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: 9a48924ad1
ℹ️ 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 429 from the revocation endpoint surfaced as a generic API error (exit 7) rather than the CLI's rate-limit class (exit 5, Retry-After honoured), and the blanket 5xx-is-retryable rule marked a 507 account limit retryable when the rest of the CLI treats it as a verdict. The revocation failures now follow the same table.
There was a problem hiding this comment.
🟡 Changes recommended
Local-store failures can orphan credentials, retry guidance conflicts with error taxonomy, and revocation metadata lacks issuer validation.
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/commands/profile.go:381
errhere is a local store read/delete failure, not a best-effort server revocation failure (those are returned insideresult). Swallowing it and unregistering the profile can leave a live credential orphaned underprofile:<name>with no profile available to select and retry cleanup. Preserve the previous invariant by aborting profile deletion when local credential removal fails.
- Files reviewed: 14/14 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: 1f72054a69
ℹ️ 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".
Third review round on revoke-on-logout: The metadata fetch accepted any document the GET ended on. The lane client follows GET redirects, so another server's perfectly valid metadata could have named where both tokens were POSTed. The document's issuer must now equal the issuer it was fetched for, as login discovery already requires. `auth revoke` advertised a retry for every failure, including refusals its own classification marks final; the hint now offers a retry only when the error is retryable, and logout otherwise. `auth logout work` — a profile name where none is taken — was accepted and revoked the selected credential; the command takes no arguments now, like `auth revoke`. Deleting the active profile anchors its revocation on the effective configuration, environment overrides included, as at its login.
There was a problem hiding this comment.
🟡 Changes recommended
RFC 8414 path-based issuer discovery and direct terminal-warning sanitization must be corrected.
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/revoke.go:305
- This constructs the RFC 8414 metadata URL correctly only when the issuer has no path. Section 3 requires inserting
/.well-known/oauth-authorization-serverbetween the authority and an issuer path (for example,https://host/tenantmaps tohttps://host/.well-known/oauth-authorization-server/tenant), whereas this requestshttps://host/tenant/.well-known/.... Resource discovery does not restrictres.Issuerto an origin, so a valid path-based BC5 issuer can log in but can never be revoked; construct the well-known URL by parsing the issuer and inserting the path per RFC 8414.
- Files reviewed: 14/14 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: d576a10eeb
ℹ️ 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".
Fourth review round on revoke-on-logout: When a failed revocation leaves only the access token, the summary promised it "expires within the hour" — Basecamp's lifetime, not necessarily the one the server reported, which the CLI treats as non-expiring when absent. The summary now states the stored expiry: how long is left, that it has already passed, or that none was reported. The profile-delete warning writes the store's error text straight to the terminal, so it is scrubbed of control sequences first.
There was a problem hiding this comment.
🟡 Changes recommended
Invalid credentials are reported as absent, and some revocation outcomes use an incorrect lane or remaining-token description.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
internal/auth/revoke.go:122
- This branch has proved that a blob was stored but malformed. Returning
ErrNoCredentialafter deleting it makesauth logoutreportNot logged inand exit successfully, contrary to the contract that only a genuine miss gets that result; propagate the originalErrInvalidCredentialsafter successful cleanup so the corruption is reported.
internal/auth/revoke.go:257 - When both token fields are empty,
remainingis stillRemainingAccess, so logout emitsremaining: "access_token"and says that an access token remains even though this branch confirms none exists. A valid JSON blob such as{"oauth_type":"bc5"}reaches this path; represent no remaining token and avoid appending the outstanding-token suffix for that outcome.
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
The previous round special-cased the active profile to anchor its revocation on the effective configuration, so that a login made under a base-URL override could reach the issuer it came from. That opened the opposite hole: deleting an active loopback profile while a production override is in force put the revocation on the production lane, which refuses the loopback issuer, and the credential was then discarded unrevoked. One rule, written down: the anchor is the deleted profile's own saved base URL, the operator's configuration for that profile. A credential minted under a transient override is not recorded as such — the store must not choose its own egress policy — so that case reaches the summary as a failed revocation rather than a silent success.
There was a problem hiding this comment.
🔵 Needs a closer look
Near-expiry access tokens can be incorrectly reported as already expired because expiry is rounded before comparison.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/auth/revoke.go:89
- Rounding before testing expiry misclassifies any token with less than 30 seconds remaining as already expired (
Duration.Round(time.Minute)returns0s). Check the raw duration first, and round only when formatting a future expiry.
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
|
Review threads: 7 resolved (4 fixed, 3 declined with the reasoning in each thread). Declined:
|
What
auth logout(and thelogoutalias) revokes the credential with its authorization server (RFC 7009) before removing it locally: the refresh token first, which revokes its whole family, then the access token, as the publicbasecamp-cliclient. Local removal happens regardless of the server's answer.profile delete <name>takes the same revoke-then-delete path forprofile:<name>instead of calling the store directly, with the revocation's egress lane anchored on that profile's own base URL rather than the active configuration's.auth revoke [-P profile]: the explicit "kill this token" verb. It revokes and then removes the credential (a revoked refresh family leaves nothing usable), but unlike logout it keeps a credential it could not revoke so the revocation can be retried, and refuses Launchpad and imported tokens with the reason and a hint. ItsLongspells out the difference from logout.Logged out (token revoked);Logged out locally; could not revoke the token server-side: <reason> — the refresh token stays valid until it is revoked(or— only the access token remains and it expires in 42m0s/has already expired/reports no expiry, from the stored expiry, when the failure struck after the refresh token was accepted or there was none);Logged out (Launchpad tokens cannot be revoked from the CLI);Logged out (forgot the imported token; it stays valid until revoked in Basecamp). Nothing stored isNot logged in, exit 0. JSON keepsstatusand gainsrevoked(plusreasonandremainingwhen it is false).auth logoutandauth revokerefuse whileBASECAMP_TOKENis set: the stored credential is not the session in use, and revoking it would kill an unrelated refresh family for good.Not logged in; only a genuine miss does that, and an unreadable blob is cleared.retryable, a refusal does not, soauth revoke's "retry" hint and the JSONretryableflag agree.Manager.Revoke,Manager.Logout(ctx)returning aLogoutResult,Manager.LogoutCredential(ctx, key, baseURL), andManager.RevokeStored(ctx);Store.Loadmarks an unreadable blob withErrInvalidCredentialsso logout can clear it (and report a failure to) rather than mistake it for nothing stored;Credentialsgainsissuer, recorded at login. Credentials stored before that derive the issuer from the token endpoint's origin, which Basecamp mounts under the issuer.revocation_endpoint, so the metadata read is a small GET here: the document'sissuermust equal the issuer it was fetched for (a GET may follow redirects; another server's valid metadata must not name where the tokens go), the endpoint is validated like every other server-named endpoint, and both ride the BC5 lane client so the egress policy applies. Each request is bounded to 10 s.auth logouttakes no arguments (auth logout workused to revoke the selected credential);auth revokeadvertises a retry only when the failure is retryable..surfaceregenerated for the new command; one SKILL.md line under "Authentication errors";auth revokemarked out of scope in the smoke suite alongsideauth logout.Why
The smoke tests flagged "CLI auth logout / profile delete never revokes the server-side token (in-house issuer)": logout only deleted the local copy while the server advertised a revocation endpoint the CLI never called, leaving the refresh token and its access tokens live for anyone who had captured them. The operator also asked for the CLI to be able to revoke its own token explicitly, hence
auth revoke.Launchpad has no revocation endpoint, and an imported personal access token is the operator's (the same token lives in their secret store), so those are forgotten locally and the summary says so rather than pretending.
The companion fix for a refused
--expect-identitylogin leaving its freshly minted grant live is stacked on this branch as a separate PR.Before / After
Before:
After:
Borrowed from Codex
Codex's
logout_with_revokerevokes best-effort with a 10 s timeout and then always clears the local store, exiting 0 either way;auth logoutdoes the same, and additionally sends both tokens (the hint is advisory) and tells the user which of the four outcomes they got. Codex has no explicit revoke verb.Testing
internal/auth/revoke_test.go: the two POSTs and their form fields (no client secret), issuer derivation from the token endpoint, only-the-tokens-it-holds, metadata without an endpoint, unparseable/404 metadata, an insecure revocation endpoint refused before any POST, a 503 reported without the token in the message, the per-request timeout, logout deleting on success and on failure, Launchpad and imported tokens skipped,LogoutCredentialclearing only the named key, a login recording its issuer, andRevokeStoreddeleting on success / keeping the credential on refusal / refusing Launchpad and imported tokens.startDeviceASnow serves/oauth/revocationsand advertises it.internal/commands/auth_logout_test.go: the JSON shape and every human summary forauth logoutandauth revoke, includingNot logged inand the kept-credential path.profile_test.go: deleting a profile revokes its credential; a profile that never logged in revokes nothing.make fmt-check vet lint,make check-surface check-skill-drift check-smoke-coverage, andmake test-e2epass. Eleven pre-existing terminal-detection tests (appctx,cli,commands,stdinarg,tui/resolve) fail on this machine for a pristineorigin/maincheckout with the identical set — a pty quirk of the session, not the diff; the full suite was also run on a clean Linux host.