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. |
github-actions
Bot
added
commands
There was a problem hiding this comment.
🟡 Changes recommended
Server checks can return stale credential state, and direct terminal rendering permits unsanitized values.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Expands auth status into an identity and credential-health report with optional server validation.
Changes:
- Adds detailed JSON and terminal authentication status.
- Adds
--checkand comprehensive tests. - Updates CLI surface and authentication documentation.
[!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/commands/auth.go |
Implements status reporting and server checks. |
internal/commands/auth_status_test.go |
Tests status output and validation. |
skills/basecamp/SKILL.md |
Documents offline and checked status modes. |
install.md |
Updates expected login verification output. |
.surface |
Registers the new --check flag. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: 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: ef3ac7a597
ℹ️ 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
Generated login remedies do not safely quote configured profile names.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
A non-refreshable token inside the refresh window is marked expired in JSON but described as still expiring in terminal output.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Markdown status output can incorrectly contain ANSI styling, and two user-facing descriptions overstate the implemented output.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2b3376cb1
ℹ️ 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
User-facing help incorrectly says the stored credential is checked when an environment token may take precedence.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
`auth status` answered "Authenticated (scope: full)" plus a few keys: no identity, account, server, storage backend, or expiry timestamp, and no remedy when nothing was stored. Finding out which account a shell addressed as whom meant `me` (a network round-trip) or reading the credential file. The JSON data gains base_url, account_id, user_email, expires_at, refreshable, and storage, keeping every existing key. A terminal gets prose: a "Logged in to <server> as <email> (user <id>)" headline, then profile, account, access, source, token expiry, and storage; an expired token that cannot refresh names the login to run. Not logged in keeps exit 0 and authenticated:false for scripts, with the login command as the hint. Terminal versus envelope follows the output writer's effective format, so a piped status stays JSON. --check makes one authenticated request, the authorization lookup `me` makes, and reports whether the server still accepts the token as "valid"; a 401 is the rejected verdict (still exit 0), a fault that is no verdict is returned as itself. Without the flag nothing is fetched.
Review of the status command found the report built before --check's request, so a refresh (or a forgotten dead credential) the request caused was not what the fields described; the check now goes first and the report is built from what is stored afterwards. The login remedy moves from a breadcrumb, which --hints controls, to the envelope's notice, and a rejected BASECAMP_TOKEN is told to unset or replace the variable rather than to log in. The terminal path sanitizes every line at the sink, honors --styled when redirected, and prints the --stats footer. A token with nothing to refresh with is reported expired once inside the refresh window, where every command already refuses it; the BASECAMP_TOKEN branch reports refreshable: false; and a bc_at_ environment token resolves the authorization document at the resource origin, as a stored BC5 credential does.
…eral The refresh-window line still read "expires in" while the field said expired; it now says expired and explains why. Markdown output goes through the envelope renderer again so --md stays literal on a terminal. The help text and install guide describe the output as it is.
The terminal path dropped every Fprintln error, so a full disk could leave partial output and exit 0; the first write failure is returned now, as the envelope path does. The --check help and the skill said the stored token was checked, but BASECAMP_TOKEN takes precedence, so they now say the active token.
With no environment token and no stored credential the check was skipped and the output carried no valid field, indistinguishable from status without --check. It now answers valid: false and says there is no token to check, without claiming a request was made.
--check produces the token before the request, so a credential with nothing to refresh with inside the refresh window, or a refresh the token endpoint refused, is reported as a token that could not be sent rather than as the server's rejection; a refused refresh also replaces the offline line's promise to refresh on next use. A refresh token counts as refreshable only where a refresh can run: not for the removed bc3 flow, and not for a BC5 credential without its token endpoint.
The envelope stores the summary as given and sanitizes at terminal sinks, so sanitizing the email while building it left JSON output with an altered value; the sanitizer now only decides whether there is an email to name. A legacy bc3 credential, or a BC5 one without its token endpoint, holds a refresh token the CLI will not redeem; inside the refresh window the report called that "no refresh token". It now names the actual reason.
The --check verdict replaces the offline promise for every auth-class failure the token lookup hits, and a half-configured OAuth client fails before any request reaches the token endpoint; the line after names the actual failure.
The status report's rule for whether a stored refresh token counts lived apart from the refresh itself, and missed one of its pre-request refusals: a persisted token endpoint the CLI will not post to. The rule now lives in package auth beside the refresh, with that check included.
The status report kept its own list of the checks a refresh makes before sending anything, and each review found one it lacked; the latest was an OAuth client half-configured in the environment. The refresh's pre-request half is now its own step, and the report runs that step on a copy of the credential, so the two cannot disagree.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f398338908
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2293e38118
ℹ️ 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
--check can misreport a local refresh failure as a server rejection because it resolves the access token twice.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91e9c70968
ℹ️ 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".
| Nothing is fetched unless --check is given, which makes one authenticated | ||
| request (the same authorization lookup "basecamp me" makes) and reports | ||
| whether the server accepts the token the CLI would send — BASECAMP_TOKEN | ||
| when it is set, otherwise the stored login: "valid" in the JSON data. |
| status["expires_in"] = expiresIn.Round(time.Second).String() | ||
| status["expired"] = expiresIn < 0 | ||
| } | ||
| cmd.Flags().BoolVar(&check, "check", false, "Ask the server whether the active token (BASECAMP_TOKEN, else the stored login) is accepted (one authenticated request)") |
| @@ -57,87 +57,378 @@ func newAuthLogoutCmd() *cobra.Command { | |||
| } | |||
|
|
|||
| func newAuthStatusCmd() *cobra.Command { | |||
| // credential, or forget a dead one, and the report must describe | ||
| // what is stored afterwards. | ||
| var verdict *checkVerdict | ||
| if check && (os.Getenv("BASECAMP_TOKEN") != "" || app.Auth.IsAuthenticated()) { |
| // checkVerdict is the server's answer to --check: whether it accepted the | ||
| // token the CLI would send right now. | ||
| type checkVerdict struct { |
| s.details = append(s.details, "Token: none could be sent ("+v.reason+")") | ||
| } | ||
| switch { | ||
| case os.Getenv("BASECAMP_TOKEN") != "": |
| // authStatus is what `auth status` learned about the active credential: | ||
| // the envelope data, and the same facts as prose for a terminal. | ||
| type authStatus struct { |
| // authStatusReport inspects the active credential without touching the | ||
| // network. A BASECAMP_TOKEN session never reaches the credential store, so | ||
| // it neither pays the keyring probe nor reports another credential's | ||
| // identity as its own. | ||
| func authStatusReport(app *appctx.App) (*authStatus, error) { |
What
basecamp auth statusbecomes the "who am I" surface.JSON data adds
base_url,account_id(when configured),user_email(when stored),expires_at(RFC 3339 UTC),refreshable(the next command's refresh would be sent: the checks a refresh makes before any request — a stored refresh token, not a legacy bc3 grant, a token endpoint the CLI will post to, an OAuth client fully configured — pass; otherwisefalse) andstorage(keyring|file|env), and keeps every existing key (authenticated,source,oauth_type,scope,profile,user_id,expires_in,expired).On a terminal the output is prose instead of a key/value dump:
Unknown segments are omitted. An expired token that can refresh says "expired, will refresh on next use"; one that cannot says "expired" and adds
Run: basecamp auth login [-P <profile>]. A token that cannot refresh is reported expired once it is inside the five-minute refresh window, which is when every command starts refusing it, and the detail line gives the refusal the next command's refresh would make (no refresh token, a legacy bc3 grant, a missing or unusable token endpoint, a half-configured BASECAMP_OAUTH_CLIENT_ID/SECRET).Not logged in keeps exit 0 and
authenticated: false(scripts key on both), but the summary becomes "Not logged in to <base_url>" with the login command as a hint line (the envelope'snoticefield in JSON, which does not depend on--hints).New
--checkflag: one authenticated request — the authorization lookupbasecamp memakes — reporting whether the server still accepts the token the CLI would send. The request goes first and the report is built from what is stored afterwards, so a refresh it triggers (or a dead credential it forgets) is what the fields describe. JSON gainsvalid: true|false; the terminal shows "Token: valid (checked just now)" or "Token: rejected by the server" with the remedy: the login, or, underBASECAMP_TOKEN, to unset or replace the variable since a login cannot change what requests send. A 401 (or a credential that cannot be refreshed) is the "rejected" verdict and still exits 0; a fault that is not a verdict (server unreachable, 5xx) is returned as itself. Without--checknothing is fetched, as before.Terminal vs envelope is decided by the output writer's effective format (
--json, config, or a pipe all get the envelope), sobasecamp auth status | jqkeeps working. Terminal lines are sanitized to single control-free lines at the sink,--styledkeeps styling when redirected, and--statsprints its footer.A
bc_at_BASECAMP_TOKENnow resolves the authorization document at the resource origin (as a stored BC5 credential does) instead of under a pathful base URL, which--checkandmeboth depend on.Based on #711, which provides the profile-aware login hint the status output reuses; merge that first.
Why
auth statussaid "Authenticated (scope: full)" and a handful of keys: no identity, no account, no server, no storage backend, no expiry timestamp, and no remedy when not authenticated. Every "which account is this shell talking to as whom" question neededme(a network round-trip) or a look inside the credential file.--checkanswers the follow-up — "and does the server still take this token?" — without running a real command to find out.Before / After
Before:
After:
Borrowed from Codex
codex login statusis one line ("Logged in using ChatGPT" / "Not logged in"); the headline here follows that shape and adds the who/where/how a CLI with profiles and accounts needs.Testing
internal/commands/auth_status_test.go: full bc5 credential (every JSON field and the detail lines), expired-but-refreshable, expired imported token (hint andloginbreadcrumb), not logged in (exit 0,authenticated: false, summary, breadcrumb),BASECAMP_TOKEN(storage: env, token never printed), terminal rendering (exact lines),coarseDuration, and--checkaccepted / rejected / server fault / no request without the flag — all against the existing httptest identity server.make update-surfacefor the new flag;.surfacecommitted.install.md's expected status line and the skill's "Authentication errors" block updated.make fmt-check vet lint,go test -tags dev ./internal/...,make check-surface check-skill-drift, and the rest ofmake check(e2e, naming, lockstep, tidy).