fix(auth): attribute token failures to the credential ref and its source - #4
Conversation
An expired/revoked refresh token surfaced as a bare 'oauth2: "invalid_grant" "Token has been expired or revoked"' with no indication of WHICH credential failed or where it was selected. On a multi-profile machine that reads as 'the tool is broken' when the real state is 'this one profile is stale and others are fine' - a first-hand mis-diagnosis this change exists to prevent. - config: record CredentialRef provenance (flag/env/config/default/explicit) when the ref is resolved; carried on the Config, never serialized. - keychain: effectiveRef/applyCredentialRefOverride report the winning source; Store exposes RefSource(); DescribeRefSource renders the human label (including the derived <SERVICE>_CREDENTIAL_REF env name). - auth: wrap the runtime token source so auth-class failures name the ref, its source, and remediation (profiles list / init). Non-auth errors pass through untouched - a network outage must not be attributed to a credential. The no-token-stored error names the ref and source too. - auth: promote initcmd's isAuthError to auth.IsAuthError so the re-auth gate and the attribution wrapper share one definition. - configcmd: 'config show' displays the ref source (human + JSON).
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 63f630509ab3
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
go:implementation-tests (1 finding)
Minor - config/config.go:381
applyDefaults sets credentialRefSource to RefSourceConfig or RefSourceDefault depending on whether CredentialRef was already populated when LoadConfig ran — this is the base-case provenance the rest of the attribution feature (auth error messages,
config show) relies on when no --ref/env override applies. None of the existing TestLoadConfig subtests in config/config_test.go (unmodified by this PR) assert CredentialRefSource() after a load with an explicit config.yml credential_ref vs. a fresh install with none; keychain/credref_test.go only covers the override path using a directly-constructed *Config, which bypasses applyDefaults entirely. A regression here (e.g. accidentally swapping the two branches, or a code path that skips applyDefaults) would silently mislabel every unwrapped auth error as coming from the wrong source and would not be caught by any test in this diff. Add a subtest to TestLoadConfig asserting cfg.CredentialRefSource() == RefSourceDefault when credential_ref is absent and == RefSourceConfig when it is set in config.yml.
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | complete_broad | auth/auth.go, auth/autherror_test.go, config/config.go, configcmd/config.go, initcmd/init.go, initcmd/init_test.go, keychain/credref_test.go, keychain/keychain.go | unavailable | unavailable |
| policies:conventions | complete_broad | config/config.go, configcmd/config.go, initcmd/init.go, keychain/keychain.go | unavailable | Local docs/ (repo-owned standards) not present in this checkout; could not verify doc-update requirements. |
| structure:repo-health | complete_broad | auth/auth.go, config/config.go, initcmd/init.go, keychain/keychain.go | unavailable | Repo checkout was an artifact clone; docs/ and AGENTS.md-referenced source-of-truth files (docs/development.md, docs/README.md) were not present in the working tree, so no assessment was made of whether the new RefSource/provenance concept should be documented there. |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 4m 52s | $2.87 | 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 52s wall · 9m 29s compute |
| Cost | $2.87 |
| Tokens | 148 in / 32.6k out |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-5 | 6 | 2.6k | 59.6k | 14.6k | $0.15 | 29s |
| go:implementation-tests | claude-sonnet-5 | 52 | 9.3k | 1.3M | 63.7k | $0.92 | 2m 12s |
| policies:conventions | claude-sonnet-5 | 42 | 8.4k | 926.7k | 60.6k | $0.77 | 4m 00s |
| structure:repo-health | claude-sonnet-5 | 42 | 12.0k | 1.1M | 66.1k | $0.91 | 2m 34s |
| orchestrator-rollup | claude-sonnet-5 | 6 | 408 | 72.3k | 18.0k | $0.14 | 12s |
| @@ -349,6 +381,9 @@ func loadLegacyJSON(cfg *Config) error { | |||
| func (c *Config) applyDefaults() { | |||
There was a problem hiding this comment.
applyDefaults sets credentialRefSource to RefSourceConfig or RefSourceDefault depending on whether CredentialRef was already populated when LoadConfig ran — this is the base-case provenance the rest of the attribution feature (auth error messages, config show) relies on when no --ref/env override applies. None of the existing TestLoadConfig subtests in config/config_test.go (unmodified by this PR) assert CredentialRefSource() after a load with an explicit config.yml credential_ref vs. a fresh install with none; keychain/credref_test.go only covers the override path using a directly-constructed *Config, which bypasses applyDefaults entirely. A regression here (e.g. accidentally swapping the two branches, or a code path that skips applyDefaults) would silently mislabel every unwrapped auth error as coming from the wrong source and would not be caught by any test in this diff. Add a subtest to TestLoadConfig asserting cfg.CredentialRefSource() == RefSourceDefault when credential_ref is absent and == RefSourceConfig when it is set in config.yml.
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
Every command on a machine with a stale active profile failed with a bare:
Nothing named which credential ref failed, where it was selected (--ref flag vs env vs config.yml vs built-in default), or that other profiles might be healthy. That exact message caused a first-hand "gro is dead" mis-diagnosis when only
google-readonly/defaultwas stale.What
config:
Configrecords CredentialRef provenance (RefSource: flag/env/config/default/explicit) at resolution time. Unexported field — provenance, not configuration; never serialized.keychain:
effectiveRef/applyCredentialRefOverridereport the winning source;Store.RefSource();DescribeRefSourcerenders the human label (incl. the derived<SERVICE>_CREDENTIAL_REFenv-var name).auth: the runtime token source is wrapped so auth-class failures become:
Non-auth errors (network, API outage) pass through untouched — attributing an outage to a credential would be its own mis-diagnosis. The underlying
*oauth2.RetrieveErrorstayserrors.As-able through the wrap. The "no token stored" error also names ref + source.auth:
initcmd.isAuthErrorpromoted toauth.IsAuthError— init's re-auth gate and this wrapper now share one definition (test moved with it).configcmd:
config showprintsCredential ref: ... (via <source>)and addscredential_ref_sourceto JSON.Note: the hint references
profiles list, which lands in a sibling PR before any consumer release picks either up.Tests
invalid_grant; passes through non-auth errors and success; preserveserrors.As.DescribeRefSourcelabels pinned (incl. derived env-var name).make checkgreen (tidy + lint + test -race + build).