feat(profiles): add 'profiles list' and 'profiles use' - #6
Conversation
THE missing affordance from the stale-token incident: with one expired profile active, every command failed and discovering that other (healthy) profiles existed required dumping the macOS keychain with security+awk. profiles list: every stored profile in one command - active marker, token presence, and the account email each profile holds (from a new non-secret identity cache; emails are re-verified live and re-cached by --check). --check answers 'which of my accounts still work?' with ok / expired or revoked / error per profile, one API round-trip each. profiles use <profile>: makes the active binding deliberate and visible instead of a hand-edit of config.yml. Accepts a bare profile or this CLI's full ref (cross-service refs rejected). Switching never touches tokens; a missing token warns loudly (naming existing profiles, so typos surface) but proceeds - 'use work' then 'init' is the add-second-account flow. Mechanism: cli-common v0.5.0's credstore.ListProfiles, wrapped as keychain.ListProfiles + HasTokenFor (cross-profile presence); auth.GetHTTPClientForRef + gmail.NewClientForRef bind a client to an explicit ref so --check can probe non-active profiles; identitycache is a cache-dir sidecar (an email is not an access secret, and the keyring allowlist stays exactly [oauth_token]).
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: f7a9080a20da
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| policies:conventions | 0 |
| structure:repo-health | 3 |
go:implementation-tests (1 finding)
Minor - profilescmd/profiles.go:196
checkProfile(ctx, profile, ref, present bool, row *profileRow) mutates its caller's row via a pointer (setting row.Email/row.VerifiedAt as a side effect) instead of returning the values it computes; the health string is returned but the email/verified-at fields are written invisibly. This is the one spot in the new command code that relies on a mutable out-param instead of value composition (contrast with the rest of runList, which builds each profileRow by value). It works and is covered by TestRunList_Check, but it makes the function's real contract (three outputs: health, email, verifiedAt) implicit, and a future edit to runList's loop that reorders or reuses
rowrisks silently dropping the mutation. Prefer returning a small result, e.g.func checkProfile(ctx, profile, ref string, present bool) (health, email string, verifiedAt time.Time), and have the caller assign all three fields explicitly in runList.
structure:repo-health (3 findings)
Major - identitycache/identitycache.go:65
identitycache.Put hand-rolls its own temp-file->chmod->rename atomic write (lines 72-102) instead of reusing the repo's one established cache-dir seam: the
cachepackage, which already wraps cli-common/cache's generic WriteResource/ReadResource envelope (atomic write, corrupt-as-miss, versioned) for exactly this kind of per-CLI JSON state under the OS cache dir (see cache/cache.go SetDrives/GetDrives). The PR intent even cites config.SaveConfig as its precedent but doesn't explain why the closer analogue, the cache package, wasn't extended instead. Two independent atomic-write-to-cache-dir implementations now exist with no shared invariant enforcing that future cache-dir writers use the same envelope; the next such package is more likely to copy identitycache's ad hoc version than clicache's. Fix: either implement identitycache's Load/Put via clicache.ReadResource[map[string]Entry]/WriteResource on a Locator (dropping the manual temp-file dance), or add a one-line note in identitycache.go explaining why the envelope's TTL/staleness model doesn't fit this never-auto-expiring data.
Minor - profilescmd/profiles.go:1
README.md's Layout table is the only versioned package map in this repo and enumerates every existing package by responsibility (config/keychain/auth, gmail/calendar/..., mailcmd/initcmd/configcmd/setcred/refreshcmd/rootutil, etc.), but this PR adds two new packages, identitycache and profilescmd, without a corresponding row. Future contributors/agents scanning README.md for 'where does X live' won't discover either package. Fix: add identitycache to the support-utilities row (with cache) and profilescmd to the cobra-command-packages row.
Nits - gmail/client.go:46
NewClientForRef (lines 46-61) duplicates NewClient (lines 26-41) verbatim except for the auth call used to obtain the HTTP client. As more *ForRef variants get added elsewhere in this pattern, the gmail.Service construction and Client struct literal will drift out of sync between the two paths. Fix: factor the shared
option.WithHTTPClient(client)+ Client construction into a small helper both functions call.
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | complete_broad | auth/auth.go, gmail/client.go, go.mod, go.sum, identitycache/identitycache.go, identitycache/identitycache_test.go, keychain/keychain.go, keychain/profiles_test.go, profilescmd/profiles.go, profilescmd/profiles_test.go | unavailable | unavailable |
| policies:conventions | incomplete_failed | unavailable | unavailable | structured output invalid after retry: first: llm: constraints entry length out of bounds; second: llm: constraints entry length out of bounds |
| structure:repo-health | complete_broad | auth/auth.go, gmail/client.go, identitycache/identitycache.go, identitycache/identitycache_test.go, keychain/keychain.go, profilescmd/profiles.go | unavailable | docs/ referenced by AGENTS.md is absent from this checkout; reviewed against README.md's package table instead. |
Reviewer Diagnostics
| Reviewer | Status | Diagnostic |
|---|---|---|
| policies:conventions | failed | structured output invalid after retry: first: llm: constraints entry length out of bounds; second: llm: constraints entry length out of bounds |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 6m 54s | $3.40 | claude-sonnet-5 | cr 0.10.268
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 6m 54s wall · 14m 04s compute |
| Cost | $3.40 |
| Tokens | 194 in / 42.7k out |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-5 | 6 | 2.7k | 58.0k | 14.9k | $0.15 | 31s |
| go:implementation-tests | claude-sonnet-5 | 56 | 12.6k | 1.4M | 64.9k | $1.00 | 2m 53s |
| policies:conventions | claude-sonnet-5 | 76 | 16.4k | 2.0M | 64.9k | $1.22 | 5m 59s |
| structure:repo-health | claude-sonnet-5 | 50 | 10.3k | 1.2M | 61.1k | $0.88 | 4m 26s |
| orchestrator-rollup | claude-sonnet-5 | 6 | 707 | 74.4k | 20.0k | $0.15 | 14s |
| // checkProfile classifies one profile's live token state. The email learned | ||
| // from a healthy check refreshes the identity cache (best-effort) and the | ||
| // row, so --check is also how a listing heals missing/stale emails. | ||
| func checkProfile(ctx context.Context, profile, ref string, present bool, row *profileRow) string { |
There was a problem hiding this comment.
checkProfile(ctx, profile, ref, present bool, row *profileRow) mutates its caller's row via a pointer (setting row.Email/row.VerifiedAt as a side effect) instead of returning the values it computes; the health string is returned but the email/verified-at fields are written invisibly. This is the one spot in the new command code that relies on a mutable out-param instead of value composition (contrast with the rest of runList, which builds each profileRow by value). It works and is covered by TestRunList_Check, but it makes the function's real contract (three outputs: health, email, verifiedAt) implicit, and a future edit to runList's loop that reorders or reuses row risks silently dropping the mutation. Prefer returning a small result, e.g. func checkProfile(ctx, profile, ref string, present bool) (health, email string, verifiedAt time.Time), and have the caller assign all three fields explicitly in runList.
Reply inline to this comment.
| // racy across concurrent processes in principle, but the value is a | ||
| // re-derivable cache: the loser of a race costs one future re-verification, | ||
| // nothing more. | ||
| func Put(profile, email string) error { |
There was a problem hiding this comment.
identitycache.Put hand-rolls its own temp-file->chmod->rename atomic write (lines 72-102) instead of reusing the repo's one established cache-dir seam: the cache package, which already wraps cli-common/cache's generic WriteResource/ReadResource envelope (atomic write, corrupt-as-miss, versioned) for exactly this kind of per-CLI JSON state under the OS cache dir (see cache/cache.go SetDrives/GetDrives). The PR intent even cites config.SaveConfig as its precedent but doesn't explain why the closer analogue, the cache package, wasn't extended instead. Two independent atomic-write-to-cache-dir implementations now exist with no shared invariant enforcing that future cache-dir writers use the same envelope; the next such package is more likely to copy identitycache's ad hoc version than clicache's. Fix: either implement identitycache's Load/Put via clicache.ReadResource[map[string]Entry]/WriteResource on a Locator (dropping the manual temp-file dance), or add a one-line note in identitycache.go explaining why the envelope's TTL/staleness model doesn't fit this never-auto-expiring data.
Reply inline to this comment.
| // NewClientForRef is NewClient bound to an explicit credential ref instead of | ||
| // the active one. It exists for `profiles list --check`, which live-verifies | ||
| // every stored profile's token in one process. | ||
| func NewClientForRef(ctx context.Context, ref string) (*Client, error) { |
There was a problem hiding this comment.
NewClientForRef (lines 46-61) duplicates NewClient (lines 26-41) verbatim except for the auth call used to obtain the HTTP client. As more *ForRef variants get added elsewhere in this pattern, the gmail.Service construction and Client struct literal will drift out of sync between the two paths. Fix: factor the shared option.WithHTTPClient(client) + Client construction into a small helper both functions call.
Reply inline to this comment.
| @@ -0,0 +1,334 @@ | |||
| // Package profilescmd implements the profiles command group: the multi- | |||
There was a problem hiding this comment.
File-level note: profilescmd/profiles.go
README.md's Layout table is the only versioned package map in this repo and enumerates every existing package by responsibility (config/keychain/auth, gmail/calendar/..., mailcmd/initcmd/configcmd/setcred/refreshcmd/rootutil, etc.), but this PR adds two new packages, identitycache and profilescmd, without a corresponding row. Future contributors/agents scanning README.md for 'where does X live' won't discover either package. Fix: add identitycache to the support-utilities row (with cache) and profilescmd to the cobra-command-packages row.
Reply inline to this comment.
…n checkProfile; dedupe gmail construction; README package rows
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 64704f17e632
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 2 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
go:implementation-tests (2 findings)
Minor - profilescmd/profiles.go:250
runUse manually calls
_ = st.Close()before every one of its four early-return error paths (lines 250, 255, 263, 274) instead ofdefer func() { _ = st.Close() }()right after the successful open, which is exactly the pattern runList uses two functions above it (line 109) for the same Store. The manual-close style is more brittle: a future edit that adds another early return in runUse (there are already four) can easily forget the close call, leaking the underlying credstore handle, whereas a defer placed once cannot be missed. Align runUse with runList's own established seam in this file — open,deferthe close immediately, then let every return fall through it.
Minor - profilescmd/profiles.go:304
The env-override warning in runUse (
if env := os.Getenv(keychain.CredentialRefEnvVar()); env != "" && env != ref { ... }) is real, non-trivial behavior — it reads an environment variable and conditionally prints a warning that the switch is about to be shadowed — but profiles_test.go has no test exercising it (no test sets the<SERVICE>_CREDENTIAL_REFenv var around arunUsecall). This is the kind of branch that regresses silently: a refactor ofkeychain.CredentialRefEnvVar()or the comparison logic would not be caught by the existing suite. Add a test that sets the env var to a different ref before calling runUse and asserts the warning appears (and one where env equals ref and the warning is absent).
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | complete_broad | auth/auth.go, gmail/client.go, go.mod, go.sum, identitycache/identitycache.go, identitycache/identitycache_test.go, keychain/keychain.go, keychain/profiles_test.go, profilescmd/profiles.go, profilescmd/profiles_test.go | unavailable | git history/diff tooling was unavailable in this environment (git commands were blocked), so the review reads the full contents of each assigned file at HEAD rather than a line-level diff; findings are anchored using the visible file content. |
| policies:conventions | complete_broad | README.md, auth/auth.go, go.mod, go.sum, keychain/keychain.go, profilescmd/profiles.go, profilescmd/profiles_test.go | unavailable | cli-common's shared docs (github.com/open-cli-collective/cli-common/tree/main/docs) were not present in the review context, so section references like §1.5.2/§1.6/§1.8 cited in code comments could not be checked against source-of-truth text and were treated as given. |
| structure:repo-health | complete_broad | auth/auth.go, gmail/client.go, identitycache/identitycache.go, identitycache/identitycache_test.go, keychain/keychain.go, profilescmd/profiles.go | unavailable | AGENTS.md points to docs/development.md and docs/README.md, but no docs/ directory exists in this checkout; AGENTS.md is not part of this diff's assigned files, so this is noted but not raised as a finding. |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 4m 03s | $2.91 | claude-sonnet-5 | cr 0.10.268
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 4m 03s wall · 8m 58s compute |
| Cost | $2.91 |
| Tokens | 150 in / 30.8k out |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-5 | 6 | 2.2k | 89.7k | 29.9k | $0.24 | 26s |
| go:implementation-tests | claude-sonnet-5 | 50 | 11.7k | 1.2M | 64.8k | $0.93 | 2m 49s |
| policies:conventions | claude-sonnet-5 | 38 | 6.0k | 728.1k | 58.9k | $0.66 | 2m 59s |
| structure:repo-health | claude-sonnet-5 | 50 | 10.4k | 1.1M | 57.6k | $0.83 | 2m 31s |
| orchestrator-rollup | claude-sonnet-5 | 6 | 465 | 103.5k | 34.0k | $0.24 | 10s |
| if strings.Contains(arg, "/") { | ||
| svc, p, perr := credstore.ParseRef(arg) | ||
| if perr != nil { | ||
| _ = st.Close() |
There was a problem hiding this comment.
runUse manually calls _ = st.Close() before every one of its four early-return error paths (lines 250, 255, 263, 274) instead of defer func() { _ = st.Close() }() right after the successful open, which is exactly the pattern runList uses two functions above it (line 109) for the same Store. The manual-close style is more brittle: a future edit that adds another early return in runUse (there are already four) can easily forget the close call, leaking the underlying credstore handle, whereas a defer placed once cannot be missed. Align runUse with runList's own established seam in this file — open, defer the close immediately, then let every return fall through it.
Reply inline to this comment.
| } | ||
| fmt.Printf("Active profile is now %s.\n", ref) | ||
|
|
||
| if env := os.Getenv(keychain.CredentialRefEnvVar()); env != "" && env != ref { |
There was a problem hiding this comment.
The env-override warning in runUse (if env := os.Getenv(keychain.CredentialRefEnvVar()); env != "" && env != ref { ... }) is real, non-trivial behavior — it reads an environment variable and conditionally prints a warning that the switch is about to be shadowed — but profiles_test.go has no test exercising it (no test sets the <SERVICE>_CREDENTIAL_REF env var around a runUse call). This is the kind of branch that regresses silently: a refactor of keychain.CredentialRefEnvVar() or the comparison logic would not be caught by the existing suite. Add a test that sets the env var to a different ref before calling runUse and asserts the warning appears (and one where env equals ref and the warning is absent).
Reply inline to this comment.
…mail list (#175) ## Why Delivers the credential/profile UX overhaul to gro by bumping **google-cli-common v0.2.0 → v0.3.0** ([#4](open-cli-collective/google-cli-common#4), [#5](open-cli-collective/google-cli-common#5), [#6](open-cli-collective/google-cli-common#6), [#7](open-cli-collective/google-cli-common#7)) and registering the new `profiles` command group. Driven by a first-hand incident: the active profile's token went stale, every command failed with a bare `oauth2: "invalid_grant"`, and it read as "gro is dead" — when other profiles were fine and there was no way to list them short of dumping the macOS keychain. ## What gro users get - **`gro profiles list [--check] [--json]`** — every stored profile, the account email it holds, an active marker with *where* the selection came from, and (with `--check`) per-profile live token health: `ok` / `expired or revoked` / `error`. - **`gro profiles use <profile>`** — deliberate, visible switching of the active binding. - **Attributed auth errors** — verified live against Google's token endpoint: ``` credential google-readonly/work (selected via config.yml credential_ref) can no longer authenticate: oauth2: "invalid_grant" "Bad Request"; other profiles may be unaffected - run 'gro profiles list' to check them, or 'gro init' to re-authenticate this one ``` - **`gro init`** announces which profile/account it will (re)authenticate before any prompt or write, and `gro init --profile <name>` adds a NEW account without touching the active profile's token. - **`gro mail list`** — the audit of item 6 found no `--max` divergence among siblings; the real gap was the missing `list` command (cobra's `unknown flag: --max` came from the parent command). - **`gro config show`** names the credential-ref source. ## Changes here - go.mod bump (also pulls cli-common v0.4.1 → v0.5.0 for `credstore.ListProfiles`) - `internal/cmd/root`: register `profilescmd.NewCommand()`; root test pins it - README: `gro mail list` examples + command reference A follow-up docs PR documents the profile model (`profiles` commands, `init --profile`, and the decision to keep the `default` profile). Full flow smoke-tested end-to-end with a hermetic HOME + file-backend keyring, including the real `invalid_grant` path. `make check` green.
2 participants
Why
The headline ask from the stale-token incident. With one expired token on the active profile, every command failed, and discovering that other — healthy — profiles existed required dumping the macOS keychain with
security+awk. There was no way to list profiles, see which account each holds, or switch between them.What
profiles list[--json] [--check]credstore.ListProfiles(no keychain dumping).[oauth_token]).--checkre-verifies and re-caches.--checkanswers "which of my accounts still work?":ok/expired or revoked/error: …per profile, one API round-trip each.OpenNoMigrateso it stays usable during a §1.8 migration conflict.profiles use <profile>Deliberate, visible switching of config.yml's
credential_ref(no more hand-editing). Bare profile or full same-service ref; cross-service refs rejected (they'd point this CLI at another tool's credentials). Never touches tokens. A missing token warns loudly — naming existing profiles, so typos surface — but proceeds:use work→initis the add-second-account flow. Warns when the env override shadows the switch.Supporting mechanism
keychain.ListProfiles/HasTokenFor(cross-profile presence off one service-scoped handle)auth.GetHTTPClientForRef+gmail.NewClientForRef— client bound to an explicit ref, so--checkprobes non-active profiles (sharedclientFromStorekeeps persist-on-refresh + error attribution identical on both paths)identitycachepackage (atomic writes, best-effort loads, corrupt-file tolerant)Tests
Hermetic (credtest file-backend): listing with active marker, fresh install, cached email, JSON shape, the full incident scenario under
--check(stale + healthy + network-error profiles), switch semantics (config written, tokens untouched), typo-warning flow, cross-service and invalid-profile rejection, identity-cache roundtrip/corruption, keychain cross-profile reads.make checkgreen.