fix: harden oauth code exchange timeout handling#31
Conversation
Add timeout and abort boundary for authorization code exchange. Wire fetch timeout config into oauth exchange callsites. Add tests for network and timeout failure behavior. Co-authored-by: Codex <noreply@openai.com>
📝 Walkthroughwalkthroughadds configurable oauth token-exchange timeout pulled from plugin config, applies it to exchangeAuthorizationCode calls, adds abortable timeout-aware fetch paths, introduces resilient fetch-with-retries utility, and adds sync-read-with-retry plus sanitization for plugin config reads. (≤50 words) changes
sequence diagram(s)sequenceDiagram
autonumber
participant client as "client"
participant manager as "codex-manager\n(`lib/codex-manager.ts`)"
participant cfg as "config loader\n(`lib/config.ts`)"
participant auth as "oauth exchange\n(`lib/auth/auth.ts`)"
participant token as "oauth token endpoint"
client->>manager: initiate oauth flow / provide auth code
manager->>cfg: loadPluginConfig()
cfg-->>manager: plugin config (fetchTimeoutMs)
manager->>auth: exchangeAuthorizationCode(code, { timeoutMs })
auth->>auth: buildAbortContext(timeoutMs)
auth->>token: fetch token (with AbortSignal)
alt token responds
token-->>auth: token payload
auth-->>manager: success result
else token errors or aborts
token-->>auth: http / network / abort
auth-->>manager: failure result (categorized)
end
estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes suggested labels
notes for reviewer
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.ts`:
- Around line 463-470: Duplicate logic computing oauthFetchTimeoutMs
(loadPluginConfig() + getFetchTimeoutMs(...)) is used before calling
exchangeAuthorizationCode in multiple places; extract a small helper (e.g.,
resolveOauthFetchTimeout or getOauthFetchTimeoutMs) that returns the timeout by
calling loadPluginConfig() and getFetchTimeoutMs, then replace inline logic at
both call sites that invoke exchangeAuthorizationCode (and any other OAuth fetch
callers) to call the helper instead so both flows stay consistent with
lib/auth/auth.ts behavior.
- Around line 463-470: Add regression tests that assert the timeout is wired
through to exchangeAuthorizationCode: in test/index.test.ts add tests covering
the OAuth callback path that invokes exchangeAuthorizationCode (the code branch
that uses parsed.code, pkce.verifier and REDIRECT_URI) and verify it is called
with the options object containing timeoutMs returned from
getFetchTimeoutMs(loadPluginConfig()). Do the same in
test/codex-manager-cli.test.ts for the alternate OAuth flow (the other
occurrence of exchangeAuthorizationCode), stubbing
loadPluginConfig/getFetchTimeoutMs to a known value and asserting
exchangeAuthorizationCode was called with { timeoutMs: <value> }. Also add
concurrency and Windows-edge-case tests mirroring the patterns used in the
codex-manager-cli tests (windows-ebusy and token-refresh-race modes) to exercise
concurrent OAuth flows and ensure timeout wiring holds under race conditions.
In `@lib/auth/auth.ts`:
- Around line 149-153: Close the micro race by registering the abort listener
before checking the current aborted state: call
upstreamSignal.addEventListener("abort", onUpstreamAbort, { once: true }) first,
then immediately check if upstreamSignal.aborted and if so invoke
onUpstreamAbort (or remove the just-attached listener then call onUpstreamAbort)
so the callback cannot be missed; update the logic around upstreamSignal and the
onUpstreamAbort handler in lib/auth/auth.ts accordingly.
In `@lib/codex-manager.ts`:
- Around line 1255-1262: Add a vitest that exercises the full integration path
from runOAuthFlow through loadPluginConfig → getFetchTimeoutMs →
exchangeAuthorizationCode (cite the new test name in the test suite, e.g.,
"oauth timeout integration test") to assert the timeout is propagated; and
update loadPluginConfig to use the same retry-on-retryable-fs-codes logic used
elsewhere (use the project’s RETRYABLE_FS_CODES or the existing fs retry
helper/wrapper used in storage.ts/config.ts) when calling readFileSync so
EBUSY/EPERM on Windows is retried a few times before failing; keep
resolveNumberSetting behavior (1000ms minimum) as-is and ensure the new
integration test covers the Windows retry behavior by simulating transient read
errors.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
index.tslib/auth/auth.tslib/codex-manager.tstest/auth-logging.test.tstest/auth.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/auth.test.tstest/auth-logging.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager.tslib/auth/auth.ts
🧬 Code graph analysis (4)
test/auth.test.ts (1)
lib/auth/auth.ts (2)
exchangeAuthorizationCode(178-237)REDIRECT_URI(12-12)
lib/codex-manager.ts (2)
lib/config.ts (2)
loadPluginConfig(184-247)getFetchTimeoutMs(775-782)lib/auth/auth.ts (2)
exchangeAuthorizationCode(178-237)REDIRECT_URI(12-12)
lib/auth/auth.ts (3)
lib/logger.ts (2)
error(389-393)logError(348-352)lib/schemas.ts (1)
safeParseOAuthTokenResponse(310-316)lib/utils.ts (1)
isAbortError(20-24)
test/auth-logging.test.ts (2)
lib/auth/auth.ts (2)
exchangeAuthorizationCode(178-237)REDIRECT_URI(12-12)lib/logger.ts (1)
logError(348-352)
🔇 Additional comments (6)
lib/auth/auth.ts (3)
14-14: default timeout looks reasonable.60 seconds aligns with typical network timeout expectations for oauth exchanges. matches the fallback in
lib/codex-manager.ts:getFetchTimeoutMs.
217-225: success path preserved - lgtm.refresh token trimming and expiry calculation look correct.
multiAccount: trueflag aligns with the multi-auth plugin context.
228-233: both timeout and user abort returnreason: "unknown"- callers parsing message string for timeout logic is expected.the timeout path is set up in
buildExchangeAbortContext(lib/auth/auth.ts:134-162) with a 60 second default. when the timeout fires, it callscontroller.abort()with message "OAuth token exchange timed out after Xms", which gets caught by the sameisAbortErrorhandler at line 228 as upstream signal aborts. if timeout-specific retry logic is needed later, consider adding a dedicated reason enum value.test coverage confirmed: test/auth.test.ts:546-561 exercises generic abort scenario (expects
reason: "unknown"), and test/auth-logging.test.ts:67-102 exercises timeout scenario (verifieslogErrorcalled with timeout message). logging is safe—onlyerr?.messagegets passed to metadata, no tokens or emails leak.lib/codex-manager.ts (1)
26-26: import addition for config helpers - lgtm.imports
getFetchTimeoutMsandloadPluginConfigto wire timeout into oauth exchange.test/auth.test.ts (1)
350-366: good deterministic coverage for network + timeout failure paths.nice addition.
test/auth.test.ts:350-406now validates both thrown network errors and abort-on-timeout behavior againstlib/auth/auth.ts:177-236, and the fake-timer usage keeps it deterministic.Also applies to: 368-406
test/auth-logging.test.ts (1)
8-8: solid timeout logging regression test.
test/auth-logging.test.ts:67-106cleanly verifies abort timeout metadata, and usingREDIRECT_URIfromlib/auth/auth.ts:11attest/auth-logging.test.ts:8avoids hardcoded drift.Also applies to: 67-106
- dedupe OAuth timeout resolution in plugin flows\n- close upstream abort-listener race in auth exchange\n- add transient config-read retry handling for EBUSY/EPERM\n- add timeout propagation and retry regression tests\n\nCo-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/auth/auth.ts`:
- Around line 134-177: Add deterministic vitest tests for
buildExchangeAbortContext in test/auth.test.ts covering three regression cases:
(1) pre-aborted upstream: create an AbortController, call abort() before passing
its signal to buildExchangeAbortContext(options) and assert the returned signal
is already aborted and cleanup clears timers without scheduling a timeout (use
vi.useFakeTimers/advanceTimersByTime to ensure no timeout fires); (2) mid-flight
upstream abort mapping: create upstream AbortController, call
buildExchangeAbortContext({ timeoutMs: small, signal: upstream.signal }), then
call upstream.abort(new Error("reason")) and assert the returned signal becomes
aborted with the same Error reason and that cleanup clears the timeout; (3)
listener/timer leak detection across repeated calls: spy on
upstream.signal.addEventListener and removeEventListener via vi.spyOn (or wrap
signal methods) then call buildExchangeAbortContext twice with the same upstream
signal, call cleanup for each returned context, and assert addEventListener was
called per build and removeEventListener was called for each cleanup and
advancing timers after cleanup does not trigger extra aborts — use
vi.useFakeTimers to make timers deterministic and ensure all assertions are
synchronous/deterministic.
- Around line 199-203: The token-exchange error handling currently logs and
returns the raw token-endpoint body (variable res/text) via logError and the
failure payload; stop logging or returning the raw text to avoid leaking
credential fields. Update the code in the token-exchange handler (the branch
that checks if (!res.ok)) to redact or omit sensitive response content: log only
non-sensitive metadata (e.g., res.status and a fixed message or
truncated/redacted snippet), and return a generic failure object (type:
"failed", reason: "http_error", statusCode: res.status) without including the
full text; ensure you sanitize any message content before logging and adjust
tests (vitest) that asserted on the previous raw text behavior.
In `@lib/config.ts`:
- Around line 282-294: The savePluginConfig flow is vulnerable to transient
Windows EBUSY/EPERM read failures because it can receive null from the current
read and then write a partial patch; update savePluginConfig to use the existing
readFileSyncWithRetry helper (and its retry constants
CONFIG_READ_MAX_ATTEMPTS/CONFIG_READ_RETRY_BASE_DELAY_MS and isRetryableFsError)
instead of the nullable read path so reads retry on transient errors before
merging, and ensure the merge logic preserves existing keys (load full config,
apply patch, then atomically write). Add a vitest regression that simulates
transient EBUSY/EPERM during reads to assert no keys are lost after
savePluginConfig completes, and verify logging added around retries does not
include secrets/tokens/emails.
In `@test/index.test.ts`:
- Around line 533-565: The test for the browser OAuth path should also assert
that getFetchTimeoutMs was called with the loaded plugin config: after mocking
configModule.loadPluginConfig to return pluginConfig and before invoking the
browser-based authorize flow (plugin.auth.methods[0].authorize), add an
expectation that configModule.getFetchTimeoutMs was called with the pluginConfig
object so the browser path’s timeout wiring is verified (refer to
getFetchTimeoutMs, loadPluginConfig, and the authorize method on
plugin.auth.methods[0]).
- Around line 567-597: The current concurrency test uses identical mocked values
so it can't detect cross-call state/verifier/code mixups; update the mocks to
return distinct per-call values and assert they are passed through per-call:
make vi.mocked(authModule.createAuthorizationFlow) return a different {
pkce.verifier, pkce.challenge, state, url } for each invocation (e.g., via
mockResolvedValueOnce / a call-indexed factory), make
vi.mocked(serverModule.startLocalOAuthServer) provide per-call waitForCode
results with different codes, call the plugin.auth.methods[0].authorize()
concurrently as before, and then assert
vi.mocked(authModule.exchangeAuthorizationCode) was called three times and that
for each call the passed arguments include the matching state, verifier (from
pkce) and code as objects unique to that invocation (reference
createAuthorizationFlow, startLocalOAuthServer.waitForCode,
plugin.auth.methods[0].authorize, and exchangeAuthorizationCode to locate and
update the mocks and assertions).
In `@test/plugin-config.test.ts`:
- Around line 203-220: Add a second deterministic regression for Windows-style
EPERM transient lock errors by extending the existing test for retries: either
duplicate the 'retries transient config read lock errors and succeeds' test or
modify it to simulate a transient error with code 'EPERM' in addition to 'EBUSY'
(use the same pattern as the current transientReadError and
mockReadFileSync.mockImplementationOnce to throw an Error object with { code:
'EPERM' }), then assert loadPluginConfig() still succeeds (check
config.codexMode and mockReadFileSync call count) and that no failed-load
warnings were logged; reference the existing test, loadPluginConfig,
mockReadFileSync, mockExistsSync, and logger.logWarn when adding the EPERM
scenario.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
index.tslib/auth/auth.tslib/config.tstest/codex-manager-cli.test.tstest/index.test.tstest/plugin-config.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/plugin-config.test.tstest/codex-manager-cli.test.tstest/index.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/auth/auth.tslib/config.ts
🔇 Additional comments (5)
lib/config.ts (1)
198-198: good retry wiring on the load path
lib/config.ts:198(line [198]) correctly routes config load through retry-aware io and improves resilience to transientebusy/eperm.index.ts (1)
425-427: timeout propagation is consistently wired in both oauth exchange paths
index.ts:425,index.ts:467, andindex.ts:518(line [425], line [467], line [518]) now use one resolver and pass the sametimeoutMscontract toexchangeAuthorizationCode, which removes drift risk across the two flows.Also applies to: 467-473, 518-524
lib/auth/auth.ts (1)
149-154: good abort-listener ordering fix
lib/auth/auth.ts:149-153(line [149]-line [153]) attaches the listener before checkingaborted, which closes the missed-abort race window.test/codex-manager-cli.test.ts (1)
837-839: solid deterministic regression for timeout propagation
test/codex-manager-cli.test.ts:837andtest/codex-manager-cli.test.ts:879(line [837], line [879]) now pinfetchTimeoutMsand assert it is passed intoexchangeAuthorizationCode, which is exactly the boundary this change needs to lock down.Also applies to: 879-884
test/index.test.ts (1)
93-114: good timeout test wiring and mock control.
test/index.test.ts:93,test/index.test.ts:114, andtest/index.test.ts:508look solid. usingvi.fnhere makes per-test timeout overrides safe, and the manual flow assertion correctly verifiesexchangeAuthorizationCode(..., { timeoutMs })plusgetFetchTimeoutMs(pluginConfig).Also applies to: 508-531
- sanitize OAuth token error logging and payloads - add deterministic upstream-abort and cleanup regressions - harden config-save merge path against transient EBUSY/EPERM reads - strengthen timeout wiring and EPERM regression tests Co-authored-by: Codex <noreply@openai.com>
Prevent sensitive OAuth payload leakage during refresh HTTP failures by logging status and body length only, and return a generic safe error message when response text exists. Adds regression tests for auth behavior and sanitized logging. Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/index.test.ts (1)
615-622:⚠️ Potential issue | 🟠 Majorconcurrency assertion still misses code/verifier pairing bugs.
in
test/index.test.ts:617(Line 617) throughtest/index.test.ts:622(Line 622), separate set checks can still pass if concurrent calls cross-wire codes and verifiers. assert pair tuples so the regression actually catches request-local mixups.proposed fix
expect(vi.mocked(authModule.exchangeAuthorizationCode)).toHaveBeenCalledTimes(3); const calls = vi.mocked(authModule.exchangeAuthorizationCode).mock.calls; - expect(calls.map((call) => call[0])).toEqual( - expect.arrayContaining(["code-1", "code-2", "code-3"]), - ); - expect(calls.map((call) => call[1])).toEqual( - expect.arrayContaining(["verifier-1", "verifier-2", "verifier-3"]), - ); + const codeVerifierPairs = calls + .map((call) => `${String(call[0])}|${String(call[1])}`) + .sort(); + expect(codeVerifierPairs).toEqual( + ["code-1|verifier-1", "code-2|verifier-2", "code-3|verifier-3"].sort(), + ); for (const call of calls) { expect(call[3]).toEqual({ timeoutMs: expectedTimeoutMs }); }As per coding guidelines
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index.test.ts` around lines 615 - 622, The test currently checks codes and verifiers separately which can miss cross-wiring; update the assertions around authModule.exchangeAuthorizationCode to assert paired tuples instead: capture vi.mocked(authModule.exchangeAuthorizationCode).mock.calls and assert that the array of [call[0], call[1]] pairs contains the expected tuples (["code-1","verifier-1"], ["code-2","verifier-2"], ["code-3","verifier-3"]) and still assert the call count is 3 to keep determinism; use expect.arrayContaining on the pairs array so the test fails if any code is matched with the wrong verifier.lib/config.ts (1)
356-367:⚠️ Potential issue | 🟠 Majordo not downgrade exhausted retryable read errors to
nullduring save merges.in
lib/config.ts:356(Line 356) throughlib/config.ts:367(Line 367), persistentebusy/epermafter retry is still coerced tonull.savePluginConfigthen merges against{}atlib/config.ts:415(Line 415) andlib/config.ts:428(Line 428), which can overwrite and drop existing keys. fail closed on exhausted retryable fs errors, and add a vitest regression that keeps the original file intact when all read attempts fail.proposed fix
function readConfigRecordFromPath(configPath: string): Record<string, unknown> | null { if (!existsSync(configPath)) return null; try { const fileContent = readFileSyncWithRetry(configPath, "utf-8"); const normalizedFileContent = stripUtf8Bom(fileContent); const parsed = JSON.parse(normalizedFileContent) as unknown; return isRecord(parsed) ? parsed : null; } catch (error) { + if (isRetryableFsError(error)) { + throw error; + } logConfigWarnOnce( `Failed to read config from ${configPath}: ${ error instanceof Error ? error.message : String(error) }`, ); return null; } }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/config.ts` around lines 356 - 367, The current catch in readFileSyncWithRetry path (surrounding readFileSyncWithRetry / stripUtf8Bom / JSON.parse) swallows exhausted retryable FS errors and returns null, which causes savePluginConfig to merge against {} and potentially drop keys; change the catch to detect retry-exhaustion (EBUSY/EPERM or a specific RetryExhaustedError thrown by readFileSyncWithRetry) and rethrow that error instead of returning null so callers like savePluginConfig will abort instead of merging; keep logConfigWarnOnce for non-retryable errors but ensure retry-exhausted errors propagate, update savePluginConfig to handle propagated errors (fail closed) and add a Vitest regression that simulates readFileSyncWithRetry exhausting with EBUSY/EPERM and asserts the original file contents are unchanged.
🤖 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/auth/auth.ts`:
- Around line 232-237: The catch block in the code->token flow currently relies
on isAbortError(err) and misclassifies upstream aborts; change the logic to
first check abortContext.signal.aborted (or the relevant abort signal used by
the code->token flow) and treat any such case as the aborted/unknown path before
calling isAbortError(err), updating the return payload and processLogger call in
the catch handler (look for the catch around code->token and the isAbortError
call). Also update the "shared-upstream-abort" test in test/auth.test.ts to
assert result.reason === 'unknown' when a plain Error is forwarded from an
upstream abort so the regression is caught.
---
Duplicate comments:
In `@lib/config.ts`:
- Around line 356-367: The current catch in readFileSyncWithRetry path
(surrounding readFileSyncWithRetry / stripUtf8Bom / JSON.parse) swallows
exhausted retryable FS errors and returns null, which causes savePluginConfig to
merge against {} and potentially drop keys; change the catch to detect
retry-exhaustion (EBUSY/EPERM or a specific RetryExhaustedError thrown by
readFileSyncWithRetry) and rethrow that error instead of returning null so
callers like savePluginConfig will abort instead of merging; keep
logConfigWarnOnce for non-retryable errors but ensure retry-exhausted errors
propagate, update savePluginConfig to handle propagated errors (fail closed) and
add a Vitest regression that simulates readFileSyncWithRetry exhausting with
EBUSY/EPERM and asserts the original file contents are unchanged.
In `@test/index.test.ts`:
- Around line 615-622: The test currently checks codes and verifiers separately
which can miss cross-wiring; update the assertions around
authModule.exchangeAuthorizationCode to assert paired tuples instead: capture
vi.mocked(authModule.exchangeAuthorizationCode).mock.calls and assert that the
array of [call[0], call[1]] pairs contains the expected tuples
(["code-1","verifier-1"], ["code-2","verifier-2"], ["code-3","verifier-3"]) and
still assert the call count is 3 to keep determinism; use expect.arrayContaining
on the pairs array so the test fails if any code is matched with the wrong
verifier.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
lib/auth/auth.tslib/config.tstest/auth-logging.test.tstest/auth.test.tstest/config-save.test.tstest/index.test.tstest/plugin-config.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/config-save.test.tstest/plugin-config.test.tstest/index.test.tstest/auth.test.tstest/auth-logging.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/auth/auth.tslib/config.ts
🧬 Code graph analysis (4)
test/config-save.test.ts (1)
lib/config.ts (1)
savePluginConfig(408-433)
test/plugin-config.test.ts (1)
lib/config.ts (1)
loadPluginConfig(186-249)
test/auth.test.ts (1)
lib/auth/auth.ts (2)
exchangeAuthorizationCode(179-243)REDIRECT_URI(12-12)
test/auth-logging.test.ts (2)
lib/auth/auth.ts (3)
exchangeAuthorizationCode(179-243)REDIRECT_URI(12-12)refreshAccessToken(276-333)lib/logger.ts (1)
logError(348-352)
🔇 Additional comments (7)
test/config-save.test.ts (1)
96-160: nice deterministic windows-lock merge regression.in
test/config-save.test.ts:96(Line 96) throughtest/config-save.test.ts:160(Line 160), you simulateebusytheneperm, verify key preservation after merge, and assert no noisy failed-read warnings. this is the right coverage for the merge-save windows lock path.test/plugin-config.test.ts (1)
203-241: good ebusy/eperm retry coverage for config loads.
test/plugin-config.test.ts:203(Line 203) throughtest/plugin-config.test.ts:241(Line 241) now covers both transient lock codes with deterministic retries and explicit no-warning assertions. this closes the windows read-lock regression gap cleanly.lib/config.ts (1)
37-38: bounded read retry wiring looks solid for transient windows locks.
lib/config.ts:37(Line 37),lib/config.ts:198(Line 198), andlib/config.ts:282(Line 282) introduce a tight, bounded retry path and apply it consistently in load/save-read paths. this is a good hardening step.Also applies to: 198-198, 278-294
lib/auth/auth.ts (1)
199-207: http failure logging is now token-safe.
lib/auth/auth.ts:201(Line 201) andlib/auth/auth.ts:294(Line 294) now log only status/body length and return generic messages, which avoids leaking token-bearing payloads.Also applies to: 294-300
test/index.test.ts (1)
508-566: timeout propagation coverage is well wired for both oauth paths.
test/index.test.ts:508(Line 508) throughtest/index.test.ts:566(Line 566) verifiesloadPluginConfig -> getFetchTimeoutMs -> exchangeAuthorizationCodefor both manual and browser flows. this is good end-to-end wiring coverage.test/auth-logging.test.ts (1)
67-157: good logging regressions for timeout and sanitized http failures.
test/auth-logging.test.ts:67(Line 67) throughtest/auth-logging.test.ts:157(Line 157) gives deterministic timeout-abort coverage and confirms only sanitized metadata is logged for token exchange/refresh http failures.test/auth.test.ts (1)
343-343: abort/timeout regressions are strong and deterministic.
test/auth.test.ts:350(Line 350) throughtest/auth.test.ts:556(Line 556) adds solid vitest coverage for timeout aborts, pre-aborted/mid-flight upstream signals, and listener/timer cleanup across concurrent exchanges. the expectation updates attest/auth.test.ts:343(Line 343) andtest/auth.test.ts:654(Line 654) also correctly track the new sanitized error messaging.Also applies to: 350-556, 654-656
Add schema-aware config sanitization, introduce a resilient fetch primitive with timeout/retry instrumentation, apply it to remote prompt/update fetches, and add shutdown timeout boundaries with concurrent cleanup deduplication. Includes new regression tests for network retries, config sanitization, and shutdown timeout behavior. Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/prompts/codex.ts (1)
152-204:⚠️ Potential issue | 🟡 Minoradd vitest regression tests for codex github 429 retry and refresh dedup paths.
lib/prompts/codex.ts:152-204,360-384, and432-461wire upfetchWithTimeoutAndRetrywithGITHUB_RETRYABLE_STATUSES = [429]and background refresh dedup viarefreshPromises, buttest/codex-prompts.test.tshas no regressions testing:
- 429 recovery on api fetch (line 152)
- 429 recovery on prompt download (line 360)
- concurrent refresh dedup preventing duplicate in-flight requests (line 432)
- background refresh error handling (line 437)
network.test.ts validates 503 retry but no 429 codex-specific scenario. need explicit test coverage for these paths per coding guidelines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/prompts/codex.ts` around lines 152 - 204, Add Vitest regression tests in test/codex-prompts.test.ts that exercise the codex retry and refresh-dedup paths: stub/mock fetch so that GITHUB_API_RELEASES and GITHUB_HTML_RELEASES calls (the code that uses fetchWithTimeoutAndRetry) return a 429 then a 200 to verify retries are attempted and the code recovers; add a test for the prompt download path (the block around the prompt fetch that also uses fetchWithTimeoutAndRetry) to simulate 429 -> 200 and assert successful download after retries; add a concurrency test that calls the background refresh function twice concurrently and asserts only one network request is made and both callers receive the same resolved result (verifying refreshPromises dedup behavior); and add a test where the background refresh request fails to ensure the error is caught/logged and does not leave refreshPromises in a bad state (verify subsequent refresh can run). Use the same symbols from the diff (GITHUB_API_RELEASES, GITHUB_HTML_RELEASES, fetchWithTimeoutAndRetry, refreshPromises) to locate the code under test.
🤖 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/auto-update-checker.ts`:
- Around line 177-181: The retry configuration in the auto-update fetch uses
zero backoff (baseDelayMs: 0, jitterMs: 0) which will immediately retry on
retryable statuses (UPDATE_FETCH_RETRYABLE_STATUSES) and can hammer the registry
for 429/5xx; change the retry strategy in lib/auto-update-checker.ts by giving
baseDelayMs a sensible non-zero base (e.g., exponential backoff seed) and add
jitterMs > 0 and/or exponential growth so retries from UPDATE_FETCH_RETRIES are
spaced, and add special-case backoff escalation when the response status is 429
or EBUSY to increase delay more aggressively; update or add vitest specs that
exercise UPDATE_FETCH_RETRIES/UPDATE_FETCH_RETRYABLE_STATUSES for 429/5xx and
EBUSY scenarios to assert spacing between retries and that the new queue
handling respects the backoff escalation.
- Around line 171-192: Add a regression test that verifies
fetchWithTimeoutAndRetry actually retries on retryable HTTP statuses: mock
global fetch in test/auto-update-checker.test.ts to return two 500 responses
followed by a 200 success (or similar sequence), call the code path that
triggers fetchWithTimeoutAndRetry (the npm registry call that uses
NPM_REGISTRY_URL and UPDATE_FETCH_RETRYABLE_STATUSES in
lib/auto-update-checker.ts), assert that fetch was invoked multiple times and
that the final result is the successful response, and also assert retry behavior
(e.g., attempts > 1 or that the onRetry path was exercised) to ensure
retry-on-status boundary is covered.
In `@lib/config.ts`:
- Around line 314-320: The current fallback in the NUMERIC_PLUGIN_CONFIG_KEYS
handling (the block that checks NUMERIC_PLUGIN_CONFIG_KEYS.has(key) && typeof
value === "number" && Number.isFinite(value) and assigns sanitized[key] = value)
lets any finite number bypass schema bounds; remove this fallback so numeric
plugin keys are not accepted unless they passed schema parsing earlier—i.e.,
delete that conditional block and ensure sanitized only receives values that
have already been validated by the schema, leaving invalid numeric fields
undefined/omitted; update/verify related tests that assert out-of-range
timeout/quota values are sanitized away.
In `@lib/network.ts`:
- Around line 112-114: The retry backoff currently calls sleep(delayMs)
unconditionally (references to delayMs and sleep in lib/network.ts) which
ignores caller cancellation; update the retry logic (where sleep is invoked
around delayMs at the two places) to use a cancellable sleep that listens to the
request's AbortSignal (e.g., pass the existing signal/abortSignal into sleep or
replace with a sleepUntilAbort that throws if signal.aborted) and bail out
immediately when aborted so the retry loop stops; also add a vitest regression
test "abort during backoff" in test/network.test.ts that starts a request,
triggers abort during the backoff window, and asserts no further retries/run
after abort.
- Around line 3-10: The RetryAttemptInfo interface and its usage are currently
forwarding raw error messages into the error field (e.g., where
RetryAttemptInfo.error is set), which can leak sensitive data; update the code
that constructs RetryAttemptInfo (references: RetryAttemptInfo and the place
where attempt/maxAttempts/delayMs/reason/status/error are populated) to stop
copying raw error.message — instead either omit the error text, store a
non-sensitive enum/code (e.g., "network_error"/"timeout"), or store a
sanitized/truncated token-less summary (no URLs, query params, tokens, or
emails); ensure callers can still detect error type via the reason/status fields
but cannot log raw error.message.
In `@lib/prompts/host-codex-prompt.ts`:
- Around line 288-313: Add a Vitest that reproduces the combined scenario: mock
fetch (or fetchWithTimeoutAndRetry behavior) to return a 429 on first attempt
and a 200 on retry, then call Promise.all([getHostCodexPrompt(),
getHostCodexPrompt()]) to exercise concurrent refresh deduplication via the
module-level refreshPromise in host-codex-prompt.ts; assert that the retry path
in fetchWithTimeoutAndRetry runs (e.g., onRetry invoked or attempts > 1) and
that only a single underlying fetch/fetchWithTimeoutAndRetry invocation
completes the network work (verify your fetch/mock call count is 1 or that
refreshPromise was reused), and finally verify both promises resolve to the same
prompt result to prove deduplication during retry.
In `@lib/shutdown.ts`:
- Around line 50-57: The Promise.race in runCleanup assigns a timeout but never
clears its timer handle, leaving a pending timer (and keeping the event loop
alive) when runner resolves first; modify runCleanup/cleanupInFlight to capture
the setTimeout return value into a local timeoutId and clearTimeout(timeoutId)
in the finally block (or immediately when runner wins) just like
request/response-handler.ts and stream-failover.ts do, ensuring cleanupInFlight
is still nulled; also add a regression test in test/shutdown.test.ts that runs
runCleanup with fast-resolving cleanup functions (no fake timers) and asserts
the process can exit without a pending timer.
In `@test/network.test.ts`:
- Around line 20-105: Add deterministic regression tests for
fetchWithTimeoutAndRetry covering timeout-triggered retry, mid-flight abort,
abort-during-backoff, and listener/timer cleanup: write tests that (1) mock
fetch to delay beyond timeout so the timeout path at lib/network.ts:95-98
triggers a retry and assert attempts, fetch calls, and onRetry(reason:
"timeout"); (2) start a fetch and then call controller.abort() mid-flight to
exercise the mid-flight abort path and assert the promise rejects with
AbortError and no retries occur; (3) simulate abort-during-backoff by forcing
the first attempt to fail, advance timers only enough to schedule backoff, then
abort before the next fetch and assert no additional fetch calls and promise
rejects as aborted; and (4) after each scenario run vi.runAllTimersAsync() and
assert no further fetch calls occur to ensure listeners/timers from
lib/network.ts:142-143 are cleaned up; use the existing helpers like
AbortController, onRetry mock, and expect(result.attempts) / fetchMock call
counts to validate behavior.
In `@test/plugin-config.test.ts`:
- Around line 643-663: Add a deterministic vitest regression test to cover
numeric boundary sanitization by extending the existing spec around
loadPluginConfig: use mockExistsSync.mockReturnValue(true) and
mockReadFileSync.mockReturnValue(...) with out-of-range numeric values (e.g.
fetchTimeoutMs: 10 and preemptiveQuotaRemainingPercent5h: 500) and assert that
loadPluginConfig() returns the safe/defaulted values (e.g. fetchTimeoutMs falls
back to 60000 and preemptiveQuotaRemainingPercent5h falls back to 5); keep the
test synchronous and deterministic (no real secrets, no async races), reuse the
same test helpers (mockExistsSync, mockReadFileSync) and the existing test name
pattern so it exercises the same sanitization paths in loadPluginConfig.
In `@test/shutdown.test.ts`:
- Around line 83-107: The timeout created inside the Promise.race in runCleanup
is never cleared when a cleanup resolves quickly, leaving a pending timer;
modify runCleanup (and any helper that races cleanup promises) to capture the
setTimeout handle and call clearTimeout(handle) when the cleanup promise settles
(both resolve and reject), so the timer is removed; reference the runCleanup
function and the Promise.race wrapper in lib/shutdown.ts and ensure the timeout
handle is cleared in the finally/settle path so vitest no longer detects pending
timers (also add a deterministic regression test in shutdown.test.ts that
registers a fast-resolving cleanup via registerCleanup, calls runCleanup,
advances timers, and asserts there are no pending timers).
---
Outside diff comments:
In `@lib/prompts/codex.ts`:
- Around line 152-204: Add Vitest regression tests in test/codex-prompts.test.ts
that exercise the codex retry and refresh-dedup paths: stub/mock fetch so that
GITHUB_API_RELEASES and GITHUB_HTML_RELEASES calls (the code that uses
fetchWithTimeoutAndRetry) return a 429 then a 200 to verify retries are
attempted and the code recovers; add a test for the prompt download path (the
block around the prompt fetch that also uses fetchWithTimeoutAndRetry) to
simulate 429 -> 200 and assert successful download after retries; add a
concurrency test that calls the background refresh function twice concurrently
and asserts only one network request is made and both callers receive the same
resolved result (verifying refreshPromises dedup behavior); and add a test where
the background refresh request fails to ensure the error is caught/logged and
does not leave refreshPromises in a bad state (verify subsequent refresh can
run). Use the same symbols from the diff (GITHUB_API_RELEASES,
GITHUB_HTML_RELEASES, fetchWithTimeoutAndRetry, refreshPromises) to locate the
code under test.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (9)
lib/auto-update-checker.tslib/config.tslib/network.tslib/prompts/codex.tslib/prompts/host-codex-prompt.tslib/shutdown.tstest/network.test.tstest/plugin-config.test.tstest/shutdown.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/shutdown.test.tstest/network.test.tstest/plugin-config.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/prompts/host-codex-prompt.tslib/auto-update-checker.tslib/prompts/codex.tslib/network.tslib/config.tslib/shutdown.ts
🧬 Code graph analysis (7)
test/shutdown.test.ts (1)
lib/shutdown.ts (2)
registerCleanup(18-21)runCleanup(30-60)
lib/prompts/host-codex-prompt.ts (2)
lib/network.ts (1)
fetchWithTimeoutAndRetry(79-150)lib/logger.ts (1)
logDebug(325-331)
lib/auto-update-checker.ts (1)
lib/network.ts (1)
fetchWithTimeoutAndRetry(79-150)
lib/prompts/codex.ts (2)
lib/network.ts (1)
fetchWithTimeoutAndRetry(79-150)lib/logger.ts (1)
logDebug(325-331)
test/network.test.ts (1)
lib/network.ts (1)
fetchWithTimeoutAndRetry(79-150)
test/plugin-config.test.ts (1)
lib/config.ts (1)
loadPluginConfig(212-276)
lib/config.ts (2)
lib/schemas.ts (1)
PluginConfigSchema(13-60)lib/utils.ts (1)
isRecord(11-13)
🔇 Additional comments (5)
test/plugin-config.test.ts (1)
203-241: good windows lock retry regressions.
test/plugin-config.test.ts:203-241(Line [203]-Line [241]) is deterministic and correctly validates bothebusyandepermtransient read failures with concrete assertions on retry count and warning behavior.lib/config.ts (1)
224-225: retry wiring for windows read locks looks correct.
lib/config.ts:224-225andlib/config.ts:408-409(Line [224]-Line [225], Line [408]-Line [409]) correctly route both load paths throughreadFileSyncWithRetry, and the new vitest coverage intest/plugin-config.test.ts:203-241validatesebusy/epermbehavior.Also applies to: 408-409
lib/shutdown.ts (2)
9-16: timeout bounds parsing is clean and safe.
lib/shutdown.ts:9-16(line 9-16) clamps invalid values and enforces sane min/max boundaries. this matches the configured-timeout coverage intest/shutdown.test.ts:83-107.
65-70: signal dedupe guard is a good concurrency hardening.
lib/shutdown.ts:65-70(line 65-70) correctly prevents duplicate cleanup on repeated signal delivery.test/shutdown.test.ts (1)
50-57: good concurrency regression for in-flight dedupe.
test/shutdown.test.ts:50-57(line 50-57) directly validates that concurrentrunCleanup()calls execute cleanup only once.
Address unresolved hardening review items across auth abort classification, config sanitization, network retry cancellation, update-check backoff, prompt refresh dedupe coverage, and shutdown timer cleanup. Adds deterministic regression tests for each scenario and aligns config-save expectations with schema-bound validation. Co-authored-by: Codex <noreply@openai.com>
1 participant
Summary
Risk
Verification
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
added comprehensive timeout and abort handling to oauth authorization code exchange boundary. timeout configuration flows from
pluginConfig.fetchTimeoutMsthrough plugin and cli oauth callsites into newexchangeAuthorizationCodeoptions parameter. network and abort failures now return typedTokenResultinstead of throwing.introduced new
lib/network.tswithfetchWithTimeoutAndRetryutility providing timeout, exponential backoff with jitter, and configurable retry on specific http statuses. properly chains abort signals and never retries caller-initiated aborts. migrated auto-update checker and prompt fetch callsites to resilient fetch.hardened config loading with
readFileSyncWithRetryusingAtomics.waitfor windows EBUSY/EPERM retry, plus newsanitizePluginConfigRecordvalidating keys against zod schema to prevent undefined values from polluting merged config.enhanced shutdown with race-safe cleanup deduplication, configurable timeout boundary via
CODEX_AUTH_SHUTDOWN_TIMEOUT_MS, and signal-handled flag preventing double-execution on concurrent signals.logging redaction: oauth http failures log only sanitized metadata (
status,bodyLength), not response bodies that could contain tokens. structured logging prevents leakage.test coverage: added 7 auth timeout/abort tests including listener cleanup verification, 3 logging sanitization tests, 8 resilient fetch tests, 4 config timeout wiring tests, shutdown race/timeout tests, and config read retry tests. all regression tests validate windows filesystem resilience and token redaction.
Confidence Score: 5/5
Atomics.wait, (4) logging sanitization prevents token leakage by logging only metadata (status, bodyLength), (5) shutdown cleanup is race-safe with deduplication and timeout boundaries, (6) response bodies are cancelled before retry preventing memory leaks, (7) comprehensive test coverage validates all new paths including listener cleanup, (8) follows all project conventions for windows filesystem safety and error handlingImportant Files Changed
TokenResult, includes proper cleanup in finally block, sanitizes http error logging to prevent token leakagereadFileSyncWithRetryusingAtomics.waitfor windows EBUSY/EPERM resilience, addedsanitizePluginConfigRecordto validate keys against zod schema before merge, prevents undefined values from polluting configfetchTimeoutMsinto both manual and browser oauth flows via newresolveOAuthFetchTimeoutMshelperCODEX_AUTH_SHUTDOWN_TIMEOUT_MSenv var, prevents double-execution on concurrent signalsFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[OAuth Flow Starts] --> B[Load Config] B --> C[Get fetchTimeoutMs] C --> D[Create AbortController] D --> E[Set Timeout Timer] E --> F[Bind Upstream Signal] F --> G[Execute Fetch Request] G --> H{Request Outcome} H -->|HTTP 200| I[Parse Token Response] I --> J[Validate Schema] J --> K{Valid?} K -->|Yes| L[Cleanup Timers] L --> M[Return Success TokenResult] K -->|No| N[Log Metadata Only] N --> O[Cleanup Timers] O --> P[Return Failed TokenResult] H -->|HTTP Error| Q[Read Response Body] Q --> R[Log Status and Length] R --> S[Cleanup Timers] S --> T[Return Failed with http_error] H -->|Network Error| U[Catch Exception] U --> V[Log Error Message] V --> W[Cleanup Timers] W --> X[Return Failed with network_error] H -->|Timeout| Y[Timer Fires] Y --> Z[Controller Aborts] Z --> AA[Log Abort Info] AA --> AB[Cleanup Timers] AB --> AC[Return Failed with unknown] H -->|Caller Abort| AD[Upstream Signal Fires] AD --> AE[Propagate to Controller] AE --> AF[Cleanup Timers] AF --> AG[Return Failed with unknown]Last reviewed commit: 2287675