fix(routing): keep unbound account quota unknown - #1195
Conversation
📝 WalkthroughWalkthroughRouting policy and dry-run candidate assembly no longer infer quota evidence from active Codex or Anthropic accounts. Tests verify unknown quota without explicit account selection. Documentation defines the account-bound quota 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
1/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
![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: 6eff3f6a58
ℹ️ 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".
| quota: quotaEvidenceForCandidate({ | ||
| provider: candidate.provider, | ||
| model: candidate.model, | ||
| ...(candidate.provider === OPENAI_CODEX_PROVIDER_ID | ||
| && providerCodexAccountMode( | ||
| OPENAI_CODEX_PROVIDER_ID, | ||
| config.providers[OPENAI_CODEX_PROVIDER_ID], | ||
| ) === "pool" | ||
| ? (() => { | ||
| const codexAccountId = getEffectiveActiveCodexAccountId(config); | ||
| return { | ||
| codexAccountId, | ||
| codexAccountPlan: codexAccountId | ||
| ? config.codexAccounts?.find(account => account.id === codexAccountId)?.plan | ||
| : undefined, | ||
| }; | ||
| })() | ||
| : {}), | ||
| accountRef: candidate.provider === "anthropic" | ||
| ? getAccountSet("anthropic")?.activeAccountId | ||
| : undefined, | ||
| }), |
There was a problem hiding this comment.
When anthropicAccountPool.enabled is false (the default), request execution deterministically uses the store's active account through getValidAccessTokenSnapshot, so its cached quota is not ambiguous. Removing accountRef here makes every such policy candidate report unknown quota; profiles with unknownEvidence.quota: "exclude" reject the route, while minQuotaHeadroom and quota optimization stop using valid evidence. Only omit the active account when Anthropic pooling can actually select a different account, and mirror that condition in management dry-runs.
Useful? React with 👍 / 👎.
|
@luvs01 Republished this on current Your fix commit is unchanged and carries Full suite on the rebased head: 9992 pass / 0 fail. I'll close this once #1288 lands. |
* fix(routing): keep unbound account quota unknown (#1195) Policy profiles choose a provider and model before the request path resolves Pool/Direct identity, thread affinity, Anthropic session affinity, or round-robin/fill-first selection. Attaching the process-global active account during policy evaluation could therefore score or exclude a candidate using account A's quota and then execute the request on account B. An unbound candidate now stays quota-unknown in both the live route trace and the management dry-run, which is more accurate than inventing an account reference and keeps account selection, cooldowns, and session affinity authoritative. Unknown quota already has an explicit profile policy. Explicit `codexAccountId` and account-ref evidence remains unchanged. Republished from #1195 by luvs01, whose branch was 300 commits behind dev. Rebased onto f5147cb with no conflicts; authorship preserved below. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * test(routing): prove the management dry-run leaves unbound candidates unknown Maintainer-added coverage for the #1195 republish. The contributor's patch deletes the same block from the live router and the management dry-run path, but only the live path had a regression. The existing dry-run test covers a candidate with an explicitly supplied codexAccountId, which stays known and is unaffected by the fix, so the dry-run half of the parity claim was unproven. These two tests exercise an unbound Codex candidate with an active pool account, and an unbound Anthropic candidate with an active account, and assert both stay quota-unknown with no accountRef. Restoring either deleted block fails them. --------- Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
|
Landed on Note for the record: the merged commit also carries a separate maintainer-authored test commit adding two management dry-run regressions. Those are mine, not yours — your fix commit is unmodified. Without them the dry-run half of the parity claim had no coverage, since the existing dry-run test supplies Closing this as superseded. The ordering argument was correct and worth landing. |
2 participants
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