feat(api): add named-parameter overloads and contract docs#20
Conversation
- add additive named-parameter overloads for selected public helpers while preserving existing positional signatures - document public API stability tiers and error contracts in reference docs - harden installer atomic writes with Windows retry semantics to stabilize concurrent invocation - add/extend tests for API compatibility and docs coverage Co-authored-by: Codex <noreply@openai.com>
📝 WalkthroughWalkthroughintroduces named-parameter overloads across five core library modules (parallel-probe, fetch-helpers, rate-limit-backoff, request-transformer, rotation) alongside new contract documentation (public-api.md, error-contracts.md), retry logic for file operations, and corresponding test coverage validating both invocation styles. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes critical flags
🚥 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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/reference/error-contracts.md`:
- Around line 82-87: The docs/reference/error-contracts.md contains relative
links that appear correct but aren’t covered by the existing internal-link unit
test; update test/documentation.test.ts (the internal link validation test that
currently only checks docs/README.md) to either scan all markdown files under
docs/ (recursively) or include docs/reference/error-contracts.md in its input
set so internal links like public-api.md, commands.md, ../troubleshooting.md and
../upgrade.md are validated; modify the test harness to collect all .md files
under the docs directory and run the same link-resolution/assertion logic used
for README.md.
In `@lib/parallel-probe.ts`:
- Around line 75-77: The runtime validation in getTopCandidates
(lib/parallel-probe.ts) is too weak: ensure the resolved account manager object
(accountManager / manager) is present and implements the expected interface
(e.g., has methods like rotateAuth or whatever manager methods you use) and that
resolvedMaxCandidates is a finite integer > 0 (not NaN, not negative, not
Infinity); if not, throw a clear TypeError indicating which param is invalid.
Update the input checks around resolvedModelFamily / resolvedMaxCandidates and
the account manager usage (the code paths that assume a valid manager at lines
referenced) to validate types before use, and add vitest regression tests in
test/parallel-probe.test.ts that assert invalid named params (missing/invalid
account manager) and invalid maxCandidates values (NaN, negative, zero,
Infinity) produce the expected TypeError; ensure tests mention auth-rotation,
concurrency, and handle EBUSY/429 considerations per repo guidelines.
In `@lib/request/rate-limit-backoff.ts`:
- Around line 138-157: The named-overload path for getRateLimitBackoffWithReason
does not validate accountIndex before calling getRateLimitBackoff, allowing an
invalid accountIndex in accountIndexOrParams to flow into shared 429 state;
update the guard alongside the existing quotaKey check in the function that
reads accountIndexOrParams (validate that resolvedAccountIndex is a finite
non-negative number or throw a TypeError) so you never call getRateLimitBackoff
with an invalid index, and add a Vitest regression in
test/rate-limit-backoff.test.ts that passes invalid named params (e.g.,
non-number or negative accountIndex) asserting the function throws and that
existing shared rate-limit state is not mutated by the call.
In `@lib/rotation.ts`:
- Around line 483-500: The exponential backoff computation can produce NaN when
attemptOrOptions is a named object missing attempt (or positional invalid), so
add input validation in the function that computes delay: verify
attemptOrOptions (and derived normalizedAttempt) is a finite positive integer
before using it to compute delay (symbols: attemptOrOptions, normalizedAttempt,
normalizedBaseMs, normalizedMaxMs, addJitter), and throw a clear TypeError for
invalid inputs to prevent NaN delays. Then add vitest regression cases in
test/rotation.test.ts covering invalid named input (e.g. {}) and invalid
positional inputs (e.g. undefined, non-number, negative, NaN) asserting the
function throws the expected error; mention the change in tests and ensure
addJitter is still called only after validation.
- Around line 349-363: The code now accepts a named-parameter overload but
immediately calls .filter on resolvedAccounts (in selectHybridAccount), which
can throw if a caller passes a malformed object; update selectHybridAccount to
validate that resolvedAccounts is an Array before using .filter and throw a
clear TypeError (e.g., "selectHybridAccount requires accounts to be an array")
when it's not; also add a vitest regression in test/rotation.test.ts that calls
selectHybridAccount with invalid named params (e.g., accounts: {} or accounts:
null) to assert the new TypeError is thrown, and ensure the test suite mentions
this change so CI covers the guard.
In `@scripts/install-codex-auth.js`:
- Around line 44-47: Add vitest regression tests in
test/install-codex-auth.test.ts that exercise the retry logic driven by
FILE_RETRY_CODES, FILE_RETRY_MAX_ATTEMPTS and FILE_RETRY_BASE_DELAY_MS in
scripts/install-codex-auth.js: mock the fs operations used by the installer (the
specific functions the installer calls around the retry block) to throw
transient errors (EBUSY/EPERM/EAGAIN/ENOTEMPTY) for the first N attempts and
then succeed, using vi.useFakeTimers() / advanceTimersByTime() to simulate
exponential backoff and assert the operation eventually succeeds and the
expected number of retries occurred; also add a case where the mock keeps
throwing beyond FILE_RETRY_MAX_ATTEMPTS and assert the installer surfaces the
error. Follow the pattern used in test/storage.test.ts:1407-1498 for structuring
the mocks, timers, and assertions.
In `@test/documentation.test.ts`:
- Around line 141-155: Update the test "documents public API stability tiers and
error contracts" to also assert that docs/reference/error-contracts.md contains
the "options-object compatibility contract" section and the six dual-call helper
names documented there: use the existing errorContracts variable and add
expect(errorContracts).toContain('options-object compatibility contract') plus
expect(...).toContain('<helperName>') for each of the six dual-call helper
function names (copy the exact names from the file into the test so future
removals are caught).
In `@test/fetch-helpers.test.ts`:
- Around line 240-255: The test only asserts three headers and can miss
regressions for other required headers; update the 'supports named-parameter
options form' test that calls createCodexHeaders(...) to assert full header
parity between the positional and named variants: add equality checks for
OPENAI_HEADERS.CONVERSATION_ID (or 'conversation_id'), any beta/originator
header (e.g., OPENAI_HEADERS.ORIGINATOR or the beta header key used in
createCodexHeaders), and the 'Accept' header, plus any other headers returned by
createCodexHeaders such as content-type or custom feature flags, ensuring
named.get(...) === positional.get(...) for each header key so the contract is
fully covered and deterministic under vitest.
In `@test/public-api-contract.test.ts`:
- Around line 17-27: The tuple destructuring in the test loses type safety
because `required` is inferred as (string | module)[][]; change `required` to a
readonly tuple type (e.g., `as const` or an explicit type like `Array<[string,
typeof rotation]>`) so `name` and `mod` have precise types, and add regression
assertions that call both positional and options-object overloads for each
exported API that gained named-parameter overloads (e.g., selectHybridAccount,
exponentialBackoff, getTopCandidates, createCodexHeaders,
getRateLimitBackoffWithReason, transformRequestBody) to ensure both call forms
compile and behave as expected.
In `@test/rotation.test.ts`:
- Around line 588-601: The test that mocks Math.random in the "supports
named-parameter options form" case should guarantee restore even if assertions
throw: wrap the mocking and assertions in a try/finally and call vi.spyOn(Math,
"random").mockRestore() in the finally block. Locate the test using the
exponentialBackoff calls (positional and named) and the vi.spyOn(Math, "random")
usage and move the mockRestore into a finally so Math.random is always restored
after the test.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (21)
.gitignoreREADME.mddocs/DOCUMENTATION.mddocs/README.mddocs/reference/commands.mddocs/reference/error-contracts.mddocs/reference/public-api.mddocs/releases/v0.1.1.mdlib/parallel-probe.tslib/request/fetch-helpers.tslib/request/rate-limit-backoff.tslib/request/request-transformer.tslib/rotation.tsscripts/install-codex-auth.jstest/documentation.test.tstest/fetch-helpers.test.tstest/parallel-probe.test.tstest/public-api-contract.test.tstest/rate-limit-backoff.test.tstest/request-transformer.test.tstest/rotation.test.ts
💤 Files with no reviewable changes (1)
- docs/releases/v0.1.1.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
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/public-api-contract.test.tstest/documentation.test.tstest/fetch-helpers.test.tstest/parallel-probe.test.tstest/rotation.test.tstest/rate-limit-backoff.test.tstest/request-transformer.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/reference/error-contracts.mddocs/reference/commands.mddocs/README.mddocs/DOCUMENTATION.mddocs/reference/public-api.md
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/rate-limit-backoff.tslib/rotation.tslib/request/request-transformer.tslib/parallel-probe.tslib/request/fetch-helpers.ts
🧬 Code graph analysis (9)
test/fetch-helpers.test.ts (2)
lib/request/fetch-helpers.ts (1)
createCodexHeaders(526-568)lib/constants.ts (1)
OPENAI_HEADERS(29-35)
test/parallel-probe.test.ts (1)
lib/parallel-probe.ts (1)
getTopCandidates(50-117)
lib/request/request-transformer.ts (2)
lib/types.ts (2)
RequestBody(92-113)UserConfig(12-21)lib/logger.ts (1)
logDebug(325-331)
test/rotation.test.ts (1)
lib/rotation.ts (3)
AccountWithMetrics(267-271)selectHybridAccount(341-428)exponentialBackoff(477-501)
test/rate-limit-backoff.test.ts (1)
lib/request/rate-limit-backoff.ts (2)
getRateLimitBackoffWithReason(132-169)clearRateLimitBackoffState(95-97)
scripts/install-codex-auth.js (1)
lib/utils.ts (1)
sleep(54-56)
test/request-transformer.test.ts (2)
lib/types.ts (1)
RequestBody(92-113)lib/request/request-transformer.ts (1)
transformRequestBody(843-1054)
lib/parallel-probe.ts (3)
lib/accounts.ts (2)
AccountManager(102-795)ManagedAccount(82-100)lib/prompts/codex.ts (1)
ModelFamily(47-52)lib/rotation.ts (3)
getHealthTracker(510-515)getTokenTracker(517-522)AccountWithMetrics(267-271)
lib/request/fetch-helpers.ts (1)
lib/constants.ts (2)
OPENAI_HEADERS(29-35)OPENAI_HEADER_VALUES(38-41)
🔇 Additional comments (19)
.gitignore (1)
12-12: good additive ignore rule for exact.tmpartifacts.this is a safe, backward-compatible cleanup. it complements existing wildcard ignores and does not introduce windows edge cases, concurrency risks, or regression-test needs. (reviewed at
.gitignore:12; nolib/...ortest/...lines were changed in this hunk.)test/request-transformer.test.ts (1)
2290-2332: good parity test for named transformrequestbody calls.this cleanly validates positional vs named equivalence and keeps the setup deterministic (
test/request-transformer.test.ts:2312,test/request-transformer.test.ts:2321).test/parallel-probe.test.ts (1)
264-288: good named-params compatibility check for gettopcandidates.this is deterministic and directly validates backward-compatible behavior (
test/parallel-probe.test.ts:274,test/parallel-probe.test.ts:280).lib/request/fetch-helpers.ts (1)
517-550: named and positional header construction is clean and backward-compatible.the resolved-param flow is consistent and the required account/token guard is explicit (
lib/request/fetch-helpers.ts:548-550), while preserving existing header behavior.Also applies to: 551-567
test/rotation.test.ts (1)
515-530: good compatibility coverage for object-form parity.
test/rotation.test.ts:515(line 515) correctly validates named-parameter behavior against positional baseline forselectHybridAccount. this is deterministic and aligned with the new api contract.lib/request/request-transformer.ts (2)
24-32: interface addition is clean and additive.
lib/request/request-transformer.ts:24(line 24) defines a clear options object contract and keeps backward compatibility with positional overloads.
895-999: resolved-parameter wiring is consistent across fast-session and codex-mode branches.
lib/request/request-transformer.ts:895(line 895) throughlib/request/request-transformer.ts:991(line 991) correctly routes named and positional inputs through shared resolved variables without changing core behavior.README.md (1)
262-263: docs index links are consistent with the new contract pages.
README.md:262(line 262) andREADME.md:263(line 263) correctly expose the new reference docs from the project entrypoint.docs/reference/commands.md (1)
118-121: related section update is coherent and link-safe.
docs/reference/commands.md:118(line 118) adds the public api and error contract references without dropping core navigation.As per coding guidelines
docs/**: keep README, SECURITY, and docs consistent with actual cli flags and workflows.docs/DOCUMENTATION.md (1)
32-33: source-of-truth map update is correct.
docs/DOCUMENTATION.md:32(line 32) anddocs/DOCUMENTATION.md:33(line 33) correctly register the new contract docs in governance mapping.As per coding guidelines
docs/**: keep README, SECURITY, and docs consistent with actual cli flags and workflows.docs/reference/public-api.md (1)
49-63: calling-style guidance is clear and matches the new overload strategy.
docs/reference/public-api.md:49(line 49) throughdocs/reference/public-api.md:63(line 63) clearly documents options-object preference while preserving positional compatibility.As per coding guidelines
docs/**: whenever behavior changes, require docs consistency with actual workflows.docs/README.md (1)
40-41: portal reference entries are aligned and discoverable.
docs/README.md:40(line 40) anddocs/README.md:41(line 41) correctly surface both new contract documents in the reference section.As per coding guidelines
docs/**: keep internal links valid and reflect new contract references.test/rate-limit-backoff.test.ts (1)
158-168: named-parameter parity test is deterministic and solid.
test/rate-limit-backoff.test.ts:158(line 158) validates object-form parity against positional behavior, andtest/rate-limit-backoff.test.ts:160(line 160) correctly resets state to avoid cross-call coupling.As per coding guidelines
test/**: tests must stay deterministic and use vitest.test/public-api-contract.test.ts (1)
3-9: lgtm on root export alignmentdeterministic, uses vitest, and validates the aliasing contract (
OpenAIAuthPlugin === OpenAIOAuthPlugin === default). straightforward and correct.test/documentation.test.ts (2)
19-20: lgtm on userDocs array additionsadding the new reference paths keeps the doc presence checks aligned with the pr's new documentation pages.
72-73: lgtm on portal link assertionsvalidates that
docs/README.mdreferences the new contract pages. consistent with the coding guidelines requiring docs consistency.docs/reference/error-contracts.md (3)
67-79: options-object section aligns with pr objectivesdocuments the six dual-call helpers from the pr summary. this matches the named-parameter overloads added in
lib/rotation.ts,lib/parallel-probe.ts,lib/request/fetch-helpers.ts,lib/request/rate-limit-backoff.ts, andlib/request/request-transformer.ts.
56-58: no action needed - documentation is accurate
handleErrorResponsecorrectly exports and returnsrateLimit.retryAfterMsmetadata as documented. verified at lib/request/fetch-helpers.ts:575, RateLimitInfo interface at :39-42, and extractRateLimitInfoFromBody at :693.
32-38: all documented--jsoncommands are implemented with test coverageall five commands documented in lines 32-38 (forecast, report, fix, doctor, verify-flagged) are correctly implemented in lib/codex-manager.ts with arg parsing for
--json/-jflags, and each has regression tests validating JSON output format in test/codex-manager-cli.test.ts (forecast at 240, report at 1108, fix at 412, doctor at 1035, verify-flagged at 333). documentation is accurate and no drift detected.
- validate named/positional overload inputs for parallel probe, rotation, rate-limit backoff, and request transformer APIs - harden selectHybridAccount and exponentialBackoff against malformed runtime inputs - move installer file-operation retry helpers into shared utils and reuse from installer script Co-authored-by: Codex <noreply@openai.com>
- add regression tests for overload guard behavior and parity across named/positional forms - expand docs integrity checks to validate internal links recursively under docs/ - verify error-contract docs include options-object compatibility section and helper list - add deterministic retry-helper tests for installer transient filesystem errors Co-authored-by: Codex <noreply@openai.com>
Fixed items
Deferred items
Blocked items
Validation evidence
|
Fixed items
Deferred items
Blocked items
Validation evidence
|
|
@greptile |
Fixed items
Deferred items
Blocked items
Validation evidencepm run clean:repo:check - passpm run audit:ci - passpm run typecheck - passpm run lint - passpm run build - passpm test - pass (92 files, 2113 tests)
|
Resolves all open Dependabot alerts on package-lock.json: - hono <4.12.14: JSX SSR HTML injection, cookie name bypass, IPv4-mapped IPv6 ipRestriction, setCookie validation, serveStatic repeated-slash bypass, toSSG path traversal (alerts #16, #18, #20, #22, #24, #26) - vite <7.3.2: dev server WebSocket arbitrary file read, optimized deps .map path traversal, server.fs.deny query bypass (alerts #12, #13, #14) Lockfile refreshed via npm install --package-lock-only. Typecheck, lint, and 3418/3418 tests pass.
Resolves all open Dependabot alerts on package-lock.json: - hono <4.12.14: JSX SSR HTML injection, cookie name bypass, IPv4-mapped IPv6 ipRestriction, setCookie validation, serveStatic repeated-slash bypass, toSSG path traversal (alerts #16, #18, #20, #22, #24, #26) - vite <7.3.2: dev server WebSocket arbitrary file read, optimized deps .map path traversal, server.fs.deny query bypass (alerts #12, #13, #14) Lockfile refreshed via npm install --package-lock-only. Typecheck, lint, and 3418/3418 tests pass.
1 participant
Summary
What Changed
lib/rotation.tsselectHybridAccount(params: SelectHybridAccountParams)exponentialBackoff(options: ExponentialBackoffOptions)lib/parallel-probe.tsgetTopCandidates(params: GetTopCandidatesParams)lib/request/fetch-helpers.tscreateCodexHeaders(params: CreateCodexHeadersParams)lib/request/rate-limit-backoff.tsgetRateLimitBackoffWithReason(params: RateLimitBackoffWithReasonParams)lib/request/request-transformer.tstransformRequestBody(params: TransformRequestBodyParams)docs/reference/public-api.mddocs/reference/error-contracts.mdREADME.md,docs/README.md,docs/DOCUMENTATION.md,docs/reference/commands.mdscripts/install-codex-auth.jsValidation
npm run lintnpm run typechecknpm testnpm test -- test/documentation.test.tsnpm run buildnpm run test:coverage(fails on existing global branch threshold: 77.41% < 80%)npm run audit:ci(fails on upstream advisory:honoGHSA-xh87-mx6m-69f3)Docs and Governance Checklist
docs/getting-started.mdupdated (not applicable)docs/features.mdupdated (not applicable)docs/reference/*pages updated (if commands/settings/paths changed)docs/upgrade.mdupdated (not applicable; no breaking migration)SECURITY.mdandCONTRIBUTING.mdreviewed for alignmentRisk and Rollback
03f1b27614c65d01c1805d86d7b444a2bfa55781to restore prior API/docs/installer behavior.Additional Notes
origin/mainconfirms changes are additive and preserve prior positional signatures.API Review
Summary
Overall: APPROVED
Breaking Changes: NONE
Breaking Changes Found
API Design Issues
Error Contract Issues
docs/reference/error-contracts.md.Versioning Recommendation
Suggested bump: MINOR
Rationale: Introduces new backward-compatible call forms and contract documentation without removing or changing existing public signatures.
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
added backward-compatible named-parameter overloads to six public helpers (
selectHybridAccount,exponentialBackoff,getTopCandidates,createCodexHeaders,getRateLimitBackoffWithReason,transformRequestBody) to reduce positional-argument brittleness. hardened installer atomic write path with retry logic for windows filesystem contention (EBUSY, EPERM, EAGAIN, ENOTEMPTY errors). added comprehensive API stability tier documentation and error contract reference pages.key changes:
windows filesystem safety:
Confidence Score: 4/5
Important Files Changed
selectHybridAccountandexponentialBackoffwith runtime guards; preserves positional signatureswithFileOperationRetryhelper with exponential backoff (25ms base, 6 attempts) for transient windows filesystem errorsgetTopCandidateswith runtime validation; detects call form viatypeof modelFamily === "undefined"createCodexHeaders; detection uses multiple undefined checks and property presencegetRateLimitBackoffWithReason; includes quotaKey trimming and comprehensive validationtransformRequestBody; complex detection logic with fallback defaults preservedLast reviewed commit: 5eb15c1
Context used:
dashboard- What: Every code change must explain how it defends against Windows filesystem concurrency bugs and ... (source)