feat: profiles list/use, attributed auth errors, init target naming, mail list - #175
Conversation
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: abdd18340c2e
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| documentation:docs | 2 |
| structure:repo-health | 0 |
documentation:docs (2 findings)
Major - README.md:277
The PR's stated purpose is to expose
gro profiles list [--check] [--json]andgro profiles use <profile>, and root.go confirmsprofilescmd.NewCommand()is now registered as a top-level command — but README.md never mentionsgro profilesanywhere (not in Features, not in the Configuration Commands quick-reference block ending here, not in the Command Reference section). A user hitting the exactinvalid_grantscenario this PR was built to fix has no documented way to discovergro profiles list/gro profiles use. Add agro profiles list/gro profiles useentry to the Configuration Commands block and a matching### gro profiles list/### gro profiles usesection under Command Reference, describing the active-profile marker,--checktoken-health output, and--json.
Major - README.md:277
The PR dossier states
gro init --profile <name>adds a new account without touching the active profile's token, but thegro initCommand Reference flags list (lines 512-516) only documents--auth-code-stdin,--credentials-file,--no-browser, and--no-verify—--profileis missing. Add a--profile stringrow here (and a short usage example under the '4. Run the wizard' setup walkthrough) so users know how to add an additional account instead of overwriting the active one.
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | complete_broad | go.mod, go.sum, internal/cmd/root/root.go, internal/cmd/root/root_test.go | unavailable | unavailable |
| policies:conventions | complete_broad | README.md, go.mod, internal/cmd/root/root.go | unavailable | Reviewed the current on-disk state of the assigned files (head checkout) rather than a line-anchored unified diff, so the README finding is anchored at file level rather than to a specific added line. |
| documentation:docs | complete_broad | README.md | unavailable | Could not run git diff against the base SHA in this sandbox (git commands required approval that was not available), so exact added-line ranges in README.md were inferred by cross-referencing README content against internal/cmd/root/root.go and the PR dossier rather than a line-level diff. |
| structure:repo-health | complete_broad | internal/cmd/root/root.go | unavailable | unavailable |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 2m 58s | $2.03 | claude-sonnet-5 | cr 0.10.268
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | go:implementation-tests, policies:conventions, documentation:docs, structure:repo-health |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 2m 58s wall · 5m 43s compute |
| Cost | $2.03 |
| Tokens | 110 in / 24.7k out |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-5 | 6 | 2.3k | 57.5k | 14.1k | $0.14 | 28s |
| go:implementation-tests | claude-sonnet-5 | 22 | 3.6k | 330.8k | 22.7k | $0.29 | 50s |
| policies:conventions | claude-sonnet-5 | 34 | 8.5k | 628.7k | 34.5k | $0.52 | 1m 46s |
| documentation:docs | claude-sonnet-5 | 30 | 8.0k | 699.7k | 61.0k | $0.70 | 1m 36s |
| structure:repo-health | claude-sonnet-5 | 12 | 1.6k | 174.9k | 21.3k | $0.21 | 29s |
| orchestrator-rollup | claude-sonnet-5 | 6 | 767 | 81.4k | 23.2k | $0.18 | 32s |
| @@ -277,6 +277,11 @@ gro -v <command> | |||
| All Gmail commands are under `gro mail`: | |||
There was a problem hiding this comment.
File-level note: README.md
The PR's stated purpose is to expose gro profiles list [--check] [--json] and gro profiles use <profile>, and root.go confirms profilescmd.NewCommand() is now registered as a top-level command — but README.md never mentions gro profiles anywhere (not in Features, not in the Configuration Commands quick-reference block ending here, not in the Command Reference section). A user hitting the exact invalid_grant scenario this PR was built to fix has no documented way to discover gro profiles list/gro profiles use. Add a gro profiles list / gro profiles use entry to the Configuration Commands block and a matching ### gro profiles list / ### gro profiles use section under Command Reference, describing the active-profile marker, --check token-health output, and --json.
Reply inline to this comment.
| @@ -277,6 +277,11 @@ gro -v <command> | |||
| All Gmail commands are under `gro mail`: | |||
There was a problem hiding this comment.
File-level note: README.md
The PR dossier states gro init --profile <name> adds a new account without touching the active profile's token, but the gro init Command Reference flags list (lines 512-516) only documents --auth-code-stdin, --credentials-file, --no-browser, and --no-verify — --profile is missing. Add a --profile string row here (and a short usage example under the '4. Run the wizard' setup walkthrough) so users know how to add an additional account instead of overwriting the active one.
Reply inline to this comment.
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: e919a43d1450
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 1 |
| documentation:docs | 0 |
| structure:repo-health | 0 |
policies:conventions (1 finding)
Nits - README.md:149
The two prior review threads about missing
gro profilesdocs are resolved in this diff:--profileis now listed undergro initflags, andgro profiles list/gro profiles useare documented in both the Configuration Commands quick reference and the Command Reference. One gap remains: the top-level Features section (unchanged by this diff) still doesn't mention the profiles UX, even though the PR's stated purpose is discoverability of profile/token state. Consider adding a short Features bullet (e.g. alongside 'Secure storage') pointing atgro profiles list --check.
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | complete_broad | go.mod, go.sum, internal/cmd/root/root.go, internal/cmd/root/root_test.go | unavailable | profilescmd.NewCommand() itself lives in the external google-cli-common dependency, not this repo, so its internal implementation and tests were not reviewable here; review was limited to the version bump and registration wiring in this repo. |
| policies:conventions | complete_broad | README.md, go.mod, internal/cmd/root/root.go | unavailable | unavailable |
| documentation:docs | complete_broad | README.md | unavailable | Reviewed the current head README.md in the workbench checkout only; could not run git diff against the base SHA due to sandbox restrictions, so exact added lines were inferred from the +62/-0 change map plus content inspection rather than a line-level diff. |
| structure:repo-health | complete_broad | internal/cmd/root/root.go | unavailable | Scope limited to internal/cmd/root/root.go per assignment; README documentation gaps for the new profiles command group (already flagged in existing inline discussion) fall outside the assigned file and are not re-raised here. |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 31s | $1.94 | claude-sonnet-5 | cr 0.10.268
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | go:implementation-tests, policies:conventions, documentation:docs, structure:repo-health |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 3m 31s wall · 7m 02s compute |
| Cost | $1.94 |
| Tokens | 112 in / 20.1k out |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-5 | 6 | 1.3k | 95.7k | 31.7k | $0.24 | 17s |
| go:implementation-tests | claude-sonnet-5 | 22 | 3.5k | 401.1k | 28.4k | $0.34 | 46s |
| policies:conventions | claude-sonnet-5 | 24 | 5.5k | 390.3k | 25.2k | $0.35 | 2m 38s |
| documentation:docs | claude-sonnet-5 | 34 | 6.4k | 608.3k | 25.6k | $0.43 | 2m 26s |
| structure:repo-health | claude-sonnet-5 | 20 | 2.9k | 345.1k | 28.3k | $0.32 | 42s |
| orchestrator-rollup | claude-sonnet-5 | 6 | 472 | 106.4k | 34.9k | $0.25 | 10s |
| @@ -149,6 +149,14 @@ After creating the OAuth credentials in step 2 (or 3), run: | |||
| gro init | |||
There was a problem hiding this comment.
File-level note: README.md
The two prior review threads about missing gro profiles docs are resolved in this diff: --profile is now listed under gro init flags, and gro profiles list/gro profiles use are documented in both the Configuration Commands quick reference and the Command Reference. One gap remains: the top-level Features section (unchanged by this diff) still doesn't mention the profiles UX, even though the PR's stated purpose is discoverability of profile/token state. Consider adding a short Features bullet (e.g. alongside 'Secure storage') pointing at gro profiles list --check.
Reply inline to this comment.
…mail list (google-cli-common v0.3.0) Bumps google-cli-common v0.2.0 -> v0.3.0 and registers the new profiles command group. This delivers the credential/profile UX overhaul driven by a first-hand mis-diagnosis (a stale default-profile token read as 'gro is dead'): - gro profiles list [--check] - every stored profile, its account email, an active marker (with where the selection came from), and per-profile live token health. No more dumping the OS keychain to discover accounts. - gro profiles use <profile> - deliberate, visible switching of the active binding in config.yml. - Auth errors now name the failing credential ref, its selection source, and that other profiles may be unaffected (with the profiles-list hint). - gro init announces which profile/account it will (re)authenticate before any prompt or write; gro init --profile <name> adds a NEW account without touching the active profile's token. - gro mail list - sibling symmetry with calendar/contacts/drive list (the 'unknown flag: --max' confusion was a missing command, not a flag divergence). - gro config show names the credential-ref source. README documents the new commands; root test pins the registered command.
e919a43 to
03a6b6e
Compare
## Why Closes out the headline question from the credential-UX review: **should the `default` profile exist at all?** It was reconsidered seriously — the incident that prompted it was real (a silently-stale active profile misread as "gro is dead"). ## Decision: keep `default`; fix the failure mode mechanically - `<service>/default` is the codified family-wide convention (cli-common `working-with-secrets.md` §2.1, `credstore.DefaultProfile` — same as `atlassian-cli/default`). Removing or renaming it is a breaking change for every existing install and diverges gro from its siblings, for no mechanical gain. - **Requiring an explicit profile** on every invocation punishes the single-account majority to protect the multi-account minority. - **Naming profiles after accounts** can't be the default: emails aren't valid ref segments (`[A-Za-z0-9_-]`), and the account is only known *after* authentication. Users who want purposeful names already have them: `gro init --profile work`. - The actual failure mode was never the name — it was an **invisible pointer that silently went stale**. That's now fixed mechanically (#175 / google-cli-common v0.3.0): `profiles list` (visibility), attributed auth errors (blames the right profile, points at the list), init target naming + `--profile` (no silent overwrites), `profiles use` (deliberate binding). ## What A README section ("Profiles: how gro handles multiple Google accounts") making the model explicit: profiles are independent; the active binding is visible and deliberate; adding an account never overwrites one. Placed directly above the existing per-invocation `--ref` section. Docs only; no code changes.
2 participants
Why
Delivers the credential/profile UX overhaul to gro by bumping google-cli-common v0.2.0 → v0.3.0 (#4, #5, #6, #7) and registering the new
profilescommand 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:
gro initannounces which profile/account it will (re)authenticate before any prompt or write, andgro init --profile <name>adds a NEW account without touching the active profile's token.gro mail list— the audit of item 6 found no--maxdivergence among siblings; the real gap was the missinglistcommand (cobra'sunknown flag: --maxcame from the parent command).gro config shownames the credential-ref source.Changes here
credstore.ListProfiles)internal/cmd/root: registerprofilescmd.NewCommand(); root test pins itgro mail listexamples + command referenceA follow-up docs PR documents the profile model (
profilescommands,init --profile, and the decision to keep thedefaultprofile).Full flow smoke-tested end-to-end with a hermetic HOME + file-backend keyring, including the real
invalid_grantpath.make checkgreen.