fix(codex): bounded single pool retry on account-specific unsupported-model 400 (#335)

[lidge-jun]

Summary

Fixes #335: in Pool mode, an account-specific unsupported-model 400 from the ChatGPT backend was returned directly to the client even when another pooled account could serve the model. The proxy now performs exactly ONE bounded retry with a different eligible pooled account.

Design (intentional refinement of the 35d28a0 policy)

The 4xx = caller fault, no failover classification from 35d28a0 is deliberately preserved: this change adds a narrow, allow-listed exception that never touches account health.

• Allow-list matcher: string equality (after whitespace/case normalization) against the exact backend sentence The '<requested routed model id>' model is not supported when using Codex with a ChatGPT account. — the interpolated model id must match the routed request. No substring/keyword/regex matching; regex injection is structurally impossible.
• Bounded inspection: the 400 body is inspected via a bounded clone read; timeout, truncation, oversize, or display-unsafe outcomes never authorize a retry and the original bytes reach the client unchanged.
• Hard cap: one retry, to a different credential/health-eligible account (request-local exclusion — global selection state is untouched). No eligible alternate → original 400 returned. Second allow-listed 400 → returned, no third dispatch.
• Zero health mutation: the pinned regression caller and model 4xx responses do not penalize account health stays green and unmodified (src/codex/routing.ts untouched).
• Retry window: strictly before any client-facing Response/relay construction, in both stream modes (legacy-tee and eager bounded relay).
• WebSocket rebind: a successful retry republishes the resolved auth context so the WS account registry migrates A→B (activation-tested with registry snapshots).

Tests

Activations A–E plus a 10-test negative matrix (8 hostile-body classes + 2 retry-failure paths) as named tests in tests/server-auth.test.ts / tests/codex-auth-context.test.ts (+483 lines):

• typecheck exit 0; focused 129 pass / 0 fail; full suite 3879 pass / 0 fail (313 files); privacy:scan pass.

Security review

This touches pool account-selection logic (MAINTAINERS.md security boundary). Self-audit: exact model-bound equality, bounded clone inspection, single alternate dispatch, unchanged caller-health policy, no new body/token/account logging. Explicit maintainer security review requested before merge.

Design/audit trail: devlog/_plan/260724_bugfix_train/020_pool_retry_400.md (security-focused adversarial audit, 2 rounds).

Closes #335.

[coderabbitai]

Warning

Review limit reached

@lidge-jun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 1 minute

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b12c4287-9455-4b9a-9659-c9e857992125

📥 Commits

Reviewing files that changed from the base of the PR and between 05ee9ec and 8e2b247.

📒 Files selected for processing (4)

• src/codex/auth-context.ts
• src/server/responses/core.ts
• tests/codex-auth-context.test.ts
• tests/server-auth.test.ts

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/260724-pool-retry

---

_{Comment @coderabbitai help to get the list of available commands.}

[Wibias]

Dual review (security + bugs/edge cases)

No unresolved bot review threads (CodeRabbit rate-limited; 0 inline comments). CI is green. This touches pool account-selection (MAINTAINERS.md security boundary) — independent security + edge-case pass below.

Security categories checked

No malware, backdoor, hidden C2, local admission bypass, classic XSS, or new credential/body logging. Retry authorization is server-side only: the allow-list matches a cloned, bounded upstream 400 body against an exact model-bound sentence (whitespace/case normalized), never against client-supplied text. Regex injection is structurally avoided (string equality after normalize). recordCodexUpstreamOutcome(..., 400) is a no-op for health (caller class early-returns in src/codex/routing.ts), so the “zero health mutation” claim holds. Failed alternate resolution fails closed to the original 400.

Blockers

None for merge from a security-boundary perspective, assuming the sticky-affinity tradeoff below is accepted or fixed.

Should-fix before merge

1. Successful retry does not rebind thread affinity (Medium) — src/codex/auth-context.ts + src/server/responses/core.ts
   With excludeAccountId, selection uses pickLowestUsageCodexAccount and intentionally skips resolveCodexAccountForThreadDetailed / bindThreadAffinity (tests assert activeCodexAccountId stays on A). After A→B succeeds, HTTP thread affinity can remain on A, so every later turn on that thread pays another A 400 + B retry. WS registry migration is covered; HTTP affinity is not.
   Prefer: on successful alternate response, rebind affinity for x-codex-parent-thread-id to B (still without health penalties / without changing global active unless you want that).

2. Retry dispatch skips transient-5xx absorption (Medium) — src/server/responses/core.ts
   The first hop uses fetchWithTransientRetry; the alternate hop uses bare fetchWithHeaderTimeout. A flaky 502/520 on B becomes an immediate client failure instead of the same short absorb window. Wrap the retry fetch in the same transient helper (still hard-capped at one account switch).

3. Hardcoded "pool" on retry (Low) — retry calls resolveCodexAuthContext(..., "pool") and applyCodexAuthContextToProvider(..., "pool") instead of route.codexAccountMode. Safe today because the gate is usesCodexForwardPoolAuth, but prefer the route mode for consistency / future modes.

Non-blocking / already solid

• Allow-list + bounded body (displaySafe / !truncated) + single alternate + negative matrix (hostile bodies, wrong model id, oversize, timeout) look carefully designed.
• Transport failure on B records only B and does not triple-dispatch (tested).
• No eligible alternate returns the original 400 unchanged (tested).

Suggested fix order

1. Rebind thread affinity to the successful retry account (add HTTP affinity regression beside WS-REBIND-01).
2. Run the alternate hop through fetchWithTransientRetry.
3. Pass route.codexAccountMode instead of a literal "pool".

Maintainer security sign-off still recommended given the pool-selection adjacency, but this review did not find an auth bypass or health-policy regression.