Skip to content

fix(server): type the account rate-limit runtime payload - #5473

Open
vasars2024-hub wants to merge 2 commits into
pingdotgg:mainfrom
vasars2024-hub:fix/type-account-rate-limits-payload
Open

fix(server): type the account rate-limit runtime payload#5473
vasars2024-hub wants to merge 2 commits into
pingdotgg:mainfrom
vasars2024-hub:fix/type-account-rate-limits-payload

Conversation

@vasars2024-hub

@vasars2024-hub vasars2024-hub commented Aug 6, 2026

Copy link
Copy Markdown

Problem

account.rate-limits.updated is declared with rateLimits: Schema.Unknown in packages/contracts/src/providerRuntime.ts. Both adapters that emit it forward their provider's native message verbatim:

  • ClaudeAdapter.ts passes the whole SDKRateLimitEvent
  • CodexAdapter.ts passes the whole V2AccountRateLimitsUpdatedNotification

So 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.updated and AccountRateLimitsUpdated across 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/secondary windows with usedPercent, resetsAt, and windowDurationMins, and the Claude SDK types rate_limit_info with a status, window type, utilization, and reset time.

Change

Normalize at the adapter boundary, per "complexity belongs at the adapter boundary" in AGENTS.md:

const AccountRateLimitsUpdatedPayload = Schema.Struct({
  status: Schema.optional(RateLimitStatus),        // "allowed" | "warning" | "rejected"
  windows: Schema.Array(RateLimitWindow),          // { kind, usedPercent, resetsAt?, windowDurationMins? }
});

windows is 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.

  • Claude maps rate_limit_infoallowed_warning becomes warning, and a bare status with no figures yields an empty windows rather than a fabricated one. utilization is treated as a 0–100 percentage, matching how the same SDK documents the rate_limits windows on its usage response.
  • Codex maps the primary/secondary pair and derives rejected from rateLimitReachedType, 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 --noEmit in apps/server and packages/contracts — 0 errors; the 9 remaining diagnostics are pre-existing suggestion-level hits in orchestration/decider.ts and orchestration/workflowScriptQuery.ts, none in the touched files
  • vp test run src/provider/Layers/CodexAdapter.test.ts src/provider/Layers/ClaudeAdapter.test.ts — 92 passed
  • vp lint and vp format --check clean on the five touched files
  • Rebased on main at the time of opening

Model: 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 rateLimits blob on account.rate-limits.updated with a shared contract: optional status (allowed | warning | rejected) and a windows array (kind, usedPercent, optional resetsAt / windowDurationMins). Sparse updates are documented so consumers merge by kind instead of replacing state wholesale.

Claude now maps SDK rate_limit_event.rate_limit_info through rateLimitsPayloadFromSdk (e.g. allowed_warningwarning; status-only events yield empty windows).

Codex maps account/rateLimits/updated through rateLimitsPayloadFromNotification (primary/secondary windows; rejected only 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 raw for 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.updated runtime payload with canonical status and windows

  • Replaces the opaque rateLimits: Unknown field in AccountRateLimitsUpdatedPayload (providerRuntime.ts) with a structured schema: optional status ('allowed' | 'warning' | 'rejected') and a windows array of RateLimitWindow objects.
  • Updates ClaudeAdapter.ts to normalize SDK rate_limit_event messages into the canonical payload via a new rateLimitsPayloadFromSdk helper, mapping SDK statuses (e.g. 'allowed_warning''warning').
  • Updates CodexAdapter.ts to map account/rateLimits/updated notifications into canonical windows, setting status: 'rejected' when spendControlReached or rateLimitReachedType is set.
  • Behavioral Change: both adapters now emit a structured { status, windows } payload instead of a provider-specific blob; consumers reading the old rateLimits field will receive undefined.

Macroscope summarized 9de6f0e.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 56835ea5-b2be-42b2-9671-12b5c35cd708

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Aug 6, 2026
`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.
Comment thread apps/server/src/provider/Layers/CodexAdapter.ts Outdated
@@ -1393,16 +1428,18 @@ function mapToRuntimeEvents(
}

if (event.method === "account/rateLimits/updated") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.

Fix All in Cursor

❌ 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] : [],
};
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 60e720f. Configure here.

Comment thread apps/server/src/provider/Layers/CodexAdapter.ts
Comment thread apps/server/src/provider/Layers/CodexAdapter.ts
@macroscopeapp

macroscopeapp Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: 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.

@vasars2024-hub

Copy link
Copy Markdown
Author

Pushed 9de6f0e5. Two of the three findings were real; the third I've pushed back on.

Fixed — spendControlReached ignored. Correct, and I missed the field entirely. A spend-control-blocked account normalized to allowed.

Fixed — sparse updates. This is the more serious one, and the generated schema states it outright in the annotation on spendControlReached: "None is unavailable, not a sparse-update recovery." Asserting allowed from an absent rateLimitReachedType would let a usage-only delta clear a prior rejection.

Rather than merge state in the adapter, status is now claimed only on positive evidence of exhaustion — from either rateLimitReachedType or spendControlReached — and left absent otherwise. The field was already optional, so absent reads as "this update didn't say", which is what the wire actually carries. That keeps the adapter stateless and pushes the merge to whoever eventually consumes the event. I've documented that windows is sparse too, so consumers merge by kind rather than replace. Happy to switch to adapter-side merging if you'd rather the event always carry a complete snapshot — that's a design call I don't want to make unilaterally.

Pushing back — Claude utilization scale. Bugbot flags this as High severity, asserting utilization is a 0-1 fraction, but doesn't cite a source. My reading is 0-100, and the evidence is inside the same SDK: SDKControlGetUsageResponse.rate_limits documents utilization as "Percentage of the window used, 0-100" on five separate window types (five_hour, seven_day, seven_day_opus, seven_day_sonnet, model_scoped), for the same claude.ai plan windows, populated from the same internal cache. SDKRateLimitInfo.utilization is undocumented, so this is one SDK reusing one field name for one concept.

That said, it is undocumented, and being wrong here is a silent 100× error rather than a visible one. If anyone has an observed rate_limit_event payload with a populated utilization, that settles it in one line. Worth noting the field is omitted entirely in normal allowed operation, which is why the empty-windows path exists and is probably the common case.

Verification on 9de6f0e5: tsgo --noEmit clean in apps/server and packages/contracts (the 9 remaining diagnostics are pre-existing suggestion-level hits in orchestration/, none in touched files); both adapter test files pass, 93 tests; lint and format clean.

…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100-499 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant