fix(routing): keep unbound account quota unknown - #1195
Conversation
📝 WalkthroughWalkthroughThe routing policy and dry-run candidate assembly no longer infer quota evidence from active Codex or Anthropic accounts. Tests verify unknown quota state without explicit account selection. Documentation describes the updated behavior. ChangesAccount-bound quota evidence
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
⏳ DRAFT
What to do
Review readiness checklist
2/4 boxes ticked. This PR stays in draft until every box above is ticked. |
|
✅ Deterministic PR hygiene checks passed. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@docs-site/src/content/docs/reference/configuration/routing.md`:
- Around line 162-163: Clarify the paragraph around the quota-aware dry-run
example by explicitly naming POST /api/routing-profiles/dry-run as the supported
source of candidate account evidence. Remove the ambiguous “dry-run/API” wording
and retain the statement that the CLI dry-run cannot provide
candidates[].codexAccountId or candidates[].accountRef evidence.
In `@tests/quota-scoring.test.ts`:
- Around line 209-211: Extend the unbound-candidate assertions in the
quota-scoring test to verify that the unbound Codex candidate’s account identity
field codexAccountId and the unbound Anthropic candidate’s account identity
field accountRef are undefined. Keep the existing quota assertions and use the
route-decision type’s equivalent fields if these names differ.
- Around line 213-225: Clear the Anthropic OAuth account state created by
saveCredential in the test “execution path does not invent Anthropic quota
evidence from the active account” after it completes, using the existing test
teardown or an isolated OAuth store. Ensure the persisted active account uuid-a
cannot leak into later tests while preserving this test’s quota-cache setup and
assertions.
🪄 Autofix
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: db674f89-89f4-42bb-a34b-21bba28a6563
📒 Files selected for processing (4)
docs-site/src/content/docs/reference/configuration/routing.mdsrc/router.tssrc/server/management/routing-profile-routes.tstests/quota-scoring.test.ts
💤 Files with no reviewable changes (2)
- src/server/management/routing-profile-routes.ts
- src/router.ts
| it only feeds policy scoring. To see quota-aware behavior in a dry-run, supply account refs through | ||
| the dry-run/API candidate evidence: `candidates[].codexAccountId` (Codex pool, provider `openai`) or |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify that account evidence is API-only.
Lines 125-126 state that the CLI dry-run cannot provide candidate evidence. The phrase “dry-run/API candidate evidence” on Lines 162-163 can imply that the CLI accepts candidates[].codexAccountId and candidates[].accountRef.
Name POST /api/routing-profiles/dry-run explicitly and keep the CLI limitation in this paragraph.
Proposed documentation fix
- To see quota-aware behavior in a dry-run, supply account refs through the dry-run/API candidate evidence:
+ To see quota-aware behavior in an API dry-run, supply account refs through the candidate evidence:As per path instructions, user-facing documentation must stay synchronized with actual CLI/API behavior.
🤖 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 `@docs-site/src/content/docs/reference/configuration/routing.md` around lines
162 - 163, Clarify the paragraph around the quota-aware dry-run example by
explicitly naming POST /api/routing-profiles/dry-run as the supported source of
candidate account evidence. Remove the ambiguous “dry-run/API” wording and
retain the statement that the CLI dry-run cannot provide
candidates[].codexAccountId or candidates[].accountRef evidence.
Source: Path instructions
| expect(route.routeDecision!.candidates[0]!.quota?.known).toBe(false); | ||
| expect(route.routeDecision!.candidates[0]!.quota?.headroom).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Assert that unbound candidates have no account identity.
These assertions prove that the active cache entry is not applied. They do not directly prove that the active account was not copied into the candidate.
Add assertions that the unbound Codex candidate has no codexAccountId and the unbound Anthropic candidate has no accountRef, or use the equivalent account-bearing fields in the route-decision type.
Also applies to: 239-241
🤖 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 `@tests/quota-scoring.test.ts` around lines 209 - 211, Extend the
unbound-candidate assertions in the quota-scoring test to verify that the
unbound Codex candidate’s account identity field codexAccountId and the unbound
Anthropic candidate’s account identity field accountRef are undefined. Keep the
existing quota assertions and use the route-decision type’s equivalent fields if
these names differ.
| test("execution path does not invent Anthropic quota evidence from the active account", async () => { | ||
| await saveCredential("anthropic", { | ||
| access: "access-a", | ||
| refresh: "refresh-a", | ||
| expires: Date.now() + 3_600_000, | ||
| accountId: "uuid-a", | ||
| email: "a@example.test", | ||
| }); | ||
| const activeId = getAccountSet("anthropic")!.activeAccountId; | ||
| setCachedProviderAccountQuotaForTests("anthropic", activeId, { | ||
| fiveHourPercent: 40, | ||
| updatedAt: Date.now(), | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 6 \
'afterEach|beforeEach|reset.*(auth|oauth|quota)|clear.*(auth|oauth|quota)|saveCredential|setCachedProviderAccountQuotaForTests' \
tests/quota-scoring.test.ts \
src/oauth/store.ts \
src/providers/quota.tsRepository: lidge-jun/opencodex
Length of output: 10521
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== quota test hooks and OAuth imports =="
sed -n '1,35p' tests/quota-scoring.test.ts
echo "== relevant quota.ts functions =="
nl -ba src/providers/quota.ts | sed -n '1103,1126p'
echo "== all OAuth store mutation functions and exports =="
nl -ba src/oauth/store.ts | sed -n '440,540p'
rg -n "export function (getAccountSet|clearCredentials|deleteAccount|revokeCredential|deleteCredential)" src/oauth/store.ts
echo "== usages/test fixtures that clear account state =="
rg -n "createRequestHistory|getAccountSet|deleteCredential|clearCredentials|revokeCredential|setCachedProviderAccountQuotaForTests|clearAccountQuotaCache" tests src/routing tests/quota-scoring.test.tsRepository: lidge-jun/opencodex
Length of output: 1653
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== quota.ts cache functions =="
awk 'NR>=1103 && NR<=1126 {printf "%6d:%s\n",NR,$0}' src/providers/quota.ts
echo "== oauth store relevant implementation =="
awk 'NR>=475 && NR<=535 {printf "%6d:%s\n",NR,$0}' src/oauth/store.ts
echo "== clear/delete credential-related exports and tests =="
awk 'NR>=1 && NR<=800 {if (/clearCredentials|deleteAccount|deleteCredential|revokeCredential|createRequestHistory|getAccountSet|setCachedProviderAccountQuotaForTests|clearAccountQuotaCache/) printf "%6d:%s\n",NR,$0}' src src/oauth src/routing 2>/dev/null | grep -E 'src/|tests/' || true
rg -n "clearCredentials|deleteAccount|deleteCredential|revokeCredential|createRequestHistory|getAccountSet|setCachedProviderAccountQuotaForTests|clearAccountQuotaCache" tests src || true
echo "== deterministic state-shape probe from source =="
python3 - <<'PY'
from pathlib import Path
quota = Path("src/providers/quota.ts").read_text()
oauth = Path("src/oauth/store.ts").read_text()
checks = {
"quota exported null guard deletes entry": "if (quota === null) { accountQuotaCache.delete(key); return; }" in quota,
"quota account map initialized outside saveCredential": "const accountQuotaCache = new Map" in quota,
"quota afterEach clear imports": "clearAccountQuotaCache" in Path("tests/quota-scoring.test.ts").read_text(),
"oauth saveCredential mutates file path": "storeFile(" in oauth,
"oauth saveCredential exports activeAccountId": "activeAccountId:" in oauth,
"tests quota-scoring no credential delete/export": r"deleteCredential|clearCredentials" not in Path("tests/quota-scoring.test.ts").read_text(),
}
for k, v in checks.items(): print(f"{k}: {str(v).lower()}")
PYRepository: lidge-jun/opencodex
Length of output: 28897
Clear Anthropic OAuth state after saving the active account.
tests/quota-scoring.test.ts clears the shared quota cache after each test, but this test also calls saveCredential("anthropic", ...) at tests/quota-scoring.test.ts:213-220, which persists uuid-a as the Anthropic active account in src/oauth/store.ts:475-523. Add a test fixture, teardown, or isolated OAuth store for this test so it does not carry Anthropic account state into later tests.
🤖 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 `@tests/quota-scoring.test.ts` around lines 213 - 225, Clear the Anthropic
OAuth account state created by saveCredential in the test “execution path does
not invent Anthropic quota evidence from the active account” after it completes,
using the existing test teardown or an isolated OAuth store. Ensure the
persisted active account uuid-a cannot leak into later tests while preserving
this test’s quota-cache setup and assertions.
1 participant
Summary
Why
Policy profiles choose a provider/model before the request path resolves Pool/Direct identity, thread affinity, Anthropic session affinity, or round-robin/fill-first selection. Using a process-global active account during policy evaluation can therefore score or exclude a candidate with account A's quota and then execute the request with account B.
Unknown quota already has an explicit profile policy. Leaving an unbound candidate unknown is more accurate than inventing an account reference, and it keeps account selection, cooldowns, and session affinity authoritative.
Verification
b22e0e6d0): the same suites 55/55 passed.codexAccountId/account-ref quota evidence remains covered by existing tests.bun x tsc --noEmit: passed.bun scripts/privacy-scan.ts: passed.git diff --check: passed.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit