Conversation
![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: 75da336c4cad
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| documentation:docs | 1 |
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
documentation:docs (1 finding)
Minor - README.md:83
The precedence description names a credential-reference environment override but never tells users which variable to set, leaving that documented selector unusable without source inspection. Document the concrete per-binary names, e.g.
GOOGLE_READONLY_CREDENTIAL_REFforgroandGOOGLE_READWRITE_CREDENTIAL_REFforgrw, alongside a short example.
Reviewer Coverage
documentation:docs— complete (broad); inspected 1 assigned file (16 inspected across reviewers):README.md; skipped: none; constraints: Reviewed README.md against the current profile-selection and rename implementation; scope limited to assigned documentation file.go:implementation-tests— complete (broad); inspected 15 assigned files (16 inspected across reviewers):internal/app/gro/credref_wire_test.go,internal/app/grw/credref_wire_test.go,internal/app/grw/main_test.go,internal/cmd/init/init.go,internal/cmd/init/init_test.go,internal/cmd/profiles/profiles.go,internal/cmd/profiles/profiles_test.go,internal/cmd/setcred/setcred.go,internal/cmd/setcred/setcred_test.go,internal/identitycache/identitycache.go,internal/identitycache/identitycache_test.go,internal/keychain/keychain.go,internal/keychain/profiles_test.go,internal/rootutil/rootutil.go,internal/rootutil/rootutil_test.go; skipped: none; constraints: Native CGO test execution is blocked by the workspace path/compiler module-cache environment; the assigned packages pass with CGO_ENABLED=0.policies:conventions— complete (broad); inspected 5 assigned files (16 inspected across reviewers):README.md,internal/cmd/init/init.go,internal/cmd/profiles/profiles.go,internal/cmd/setcred/setcred.go,internal/rootutil/rootutil.go; skipped: none; constraints: Shared cli-common and .github convenience copies were not present locally; review used the repository-local standards and visible diff only.structure:repo-health— complete (broad); inspected 6 assigned files (16 inspected across reviewers):internal/cmd/init/init.go,internal/cmd/profiles/profiles.go,internal/cmd/setcred/setcred.go,internal/identitycache/identitycache.go,internal/keychain/keychain.go,internal/rootutil/rootutil.go; skipped: none; constraints: Focused structural review of the six assigned changed files. Targeted Go tests could not run because the required RTK command proxy misparsed the workspace path containing spaces; static inspection and diff checks completed.
Inspected files (16)
README.mdinternal/app/gro/credref_wire_test.gointernal/app/grw/credref_wire_test.gointernal/app/grw/main_test.gointernal/cmd/init/init.gointernal/cmd/init/init_test.gointernal/cmd/profiles/profiles.gointernal/cmd/profiles/profiles_test.gointernal/cmd/setcred/setcred.gointernal/cmd/setcred/setcred_test.gointernal/identitycache/identitycache.gointernal/identitycache/identitycache_test.gointernal/keychain/keychain.gointernal/keychain/profiles_test.gointernal/rootutil/rootutil.gointernal/rootutil/rootutil_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 2m 37s | gpt-5.6-terra | cr 0.10.310
| Field | Value |
|---|---|
| Model | gpt-5.6-terra |
| Reviewers | documentation:docs, go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | codex_cli · gpt-5.6-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 2m 37s wall · 5m 14s compute |
| Cost | unavailable |
| Tokens | 1.5M in / 11.2k out |
Per-workstream usage
orchestrator-selection— gpt-5.6-terra- In: 18.1k
- Out: 503
- Cache read: 11.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 13s
documentation:docs— gpt-5.6-terra- In: 160.5k
- Out: 1.5k
- Cache read: 114.4k
- Cache create: unavailable
- Cost: unavailable
- Duration: 41s
go:implementation-tests— gpt-5.6-terra- In: 724.4k
- Out: 4.5k
- Cache read: 653.1k
- Cache create: unavailable
- Cost: unavailable
- Duration: 2m 07s
policies:conventions— gpt-5.6-terra- In: 196.3k
- Out: 1.8k
- Cache read: 148.2k
- Cache create: unavailable
- Cost: unavailable
- Duration: 45s
structure:repo-health— gpt-5.6-terra- In: 411.5k
- Out: 2.8k
- Cache read: 336.6k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 20s
orchestrator-rollup— gpt-5.6-terra- In: 21.4k
- Out: 81
- Cache read: 17.2k
- Cache create: unavailable
- Cost: unavailable
- Duration: 6s
| gro --ref google-readonly/work mail list | ||
| ``` | ||
|
|
||
| The selector precedence is explicit flag (`--profile` or `--ref`), credential |
There was a problem hiding this comment.
The precedence description names a credential-reference environment override but never tells users which variable to set, leaving that documented selector unusable without source inspection. Document the concrete per-binary names, e.g. GOOGLE_READONLY_CREDENTIAL_REF for gro and GOOGLE_READWRITE_CREDENTIAL_REF for grw, alongside a short example.
Reply inline to this comment.
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 626c32eb2d5b
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| documentation:docs | 0 |
| go:implementation-tests | 1 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
go:implementation-tests (1 finding)
Major - internal/cmd/profiles/profiles.go:391
If saving the active ref fails after the copy, the new bundle is left behind. Retrying
profiles rename old newthen always fails the destination-collision check, and the CLI has no profile-delete operation to complete the rename or clean up the duplicate. Roll back the newly copied destination on config-save failure (reporting any rollback failure), or make retry safely recognize and continue the prior copy; add a test that retries after a transient config-save failure.
Reviewer Coverage
documentation:docs— complete (constrained); inspected 1 assigned file (16 inspected across reviewers):README.md; skipped: none; constraints: Reviewed the assigned README.md against the updated implementation; scope limited to the documentation file.go:implementation-tests— complete (constrained); inspected 15 assigned files (16 inspected across reviewers):internal/app/gro/credref_wire_test.go,internal/app/grw/credref_wire_test.go,internal/app/grw/main_test.go,internal/cmd/init/init.go,internal/cmd/init/init_test.go,internal/cmd/profiles/profiles.go,internal/cmd/profiles/profiles_test.go,internal/cmd/setcred/setcred.go,internal/cmd/setcred/setcred_test.go,internal/identitycache/identitycache.go,internal/identitycache/identitycache_test.go,internal/keychain/keychain.go,internal/keychain/profiles_test.go,internal/rootutil/rootutil.go,internal/rootutil/rootutil_test.go; skipped: none; constraints: Test execution could not initialize the Go build cache in this read-only review workspace.policies:conventions— complete (constrained); inspected 5 assigned files (16 inspected across reviewers):README.md,internal/cmd/init/init.go,internal/cmd/profiles/profiles.go,internal/cmd/setcred/setcred.go,internal/rootutil/rootutil.go; skipped: none; constraints: Shared cli-common and .github convenience copies were not present locally; review used repository-local standards and the visible diff.structure:repo-health— complete (constrained); inspected 6 assigned files (16 inspected across reviewers):internal/cmd/init/init.go,internal/cmd/profiles/profiles.go,internal/cmd/setcred/setcred.go,internal/identitycache/identitycache.go,internal/keychain/keychain.go,internal/rootutil/rootutil.go; skipped: none; constraints: Focused review limited to the six assigned implementation files and their relevant updated test coverage.
Inspected files (16)
README.mdinternal/app/gro/credref_wire_test.gointernal/app/grw/credref_wire_test.gointernal/app/grw/main_test.gointernal/cmd/init/init.gointernal/cmd/init/init_test.gointernal/cmd/profiles/profiles.gointernal/cmd/profiles/profiles_test.gointernal/cmd/setcred/setcred.gointernal/cmd/setcred/setcred_test.gointernal/identitycache/identitycache.gointernal/identitycache/identitycache_test.gointernal/keychain/keychain.gointernal/keychain/profiles_test.gointernal/rootutil/rootutil.gointernal/rootutil/rootutil_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 1m 35s | gpt-5.6-terra | cr 0.10.310
| Field | Value |
|---|---|
| Model | gpt-5.6-terra |
| Reviewers | documentation:docs, go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | codex_cli · gpt-5.6-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 1m 35s wall · 1m 54s compute |
| Cost | unavailable |
| Tokens | 985.8k in / 4.2k out |
Per-workstream usage
documentation:docs— gpt-5.6-terra- In: 106.3k
- Out: 491
- Cache read: 95.7k
- Cache create: unavailable
- Cost: unavailable
- Duration: 16s
go:implementation-tests— gpt-5.6-terra- In: 475.5k
- Out: 2.4k
- Cache read: 441.1k
- Cache create: unavailable
- Cost: unavailable
- Duration: 55s
policies:conventions— gpt-5.6-terra- In: 211.9k
- Out: 777
- Cache read: 186.6k
- Cache create: unavailable
- Cost: unavailable
- Duration: 21s
structure:repo-health— gpt-5.6-terra- In: 167.9k
- Out: 459
- Cache read: 152.1k
- Cache create: unavailable
- Cost: unavailable
- Duration: 15s
orchestrator-rollup— gpt-5.6-terra- In: 24.3k
- Out: 80
- Cache read: 21.2k
- Cache create: unavailable
- Cost: unavailable
- Duration: 6s
| if activeChanged { | ||
| cfg.CredentialRef = newRef | ||
| cfg.SetCredentialRefSource(config.RefSourceConfig) | ||
| if err := renameSaveConfig(cfg); err != nil { |
There was a problem hiding this comment.
If saving the active ref fails after the copy, the new bundle is left behind. Retrying profiles rename old new then always fails the destination-collision check, and the CLI has no profile-delete operation to complete the rename or clean up the duplicate. Roll back the newly copied destination on config-save failure (reporting any rollback failure), or make retry safely recognize and continue the prior copy; add a test that retries after a transient config-save failure.
Reply inline to this comment.
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 6063e5467d4a
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| documentation:docs | 1 |
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
documentation:docs (1 finding)
Minor - README.md:104
This states that the copied destination is removed after an active-profile save failure, but the implementation only attempts that rollback; if deleting the copy also fails, it reports that the destination may remain. Qualify this as an attempted removal and mention that a rollback failure is reported, so users do not assume a retry cannot encounter a destination collision.
Reviewer Coverage
documentation:docs— complete (constrained); inspected 1 assigned file (16 inspected across reviewers):README.md; skipped: none; constraints: Reviewed the assigned README.md against the latest profile-rename implementation; scope limited to the documentation file.go:implementation-tests— complete (constrained); inspected 15 assigned files (16 inspected across reviewers):internal/app/gro/credref_wire_test.go,internal/app/grw/credref_wire_test.go,internal/app/grw/main_test.go,internal/cmd/init/init.go,internal/cmd/init/init_test.go,internal/cmd/profiles/profiles.go,internal/cmd/profiles/profiles_test.go,internal/cmd/setcred/setcred.go,internal/cmd/setcred/setcred_test.go,internal/identitycache/identitycache.go,internal/identitycache/identitycache_test.go,internal/keychain/keychain.go,internal/keychain/profiles_test.go,internal/rootutil/rootutil.go,internal/rootutil/rootutil_test.go; skipped: none; constraints: Test execution was not available in this read-only review workspace because Go could not initialize its build cache.policies:conventions— complete (constrained); inspected 5 assigned files (16 inspected across reviewers):README.md,internal/cmd/init/init.go,internal/cmd/profiles/profiles.go,internal/cmd/setcred/setcred.go,internal/rootutil/rootutil.go; skipped: none; constraints: Shared cli-common and .github convenience copies were not present locally; review used repository-local standards and the visible diff.structure:repo-health— complete (constrained); inspected 6 assigned files (16 inspected across reviewers):internal/cmd/init/init.go,internal/cmd/profiles/profiles.go,internal/cmd/setcred/setcred.go,internal/identitycache/identitycache.go,internal/keychain/keychain.go,internal/rootutil/rootutil.go; skipped: none; constraints: Focused review of the assigned implementation files, including the updated rename recovery path and its tests.
Inspected files (16)
README.mdinternal/app/gro/credref_wire_test.gointernal/app/grw/credref_wire_test.gointernal/app/grw/main_test.gointernal/cmd/init/init.gointernal/cmd/init/init_test.gointernal/cmd/profiles/profiles.gointernal/cmd/profiles/profiles_test.gointernal/cmd/setcred/setcred.gointernal/cmd/setcred/setcred_test.gointernal/identitycache/identitycache.gointernal/identitycache/identitycache_test.gointernal/keychain/keychain.gointernal/keychain/profiles_test.gointernal/rootutil/rootutil.gointernal/rootutil/rootutil_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 48s | gpt-5.6-terra | cr 0.10.310
| Field | Value |
|---|---|
| Model | gpt-5.6-terra |
| Reviewers | documentation:docs, go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | codex_cli · gpt-5.6-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 48s wall · 1m 08s compute |
| Cost | unavailable |
| Tokens | 751.5k in / 2.1k out |
Per-workstream usage
documentation:docs— gpt-5.6-terra- In: 123.7k
- Out: 636
- Cache read: 114.2k
- Cache create: unavailable
- Cost: unavailable
- Duration: 17s
go:implementation-tests— gpt-5.6-terra- In: 227.2k
- Out: 551
- Cache read: 214.5k
- Cache create: unavailable
- Cost: unavailable
- Duration: 16s
policies:conventions— gpt-5.6-terra- In: 176.0k
- Out: 467
- Cache read: 160.3k
- Cache create: unavailable
- Cost: unavailable
- Duration: 15s
structure:repo-health— gpt-5.6-terra- In: 197.6k
- Out: 388
- Cache read: 181.8k
- Cache create: unavailable
- Cost: unavailable
- Duration: 14s
orchestrator-rollup— gpt-5.6-terra- In: 27.1k
- Out: 66
- Cache read: 23.3k
- Cache create: unavailable
- Cost: unavailable
- Duration: 5s
|
|
||
| Renaming moves the stored credentials without re-authentication, updates the | ||
| saved active profile when necessary, and refuses a destination that already | ||
| has credentials. If saving the active-profile update fails, the copied |
There was a problem hiding this comment.
This states that the copied destination is removed after an active-profile save failure, but the implementation only attempts that rollback; if deleting the copy also fails, it reports that the destination may remain. Qualify this as an attempted removal and mention that a rollback failure is reported, so users do not assume a retry cannot encounter a destination collision.
Reply inline to this comment.
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 12d4c8394b62
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]
Summary
| Reviewer | Findings |
|---|---|
| documentation:docs | 0 |
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| structure:repo-health | 0 |
Reviewer Coverage
documentation:docs— complete (constrained); inspected 1 assigned file (16 inspected across reviewers):README.md; skipped: none; constraints: Reviewed the assigned README.md against the latest profile-rename implementation; scope limited to the documentation file.go:implementation-tests— complete (constrained); inspected 15 assigned files (16 inspected across reviewers):internal/app/gro/credref_wire_test.go,internal/app/grw/credref_wire_test.go,internal/app/grw/main_test.go,internal/cmd/init/init.go,internal/cmd/init/init_test.go,internal/cmd/profiles/profiles.go,internal/cmd/profiles/profiles_test.go,internal/cmd/setcred/setcred.go,internal/cmd/setcred/setcred_test.go,internal/identitycache/identitycache.go,internal/identitycache/identitycache_test.go,internal/keychain/keychain.go,internal/keychain/profiles_test.go,internal/rootutil/rootutil.go,internal/rootutil/rootutil_test.go; skipped: none; constraints: nonepolicies:conventions— complete (constrained); inspected 5 assigned files (16 inspected across reviewers):README.md,internal/cmd/init/init.go,internal/cmd/profiles/profiles.go,internal/cmd/setcred/setcred.go,internal/rootutil/rootutil.go; skipped: none; constraints: Shared cli-common and .github convenience copies were not present locally; review used repository-local standards and the visible diff.structure:repo-health— complete (constrained); inspected 6 assigned files (16 inspected across reviewers):internal/cmd/init/init.go,internal/cmd/profiles/profiles.go,internal/cmd/setcred/setcred.go,internal/identitycache/identitycache.go,internal/keychain/keychain.go,internal/rootutil/rootutil.go; skipped: none; constraints: Focused review limited to the six assigned implementation files; latest changes outside that set were not assessed for findings.
Inspected files (16)
README.mdinternal/app/gro/credref_wire_test.gointernal/app/grw/credref_wire_test.gointernal/app/grw/main_test.gointernal/cmd/init/init.gointernal/cmd/init/init_test.gointernal/cmd/profiles/profiles.gointernal/cmd/profiles/profiles_test.gointernal/cmd/setcred/setcred.gointernal/cmd/setcred/setcred_test.gointernal/identitycache/identitycache.gointernal/identitycache/identitycache_test.gointernal/keychain/keychain.gointernal/keychain/profiles_test.gointernal/rootutil/rootutil.gointernal/rootutil/rootutil_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 47s | gpt-5.6-terra | cr 0.10.310
| Field | Value |
|---|---|
| Model | gpt-5.6-terra |
| Reviewers | documentation:docs, go:implementation-tests, policies:conventions, structure:repo-health |
| Engine | codex_cli · gpt-5.6-terra |
| Reviewed by | cr · rianjs-bot[bot] |
| Duration | 47s wall · 57s compute |
| Cost | unavailable |
| Tokens | 719.0k in / 1.4k out |
Per-workstream usage
documentation:docs— gpt-5.6-terra- In: 140.2k
- Out: 359
- Cache read: 131.6k
- Cache create: unavailable
- Cost: unavailable
- Duration: 13s
go:implementation-tests— gpt-5.6-terra- In: 243.2k
- Out: 444
- Cache read: 237.1k
- Cache create: unavailable
- Cost: unavailable
- Duration: 16s
policies:conventions— gpt-5.6-terra- In: 198.0k
- Out: 395
- Cache read: 188.9k
- Cache create: unavailable
- Cost: unavailable
- Duration: 13s
structure:repo-health— gpt-5.6-terra- In: 107.9k
- Out: 136
- Cache read: 103.2k
- Cache create: unavailable
- Cost: unavailable
- Duration: 6s
orchestrator-rollup— gpt-5.6-terra- In: 29.7k
- Out: 36
- Cache read: 26.4k
- Cache create: unavailable
- Cost: unavailable
- Duration: 5s
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Users had to spell a full
--ref <service>/<profile>for one-off account selection, and there was no safe way to rename a stored profile without re-authenticating.Changes
--profile <name>shorthand for bothgroandgrw, while retaining--refcompatibility and clear selector conflict errors.init --profile <name>behavior, including named-profile authentication without changing the saved active profile.profiles rename <old> <new>with per-CLI namespace validation, destination collision checks, active-ref updates, cache migration, and environment override guidance.Validation
make check(tidy, golangci-lint: 0 issues, race tests, both builds)make test-cover-check(75.8%, 60% threshold)git diff --checkcr review --rerunwith the routedcodex-rianjs-botprofile (final rune72438d4-9a87-465f-97b8-301df875799a, approved with 0 findings)No live Google API or user Google credential changes were performed.