fix(combos): scope cached quota to provider-wide inference limits - #4090
fix(combos): scope cached quota to provider-wide inference limits#4090luvs01 wants to merge 2 commits into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
|
✅ Deterministic PR hygiene checks passed. |
|
@coderabbitai review Please review current head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR metadata ==='
gh pr view 4090 --repo lidge-jun/opencodex --json number,title,headRefOid,baseRefOid,files,commits,url
echo '=== Changed files ==='
gh pr diff 4090 --repo lidge-jun/opencodex --name-only
echo '=== Diff ==='
gh pr diff 4090 --repo lidge-jun/opencodex --patch
echo '=== Candidate quota/combo files ==='
fd -t f . | rg -i '(combo|quota|routing|provider|credential|oauth)'
echo '=== Structural outline of changed TypeScript files ==='
while IFS= read -r f; do
case "$f" in
*.ts|*.tsx)
echo "--- $f ---"
ast-grep outline "$f" --items all --type class,function,interface,type --view expanded || true
;;
esac
done < <(gh pr diff 4090 --repo lidge-jun/opencodex --name-only)Length of output: 119859 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Outlines ==='
for f in src/providers/quota-routing-cache.ts src/providers/quota.ts src/combos/resolve.ts src/providers/key-store.ts src/providers/registry.ts src/router.ts; do
echo "--- $f ---"
ast-grep outline "$f" --items all --view expanded || true
done
echo '=== Routing cache ==='
cat -n src/providers/quota-routing-cache.ts
echo '=== Combo resolution ==='
cat -n src/combos/resolve.ts
echo '=== Key-store and registry functions used by binding ==='
rg -n -C 12 'function (resolveProviderApiKey|providerUsesKeyAuthOverride)|export (function|const) (resolveProviderApiKey|providerUsesKeyAuthOverride)' \
src/providers/key-store.ts
rg -n -C 12 'function getProviderRegistryEntry|export (function|const) getProviderRegistryEntry|authKind' \
src/providers/registry.ts
echo '=== Quota producer and cache publication flow ==='
rg -n -C 14 'function (keyReport|maybeFetchProviderQuota)|export async function fetchProviderQuotaReports|replaceCachedProviderQuotas|routingEvidence|providerQuotaBeforePublishForTests' \
src/providers/quota.ts
echo '=== All keyReport call sites ==='
rg -n -C 3 '\bkeyReport\s*\(' src/providers/quota.ts
echo '=== Effective routed provider configuration ==='
rg -n -C 12 'function routedProviderConfig|export (function|const) routedProviderConfig' src/router.ts
rg -n -C 8 '\broutedProviderConfig\s*\(' src --glob '*.ts'
echo '=== Combo cooldown/wait consumers ==='
rg -n -C 10 'pickComboTargetWithWait|targetProviderIsUsable|resetWindowIndex|getCachedProviderRoutingQuota' \
src/combos --glob '*.ts'
echo '=== Quota cache reader consumers ==='
rg -n -C 4 'getCachedProviderQuota|getCachedProviderRoutingQuota|replaceCachedProviderQuotas' src tests --glob '*.ts'Length of output: 230200 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== routedProviderConfig complete implementation ==='
sed -n '295,430p' src/router.ts | cat -n
echo '=== API-key selection capture and pool selection ==='
cat -n src/providers/api-key-selection-capture.ts
sed -n '1,180p' src/providers/api-key-selection.ts | cat -n
rg -n -C 10 'apiKeyPool|_apiKeyAttempt|captureProviderApiKeySelection' src/providers --glob '*.ts'
echo '=== Quota readers and dispatch selection ==='
sed -n '400,610p' src/providers/quota.ts | cat -n
sed -n '850,1100p' src/providers/quota.ts | cat -n
sed -n '2280,2470p' src/providers/quota.ts | cat -n
sed -n '2890,2985p' src/providers/quota.ts | cat -n
echo '=== Cache key and refresh merge details ==='
sed -n '165,190p' src/providers/quota.ts | cat -n
sed -n '3060,3158p' src/providers/quota.ts | cat -n
echo '=== Provider config and apiKeyPool types ==='
rg -n -C 10 'interface OcxProviderConfig|type OcxProviderConfig|apiKeyPool|_apiKeyAttempt' src/types --glob '*.ts'
echo '=== Existing pool and binding tests ==='
rg -n -C 12 'apiKeyPool|routing quota scope|setProviderQuotaBeforePublishForTests|Authorization override|x-api-key' \
tests/providers/provider-quota.test.ts tests/codex-integration/combos.test.ts tests/providers/api-key-selection-capture.test.ts
echo '=== Calls to report() in quota producers after this change ==='
rg -n -C 3 '\breport\s*\(' src/providers/quota.tsLength of output: 160537 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Direct-report producer implementations ==='
for r in '585,635' '900,1035' '1080,1175'; do
echo "--- src/providers/quota.ts:$r ---"
sed -n "$r"p src/providers/quota.ts | nl -ba -v "${r%,*}"
done
echo '=== Key quota reader names and relevant direct-report providers ==='
rg -n -C 8 'function (isCanonicalMinimaxBaseUrl|isCanonicalDeepInfraBaseUrl|isCanonicalNeuralwattBaseUrl)|fetch(Minimax|DeepInfra|Neuralwatt)Quota|keyQuotaReaderForProvider' src/providers/quota.ts
echo '=== Existing Combo behavior tests for Minimax, DeepInfra, Neuralwatt, and direct reports ==='
rg -n -i -C 8 '(minimax|deepinfra|neuralwatt).*(combo|quota|exhaust|reset)|(?:combo|quota|exhaust|reset).*(minimax|deepinfra|neuralwatt)' tests src --glob '*.test.ts'
echo '=== Static credential headers in registry ==='
rg -n -i -C 6 'staticHeaders|authorization|x-api-key|x-goog-api-key' src/providers/registry.ts
echo '=== Exact changed-source line references ==='
nl -ba src/providers/quota.ts | sed -n '905,950p'
nl -ba src/providers/quota-routing-cache.ts | sed -n '20,82p'
nl -ba src/combos/resolve.ts | sed -n '60,150p'
echo '=== Current PR check results ==='
gh pr checks 4090 --repo lidge-jun/opencodex || trueLength of output: 1713 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Direct-report producer implementations ==='
for r in '585,635' '900,1035' '1080,1175'; do
echo "--- src/providers/quota.ts:$r ---"
sed -n "$r"p src/providers/quota.ts | cat -n
done
echo '=== Key quota reader names and relevant direct-report providers ==='
rg -n -C 8 'function (isCanonicalMinimaxBaseUrl|isCanonicalDeepInfraBaseUrl|isCanonicalNeuralwattBaseUrl)|fetch(Minimax|DeepInfra|Neuralwatt)Quota|keyQuotaReaderForProvider' src/providers/quota.ts
echo '=== Existing Combo behavior tests for Minimax, DeepInfra, Neuralwatt, and direct reports ==='
rg -n -i -C 8 '(minimax|deepinfra|neuralwatt).*(combo|quota|exhaust|reset)|(?:combo|quota|exhaust|reset).*(minimax|deepinfra|neuralwatt)' tests src --glob '*.test.ts' || true
echo '=== Static credential headers in registry ==='
rg -n -i -C 6 'staticHeaders|authorization|x-api-key|x-goog-api-key' src/providers/registry.ts
echo '=== Exact changed-source line references ==='
sed -n '905,950p' src/providers/quota.ts | cat -n
sed -n '20,82p' src/providers/quota-routing-cache.ts | cat -n
sed -n '60,150p' src/combos/resolve.ts | cat -n
echo '=== Current PR check results ==='
gh pr checks 4090 --repo lidge-jun/opencodex || trueLength of output: 60495
P1 — Keep the MiniMax Token Plan quota as routing evidenceIn This probe:
However, This changes the existing behavior. A MiniMax Token Plan at 100% can be selected first and fail only after dispatch. Use the credential-bound producer path here: return keyReport(provider, "minimax:token-plan-remains", {
customWindows: [{ label, percent }],
updatedAt: Date.now(),
}, config, apiKey);Add a regression test that returns The credential binding, OAuth normalization, configured-header handling, and both Combo consumers otherwise match the stated design. The current GitHub You are interacting with an AI system. |
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This PR stays in draft until every box above is ticked. |
리뷰 · 우선순위 66 / 80이 PR은 Combo가 “제공자 전체 할당량 캐시”로 타깃을 미리 버릴 때, 그 증거가 지금 설정된 단일 API 키의 모델 추론에만 적용되는 경우에만 쓰이게 고칩니다. 지금 라인 src/providers/quota-routing-cache.ts · providerQuotaRoutingBinding - 자격증명 원문은 캐시에 안 남기고 다이제스트만 씁니다. 보안 리뷰 포인트로 PR 본문이 이미 표시한 부분이라, 머지 전에 “리포트 JSON/로그에 binding이 새지 않는지”만 한 번 더 확인하면 됩니다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
@coderabbitai The MiniMax Token Plan row intentionally remains display-only in this change; I am not adopting the proposed unconditional The current parser derives a displayed share from Binding a snapshot to the correct key would establish ownership, but would not establish this missing scope. Treating the countdown row as a whole-provider veto would reproduce the class of false exclusion this PR fixes. The existing producer does not expose a documented, exhausted all-resource balance or a corresponding resource-selection policy, so the runtime leaves this row for display and lets actual dispatch/admission decide availability. The positive single-key OpenRouter spending-cap control remains covered and enforced. MiniMax could gain routing evidence later if an authoritative response and resource policy establish an applicable inference-wide limit. The current HEAD remains |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🤖 web_search executed:
Length of output: 13269
The documentation states that, after the 5-hour or weekly Token Plan limit, the same Subscription Key can automatically use purchased Credits when available. Therefore, a I withdraw the MiniMax finding. A future routing-evidence change needs both of these conditions:
The OpenRouter single-key spending-cap control remains the appropriate positive enforcement case. ✏️ Learnings added
You are interacting with an AI system. |
|
Follow-up on the recorded CI blocker, without reclassifying a failed run as green: I have independently reviewed and approved the narrow Windows ACL-cleanup correction in #4104. It awaits integration; please use the resulting dev revision when rebasing this PR, then run the final full suite rather than repeatedly retrying 44e428c. The quota-scope change itself remains unapproved in this pass. In particular, keep credential ownership and inference-wide exhaustion as separate requirements; a display-only countdown is not made provider-wide merely by binding it to a key. #4105 remains dependent on this runtime decision and should not ship the editor interpretation first. |
Ownership and inference-wide scope are already two separate requirements@Ingwannu Thank you — that is the right constraint, and I want to show where it is enforced rather than just assert agreement, because the two predicates are deliberately independent in this change and neither one alone can veto a provider. Ownership is Inference-wide scope is a separate per-producer opt-in, That is exactly why a display-only countdown does not become provider-wide by being bound to a key. Binding it would at most produce a valid If you would prefer the scope decision to be structurally explicit rather than a defaulted parameter — for example an explicit per-producer declaration that must name why each retained window is inference-wide — I will restructure it that way. I would rather change the shape now than have the reviewer infer the invariant from a default argument. On sequencing: #4104 is now rebased onto |
|
Thanks for separating ownership from inference scope. I checked the current 44e428c implementation: providerQuotaRoutingBinding and the multi-key guard are independent of keyReport's inferenceQuota parameter, as you described. Yes, please take the structurally explicit option you offered: make a producer deliberately opt into routing evidence and provide its inference-only projection, with omission remaining display-only rather than defaulting inferenceQuota to the full display quota. That avoids a future quota producer inheriting provider-veto authority merely by calling the reporting helper. Keep the confirmed single-key spending-limit positive control, Synthetic search-window exclusion, legacy Z.ai MCP exclusion, and MiniMax display-only behavior. The tests should show that a credential-bound display-only report still cannot rank/veto the provider; ownership by itself is not the scope decision. This is a small publication-contract change, not a request for a broader quota rewrite. #4104's latest e16fa26 runs are now execution-approved; keep the stated landing/rebase sequence. |
Routing evidence is now an explicit producer opt-inDone in Each existing producer now states its own scope at the call site:
The inline object literals at four of those call sites are hoisted into a named VerificationA new regression covers exactly the case you asked for. MiniMax is configured as a single-key
Per your sequence I have not rebased: this commit sits on |
Summary
Keep account-, model- and service-scoped quota reports from excluding an entire Combo provider before its account and credential selection runs. For example, an exhausted Gemini group must not discard an Antigravity target using Claude, and an exhausted active key must not stand for other keys in a pool.
Quota producers now opt into routing evidence explicitly by handing the reporting helper the inference-wide subset that applies to the current single configured API key. A producer that omits that projection stays display-only, so reporting through the credential-bound helper never confers provider-veto authority on its own. The routing cache retains that evidence privately, bound to the captured key and destination, and rechecks it against the current configuration. Immediate selection, bounded cooldown waiting and reset-window ordering use the same evidence. A changed key, OAuth normalization or custom credential header cannot reuse another credential's cap. Single-key spending caps remain effective; Synthetic search windows and legacy ZAI MCP monthly figures remain display data.
This is one runtime quota-selection correction: three runtime files, two existing test files, the transport contract and eight Combo guide locales. Existing account admission, explicit target eligibility and response-driven cooldown/retry rules still decide whether a selected route can actually serve the request. The separate editor save-eligibility interpretation of display reports needs its own follow-up and is not changed here.
The credential binding and scope metadata never enter quota-report JSON or logs. Please include them in the security review of this change.
Verification
Based on dev
8026405d9a527085b3c972dc8630abf8fe3b0441; current HEAD602ad8d3da97fd639366a39df0638b63955c03bf, using Bun 1.4.2. The CI evidence below belongs to the earlier44e428cehead; final full CI is deliberately deferred until this branch is rebased onto the dev revision that carries #4104, as the maintainer requested.x-api-keyoverride case also failed before its correction.git diff --checkpassed after the final changes.test:changedselected 824 of 1153 files and reached the 900-second lane limit after 7,008 passes, five named failures and 16 skips, with two additional Bun worker crashes. It has no completed full-suite result. Both crashed routing suites passed when isolated (36 tests, 290 assertions), as did four of the five named failures. The remaining model-detour LRU case timed out at 32.7s in the combined rerun; matched single-case runs passed on both clean dev (31.2s) and this candidate (31.0s), with 2,053 assertions each. The full run remains incomplete rather than being relabeled green; full author cross-platform CI and current-head review remain required before readiness.All quota responses, credentials and endpoints in the regressions are synthetic or isolated test transports. No live provider or real credential was used.
Current-head CI follow-up
34322578719, job102372402651).afterEach: deleting its temporary directory returned EPERM after the helper's 50 attempts. The refresh/persist operation had completed; the log does not distinguish permissions from an open handle. Job evidence (author run34322578719, job102378149666).hardenConfigDir()starts optional asynchronous Windows ACL work, but this fixture deletes its home without awaiting the existingflushConfigDirHardening(dir)API. The narrowly scoped fixture correction is in test(oauth): await guardian fixture ACL hardening before cleanup #4104 (one test file, 4 additions / 1 deletion; its complete file passed 10 tests / 31 assertions). The failed CI did not capture the owning process handle, so this remains the strongest identified cause rather than proof that every EPERM has that origin.Publication contract follow-up
At maintainer request,
602ad8d3dremoves the defaulted inference projection.keyReport()now publishes routing evidence only when the producer passes its inference-only copy, and returns before resolving a binding otherwise. OpenRouter keeps the single-key spending-cap control, Synthetic keeps its search-window exclusion, legacy Z.ai keeps its MCP exclusion, and MiniMax stays display-only. The remaining producers pass their reports explicitly, so their behavior is unchanged and their scope decision is now recorded at the call site.A new regression puts an exhausted MiniMax Token Plan behind a single-key
key-auth provider in a failover Combo: the ownership binding would resolve, and the combo still selects MiniMax. Provider-quota passed 159 tests / 564 assertions, Combos passed 80 tests / 362 assertions, and typecheck and privacy scan are clean.Review disposition
CodeRabbit proposed using MiniMax's displayed Token Plan depletion as key-scoped routing evidence. The source-backed disposition keeps it display-only: a plan-duration/countdown row does not prove all-resource inference unavailability, and MiniMax documents purchased-credit fallback after plan exhaustion. Binding such a row to a key would establish ownership but not the missing scope. The existing single-key OpenRouter spending-cap control remains enforced.
Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
Current management state
This branch carries the requested publication-contract change on top of the reviewed prerequisite head. The maintainer explicitly requested waiting for #4104 to land in dev before rebasing and running final CI. #4104 is now ready at e16fa26 with 26/26 author CI jobs passing; its upstream execution approval and landing are still pending. No obsolete-head CI retry was dispatched here. Credential ownership and inference-wide exhaustion remain separate conditions; the response to that review is recorded above.