Repository navigation
fix(sidecars): rotate generic OAuth accounts on 429 (#2568) - #2607
Merged
Merged
Conversation
Widen the image and web-search on429 hooks to accept an awaitable adapter and await them at both consumption sites. Existing synchronous key-pool callbacks remain source-compatible and are still exercised unchanged. Option (a) is required because failoverAccountSnapshot may refresh a token asynchronously. Awaiting the hook preserves full snapshot atomicity (including Antigravity projectId and Kiro routing metadata) without eager refresh work, blocking tricks, or a fire-and-forget race. The shared sidecar callback tries key-pool rotation first, then bounded generic OAuth rotation, retaining the existing OpenAI/Anthropic exclusions and single-account no-op.
…ey-pool-only The two loop tests only prove the loops AWAIT the hook; they stub it and never reach the OAuth branch core.ts injects. The hook is a closure over request-local state, so it is not importable, and driving it end to end means a full sidecar request against a stubbed provider. These are structural assertions on the source, like the route-inventory contract: they cannot prove rotation works, but they catch the regression that actually threatens this feature - one sidecar keeping a key-pool-only hook while the other gets the OAuth-aware one. That divergence is how the gap arose in the first place. Falsified both ways: inlining a key-pool-only hook at the web-search site fails the identity assertion (expected 1 distinct hook, received 2); removing the knob gate fails the gate assertion.
Contributor
|
✅ Deterministic PR hygiene checks passed. |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughImage and web-search 429 handling now supports asynchronous adapter rotation. Both bridges use shared sidecar failover logic that can rotate transport keys, fail over generic OAuth accounts, refresh credentials, rebuild adapters, and restore reasoning replay scope. ChangesSidecar provider failover
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ImageBridge
participant WebSearchBridge
participant rotateSidecarProviderOn429
participant ProviderAdapter
ImageBridge->>rotateSidecarProviderOn429: handle 429
WebSearchBridge->>rotateSidecarProviderOn429: handle 429
rotateSidecarProviderOn429->>ProviderAdapter: rotate transport or rebuild OAuth adapter
ProviderAdapter-->>rotateSidecarProviderOn429: replacement adapter
rotateSidecarProviderOn429-->>ImageBridge: awaited adapter result
rotateSidecarProviderOn429-->>WebSearchBridge: awaited adapter result
Suggested reviewers: ✨ Finishing Touches📝 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1640b6b40
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const snapshot = await failoverAccountSnapshot(route.providerName, nextAccountId); | ||
| genericFailoverAccountId = nextAccountId; | ||
| genericFailovers += 1; | ||
| route.provider = { ...route.provider, apiKey: snapshot.accessToken }; |
There was a problem hiding this comment.
When two github-copilot OAuth accounts have different stored endpoints.api origins, this swaps only the bearer while preserving the baseUrl resolved for the original account at src/server/responses/core.ts:2821-2825. Because failoverAccountSnapshot also omits the account-scoped apiBaseUrl (src/oauth/index.ts:53-66), the retry sends the replacement account's token to the previous account's Copilot endpoint and can fail with 401 instead of recovering. Include the validated Copilot origin in the failover snapshot and run the replacement through resolveProviderTransport before rebuilding the adapter.
AGENTS.md reference: src/AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
lidge-jun
added a commit
that referenced
this pull request
Aug 26, 2026
…(#2568d) (#2640) * feat(oauth): activate multi-account 429 failover on account presence (#2568d) The mechanism shipped in #2590/#2607/#2608 but stayed gated behind oauthAccountFailover.enabled, which is false for every install that never edited its config — so the workflow #2568 actually reported (three xAI accounts logged in, a 429 on the active one) was still broken by default. The owner has now settled the escalation recorded in the wp7 audit response: always on. Presence is the activation signal, not an unconditional true. The predicate also decides whether a runTurn stream is wrapped in preflightRunTurnFailover, and that wrapper can never rotate for a single-account install, so an unconditional true would add a preflight for users who get nothing from it. Two or more eligible accounts is the same consent rule hasKeyPoolFailover already applies to a 2+ key pool. Explicit config still wins, now at two levels: providers.<name>.oauthAccountFailover beats the global knob, and both beat presence. Default-off was hiding three defects in the rotation path that presence-driven activation would have handed to every multi-account user: - Copilot pins its bearer to an account-scoped regional origin. core.ts pairs them on the initial resolve, but OAuthAccessSnapshot did not carry apiBaseUrl, so a rotation sent account B's token to account A's host. resolveGithubCopilotTransport fails closed to the canonical host, which is why this was misrouting rather than a leak — it was still wrong. - Antigravity rotation wrote project only when the new snapshot had one, so a project-less account inherited the FAILED account's project. That account is reachable: refresh tolerates project discovery failing. - The presence check would have run loadAuthStore on paths that never see a 429, and that function chmods, reads, parses and normalizes the whole store every call. The first two shared a root cause: three rotation sites each inlined the same credential swap, and the duplication is what let the paired metadata drift. They now go through one applyFailoverSnapshot helper that assembles the whole identity or refuses — a cloud-code-assist snapshot with no project aborts the rotation rather than mixing two accounts. Presence is answered from a TTL-bounded per-provider count, invalidated on rotation; it caches a number, never a credential. Separately, upsertOAuthProvider rebuilds the provider row from the registry preset on every login. Without preservation the sequence was: operator sets enabled false, operator adds a second account, and that one action both deletes the opt-out and creates the quorum that turns rotation on. Tests: activation matrix (no config, global both ways, per-provider both ways, reauth-flagged second account, cache TTL), structural contracts pinning that the bearer is written in exactly one place and that the helper fails closed, an explicit-opt-out e2e case, and an incremental SSE read proving the first delta reaches the client before completion. Both new contracts were driven red: forcing preflight to buffer the whole turn fails the delivery test at its 5s bound, and disabling the preservation branch fails both upsert cases. Closes #2568 * fix(gui): translate the alias catalog keys and unbreak the locale parity parser The gates job was red on dev before this branch existed, on four locale tests with two unrelated causes. Three of them read a catalog by regex anchored at "^\s*", but these files pack several entries onto one line. The parser therefore saw only the first entry per line and reported "models.newBadge" missing from every non-English locale — a key that is present in all of them. The catalogs were identical; the check was not reading them. Dropping the line anchor fixes all three, and the module-level check in i18n-locales.test.ts already agreed the key sets match, which is what made the phantom visible. The fourth was real: #2463 landed thirteen alias keys with the English string copied into all eight locales. They are now translated. "models.aliasAuto" is the exception and joins the intentional-English allowlist, because "auto" is the same word in French and labels a machine-derived alias source rather than prose. gui: bun test 994 pass, 0 fail (was 990/4).
tarunravi
pushed a commit
to tarunravi/opencodex
that referenced
this pull request
Sep 14, 2026
…idge-jun#2607) * fix(sidecars): rotate generic OAuth accounts on 429 Widen the image and web-search on429 hooks to accept an awaitable adapter and await them at both consumption sites. Existing synchronous key-pool callbacks remain source-compatible and are still exercised unchanged. Option (a) is required because failoverAccountSnapshot may refresh a token asynchronously. Awaiting the hook preserves full snapshot atomicity (including Antigravity projectId and Kiro routing metadata) without eager refresh work, blocking tricks, or a fire-and-forget race. The shared sidecar callback tries key-pool rotation first, then bounded generic OAuth rotation, retaining the existing OpenAI/Anthropic exclusions and single-account no-op. * test(sidecars): pin the shared on429 hook so neither sidecar drifts key-pool-only The two loop tests only prove the loops AWAIT the hook; they stub it and never reach the OAuth branch core.ts injects. The hook is a closure over request-local state, so it is not importable, and driving it end to end means a full sidecar request against a stubbed provider. These are structural assertions on the source, like the route-inventory contract: they cannot prove rotation works, but they catch the regression that actually threatens this feature - one sidecar keeping a key-pool-only hook while the other gets the OAuth-aware one. That divergence is how the gap arose in the first place. Falsified both ways: inlining a key-pool-only hook at the web-search site fails the identity assertion (expected 1 distinct hook, received 2); removing the knob gate fails the gate assertion.
tarunravi
pushed a commit
to tarunravi/opencodex
that referenced
this pull request
Sep 14, 2026
…(#2568d) (lidge-jun#2640) * feat(oauth): activate multi-account 429 failover on account presence (#2568d) The mechanism shipped in lidge-jun#2590/lidge-jun#2607/lidge-jun#2608 but stayed gated behind oauthAccountFailover.enabled, which is false for every install that never edited its config — so the workflow lidge-jun#2568 actually reported (three xAI accounts logged in, a 429 on the active one) was still broken by default. The owner has now settled the escalation recorded in the wp7 audit response: always on. Presence is the activation signal, not an unconditional true. The predicate also decides whether a runTurn stream is wrapped in preflightRunTurnFailover, and that wrapper can never rotate for a single-account install, so an unconditional true would add a preflight for users who get nothing from it. Two or more eligible accounts is the same consent rule hasKeyPoolFailover already applies to a 2+ key pool. Explicit config still wins, now at two levels: providers.<name>.oauthAccountFailover beats the global knob, and both beat presence. Default-off was hiding three defects in the rotation path that presence-driven activation would have handed to every multi-account user: - Copilot pins its bearer to an account-scoped regional origin. core.ts pairs them on the initial resolve, but OAuthAccessSnapshot did not carry apiBaseUrl, so a rotation sent account B's token to account A's host. resolveGithubCopilotTransport fails closed to the canonical host, which is why this was misrouting rather than a leak — it was still wrong. - Antigravity rotation wrote project only when the new snapshot had one, so a project-less account inherited the FAILED account's project. That account is reachable: refresh tolerates project discovery failing. - The presence check would have run loadAuthStore on paths that never see a 429, and that function chmods, reads, parses and normalizes the whole store every call. The first two shared a root cause: three rotation sites each inlined the same credential swap, and the duplication is what let the paired metadata drift. They now go through one applyFailoverSnapshot helper that assembles the whole identity or refuses — a cloud-code-assist snapshot with no project aborts the rotation rather than mixing two accounts. Presence is answered from a TTL-bounded per-provider count, invalidated on rotation; it caches a number, never a credential. Separately, upsertOAuthProvider rebuilds the provider row from the registry preset on every login. Without preservation the sequence was: operator sets enabled false, operator adds a second account, and that one action both deletes the opt-out and creates the quorum that turns rotation on. Tests: activation matrix (no config, global both ways, per-provider both ways, reauth-flagged second account, cache TTL), structural contracts pinning that the bearer is written in exactly one place and that the helper fails closed, an explicit-opt-out e2e case, and an incremental SSE read proving the first delta reaches the client before completion. Both new contracts were driven red: forcing preflight to buffer the whole turn fails the delivery test at its 5s bound, and disabling the preservation branch fails both upsert cases. Closes lidge-jun#2568 * fix(gui): translate the alias catalog keys and unbreak the locale parity parser The gates job was red on dev before this branch existed, on four locale tests with two unrelated causes. Three of them read a catalog by regex anchored at "^\s*", but these files pack several entries onto one line. The parser therefore saw only the first entry per line and reported "models.newBadge" missing from every non-English locale — a key that is present in all of them. The catalogs were identical; the check was not reading them. Dropping the line anchor fixes all three, and the module-level check in i18n-locales.test.ts already agreed the key sets match, which is what made the phantom visible. The fourth was real: lidge-jun#2463 landed thirteen alias keys with the English string copied into all eight locales. They are now translated. "models.aliasAuto" is the exception and joins the intentional-English allowlist, because "auto" is the same word in French and labels a machine-derived alias source rather than prose. gui: bun test 994 pass, 0 fail (was 990/4).
agentHits
pushed a commit
to agentHits/opencodex
that referenced
this pull request
Sep 17, 2026
…idge-jun#2607) * fix(sidecars): rotate generic OAuth accounts on 429 Widen the image and web-search on429 hooks to accept an awaitable adapter and await them at both consumption sites. Existing synchronous key-pool callbacks remain source-compatible and are still exercised unchanged. Option (a) is required because failoverAccountSnapshot may refresh a token asynchronously. Awaiting the hook preserves full snapshot atomicity (including Antigravity projectId and Kiro routing metadata) without eager refresh work, blocking tricks, or a fire-and-forget race. The shared sidecar callback tries key-pool rotation first, then bounded generic OAuth rotation, retaining the existing OpenAI/Anthropic exclusions and single-account no-op. * test(sidecars): pin the shared on429 hook so neither sidecar drifts key-pool-only The two loop tests only prove the loops AWAIT the hook; they stub it and never reach the OAuth branch core.ts injects. The hook is a closure over request-local state, so it is not importable, and driving it end to end means a full sidecar request against a stubbed provider. These are structural assertions on the source, like the route-inventory contract: they cannot prove rotation works, but they catch the regression that actually threatens this feature - one sidecar keeping a key-pool-only hook while the other gets the OAuth-aware one. That divergence is how the gap arose in the first place. Falsified both ways: inlining a key-pool-only hook at the web-search site fails the identity assertion (expected 1 distinct hook, received 2); removing the knob gate fails the gate assertion.
agentHits
pushed a commit
to agentHits/opencodex
that referenced
this pull request
Sep 17, 2026
…(#2568d) (lidge-jun#2640) * feat(oauth): activate multi-account 429 failover on account presence (#2568d) The mechanism shipped in lidge-jun#2590/lidge-jun#2607/lidge-jun#2608 but stayed gated behind oauthAccountFailover.enabled, which is false for every install that never edited its config — so the workflow lidge-jun#2568 actually reported (three xAI accounts logged in, a 429 on the active one) was still broken by default. The owner has now settled the escalation recorded in the wp7 audit response: always on. Presence is the activation signal, not an unconditional true. The predicate also decides whether a runTurn stream is wrapped in preflightRunTurnFailover, and that wrapper can never rotate for a single-account install, so an unconditional true would add a preflight for users who get nothing from it. Two or more eligible accounts is the same consent rule hasKeyPoolFailover already applies to a 2+ key pool. Explicit config still wins, now at two levels: providers.<name>.oauthAccountFailover beats the global knob, and both beat presence. Default-off was hiding three defects in the rotation path that presence-driven activation would have handed to every multi-account user: - Copilot pins its bearer to an account-scoped regional origin. core.ts pairs them on the initial resolve, but OAuthAccessSnapshot did not carry apiBaseUrl, so a rotation sent account B's token to account A's host. resolveGithubCopilotTransport fails closed to the canonical host, which is why this was misrouting rather than a leak — it was still wrong. - Antigravity rotation wrote project only when the new snapshot had one, so a project-less account inherited the FAILED account's project. That account is reachable: refresh tolerates project discovery failing. - The presence check would have run loadAuthStore on paths that never see a 429, and that function chmods, reads, parses and normalizes the whole store every call. The first two shared a root cause: three rotation sites each inlined the same credential swap, and the duplication is what let the paired metadata drift. They now go through one applyFailoverSnapshot helper that assembles the whole identity or refuses — a cloud-code-assist snapshot with no project aborts the rotation rather than mixing two accounts. Presence is answered from a TTL-bounded per-provider count, invalidated on rotation; it caches a number, never a credential. Separately, upsertOAuthProvider rebuilds the provider row from the registry preset on every login. Without preservation the sequence was: operator sets enabled false, operator adds a second account, and that one action both deletes the opt-out and creates the quorum that turns rotation on. Tests: activation matrix (no config, global both ways, per-provider both ways, reauth-flagged second account, cache TTL), structural contracts pinning that the bearer is written in exactly one place and that the helper fails closed, an explicit-opt-out e2e case, and an incremental SSE read proving the first delta reaches the client before completion. Both new contracts were driven red: forcing preflight to buffer the whole turn fails the delivery test at its 5s bound, and disabling the preservation branch fails both upsert cases. Closes lidge-jun#2568 * fix(gui): translate the alias catalog keys and unbreak the locale parity parser The gates job was red on dev before this branch existed, on four locale tests with two unrelated causes. Three of them read a catalog by regex anchored at "^\s*", but these files pack several entries onto one line. The parser therefore saw only the first entry per line and reported "models.newBadge" missing from every non-English locale — a key that is present in all of them. The catalogs were identical; the check was not reading them. Dropping the line anchor fixes all three, and the module-level check in i18n-locales.test.ts already agreed the key sets match, which is what made the phantom visible. The fourth was real: lidge-jun#2463 landed thirteen alias keys with the English string copied into all eight locales. They are now translated. "models.aliasAuto" is the exception and joins the intentional-English allowlist, because "auto" is the same word in French and labels a machine-derived alias source rather than prose. gui: bun test 994 pass, 0 fail (was 990/4).
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.
Summary
Completes the sidecar half of #2568. Generic OAuth account rotation on 429 already worked on the main response path, but the image bridge and the web-search loop each received an
on429hook that only calledrotateProviderTransportOn429— key-pool-only, sincehasKeyPoolFailoverreturns false forauthMode: "oauth". So a provider with three logged-in accounts recovered from a 429 on a normal turn and failed on an image or web-search turn.The blocking constraint was that
on429is synchronous (ProviderAdapter | null) while OAuth rotation needsfailoverAccountSnapshot, which may refresh a token. The contract is widened toProviderAdapter | null | Promise<ProviderAdapter | null>and both loops await it; every existing synchronous key-pool caller keeps working unchanged.Both injection sites now share one hook. It tries the key pool first and unconditionally, so an API-key provider behaves exactly as before, and only falls through to the OAuth roster when the key pool declines. The rotation swaps in the FULL credential snapshot rather than a bare bearer — Kiro carries routing metadata and Antigravity pairs an account-matched
projectIdwith its token, so a token-only swap would mix one account's credential with another's routing data.Codex and Anthropic stay excluded through
isGenericFailoverProvider: their pools own quota scopes, probe leases and affinity that this must not reimplement. A single-account provider is a strict no-op, and the whole path stays behindoauthAccountFailover.enabled.Verification
Merged onto
devatf392e02eband verified there, not on the branch's own base:Falsification, all four directions:
awaitremoved → fails, expected 2 fetches received 1awaitremoved → fails, expected 200 received 502The last two come from a test added on review. The two loop tests stub
on429and therefore never reach the OAuth branch; the hook is a closure over request-local state (route,genericFailoverAccountId,genericFailovers) and is not importable. The added assertions are structural, in the same spirit as the route-inventory contract — they cannot prove rotation works, but they do catch the regression that actually threatens this feature: one sidecar silently keeping the key-pool-only hook while the other gets the OAuth-aware one. That is exactly how the gap arose in the first place.Checklist
devCloses part of #2568.
Summary by CodeRabbit
New Features
Bug Fixes
Tests