fix: scope OAuth clients and cache by profile - #31
Conversation
Test coverage assessment — PR #31Major
Minor
Focused test run: |
![rianjs-bot[bot]](https://avatars.githubusercontent.com/u/7378790?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 975d0e19236f
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| policies:conventions | 1 |
| structure:repo-health | 0 |
| documentation:docs | 0 |
go:implementation-tests (1 finding)
Major - internal/keychain/migrate.go:403
The migration records a newly derived profile association only in the in-memory cfg. When credentials.json is migrated but discover() finds no legacy token candidates, migrateLegacyOverwrite returns at line 67 before its only SaveConfig call. A legacy install with credentials.json and no existing config therefore has its client file moved/deleted, but the next process reloads no profile-owned path and fails with “no OAuth client configured.” Persist the canonical config whenever migrateOAuthClientJSON creates/changes the profile association (or return a changed flag and save before the no-candidates return), and add a reload-based regression test for that no-token migration path.
policies:conventions (1 finding)
Major - internal/cmd/me/me.go:69
The new selected-profile scope gate has no regression test. The added me tests cover rendering and Gmail-email fallback, but none seeds distinct
defaultand selected-profile scope records and verifies thatgro --profile work meuses only work’s scopes. Add a test where the default profile is current and work is stale (and ideally the inverse) to prevent a fallback-to-active-profile regression in this authentication safety path.
Reviewer Coverage
go:implementation-tests— complete (broad); inspected 24 assigned files (26 inspected across reviewers):internal/auth/auth.go,internal/auth/auth_test.go,internal/cache/cache.go,internal/cache/cache_test.go,internal/cmd/config/behavior_test.go,internal/cmd/config/config.go,internal/cmd/drive/main_test.go,internal/cmd/init/init.go,internal/cmd/init/init_test.go,internal/cmd/init/sibling_test.go,internal/cmd/me/me.go,internal/cmd/me/me_test.go,internal/cmd/profiles/profiles_test.go,internal/cmd/refresh/main_test.go,internal/config/config.go,internal/config/config_test.go,internal/config/identity.go,internal/config/identity_test.go,internal/config/relocate.go,internal/config/relocate_test.go,internal/e2e/profile_scoped_test.go,internal/keychain/keychain.go,internal/keychain/keychain_test.go,internal/keychain/migrate.go; skipped: none; constraints: Focused Go implementation and behavioral-test review. Targeted go test invocation could not build because clang rejected the workspace path.policies:conventions— complete (broad); inspected 21 assigned files (26 inspected across reviewers):README.md,docs/architecture.md,internal/cmd/config/behavior_test.go,internal/cmd/config/config.go,internal/cmd/drive/main_test.go,internal/cmd/init/init.go,internal/cmd/init/init_test.go,internal/cmd/init/sibling_test.go,internal/cmd/me/me.go,internal/cmd/me/me_test.go,internal/cmd/profiles/profiles_test.go,internal/cmd/refresh/main_test.go,internal/config/config.go,internal/config/config_test.go,internal/config/identity.go,internal/config/identity_test.go,internal/config/relocate.go,internal/config/relocate_test.go,internal/keychain/keychain.go,internal/keychain/keychain_test.go,internal/keychain/migrate.go; skipped: none; constraints: Focused Go tests could not build because clang could not resolve the workspace path containing spaces. Shared cli-common convention documents were not available locally, so findings rely on repository-local standards and visible diff context.structure:repo-health— complete (broad); inspected 12 assigned files (26 inspected across reviewers):internal/cmd/config/config.go,internal/cmd/init/init.go,internal/cmd/me/me.go,internal/config/config.go,internal/config/config_test.go,internal/config/identity.go,internal/config/identity_test.go,internal/config/relocate.go,internal/config/relocate_test.go,internal/keychain/keychain.go,internal/keychain/keychain_test.go,internal/keychain/migrate.go; skipped: none; constraints: Focused Go test command could not build in this workspace because the toolchain failed to resolve the path containing spaces; diff checks passed. Focused on assigned profile/config/keychain boundary files; did not review unrelated auth/cache implementation in depth.documentation:docs— complete (broad); inspected 2 assigned files (26 inspected across reviewers):README.md,docs/architecture.md; skipped: none; constraints: Review limited to the assigned documentation changes and their directly relevant implementation context.
Inspected files (26)
README.mddocs/architecture.mdinternal/auth/auth.gointernal/auth/auth_test.gointernal/cache/cache.gointernal/cache/cache_test.gointernal/cmd/config/behavior_test.gointernal/cmd/config/config.gointernal/cmd/drive/main_test.gointernal/cmd/init/init.gointernal/cmd/init/init_test.gointernal/cmd/init/sibling_test.gointernal/cmd/me/me.gointernal/cmd/me/me_test.gointernal/cmd/profiles/profiles_test.gointernal/cmd/refresh/main_test.gointernal/config/config.gointernal/config/config_test.gointernal/config/identity.gointernal/config/identity_test.gointernal/config/relocate.gointernal/config/relocate_test.gointernal/e2e/profile_scoped_test.gointernal/keychain/keychain.gointernal/keychain/keychain_test.gointernal/keychain/migrate.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 38s | gpt-5.6-terra | cr 0.10.311
| Field | Value |
|---|---|
| Model | gpt-5.6-terra |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health, documentation:docs |
| Engine | codex_cli · gpt-5.6-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 3m 38s wall · 6m 49s compute |
| Cost | unavailable |
| Tokens | 2.5M in / 16.7k out |
Per-workstream usage
orchestrator-selection— gpt-5.6-terra- In: 17.5k
- Out: 663
- Cache read: 13.1k
- Cache create: unavailable
- Cost: unavailable
- Duration: 15s
go:implementation-tests— gpt-5.6-terra- In: 1.5M
- Out: 7.2k
- Cache read: 1.4M
- Cache create: unavailable
- Cost: unavailable
- Duration: 2m 58s
policies:conventions— gpt-5.6-terra- In: 374.7k
- Out: 3.1k
- Cache read: 305.2k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 19s
structure:repo-health— gpt-5.6-terra- In: 465.6k
- Out: 3.5k
- Cache read: 386.8k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 36s
documentation:docs— gpt-5.6-terra- In: 131.1k
- Out: 1.5k
- Cache read: 96.3k
- Cache create: unavailable
- Cost: unavailable
- Duration: 33s
orchestrator-rollup— gpt-5.6-terra- In: 39.5k
- Out: 838
- Cache read: 30.2k
- Cache create: unavailable
- Cost: unavailable
- Duration: 6s
| _, _ = fmt.Fprintln(errOut, msg) | ||
| return errReauth | ||
| if ref, rerr := keychain.ResolveEffectiveCredentialRef(); rerr == nil { | ||
| if msg := auth.CheckScopesMigration(cfg.GrantedScopesForRef(ref)); msg != "" { |
There was a problem hiding this comment.
The new selected-profile scope gate has no regression test. The added me tests cover rendering and Gmail-email fallback, but none seeds distinct default and selected-profile scope records and verifies that gro --profile work me uses only work’s scopes. Add a test where the default profile is current and work is stale (and ideally the inverse) to prevent a fallback-to-active-profile regression in this authentication safety path.
Reply inline to this comment.
There was a problem hiding this comment.
Covered by TestRunProfileUsesSelectedScopeRecordBeforeClientCreation and TestRunProfileExtendedRendersSelectedScopes; the former seeds current default scopes plus stale work scopes and fails if ClientFactory is invoked.
There was a problem hiding this comment.
Thanks—those tests cover the selected-profile scope lookup and prevent fallback to the active default profile. This finding is addressed.
Summary:
Selected-profile scope behavior is covered by regression tests, including stale work scopes with current default scopes and extended selected-scope rendering.
| if state.OAuthClientPath == "" { | ||
| state.OAuthClientPath = target | ||
| } | ||
| if err := cfg.SetProfileOAuth(ref, state); err != nil { |
There was a problem hiding this comment.
The migration records a newly derived profile association only in the in-memory cfg. When credentials.json is migrated but discover() finds no legacy token candidates, migrateLegacyOverwrite returns at line 67 before its only SaveConfig call. A legacy install with credentials.json and no existing config therefore has its client file moved/deleted, but the next process reloads no profile-owned path and fails with “no OAuth client configured.” Persist the canonical config whenever migrateOAuthClientJSON creates/changes the profile association (or return a changed flag and save before the no-candidates return), and add a reload-based regression test for that no-token migration path.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed - persist the selected-profile client association before the no-token early return, with a reload regression for a non-default active profile.
There was a problem hiding this comment.
Thanks—the persistence before the early return and reload-based regression cover the reported migration-loss path. This finding is addressed.
Summary:
OAuth-client migration now persists the selected-profile association even when no legacy token candidates exist, with reload coverage for a non-default active profile.
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 93c06dc9e803
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| structure:repo-health | 1 |
| documentation:docs | 0 |
structure:repo-health (1 finding)
Major - internal/keychain/migrate.go:75
The new persistence step runs only after
migrateOAuthClientJSONhas already copied the client and removedcredentials.json. IfSaveConfigfails here, the command returns an error but the retry sees no legacy file, so it never recreates the selected profile association; that profile is left unable to locate its migrated client. Preserve retryability by persisting the association before deleting the legacy source (or otherwise retaining enough migration state to repair it on the next run), and add a regression that forces the config save to fail then retries.
Reviewer Coverage
go:implementation-tests— complete (constrained); inspected 24 assigned files (26 inspected across reviewers):internal/auth/auth.go,internal/auth/auth_test.go,internal/cache/cache.go,internal/cache/cache_test.go,internal/cmd/config/behavior_test.go,internal/cmd/config/config.go,internal/cmd/drive/main_test.go,internal/cmd/init/init.go,internal/cmd/init/init_test.go,internal/cmd/init/sibling_test.go,internal/cmd/me/me.go,internal/cmd/me/me_test.go,internal/cmd/profiles/profiles_test.go,internal/cmd/refresh/main_test.go,internal/config/config.go,internal/config/config_test.go,internal/config/identity.go,internal/config/identity_test.go,internal/config/relocate.go,internal/config/relocate_test.go,internal/e2e/profile_scoped_test.go,internal/keychain/keychain.go,internal/keychain/keychain_test.go,internal/keychain/migrate.go; skipped: none; constraints: nonepolicies:conventions— complete (constrained); inspected 21 assigned files (26 inspected across reviewers):README.md,docs/architecture.md,internal/cmd/config/behavior_test.go,internal/cmd/config/config.go,internal/cmd/drive/main_test.go,internal/cmd/init/init.go,internal/cmd/init/init_test.go,internal/cmd/init/sibling_test.go,internal/cmd/me/me.go,internal/cmd/me/me_test.go,internal/cmd/profiles/profiles_test.go,internal/cmd/refresh/main_test.go,internal/config/config.go,internal/config/config_test.go,internal/config/identity.go,internal/config/identity_test.go,internal/config/relocate.go,internal/config/relocate_test.go,internal/keychain/keychain.go,internal/keychain/keychain_test.go,internal/keychain/migrate.go; skipped: none; constraints: Shared cli-common convention documents were not available locally; review relied on repository-local standards and visible diff context.structure:repo-health— complete (constrained); inspected 12 assigned files (26 inspected across reviewers):internal/cmd/config/config.go,internal/cmd/init/init.go,internal/cmd/me/me.go,internal/config/config.go,internal/config/config_test.go,internal/config/identity.go,internal/config/identity_test.go,internal/config/relocate.go,internal/config/relocate_test.go,internal/keychain/keychain.go,internal/keychain/keychain_test.go,internal/keychain/migrate.go; skipped: none; constraints: Focused on assigned profile/config/keychain boundary files and the follow-up migration fix.documentation:docs— complete (constrained); inspected 2 assigned files (26 inspected across reviewers):README.md,docs/architecture.md; skipped: none; constraints: Review limited to the assigned documentation changes and their directly relevant implementation context.
Inspected files (26)
README.mddocs/architecture.mdinternal/auth/auth.gointernal/auth/auth_test.gointernal/cache/cache.gointernal/cache/cache_test.gointernal/cmd/config/behavior_test.gointernal/cmd/config/config.gointernal/cmd/drive/main_test.gointernal/cmd/init/init.gointernal/cmd/init/init_test.gointernal/cmd/init/sibling_test.gointernal/cmd/me/me.gointernal/cmd/me/me_test.gointernal/cmd/profiles/profiles_test.gointernal/cmd/refresh/main_test.gointernal/config/config.gointernal/config/config_test.gointernal/config/identity.gointernal/config/identity_test.gointernal/config/relocate.gointernal/config/relocate_test.gointernal/e2e/profile_scoped_test.gointernal/keychain/keychain.gointernal/keychain/keychain_test.gointernal/keychain/migrate.go
2 PR discussion threads considered. 2 summarized; 2 resolved.
Completed in 1m 33s | gpt-5.6-terra | cr 0.10.311
| Field | Value |
|---|---|
| Model | gpt-5.6-terra |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health, documentation:docs |
| Engine | codex_cli · gpt-5.6-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 1m 33s wall · 2m 26s compute |
| Cost | unavailable |
| Tokens | 4.3M in / 22.0k out |
Per-workstream usage
go:implementation-tests— gpt-5.6-terra- In: 2.1M
- Out: 8.7k
- Cache read: 2.0M
- Cache create: unavailable
- Cost: unavailable
- Duration: 35s
policies:conventions— gpt-5.6-terra- In: 876.4k
- Out: 4.9k
- Cache read: 759.6k
- Cache create: unavailable
- Cost: unavailable
- Duration: 48s
structure:repo-health— gpt-5.6-terra- In: 886.9k
- Out: 5.3k
- Cache read: 694.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 44s
documentation:docs— gpt-5.6-terra- In: 238.0k
- Out: 1.9k
- Cache read: 190.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 12s
orchestrator-rollup— gpt-5.6-terra- In: 117.7k
- Out: 1.2k
- Cache read: 100.1k
- Cache create: unavailable
- Cost: unavailable
- Duration: 4s
| // the selected profile even when there is no legacy token. Persist that | ||
| // association, unless promotion already saved the same canonical config. | ||
| if clientPathChanged && !promotedConfig { | ||
| if err := config.SaveConfig(cfg); err != nil { |
There was a problem hiding this comment.
The new persistence step runs only after migrateOAuthClientJSON has already copied the client and removed credentials.json. If SaveConfig fails here, the command returns an error but the retry sees no legacy file, so it never recreates the selected profile association; that profile is left unable to locate its migrated client. Preserve retryability by persisting the association before deleting the legacy source (or otherwise retaining enough migration state to repair it on the next run), and add a regression that forces the config save to fail then retries.
Reply inline to this comment.
There was a problem hiding this comment.
Fixed in 2c25ab4: the Open migration now retains credentials.json until the selected-profile association is saved. A regression forces the first SaveConfig to fail, verifies the legacy source remains, retries, reloads the work association, and then verifies cleanup.
There was a problem hiding this comment.
Thanks—the migration now retains the legacy source until the profile association is durable, and the forced-save-failure retry test covers the recovery path. This finding is addressed.
Summary:
OAuth-client migration preserves credentials.json until saving the selected-profile association succeeds; regression coverage verifies failed-save retry, reload, and subsequent cleanup.
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 74693373ebf4
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
| documentation:docs | 0 |
Reviewer Coverage
go:implementation-tests— complete (constrained); inspected 24 assigned files (26 inspected across reviewers):internal/auth/auth.go,internal/auth/auth_test.go,internal/cache/cache.go,internal/cache/cache_test.go,internal/cmd/config/behavior_test.go,internal/cmd/config/config.go,internal/cmd/drive/main_test.go,internal/cmd/init/init.go,internal/cmd/init/init_test.go,internal/cmd/init/sibling_test.go,internal/cmd/me/me.go,internal/cmd/me/me_test.go,internal/cmd/profiles/profiles_test.go,internal/cmd/refresh/main_test.go,internal/config/config.go,internal/config/config_test.go,internal/config/identity.go,internal/config/identity_test.go,internal/config/relocate.go,internal/config/relocate_test.go,internal/e2e/profile_scoped_test.go,internal/keychain/keychain.go,internal/keychain/keychain_test.go,internal/keychain/migrate.go; skipped: none; constraints: nonepolicies:conventions— complete (constrained); inspected 21 assigned files (26 inspected across reviewers):README.md,docs/architecture.md,internal/cmd/config/behavior_test.go,internal/cmd/config/config.go,internal/cmd/drive/main_test.go,internal/cmd/init/init.go,internal/cmd/init/init_test.go,internal/cmd/init/sibling_test.go,internal/cmd/me/me.go,internal/cmd/me/me_test.go,internal/cmd/profiles/profiles_test.go,internal/cmd/refresh/main_test.go,internal/config/config.go,internal/config/config_test.go,internal/config/identity.go,internal/config/identity_test.go,internal/config/relocate.go,internal/config/relocate_test.go,internal/keychain/keychain.go,internal/keychain/keychain_test.go,internal/keychain/migrate.go; skipped: none; constraints: Follow-up review focused on the two files changed since the prior reviewed head. Shared cli-common convention documents were not available locally. Shared cli-common convention documents were not available locally; review used repository-local standards and visible diff context.structure:repo-health— complete (constrained); inspected 12 assigned files (26 inspected across reviewers):internal/cmd/config/config.go,internal/cmd/init/init.go,internal/cmd/me/me.go,internal/config/config.go,internal/config/config_test.go,internal/config/identity.go,internal/config/identity_test.go,internal/config/relocate.go,internal/config/relocate_test.go,internal/keychain/keychain.go,internal/keychain/keychain_test.go,internal/keychain/migrate.go; skipped: none; constraints: Follow-up review focused on the resolved migration retryability path and its regression coverage.documentation:docs— complete (constrained); inspected 2 assigned files (26 inspected across reviewers):README.md,docs/architecture.md; skipped: none; constraints: Review limited to the assigned documentation changes and their directly relevant implementation context.
Inspected files (26)
README.mddocs/architecture.mdinternal/auth/auth.gointernal/auth/auth_test.gointernal/cache/cache.gointernal/cache/cache_test.gointernal/cmd/config/behavior_test.gointernal/cmd/config/config.gointernal/cmd/drive/main_test.gointernal/cmd/init/init.gointernal/cmd/init/init_test.gointernal/cmd/init/sibling_test.gointernal/cmd/me/me.gointernal/cmd/me/me_test.gointernal/cmd/profiles/profiles_test.gointernal/cmd/refresh/main_test.gointernal/config/config.gointernal/config/config_test.gointernal/config/identity.gointernal/config/identity_test.gointernal/config/relocate.gointernal/config/relocate_test.gointernal/e2e/profile_scoped_test.gointernal/keychain/keychain.gointernal/keychain/keychain_test.gointernal/keychain/migrate.go
1 PR discussion threads considered. 1 summarized; 1 resolved.
Completed in 1m 25s | gpt-5.6-terra | cr 0.10.314
| Field | Value |
|---|---|
| Model | gpt-5.6-terra |
| Reviewers | go:implementation-tests, policies:conventions, structure:repo-health, documentation:docs |
| Engine | codex_cli · gpt-5.6-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 1m 25s wall · 1m 59s compute |
| Cost | unavailable |
| Tokens | 7.4M in / 32.7k out |
Per-workstream usage
go:implementation-tests— gpt-5.6-terra- In: 2.7M
- Out: 10.0k
- Cache read: 2.3M
- Cache create: unavailable
- Cost: unavailable
- Duration: 31s
policies:conventions— gpt-5.6-terra- In: 3.0M
- Out: 12.8k
- Cache read: 2.5M
- Cache create: unavailable
- Cost: unavailable
- Duration: 49s
structure:repo-health— gpt-5.6-terra- In: 1.1M
- Out: 6.2k
- Cache read: 828.7k
- Cache create: unavailable
- Cost: unavailable
- Duration: 21s
documentation:docs— gpt-5.6-terra- In: 366.8k
- Out: 2.3k
- Cache read: 262.1k
- Cache create: unavailable
- Cost: unavailable
- Duration: 12s
orchestrator-rollup— gpt-5.6-terra- In: 179.2k
- Out: 1.4k
- Cache read: 140.5k
- Cache create: unavailable
- Cost: unavailable
- Duration: 4s
1 participant
Problem
--profileselected a token ref, but OAuth client JSON, granted scopes, and Drive metadata cache remained global/default state. A selected profile could therefore use another profile’s client, scope record, or cache.Minimal changes
oauth_client_pathandgranted_scopesunderprofiles.<bare-name>while keepingcredential_refand keyring selection global.profiles list --check, init, config/me scope checks, and the cache instance key.Migration behavior
Existing legacy client files remain referenced in place during schema migration. The next config save writes canonical profile-owned state. Existing unscoped Drive cache data is disposable and is not guessed into a profile;
config clear --allstill removes the whole CLI cache/config reset surface.Validation
go test ./internal/... -count=1— 1,760 passed across 46 packages.make check— module tidy, golangci-lint (0 issues), race tests, and builds.make test-cover-check— 74.9% total coverage (60% threshold).git diff --check.go test ./internal/cmd/profiles -run TestRunList_Check -count=1—profiles list --checkverifies each explicit full ref once.go test ./internal/cmd/config -run TestRunClearSemantics -count=1—config clear --allbehavior remains covered (7 passed).go test ./internal/e2e -run TestBuiltCLIProfileStateIsHermetic -count=1— built CLI proves distinct fingerprints/scopes, no unconfigured fallback, and profile-isolated cache status.Closes #30