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. |
There was a problem hiding this comment.
🟡 Changes recommended
The revocation-failure warning is misleading, and Launchpad refusal coverage is missing.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds best-effort revocation for grants rejected by login verification while preserving the original refusal error.
Changes:
- Cleans up refused device-flow grants and safely no-ops for Launchpad.
- Warns when revocation fails.
- Adds device-flow revocation and warning tests.
Review findings:
- Moderate (1 vote) —
internal/auth/revoke.go:140: The warning may falsely claim the credential expires within an hour when refresh revocation fails or expiry is absent. - Nit (2 votes) —
internal/auth/auth.go:615: Add coverage for Launchpad refusal cleanup and no-op behavior.
File summaries
| File | Description |
|---|---|
internal/auth/revoke.go |
Implements refused-grant cleanup and warning handling. |
internal/auth/revoke_test.go |
Tests successful and failed device-flow cleanup. |
internal/auth/auth.go |
Invokes cleanup after verification failures. |
Review details
Suppressed comments (1)
internal/auth/revoke.go:140
Revokestops after a failed refresh-token POST (revoke.go:181-190), so the refresh token can remain valid indefinitely;Credentials.ExpiresAtcan also be zero when the server omits an expiry. This fixed “it expires within the hour” claim is therefore false or misleading on a revocation failure. Report that the credential may remain valid until revoked (or distinguish which token failed), and update the warning test accordingly.
log(richtext.SanitizeSingleLine("warning: could not revoke the refused credential server-side: " + result.Err.Error() + " — it expires within the hour"))
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
💡 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: 8cf1f3fe92
ℹ️ 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
Cleanup can fail when the verification context is canceled, and the revocation warning can be inaccurate.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
There was a problem hiding this comment.
🟢 Approved
The reviewed changes are covered by tests and have no unresolved blocking issues.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
There was a problem hiding this comment.
🟢 Approved
The reviewed changes and test coverage address the refused-login cleanup paths without unresolved blocking issues.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
There was a problem hiding this comment.
🟢 Approved
No unresolved review comments remain, and the cleanup paths are covered by tests.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
A login refused by its Verify hook — an --expect-identity mismatch above all — correctly stored nothing, but the full-scope refresh and access tokens the server had just issued stayed live with no local record of them: an orphan grant only the server's session list could find. The smoke test flagged it as a live orphan grant needing operator action. The refused credential is now revoked before the refusal is returned, on both the device and Launchpad flows (a no-op on Launchpad, which has no revocation endpoint). The refusal stays the error the caller sees; a failed revocation adds one warning line so the operator knows a live token is out there until it expires.
|
Review threads: 2 resolved (2 fixed, 0 declined with the reasoning in each thread). |
What
Verifyhook — an--expect-identitymismatch, an unreportable identity, an out-of-range expiry — now revokes the grant it just minted before returning the refusal, on both the device flow and the Launchpad flow (where revocation is a no-op: Launchpad has no revocation endpoint).context.WithoutCancel): aVerifythat failed because the login was cancelled or timed out is exactly the case that would otherwise orphan the grant, and the per-request timeouts still bound the calls.Manager.Revoke/revokeForDiscard/LogoutResult.Outstandingfrom Revoke the credential with the server on logout, profile delete and auth revoke #713; this PR adds onlydiscardGrantand the two call sites.Stacked on #713 (
authux-4); the base moves tomainonce that merges.Why
The smoke test "expect-identity mismatch discards a freshly minted grant without revoking it (live orphan grant, operator action)": the login correctly stored nothing, but the full-scope refresh token and access token the server had just issued stayed live with no local record of them — an orphan grant only the server's session list could find. A refused login should leave nothing behind, locally or server-side.
Before / After
Before:
After:
Borrowed from Codex
Nothing here — Codex has no pre-store identity check, so no equivalent path exists.
Testing
internal/auth/revoke_test.go:TestLoginDevice_VerifyFailureRevokesTheGrant(both tokens reach the mock revocation endpoint, nothing stored, no warning),TestLoginDevice_VerifyFailureWarnsWhenTheGrantOutlivesIt(a 500 from the endpoint produces the one warning line, without the token in it, and the refusal remains the error),TestLoginDevice_VerifyFailureRevokesAfterCancellation(a Verify that cancels the login context still gets the grant revoked), andTestLoginLaunchpad_VerifyFailureRevokesNothing(a refused Launchpad login stores nothing, sends no revocation request, prints no warning).make fmt-check vet lint,make check-surface check-skill-drift, and the full Go suite (on a clean Linux host; this machine has a pre-existing pty quirk that fails eleven terminal-detection tests onorigin/maintoo).