fix(server): type the account rate-limit runtime payload - #5473
fix(server): type the account rate-limit runtime payload#5473vasars2024-hub wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
`account.rate-limits.updated` carried `rateLimits: Schema.Unknown` — each
adapter forwarded its provider's native message verbatim, so the event was
untyped and shaped differently per provider. Nothing consumes it today, and
nothing can until the two shapes agree.
Normalize at the adapter boundary onto `status` plus a `windows` array of
`{ kind, usedPercent, resetsAt?, windowDurationMins? }`. Claude contributes
the single governing window from `rate_limit_info`; Codex contributes its
primary/secondary pair, which the app-server already types. The native
message is unchanged and still reaches consumers via `raw.payload`, so
dropping the passthrough field loses nothing.
Model: Claude Opus 5. Harness: Claude Code.
| @@ -1393,16 +1428,18 @@ function mapToRuntimeEvents( | |||
| } | |||
|
|
|||
| if (event.method === "account/rateLimits/updated") { | |||
There was a problem hiding this comment.
🟡 Medium Layers/CodexAdapter.ts:1430
The account.rate-limits.updated event replaces the canonical rate-limit payload with rateLimitsPayloadFromNotification(payload.rateLimits), but Codex sends sparse rolling updates — fields it omits mean "unchanged," not "cleared." When an unchanged primary/secondary window is omitted from the notification, the emitted windows array drops it; when rateLimitReachedType is omitted, the helper emits status: "allowed" even though omission means unavailable. Any consumer that replaces its canonical state from this typed event will see false recovery and lose unchanged rate-limit windows. The adapter must merge sparse updates with the latest snapshot before emitting a complete canonical payload, or preserve optionality in the canonical contract.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/CodexAdapter.ts around line 1430:
The `account.rate-limits.updated` event replaces the canonical rate-limit payload with `rateLimitsPayloadFromNotification(payload.rateLimits)`, but Codex sends sparse rolling updates — fields it omits mean "unchanged," not "cleared." When an unchanged `primary`/`secondary` window is omitted from the notification, the emitted `windows` array drops it; when `rateLimitReachedType` is omitted, the helper emits `status: "allowed"` even though omission means unavailable. Any consumer that replaces its canonical state from this typed event will see false recovery and lose unchanged rate-limit windows. The adapter must merge sparse updates with the latest snapshot before emitting a complete canonical payload, or preserve optionality in the canonical contract.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 60e720f. Configure here.
| status: CLAUDE_RATE_LIMIT_STATUS[info.status], | ||
| windows: window ? [window] : [], | ||
| }; | ||
| } |
There was a problem hiding this comment.
Claude utilization scale mismatch
High Severity
The rateLimitsPayloadFromSdk function directly copies Claude's SDKRateLimitInfo.utilization (a 0-1 fraction) to AccountRateLimitsUpdatedPayload.usedPercent, which expects a 0-100 percentage. This causes Claude rate limit utilization to be reported at 1/100th of its actual value.
Reviewed by Cursor Bugbot for commit 60e720f. Configure here.
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. Open review comments identify potential bugs: a high-severity scale mismatch (Claude utilization 0-1 vs 0-100 percentage) and a medium-severity sparse update handling issue in Codex. These correctness concerns warrant human review. You can customize Macroscope's approvability policy. Learn more. |
|
Pushed Fixed — Fixed — sparse updates. This is the more serious one, and the generated schema states it outright in the annotation on Rather than merge state in the adapter, Pushing back — Claude That said, it is undocumented, and being wrong here is a silent 100× error rather than a visible one. If anyone has an observed Verification on |
…date Review caught two real defects in the Codex normalization. `spendControlReached` was ignored, so a spend-control-blocked account normalized to `allowed` — reported as able to send work when it cannot. Worse, `allowed` was asserted whenever `rateLimitReachedType` was absent. These notifications are sparse, and the generated schema says so directly: "`None` is unavailable, not a sparse-update recovery." A delta refreshing only usage would have cleared a prior rejection. Claim `rejected` on positive evidence from either signal, and leave `status` absent otherwise — the field is already optional. Also document that `windows` is sparse, so consumers merge by `kind` rather than replace. Model: Claude Opus 5. Harness: Claude Code.


Problem
account.rate-limits.updatedis declared withrateLimits: Schema.Unknowninpackages/contracts/src/providerRuntime.ts. Both adapters that emit it forward their provider's native message verbatim:ClaudeAdapter.tspasses the wholeSDKRateLimitEventCodexAdapter.tspasses the wholeV2AccountRateLimitsUpdatedNotificationSo the event carries two different shapes under one type, and neither is typed. Nothing in the repo consumes it today — I grepped for
account.rate-limits.updatedandAccountRateLimitsUpdatedacross the tree, and every hit is the two emitters, the contract, and the generated Codex schema. There is no projector, reactor, or client reader. Nothing can consume it until the two shapes agree.Worth noting the structure is already there and being discarded: the Codex app-server schema types
primary/secondarywindows withusedPercent,resetsAt, andwindowDurationMins, and the Claude SDK typesrate_limit_infowith a status, window type, utilization, and reset time.Change
Normalize at the adapter boundary, per "complexity belongs at the adapter boundary" in AGENTS.md:
windowsis an array rather than fixed fields because the providers genuinely disagree on how many they report: Claude sends the one window currently governing the account, Codex sends a primary/secondary pair.rate_limit_info—allowed_warningbecomeswarning, and a bare status with no figures yields an emptywindowsrather than a fabricated one.utilizationis treated as a 0–100 percentage, matching how the same SDK documents therate_limitswindows on its usage response.rejectedfromrateLimitReachedType, which is how Codex signals exhaustion — out of band from the windows themselves.The passthrough field is dropped rather than kept alongside: the native message already reaches consumers unchanged via
raw.payload, so nothing is lost.Scope
No behavior change — the event has no consumers, so this only makes it possible to add one. No UI, so no before/after images. No docs, since nothing user-visible changed.
Both adapters ship focused tests for the mapping, including the empty-windows case.
Verification
tsgo --noEmitinapps/serverandpackages/contracts— 0 errors; the 9 remaining diagnostics are pre-existingsuggestion-level hits inorchestration/decider.tsandorchestration/workflowScriptQuery.ts, none in the touched filesvp test run src/provider/Layers/CodexAdapter.test.ts src/provider/Layers/ClaudeAdapter.test.ts— 92 passedvp lintandvp format --checkclean on the five touched filesmainat the time of openingModel: Claude Opus 5. Harness: Claude Code.
🤖 Generated with Claude Code
Note
Low Risk
Contract and adapter-boundary normalization only; the event had no in-repo consumers, so runtime behavior for existing flows is unchanged aside from the emitted payload shape.
Overview
Replaces the untyped
rateLimitsblob onaccount.rate-limits.updatedwith a shared contract: optionalstatus(allowed|warning|rejected) and awindowsarray (kind,usedPercent, optionalresetsAt/windowDurationMins). Sparse updates are documented so consumers merge bykindinstead of replacing state wholesale.Claude now maps SDK
rate_limit_event.rate_limit_infothroughrateLimitsPayloadFromSdk(e.g.allowed_warning→warning; status-only events yield emptywindows).Codex maps
account/rateLimits/updatedthroughrateLimitsPayloadFromNotification(primary/secondary windows;rejectedonly when exhaustion signals are present, otherwise status is omitted).Adapter tests cover warning windows, bare status, multi-window snapshots, and spend-control rejection. Native payloads remain on
rawfor debugging.Reviewed by Cursor Bugbot for commit 9de6f0e. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Type the
account.rate-limits.updatedruntime payload with canonical status and windowsrateLimits: Unknownfield inAccountRateLimitsUpdatedPayload(providerRuntime.ts) with a structured schema: optionalstatus('allowed' | 'warning' | 'rejected') and awindowsarray ofRateLimitWindowobjects.rate_limit_eventmessages into the canonical payload via a newrateLimitsPayloadFromSdkhelper, mapping SDK statuses (e.g.'allowed_warning'→'warning').account/rateLimits/updatednotifications into canonical windows, settingstatus: 'rejected'whenspendControlReachedorrateLimitReachedTypeis set.{ status, windows }payload instead of a provider-specific blob; consumers reading the oldrateLimitsfield will receiveundefined.Macroscope summarized 9de6f0e.