Hardening: close hidden production landmines in auth/storage/recovery paths#41
Hardening: close hidden production landmines in auth/storage/recovery paths#41
Conversation
Implement targeted mitigations for high-risk production landmines:\n- CAS conflict detection for account, unified settings, and config persistence\n- account save conflict merge-retry flow\n- refresh lease timeout fail-closed policy\n- network timeouts across OAuth and prompt fetch paths\n- idempotency key propagation and safer SSE fallback handling\n- recovery storage corruption guards\n- quota cache save result signaling\n\nValidated with full test, lint, and typecheck passes. 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 optimistic concurrency, revision hashing, and conflict-recovery across storage/config/accounts/unified-settings; adds fetchWithTimeout and applies timeouts to oauth and prompt fetches; adds optional idempotencyKey for codex headers; sse parse failures now return structured json errors; saveQuotaCache returns boolean. changes
sequence diagram(s)sequenceDiagram
participant caller as caller
participant storage as storage layer
participant disk as disk io
participant merger as merge/retry logic
caller->>storage: save(payload, expectedRevision?)
activate storage
storage->>disk: read file & compute sha256
disk-->>storage: currentRevision
storage->>storage: compare currentRevision vs expectedRevision
alt mismatch -> conflict
storage-->>caller: throw ECONFLICT
caller->>merger: load latest & mergeIntoLatestStorage(local, latest)
merger->>storage: write merged payload
storage->>disk: atomic write
disk-->>storage: newRevision
else match
storage->>disk: atomic write
disk-->>storage: newRevision
end
storage-->>caller: success
deactivate storage
sequenceDiagram
participant caller as caller
participant fwt as fetchWithTimeout
participant native as native fetch
participant timer as timeout watchdog
participant fallback as fallback logic
caller->>fwt: fetchWithTimeout(url, init, timeoutMs)
activate fwt
fwt->>timer: start timeout
fwt->>native: fetch(url, { ...init, signal })
par response wins
native-->>fwt: response
and timeout wins
timer-->>fwt: abort controller -> synthetic AbortError (code "ABORT_ERR")
fwt-->>native: abort
end
alt response arrived
fwt-->>caller: return response
else timeout
fwt-->>caller: throw AbortError (name "AbortError", code "ABORT_ERR")
caller->>fallback: run fallback logic (if any)
end
fwt->>timer: cleanup listeners
deactivate fwt
estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/response-handler-logging.test.ts (1)
17-27:⚠️ Potential issue | 🟡 Minormissing regression for error-path log redaction
test/response-handler-logging.test.ts:17-27only checks success-path no-log behavior. add a parse-error case with token/email-like payload and assert logged data stays sanitized (fixed message only), to guardlib/request/response-handler.ts:84.proposed test
describe("response handler logging branch", () => { it("does not log full stream content for successful SSE conversion", async () => { + logRequestMock.mockClear(); const { convertSseToJson } = await import("../lib/request/response-handler.js"); const response = new Response( 'data: {"type":"response.done","response":{"id":"resp_logging"}}\n', ); @@ expect(logRequestMock).not.toHaveBeenCalled(); }); + + it("logs only a fixed parse error message on missing final event", async () => { + logRequestMock.mockClear(); + const { convertSseToJson } = await import("../lib/request/response-handler.js"); + const response = new Response( + 'data: {"type":"chunk","delta":"email=user@example.com token=sk-secret"}\n', + ); + + await convertSseToJson(response, new Headers()); + expect(logRequestMock).toHaveBeenCalledWith("stream-error", { + error: "No response.done event found", + }); + }); });As per coding guidelines,
lib/**requires you to “check for logging that leaks tokens or emails,” andtest/**says to “demand regression cases.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/response-handler-logging.test.ts` around lines 17 - 27, Add a regression test exercising the error-path of convertSseToJson to ensure log redaction: create a Response whose SSE data contains a parse error and a token/email-like string (e.g. "token":"sk-..." or "user@example.com"), call convertSseToJson(...), and assert that logRequestMock was called once and the logged payload contains only the fixed sanitized message (no raw token or email patterns) to cover the error-path in response-handler's logging logic (the code around convertSseToJson / response-handler error branch).lib/storage.ts (1)
1176-1201:⚠️ Potential issue | 🟡 Minorpreserve pre-built storage errors instead of rewrapping them
lib/storage.ts:1176-1201rewraps errors even when they are alreadyStorageError(for example the explicitECONFLICTfromlib/storage.ts:1092). this can degrade the original hint/message quality.minimal fix
} catch (error) { + if (error instanceof StorageError) { + throw error; + } + try { await fs.unlink(tempPath); } catch { // Ignore cleanup failure. }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 1176 - 1201, The catch block in the save/accounts flow rewraps every error into a new StorageError, losing original StorageError details (message/hint/code); change the catch in the save routine so after cleanup (await fs.unlink(tempPath) with its inner try/catch) you detect if the caught error is already an instance of StorageError and if so rethrow it unchanged, otherwise compute code/hint via formatStorageErrorHint and log.error and throw a new StorageError as currently done; reference the StorageError class, formatStorageErrorHint function, tempPath cleanup and the existing log.error usage when making this change.lib/prompts/codex.ts (1)
147-170:⚠️ Potential issue | 🟠 Majoradd vitest regressions for timeout fallback paths and getLatestReleaseTag.
getLatestReleaseTagatlib/prompts/codex.ts:138has no test coverage. timeout-specific regression tests are missing for the api-timeout fallback (lib/prompts/codex.ts:147-170) and the fetch-timeout fallback (lib/prompts/codex.ts:326-330). existing tests intest/codex-prompts.test.tsmock failed api responses (403) but don't exercise the timeout code path triggered byfetchWithTimeout. also missing: windows cache-dir behavior regression tests for disk fallback paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/prompts/codex.ts` around lines 147 - 170, Add vitest regression tests to cover getLatestReleaseTag's timeout fallback paths: simulate fetchWithTimeout timing out for the GITHUB_API_RELEASES request to trigger the HTML fallback branch (the code around getLatestReleaseTag and the API-to-HTML fallback at the fetchWithTimeout call that leads to GITHUB_HTML_RELEASES), and also simulate a timeout on the later fetchWithTimeout call that hits the fetch-timeout fallback (the second fetchWithTimeout usage); update test/codex-prompts.test.ts (or add a new test file) to mock fetchWithTimeout for both success, HTTP error (403) and timeout cases so the API fallback and HTML/disk fallback branches are exercised. Additionally add vitest cases that simulate Windows cache-dir behavior to exercise the disk fallback paths referenced in getLatestReleaseTag/cache logic, ensuring cache read/write failures and Windows-specific path handling are validated. Ensure tests assert that getLatestReleaseTag returns the HTML-derived tag or cached value when timeouts occur and that cache behavior on Windows is covered.lib/auth/auth.ts (1)
121-132:⚠️ Potential issue | 🟠 Majorauthorization-code exchange can throw on timeout/network errors.
lib/auth/auth.ts:121usesfetchWithTimeout, but this block has no catch for fetch/abort failures. that lets exceptions bubble instead of returning a typedTokenResultfailure, which can break login flow control (lib/codex-manager.ts:1265call path).proposed fix
export async function exchangeAuthorizationCode( code: string, verifier: string, redirectUri: string = REDIRECT_URI, ): Promise<TokenResult> { - const res = await fetchWithTimeout(TOKEN_URL, { - method: "POST", - headers: { "Content-Type": "application/x-www-form-urlencoded" }, - body: new URLSearchParams({ - grant_type: "authorization_code", - client_id: CLIENT_ID, - code, - code_verifier: verifier, - redirect_uri: redirectUri, - }), - }, OAUTH_TOKEN_EXCHANGE_TIMEOUT_MS); + let res: Response; + try { + res = await fetchWithTimeout(TOKEN_URL, { + method: "POST", + headers: { "Content-Type": "application/x-www-form-urlencoded" }, + body: new URLSearchParams({ + grant_type: "authorization_code", + client_id: CLIENT_ID, + code, + code_verifier: verifier, + redirect_uri: redirectUri, + }), + }, OAUTH_TOKEN_EXCHANGE_TIMEOUT_MS); + } catch (error) { + const err = error instanceof Error ? error : new Error(String(error)); + return { type: "failed", reason: "network_error", message: err.message }; + }also add a vitest regression that forces timeout on exchange and asserts a failed
TokenResultinstead of throw.As per coding guidelines
lib/**: 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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/auth/auth.ts` around lines 121 - 132, The authorization-code exchange in lib/auth/auth.ts currently calls fetchWithTimeout (against TOKEN_URL) without catching network/timeout exceptions, so wrap the fetchWithTimeout call in a try/catch inside the function that performs the token exchange (the method surrounding the shown snippet) and on any thrown error return a typed TokenResult failure object instead of letting the exception propagate; ensure the catch maps abort/timeout and other network errors to the same TokenResult shape consumed by callers (e.g., lib/codex-manager.ts), avoid logging sensitive fields (tokens/emails) in the error path, and add a vitest regression that simulates fetch timeout/abort for the token exchange and asserts the function returns a failed TokenResult rather than throwing.
🤖 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/accounts.ts`:
- Around line 803-807: The current merge loop over local.accounts uses {
...mergedAccounts[idx], ...account } which lets undefined fields in account
clobber valid existing values; update the merge logic inside the loop (the block
using claimIndex(account) and mergedAccounts[idx]) to only copy properties from
account when they are !== undefined instead of blanket spreading, and for nested
object fields (e.g., labels, cooldown metadata) perform a shallow merge like
merged = { ...merged, ...(accountFields !== undefined ? accountFields : {}) } so
that existing concrete values aren’t overwritten by undefined optional fields.
- Around line 849-874: persistStorageWithConflictRecovery currently writes a
mergedCandidate to disk on conflict but never updates the in-memory
this.accounts, causing subsequent saveToDisk calls to overwrite concurrent
changes; modify persistStorageWithConflictRecovery so after computing
mergedCandidate (from mergeIntoLatestStorage(latest, baseStorage)) and
successfully saving via saveAccounts(mergedCandidate) you update the manager's
in-memory state (this.accounts or the canonical storage holder) to that merged
result before returning, preserve existing behavior when no conflict occurs, and
avoid logging any sensitive fields; add a Vitest multi-writer regression that
simulates two managers where one triggers a conflict, verify merged result is
saved and then the second manager’s later saveToDisk does not drop the
concurrently added accounts, and ensure the new test handles Windows/EBUSY and
429-like retry behavior as per lib/** guidelines.
In `@lib/codex-manager.ts`:
- Around line 2114-2115: The calls to await persistQuotaCache(quotaCache)
(references: function persistQuotaCache and the two call sites in codex-manager
where the await occurs) ignore its boolean result so persistence failures are
hidden in --json flows; update both call sites to check the returned boolean and
on false either set the JSON envelope to an error state (or exit non‑zero) and
log a safe, non‑sensitive error message; add a vitest regression that stubs
persistQuotaCache to return false and asserts the process emits the expected
JSON error envelope (or exits non‑zero), and ensure the test exercise EBUSY/429
retry handling in the persistence queue and that any added logging does not leak
tokens or emails.
In `@lib/config.ts`:
- Around line 335-339: readConfigSnapshotFromPath currently calls fs.readFile
without retries and needs a retry loop for transient Windows EBUSY/EPERM errors:
update readConfigSnapshotFromPath (and any direct fs.readFile callers like
writeJsonFileAtomicWithRetry and savePluginConfig) to retry on
isRetryableFsError() with exponential backoff matching the existing fs.rename
retry pattern; additionally implement a cross-process file lock (lockfile)
around the save path inside withConfigSaveLock/save flow so that concurrent
processes cannot both pass the revision check then overwrite each other (use a
lockfile/mutex before reading+checking+writing to close the CAS race), and add a
cross-process concurrency test similar to unified-settings.test.ts:185 to verify
the new lock file behavior.
- Around line 451-475: The conflict-retry loop in withConfigSaveLock that reads
snapshot via readConfigSnapshotFromPath and writes via
writeJsonFileAtomicWithRetry using CONFIG_CONFLICT_RETRY_LIMIT and
isConfigConflictError lacks targeted tests: add unit tests in
test/config-save.test.ts that simulate a transient conflict
(isConfigConflictError returning true / writeJsonFileAtomicWithRetry throwing an
ECONFLICT error twice then succeeding) to assert the loop retries and eventually
returns, and a test that forces ECONFLICT on all attempts to assert retry
exhaustion (throws after CONFIG_CONFLICT_RETRY_LIMIT attempts); also add a
combined scenario where writeJsonFileAtomicWithRetry alternates filesystem
transient errors (EBUSY/EPERM) and conflict errors to ensure the backoff/retry
behavior interacts correctly with the conflict loop, and keep existing
clamp-to-100 assertion in test/plugin-config.test.ts as-is.
- Around line 307-315: readConfigSnapshotFromPath currently does a single
fs.readFile and can fail on transient Windows errors; update the function to
wrap the fs.readFile call with the same retry/backoff logic used elsewhere (use
the existing RETRYABLE_FS_CODES constant or expand that constant to include any
additional codes you intend to handle, and reference it from
readConfigSnapshotFromPath) so it retries on ebusy/eperm (and any newly added
codes) before bubbling the error; ensure you reuse the same retry/backoff helper
or pattern used by writeJsonFileAtomicWithRetry to keep behavior consistent, and
add vitest unit tests exercising transient read failures for
readConfigSnapshotFromPath to cover Windows contention cases and prevent
regressions.
In `@lib/refresh-lease.ts`:
- Around line 255-264: Add a new unit test that exercises the
wait-timeout-follow branch in lib/refresh-lease.ts by simulating a stale lock
that cannot be unlinked (unlink returns retryable/EBUSY until timeout), ensuring
readFreshResult(resultPath, tokenHash) returns a fresh result and that
refreshLease (or the function calling readFreshResult) returns role: "follower"
and does not attempt to acquire the lock; also assert the release function is a
no-op. While adding this test, verify RETRYABLE_IO_ERRORS in
lib/refresh-lease.ts includes EPERM for Windows semantics and that 429
(rate-limit) errors are treated as queued/retryable per the coding guidelines,
and confirm the processLogger/error logs emitted around the follower branches
(the statements near readFreshResult and subsequent returns) do not include
token or raw tokenHash values (only safe identifiers).
In `@lib/request/fetch-helpers.ts`:
- Around line 572-577: Add Vitest unit tests exercising the idempotency key
branch in lib/request/fetch-helpers.ts: call the exposed function that uses
resolvedOpts and headers (exercise both the positional and named-parameter call
forms used by the module) and assert header behavior for four cases: a trimmed
non-empty key sets headers.get("Idempotency-Key") to the trimmed value;
undefined or omitted idempotencyKey results in headers not containing
"Idempotency-Key"; a whitespace-only string (e.g. " ") is treated as blank and
removes the header; verify both the positional-argument API and the
named-parameter API produce identical results. Use test/fetch-helpers.test.ts to
add these tests, creating/inspecting a Headers-like object passed into the
function and asserting presence/absence and exact value of "Idempotency-Key".
In `@lib/request/response-handler.ts`:
- Around line 86-100: The SSE parse-failure branch currently returns the
upstream response.status (which can be 200) with an error envelope; change the
Response construction in response-handler (the block that builds the JSON error
with type "stream_parse_error") to use a non-2xx status (e.g. 502 or 500)
instead of response.status so callers can fail-closed; update the headers
creation (jsonHeaders.set(...)) and return call to use that fixed status, and
add a vitest regression in test/response-handler.test.ts that simulates the
parse failure and asserts the returned Response.status is non-2xx (and that the
body contains the "stream_parse_error" envelope).
In `@lib/storage.ts`:
- Around line 984-985: The recovery branches currently call
forgetKnownStorageRevision(path) which clears the stored revision and causes
expectedRevision to become undefined (skipping stale-write checks); instead, in
the recovery paths that return backup.normalized (the branches calling
forgetKnownStorageRevision), preserve or re-establish the known revision by
setting it to the recovered backup's revision (e.g., call
setKnownStorageRevision(path, backup.revision) or otherwise assign
expectedRevision = backup.revision) rather than forgetting it; update the calls
to forgetKnownStorageRevision in the functions/branches referenced (the lines
invoking forgetKnownStorageRevision(path) around where backup.normalized is
returned) so future persist operations still perform conflict detection.
In `@test/response-handler.test.ts`:
- Around line 64-81: Add a regression test covering CRLF line endings by
duplicating or adding a new case alongside the existing 'should return JSON
stream parse error if no final response found' test that uses sse content joined
with '\r\n' instead of '\n'; specifically create an sseContent string like
`data: {"type":"response.started"}\r\ndata: {"type":"chunk","delta":"text"}\r\n`
and call convertSseToJson(response, headers) (same Response/Headers setup) then
assert the returned JSON body and content-type header match the existing
expectations; this ensures convertSseToJson (the SSE parsing path) handles
Windows CRLF line endings.
- Around line 101-109: The empty-stream test is missing the content-type header
assertion: after awaiting the response and before or alongside the JSON body
check in the "empty-stream" test (where `const body = await result.json()` and
`expect(body).toEqual(...)` are asserted), add an assertion that the response
Content-Type header equals "application/json; charset=utf-8" (e.g.
`expect(result.headers.get('content-type')).toBe('application/json;
charset=utf-8')`) so the test mirrors the header check used earlier in the file
and enforces contract parity.
In `@test/unified-settings.test.ts`:
- Around line 228-267: The test doesn't prove a retry happened; update the test
around
saveUnifiedPluginConfig/loadUnifiedPluginConfigSync/getUnifiedSettingsPath to
assert the retry path executed by verifying the spy/mock was exercised more than
once (e.g., assert settingsReadCount >= 2 or
expect(readSpy).toHaveBeenCalledTimes(2)) or by asserting the intermediate
concurrentUpdate content was observed before final save; add that assertion
after the save and before restoring the spy so the test fails if no retry
occurred.
---
Outside diff comments:
In `@lib/auth/auth.ts`:
- Around line 121-132: The authorization-code exchange in lib/auth/auth.ts
currently calls fetchWithTimeout (against TOKEN_URL) without catching
network/timeout exceptions, so wrap the fetchWithTimeout call in a try/catch
inside the function that performs the token exchange (the method surrounding the
shown snippet) and on any thrown error return a typed TokenResult failure object
instead of letting the exception propagate; ensure the catch maps abort/timeout
and other network errors to the same TokenResult shape consumed by callers
(e.g., lib/codex-manager.ts), avoid logging sensitive fields (tokens/emails) in
the error path, and add a vitest regression that simulates fetch timeout/abort
for the token exchange and asserts the function returns a failed TokenResult
rather than throwing.
In `@lib/prompts/codex.ts`:
- Around line 147-170: Add vitest regression tests to cover
getLatestReleaseTag's timeout fallback paths: simulate fetchWithTimeout timing
out for the GITHUB_API_RELEASES request to trigger the HTML fallback branch (the
code around getLatestReleaseTag and the API-to-HTML fallback at the
fetchWithTimeout call that leads to GITHUB_HTML_RELEASES), and also simulate a
timeout on the later fetchWithTimeout call that hits the fetch-timeout fallback
(the second fetchWithTimeout usage); update test/codex-prompts.test.ts (or add a
new test file) to mock fetchWithTimeout for both success, HTTP error (403) and
timeout cases so the API fallback and HTML/disk fallback branches are exercised.
Additionally add vitest cases that simulate Windows cache-dir behavior to
exercise the disk fallback paths referenced in getLatestReleaseTag/cache logic,
ensuring cache read/write failures and Windows-specific path handling are
validated. Ensure tests assert that getLatestReleaseTag returns the HTML-derived
tag or cached value when timeouts occur and that cache behavior on Windows is
covered.
In `@lib/storage.ts`:
- Around line 1176-1201: The catch block in the save/accounts flow rewraps every
error into a new StorageError, losing original StorageError details
(message/hint/code); change the catch in the save routine so after cleanup
(await fs.unlink(tempPath) with its inner try/catch) you detect if the caught
error is already an instance of StorageError and if so rethrow it unchanged,
otherwise compute code/hint via formatStorageErrorHint and log.error and throw a
new StorageError as currently done; reference the StorageError class,
formatStorageErrorHint function, tempPath cleanup and the existing log.error
usage when making this change.
In `@test/response-handler-logging.test.ts`:
- Around line 17-27: Add a regression test exercising the error-path of
convertSseToJson to ensure log redaction: create a Response whose SSE data
contains a parse error and a token/email-like string (e.g. "token":"sk-..." or
"user@example.com"), call convertSseToJson(...), and assert that logRequestMock
was called once and the logged payload contains only the fixed sanitized message
(no raw token or email patterns) to cover the error-path in response-handler's
logging logic (the code around convertSseToJson / response-handler error
branch).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c62bfcfe-3fbb-488f-be8f-56d73b1da13d
📒 Files selected for processing (27)
index.tslib/accounts.tslib/auth/auth.tslib/codex-manager.tslib/config.tslib/prompts/codex.tslib/prompts/host-codex-prompt.tslib/quota-cache.tslib/recovery/storage.tslib/refresh-lease.tslib/refresh-queue.tslib/request/fetch-helpers.tslib/request/response-handler.tslib/storage.tslib/unified-settings.tslib/utils.tstest/accounts-edge.test.tstest/auth.test.tstest/plugin-config.test.tstest/quota-cache.test.tstest/recovery-storage.test.tstest/refresh-lease.test.tstest/refresh-queue.test.tstest/response-handler-logging.test.tstest/response-handler.test.tstest/storage.test.tstest/unified-settings.test.ts
📜 Review details
🧰 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/prompts/codex.tslib/quota-cache.tslib/request/fetch-helpers.tslib/refresh-queue.tslib/refresh-lease.tslib/codex-manager.tslib/prompts/host-codex-prompt.tslib/request/response-handler.tslib/auth/auth.tslib/accounts.tslib/recovery/storage.tslib/unified-settings.tslib/storage.tslib/utils.tslib/config.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/recovery-storage.test.tstest/plugin-config.test.tstest/storage.test.tstest/auth.test.tstest/response-handler.test.tstest/quota-cache.test.tstest/response-handler-logging.test.tstest/refresh-queue.test.tstest/accounts-edge.test.tstest/refresh-lease.test.tstest/unified-settings.test.ts
🧬 Code graph analysis (16)
lib/prompts/codex.ts (1)
lib/utils.ts (1)
fetchWithTimeout(76-112)
lib/quota-cache.ts (1)
lib/logger.ts (2)
error(389-393)logWarn(341-346)
test/recovery-storage.test.ts (1)
lib/recovery/constants.ts (2)
MESSAGE_STORAGE(31-31)PART_STORAGE(32-32)
test/plugin-config.test.ts (1)
lib/config.ts (1)
getRetryAllAccountsMaxRetries(661-668)
test/storage.test.ts (1)
lib/storage.ts (3)
AccountStorageV3(26-26)saveAccounts(1223-1227)loadAccounts(876-878)
test/auth.test.ts (1)
lib/auth/auth.ts (1)
refreshAccessToken(194-246)
lib/codex-manager.ts (1)
lib/quota-cache.ts (2)
QuotaCacheData(22-25)saveQuotaCache(219-263)
test/response-handler.test.ts (1)
lib/request/response-handler.ts (1)
convertSseToJson(51-125)
test/quota-cache.test.ts (1)
lib/quota-cache.ts (1)
saveQuotaCache(219-263)
lib/prompts/host-codex-prompt.ts (1)
lib/utils.ts (1)
fetchWithTimeout(76-112)
lib/auth/auth.ts (1)
lib/utils.ts (2)
fetchWithTimeout(76-112)isAbortError(20-24)
test/unified-settings.test.ts (1)
lib/unified-settings.ts (3)
getUnifiedSettingsPath(293-295)saveUnifiedPluginConfig(357-376)loadUnifiedPluginConfigSync(306-314)
lib/accounts.ts (3)
lib/storage.ts (3)
AccountStorageV3(26-26)saveAccounts(1223-1227)loadAccounts(876-878)lib/prompts/codex.ts (2)
MODEL_FAMILIES(60-66)ModelFamily(49-54)lib/utils.ts (1)
sleep(65-67)
lib/recovery/storage.ts (4)
lib/logger.ts (2)
createLogger(366-424)error(389-393)lib/utils.ts (1)
isRecord(11-13)lib/recovery/types.ts (2)
StoredMessageMeta(15-25)StoredPart(67-78)lib/recovery/constants.ts (2)
MESSAGE_STORAGE(31-31)THINKING_TYPES(34-34)
lib/storage.ts (1)
lib/storage/migrations.ts (1)
AccountStorageV3(59-64)
lib/config.ts (1)
lib/utils.ts (1)
isRecord(11-13)
🔇 Additional comments (23)
lib/request/response-handler.ts (1)
1-1: good hardening on logging surface
lib/request/response-handler.ts:1removes the old full-stream logging gate path, which lowers token/email leak risk in request logs.index.ts (1)
1598-1606: idempotency-key propagation is consistent across primary and failover sendsgood wiring at
lib/index.ts:1605andlib/index.ts:2168; both request paths now carry the same idempotency signal for retry/failover safety.Also applies to: 2161-2169
test/refresh-lease.test.ts (1)
379-380: good regression assertion for stale-lock ownership behaviorthe updated check at
test/refresh-lease.test.ts:379-380correctly asserts owner acquisition and explicit release, which strengthens this staleness path.lib/refresh-queue.ts (1)
172-182: bypass wait-timeout early return is safe by design; no cleanup neededthe bypass lease role is designed as a no-op handle.
createBypassHandle()at lib/refresh-lease.ts:295-302 returns a release function with an explicit "// No-op" comment. the early return at lib/refresh-queue.ts:172 skips the finally block, but this is correct: since bypass.release() does nothing, skipping it avoids a redundant call. the test at test/refresh-queue.test.ts:900-923 validates this path works correctly, confirming the early return behavior is intentional and safe. no resource cleanup needed.logging at lib/refresh-queue.ts:173-176 uses only tokenSuffix (last 6 chars) and literal strings; no tokens or emails leak.
> Likely an incorrect or invalid review comment.lib/unified-settings.ts (1)
73-87: read-side operations correctly fail fast to prevent stale overwritesthe code at
lib/unified-settings.ts:73-113intentionally does not retry transient lock errors on reads. this is deliberate: whenreadSettingsSnapshotAsync(line 361, 417) fails with EBUSY/EPERM, the operation fails immediately, preventing stale reads from overwriting fresh data.the test at
test/unified-settings.test.ts:269("refuses overwriting settings sections when a read fails") validates this behavior—it mocks readFile to throw EBUSY and expects the save operation to reject without proceeding.write-side operations do handle transient windows lock errors: the rename loops at
lib/unified-settings.ts:192-203andlib/unified-settings.ts:251-265retryisRetryableFsError(EBUSY/EPERM) with exponential backoff. tests attest/unified-settings.test.ts:145andtest/unified-settings.test.ts:163confirm this works.the outer retry loops at
lib/unified-settings.ts:328-345,lib/unified-settings.ts:357-376, andlib/unified-settings.ts:411-432only retryECONFLICTbecause they handle concurrent modification (reader-writer races), not transient lock contention. the latter is handled at the rename layer.no changes needed.
> Likely an incorrect or invalid review comment.test/recovery-storage.test.ts (2)
142-161: good invalid-shape coverage for message reads.deterministic setup and clear assertions lock behavior at
test/recovery-storage.test.ts:142-161.
206-225: good invalid-shape coverage for part reads.this regression test is focused and stable at
test/recovery-storage.test.ts:206-225.test/storage.test.ts (2)
21-22: type import update is clean.using
AccountStorageV3as a type-only import keeps runtime unchanged attest/storage.test.ts:21-22.
731-767: strong regression for stale-write conflict handling.this test directly validates optimistic concurrency rejection and preserved external state at
test/storage.test.ts:731-767.test/accounts-edge.test.ts (1)
419-454: good conflict-recovery regression coverage.the new case at
test/accounts-edge.test.ts:419-454validates retry-on-ECONFLICTplus merge of concurrent disk state.test/auth.test.ts (1)
509-542: solid timeout regression for refresh stalls.this test reliably validates abort-on-timeout behavior at
test/auth.test.ts:509-542.lib/prompts/codex.ts (1)
7-23: timeout wiring is clean.the
fetchWithTimeoutimport and constant introduction atlib/prompts/codex.ts:7-23are straightforward and readable.lib/quota-cache.ts (1)
219-261: boolean persistence result is implemented correctly.the success/failure return contract in
lib/quota-cache.ts:219-261is consistent with the existing retry and temp-file cleanup flow.test/refresh-queue.test.ts (1)
900-923: good fail-closed lease timeout regression.the test at
test/refresh-queue.test.ts:900-923correctly asserts wait-timeout bypass returns failure and blocks downstream refresh calls.test/quota-cache.test.ts (1)
36-50: good deterministic coverage for quota-cache save outcomes.
test/quota-cache.test.ts:36,test/quota-cache.test.ts:181, andtest/quota-cache.test.ts:338now assert the boolean persistence contract directly. this is solid, and it complements the existing windows-style transient fs behavior coverage (ebusy/eperm) in this suite.Also applies to: 181-195, 338-340
test/plugin-config.test.ts (1)
108-109: defaults and hard-cap behavior are well pinned in tests.
test/plugin-config.test.ts:108,test/plugin-config.test.ts:166,test/plugin-config.test.ts:421,test/plugin-config.test.ts:485,test/plugin-config.test.ts:543, andtest/plugin-config.test.ts:942correctly lock in the new default (12) and hard upper bound (100).Also applies to: 166-167, 421-422, 485-486, 543-544, 942-947
lib/codex-manager.ts (1)
605-614: nice consolidation of quota-cache persistence.
lib/codex-manager.ts:605centralizes quota-cache persistence and tty warning behavior cleanly, and the updated call sites (lib/codex-manager.ts:649,lib/codex-manager.ts:1632,lib/codex-manager.ts:2203,lib/codex-manager.ts:3067) are consistent.Also applies to: 649-649, 1632-1632, 2203-2203, 3067-3067
lib/utils.ts (1)
76-112: fetch timeout helper is implemented cleanly.
lib/utils.ts:76-lib/utils.ts:112correctly handles abort chaining, timeout enforcement, and listener/timer cleanup.lib/auth/auth.ts (1)
199-245: refresh timeout/abort mapping is a good hardening step.
lib/auth/auth.ts:199-lib/auth/auth.ts:245consistently maps abort/timeout failures into typed refresh failures, which keeps retry/queue callers deterministic.lib/recovery/storage.ts (1)
24-61: good fail-closed recovery parsing and bounded corruption logging.
lib/recovery/storage.ts:24-lib/recovery/storage.ts:61plus the guarded parse paths atlib/recovery/storage.ts:118,lib/recovery/storage.ts:160,lib/recovery/storage.ts:345, andlib/recovery/storage.ts:443now skip corrupted artifacts instead of crashing. this is the right hardening move.Also applies to: 118-126, 160-168, 345-357, 443-460
lib/refresh-lease.ts (2)
47-47: good reason plumbing for bypass diagnostics.line 47 (
lib/refresh-lease.ts:47) and lines 295-303 (lib/refresh-lease.ts:295-303) cleanly threadreasonthrough bypass handles with no new token/email exposure.Also applies to: 295-303
255-264: good fail-closed timeout behavior with final-result fallback.lines 255-284 (
lib/refresh-lease.ts:255-284) re-check cached final results before bypassing on timeout. this is a solid concurrency hardening step.Also applies to: 275-284
lib/config.ts (1)
38-40: bounded retry defaults and hard cap look good.lines 38-40, 131, and 665-667 (
lib/config.ts) replace unbounded retry behavior with a clear default and hard upper bound.Also applies to: 131-131, 665-667
Preserve concrete disk fields when resolving ECONFLICT account saves and refresh in-memory state after merged persistence. Also tolerate malformed env-path JSON snapshots during config CAS saves. Co-authored-by: Codex <noreply@openai.com>
Add env-path save regressions for read contention, optimistic conflict retries, mixed conflict+rename contention, and lockfile wait behavior to protect the new CAS persistence flow. Co-authored-by: Codex <noreply@openai.com>
Return non-zero from forecast/fix json paths when quota cache persistence fails, and include explicit persistence status in the json payload. Add CLI regressions and set default quota-cache mock success to keep baseline test behavior stable. Co-authored-by: Codex <noreply@openai.com>
Use timed fetch for token refresh with consistent timeout classification, keep SSE parse failures on structured error payloads, and add regression coverage for idempotency headers plus response logging/error branches. Co-authored-by: Codex <noreply@openai.com>
Track known revisions for recovered storage payloads, preserve StorageError semantics, and add lease wait-timeout follower-result coverage plus stronger storage conflict assertions. Co-authored-by: Codex <noreply@openai.com>
Add regressions for GitHub API timeout fallback paths in codex prompt loaders and tighten unified settings conflict-read assertion coverage. Co-authored-by: Codex <noreply@openai.com>
- tighten merge assignment typing in accounts storage reconciliation - align chaos SSE parse-failure expectations with 502 contract Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/auth/auth.ts (1)
248-253: 🧹 Nitpick | 🔵 Trivialredundant optional chaining on guaranteed Error.
erris already narrowed toErrorat line 248, soerr?.messageon lines 250 and 253 can just beerr.message. harmless but slightly noisy.also note the asymmetry:
refreshAccessTokenlogs network errors at line 252 whileexchangeAuthorizationCodedoes not. if intentional (e.g., refresh failures are more diagnostic), consider adding a brief comment.cleanup optional chaining
} catch (error) { const err = error instanceof Error ? error : new Error(String(error)); if (isAbortError(err) || /timeout/i.test(err.message)) { - return { type: "failed", reason: "unknown", message: err?.message ?? "Request aborted" }; + return { type: "failed", reason: "unknown", message: err.message }; } logError("Token refresh error", err); - return { type: "failed", reason: "network_error", message: err?.message }; + return { type: "failed", reason: "network_error", message: err.message }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/auth/auth.ts` around lines 248 - 253, The code narrows error to Error in the refreshAccessToken flow (variable err) so remove redundant optional chaining: replace err?.message with err.message in both the timeout/abort branch and the network_error return in refreshAccessToken. While here, check asymmetry with exchangeAuthorizationCode: either add a brief comment on why only refreshAccessToken calls logError("Token refresh error", err) or add equivalent logging in exchangeAuthorizationCode to keep behavior consistent.lib/storage.ts (1)
1090-1172:⚠️ Potential issue | 🔴 Criticalstale-write rejection is still racy across processes.
in
lib/storage.ts:1090-1106(line 1096), revision is checked before commit, then actual commit happens later atlib/storage.ts:1163(line 1163). without a cross-process file lock around this full critical section, two writers can pass the same check and last rename wins.please wrap check+write+rename under an inter-process lock (similar to config lock strategy), and add a vitest overlap regression (not just sequential stale write) since
test/storage.test.ts:731-770is sequential.as per coding guidelines,
lib/**: 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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 1090 - 1172, The revision check/readStorageRevision + write + fs.rename critical section is racy across processes; wrap the whole sequence (expectedRevision check, synthetic-storage probe, createRotatingAccountsBackup, writing walPath/tempPath, fs.rename(tempPath, path), unlink(walPath), lastAccountsSaveTimestamp and rememberKnownStorageRevision) in an inter-process lock (reuse the config lock strategy) so only one process can perform the check+write+rename at a time; update the code paths referencing knownStorageRevisionByPath, readStorageRevision, looksLikeSyntheticFixtureStorage, createRotatingAccountsBackup, walPath/tempPath, computeSha256 and rememberKnownStorageRevision to execute under that lock, and add a vitest overlap/regression that spawns concurrent writers (not just sequential) to assert stale-write rejection and proper retry/backoff for EBUSY/EPERM, while ensuring logs do not contain tokens/emails and tests reference the modified behavior.
🤖 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/config.ts`:
- Around line 409-473: The lockfile currently only contains a PID and unlink
operations in acquireConfigSaveFileLock (stale eviction) and
releaseConfigSaveFileLock remove the file by path without verifying ownership,
allowing a stale process to delete a new holder’s lock; fix by writing a unique,
unguessable token (e.g., UUID or crypto random) into the lock file alongside the
PID when creating the lock in acquireConfigSaveFileLock, and whenever you
consider unlinking (both the stale-eviction branch inside
acquireConfigSaveFileLock and in releaseConfigSaveFileLock) first read and
validate the token in the existing lock file matches the token your process
wrote before deleting; if tokens differ, do not unlink and treat it as another
active owner (retry/wait or throw after timeout); add a vitest regression that
simulates process A creating a lock, process B taking over after eviction, then
A attempting to release to assert A cannot remove B’s lock, and ensure tests
avoid logging the token or other sensitive data.
In `@lib/storage.ts`:
- Around line 395-406: The readStorageRevision function (and the other
single-attempt reads at the ranges mentioned: 898-910 and 1068-1074) must be
hardened against transient Windows FS errors by implementing a bounded retry
with exponential backoff for readFile failures that return EBUSY, EPERM (and
optionally EAGAIN), returning null only for ENOENT; update readStorageRevision
to catch those transient codes, retry a small fixed number of times with
increasing delay and then throw if still failing, and apply the same retry
helper to the reads at 898-910 and 1068-1074 (extract a shared
readWithTransientRetry helper). Add a vitest regression that simulates transient
EBUSY/EPERM on fs.readFile for the first N attempts then succeeds to assert the
function returns the correct SHA (and that ENOENT still returns null), and
ensure tests reference the modified functions and note that logging added for
retries must not include any tokens/emails.
In `@test/auth.test.ts`:
- Around line 530-563: Add a new test titled like "concurrent refreshes with one
timeout do not corrupt results" that uses vi.useFakeTimers and a
globalThis.fetch mock which can distinguish two concurrent requests (e.g., by
inspecting init or input) so the first call resolves quickly with a successful
token response and the second call stalls until aborted; call refreshAccessToken
twice concurrently (e.g., refreshAccessToken('fast-token', {...}) and
refreshAccessToken('slow-token', { timeoutMs: 1_000 })), advance timers to
trigger the timeout for the stalled request (vi.advanceTimersByTimeAsync), then
assert the first result is a success and the second result is a failed timeout
(check result.type, result.message contains "timeout" and result.reason), and
finally restore globalThis.fetch and vi.useRealTimers to avoid leaking state;
reference refreshAccessToken, globalThis.fetch, vi.useFakeTimers,
vi.advanceTimersByTimeAsync in the test.
In `@test/chaos/fault-injection.test.ts`:
- Around line 313-317: The tests in fault-injection.test.ts (cases around the
convertSseToJson usages) only assert response.status and must also assert the
error envelope contract; update the failing-cases (examples at the empty stream
and the other parse-failure cases) to check that the returned Headers include
"content-type" containing "application/json" and that the parsed JSON body has
error.type === "stream_parse_error" (use convertSseToJson result parsing to
obtain the JSON body and assert error.type). Reference convertSseToJson and the
response-handler contract (error.type "stream_parse_error") when adding these
expectations so tests lock the non-leaky envelope shape enforced by
response-handler.ts.
In `@test/codex-prompts.test.ts`:
- Around line 374-391: Add an assertion to this test to verify fetch call count
so unexpected retries are caught: after invoking getCodexInstructions("gpt-5.2")
assert mockFetch.toHaveBeenCalledTimes(2) (one for the tag API and one for the
prompt fetch that times out), and add a short inline comment explaining that
mockedReadFile returns malformed JSON to force cachedMetadata = null so the test
exercises fetchAndPersistInstructions' timeout path; locate the test block
containing getCodexInstructions and the mockFetch/mockedReadFile setup to insert
these changes.
In `@test/config-save.test.ts`:
- Around line 272-296: Add a deterministic Vitest case in
test/config-save.test.ts that verifies the fail-closed timeout path of
savePluginConfig: create the config file and a persistent `${configPath}.lock`
that is not removed, then call savePluginConfig({ codexMode: false }) and assert
the promise rejects (use await expect(...).rejects.toThrow()) and that the
original file contents remain unchanged (read file and assert codexMode is still
true); ensure the test uses real fs ops, cleans up created files, and does not
rely on flaky timing (use the same timeout/config mechanism the implementation
reads or stub that config only if safe), and reference savePluginConfig to
locate the target logic.
In `@test/refresh-lease.test.ts`:
- Around line 215-282: The test currently simulates only EBUSY when unlinking
lock files; add a regression for Windows by updating the fsOps.unlink stub in
the "returns follower on wait-timeout when a fresh result appears" test in
test/refresh-lease.test.ts to also simulate EPERM for paths that
endWith(".lock") (or run the same test twice: once throwing EBUSY and once
throwing EPERM) so the stale-lock wait-timeout follower branch in
RefreshLeaseCoordinator (acquire, release behavior in lib/refresh-lease.ts) is
exercised under Windows semantics; keep the rest of the test flow identical
(write the stale .lock, publish the result after the same delay, assert
handle.role === "follower", result equality, fsOps.unlink called, and that
lockPath still exists after release).
- Around line 261-275: The test uses real setTimeout and real timing for the
publishResult vs coordinator.acquire race, making it flaky; change the test to
use vitest fake timers (vi.useFakeTimers()) around the section creating
publishResult and acquiring the lease (symbols: publishResult,
coordinator.acquire, refreshToken) and replace the real setTimeout with
advancing timers explicitly (vi.advanceTimersByTime(...)) so the writeFile
occurs deterministically before or after the coordinator.waitTimeoutMs path;
also restore real timers after the test (vi.useRealTimers()) and mirror this
deterministic control for any related timing in lib/refresh-lease.ts logic that
depends on waitTimeoutMs to ensure the race is reproducible.
---
Outside diff comments:
In `@lib/auth/auth.ts`:
- Around line 248-253: The code narrows error to Error in the refreshAccessToken
flow (variable err) so remove redundant optional chaining: replace err?.message
with err.message in both the timeout/abort branch and the network_error return
in refreshAccessToken. While here, check asymmetry with
exchangeAuthorizationCode: either add a brief comment on why only
refreshAccessToken calls logError("Token refresh error", err) or add equivalent
logging in exchangeAuthorizationCode to keep behavior consistent.
In `@lib/storage.ts`:
- Around line 1090-1172: The revision check/readStorageRevision + write +
fs.rename critical section is racy across processes; wrap the whole sequence
(expectedRevision check, synthetic-storage probe, createRotatingAccountsBackup,
writing walPath/tempPath, fs.rename(tempPath, path), unlink(walPath),
lastAccountsSaveTimestamp and rememberKnownStorageRevision) in an inter-process
lock (reuse the config lock strategy) so only one process can perform the
check+write+rename at a time; update the code paths referencing
knownStorageRevisionByPath, readStorageRevision,
looksLikeSyntheticFixtureStorage, createRotatingAccountsBackup,
walPath/tempPath, computeSha256 and rememberKnownStorageRevision to execute
under that lock, and add a vitest overlap/regression that spawns concurrent
writers (not just sequential) to assert stale-write rejection and proper
retry/backoff for EBUSY/EPERM, while ensuring logs do not contain tokens/emails
and tests reference the modified behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 75332c86-0400-44bd-be05-bd3795c66b6d
📒 Files selected for processing (19)
lib/accounts.tslib/auth/auth.tslib/codex-manager.tslib/config.tslib/request/response-handler.tslib/storage.tstest/accounts-edge.test.tstest/auth.test.tstest/chaos/fault-injection.test.tstest/codex-manager-cli.test.tstest/codex-prompts.test.tstest/config-save.test.tstest/fetch-helpers.test.tstest/host-codex-prompt.test.tstest/refresh-lease.test.tstest/response-handler-logging.test.tstest/response-handler.test.tstest/storage.test.tstest/unified-settings.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/codex-prompts.test.tstest/storage.test.tstest/config-save.test.tstest/codex-manager-cli.test.tstest/fetch-helpers.test.tstest/chaos/fault-injection.test.tstest/response-handler.test.tstest/refresh-lease.test.tstest/accounts-edge.test.tstest/host-codex-prompt.test.tstest/auth.test.tstest/response-handler-logging.test.tstest/unified-settings.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/accounts.tslib/storage.tslib/request/response-handler.tslib/codex-manager.tslib/auth/auth.tslib/config.ts
🧬 Code graph analysis (17)
test/codex-prompts.test.ts (1)
lib/prompts/codex.ts (1)
getCodexInstructions(214-303)
test/config-save.test.ts (1)
lib/config.ts (1)
savePluginConfig(538-581)
test/codex-manager-cli.test.ts (1)
lib/codex-manager.ts (1)
runCodexMultiAuthCli(4061-4122)
test/fetch-helpers.test.ts (1)
lib/request/fetch-helpers.ts (1)
createCodexHeaders(535-580)
test/chaos/fault-injection.test.ts (1)
lib/request/response-handler.ts (1)
convertSseToJson(51-125)
test/response-handler.test.ts (1)
lib/request/response-handler.ts (1)
convertSseToJson(51-125)
test/refresh-lease.test.ts (1)
lib/refresh-lease.ts (1)
RefreshLeaseCoordinator(163-425)
test/accounts-edge.test.ts (1)
lib/accounts.ts (1)
AccountManager(103-958)
lib/accounts.ts (2)
lib/storage.ts (3)
AccountStorageV3(26-26)saveAccounts(1234-1238)loadAccounts(883-885)lib/utils.ts (2)
isRecord(11-13)sleep(65-67)
test/host-codex-prompt.test.ts (1)
lib/prompts/host-codex-prompt.ts (1)
getHostCodexPrompt(351-382)
test/auth.test.ts (1)
lib/auth/auth.ts (2)
exchangeAuthorizationCode(116-169)refreshAccessToken(203-255)
lib/storage.ts (1)
lib/storage/migrations.ts (1)
AccountStorageV3(59-64)
test/response-handler-logging.test.ts (1)
lib/request/response-handler.ts (1)
convertSseToJson(51-125)
lib/codex-manager.ts (1)
lib/quota-cache.ts (2)
QuotaCacheData(22-25)saveQuotaCache(219-263)
test/unified-settings.test.ts (1)
lib/unified-settings.ts (3)
getUnifiedSettingsPath(293-295)saveUnifiedPluginConfig(357-376)loadUnifiedPluginConfigSync(306-314)
lib/auth/auth.ts (1)
lib/utils.ts (2)
fetchWithTimeout(76-112)isAbortError(20-24)
lib/config.ts (1)
lib/utils.ts (2)
sleep(65-67)isRecord(11-13)
🔇 Additional comments (24)
test/codex-prompts.test.ts (1)
242-263: solid regression test for api timeout → html fallback flow.this covers the
fetchWithTimeouthardening from the pr. the AbortError mock is correctly shaped withname: "AbortError"and the assertion onmockFetch.toHaveBeenCalledTimes(3)confirms the full cascade: api timeout → html releases page → raw content fetch.test/unified-settings.test.ts (1)
228-269: good regression for optimistic-conflict retry path.this now explicitly proves retry execution (
settingsReadCount >= 3) and validates the final persisted state, aligned with the retry loop inlib/unified-settings.ts:356-375. setup/teardown and spy restore intest/unified-settings.test.ts:241-267keep this deterministic.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."lib/request/response-handler.ts (1)
86-100: fail-closed parse handling is correct here.good change in
lib/request/response-handler.ts:86-100. returning502with a structured json envelope and explicit json content-type is the right contract, and it aligns with the updated vitest coverage intest/response-handler.test.ts:64-100andtest/chaos/fault-injection.test.ts:313-363.test/response-handler-logging.test.ts (1)
17-28: logging regression coverage is strong.
test/response-handler-logging.test.ts:17-46now pins both sides: no logging on success, and fixed/sanitized logging on parse failure without raw token/email content. this is exactly the hardening we want for oc-chatgpt-multi-auth.Also applies to: 30-46
test/response-handler.test.ts (1)
64-82: these response-handler regressions are well covered.nice updates in
test/response-handler.test.ts:64-100andtest/response-handler.test.ts:120-130. you now lock the502 + json envelope + content-typecontract and include a crlf case for windows line endings.Also applies to: 84-100, 120-130
test/refresh-lease.test.ts (1)
448-449: owner expectation for malformed lock payload looks correct.
test/refresh-lease.test.ts:448-449now matches the staleness recovery behavior inlib/refresh-lease.ts:162-424where invalid lock payloads are treated as recoverable and allow owner acquisition.lib/auth/auth.ts (2)
6-6: lgtm on imports and timeout constants.the 30s timeout aligns with typical oauth provider recommendations.
fetchWithTimeoutusage fromlib/utils.ts:75-111correctly chains abort signals and creates named abort errors.Also applies to: 21-22
121-140: error handling for timeout/abort looks correct.the normalization at line 135 ensures non-Error throwables are handled. the dual check
isAbortError(err) || /timeout/i.test(err.message)covers both the AbortError fromfetchWithTimeoutand potential platform quirks. tests added attest/auth.test.ts:390-409validate this path.test/auth.test.ts (2)
390-409: test correctly covers abort path for exchange.mocking fetch to throw an AbortError with a "timeout" message validates the error normalization at
lib/auth/auth.ts:134-140. cleanup in finally block is correct.
530-563: good use of fake timers for deterministic timeout test.the mock correctly propagates the abort reason from
fetchWithTimeout(lines 538-542). the 1200ms advance exceeds the 1000ms timeout as expected. cleanup in finally is thorough.one note:
fetchWithTimeoutclamps minimum timeout to 1000ms (lib/utils.ts:80), so this test happens to use the boundary value. if you want explicit coverage of sub-1s requests being clamped, consider a separate case passingtimeoutMs: 500and verifying it still takes ~1000ms to fail.test/codex-manager-cli.test.ts (3)
229-229: good deterministic mock baseline.
test/codex-manager-cli.test.ts:229sets a stable default (true) so only targeted tests flip persistence failure behavior, which keeps this suite deterministic.As per coding guidelines
test/**: tests must stay deterministic and use vitest.
283-321: strong regression for forecast json failure contract.
test/codex-manager-cli.test.ts:283-321correctly pins the non-zero exit and structured payload behavior for quota-cache persistence failure, matchinglib/codex-manager.ts:2113-2137.As per coding guidelines
test/**: demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.
469-503: fix json failure path is covered well.
test/codex-manager-cli.test.ts:469-503validates the new failure contract forfix --live --jsonand aligns withlib/codex-manager.ts:2994-3022.As per coding guidelines
test/**: tests must stay deterministic and use vitest.test/fetch-helpers.test.ts (2)
241-263: idempotency-key normalization coverage is solid.
test/fetch-helpers.test.ts:241-263captures trim behavior and blank/undefined clearing, matchinglib/request/fetch-helpers.ts:571-579and preventing stale header carryover.
288-315: good parity checks for named vs positional header builders.
test/fetch-helpers.test.ts:288-315ensures equivalent idempotency behavior across both invocation forms inlib/request/fetch-helpers.ts:534-579.lib/codex-manager.ts (4)
605-614: centralized quota-cache persistence wrapper looks correct.
lib/codex-manager.ts:605-614cleanly standardizes the boolean save contract and keeps warning output generic, so it avoids token/email leakage.As per coding guidelines
lib/**: check for logging that leaks tokens or emails.
649-650: callsite migration is consistent and low risk.routing these paths through
persistQuotaCacheatlib/codex-manager.ts:649-650,1632-1633,2208-2209, and3075-3076is consistent. no new concurrency risk is introduced by these callsite swaps.As per coding guidelines
lib/**: focus on auth rotation, windows filesystem IO, and concurrency.Also applies to: 1632-1633, 2208-2209, 3075-3076
2113-2137: forecast json now correctly reports persistence failures.
lib/codex-manager.ts:2113-2137now surfacesquotaCachePersisted, appends a probe error when persistence fails, and returns non-zero. regression coverage is present attest/codex-manager-cli.test.ts:283-321.As per coding guidelines
lib/**: verify every change cites affected tests (vitest).
2994-3022: fix json now correctly fails closed on persistence failure.
lib/codex-manager.ts:2994-3022propagates quota-cache persistence status into the JSON envelope and exit code. this is covered bytest/codex-manager-cli.test.ts:469-503.As per coding guidelines
lib/**: verify every change cites affected tests (vitest).test/config-save.test.ts (1)
118-270: good conflict + windows contention coverage here.
test/config-save.test.ts:118-270exercises transientebusy, optimistic conflict retries, and mixed rename contention with concrete assertions. this is strong regression coverage for the new save hardening.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.lib/accounts.ts (1)
895-923: conflict-recovery flow looks correct and is now state-safe after retries.
lib/accounts.ts:895-923now appliesapplyPersistedStorageSnapshotafter conflict-retry success, which keeps manager memory aligned with merged disk state. coverage is present intest/accounts-edge.test.ts:419-462andtest/accounts-edge.test.ts:464-510.as per coding guidelines,
lib/**: 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.test/accounts-edge.test.ts (1)
419-510: good regression coverage for conflict merge semantics.
test/accounts-edge.test.ts:419-510directly validates retry-on-econflictmerge behavior and protects against undefined local fields clobbering concrete disk fields. this is the right guardrail for the new concurrency path.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.test/storage.test.ts (1)
731-770: solid stale-write regression foreconflict.
test/storage.test.ts:731-770correctly verifies stale overwrite rejection and ensures externally written accounts remain intact after rejection.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.lib/config.ts (1)
542-567: bounded conflict retry and retry-clamp behavior look good.
lib/config.ts:542-567adds a clear conflict-retry loop with revision snapshots, andlib/config.ts:753-759clamps retry-all max retries to a bounded hard limit. this aligns with the hardening intent and is covered bytest/config-save.test.ts:152-220.as per coding guidelines,
lib/**: 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.Also applies to: 753-759
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (1)
the sync variant is presumably called from synchronous plugin init paths, but // instead of Atomics.wait(...), let retry callers use the async path
// or accept that the sync path has no retry delay and just rethrows immediatelyat minimum, document that this blocks the event loop so future maintainers don't call it from hot paths. Prompt To Fix With AIThis is a comment left during a code review.
Path: lib/unified-settings.ts
Line: 196-203
Comment:
**`Atomics.wait` blocks the node.js event loop**
`Atomics.wait` is specified to block the *calling thread*. in node.js worker threads that's fine, but on the main thread it freezes the entire event loop for `10 * 2^attempt` ms (10 ms, 20 ms, 40 ms…). node.js does not throw in modern versions, but any i/o, timers, or incoming plugin messages are starved while the thread sleeps.
the sync variant is presumably called from synchronous plugin init paths, but `writeSettingsRecordSync` is also called inside `saveUnifiedPluginConfigSync` which can be invoked during active sessions. replace with a short `Atomics.waitAsync` (node 16+) or restructure the retry as async:
```typescript
// instead of Atomics.wait(...), let retry callers use the async path
// or accept that the sync path has no retry delay and just rethrows immediately
```
at minimum, document that this blocks the event loop so future maintainers don't call it from hot paths.
How can I resolve this? If you propose a fix, please make it concise. |
Close remaining PR #41 feedback by enforcing config lock ownership tokens, retrying transient storage reads, and adding deterministic/regression tests across auth, prompt fallback, chaos SSE, config save, and refresh lease paths. Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
test/refresh-lease.test.ts (1)
267-289:⚠️ Potential issue | 🟠 Majorstill non-deterministic — use fake timers to control the race at
test/refresh-lease.test.ts:268.line 268 uses real
setTimeout(resolve, 80)racing againstwaitTimeoutMs: 220at line 248. on slower runners this can flip the outcome betweenfollowerandbypass. the 10_000ms test timeout at line 288 masks the flakiness but doesn't fix it.the previous review provided a concrete diff using
vi.useFakeTimers()andvi.advanceTimersByTimeAsync()to make timing explicit. that approach should be applied here.as per coding guidelines,
test/**: "tests must stay deterministic and use vitest."proposed deterministic fix
it.each(["EBUSY", "EPERM"] as const)( "returns follower on wait-timeout when a fresh result appears (%s stale lock delete)", async (unlinkCode) => { + vi.useFakeTimers(); const refreshToken = "token-timeout-follower"; const tokenHash = hashToken(refreshToken); const resultPath = join(leaseDir, `${tokenHash}.result.json`); // ... fsOps setup unchanged ... - const publishResult = (async () => { - await new Promise((resolve) => setTimeout(resolve, 80)); + let resultPublished = false; + const publishResult = async () => { await writeFile( resultPath, JSON.stringify({ tokenHash, createdAt: Date.now(), result: sampleSuccessResult, }), "utf8", ); - })(); + resultPublished = true; + }; - const handle = await coordinator.acquire(refreshToken); - await publishResult; + const acquirePromise = coordinator.acquire(refreshToken); + // advance past the 80ms point to publish result + await vi.advanceTimersByTimeAsync(100); + await publishResult(); + // advance past waitTimeoutMs to trigger follower path + await vi.advanceTimersByTimeAsync(150); + const handle = await acquirePromise; expect(handle.role).toBe("follower"); expect(handle.result).toEqual(sampleSuccessResult); await handle.release(sampleSuccessResult); await expect(fsPromises.stat(lockPath)).resolves.toBeTruthy(); }, - 10_000, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/refresh-lease.test.ts` around lines 267 - 289, The test is non-deterministic because publishResult uses a real setTimeout to wait 80ms and races against the coordinator's waitTimeoutMs; replace real timers with Vitest fake timers around this test: call vi.useFakeTimers() before creating publishResult, change the async waiter so it relies on setTimeout but then advance time via vi.advanceTimersByTimeAsync(80) (or advance past waitTimeoutMs) before awaiting publishResult, and finally call vi.useRealTimers() (or vi.restoreAllMocks()/vi.useRealTimers()) after the test; update references around publishResult, coordinator.acquire, refreshToken, and waitTimeoutMs to ensure the timing is driven by vi timers so the follower/bypass race is 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 `@lib/config.ts`:
- Around line 338-343: The exists-then-read path has a race: after existsSync()
the call to readFileUtf8WithRetry(configPath) can throw ENOENT if the file was
deleted; update the read logic in lib/config.ts (the block using existsSync,
readFileUtf8WithRetry, stripUtf8Bom, computeSha256) to catch errors from
readFileUtf8WithRetry and if err.code === 'ENOENT' return { record: null,
revision: null } (otherwise rethrow), so callers like savePluginConfig no longer
fail on concurrent deletes.
In `@lib/storage.ts`:
- Around line 1125-1135: The revision check in the commit path currently does a
read-compare-then-write without an inter-process lock (see readStorageRevision
and the surrounding commit/write code in lib/storage.ts), so concurrent
processes can both pass the check and cause a stale overwrite; wrap the revision
read + file write into an OS-level file lock (or fs mutex) held across the
compare-and-commit window (use a cross-process locking primitive for the storage
file), retry or fail cleanly on EBUSY/EBADF/EBUSY-like errors and translate
429-style contention into a retry/backoff policy, and ensure the commit path
throws StorageError with ECONFLICT if the lock cannot be acquired or the
revision changes while holding the lock; add a vitest regression that spawns two
independent writer processes (or child processes) to reproduce the race, assert
one writer gets ECONFLICT or a retried success, and update tests to check
logging avoids leaking tokens/emails and that EBUSY/429 scenarios are handled.
- Around line 1038-1040: The code records a non-null revision when handling the
ENOENT fallback, which later causes a false ECONFLICT; change the ENOENT branch
so it does NOT record the migrated legacy's revision (call
rememberKnownStorageRevisionForStorage(path, null) or omit the call) so the
known revision stays null for a missing primary file, and ensure subsequent save
logic in the same flow succeeds; add a regression vitest in test/storage.test.ts
that loads via the legacy fallback (migratedLegacyStorage path) and then
performs a follow-up save that must succeed; touch the symbols
rememberKnownStorageRevisionForStorage, migratedLegacyStorage and the
save/compare logic around load/save in lib/storage.ts and add the test,
verifying windows IO/concurrency behaviors and EBUSY/429 retry considerations
and that logs do not leak tokens/emails.
In `@test/codex-prompts.test.ts`:
- Around line 374-393: Add a new deterministic vitest it(...) test (near the
existing "should fall back to disk cache when prompt fetch times out") that
verifies Windows fs error codes fall back to disk: mock mockedReadFile to first
reject with an Error that has code 'EPERM' (or 'EBUSY') for the -meta.json path
(e.g. Object.assign(new Error("win fs busy"), { code: "EPERM" })), then resolve
with the disk string for subsequent reads; mock mockFetch the same way as the
existing test (first a successful tag_name JSON, then an AbortError timeout) and
call getCodexInstructions("gpt-5.2"); assert the returned value equals the disk
fallback string and that mockFetch was called the expected number of times (2),
ensuring the test uses vitest and is deterministic.
In `@test/config-save.test.ts`:
- Around line 126-158: Add a deterministic regression test alongside the
existing EBUSY case that simulates the exists-then-delete ENOENT race for
savePluginConfig: in the test that imports savePluginConfig, spy on fs.readFile
(like readSpy) and mock it so that when the target path equals configPath it
throws an ENOENT error exactly once (use makeErrnoError("noent","ENOENT") or
equivalent) and then delegates to the original readFile for subsequent calls;
call await savePluginConfig({ codexMode: false, retries: 3 }) and assert the
final file contents reflect the saved values and that the ENOENT counter is 1 to
prove savePluginConfig recovers from the race (refer to savePluginConfig and the
readFile spy to locate the code to change).
In `@test/host-codex-prompt.test.ts`:
- Around line 269-308: The test's assertions are too loose; change it so both
concurrent getHostCodexPrompt() callers deterministically return the stale value
("Old cached content"), then after advancing timers assert writeFile was called
with the new fetched content (i.e. check writeFile was invoked with "Prompt from
timeout fallback source" and appropriate cache metadata), then invoke
getHostCodexPrompt() once more and assert it returns "Prompt from timeout
fallback source"; also add a separate regression case that simulates Windows
EBUSY/EPERM during cache persistence (following the existing EBUSY pattern used
earlier in this file) to ensure the cache write-after-timeout-fallback path
handles those errors.
---
Duplicate comments:
In `@test/refresh-lease.test.ts`:
- Around line 267-289: The test is non-deterministic because publishResult uses
a real setTimeout to wait 80ms and races against the coordinator's
waitTimeoutMs; replace real timers with Vitest fake timers around this test:
call vi.useFakeTimers() before creating publishResult, change the async waiter
so it relies on setTimeout but then advance time via
vi.advanceTimersByTimeAsync(80) (or advance past waitTimeoutMs) before awaiting
publishResult, and finally call vi.useRealTimers() (or
vi.restoreAllMocks()/vi.useRealTimers()) after the test; update references
around publishResult, coordinator.acquire, refreshToken, and waitTimeoutMs to
ensure the timing is driven by vi timers so the follower/bypass race is
deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8783f5c7-afb4-4702-ba67-fa1085b40623
📒 Files selected for processing (9)
lib/config.tslib/storage.tstest/auth.test.tstest/chaos/fault-injection.test.tstest/codex-prompts.test.tstest/config-save.test.tstest/host-codex-prompt.test.tstest/refresh-lease.test.tstest/storage.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/codex-prompts.test.tstest/auth.test.tstest/config-save.test.tstest/refresh-lease.test.tstest/storage.test.tstest/host-codex-prompt.test.tstest/chaos/fault-injection.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/storage.tslib/config.ts
🧬 Code graph analysis (6)
test/codex-prompts.test.ts (1)
lib/prompts/codex.ts (1)
getCodexInstructions(214-303)
test/auth.test.ts (1)
lib/auth/auth.ts (2)
exchangeAuthorizationCode(116-169)refreshAccessToken(203-255)
lib/config.ts (1)
lib/utils.ts (2)
sleep(65-67)isRecord(11-13)
test/config-save.test.ts (1)
lib/config.ts (1)
savePluginConfig(638-681)
test/refresh-lease.test.ts (1)
lib/refresh-lease.ts (1)
RefreshLeaseCoordinator(163-425)
test/host-codex-prompt.test.ts (1)
lib/prompts/host-codex-prompt.ts (1)
getHostCodexPrompt(351-382)
🔇 Additional comments (15)
test/chaos/fault-injection.test.ts (1)
316-320: good hardening regression coverage for sse parse failures.these updates now lock the 502 json envelope contract with
error.type: "stream_parse_error"intest/chaos/fault-injection.test.ts:316and align withlib/request/response-handler.ts:88.Also applies to: 327-331, 338-342, 374-378
test/auth.test.ts (3)
390-409: good abort/timeout normalization regression coverage.
test/auth.test.ts:390now validates the abort/timeout error mapping expected bylib/auth/auth.ts:130-134, andtest/auth.test.ts:399-408correctly restores global fetch state.as per coding guidelines, "tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior."
530-563: good deterministic timeout regression for stalled refresh.
test/auth.test.ts:530-558exercises the timeout path for stalled refresh requests in a deterministic way with fake timers, aligned withlib/auth/auth.ts:202-254. cleanup attest/auth.test.ts:559-562is correct.as per coding guidelines, "tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior."
565-616: good concurrency race coverage for refresh flow.
test/auth.test.ts:565-611explicitly covers a concurrency risk: one refresh succeeds while another times out, and results stay isolated. this directly stress-tests the refresh logic inlib/auth/auth.ts:202-254.as per coding guidelines, "tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior."
test/codex-prompts.test.ts (1)
242-263: solid timeout regression for html fallback path.this test is deterministic and scoped well: it drives
AbortErrorand verifies the three-step fallback contract withexpect(mockFetch).toHaveBeenCalledTimes(3)intest/codex-prompts.test.ts:242andtest/codex-prompts.test.ts:262. this tightens retry/fallback safety without adding concurrency risk.As per coding guidelines,
test/**: tests must stay deterministic and use vitest.test/host-codex-prompt.test.ts (2)
215-233: this test injectsAbortErrordirectly, which doesn't exercise real timeout wiring inlib/prompts/host-codex-prompt.ts:350.since test 2 (lines 235-267) now exercises the actual 15s timeout via fake timers, this test is somewhat redundant as a unit-level sanity check. it's fine to keep for fast coverage, but understand it won't catch regressions where
fetchWithTimeoutstops passing the signal correctly.
235-267: this deterministic timeout regression looks solid.uses fake timers, advances past the 15s threshold, and validates both fetch calls and final result. the
try/finallycleanup pattern is correct.one minor note: consider also asserting that the first fetch received a signal (
expect(mockFetch.mock.calls[0]?.[1]?.signal).toBeInstanceOf(AbortSignal)) to confirm the timeout wiring is actually passed through.test/refresh-lease.test.ts (3)
27-31: lgtm — cleanup ensures timer state doesn't leak between tests.adding
vi.useRealTimers()in afterEach prevents fake timer state from one test bleeding into subsequent tests.
111-166: good — parameterized test now covers both EBUSY and EPERM for windows locked-file semantics.this addresses the regression gap for
lib/refresh-lease.ts:162-424where stale-lock wait-timeout follower behavior needed coverage across both error codes. the retry count assertion at line 163 confirms the coordinator properly retries transient unlink failures before acquiring ownership.
442-456: behavior change is correct — invalid payloads are treated as stale, not unknown.verified:
lib/refresh-lease.ts:80-100parseLeasePayloadstrictly validates pid as a number. when pid is "bad" (string), it coerces to NaN, failsNumber.isFinite(pid)at line 88, and returns null. thenassessLockStalenessat line 376-378 returns{ state: "stale", reason: "invalid-payload" }, causing the corrupted lock to be removed at line 252 so the process can retry and acquire as owner.this is the right distinction: unparseable json
"{"stays unreadable (bypass, don't touch), but parseable-but-invalid schemas are corrupted (stale, cleanup and recover). the test case attest/refresh-lease.test.ts:436-479properly regression-tests this with valid lock operations, stat behavior, and payload validation—no mocked secrets, deterministic with vitest.lib/storage.ts (1)
398-435: good windows read-retry hardening here.
lib/storage.ts:398-435cleanly centralizes transientEBUSY/EPERM/EAGAINretries and keepsENOENTsemantics explicit.test/storage.test.ts (2)
731-770: nice regression for stale-write rejection.
test/storage.test.ts:731-770is a solid, deterministic concurrency case. it proves external disk updates are preserved and stale writes are rejected withECONFLICT.
1896-1982: read retry tests are strong and targeted.
test/storage.test.ts:1896-1982covers transient contention in both load and pre-save revision reads with explicit assertions on retry counts and persisted output.lib/config.ts (1)
439-470: ownership-gated lock removal is a good safety improvement.
lib/config.ts:439-470correctly avoids unlinking lockfiles when token/fingerprint ownership does not match. this closes the stale-owner delete race cleanly.test/config-save.test.ts (1)
325-389: excellent coverage for lock ownership race behavior.
test/config-save.test.ts:325-389validates both release-time owner swap and stale-eviction owner swap paths with clear assertions. this is strong concurrency regression coverage.
Address remaining review findings across config/storage CAS locking, refresh lease timeout handling, recovery corruption logging, and deterministic Windows-focused regressions. Includes follow-up fixes to keep lock-release error semantics and refresh queue imports clean while preserving behavior. Co-authored-by: Codex <noreply@openai.com>
- keep disk refresh/access tokens authoritative during conflict merges matched by account identity - guard sync/async settings snapshot JSON parsing so malformed files are treated as recoverable - add regressions for identity-based token merge and malformed settings save recovery Co-authored-by: Codex <noreply@openai.com>
- guard sync read-after-exists paths against ENOENT TOCTOU in revision and snapshot readers - add sync save regression for existsSync/readFileSync ENOENT race Co-authored-by: Codex <noreply@openai.com>
Treat valid non-object unified settings roots as recoverable null snapshots in sync/async readers, and avoid surfacing false save failures when lock release fails after a successful write. Add regressions for both paths. Co-authored-by: Codex <noreply@openai.com>
When loadAccounts recovers from WAL or backup but primary persistence fails or the primary file is absent, refresh known revision from disk instead of deriving it from in-memory payloads. This prevents false ECONFLICT on the next save. Add regression coverage for failed persist-after-backup recovery. Co-authored-by: Codex <noreply@openai.com>
Update withConfigSaveLock to preserve successful writes when lock release fails by logging and swallowing release errors, while preserving original task failures. Add regression coverage for successful save with release-time EACCES failure. Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/storage.ts (1)
1200-1263:⚠️ Potential issue | 🟠 Majorknown revision can desync after v1→v3 migration persist and trigger false econFLICT
in the v1→v3 migration path, you
await persistMigration(normalized)(lib/storage.ts:1205-1214, Line 1205-1214) and then unconditionally callrememberKnownStorageRevision(path, rawChecksum)(lib/storage.ts:1256, Line 1256). if the persist succeeds,saveAccountsUnlocked()already updated the known revision to the new v3 content hash, and this line overwrites it with the pre-migration checksum. nextsaveAccounts()can throwECONFLICTeven without an external writer.proposed fix
try { - const { normalized, storedVersion, schemaErrors, rawChecksum } = await loadAccountsFromPath(path); + const { normalized, storedVersion, schemaErrors, rawChecksum } = await loadAccountsFromPath(path); if (schemaErrors.length > 0) { log.warn("Account storage schema validation warnings", { errors: schemaErrors.slice(0, 5) }); } + let persistedNormalized = false; if (normalized && storedVersion !== normalized.version) { log.info("Migrating account storage to v3", { from: storedVersion, to: normalized.version }); if (persistMigration) { try { await persistMigration(normalized); + persistedNormalized = true; } catch (saveError) { log.warn("Failed to persist migrated storage", { error: String(saveError) }); } } } @@ - rememberKnownStorageRevision(path, rawChecksum); + if (persistedNormalized && normalized) { + rememberKnownStorageRevisionForStorage(path, normalized); + } else { + rememberKnownStorageRevision(path, rawChecksum); + } return normalized; } catch (error) {also add a regression in
test/storage.test.tsthat does: write v1 →loadAccounts()(migration persists) →saveAccounts()again should succeed (noECONFLICT).as per coding guidelines,
lib/**: "focus on auth rotation, windows filesystem io, and concurrency."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/storage.ts` around lines 1200 - 1263, The code unconditionally calls rememberKnownStorageRevision(path, rawChecksum) after migration even when persistMigration(normalized) succeeded, which can overwrite the updated v3 checksum and cause a false ECONFLICT on subsequent save; change the flow in the load/migrate block (around loadAccountsFromPath, persistMigration, and the later rememberKnownStorageRevision call) so that if persistMigration(normalized) was invoked and completed without throwing, you do NOT call rememberKnownStorageRevision(path, rawChecksum) but instead record the migrated/v3 revision (either by calling rememberKnownStorageRevisionForStorage(path, normalized) or computing the checksum for normalized) — leave the fallback behavior unchanged when no persistMigration was run or when persistMigration failed; also add a regression test in test/storage.test.ts that writes v1, calls loadAccounts() (which persists migration) and then calls saveAccounts() and asserts no ECONFLICT is thrown.lib/unified-settings.ts (1)
220-260:⚠️ Potential issue | 🟠 MajorexpectedRevision check is too early; conflicts can still slip in during temp write
in lib/unified-settings.ts:224-230 (line 224) and lib/unified-settings.ts:286-292 (line 286), the expectedRevision check happens before writing the temp file. that leaves a large cas window: another process can modify settings.json after the check but before rename, and this write will still clobber it without ever throwing econFLICT.
move the expectedRevision check to just before the rename loop (after the temp file is fully written) to materially reduce stale-writer wins.
proposed fix (move revision check closer to commit)
function writeSettingsRecordSync( record: JsonRecord, options?: { expectedRevision?: string | null }, ): void { - const expectedRevision = options?.expectedRevision; - if (expectedRevision !== undefined) { - const currentRevision = readCurrentSettingsRevisionSync(); - if (currentRevision !== expectedRevision) { - throw createSettingsConflictError(); - } - } - mkdirSync(getCodexMultiAuthDir(), { recursive: true }); const payload = normalizeForWrite(record); const data = `${JSON.stringify(payload, null, 2)}\n`; const tempPath = `${UNIFIED_SETTINGS_PATH}.${process.pid}.${Date.now()}.tmp`; writeFileSync(tempPath, data, "utf8"); let moved = false; try { + const expectedRevision = options?.expectedRevision; + if (expectedRevision !== undefined) { + const currentRevision = readCurrentSettingsRevisionSync(); + if (currentRevision !== expectedRevision) { + throw createSettingsConflictError(); + } + } for (let attempt = 0; attempt < 5; attempt += 1) { try { renameSync(tempPath, UNIFIED_SETTINGS_PATH); moved = true; @@ async function writeSettingsRecordAsync( record: JsonRecord, options?: { expectedRevision?: string | null }, ): Promise<void> { - const expectedRevision = options?.expectedRevision; - if (expectedRevision !== undefined) { - const currentRevision = await readCurrentSettingsRevisionAsync(); - if (currentRevision !== expectedRevision) { - throw createSettingsConflictError(); - } - } - await fs.mkdir(getCodexMultiAuthDir(), { recursive: true }); const payload = normalizeForWrite(record); const data = `${JSON.stringify(payload, null, 2)}\n`; const tempPath = `${UNIFIED_SETTINGS_PATH}.${process.pid}.${Date.now()}.${Math.random() .toString(36) .slice(2, 8)}.tmp`; await fs.writeFile(tempPath, data, "utf8"); let moved = false; try { + const expectedRevision = options?.expectedRevision; + if (expectedRevision !== undefined) { + const currentRevision = await readCurrentSettingsRevisionAsync(); + if (currentRevision !== expectedRevision) { + throw createSettingsConflictError(); + } + } for (let attempt = 0; attempt < 5; attempt += 1) { try { await fs.rename(tempPath, UNIFIED_SETTINGS_PATH); moved = true;as per coding guidelines
lib/**: "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."Also applies to: 282-322
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/unified-settings.ts` around lines 220 - 260, The expectedRevision check in writeSettingsRecordSync is done too early and must be moved to just before the rename/commit so a concurrent writer cannot change settings between the temp file write and the atomic rename; after writing the temp file (after writeFileSync(tempPath, ...)) call readCurrentSettingsRevisionSync() and compare to options?.expectedRevision, throwing createSettingsConflictError() if they differ, then proceed into the existing renameSync retry loop and cleanup logic; ensure you keep the existing tempPath creation, moved flag, retry/backoff logic, and unlinkSync cleanup unchanged.
♻️ Duplicate comments (1)
test/refresh-lease.test.ts (1)
219-289:⚠️ Potential issue | 🟠 Majorwait-timeout follower regression does not reliably hit the wait-timeout branch
right now the result is published at +80ms (
test/refresh-lease.test.ts:269-279, Line 269-279). withpollIntervalMs: 25and the stale-unlink retry backoff insafeUnlink(),coordinator.acquire()can return via the cached-result fast path (lib/refresh-lease.ts:215-224, Line 215-224) around ~130ms, beforewaitTimeoutMs: 220elapses. that means this test can pass while never exercising the new wait-timeout-follow logic atlib/refresh-lease.ts:255-290(Line 255-290).you want the result to appear after the second loop iteration begins (after the ~130ms poll) so cachedResult can’t short-circuit, and the follower return happens from the timeout path.
proposed fix
it.each(["EBUSY", "EPERM"] as const)( "returns follower on wait-timeout when a fresh result appears (%s stale lock delete)", async (unlinkCode) => { vi.useFakeTimers(); vi.setSystemTime(new Date("2026-01-01T00:00:00.000Z")); const refreshToken = "token-timeout-follower"; @@ const coordinator = new RefreshLeaseCoordinator({ enabled: true, leaseDir, leaseTtlMs: 2_000, waitTimeoutMs: 220, pollIntervalMs: 25, resultTtlMs: 2_000, fsOps, }); @@ - const acquirePromise = coordinator.acquire(refreshToken); - await vi.advanceTimersByTimeAsync(80); + const acquirePromise = coordinator.acquire(refreshToken); + // publish after the first poll (~130ms) so cachedResult does not short-circuit. + await vi.advanceTimersByTimeAsync(200); await writeFile( resultPath, JSON.stringify({ tokenHash, createdAt: Date.now(), result: sampleSuccessResult, }), "utf8", ); - await vi.advanceTimersByTimeAsync(1_000); + await vi.advanceTimersByTimeAsync(200); const handle = await acquirePromise;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/refresh-lease.test.ts` around lines 219 - 289, The test currently writes the result at +80ms which allows RefreshLeaseCoordinator.acquire to hit the cached-result fast path (pollIntervalMs and safeUnlink retry backoff) before waitTimeoutMs elapses; change the timing so the result file is written only after the second poll iteration starts (i.e., after the ~130ms point) to force the wait-timeout follower path — specifically update the vi.advanceTimersByTimeAsync calls around where resultPath is written in the test so the write occurs after advancing past the first poll (~130ms) (affects test/refresh-lease.test.ts where acquire() is called and result is written, and targets behavior in RefreshLeaseCoordinator.acquire and safeUnlink).
🤖 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/accounts.ts`:
- Around line 75-77: DISK_PREFERRED_MERGE_KEYS currently contains "refreshToken"
and "accessToken" but not "expiresAt", which causes merged records to end up
with disk accessToken + incoming expiresAt and break token validity; update the
DISK_PREFERRED_MERGE_KEYS Set to include "expiresAt" and ensure
mergeStoredAccountRecords preserves expiresAt the same way it preserves
refreshToken/accessToken, then add a regression test that refreshes a token
locally (updates accessToken and expiresAt), forces a save conflict with stale
disk state, runs the merge path, and asserts the final StoredAccount has
matching accessToken and expiresAt.
In `@lib/recovery/storage.ts`:
- Around line 11-73: The local helper isRecord currently returns true for arrays
which diverges from the shared utility; replace the local isRecord with the
canonical implementation (import the shared isRecord from lib/utils.js or adjust
this function to return false for arrays using Array.isArray) so
isStoredMessageMeta and isStoredPart get consistent shape checks; update the
imports and ensure isStoredMessageMeta and isStoredPart still call isRecord and
run/adjust associated vitest cases to cover array inputs and corrupted
artifacts.
In `@lib/refresh-queue.ts`:
- Around line 14-18: The wait-timeout bypass branch currently returns a failed
result immediately for REFRESH_LEASE_BYPASS_REASON_WAIT_TIMEOUT; change it to
mirror the later try-block pattern by calling and awaiting
lease.getSupersedingPromise() before returning, and if a superseding generation
exists return the superseded result (include supersedingGeneration), otherwise
return the wait-timeout failure; update the logic in the function handling
leases (use the same handling around getSupersedingPromise() as the try block)
and add a unit test in test/refresh-queue.test.ts that simulates an entry being
superseded during the wait timeout to prevent regressions.
In `@lib/storage.ts`:
- Around line 497-657: readStorageSaveLockObservation currently throws on
transient read errors (EBUSY/EPERM/EAGAIN) — treat these the same as ENOENT and
return null so callers will continue polling; in readStorageSaveLockObservation
handle NodeJS transient errno codes and return null instead of rethrowing.
removeStorageSaveLockIfOwnerMatches must not throw on transient unlink failures
— catch EBUSY/EPERM/EAGAIN and return false (don’t rethrow) so the caller can
retry instead of leaving the .lock stranded. withStorageSaveFileLock’s finally
block should not rethrow or replace the original task error when
releaseStorageSaveFileLock fails — log the release failure but allow the
original task error to propagate without throwing from finally.
In `@test/storage.test.ts`:
- Around line 2039-2125: Add a deterministic regression test that writes a
v1-format storage file, calls loadAccounts() to trigger migration+persist, then
calls saveAccounts() to ensure no ECONFLICT is thrown and the final persisted
file has the expected data; implement it alongside the existing "read retry
hardening" tests, using the same testWorkDir/testStoragePath setup, and assert
that loadAccounts migrates (version becomes 3 or expected structure) and
saveAccounts succeeds and writes the updated account (no exception and
persisted.accounts[0].accountId matches the saved value). Target functions:
loadAccounts and saveAccounts; ensure the test fails before the lib/storage.ts:
revision desync fix and passes after the fix.
---
Outside diff comments:
In `@lib/storage.ts`:
- Around line 1200-1263: The code unconditionally calls
rememberKnownStorageRevision(path, rawChecksum) after migration even when
persistMigration(normalized) succeeded, which can overwrite the updated v3
checksum and cause a false ECONFLICT on subsequent save; change the flow in the
load/migrate block (around loadAccountsFromPath, persistMigration, and the later
rememberKnownStorageRevision call) so that if persistMigration(normalized) was
invoked and completed without throwing, you do NOT call
rememberKnownStorageRevision(path, rawChecksum) but instead record the
migrated/v3 revision (either by calling
rememberKnownStorageRevisionForStorage(path, normalized) or computing the
checksum for normalized) — leave the fallback behavior unchanged when no
persistMigration was run or when persistMigration failed; also add a regression
test in test/storage.test.ts that writes v1, calls loadAccounts() (which
persists migration) and then calls saveAccounts() and asserts no ECONFLICT is
thrown.
In `@lib/unified-settings.ts`:
- Around line 220-260: The expectedRevision check in writeSettingsRecordSync is
done too early and must be moved to just before the rename/commit so a
concurrent writer cannot change settings between the temp file write and the
atomic rename; after writing the temp file (after writeFileSync(tempPath, ...))
call readCurrentSettingsRevisionSync() and compare to options?.expectedRevision,
throwing createSettingsConflictError() if they differ, then proceed into the
existing renameSync retry loop and cleanup logic; ensure you keep the existing
tempPath creation, moved flag, retry/backoff logic, and unlinkSync cleanup
unchanged.
---
Duplicate comments:
In `@test/refresh-lease.test.ts`:
- Around line 219-289: The test currently writes the result at +80ms which
allows RefreshLeaseCoordinator.acquire to hit the cached-result fast path
(pollIntervalMs and safeUnlink retry backoff) before waitTimeoutMs elapses;
change the timing so the result file is written only after the second poll
iteration starts (i.e., after the ~130ms point) to force the wait-timeout
follower path — specifically update the vi.advanceTimersByTimeAsync calls around
where resultPath is written in the test so the write occurs after advancing past
the first poll (~130ms) (affects test/refresh-lease.test.ts where acquire() is
called and result is written, and targets behavior in
RefreshLeaseCoordinator.acquire and safeUnlink).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1ec4bfb8-29ba-447d-9c81-665b4c5a90fa
📒 Files selected for processing (16)
index.tslib/accounts.tslib/config.tslib/recovery/storage.tslib/refresh-lease.tslib/refresh-queue.tslib/storage.tslib/unified-settings.tstest/accounts-edge.test.tstest/codex-prompts.test.tstest/config-save.test.tstest/host-codex-prompt.test.tstest/recovery-storage.test.tstest/refresh-lease.test.tstest/storage.test.tstest/unified-settings.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/storage.test.tstest/config-save.test.tstest/accounts-edge.test.tstest/host-codex-prompt.test.tstest/unified-settings.test.tstest/recovery-storage.test.tstest/codex-prompts.test.tstest/refresh-lease.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/refresh-queue.tslib/refresh-lease.tslib/accounts.tslib/unified-settings.tslib/recovery/storage.tslib/storage.tslib/config.ts
🧬 Code graph analysis (12)
test/storage.test.ts (1)
lib/storage.ts (6)
saveAccounts(1491-1495)AccountStorageV3(26-26)loadAccounts(1134-1136)setStoragePath(686-716)getStoragePath(732-737)setStoragePathDirect(718-726)
test/config-save.test.ts (1)
lib/config.ts (1)
savePluginConfig(647-690)
test/host-codex-prompt.test.ts (1)
lib/prompts/host-codex-prompt.ts (1)
getHostCodexPrompt(351-382)
lib/refresh-queue.ts (1)
lib/refresh-lease.ts (1)
REFRESH_LEASE_BYPASS_REASON_WAIT_TIMEOUT(16-16)
lib/refresh-lease.ts (1)
lib/utils.ts (1)
sleep(65-67)
lib/accounts.ts (3)
lib/storage.ts (3)
AccountStorageV3(26-26)saveAccounts(1491-1495)loadAccounts(1134-1136)lib/prompts/codex.ts (2)
MODEL_FAMILIES(60-66)ModelFamily(49-54)lib/utils.ts (2)
isRecord(11-13)sleep(65-67)
lib/recovery/storage.ts (4)
lib/logger.ts (2)
createLogger(366-424)error(389-393)lib/utils.ts (1)
isRecord(11-13)lib/recovery/types.ts (2)
StoredMessageMeta(15-25)StoredPart(67-78)lib/recovery/constants.ts (2)
MESSAGE_STORAGE(31-31)THINKING_TYPES(34-34)
test/unified-settings.test.ts (1)
lib/unified-settings.ts (4)
getUnifiedSettingsPath(340-342)saveUnifiedPluginConfig(404-423)loadUnifiedPluginConfigSync(353-361)saveUnifiedPluginConfigSync(375-392)
test/recovery-storage.test.ts (1)
lib/recovery/constants.ts (2)
MESSAGE_STORAGE(31-31)PART_STORAGE(32-32)
lib/config.ts (1)
lib/utils.ts (2)
sleep(65-67)isRecord(11-13)
test/codex-prompts.test.ts (1)
lib/prompts/codex.ts (1)
getCodexInstructions(214-303)
test/refresh-lease.test.ts (1)
lib/refresh-lease.ts (1)
RefreshLeaseCoordinator(164-426)
🪛 Biome (2.4.4)
lib/storage.ts
[error] 649-649: Unsafe usage of 'throw'.
(lint/correctness/noUnsafeFinally)
🔇 Additional comments (35)
test/codex-prompts.test.ts (2)
242-263: nice deterministic timeout fallback coverage for html release discovery.line [242] through line [263] cleanly models abort-on-primary then html fallback, and the
mockFetchcall bound keeps this path stable intest/codex-prompts.test.ts:242.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.
374-423: good disk-timeout and windows transient fs regression coverage.line [374] through line [423] validates both timeout fallback to disk and transient windows metadata-read failure handling (
EPERM) with explicit assertions intest/codex-prompts.test.ts:374andtest/codex-prompts.test.ts:395.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.test/host-codex-prompt.test.ts (2)
235-267: strong deterministic timeout + concurrency race coverage.line [235] through line [318] now proves real timeout progression with fake timers and confirms stale-cache concurrent callers stay deterministic before background refresh settles in
test/host-codex-prompt.test.ts:235andtest/host-codex-prompt.test.ts:269.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: 269-318
320-370: windows transient write retry regression is well covered.line [320] through line [370] parameterizes both
EBUSYandEPERMon the timeout-fallback persistence path with explicit retry assertions intest/host-codex-prompt.test.ts:320.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.lib/refresh-lease.ts (3)
15-51: typed wait-timeout reason + handle surface is cleanexporting
REFRESH_LEASE_BYPASS_REASON_WAIT_TIMEOUTand surfacingreason?: stringonRefreshLeaseHandlemakes downstream handling inlib/refresh-queue.tsmuch less fragile than string contains checks. seelib/refresh-lease.ts:16(Line 16) andlib/refresh-lease.ts:46-51(Line 46-51).
199-294: timeout paths returning follower on fresh result are the right fail-closed shapethe new
finalResultreads on wait-timeout (both stale-lock and non-stale branches) reduce unnecessary bypasses and keep cross-process refresh consistent. seelib/refresh-lease.ts:255-290(Line 255-290). release remaining a no-op for followers is correct and avoids accidental lock cleanup by non-owners.
365-402: staleness classification change looks correct and matches new regression intenttreating
invalid-payloadandtoken-mismatchasstaleshould help clear corrupted artifacts instead of spinning onunknown. seelib/refresh-lease.ts:377-386(Line 377-386).test/refresh-lease.test.ts (1)
23-31: good windows coverage + timer hygiene; expectations match new staleness semantics
- restoring timers in
afterEachavoids cross-test contamination. seetest/refresh-lease.test.ts:27-31(Line 27-31).- the
it.each(["EBUSY","EPERM"])coverage is exactly what we need for windows locked-file behavior. seetest/refresh-lease.test.ts:111-167(Line 111-167).- updating the staleness edge-case expectation to
ownermatcheslib/refresh-lease.ts:379-383(Line 379-383). seetest/refresh-lease.test.ts:455-457(Line 455-457).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: 111-167, 436-457
lib/storage.ts (3)
59-101: storageerror + platform hints are a strong diagnosability win
StorageError+formatStorageErrorHint()looks production-ready and keeps sensitive data out of logs (no refresh tokens / emails). seelib/storage.ts:62-74(Line 62-74) andlib/storage.ts:79-101(Line 79-101).as per coding guidelines,
lib/**: "check for logging that leaks tokens or emails."
37-43: transient read retry + raw checksum tracking is the right windows hardeningthe bounded retry on
EBUSY/EPERM/EAGAINfor reads (lib/storage.ts:401-425, Line 401-425) plusrawChecksumtracking from the actual file bytes (lib/storage.ts:1153-1161, Line 1153-1161) is a good combo: it avoids false conflicts from normalization-only differences and reduces windows fs contention flakes. the new regressions intest/storage.test.ts:2055-2124cover these paths well.as per coding guidelines,
lib/**: "focus on auth rotation, windows filesystem io, and concurrency."Also applies to: 401-438, 1149-1161
686-727: good: revision state reset on path swaps; clearaccounts removes lock artifactsforgetting the previous path’s revision state in
setStoragePath()/setStoragePathDirect()avoids carryingexpectedRevisionacross identities. seelib/storage.ts:686-726(Line 686-726). also good to seeclearAccounts()removing the.lockartifact and resetting revision to null (lib/storage.ts:1505-1529, Line 1505-1529).Also applies to: 1501-1533
test/storage.test.ts (2)
731-849: solid regressions for lock-release failure + stale-write rejectionthese new cases are on point for the hardening goals:
- lock release failure after a successful write stays a success (
test/storage.test.ts:731-760, Line 731-760).ECONFLICTon stale overwrite after an out-of-band disk change (test/storage.test.ts:762-801, Line 762-801).- cross-module-instance concurrency reproducer (
test/storage.test.ts:803-848, Line 803-848).as per coding guidelines,
test/**: "demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior."
1184-1247: good coverage for enoent legacy fallback + follow-up savethe follow-up save assertion is exactly what prevents “load via fallback then permanently stuck” regressions. see
test/storage.test.ts:1184-1247(Line 1184-1247).test/recovery-storage.test.ts (2)
27-54: logger mocking wiring is correct and keeps tests focusedmocking
createLoggerviavi.mock("../lib/logger.js", ...)and resetting implementation inbeforeEachmakes the warning-budget assertions reliable. seetest/recovery-storage.test.ts:27-54(Line 27-54).
154-227: shape-guard + corruption warning budget regressions are strongthe new cases for invalid shapes and capped corruption warnings are exactly the kind of “corrupt artifact shouldn’t brick recovery” regression we need. see
test/recovery-storage.test.ts:154-226(Line 154-226) andtest/recovery-storage.test.ts:271-290(Line 271-290).as per coding guidelines,
test/**: "demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior."Also applies to: 271-291
lib/accounts.ts (1)
729-951: conflict recovery flow + in-memory rehydration is the right direction
persistStorageWithConflictRecovery()+applyPersistedStorageSnapshot()fixes the classic “merge saved to disk but manager memory stays stale” failure mode. seelib/accounts.ts:901-946(Line 901-946). the retry is bounded and uses backoff, which is good for contention.test/unified-settings.test.ts (3)
68-167: good recovery coverage for malformed/non-object settings + sync enoent racetest/unified-settings.test.ts:68-167 (line 68+) adds solid vitest coverage for malformed json and valid-but-non-object roots across both async and sync save paths, plus a deterministic existsSync→readFileSync enoent injection in the sync reader. the explicit
expect(noentInjected).toBe(true)at test/unified-settings.test.ts:162 (line 162) keeps this from being a vacuous regression.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."
329-369: retry regression now proves the retry loop ranthe
expect(settingsReadCount).toBeGreaterThanOrEqual(3)assertion at test/unified-settings.test.ts:367 (line 367) is the right tightening. it ensures the optimistic-conflict retry path can’t silently stop executing while still passing the test.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."
371-413: async enoent toctou regression is deterministictest/unified-settings.test.ts:371-413 injects a single enoent on
fs.readFileand asserts both the injection (noentInjected) and the final persisted config. this is a nice guard for windows-style read-after-exists races.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."test/config-save.test.ts (4)
27-40: test helpers make errno/lock simulations clearer
makeErrnoErrorandmakeLockPayloadin test/config-save.test.ts:27-39 (line 27+) make the new contention/lock tests easier to read and keep the injected failures explicit.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."
126-193: transient read contention + enoent toctou regressions are well assertedthe ebusy retry test at test/config-save.test.ts:126-158 (line 126) and the enoent race test at test/config-save.test.ts:160-193 (line 160) both prove the injected paths executed (
busyFailures/noentFailures) and validate the final persisted content. good windows/fs contention coverage.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."
194-263: econFLICT retry success + exhaustion coverage looks solidtest/config-save.test.ts:194-263 (line 194) exercises both eventual success and retry exhaustion for the optimistic expectedRevision path. the
configReadCountassertions (test/config-save.test.ts:229, line 229) help ensure the loop is actually doing work.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."
264-313: mixed conflict + rename contention regression is valuablethe combined scenario in test/config-save.test.ts:264-313 (line 264) forces both conflict churn and a retryable rename error (
ebusy) and assertsrenameAttempts >= 2at test/config-save.test.ts:311 (line 311). this is the kind of “realistic failure cocktail” that catches windows-only bugs.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."test/accounts-edge.test.ts (1)
419-623: conflict-merge edge coverage matches the hardening goalsthese new vitest cases in test/accounts-edge.test.ts:419-623 (line 419+) hit the right tricky branches: econFLICT retry payload composition, “undefined doesn’t clobber disk” merges, identity-based disk credential preference, and timestamp/rate-limit precedence. the second
saveToDisk()assertion in test/accounts-edge.test.ts:455-462 (line 455) is a good guard that the merged snapshot actually sticks.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."lib/recovery/storage.ts (4)
106-111: safe debug logging on directory enumeration failurethe try/catch around
readdirSync(MESSAGE_STORAGE)in lib/recovery/storage.ts:99-111 (line 99) is a good resilience improvement. the debug log includes only the storage path + error message, which keeps it useful without dumping file contents.as per coding guidelines
lib/**: "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."
124-148: readMessages now tolerates malformed artifacts without aborting the whole scanlib/recovery/storage.ts:124-148 (line 124) switches to “skip invalid + warn (bounded)” semantics. this is the right behavior for recovery: keep the good messages, drop the broken ones, and avoid log spam.
as per coding guidelines
lib/**: "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."
167-194: readParts mirrors readMessages corruption handlinglib/recovery/storage.ts:167-194 (line 167) applies the same validated-parse + bounded-warn approach for parts. good symmetry and much safer than assuming the file contents are always trustworthy.
as per coding guidelines
lib/**: "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."
352-384: mutation helpers now skip invalid part shapes safelylib/recovery/storage.ts:352-384 (line 352) and lib/recovery/storage.ts:451-489 (line 451) now validate the parsed part shape before unlinking or rewriting. this avoids “recovery tool crashes on one corrupted json file” while keeping behavior best-effort.
as per coding guidelines
lib/**: "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."Also applies to: 451-489
lib/unified-settings.ts (2)
11-161: snapshot+revision readers handle malformed/non-object json and enoent raceslib/unified-settings.ts:73-161 (line 73+) does the right thing for hardening: keep a revision even when json is malformed or the root isn’t an object, and treat enoent after existsSync as “no record”. this is well covered by:
- malformed + non-object recovery tests in test/unified-settings.test.ts:68-124 (line 68)
- sync enoent toctou regression in test/unified-settings.test.ts:126-167 (line 126)
- async enoent toctou regression in test/unified-settings.test.ts:371-413 (line 371)
as per coding guidelines
lib/**: "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."Also applies to: 173-186
375-479: retry loops on econFLICT are clear and boundedthe bounded retry loops in lib/unified-settings.ts:375-479 (line 375+) are straightforward and consistent across sync/async plugin saves and dashboard saves. the retry execution is explicitly asserted by test/unified-settings.test.ts:329-369 (line 329).
as per coding guidelines
lib/**: "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."lib/config.ts (5)
38-45: bounded retry-all defaults/clamps are a good safety netthe shift to
RETRY_ALL_ACCOUNTS_DEFAULT_MAX_RETRIESat lib/config.ts:135 (line 135) and the clamp in lib/config.ts:862-868 (line 862) prevents config values likeinfinityfrom turning into unbounded loops. the existing sanitization test at test/config-save.test.ts:79-108 (line 79) also keeps the persisted json clean.as per coding guidelines
lib/**: "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."Also applies to: 125-139, 862-868
288-364: snapshot reads now retry windows contention and guard enoent after exists
readFileUtf8WithRetry(lib/config.ts:306-318, line 306) plus the enoent guard inreadConfigSnapshotFromPath(lib/config.ts:341-350, line 341) is the right way to harden existsSync→read races and transient windows lock contention. the new regressions in test/config-save.test.ts:126-193 (line 126) cover both ebusy and enoent behaviors.as per coding guidelines
lib/**: "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."
366-405: expectedRevision enforcement in atomic writes lines up with conflict teststhe
expectedRevisioncheck inwriteJsonFileAtomicWithRetry(lib/config.ts:371-377, line 371) provides a clean econFLICT signal for stale snapshots, and the retry/exhaustion paths are exercised by test/config-save.test.ts:194-263 (line 194).as per coding guidelines
lib/**: "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."
407-479: lock token + fingerprint ownership checks close the stale-unlink footgun
readConfigSaveLockObservation+removeConfigSaveLockIfOwnerMatchesin lib/config.ts:432-479 (line 432) ensure stale eviction and release paths don’t blindly unlink by path. the “ownership changes before release” regression (test/config-save.test.ts:359-386, line 359) and the stale-check race regression (test/config-save.test.ts:388-422, line 388) are good coverage. also good: no token/email values are logged.as per coding guidelines
lib/**: "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."
481-582: cross-process lock wrapping + conflict retry loop aligns with hardening goalswithConfigSaveLock acquiring/releasing a lockfile around the whole save (lib/config.ts:481-491, line 481) closes the classic cas race for cross-process writers, and the fail-closed timeout behavior is covered by test/config-save.test.ts:340-357 (line 340). the “waits for lockfile release” regression (test/config-save.test.ts:314-338, line 314) also validates the expected blocking semantics.
as per coding guidelines
lib/**: "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."Also applies to: 647-690
- preserve expiresAt during disk-preferred conflict merges - join superseding refresh generation on wait-timeout bypass - harden storage lock read/unlink transient handling - sync known revision after migration persistence - align recovery record guards and add regression tests Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/storage.ts`:
- Around line 497-523: readStorageSaveLockObservation currently converts
transient read errors to null causing removeStorageSaveLockIfOwnerMatches to
treat transient failures as "lock released" and falsely succeed; change
readStorageSaveLockObservation to rethrow transient read errors instead of
returning null so callers can retry/handle contention, and update
removeStorageSaveLockIfOwnerMatches to retry on transient read/unlink errors (or
fail so higher-level logic retries) until the lock is actually removed or
confirmed owned. Add a Vitest regression that simulates transient read/unlink
contention (e.g., mock readFileUtf8WithTransientRetry/unlink to throw transient
errors like EBUSY/EACCES on first attempts then succeed) and assert that the
release operation eventually removes the .lock file and does not return success
when the file still exists; apply the same change/coverage for the analogous
code ranges around
readStorageSaveLockObservation/removeStorageSaveLockIfOwnerMatches (lines
referenced around 537-549 and 620-630) so all release paths handle transient FS
contention correctly.
- Around line 566-583: The save lock code only evicts locks when
observation.acquiredAt is a valid number, which leaves zero-byte or malformed
lock files un-evictable; update the logic in the EEXIST handler (around
readStorageSaveLockObservation and removeStorageSaveLockIfOwnerMatches) to treat
missing/invalid acquiredAt as stale when the lock file's age (fs.stat mtime or
Date.now() - fsStat.mtimeMs) exceeds STORAGE_SAVE_LOCK_STALE_AFTER_MS and then
attempt removeStorageSaveLockIfOwnerMatches; add a vitest that seeds an old
empty "<path>.lock" (zero-byte or malformed JSON), ensures its mtime is older
than STORAGE_SAVE_LOCK_STALE_AFTER_MS, calls the save path that hits the EEXIST
branch, and asserts the save succeeds (lock evicted and operation proceeds);
also audit the code paths referenced by readStorageSaveLockObservation and the
subsequent EEXIST handling (including the later timeout handling around the save
retry logic at the 608-617 area) to ensure they treat malformed payloads as
stale and handle EBUSY/429 style retry semantics.
In `@test/storage.test.ts`:
- Around line 790-832: The test uses a timing-based setTimeout to release the
lock (unlockPromise with setTimeout(80)), which makes the test flaky; instead
replace the timeout with a deterministic signaling mechanism: create a deferred
resolver (e.g., releaseLock) and have the mocked fs.readFile (the vi.spyOn
mockImplementation referencing transientReadFailures, lockPath, lockPresent)
call releaseLock (or trigger unlink and set lockPresent = false) once the
desired number of transient read failures have been injected, then await that
promise from the test (where unlockPromise currently is) so saveAccounts()
proceeds only after the mock signals the lock release; update the unlockPromise
logic to remove setTimeout and ensure readSpy.mockRestore() and awaiting the
unlink still happen in finally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 10415b28-8e8f-444a-8039-2934405e9aff
📒 Files selected for processing (11)
lib/accounts.tslib/config.tslib/recovery/storage.tslib/refresh-queue.tslib/storage.tstest/accounts-edge.test.tstest/config-save.test.tstest/recovery-storage.test.tstest/refresh-queue.test.tstest/storage-recovery-paths.test.tstest/storage.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/accounts-edge.test.tstest/config-save.test.tstest/storage.test.tstest/recovery-storage.test.tstest/storage-recovery-paths.test.tstest/refresh-queue.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/recovery/storage.tslib/refresh-queue.tslib/accounts.tslib/storage.tslib/config.ts
🧬 Code graph analysis (9)
test/accounts-edge.test.ts (1)
lib/accounts.ts (1)
AccountManager(104-986)
test/config-save.test.ts (1)
lib/config.ts (1)
savePluginConfig(667-710)
lib/refresh-queue.ts (1)
lib/refresh-lease.ts (1)
REFRESH_LEASE_BYPASS_REASON_WAIT_TIMEOUT(16-16)
test/recovery-storage.test.ts (1)
lib/recovery/constants.ts (2)
MESSAGE_STORAGE(31-31)PART_STORAGE(32-32)
lib/accounts.ts (3)
lib/storage.ts (3)
AccountStorageV3(26-26)saveAccounts(1507-1511)loadAccounts(1144-1146)lib/prompts/codex.ts (2)
MODEL_FAMILIES(60-66)ModelFamily(49-54)lib/utils.ts (2)
isRecord(11-13)sleep(65-67)
test/storage-recovery-paths.test.ts (1)
lib/storage.ts (2)
loadAccounts(1144-1146)saveAccounts(1507-1511)
lib/storage.ts (1)
lib/storage/migrations.ts (1)
AccountStorageV3(59-64)
test/refresh-queue.test.ts (1)
lib/refresh-lease.ts (2)
REFRESH_LEASE_BYPASS_REASON_WAIT_TIMEOUT(16-16)RefreshLeaseCoordinator(164-426)
lib/config.ts (2)
lib/logger.ts (1)
logWarn(341-346)lib/utils.ts (2)
sleep(65-67)isRecord(11-13)
🔇 Additional comments (21)
lib/refresh-queue.ts (2)
14-17: typed bypass reason usage looks correct
lib/refresh-queue.ts:14now importsREFRESH_LEASE_BYPASS_REASON_WAIT_TIMEOUTfromlib/refresh-lease.ts:15, which removes fragile string matching in the lease decision path.
175-192: wait-timeout bypass branch now handles stale generation race correctly
lib/refresh-queue.ts:179checksgetSupersedingPromise()before returning fail-closed, so stale callers join the active generation instead of failing spuriously. coverage is present intest/refresh-queue.test.ts:928and the fail-closed path is covered attest/refresh-queue.test.ts:903.
as per coding guidelines,lib/**: "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."test/refresh-queue.test.ts (3)
7-10: imports align with the lease-timeout regression scope
test/refresh-queue.test.ts:7pulls in the typed wait-timeout reason and coordinator needed for precise lease-path testing.
903-926: fail-closed regression test is solid and deterministic
test/refresh-queue.test.ts:903correctly asserts the wait-timeout bypass returns failure and thatrefreshAccessTokenis not called, which protects against fail-open behavior.
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."
928-985: good concurrency regression for stale wait-timeout superseding
test/refresh-queue.test.ts:928reproduces the stale-generation timing window with fake timers and verifies both callers converge on one refresh result with a single auth call. this directly validates the race fix inlib/refresh-queue.ts:175.
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."test/recovery-storage.test.ts (1)
27-53: strong deterministic recovery regressionsgood coverage in
test/recovery-storage.test.ts:154-247andtest/recovery-storage.test.ts:292-332. these cases assert invalid-shape filtering, array-payload rejection, warning-budget capping, and per-call budget reset with explicit logger assertions.Also applies to: 154-247, 292-332
lib/config.ts (1)
335-364: good lock and conflict hardening on env-path saves
lib/config.ts:335-364,lib/config.ts:407-603, andlib/config.ts:671-696close the key race windows: ENOENT TOCTOU handling, lock ownership validation, and bounded conflict retries withexpectedRevision. this lines up with the lock/conflict regressions intest/config-save.test.ts:160-262andtest/config-save.test.ts:398-462.Also applies to: 407-603, 671-696
lib/accounts.ts (1)
75-77: conflict-merge flow is materially safer nowthe merge/retry path in
lib/accounts.ts:768-951is solid: disk-preferred token fields, max-freshness timestamp/rate-limit merges, and in-memory rehydrate after conflict persistence all reduce lost-update risk.Also applies to: 768-951
test/config-save.test.ts (1)
126-462: excellent regression coverage for lock and conflict paths
test/config-save.test.ts:126-462now covers the critical contention matrix (ebusy/enoent read races, optimistic conflicts, mixed rename contention, lock wait, timeout fail-closed, and ownership-swap races) with deterministic vitest assertions.lib/recovery/storage.ts (1)
24-70: good hardening for corrupted recovery artifacts
lib/recovery/storage.ts:24-70and the guarded call sites inlib/recovery/storage.ts:121-190,lib/recovery/storage.ts:349-381, andlib/recovery/storage.ts:448-486are a strong improvement: malformed artifacts are safely skipped, and warning spam is bounded per invocation.Also applies to: 121-190, 349-486
test/storage.test.ts (8)
21-21: typed fixture import is a good tightening.
test/storage.test.ts:21improves test payload typing and reduces accidental shape drift.
680-706: good regression for post-migration save safety.
test/storage.test.ts:680directly locks in the v1→v3 load-then-save no-conflict path and verifies persisted ids.
759-788: lock-release failure behavior is covered well.
test/storage.test.ts:759correctly verifies successful writes are preserved even when lock cleanup throws.
834-878: strong stale-lock eviction retry coverage.
test/storage.test.ts:834cleanly reproduces transient unlink contention and verifies retry semantics on stale lock cleanup.
880-919: conflict rejection test is on point.
test/storage.test.ts:880validates stale-write rejection (ECONFLICT) and confirms disk state is not polluted by stale payloads.
921-966: good concurrent writer regression across module instances.
test/storage.test.ts:921exercises real multi-instance contention and asserts exactly one winner plus oneECONFLICT, which is the right concurrency contract.
1302-1365: enoent legacy fallback follow-up save regression looks solid.
test/storage.test.ts:1302captures the rename-failure fallback path and verifies immediate subsequent persistence to canonical storage.
2157-2243: read-retry hardening coverage is valuable and targeted.
test/storage.test.ts:2157adds concrete regressions for transient read contention during load and revision checks before save.test/storage-recovery-paths.test.ts (2)
1-1: no concerns on the import update.
test/storage-recovery-paths.test.ts:1remains aligned with vitest usage and test style in this suite.
97-152: this recovery regression is well-scoped and useful.
test/storage-recovery-paths.test.ts:97verifies backup recovery remains usable after a forced persist failure, then confirms immediate save works and recreates primary storage.test/accounts-edge.test.ts (1)
419-629: excellent conflict-merge regression coverage here.
test/accounts-edge.test.ts:419adds deterministic tests forECONFLICTretry merging, token rotation precedence, and freshness-max field resolution that map directly to the storage conflict path inlib/accounts.ts:103-985.
- retry transient lock-release contention and stale-eviction paths - evict malformed stale lock files via mtime fallback - align forecast/fix quota-cache failure exits across json and text modes - make lock contention regression deterministic and expand CLI coverage Co-authored-by: Codex <noreply@openai.com>
1 participant
PR Resolution Update (2026-03-05) ### What changed - Closed remaining concurrency/reliability review findings across config/storage/settings/recovery/request paths. - Added cross-process storage save-lock coverage to protect compare-and-commit windows and prevent stale writer wins. - Hardened exists/read TOCTOU behavior for config and unified settings (
ENOENTafterexistsSync) and tightened lock ownership checks. - Replaced fragile refresh lease bypass string checks with a shared typed constant. - Finalized deterministic Windows-focused regressions forEPERM/EBUSY, timeout fallback behavior, and merge freshness semantics. ### How to test -npm test-npm run typecheck-npm run lint### Risk / rollout notes - Behavior remains backward-compatible except for correctness fixes under concurrent writers/timeouts. - Main risk area is lock contention under heavy parallel writes; mitigated by new ownership checks, retry handling, and regression coverage. - No new secrets/PII logging paths were introduced. ## Summary This PR implements targeted production hardening for hidden technical debt identified in the landmine audit. ## What changed - Added conflict detection and stale-write rejection for account storage persistence. - Added optimistic concurrency conflict handling and retries for unified settings and config writes. - Added conflict-aware merge-retry in account manager saves. - Added idempotency key propagation for request retry/failover safety. - Added fetch timeout utility and applied it to OAuth and prompt fetch paths. - Hardened refresh lease timeout behavior to fail closed on wait-timeout bypass in refresh queue. - Hardened recovery storage parsing with shape guards and bounded corruption warnings. - Updated quota cache save path to return persistence status and wired call sites to handle failure. - Hardened non-stream SSE parse failure behavior to avoid raw payload leakage and return structured error envelopes. - Bounded all-accounts retry default and clamp behavior. ## Validation - npm test - npm run typecheck - npm run lint All checks pass on the branch. ## PR Resolution Update (2026-03-05, follow-up) ### What changed - Preserved disk-authoritative credentials during account conflict merges by treatingrefreshTokenandaccessTokenas disk-preferred when a current disk value exists (lib/accounts.ts). - Guarded unified-settings snapshot parsing in both sync/async paths so malformed/truncated JSON is treated as recoverable (record: null) while keeping revision tracking (lib/unified-settings.ts). - Added focused regressions for identity-based credential merge safety and malformed settings save recovery (test/accounts-edge.test.ts,test/unified-settings.test.ts). ### How to test -npm test -- test/accounts-edge.test.ts test/unified-settings.test.ts-npm run typecheck-npm run lint-npm run build-npm test### Risk / rollout notes - No intended external behavior change beyond conflict-recovery correctness. - Main risk is conflict-merge/account identity edge handling; covered by new regression assertions. - Main settings risk is malformed file recovery semantics; saves now self-heal invalid JSON instead of failing permanently.PR Resolution Update (2026-03-05, follow-up 2)
What changed
ENOENTguards inlib/unified-settings.tsfor:readCurrentSettingsRevisionSyncreadSettingsSnapshotSyncexistsSyncand read.test/unified-settings.test.tsto verifysaveUnifiedPluginConfigSyncsucceeds when a one-timeENOENTis injected on sync read.How to test
npm test -- test/unified-settings.test.tsnpm run typechecknpm run lintnpm run build && npm test -- test/unified-settings.test.ts test/accounts-edge.test.tsRisk / rollout notes
Follow-up Review Resolution (2026-03-05)
How to test
Risk / rollout notes
Additional Follow-up Resolution (2026-03-05 lock-finalizer)
How to test
Risk / rollout notes
PR Resolution Update (2026-03-05, follow-up 3)
What changed
expiresAtas disk-preferred alongsiderefreshTokenandaccessToken(lib/accounts.ts).lib/refresh-queue.ts).(
lib/storage.ts).ECONFLICTon immediate follow-up saves (lib/storage.ts).isRecord(array-excluding) helper (lib/recovery/storage.ts).Tests added/updated
test/accounts-edge.test.tsexpiresAtis preserved with rotated credentials.test/refresh-queue.test.tstest/storage.test.tsECONFLICT).test/recovery-storage.test.tsreadMessagesandreadParts.How to test
npm test -- test/accounts-edge.test.ts test/refresh-queue.test.ts test/storage.test.ts test/recovery-storage.test.tsnpm run typechecknpm run lintRisk / rollout notes
PR Resolution Update (2026-03-05, follow-up 4)
What changed
lib/codex-manager.ts).(
lib/storage.ts).acquiredAtis absent (lib/storage.ts).test/storage.test.ts).Tests added/updated
test/codex-manager-cli.test.tsreturns non-zero in forecast text mode when quota cache persistence failsreturns non-zero in fix text mode when quota cache persistence failstest/storage.test.tsevicts stale malformed lock files using file mtime fallbackretries transient lock release contention and eventually removes lockcontinues waiting when lock observation reads hit transient errorsHow to test
npm test -- test/storage.test.ts test/codex-manager-cli.test.tsnpm run typechecknpm run lintRisk / 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 closes the remaining concurrency/reliability landmines identified in the audit: cross-process save locks for account storage and config, TOCTOU ENOENT guards in all sync and async read paths, conflict-detect-and-retry loops for settings/config/accounts writes, bounded
retryAllAccountsMaxRetriesdefault, typed refresh-lease bypass constants, per-invocation corruption loggers in recovery storage, structured SSE error envelopes,fetchWithTimeoutapplied to all outbound OAuth and prompt fetches, and idempotency key propagation. All previously flagged critical issues have regression coverage.Key hardening areas:
lib/storage.ts: cross-processacquireStorageSaveFileLock+ known-revision compare-and-commit window; migration-persist now callsrememberKnownStorageRevisionFromDiskto prevent falseECONFLICTon follow-up saves; release errors after successful writes are logged and swallowed; one minor dead-code branch in error-handling logic (harmless but should be collapsed for clarity)lib/accounts.ts:persistStorageWithConflictRecoverywith 3-attempt merge-retry;DISK_PREFERRED_MERGE_KEYSpreservesrefreshToken/accessToken/expiresAtfrom disk on identity-matched conflicts; max-freshness semantics for timestamp/rate-limit fieldslib/unified-settings.ts/lib/config.ts: TOCTOU ENOENT guards in all four snapshot readers (sync/async × revision/record); parse guards for malformed and valid-non-object JSON roots; conflict-retry loops; file-based cross-process config save locklib/refresh-queue.ts/lib/refresh-lease.ts: typedREFRESH_LEASE_BYPASS_REASON_WAIT_TIMEOUTconstant; supersede-join before fail-closed return; dead"unknown"lease states promoted to"stale"lib/utils.ts:fetchWithTimeoutwith caller-signal forwarding; applied to OAuth exchange/refresh and all prompt fetch pathslib/codex-manager.ts:saveQuotaCachereturn value wired asboolean; exit code consistently reflects quota cache persistence status across json and non-json modesConfidence Score: 5/5
Sequence Diagram
sequenceDiagram participant AM as AccountManager participant SA as saveAccountsUnlocked participant FL as withStorageSaveFileLock participant CAS as compareAndCommit participant FS as Filesystem AM->>AM: buildStorageSnapshot() AM->>SA: persistStorageWithConflictRecovery() loop up to 3 attempts SA->>FL: withStorageSaveFileLock(path, task) FL->>FS: open(lock, "wx") FS-->>FL: lock acquired (token + fingerprint) FL->>CAS: readStorageRevision(path) FS-->>CAS: currentRevision alt currentRevision != knownRevision CAS-->>SA: ECONFLICT SA->>SA: loadAccounts() → merge SA->>SA: sleep(20 * 2^attempt) else revisions match CAS->>FS: writeFile(WAL) + rename(tmp→path) CAS->>CAS: rememberKnownStorageRevision FL->>FS: unlink(lock) FL-->>SA: success SA-->>AM: done end endLast reviewed commit: 83dedbb