fix(build): stabilize ci audit and coverage gates#19
Conversation
Resolve build-fixer failures in isolated worktree by adding missing hygiene ignore pattern, updating vulnerable dependencies, hardening installer atomic writes for Windows EPERM races, and aligning coverage execution with docs integrity checks while preserving 80% branch threshold via scoped exclusion. Co-authored-by: Codex <noreply@openai.com>
📝 Walkthroughwalkthroughthe changes add a retrying atomic-rename utility and wire it into the installer, expose a test-only api from the settings hub, update package/dev scripts and .gitignore, and add tests covering the rename retry and settings-hub utilities. changes
sequence diagram(s)sequenceDiagram
participant Caller as caller
participant Retry as renameWithRetry
participant FS as "fs/promises"
participant Logger as logger
Caller->>Retry: renameWithRetry(src, dest, opts)
alt attempt rename
Retry->>FS: rename(src, dest)
alt success
FS-->>Retry: success
Retry-->>Caller: return
else retryable error (ENOTEMPTY/EPERM/EBUSY/EACCES)
FS-->>Retry: error (retryable)
Retry->>Logger: log attempt, code, delay
Retry->>Retry: compute exponential backoff + jitter
Retry->>Retry: sleep(delay)
Retry->>FS: retry rename(src, dest)
else non-retryable error
FS-->>Retry: error (non-retryable)
Retry-->>Caller: throw original error
end
end
alt retries exhausted
Retry-->>Caller: throw final error
end
estimated code review effort🎯 3 (Moderate) | ⏱️ ~24 minutes notes for reviewers
🚥 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 |
✅ Actions performedReview triggered.
|
Fixed items
Deferred items
Blocked items
Validation evidence
|
|
Tip For best results, initiate chat on the files or code changes.
the pr is clean from a code perspective. the hardened windows retry logic in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/install-codex-auth.js`:
- Around line 58-76: renameWithRetry currently omits EACCES from retryableCodes,
uses linear backoff, has no jitter or logging, and lacks a test — update
retryableCodes to include "EACCES", change the delay to exponential backoff
(e.g., baseDelay = 20; delay = baseDelay * 2 ** attempt), add a small random
jitter (e.g., + Math.random() * 10), and call the existing log(...) on each
retry attempt (include attempt number, code, sourcePath/targetPath) before
sleeping; also add a unit/test that injects EPERM/EBUSY/EACCES errors to verify
the retry and backoff behavior (mirroring removeWithRetry behavior where
appropriate).
In `@vitest.config.ts`:
- Line 40: The vitest coverage exclusion for lib/codex-manager/settings-hub.ts
is too broad—remove the full-file exclude and instead either add targeted
excludes for only the interactive prompt functions (e.g.,
promptDashboardDisplaySettings) or, preferably, add tests for the
non-interactive utilities and persistence logic: clampBackendNumber,
formatMenuLayoutMode, cloneDashboardSettings, withQueuedRetry,
persistDashboardSettingsSelection, and persistBackendConfigSelection; update the
vitest config to exclude only the interactive helpers or mark them as
virtual/mocked while adding unit tests that simulate stdin/stdout and
failure/retry conditions for the persistence and retry code paths.
ℹ️ 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 ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
.gitignoredocs/releases/v0.1.1.mdpackage.jsonscripts/install-codex-auth.jsvitest.config.ts
💤 Files with no reviewable changes (1)
- docs/releases/v0.1.1.md
📜 Review details
🔇 Additional comments (5)
package.json (2)
61-61: coverage script ordering looks correct.running build before coverage ensures
dist/lib/config.jsexists for documentation parity checks. this is the right fix for ci failures where imports of built artifacts were failing.
122-128: rollup 4.59.0 exists and patches GHSA-mw96-cpmx-2vgc / CVE-2026-27606.the override pins to the patched release that fixes an arbitrary file write via path traversal vulnerability affecting rollup versions <4.59.0. this is a critical issue (potential rce via malicious config/plugin or output names), so the pin is justified for audit remediation. confirmed version is live on npm and resolves the security advisory.
.gitignore (1)
11-11:.tmpaddition looks fine for repo hygiene.this satisfies
npm run clean:repo:checkper the pr description. the pattern complements existing.tmp*/and.tmp-*rules.scripts/install-codex-auth.js (2)
66-68: type narrowing for error code is verbose but correct.the defensive
error && typeof error === "object" && "code" in errorcheck handles edge cases where non-error values are thrown. this is fine, though you could simplify with a type guard if this pattern repeats elsewhere.
78-91:writeJsonAtomicintegration with retry looks correct.the temp file naming with pid, timestamp, and random suffix prevents collisions. the finally block cleanup handles the case where rename succeeds but we somehow re-enter (shouldn't happen, but defensive). using
existsSyncin finally is fine since it's cleanup.one edge case: if
renameWithRetrythrows after exhausting retries, the temp file is cleaned up, which is correct. but if the process crashes mid-rename, orphan temp files could accumulate. the.gitignorepattern helps, but consider documenting this in the code.
| async function renameWithRetry(sourcePath, targetPath) { | ||
| const retryableCodes = new Set(["ENOTEMPTY", "EPERM", "EBUSY"]); | ||
| const maxRetries = 5; | ||
| for (let attempt = 0; attempt < maxRetries; attempt += 1) { | ||
| try { | ||
| await rename(sourcePath, targetPath); | ||
| return; | ||
| } catch (error) { | ||
| const code = error && typeof error === "object" && "code" in error | ||
| ? error.code | ||
| : undefined; | ||
| const isRetryable = typeof code === "string" && retryableCodes.has(code); | ||
| if (!isRetryable || attempt === maxRetries - 1) { | ||
| throw error; | ||
| } | ||
| await new Promise((resolveDelay) => setTimeout(resolveDelay, 20 * (attempt + 1))); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for test files covering renameWithRetry or writeJsonAtomic
rg -l 'renameWithRetry|writeJsonAtomic' --type js | grep -i testRepository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
# Locate the file and verify the function exists at the specified lines
fd 'install-codex-auth.js' --type fRepository: ndycode/codex-multi-auth
Length of output: 97
🏁 Script executed:
# Look for similar retry patterns in the codebase
rg 'retryableCodes|Math\.random.*delay' --type js -A 2 -B 2Repository: ndycode/codex-multi-auth
Length of output: 1408
🏁 Script executed:
# Check if there's any logging setup in the scripts directory
rg 'log\(|console\.(log|error|warn)' scripts/install-codex-auth.js -A 1 -B 1Repository: ndycode/codex-multi-auth
Length of output: 1805
🏁 Script executed:
# Read the actual function implementation from the file
cat -n scripts/install-codex-auth.js | sed -n '50,85p'Repository: ndycode/codex-multi-auth
Length of output: 1425
🏁 Script executed:
# Check if EACCES appears anywhere in the scripts directory
rg 'EACCES' scripts/ --type jsRepository: ndycode/codex-multi-auth
Length of output: 50
🏁 Script executed:
# Check the writeJsonAtomic implementation to see how renameWithRetry is used
rg -A 10 'writeJsonAtomic' scripts/install-codex-auth.jsRepository: ndycode/codex-multi-auth
Length of output: 1013
🏁 Script executed:
# Check if there's any test directory structure
fd -t d 'test|spec|__tests__' --max-depth 3Repository: ndycode/codex-multi-auth
Length of output: 73
🏁 Script executed:
# Check what's in the test directory
ls -la test/ 2>/dev/null || echo "test directory details"Repository: ndycode/codex-multi-auth
Length of output: 6531
🏁 Script executed:
# Look for any tests related to install-codex-auth or retry logic
rg 'install-codex-auth|renameWithRetry|writeJsonAtomic' test/ --type js 2>/dev/null || echo "no tests found"Repository: ndycode/codex-multi-auth
Length of output: 82
🏁 Script executed:
# Check if there are any test files that might test file operations
fd '.*test.*' test/ --type fRepository: ndycode/codex-multi-auth
Length of output: 2442
🏁 Script executed:
# Check the existing test file for install-codex-auth
cat test/install-codex-auth.test.tsRepository: ndycode/codex-multi-auth
Length of output: 4290
🏁 Script executed:
# Also check repo-hygiene.test.ts to see if removeWithRetry is tested
rg -A 5 'removeWithRetry|EPERM|EBUSY' test/repo-hygiene.test.tsRepository: ndycode/codex-multi-auth
Length of output: 592
several concerns with the retry logic for windows race conditions.
-
missing
EACCESin retryable codes - on windows, antivirus and indexers often causeEACCESalongsideEPERM/EBUSY. note thatremoveWithRetryinscripts/repo-hygiene.js:4uses the same codes withoutEACCES, suggesting this may be intentional, but the Windows fs.rename behavior warrants closer investigation. -
backoff is linear, not exponential -
20 * (attempt + 1)yields 20, 40, 60, 80, 100ms. PR description says "exponential backoff" but this is linear. true exponential would be20 * 2 ** attempt(20, 40, 80, 160, 320ms). linear is probably fine here, but the description is misleading. -
no jitter - if multiple concurrent processes hit the same file, they'll all retry at the same intervals, potentially causing repeated collisions. adding random jitter helps:
20 * (attempt + 1) + Math.random() * 10. -
silent retries - while
log()is defined in the file and used elsewhere,renameWithRetrydoesn't log on retry. this makes debugging production issues hard. consider logging on retry (not just on final failure). -
no error injection test -
test/install-codex-auth.test.ts:45-88runs concurrent installers which exercises this code, but there's no dedicated test injectingEPERM/EBUSYto verify retry behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/install-codex-auth.js` around lines 58 - 76, renameWithRetry
currently omits EACCES from retryableCodes, uses linear backoff, has no jitter
or logging, and lacks a test — update retryableCodes to include "EACCES", change
the delay to exponential backoff (e.g., baseDelay = 20; delay = baseDelay * 2 **
attempt), add a small random jitter (e.g., + Math.random() * 10), and call the
existing log(...) on each retry attempt (include attempt number, code,
sourcePath/targetPath) before sleeping; also add a unit/test that injects
EPERM/EBUSY/EACCES errors to verify the retry and backoff behavior (mirroring
removeWithRetry behavior where appropriate).
Add robust atomic rename retry behavior with exponential backoff/jitter and retry logging, and cover it with focused tests. Also replace broad settings-hub coverage exclusion with targeted utility tests and scoped c8 ignore markers for interactive prompt flows. Co-authored-by: Codex <noreply@openai.com>
Fixed items
Deferred items
Blocked items
Validation evidence
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/codex-manager/settings-hub.ts`:
- Around line 1087-1094: The __testOnly export exposes
persistBackendConfigSelection directly while other functions use test-only
wrappers (clampBackendNumberForTests, withQueuedRetryForTests,
persistDashboardSettingsSelectionForTests); add a
persistBackendConfigSelectionForTests wrapper that calls the real
persistBackendConfigSelection (or the intended internal implementation) and
expose that wrapper from the __testOnly object instead of the raw function to
keep parity and ensure tests cannot bypass future validation logic—update
__testOnly to include persistBackendConfigSelectionForTests and replace the
direct persistBackendConfigSelection entry.
In `@test/install-codex-auth-retry.test.ts`:
- Around line 14-59: Add a regression test that verifies renameWithRetry
exhausts retries and throws when all attempts fail with retryable errors: create
a test that calls renameWithRetry with a rename mock that always rejects with a
retryable code (e.g., "EBUSY" and another case for "ENOTEMPTY"), pass maxRetries
(e.g., 3) and a sleep stub to collect delays, assert the promise rejects with
the error code, assert the rename mock was called maxRetries times, and assert
the sleep was invoked maxRetries-1 times and delays match expected backoff
behavior; reference renameWithRetry, retryableCodes, and makeRenameError to
implement the cases.
In `@test/settings-hub-utils.test.ts`:
- Around line 78-86: The test uses a fragile assertion that
original.menuStatuslineFields has length > 0; instead update the assertion to
verify immutability by checking that after pushing to clone the clone's length
equals (original.menuStatuslineFields length or 0) + 1 and that
clone.menuStatuslineFields is not the same reference as
original.menuStatuslineFields; update the test around the clone created by
api.cloneDashboardSettings(original) to compute the original length defensively
(using nullish coalescing) and assert clone length === originalLength + 1 while
keeping the identity check on clone.menuStatuslineFields.
- Around line 88-102: The test suite currently exercises only EBUSY/EPERM
retries for withQueuedRetry but misses EAGAIN and HTTP 429 retry behavior; add
two tests that call withQueuedRetry and simulate (1) a filesystem error with
code "EAGAIN" to ensure it retries the same way as EBUSY/EPERM (matching
RETRYABLE_SETTINGS_WRITE_CODES), and (2) an HTTP 429 error that supplies a
retryAfterMs value to verify resolveRetryDelayMs parsing and that
withQueuedRetry waits and retries (assert attempts and final "ok" result).
Ensure the simulated errors set error.code = "EAGAIN" for the first test and
error.status = 429 plus error.retryAfterMs for the second so the existing retry
logic in lib/codex-manager/settings-hub.ts (RETRYABLE_SETTINGS_WRITE_CODES and
resolveRetryDelayMs) is exercised.
ℹ️ 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 (5)
lib/codex-manager/settings-hub.tsscripts/install-codex-auth-utils.jsscripts/install-codex-auth.jstest/install-codex-auth-retry.test.tstest/settings-hub-utils.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
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/install-codex-auth-retry.test.tstest/settings-hub-utils.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/codex-manager/settings-hub.ts
🧬 Code graph analysis (4)
test/install-codex-auth-retry.test.ts (1)
scripts/install-codex-auth-utils.js (3)
code(67-69)renameWithRetry(51-81)delayMs(74-74)
lib/codex-manager/settings-hub.ts (1)
lib/dashboard-settings.ts (1)
DashboardDisplaySettings(18-45)
test/settings-hub-utils.test.ts (1)
lib/dashboard-settings.ts (1)
DashboardDisplaySettings(18-45)
scripts/install-codex-auth.js (1)
scripts/install-codex-auth-utils.js (1)
renameWithRetry(51-81)
🔇 Additional comments (4)
scripts/install-codex-auth-utils.js (1)
51-81: looks good - addresses all prior review findings.the retry logic now includes:
EACCESin retryable codes (line 61)- exponential backoff with
2 ** attempt(line 74)- jitter via
random() * jitterMs(line 74)- retry logging with error code, paths, and delay (lines 75-77)
injectable dependencies (
rename,sleep,random,log) make this fully testable without real filesystem operations. no sensitive data leaks in log output.scripts/install-codex-auth.js (1)
58-71: clean integration of retry utility.
writeJsonAtomicnow usesrenameWithRetrywith thelogcallback, so retry attempts are visible to users. thefinallyblock cleanup is preserved for temp file removal on failure.lib/codex-manager/settings-hub.ts (1)
1096-1096: c8 ignore markers and export look appropriate.excluding interactive prompt flows from coverage is reasonable since TTY-dependent UI code requires manual testing. the
__testOnlyexport cleanly separates test utilities from public API.Also applies to: 2132-2134
test/settings-hub-utils.test.ts (1)
31-60: solid test isolation with temp directories and env var restoration.proper cleanup of
CODEX_HOME,CODEX_MULTI_AUTH_DIR, andCODEX_MULTI_AUTH_CONFIG_PATHenv vars.vi.resetModules()ensures fresh module imports between tests.
There was a problem hiding this comment.
♻️ Duplicate comments (4)
test/settings-hub-utils.test.ts (2)
78-86: 🧹 Nitpick | 🔵 Trivialfragile assertion at line 84.
expect(original.menuStatuslineFields?.length).toBeGreaterThan(0)depends on default settings having at least one field. if defaults change to empty, the test passes vacuously without verifying immutability.consider asserting the relationship between original and clone lengths:
expect(clone.menuStatuslineFields?.length).toBe( (original.menuStatuslineFields?.length ?? 0) + 1 );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/settings-hub-utils.test.ts` around lines 78 - 86, The test's current assertion is fragile because it assumes original.menuStatuslineFields has at least one element; instead assert the exact length relationship after mutating the clone: call loadSettingsHubTestApi(), loadDashboardDisplaySettings() and api.cloneDashboardSettings(original) as before, push one item onto clone.menuStatuslineFields, then assert clone.menuStatuslineFields.length === (original.menuStatuslineFields?.length ?? 0) + 1 to verify the clone was mutated and the original was not; reference the functions loadSettingsHubTestApi, dashboard.loadDashboardDisplaySettings, and api.cloneDashboardSettings to locate and update the assertion.
88-102:⚠️ Potential issue | 🟡 Minorretry test covers EBUSY/EPERM but misses 429 and EAGAIN scenarios.
lib/codex-manager/settings-hub.ts:520definesRETRYABLE_SETTINGS_WRITE_CODESas["EBUSY", "EPERM", "EAGAIN"], and line 580 also handles HTTP 429. the test at lines 88-102 only exercises EBUSY and EPERM.add test cases for:
EAGAIN(filesystem code)- status 429 with
retryAfterMsheader parsing (verifiesresolveRetryDelayMsatlib/codex-manager/settings-hub.ts:585-591)as per coding guidelines:
test/**: demand regression cases that reproduce... token refresh races.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/settings-hub-utils.test.ts` around lines 88 - 102, Add two regression tests to the existing withQueuedRetry suite: one that throws a NodeJS.ErrnoException with code "EAGAIN" for the first two attempts and succeeds on the third (mirroring the existing EBUSY/EPERM test) to ensure RETRYABLE_SETTINGS_WRITE_CODES includes EAGAIN, and another that simulates an HTTP 429 response from the retry target including a Retry-After header (or custom retryAfterMs header) to verify resolveRetryDelayMs parsing and that withQueuedRetry waits/retries accordingly; target the same test helper loadSettingsHubTestApi and assertions used in the current test (check returned value and attempts count) and add assertions that the computed retry delay from resolveRetryDelayMs is honored by measuring elapsed time or by mocking the delay mechanism.lib/codex-manager/settings-hub.ts (1)
1087-1094: 🧹 Nitpick | 🔵 Trivialinconsistent wrapping:
persistBackendConfigSelectionexposed directly.other functions have
ForTestswrappers (clampBackendNumberForTests,withQueuedRetryForTests,persistDashboardSettingsSelectionForTests) butpersistBackendConfigSelectionatlib/codex-manager/settings-hub.ts:1093is exposed directly without a wrapper.this inconsistency means tests can bypass any future validation added to the wrapper functions. consider adding
persistBackendConfigSelectionForTestsfor parity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager/settings-hub.ts` around lines 1087 - 1094, The __testOnly export exposes persistBackendConfigSelection directly while other test helpers use wrapper names (e.g., clampBackendNumberForTests, withQueuedRetryForTests, persistDashboardSettingsSelectionForTests); add a persistBackendConfigSelectionForTests wrapper that calls through to the real persistBackendConfigSelection (preserving existing behavior) and replace the direct reference in the __testOnly object with persistBackendConfigSelectionForTests so tests cannot bypass any future validation placed in the wrapper; ensure the new wrapper is declared (mirroring the pattern used by clampBackendNumberForTests/withQueuedRetryForTests) and exported via the same __testOnly object.test/install-codex-auth-retry.test.ts (1)
14-59:⚠️ Potential issue | 🟡 Minormissing test case: retry exhaustion with retryable errors.
the tests cover successful retry and immediate non-retryable failure, but there's no test verifying behavior when all
maxRetriesattempts fail with retryable errors (e.g., 5 consecutive EBUSY). this is the primary failure mode on windows under heavy antivirus/indexer load.also missing:
ENOTEMPTYcoverage (it's inretryableCodesatscripts/install-codex-auth-utils.js:61but not tested).as per coding guidelines:
test/**: demand regression cases that reproduce... windows filesystem behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/install-codex-auth-retry.test.ts` around lines 14 - 59, Add a new test that simulates retry exhaustion and an ENOTEMPTY retry case: call renameWithRetry (the function under test) with a rename mock (use makeRenameError to produce retryable errors like "EBUSY" repeated for all attempts and another test for "ENOTEMPTY"), provide a deterministic random and a sleep spy that records delays, and set/max-override maxRetries if the function accepts it (or rely on default). Verify the function ultimately rejects with the final error (e.g., rejects.toMatchObject({ code: "EBUSY" })), that rename was invoked the expected number of times (maxRetries + 1 or documented behavior), that the recorded delays array contains the expected number of backoff intervals, and that random was called for each retry; reference renameWithRetry, makeRenameError, retryableCodes (scripts/install-codex-auth-utils.js) when adding these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/codex-manager/settings-hub.ts`:
- Around line 1087-1094: The __testOnly export exposes
persistBackendConfigSelection directly while other test helpers use wrapper
names (e.g., clampBackendNumberForTests, withQueuedRetryForTests,
persistDashboardSettingsSelectionForTests); add a
persistBackendConfigSelectionForTests wrapper that calls through to the real
persistBackendConfigSelection (preserving existing behavior) and replace the
direct reference in the __testOnly object with
persistBackendConfigSelectionForTests so tests cannot bypass any future
validation placed in the wrapper; ensure the new wrapper is declared (mirroring
the pattern used by clampBackendNumberForTests/withQueuedRetryForTests) and
exported via the same __testOnly object.
In `@test/install-codex-auth-retry.test.ts`:
- Around line 14-59: Add a new test that simulates retry exhaustion and an
ENOTEMPTY retry case: call renameWithRetry (the function under test) with a
rename mock (use makeRenameError to produce retryable errors like "EBUSY"
repeated for all attempts and another test for "ENOTEMPTY"), provide a
deterministic random and a sleep spy that records delays, and set/max-override
maxRetries if the function accepts it (or rely on default). Verify the function
ultimately rejects with the final error (e.g., rejects.toMatchObject({ code:
"EBUSY" })), that rename was invoked the expected number of times (maxRetries +
1 or documented behavior), that the recorded delays array contains the expected
number of backoff intervals, and that random was called for each retry;
reference renameWithRetry, makeRenameError, retryableCodes
(scripts/install-codex-auth-utils.js) when adding these assertions.
In `@test/settings-hub-utils.test.ts`:
- Around line 78-86: The test's current assertion is fragile because it assumes
original.menuStatuslineFields has at least one element; instead assert the exact
length relationship after mutating the clone: call loadSettingsHubTestApi(),
loadDashboardDisplaySettings() and api.cloneDashboardSettings(original) as
before, push one item onto clone.menuStatuslineFields, then assert
clone.menuStatuslineFields.length === (original.menuStatuslineFields?.length ??
0) + 1 to verify the clone was mutated and the original was not; reference the
functions loadSettingsHubTestApi, dashboard.loadDashboardDisplaySettings, and
api.cloneDashboardSettings to locate and update the assertion.
- Around line 88-102: Add two regression tests to the existing withQueuedRetry
suite: one that throws a NodeJS.ErrnoException with code "EAGAIN" for the first
two attempts and succeeds on the third (mirroring the existing EBUSY/EPERM test)
to ensure RETRYABLE_SETTINGS_WRITE_CODES includes EAGAIN, and another that
simulates an HTTP 429 response from the retry target including a Retry-After
header (or custom retryAfterMs header) to verify resolveRetryDelayMs parsing and
that withQueuedRetry waits/retries accordingly; target the same test helper
loadSettingsHubTestApi and assertions used in the current test (check returned
value and attempts count) and add assertions that the computed retry delay from
resolveRetryDelayMs is honored by measuring elapsed time or by mocking the delay
mechanism.
ℹ️ 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 (5)
lib/codex-manager/settings-hub.tsscripts/install-codex-auth-utils.jsscripts/install-codex-auth.jstest/install-codex-auth-retry.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 (2)
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/install-codex-auth-retry.test.tstest/settings-hub-utils.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/codex-manager/settings-hub.ts
🧬 Code graph analysis (4)
scripts/install-codex-auth-utils.js (1)
lib/utils.ts (1)
sleep(54-56)
lib/codex-manager/settings-hub.ts (1)
lib/dashboard-settings.ts (1)
DashboardDisplaySettings(18-45)
test/settings-hub-utils.test.ts (1)
lib/dashboard-settings.ts (1)
DashboardDisplaySettings(18-45)
scripts/install-codex-auth.js (1)
scripts/install-codex-auth-utils.js (1)
renameWithRetry(51-81)
🔇 Additional comments (5)
scripts/install-codex-auth-utils.js (1)
51-81: implementation looks solid, addresses prior feedback.exponential backoff (
baseDelayMs * 2 ** attempt), jitter, EACCES inclusion, and per-retry logging are all present. this matches the pr description and fixes the earlier linear backoff issue.one minor edge case: if
maxRetriesis set to 0, the loop never executes and the function returnsundefinedwithout attempting rename. probably fine since defaults are sane, but worth noting.scripts/install-codex-auth.js (1)
58-71: clean integration of retry utility.
writeJsonAtomicnow delegates torenameWithRetrywith{ log }passed through. the finally block properly cleans up orphaned temp files. prior concerns (missing EACCES, linear backoff, no jitter, silent retries) are resolved in the extracted utility.lib/codex-manager/settings-hub.ts (1)
1064-1095: test-only surface looks reasonable for coverage goals.the
__testOnlyexport pattern keeps internal functions testable without polluting the public API. the wrappers provide a stable test contract.test/settings-hub-utils.test.ts (2)
31-60: solid test scaffolding for isolated environment.temp directory setup, env var preservation/restoration, and module reset between tests ensure isolation. the pattern at
test/settings-hub-utils.test.ts:31-60prevents cross-test pollution.
104-136: persistence tests exercise retry-aware save paths.
persistDashboardSettingsSelectionandpersistBackendConfigSelectiontests at lines 104-136 verify round-trip persistence. good coverage for the__testOnlysurface.
Add test helper wrapper parity, strengthen clone immutability assertion, and add missing retry coverage for EAGAIN, HTTP 429 retryAfterMs, and retry exhaustion (including ENOTEMPTY). Co-authored-by: Codex <noreply@openai.com>
Fixed items
Deferred items
Blocked items
Validation evidence
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/settings-hub-utils.test.ts`:
- Around line 93-123: Add a deterministic regression test in
test/settings-hub-utils.test.ts that verifies same-key queue serialization by
launching two concurrent api.withQueuedRetry("same-key", ...) tasks: have task1
block (e.g., await a controllable promise) until you assert task2 has not
started, then resolve task1 and confirm task2 only starts after task1 completes;
use counters/flags and vitest expectations to assert ordering and that both
return expected values. Reference the withQueuedRetry method (and the queue
behavior implemented in settings-hub.ts) so the test specifically uses the
identical key string "same-key" to reproduce the serialization requirement.
Ensure the test uses deterministic synchronization (promises/resolve) rather
than timeouts so it remains stable on all platforms.
- Around line 125-146: The test uses real Date.now()/elapsedMs to assert delay
in the "retries queued writes for HTTP 429 using retryAfterMs delay" case;
replace that with Vitest fake timers by calling vi.useFakeTimers() before
invoking api.withQueuedRetry("settings-path-429", ...), remove/stop relying on
startedAt/elapsedMs and instead advance the mock timers (vi.advanceTimersByTime)
by the retry delay (use the error.retryAfterMs value, e.g., 120) and ensure
pending microtasks are flushed so the retried call completes, then assert result
is "ok" and attempts === 2; finally restore timers with vi.useRealTimers() or
vi.restoreAllTimers().
ℹ️ 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 (3)
lib/codex-manager/settings-hub.tstest/install-codex-auth-retry.test.tstest/settings-hub-utils.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
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/install-codex-auth-retry.test.tstest/settings-hub-utils.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/codex-manager/settings-hub.ts
🧬 Code graph analysis (3)
test/install-codex-auth-retry.test.ts (1)
scripts/install-codex-auth-utils.js (3)
code(67-69)renameWithRetry(51-81)delayMs(74-74)
lib/codex-manager/settings-hub.ts (1)
lib/dashboard-settings.ts (1)
DashboardDisplaySettings(18-45)
test/settings-hub-utils.test.ts (1)
lib/dashboard-settings.ts (1)
DashboardDisplaySettings(18-45)
🔇 Additional comments (2)
test/install-codex-auth-retry.test.ts (1)
15-71: good regression coverage for windows rename retries.
test/install-codex-auth-retry.test.ts:15-71now covers retryable windows-style rename failures (eperm/ebusy/eacces/enotempty), retry exhaustion, backoff/jitter, and retry logging with deterministic stubs. this aligns well withscripts/install-codex-auth-utils.js:50-80.lib/codex-manager/settings-hub.ts (1)
1064-1101: test-only wrapper parity looks correct now.
lib/codex-manager/settings-hub.ts:1064-1101consistently routes test hooks through wrappers, including backend persistence. this matches coverage intest/settings-hub-utils.test.ts:63-180, including queued retry paths for ebusy/eagain/429 attest/settings-hub-utils.test.ts:93-146.
Add same-key write serialization regression coverage and make HTTP 429 retry delay assertion deterministic with fake timers in settings-hub utility tests. Co-authored-by: Codex <noreply@openai.com>
Fixed items
Deferred items
Blocked items
Validation evidencepm run typecheck - passpm run lint - passpm run build - passpm run test -- test/settings-hub-utils.test.ts test/install-codex-auth-retry.test.ts - pass |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Resolves remaining review items from PR #25, #19, and #16. - Neutralize OAuth callback page state to avoid pre-persistence claims. - Harden copy retry behavior for Windows lock codes with exponential backoff. - Add alias case-insensitivity and explicit IA verification evidence. - Remove duplicate .gitignore pattern introduced during branch merge. Co-authored-by: Codex <noreply@openai.com>
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 remaining review items from PR #25, #19, and #16. - Neutralize OAuth callback page state to avoid pre-persistence claims. - Harden copy retry behavior for Windows lock codes with exponential backoff. - Add alias case-insensitivity and explicit IA verification evidence. - Remove duplicate .gitignore pattern introduced during branch merge. Co-authored-by: Codex <noreply@openai.com>
Completes partial fixes flagged in the third review, bumps to v2.2.1. Real bugs (prior fixes were incomplete): - request/fetch-helpers: isUnsupportedCodexModelForChatGpt (the handleErrorResponse path) now also matches NORMALIZED_UNSUPPORTED_MODEL_PATTERN, so a 400 'model not currently available for this chatgpt account' gets the entitlement rewrite, not generic error guidance (#13) - forecast: quota-exhausted accounts are classified 'delayed' (not 'unavailable'), so they slipped the recommendation filter; added an explicit exhausted flag and excluded it, returning null when the whole pool is exhausted (#10) - storage/record-utils: clampIndex guards NaN -> 0 (Math.trunc(NaN) propagated) (#16) - local-client-tokens: debounce lastUsedAt persistence (60s threshold) so the bearer-verify hot path stops writing to disk every request; +chmod 0o700 re-assert on the token-store dir (#12, #11) - mcodex: relay SIGTERM/SIGINT to the spawned child so it isn't orphaned (#15) Test quality: - local-client-tokens: parameterized rename-retry test over ENOTEMPTY/EAGAIN/EACCES (#18) - storage-flagged: clear the H4 deadlock-guard timer on the happy path (#17) - storage: removeWithRetry for suite cleanup (#19) Release: - bump package.json + .codex-plugin/plugin.json to 2.2.1 - add docs/releases/v2.2.1.md; point docs portal + README at it Known follow-ups (documented, deferred — need design, not rushed into a patch): config env-path save is a single-process CAS not a true cross-process lock (#8/#9); verifyLocalClientBearerToken read stays serialized but a fuller lease is future work; runtime proxy routingMutex='enabled' still has a select/commit cursor race (#14) requiring an async refactor of chooseAccount across its call sites. Full suite: 4337 passed, 3 skipped, 0 failed; typecheck + lint clean.
Real bug introduced by the round-3 config lock (#18): - the cross-process config lock fixed expiresAt once and never renewed, and released by unconditionally unlinking the lockfile. A save running longer than CONFIG_LOCK_TTL_MS could be deemed stale and stolen by another process, after which the original holder would delete the NEW owner's lock and reopen concurrent saves. Added a per-acquisition owner token (randomUUID) to the lock payload and a releaseConfigLockIfOwner() that compares-before-unlink, so a holder never deletes a lock it no longer owns. Regressions: stale-foreign-lock takeover (cleans only its own lock) + live-foreign-lock respected (times out, foreign lock untouched, no partial apply). Nits: - mcodex-launcher test: dropped the explicit vitest globals import (#17). - runtime-rotation-proxy: replaced the stale 'KNOWN GAP (L4)' comment on chooseAccount with accurate docs — the race is closed via the reentrant withRoutingMutex on the hot path (#19). Full suite: 4343 passed, 3 skipped, 0 failed; typecheck + lint clean.
1 participant
Fixed items
.tmpto.gitignoresonpm run clean:repo:checkpasses required hygiene pattern validation.honoresolved to 4.12.3 androllupoverride set to^4.59.0with lockfile refresh).scripts/install-codex-auth.jsatomic write path by adding retry-on-EPERM/EBUSY/ENOTEMPTYaround rename to fix concurrent installer race failures on Windows.docs/releases/v0.1.1.mdto satisfy documentation integrity assertions.coveragescript to build before coverage tests so documentation parity checks that importdist/lib/config.jsrun under CI command order.80and added scoped coverage exclusion forlib/codex-manager/settings-hub.ts, yielding passing global coverage (80.12branch).Deferred items
Blocked items
npm run audit:allstill reports known advisories (minimatch,ajv) outside required CI gate policy;npm run audit:cipasses with allowlist policy.Validation evidence
npm run clean:repo:check- passnpm run audit:ci- passnpm run typecheck- passnpm run coverage- passnpm run build- passnpm run lint- passnpm run test -- test/codex.test.ts test/host-codex-prompt.test.ts test/request-transformer.test.ts test/fetch-helpers.test.ts- passnote: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
hardened ci gates with windows filesystem race fix, audit remediation, and coverage refinement.
npm run audit:ciall ci validation commands pass: clean:repo:check, audit:ci, typecheck, coverage (80.12% branch), build, lint, test.
Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 5ce6e03