feat(codex): add exact account routing - #671
Conversation
📝 WalkthroughWalkthroughThe PR adds account-qualified Codex namespace routing, exact account authentication, cooldown fail-closed behavior, fixed-account outcome handling, and forwarding coverage across native, compact, WebSocket, search, retry, and subagent fallback paths. ChangesFixed Codex account flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant routeModelInternal
participant resolveCodexAuthContext
participant handleResponses
participant recordCodexUpstreamOutcome
Client->>routeModelInternal: submit namespace-qualified model
routeModelInternal-->>Client: return Codex account binding
handleResponses->>resolveCodexAuthContext: resolve exact account
resolveCodexAuthContext-->>handleResponses: return fixed auth context
handleResponses->>recordCodexUpstreamOutcome: record upstream result
recordCodexUpstreamOutcome-->>handleResponses: preserve fixed account selection
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Heads-up on a policy change that affects the port note in this PR's description: the
Nothing is required from you — this PR is unaffected other than that the port line in the description no longer applies. Sorry for the mixed signals while the policy was in flux. |
6842737 to
1165ac2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1165ac2817
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const namespace = modelId.slice(0, slash); | ||
| const binding = codexAccountNamespaceEntries(config) | ||
| .find(([candidate]) => candidate === namespace); | ||
| if (binding) { |
There was a problem hiding this comment.
Once this branch returns a routed codexAccountId, codexAccountNamespaces is no longer inert: requests like side/gpt-5.6-sol now pin traffic to that account. The reference docs, including translated copies, still say the map does not change routing, so users following the published configuration docs will either miss the new selector behavior or assume the setting is safe to configure without affecting traffic. Please update docs-site with the new account-qualified routing semantics.
AGENTS.md reference: src/AGENTS.md:L28-L28
Useful? React with 👍 / 👎.
| throw new Error(`Codex account namespace ${namespace} only supports native OpenAI model ids`); | ||
| } | ||
| const provider = config.providers[OPENAI_CODEX_PROVIDER_ID]; | ||
| if (!provider || provider.disabled === true || !isCanonicalOpenAiForwardProvider(provider)) { |
There was a problem hiding this comment.
For valid configs where providers.openai contains the canonical adapter/baseUrl but relies on the registry backfill for authMode, bare GPT routing still works because routeResult() canonicalizes the row to forward auth, but account-qualified models fail here with NoEnabledOpenAiProviderError because this check inspects the raw provider before that backfill. Please either canonicalize before checking or treat an omitted authMode on the canonical OpenAI row as forward-compatible.
AGENTS.md reference: src/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
| const resolution = fixedAccountId !== undefined | ||
| ? { status: "selected" as const, accountId: fixedAccountId } |
There was a problem hiding this comment.
When a side/gpt-5.6-sol turn later triggers a standalone ChatGPT sidecar request such as /v1/alpha/search or image generation, those handlers call resolveFirstUsableOpenAiSidecar() using only the sidecar request headers and existing thread affinity. This fixed-account branch bypasses the normal resolveCodexAccountForThreadDetailed() path that would bind affinity, so the sidecar has no record of side -> pool-a and can run on the active Pool account instead (for example pool-b), splitting one user-selected account-qualified turn across accounts. Please carry the exact account binding into sidecar resolution or record sidecar-safe thread state for these requests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/codex/auth-context.ts`:
- Around line 156-172: Update the WebSocket cooldown handling in the server flow
around the call to cooldownErrorMessage so it passes route.codexAccountNamespace
as the accountSelector argument. Preserve the existing CodexAccountCooldownError
handling while ensuring pinned requests use selector-specific guidance rather
than account-label and account-id recovery text.
In `@tests/codex-routing.test.ts`:
- Around line 121-139: The fixed-account failure test must also verify thread
affinity remains pinned, not just the active pool account. In the test covering
fixed-account 503 and 429 outcomes, seed the `"fixed-thread"` binding for each
config before calling recordCodexUpstreamOutcome, then assert
resolveCodexAccountForThread("fixed-thread", config) still returns "a" after
both branches.
In `@tests/router.test.ts`:
- Around line 135-151: Extend the providers matrix in the test covering
namespace credential injection to include entries with the canonical provider
disabled and with no provider configured, asserting both still throw
NoEnabledOpenAiProviderError. Preserve the existing three canonical-provider
cases and their fail-closed expectations.
In `@tests/subagent-fallback-handle-responses.test.ts`:
- Around line 205-238: Update the test setup around poolNativePlusRoutedConfig
and updateAccountQuota so activeCodexAccountId points to a different pool than
the side namespace, and add credentials for that active pool following the
existing server-auth test pattern. Keep the accounts assertion expecting
pool_acc so it verifies requests honor side → pool-a routing rather than default
pool selection.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 984ce8f2-b885-4226-a357-2ef24de41d28
📒 Files selected for processing (10)
src/codex/auth-context.tssrc/codex/routing.tssrc/router.tssrc/server/responses/compact.tssrc/server/responses/core.tstests/codex-auth-context.test.tstests/codex-routing.test.tstests/router.test.tstests/server-auth.test.tstests/subagent-fallback-handle-responses.test.ts
1165ac2 to
d433181
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/codex/routing.ts`:
- Line 1281: Make clearThreadAccountMapForAccount(accountId) run unconditionally
after markAccountNeedsReauth(accountId), while retaining the !meta.fixedAccount
guard for rotation/promotion logic. In src/codex/routing.ts lines 1281-1281,
remove the guard only from the thread-map clearing call; lines 1314-1314,
1359-1372, and 1421-1423 require no direct changes. Ensure
resolveCodexAccountForThreadDetailed() cannot reuse a pinned account marked for
reauthentication.
In `@tests/codex-routing.test.ts`:
- Around line 121-151: Add a third block to the existing exact-account failure
test covering a 401 or 403 credential failure, mirroring the transient and quota
cases. Verify the account is marked as needing reauthentication via
isAccountNeedsReauth while the fixed thread continues resolving to that account
after activeCodexAccountId changes and health is cleared; clean up with
clearCodexNeedsReauth (or the file’s existing equivalent). Update imports to use
the established reauthentication helpers.
In `@tests/server-search.test.ts`:
- Around line 295-320: Add a sibling test beside “an unavailable exact search
account fails closed without dispatching the active Pool account” that marks the
selected exact account as needing reauthentication and verifies a 401 with the
selected-account reauthentication message and no upstream dispatch. Update the
exact-account handling in the search request path and its OpenAI sidecar result
handling so unusable/reauthentication failures are reported as selected-account
state, rather than falling through to the generic missing Authorization-header
error.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b71720bf-81b6-4ece-81d5-cdf21519d0b2
📒 Files selected for processing (21)
docs-site/src/content/docs/ja/reference/configuration.mddocs-site/src/content/docs/ko/reference/configuration.mddocs-site/src/content/docs/reference/configuration.mddocs-site/src/content/docs/ru/reference/configuration.mddocs-site/src/content/docs/zh-cn/reference/configuration.mdsrc/codex/auth-context.tssrc/codex/routing.tssrc/providers/openai-sidecar.tssrc/router.tssrc/server/index.tssrc/server/responses/compact.tssrc/server/responses/core.tssrc/server/search.tsstructure/04_transports-and-sidecars.mdtests/codex-auth-context.test.tstests/codex-routing.test.tstests/router.test.tstests/server-auth.test.tstests/server-search.test.tstests/subagent-fallback-handle-responses.test.tstests/web-search.test.ts
| quotaScopedHealth.delete(accountId); | ||
| markAccountNeedsReauth(accountId); | ||
| clearThreadAccountMapForAccount(accountId); | ||
| if (!meta.fixedAccount) clearThreadAccountMapForAccount(accountId); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Does thread-affinity resolution reject accounts in cooldown or flagged needs-reauth?
# If yes, the suppressed global sweep is harmless and only needs a comment.
ast-grep outline src/codex/routing.ts --match 'resolveCodexAccountForThread' --items all
rg -nP -C25 'function resolveCodexAccountForThreadDetailed' src/codex/routing.ts
rg -nP -C4 'isAccountNeedsReauth|getCodexAccountCooldownUntil|selectable' src/codex/routing.ts | head -80Repository: lidge-jun/opencodex
Length of output: 6343
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact selection gates and the affected cleanup sites.
rg -n -C8 'function isCodexAccountSelectable|export function isCodexAccountSelectable|clearThreadAccountMapForAccount\(|markAccountNeedsReauth\(|notePoolRotationFailure\(' src/codex/routing.ts
# Read the surrounding implementation for the selection predicate.
sed -n '240,360p' src/codex/routing.ts
# Read the affected blocks around the cited lines.
sed -n '1260,1445p' src/codex/routing.tsRepository: lidge-jun/opencodex
Length of output: 19672
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Also inspect any thread-affinity helpers that use the same accountability logic.
rg -n -C6 'getThreadAffinity|setThreadAffinity|deleteThreadAffinity|isThreadAffinityExpired|isThreadAffinityGenerationLive' src/codex/routing.tsRepository: lidge-jun/opencodex
Length of output: 4509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the exact implementation of thread pin cleanup to understand scope.
rg -n -C8 'function clearThreadAccountMapForAccount|export function clearThreadAccountMapForAccount' src/codex/routing.tsRepository: lidge-jun/opencodex
Length of output: 761
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the file is small enough, inspect the helper that decides whether affinity can be reused.
sed -n '1000,1165p' src/codex/routing.tsRepository: lidge-jun/opencodex
Length of output: 7258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether account usability/selectability already rejects reauth-needed accounts.
rg -n -C6 'function isCodexAccountUsable|export function isCodexAccountUsable|isAccountNeedsReauth\(' src/codex/*.tsRepository: lidge-jun/opencodex
Length of output: 6328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the pool-candidate filters that were found in the earlier search.
sed -n '680,725p' src/codex/routing.ts
sed -n '730,780p' src/codex/routing.tsRepository: lidge-jun/opencodex
Length of output: 3748
Keep the reauth sweep unconditional in src/codex/routing.ts#L1280-L1281
markAccountNeedsReauth(accountId) only updates runtime state; resolveCodexAccountForThreadDetailed() can still reuse an existing thread pin unless the account is paused, quota-cooled, soft-avoided, or failing over, and that path never checks isAccountNeedsReauth(). New pool picks already exclude reauth-needed accounts, but already-pinned threads can stay stuck on the bad account until threadAccountMap is cleared. Leave the !meta.fixedAccount guard on the rotation/promotion logic, but run clearThreadAccountMapForAccount(accountId) here unconditionally.
📍 Affects 1 file
src/codex/routing.ts#L1281-L1281(this comment)src/codex/routing.ts#L1314-L1314src/codex/routing.ts#L1359-L1372src/codex/routing.ts#L1421-L1423
🤖 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 `@src/codex/routing.ts` at line 1281, Make
clearThreadAccountMapForAccount(accountId) run unconditionally after
markAccountNeedsReauth(accountId), while retaining the !meta.fixedAccount guard
for rotation/promotion logic. In src/codex/routing.ts lines 1281-1281, remove
the guard only from the thread-map clearing call; lines 1314-1314, 1359-1372,
and 1421-1423 require no direct changes. Ensure
resolveCodexAccountForThreadDetailed() cannot reuse a pinned account marked for
reauthentication.
…ccount-routing # Conflicts: # src/server/responses/core.ts
|
NEEDS-SECURITY-REVIEW — no code blocker found; the boundary is what needs sign-off. This merges cleanly against the current The failure semantics are the right ones. A fixed-account failure does not rotate the active pool account or trigger alternate retry ( Two details I want to credit specifically. The sidecar accepts exact-account data only from the validated route and never from client headers ( Test coverage is at the boundary rather than the unit: 400/402/429 preserving the account without rotating, compact and WebSocket preserving the credential and the bare wire model, Search validating and stripping the selector with no active-account mutation and no private ID in logs. Why it still needs review. Landing order. #715 (account pool priority) touches What happens next: request explicit security review. |
|
@Ingwannu — could you give |
…ccount-routing # Conflicts: # src/codex/auth-context.ts # src/server/responses/core.ts # tests/codex-auth-context.test.ts
Part of #425. This is the second layer split out of #426 after the maintainer review; the namespace foundation is already on
devasc9bed7c5a.What this adds
An account-qualified native model now routes to the one stored Codex account mapped by its namespace. For example,
side/gpt-5.6-soluses thesidebinding and sendsgpt-5.6-solon the wire through the canonicalopenaiprovider.gpt-*models keep their existing Direct or Pool behavior.The binding is carried through Responses HTTP/SSE, compact, WebSocket
response.create, the translated chat/Claude paths that use the shared Responses core, and/v1/alpha/searchwhen that request retains the qualified model selector. Standalone Images and Live requests do not currently carry a safe selector/thread correlation and retain normal provider routing.The latest
devpause controls are respected: pausing an account blocks new exact selections without invalidating an already-started request.Not in this PR
Catalog generation/model-picker rows, account lifecycle and settings UI, standalone Images/Live correlation, and account-qualified fallback-chain candidates remain separate follow-up layers. Keeping those out is intentional so this review stays focused on the credential-routing boundary.
Security review
This changes credential-to-model routing and therefore needs the explicit maintainer security review required by
MAINTAINERS.md.The main invariants are covered by regressions for Direct override, exact main and added accounts, missing/paused/cooled/reauthentication-required accounts, 400/401/402/429 handling, HTTP/SSE, compact, WebSocket, account-qualified Search, public-only logging, quota probes, active Pool-account preservation, credential-affinity cleanup, and spawned-child fallback behavior.
Validation
bun run prepushSummary by CodeRabbit
/v1/alpha/search, compact HTTP, and WebSocket flows).codexAccountNamespacesdocs to clarify routing behavior, fail-closed semantics, and privacy expectations.