Repository navigation
Give each credential exactly one refresher - #108
Merged
Merged
Conversation
OpenAI rotates a Codex refresh token on every use and invalidates the previous one, so a credential can have exactly one refresher. Two hosts sharing an account store therefore destroy each other's tokens: this was observed across a laptop and a mac mini, where 11 of 12 shared accounts failed with refresh_token_reused, some dating back to May. Team mode already routes through broker leases and never sends local refresh tokens upstream, but the local scheduler still refreshed every OAuth account to score usage, on two paths (reloadAccounts and refreshUsageScoresIfStale). Neither was gated on the broker, so a team-mode machine kept rotating tokens the vault owns. Both paths now return early when a credential broker is configured; local mode is unchanged and still refreshes, which a test pins from both sides. Uploading a credential now also hands over ownership rather than only copying the bytes: the local file moves to <store>/migrated/, which the store does not scan, so the daemon stops refreshing it while the record stays readable for rollback.
📝 WalkthroughWalkthroughCloud imports now hand off uploaded Codex credentials into a migrated directory, while broker-backed team mode bypasses local OAuth refresh and usage-score updates. Tests cover migration records, permissions, missing accounts, and team-versus-local refresh behavior. ChangesCodex credential handover
Team-mode refresh control
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CloudImport
participant CodexStore
participant MigratedDirectory
CloudImport->>CodexStore: Upload Codex credential
CloudImport->>CodexStore: MigrateStoredAway(label)
CodexStore->>MigratedDirectory: Move active JSON record
MigratedDirectory-->>CloudImport: Return rollback record path
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/subrouter/sr_cloud.go`:
- Around line 776-790: Make the UploadAccount and MigrateStoredAway sequence in
the account upload flow failure-safe: ensure local disowning succeeds before
leaving the remote account active, or compensate by disabling/deleting the newly
uploaded broker account if migration fails. Preserve rollback data and return
the migration error, and add a test covering migration failure to verify no
active remote adoption remains while the local credential is still present.
In `@internal/proxy/team_mode_refresh_test.go`:
- Around line 26-30: Update the test setup around the newServer helper and
AccountRef to persist both fixture accounts into the temporary CodexStore before
constructing the Server, so reloadAccounts can load them and invoke
RefreshAccountFn. Add an assertion that the reload count equals the number of
fixture accounts, preserving the existing team and local-mode assertions; run go
test ./... before handoff.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5da6b109-2d18-41d5-a365-6f6b333b3111
📒 Files selected for processing (6)
cmd/subrouter/sr_cloud.gointernal/accounts/codex_store.gointernal/accounts/codex_store_migrate_test.gointernal/proxy/dashboard_test.gointernal/proxy/proxy.gointernal/proxy/team_mode_refresh_test.go
Comment on lines
776
to
+790
| if _, err := client.UploadAccount(ctx, upload.body); err != nil { | ||
| return fmt.Errorf("upload %s %s: %w", upload.kind, upload.label, err) | ||
| } | ||
| uploaded++ | ||
| fmt.Fprintf(r.out, "uploaded %-20s %s\n", upload.kind, upload.label) | ||
| // Hand over ownership as well as the bytes. The provider rotates the | ||
| // refresh token on every use, so leaving a refreshable local copy makes | ||
| // this machine and the vault invalidate each other's chain. The record | ||
| // is kept for rollback, just where the daemon will not refresh it. | ||
| if upload.kind != "codex" { | ||
| continue | ||
| } | ||
| path, ok, err := r.store.MigrateStoredAway(upload.label) | ||
| if err != nil { | ||
| return fmt.Errorf("hand over %s: %w", upload.label, err) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Make remote adoption and local disowning failure-safe.
UploadAccount adopts the credential before MigrateStoredAway. If the move fails, the vault has accepted the refresh chain while the local active file remains, recreating the two-refreshers condition this PR prevents. Stage the upload until migration succeeds, or compensate by disabling/deleting the newly uploaded broker account before returning the error; cover this failure path with a test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/subrouter/sr_cloud.go` around lines 776 - 790, Make the UploadAccount and
MigrateStoredAway sequence in the account upload flow failure-safe: ensure local
disowning succeeds before leaving the remote account active, or compensate by
disabling/deleting the newly uploaded broker account if migration fails.
Preserve rollback data and return the migration error, and add a test covering
migration failure to verify no active remote adoption remains while the local
credential is still present.
Comment on lines
+26
to
+30
| newServer := func(refreshes *int32, credBroker CredentialBroker) Server { | ||
| return Server{ | ||
| Accounts: local, | ||
| SchedulerRef: selectacct.NewSchedulerRef(selectacct.NewScheduler(nil)), | ||
| AccountRef: NewAccountRef(accounts.CodexStore{Dir: t.TempDir()}, local, nil), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Populate the test store before reloading accounts.
AccountRef points to a fresh empty CodexStore, so reloadAccounts loads zero records and never reaches RefreshAccountFn. This makes the team assertions vacuous and causes the local-mode assertion to fail in CI. Persist the two fixture accounts before constructing the server, and assert that the reload count matches the fixture size.
As per coding guidelines, run go test ./... before handoff; the supplied CI run currently fails this test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/proxy/team_mode_refresh_test.go` around lines 26 - 30, Update the
test setup around the newServer helper and AccountRef to persist both fixture
accounts into the temporary CodexStore before constructing the Server, so
reloadAccounts can load them and invoke RefreshAccountFn. Add an assertion that
the reload count equals the number of fixture accounts, preserving the existing
team and local-mode assertions; run go test ./... before handoff.
Sources: Coding guidelines, Pipeline failures
The local-mode half of TestTeamModeNeverRefreshesLocalAccounts asserted that a server without a broker still refreshes. It passed locally only because reloadAccounts re-reads the account store and this machine has 16 real accounts in it; CI has none, so nothing was refreshed and the assertion failed. The test now writes its own OAuth accounts into a temp store. Verified with an empty HOME, which is what CI has.
This was referenced Aug 1, 2026
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.
Why
OpenAI rotates a Codex refresh token on every use and invalidates the previous one. A credential can therefore have exactly one refresher, and two of them destroy each other.
That is what happened in production: a laptop and a mac mini shared one account store, both refreshed independently, and 11 of 12 accounts now fail with
refresh_token_reused, some recorded as far back as 2026-05-25.sr statuson both hosts shows the same error for the same accounts. Re-adding accounts into that arrangement just kills them again, so this is a precondition for the hosted-vault migration rather than a cleanup.What was wrong
Team mode already leases from the broker and never sends a local refresh token upstream. But the local scheduler still refreshed every OAuth account to score usage, on two ungated paths:
reloadAccounts→scoreAccountsrefreshUsageScoresIfStale→scoreAccountsscoreAccountsrefreshes each OAuth account before fetching usage. So a team-mode machine kept rotating tokens the vault owns, invalidating them for the vault, while the vault invalidated them here. Someone had already noticed the scheduler is irrelevant under a broker (proxy.gogatesNoteRoutedonCredentialBroker == nil); the scoring paths were simply missed.Changes
Both refresh paths return early when a credential broker is configured. Local mode is untouched and still refreshes, since there it is the only refresher.
Upload now hands over ownership as well as bytes.
sr account importmoves each uploaded local credential to<store>/migrated/, whichListStoreddoes not scan, so the daemon stops refreshing it. The file stays readable for rollback and the command prints where each record went. The old wording promised "local files remain only as migration records" while leaving them fully refreshable; now that is actually true.Tests
TestTeamModeNeverRefreshesLocalAccountscovers both paths under a broker and asserts zero refreshes, plus a third case asserting local mode still refreshes so the gate cannot be widened into breaking scoring. Verified it fails without the gate (team mode refreshed 1 local accounts, want 0).TestMigrateStoredAwayDisownsButKeepsRecordasserts the account leaves the active store, the record remains readable, and the directory is 0700.go test ./internal/... ./cmd/...passes.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Stop local refresh of shared OAuth credentials in team mode and hand off ownership on upload to prevent
refresh_token_reusedand broken token chains.Bug Fixes
CredentialBrokerset), short-circuitreloadAccountsandrefreshUsageScoresIfStaleso no local OAuth refresh occurs; local mode unchanged. Tests now seed a temp store to make refresh behavior deterministic in CI.Migration
sr account importnow moves uploadedcodexcredentials to<store>/migrated/(0700). The store does not scan this dir, so the daemon stops refreshing them. The rollback record remains and the path is printed.Written for commit a766657. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes