Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe change centralizes quota-refusal checks and adds quota-scope-aware shared headroom. Shared bindings remain eligible below exhaustion during quota-strategy cache affinity. Quota refusals and full exhaustion retire shared state. ChangesQuota-aware shared routing
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Low Sequence Diagram(s)sequenceDiagram
participant BoundThread
participant Routing
participant Selection
participant HealthStore
BoundThread->>Routing: request shared-affinity selection
Routing->>Selection: check shared quota headroom with quotaScope
Selection->>HealthStore: check account-wide and scoped quota refusal
HealthStore-->>Selection: return health status
Selection-->>Routing: preserve or retire shared binding
Routing-->>BoundThread: return selected account
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
리뷰 · 우선순위 46 / 80계정이 여러 개면 대화는 한 계정에 붙어 있습니다. 그 계정의 사용량이 스위치 기준(기본 80%)을 넘으면, 예전에는 그 붙임을 버렸습니다. 이번 요청만 다른 모델이라 잠깐 옆 계정으로 가는 경우에도 버렸습니다. 원래 계정은 아직 요청을 받을 수 있는데, 다음 말은 앞에서 쌓아 둔 기록을 못 쓰고 처음부터 다시 나갑니다. 이 PR은 그 버리기를 늦춥니다. 캐시 붙임이 켜져 있고 고르는 방식이 사용량이 적은 순(quota)이면, 사용량이 100%가 되기 전에는 사용량만으로 공유 붙임을 안 버립니다. 사용량을 아직 모르면 여유가 있다고 봅니다. 스위치 기준을 0으로 끄면, 예전처럼 사용량만으로는 안 버립니다. 계정을 돌아가며 쓰거나, 하나를 채운 뒤 다음으로 넘기는 방식은 예전 함수를 그대로 탑니다. 새로 고르는 요청은 여전히 80% 기준입니다. 이미 붙어 있는 것만 100%까지 기다립니다. 429나 402를 받고 그 뒤 한 번도 성공하지 못한 계정은, 공유로 건강한 선택에서 빠집니다. 이 판정은 테스트는 사용량 90%인 붙임이, 모델만 다른 요청 뒤에도 원래 계정을 지키는지 봅니다. 90%를 다 쓴 것으로 보던 고정 계정 테스트 두 개는 100%로 고쳤습니다. 설명 문서 한 단락도 맞췄습니다. 베이스는
메인테이너의 판단이 필요한 지점 기준을 0으로 껐을 때, 사용량 100%인 공유 붙임을 남길지 뗄지입니다. 289행은 남기는 쪽입니다. 라이브 붙임은 떼는 쪽입니다.
너의 추천 사용량이 80%를 넘었다는 이유만으로, 이미 붙어 있는 대화를 버리는 쪽을 고친 방향은 맞습니다. 그 경우는 테스트가 잡아 둡니다. 머지 전에 284행의 "같은 기준"을 실제 차이에 맞게 고치거나, 기준 0과 100%의 동작을 라이브 붙임과 같게 맞추면 됩니다. 새 테스트 옆에, 사용량 100이거나 429가 남은 계정이 이 길에서 붙임을 놓는 경우를 하나 두면 다음 수정이 그 경우를 놓치기 어렵습니다. 닫을 중복 PR은 없습니다. 이 댓글은 grok-bot이 작성했습니다 |
59cb861 to
d1c3d89
Compare
추가 리뷰 · 우선순위 20 / 80이전 리뷰가 말한 어긋남은 없어졌습니다. 스위치 기준을 0으로 꺼도, 사용량이 100이면 공유 붙임을 놓습니다. 이미 붙어 있는 대화를 옮기는 쪽과 같습니다. 이 브랜치는 그 뒤에 다시 밀렸습니다. 테스트 두 개를 더했습니다. 사용량 100인 계정이 모델만 다른 요청을 받은 뒤, 공유로 고른 계정이 옆 계정으로 바뀌는지 봅니다. 하나는 기준 80, 하나는 기준 0입니다. 사용량 90에서 원래 계정을 지키는 테스트는 그대로입니다.
메인테이너의 판단이 필요한 지점 없습니다. 기준 0과 100%는 라이브 붙임과 같은 쪽으로 정해졌고, 테스트가 그 쪽을 고정합니다. 너의 추천 이 차이만으로 머지를 막을 이유는 없습니다. 429를 같은 테스트 옆에 하나 더 두면, 다음 수정이 거절이 남은 계정을 놓치기 어렵습니다. 없어도 이번 고친 범위는 닫힙니다. 이 댓글은 grok-bot이 작성했습니다 |
|
Review findings addressed in d1c3d89 (rebased onto dev@fec3add6c):
Validation: |
d1c3d89 to
968b1da
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Holding this one at the merge gate rather than landing it, and the reason is mechanical. I now run a preflight before merging: build the merge-result tree with Eighty-three lines over. That does not show up on your branch's own CI because the cap comparison only bites in the merged tree, which is exactly the class of failure that broke Raising the cap is not available: The change itself reads well and I would like to land it. Lifting Ping here once the split is pushed and I will re-run the preflight and merge. I have not run anything locally against this branch. |
…on and add privacy-bounded cache diagnostics (#5268) * fix(codex): preserve cache affinity across model detours Carries #5209. A gated-model detour under pool.cacheAffinity + the quota strategy evicted a cache-warm shared binding on a threshold crossing (a hint), before the account was actually exhausted. The three shared-state/affinity preservation predicates now use the 100%-exhaustion boundary via hasCodexSharedStateQuotaHeadroom, matching live-binding quota re-evaluation. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(reasoning): scope learned reasoning-effort refusals to credential identity Carries #5145. A learned upstream refusal was persisted under a destination-wide key (provider, model, effort), so every credential reaching the same destination inherited it. Each learned fact is now bound to a one-way SHA-256 digest of the active credential; the support row key becomes a JSON array; the snapshot advances to version 2 and legacy destination-wide rows are ignored on load. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(cursor): isolate live roster and Max Mode evidence by account Carries #5229. Cursor pooled accounts shared module-level singletons for the Claude wire-spelling map and the Max-Mode evidence set, so a discovery recorded under one credential could rewrite the wire id or arm ultra for a request resolved under a different account. Both maps are now keyed by a non-secret sha256 scope over the upstream destination and credential, and a provider-scoped evidence entry is dropped when its model cache clears. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(codex): fence entitlement credential refreshes behind admission Carries #5214. Background and data-plane entitlement resolves (catalog sync, convergence, serve-options /models, CLI startup discovery, ensureCodexEntitlementFreshness) could refresh or rewrite the native auth.json while native-main lifecycle, recovery, or profile-switch drains intend the physical native identity to stay untouched, and a refused claim also took down Pool discovery. Adds model-entitlement-admission.ts plus withNativeMainCredentialAdmission in native-main-admission.ts, applied at the five sites; the test file lands in the codex-integration domain registered in the layout map. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * feat(usage): show cache metrics by model Carries #4793. The Usage page's Models table now shows input tokens, output tokens, cache hits, cache writes, and cache hit rate for each model; providers without cache telemetry render an em dash. Includes translations for all supported GUI locales, dashboard documentation, and a rendered GUI regression test. Co-authored-by: xdober <10195626+xdober@users.noreply.github.com> * test(codex): move cache-affinity detour cases to a sibling under the file-size cap codex-routing.test.ts sits exactly at its file-size cap; the carried #5209 cases would have grown it 83 lines over. The three detour cases move to codex-routing-cache-affinity-detour.test.ts byte for byte with their own minimal harness, registered in both layout.json and the expected fixture. * fix(codex): bind Cursor and Devin live rosters to the observing credential The live Cursor and Devin model rosters are entitlement-specific, but their provider roster cache was scoped by provider name alone: a credential switch could read the previous account's fresh or stale plan roster, and a failed discovery's cooldown suppressed the next credential's first fetch while offering it the previous account's stale list. Bind the cache entry to an irreversible credential fingerprint (the Qoder precedent), make the stale fallback credential-scoped, and let a credential with no roster of its own fetch through another credential's cooldown. Quota and rate-limit health stay account-scoped by design: they describe the subscription, not the token generation, and the 401/403 quarantine is already generation-fenced. * fix(codex): fence cancelled entitlement refreshes behind caller cancellation A data-plane /v1/models request now passes its own signal into admitted entitlement resolution, and the native-main token refresh re-checks that signal after the upstream grant resolves and before the auth.json commit: a refresh that resolves after its caller went away no longer rewrites the physical credential on behalf of a request that no longer exists. The reauth twin already fenced its commit the same way; the roster-cache publication stays fenced by credential identity and mutation epoch, which is the correct boundary for a shared flight. * feat(usage): opt-in privacy-bounded cache diagnostic (#5178) Under OPENCODEX_CACHE_DEBUG=1 the proxy writes one record per finalized request to <config-dir>/cache-debug.jsonl (0600, 200-to-100 rolling), letting an operator compare two requests and tell a client prefix change, an account change, and a proxy transformation change apart as the cause of a cache-read drop. Records hold only presence booleans, counts, closed enums, the raw upstream cache counter before defaulting, and process-local HMAC equality tags (independent process-random key, never persisted) for the prompt-cache key, allowlisted session headers, the account log label, and ordered instruction/tool/message blocks capped at 128 per section with only the first divergent section/index. No prompt text, tool names, raw identifiers, or header values are recorded, and no tag survives a process restart, so a fingerprint can never become a public or durable correlation key. The request path reaches the module through a process-local registration hook so responses/core.ts gains no runtime import, and an all-zero usage frame with a measured cache counter now survives extraction instead of collapsing to "unreported", which is what keeps a measured zero distinct from an absent counter downstream. Off by default. * docs(devlog): record lane D account/cache-generation progress * fix(usage,tests): close review findings on the diagnostic and the moved admission test Pre-CI adversarial review found two blocking defects: the carried entitlement-admission test kept its tests-root import paths after the domain move (every case failed at load), and the diagnostic's block splitter aliased an array-valued instructions field, so observation would have mutated the live request body the adapter was about to serialize. Both are fixed, the second with a mutation regression test. The all-zero usage extraction change is reverted: it reclassified spend settlement for placeholder frames, and the measured-zero versus absent distinction already rides the provenance enum for every frame that reports tokens. * fix(catalog,codex): derive the reasoning-rung type and scope discovery cooldown to its credential Exact-head CI on this branch failed gates, both typecheck-dependent shards and one Cursor case. Three causes, fixed here. catalog/effort.ts and catalog/build-entries.ts cast a partially populated ladder to Array<{ effort?: string }> and push a canonical CODEX_REASONING_LEVELS rung into it, which also carries description. That was always a type error, but reasoning-effort.ts -> providers/reasoning-metadata.ts -> providers/key-store.ts -> the ../config barrel formed an import cycle in which the rung type degraded and the excess-property check never ran. Carried #5145 breaks that cycle by design, so the latent error surfaced here first. reasoning-effort.ts now exports CodexReasoningLevel and the three casts derive Array<Partial<CodexReasoningLevel>> from it rather than restating a narrower shape. The translator-budget contract test, which spawns tsc over the project, was downstream of these errors. The Cursor cooldown case was a real regression from this lane. Scoping only the roster reads to the credential left the failure cooldown provider-wide, so the branch had to require a credential-scoped stale entry before honouring it, and a discovery that fails before caching anything has no stale entry -- reopening the timeout storm #54 closed. The scope now sits where the observation belongs: a discovery failure records the credential that observed it and suppresses only that credential. A failure recorded without an identity stays credential-agnostic and suppresses everyone, so plain-endpoint providers and the existing Qoder branch are unchanged. cache-diagnostic.ts narrowed draft.promptCacheKey through optional chaining and then read it again unguarded; the inbound key is bound once. * fix(gui-tests): derive the usage header and locale symbol checks from their sources The carried #4793 columns broke three GUI assertions that restate what the page and the catalogs already own. usage-custom-range listed the models-table headers as English literals and omitted the API list-price column that ships today, so the case failed on any tree where both exist. The expectation now maps the ordered column keys the page renders through the en catalog, which is where that copy lives. The French accidental-English guard and the zh-TW stale-placeholder guard both flagged usage.unavailable, whose value is an em dash. A value with no letters once its placeholders are removed has nothing to translate and is identical in every locale by construction, so both checks now derive that from the value instead of taking one more allowlist entry. Real words still fail: the existing entries that carry letters, such as uptime.hour, remain allowlisted and required. * docs(devlog): record the lane D CI dispositions * test(ci): quarantine the 50 MiB sideband relay case into its own lane sideband GET /v1/live/{callId} relays a 50 MiB WebSocket frame end to end against a hard 15s deadline while sharing a process with the rest of its --shard=N/2 half, so its result measures the whole process rather than the relay. On dev it lands in shard 1 and its echo leg alone spends 7.4s of that budget. Three test files added elsewhere in this branch made Bun repartition the halves, the case moved to shard 2, and the echo leg went past 15s twice with the peer never receiving the frame -- with nothing on the sideband path changed. SERIAL_FULL_SUITE_FILES is the mechanism this repository already has for that category; its own guard describes it as quarantining load-sensitive files into one-worker lanes. The deadline, the assertion and the macOS leg are unchanged; the case simply stops sharing a process, which also keeps it from breaking the next branch that adds a test file anywhere in the tree. --------- Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> Co-authored-by: xdober <10195626+xdober@users.noreply.github.com>
|
Superseded by #5268, merged to Cache affinity now survives a model detour: passing a preemptive-switch threshold no longer drops a warm binding on its own, while real exhaustion still releases it. The four account-scoped pull requests landed together because they are one attribution defect seen at different layers: an observation belonging to an account or credential generation must not survive its replacement. Fixing them separately would have left each one's tests blind to the others' leakage. Closing as superseded rather than stale. |
2 participants
Motivation
pool.cacheAffinity+ the quota strategy, a gated-model detour can evict a cache-warm shared binding before the account is actually exhausted: the three shared-state/affinity preservation predicates readhasCodexQuotaHeadroom(usage <autoSwitchThreshold), so a threshold crossing — a hint, not evidence — retires the binding and the next turn pays a cold prefix.mayRebindAffinityForQuota); the shared-state preservation predicates were never moved to it.Description
hasCodexSharedStateQuotaHeadroombesidehasCodexQuotaHeadroom(src/codex/routing/selection.ts): under cache affinity + quota strategy it answers the 100%-exhaustion boundary (unknown usage still counts as headroom; a disabled threshold still means never drained on quota alone); anything else delegates tohasCodexQuotaHeadroom.isHealthySharedCodexSelection,healthyForSharedAffinity,activeHealthyForSharedSelection— so a model-scoped detour no longer retires shared state on a threshold crossing (src/codex/routing.ts,src/codex/routing/selection.ts).isHealthySharedCodexSelectionnow also requires no unrecovered quota refusal for the scope;hasUnrecoveredCodexQuotaRefusalandcarriesQuotaRefusalmove tohealth-store.tsso selection and routing share one definition (src/codex/routing/health-store.ts).tests/codex-integration/codex-routing.test.ts). Doc paragraph updated (structure/providers/openai-tiers.md).Testing
bun test tests/codex-integration/codex-routing.test.ts— 183 pass / 1 fail; the failure (model detour LRU stays bounded without evicting ordinary affinity, ~33s vs the 30s budget) reproduces identically on a cleanorigin/devbaseline — pre-existing/environmental, not a regression.origin/dev(getEffectiveActiveCodexAccountIdreturns the detour account — binding evicted) and passes with the change.bun x tsc --noEmit,bun run privacy:scan,bun run structure:check— all clean.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