feat(codex): show and use banked resets - #7813
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 |
There was a problem hiding this comment.
Reviewed the new banked-reset service/RPC path against the Effect service conventions. Three findings, all in the error-modeling and change-discipline rules; the adapter/registry wiring and layer usage otherwise look consistent with the existing provider modules.
Posted via Macroscope — Effect Service Conventions
| "ServerProviderRateLimitResetError", | ||
| { | ||
| instanceId: ProviderInstanceId, | ||
| reason: TrimmedNonEmptyString, |
There was a problem hiding this comment.
Every construction of this error wraps a real failure at the RPC boundary, but there is no cause field, so the underlying provider error cannot be preserved. Suggest adding an optional defect cause, matching ServerProviderUpdateError in this file.
| reason: TrimmedNonEmptyString, | |
| reason: TrimmedNonEmptyString, | |
| cause: Schema.optional(Schema.Defect()), |
Posted via Macroscope — Effect Service Conventions
| const connectCodexAppServer = Effect.fn("connectCodexAppServer")(function* ( | ||
| input: CodexAppServerConnectionInput, | ||
| ) { | ||
| const resolvedHomePath = input.homePath ? expandHomePath(input.homePath) : undefined; |
There was a problem hiding this comment.
The invariant comment explaining why homePath is expanded here (~ is not shell-expanded for env vars passed to child_process.spawn) was dropped while this code moved into connectCodexAppServer. Suggest restoring it so the reason for expandHomePath stays documented.
| const resolvedHomePath = input.homePath ? expandHomePath(input.homePath) : undefined; | |
| // `~` is not shell-expanded when env vars are set via `child_process.spawn`, | |
| // so `CODEX_HOME=~/.codex_work` would reach codex verbatim and trip | |
| // "CODEX_HOME points to '~/.codex_work', but that path does not exist". | |
| // Expand here for parity with `CodexTextGeneration`/`CodexSessionRuntime`. | |
| const resolvedHomePath = input.homePath ? expandHomePath(input.homePath) : undefined; |
Posted via Macroscope — Effect Service Conventions
| Effect.mapError( | ||
| (cause) => | ||
| new ServerProviderRateLimitResetError({ | ||
| instanceId: input.instanceId, | ||
| reason: cause.message, |
There was a problem hiding this comment.
The wrapper's reason is copied from cause.message and the underlying error is then discarded, so the caller-visible message is derived from the cause and the error chain/stack is lost. Consider a stable reason derived from this boundary (as providerMaintenanceRunner.ts does for ServerProviderUpdateError) and passing the original failure through as cause, e.g. reason: "Codex rejected the banked reset request.", cause. Requires the cause field noted on packages/contracts/src/server.ts.
Posted via Macroscope — Effect Service Conventions
There was a problem hiding this comment.
One finding on the new banked-reset UI. Everything else lines up with the shared primitives: Button/AlertDialog* are used instead of raw controls, the AlertDialogClose render={<Button variant="outline" />} pattern matches existing confirm dialogs, and the panel keeps only contextual layout/color at the call site without overriding primitive geometry.
Posted via Macroscope — UI Consistency
| open={resetTarget !== null} | ||
| onOpenChange={(open) => { | ||
| if (!open && !usingReset) setResetTarget(null); | ||
| }} |
There was a problem hiding this comment.
The dialog's open state and its content are the same resetTarget value, so clearing it (here on Cancel, and on success at line 163) blanks the copy while the popup is still visible: AlertDialogPopup composes DIALOG_POPUP_CLASS, which animates out over 200ms via data-ending-style, so the description briefly renders as "This uses one reset for . It resets both limits…".
Suggest keeping the target until the close completes, as the other in-flight confirm dialogs do (ConnectionsSettings.tsx defers clearing its pending value in onOpenChangeComplete): drive open from a separate boolean, and clear resetTarget in onOpenChangeComplete instead of in onOpenChange/the success path.
Posted via Macroscope — UI Consistency
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR introduces a complete new feature: displaying Codex rate limits and enabling users to consume banked resets. It spans contracts, server RPC with authorization, provider adapters, client state, and a new UI component — new user-facing capability warrants human review. You can add or adjust custom eligibility rules. Learn more. |
Why
Codex users can earn banked resets, but T3 Code did not show them or let users redeem one.
What changed
Test plan
Visual check
Design mockup was reviewed before implementation. Browser screenshots were not captured because computer use was not authorized for this run.
Built with GPT-5.6 Sol via Codex in T3 Code.
Note
Add banked rate-limit reset support for Codex providers
server.consumeProviderRateLimitResetRPC in rpc.ts and server.tsconsumeRateLimitResetCreditin the Codex adapter (CodexAdapter.ts) andProviderRegistry(ProviderRegistry.ts), which consumes a banked reset then performs a best-effort refreshrateLimitsfrom the Codex app-server API in snapshots (CodexProvider.ts)CodexLimitsPanelUI component and wires a confirmation modal in UsagePage.tsx to trigger the reset via the new client-runtime commandserverConsumeProviderRateLimitResetRPC requiresAuthOrchestrationOperateScopein RpcAuthorization.ts; existing clients without this scope will get authorization errorsMacroscope summarized 533a8ca.