feat: inject codex cli account selection during live rotation#131
feat: inject codex cli account selection during live rotation#131
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
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:
📝 Walkthroughwalkthroughsignature-aware, serialized codex cli selection sync added and gated by a new direct-injection flag; experimental settings and defaults updated (direct-injection + rotation quota); account manager sync API now returns boolean success. see changes
sequence diagramsequenceDiagram
participant req as requests
participant plugin as index.ts
participant queue as sync queue (promise chain)
participant acctmgr as lib/accounts.ts
participant cli as codex cli writer
req->>plugin: incoming request / event
plugin->>plugin: buildCodexCliSelectionSignature(account)
alt direct injection disabled
plugin-->>req: skip sync
else direct injection enabled
plugin->>queue: queue sync task (serialized)
end
activate queue
queue->>acctmgr: syncCodexCliActiveSelectionForIndex(index)
acctmgr->>acctmgr: validate index and load account
acctmgr->>cli: setCodexCliActiveSelection(selection)
cli-->>acctmgr: Promise<boolean> (success/fail)
acctmgr-->>queue: resolve boolean
queue->>plugin: update cached signature/index on success
deactivate queue
estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes concerns on this diff
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/accounts.ts (1)
392-404:⚠️ Potential issue | 🟠 Majorthis new boolean result is still dropped.
syncCodexCliActiveSelectionForIndex()now reports whether the cli write succeeded, but line 388 inlib/accounts.ts:388still fires it and forgets the promise. underebusy/epermwrite contention, the runtime can switch accounts while codex cli stays on the old selection. lines 162-193 intest/accounts-load-from-disk.test.ts:162-193only cover thetruepath, so that desync stays invisible.make callers await the serialized sync path and handle
falseexplicitly instead of exposing a result that can be ignored.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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/accounts.ts` around lines 392 - 404, The new boolean return from syncCodexCliActiveSelectionForIndex indicates success but callers still fire-and-forget it; update call sites that invoke syncCodexCliActiveSelectionForIndex (the code that currently triggers the sync after account switch) to await the Promise and explicitly handle a false result by retrying or aborting the account-switch transition (i.e., serialize the sync path so the account never moves on while the CLI write can fail), and add/adjust vitest cases to cover the false/ebusy/eperm and 429 scenarios; focus changes around the account-switch logic that calls syncCodexCliActiveSelectionForIndex and ensure tests in accounts-load-from-disk.test.ts (the suite that currently only covers the true path) are extended to assert behavior when the function returns false.index.ts (1)
965-976:⚠️ Potential issue | 🟠 Majorreload before syncing the cli selection.
lines 965-968 in
index.tsstill sync throughcachedAccountManager, and only lines 972-976 inindex.tsreload from disk even though that reload is explicitly there to avoid overwriting newer rotated refresh tokens. this lets stale in-memory auth state do one last write, and it also skips the serialized sync path added at lines 1172-1208 inindex.ts, soaccount.selectcan still race a fetch-time sync.possible fix
await saveAccounts(storage); - if (cachedAccountManager) { - await cachedAccountManager.syncCodexCliActiveSelectionForIndex(index); - } + if (cachedAccountManager) { + const freshManager = await reloadAccountManagerFromDisk(); + await freshManager.syncCodexCliActiveSelectionForIndex(index); + } lastCodexCliActiveSyncIndex = index; lastCodexCliActiveSyncSignature = null; - - // Reload manager from disk so we don't overwrite newer rotated - // refresh tokens with stale in-memory state. - if (cachedAccountManager) { - await reloadAccountManagerFromDisk(); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` around lines 965 - 976, The current sequence saves accounts and then calls cachedAccountManager.syncCodexCliActiveSelectionForIndex(index) before calling reloadAccountManagerFromDisk(), which allows stale in-memory auth state to do a final write and bypass the serialized sync path; to fix, call reloadAccountManagerFromDisk() before using cachedAccountManager.syncCodexCliActiveSelectionForIndex (and before any in-memory writes that could clobber rotated refresh tokens), so move the reloadAccountManagerFromDisk() call to precede the sync (and ensure lastCodexCliActiveSyncIndex/lastCodexCliActiveSyncSignature update still happens after a successful serialized sync).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager/settings-hub.ts`:
- Around line 2548-2572: The toggle for draft.codexCliDirectInjection is never
persisted because the change-detection helper and the patch-building logic (the
comparison helper and save patch builder) omit that key; update both the
comparison helper and the save-patch-builder logic in settings-hub.ts to include
codexCliDirectInjection in the list of tracked keys/fields so flips produce a
detected change and are added to the outgoing patch, and add/adjust tests in
test/settings-hub-utils.test.ts to cover toggling codexCliDirectInjection
(persist then reload) to prevent regressions.
In `@test/index.test.ts`:
- Around line 1145-1231: Add two deterministic vitest tests that exercise the
forced re-sync branches missed by current tests: one that simulates a token
refresh triggering a forced refresh-driven re-sync (use
AccountManager.consumeToken / getCurrentOrNextForFamilyHybrid mocks to simulate
a refresh race and assert syncCodexCliSelectionMock is called for the forced
branch), and another that simulates a stream failover triggering a forced
stream-failover re-sync (mock fetch to fail the first stream and succeed after
failover, call sdk.fetch from setupPlugin and assert syncCodexCliSelectionMock
was invoked for the failover path). Use the existing helpers (setupPlugin,
sdk.fetch, syncCodexCliSelectionMock) and vitest promises/resolve hooks to keep
both tests deterministic and avoid real secret or filesystem access.
---
Outside diff comments:
In `@index.ts`:
- Around line 965-976: The current sequence saves accounts and then calls
cachedAccountManager.syncCodexCliActiveSelectionForIndex(index) before calling
reloadAccountManagerFromDisk(), which allows stale in-memory auth state to do a
final write and bypass the serialized sync path; to fix, call
reloadAccountManagerFromDisk() before using
cachedAccountManager.syncCodexCliActiveSelectionForIndex (and before any
in-memory writes that could clobber rotated refresh tokens), so move the
reloadAccountManagerFromDisk() call to precede the sync (and ensure
lastCodexCliActiveSyncIndex/lastCodexCliActiveSyncSignature update still happens
after a successful serialized sync).
In `@lib/accounts.ts`:
- Around line 392-404: The new boolean return from
syncCodexCliActiveSelectionForIndex indicates success but callers still
fire-and-forget it; update call sites that invoke
syncCodexCliActiveSelectionForIndex (the code that currently triggers the sync
after account switch) to await the Promise and explicitly handle a false result
by retrying or aborting the account-switch transition (i.e., serialize the sync
path so the account never moves on while the CLI write can fail), and add/adjust
vitest cases to cover the false/ebusy/eperm and 429 scenarios; focus changes
around the account-switch logic that calls syncCodexCliActiveSelectionForIndex
and ensure tests in accounts-load-from-disk.test.ts (the suite that currently
only covers the true path) are extended to assert behavior when the function
returns false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4254d083-fcc0-4b09-9a3b-4f0cdce6322d
📒 Files selected for processing (10)
index.tslib/accounts.tslib/codex-manager/settings-hub.tslib/config.tslib/schemas.tslib/ui/copy.tstest/accounts-load-from-disk.test.tstest/index.test.tstest/plugin-config.test.tstest/settings-hub-utils.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/schemas.tslib/ui/copy.tslib/accounts.tslib/config.tslib/codex-manager/settings-hub.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/settings-hub-utils.test.tstest/accounts-load-from-disk.test.tstest/index.test.tstest/plugin-config.test.ts
🔇 Additional comments (1)
index.ts (1)
1162-1170: verify the signature is keyed on the actual expiry field.lines 1163-1169 in
index.tsuseaccount.expires, but the account shape updated elsewhere in this file usesexpiresAtat lines 557-558, 694-696, and 4075-4078 inindex.ts. ifAccountManagerreturns that same shape here, the expiry slot in the signature is always empty and the refresh-aware dedupe around lines 1650-1652 inindex.tswill miss token changes.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
test/index.test.ts (1)
1250-1335:⚠️ Potential issue | 🟠 Majorthis still does not prove the failover force-write path.
in
test/index.test.ts:1250-1331, account 1 has never been injected before the streaming request, sosyncCodexCliSelectionMock(1)would still happen if the explicit failover force-write branch disappeared and the code only reacted to the signature change from account 0 to account 1. seed an earlier account-1 injection, then fail over back to account 1 and assert a second account-1 write still occurs. add aneacces/ebusyrejection variant there too, because that is the windows-sensitive branch in this pr.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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/index.test.ts` around lines 1250 - 1335, Seed an initial account-1 injection before making the streaming request and then trigger the failover to account-1 again so the test proves the "force-write" path: call whatever inject/sync path used (e.g., invoke syncCodexCliSelectionMock(1) or simulate the initial injection via AccountManager.loadFromDisk/mock) prior to the request, then cause the failover to switch to accountTwo and back to accountTwo so the code must write account-1 a second time and assert syncCodexCliSelectionMock calls include a repeated 1; also add failover response variants that reject with EACCES and EBUSY errors (simulate the filesystem/windows-sensitive branch) to ensure those branches trigger the same forced write path. Ensure you reference syncCodexCliSelectionMock, AccountManager.loadFromDisk, and the withStreamingFailover/mock failover function when making these changes.
🤖 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 967-972: The direct write path that calls
reloadAccountManagerFromDisk() then
freshManager.syncCodexCliActiveSelectionForIndex(index) must be routed through
the same codexCliSelectionSyncQueue to avoid races; replace the direct await
sequence with enqueuing a task on codexCliSelectionSyncQueue that performs
reloadAccountManagerFromDisk() and calls
syncCodexCliActiveSelectionForIndex(index), and only when the queued task
returns set lastCodexCliActiveSyncIndex and lastCodexCliActiveSyncSignature as
currently done. Update references to ensure the queued task uses the same shared
state variables (lastCodexCliActiveSyncIndex, lastCodexCliActiveSyncSignature)
and error handling as other queued writers. Add a regression in
test/index.test.ts that simulates an in-flight sync (resolve/reject control) and
then triggers a user account.select to assert no race: the queued writer wins
and the shared cache reflects the correct final selection.
- Around line 1159-1177: The dedupe signature built by
buildCodexCliSelectionSignature uses optional fields (accountId, email,
expires/expiresAt) which can collide for different accounts at the same index;
change the signature to include the stable mandatory per-account field addedAt
instead of any optional identifiers or expiry so signatures are unique per
account instance—update buildCodexCliSelectionSignature to reference
account.addedAt (trim/format as needed) along with account.index, removing
accountId/email/expires usage.
- Around line 1179-1221: The helper syncCodexCliSelectionNow currently lets
rejections from accountManager.syncCodexCliActiveSelectionForIndex bubble up and
abort the request; wrap the call inside runSync with a try/catch that logs the
error (including context like account.index and the built signature) and returns
false on failure so CLI injection is best-effort; ensure you still update
lastCodexCliActiveSyncIndex and lastCodexCliActiveSyncSignature only on a
successful sync; keep existing queue behavior using codexCliSelectionSyncQueue
and releaseSyncQueue unchanged; add/adjust tests (e.g., test/index.test.ts) to
cover the forced failover rejection path so the function returns false instead
of propagating the error.
In `@lib/codex-manager/settings-hub.ts`:
- Around line 2555-2579: The menu items are using redundant hardcoded fallbacks
(the "?? true" on fields like draft.codexCliDirectInjection,
draft.sessionAffinity, draft.retryAllAccountsRateLimited,
draft.preemptiveQuotaEnabled) which duplicate defaults already applied by
cloneBackendPluginConfig(); remove these "?? true" fallbacks and read the
boolean values directly from the normalized draft (or resolve via
BACKEND_DEFAULTS once) when building labels with formatDashboardSettingState and
UI_COPY entries (affects the toggle entries with value types
"toggle-direct-cli-injection", "toggle-session-affinity", "toggle-pool-retry",
"toggle-preemptive-quota" and the rotation quota entry), and apply the same
change to the analogous block around the 2641-2667 region so the UI follows the
single source of truth.
In `@test/index.test.ts`:
- Around line 1181-1248: The new tests only exercise the successful sync path so
they don't catch the regression where lib/accounts.ts's sync method can return
false and poison the signature cache; update the test suite to add a
deterministic two-request case that forces syncCodexCliSelectionMock to resolve
false on the first request and true on the second, call sdk.fetch twice
(concurrently or sequentially as appropriate), and assert that the second
request triggers a retry (i.e., syncCodexCliSelectionMock was called again and
sdk.fetch succeeds both times); reference and modify the existing tests around
syncCodexCliSelectionMock and sdk.fetch to implement this deterministic
false-then-true scenario using vitest mocks.
---
Duplicate comments:
In `@test/index.test.ts`:
- Around line 1250-1335: Seed an initial account-1 injection before making the
streaming request and then trigger the failover to account-1 again so the test
proves the "force-write" path: call whatever inject/sync path used (e.g., invoke
syncCodexCliSelectionMock(1) or simulate the initial injection via
AccountManager.loadFromDisk/mock) prior to the request, then cause the failover
to switch to accountTwo and back to accountTwo so the code must write account-1
a second time and assert syncCodexCliSelectionMock calls include a repeated 1;
also add failover response variants that reject with EACCES and EBUSY errors
(simulate the filesystem/windows-sensitive branch) to ensure those branches
trigger the same forced write path. Ensure you reference
syncCodexCliSelectionMock, AccountManager.loadFromDisk, and the
withStreamingFailover/mock failover function when making these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b13e10e2-cf26-4458-8161-cf321b3bb484
📒 Files selected for processing (6)
index.tslib/accounts.tslib/codex-manager/settings-hub.tstest/accounts-load-from-disk.test.tstest/index.test.tstest/settings-hub-utils.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/settings-hub-utils.test.tstest/accounts-load-from-disk.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/accounts.tslib/codex-manager/settings-hub.ts
🪛 Gitleaks (8.30.0)
test/settings-hub-utils.test.ts
[high] 704-704: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
|
Please rerun Greptile on the latest head The PR body has been cleaned back to the repo template, the blocking review follow-ups are already patched on this branch, and the latest local validation passed:
If Greptile still finds anything new on the current head, I’ll patch it on the same branch. |
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 932-965: queueCodexCliSelectionSync's use of Promise.race lets
runSync continue running and hold the writer queue, so a timed-out task can
later mutate shared state; fix by making the writer/enqueue operations
cancellable or add a generation token check: when calling runSync from
queueCodexCliSelectionSync, generate an incrementing syncId and pass it (or an
AbortSignal) into the writer.enqueue/internal write code and into any updates to
lastCodexCliActiveSync*; in the writer (the enqueue/write functions referenced
in lib/codex-cli/writer.ts) ensure late completions either abort on the signal
or verify the syncId matches the current generation before mutating shared state
so timed-out tasks cannot wedge the queue or apply stale updates.
- Around line 1244-1271: The current call to
accountManager.syncCodexCliActiveSelectionForIndex(...) treats a boolean false
as a failure, but the underlying writer returns false for no-op
(already-current) cases; change the contract so the caller can distinguish noop
vs real failure: either have syncCodexCliActiveSelectionForIndex return a
discriminated result (e.g. {status: "synced" | "noop" | "failed"} or throw on
real errors and return true for both synced/noop) or update the writer to return
true for noop; then update this caller to treat noop as success and set
lastCodexCliActiveSyncIndex and lastCodexCliActiveSyncSignature when status is
"synced" or "noop" (only treat actual failures as errors).
In `@lib/codex-manager/settings-hub.ts`:
- Around line 1079-1082: The backend reset logic currently re-applies
BACKEND_DEFAULTS.codexCliDirectInjection and can silently re-enable the
experimental codexCliDirectInjection even though it's only exposed in the
experimental menu; update the backend reset path (the code that copies
BACKEND_DEFAULTS in the backend reset function around the block that reads
BACKEND_DEFAULTS) to preserve the existing snapshot/config value for
codexCliDirectInjection instead of overwriting it, or alternatively add
codexCliDirectInjection to the backend hub UI/model so it is explicitly
surfaced; after making the change, add a vitest that exercises the backend reset
path (resetting backend hub state) and asserts that codexCliDirectInjection
remains unchanged when the setting was previously toggled.
In `@test/index.test.ts`:
- Around line 1626-1630: The shared mock syncCodexCliSelectionMock is being
mockReset()'ed which removes its default success behavior established for the
suite (set at test/index.test.ts:296), causing later tests to observe undefined;
instead restore the default behavior after the failure-specific stubbing by
either using mockClear() + re-applying the default mockResolvedValue(true) or
call mockRestore()/re-mock the original implementation so that
syncCodexCliSelectionMock returns the intended success value by default and only
the failing case uses mockResolvedValueOnce/mockRejectedValueOnce; update the
block that currently calls syncCodexCliSelectionMock.mockReset() to clear only
invocation history and then re-establish the default behavior for
syncCodexCliSelectionMock so other tests remain deterministic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: adb562de-2b09-4c6d-9c4d-9337b3a21b68
📒 Files selected for processing (3)
index.tslib/codex-manager/settings-hub.tstest/index.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager/settings-hub.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/index.test.ts
🔇 Additional comments (2)
lib/codex-manager/settings-hub.ts (2)
277-281: nice reuse of the rotation/quota editor.
lib/codex-manager/settings-hub.ts:277-281defines a narrow action set,lib/codex-manager/settings-hub.ts:2555-2579renders it, andlib/codex-manager/settings-hub.ts:2621-2625/lib/codex-manager/settings-hub.ts:2630-2667keep everything on the samedraft, so the new experimental toggles and theoshortcut do not fork state or add another persistence path.Also applies to: 2555-2579, 2621-2625, 2630-2667
1139-1142: good fix for the stale 5h preview fallback.
lib/codex-manager/settings-hub.ts:1139-1142now lines up with the 10% default fromlib/config.ts:1-45, so the preview stops drifting whenpreemptiveQuotaRemainingPercent5his unset.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
index.ts (1)
932-968:⚠️ Potential issue | 🟠 Majorthe timeout still lets stale sync completions win the cache.
index.ts:944-959only stops waiting. the timed-outrunSync()can still finish later and updatelastCodexCliActiveSyncIndex/lastCodexCliActiveSyncSignatureatindex.ts:1270-1273after a newer account has already synced. that can suppress the next required reinjection, and it reopens the windows contention path this queue is supposed to contain. add a generation or abort guard, plus a vitest intest/index.test.tsthat forces a late completion after timeout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` around lines 932 - 968, The timeout path in queueCodexCliSelectionSync currently only stops waiting but allows a late runSync() completion to overwrite lastCodexCliActiveSyncIndex/lastCodexCliActiveSyncSignature; add a generation/epoch guard or an AbortSignal that you pass into runSync() (or wrap runSync) so that when the timeout branch resolves you mark that generation as stale and any later completion of runSync() checks the generation/abort before updating lastCodexCliActiveSyncIndex/lastCodexCliActiveSyncSignature; update codexCliSelectionSyncQueue handling as needed to propagate the guard and add a vitest in test/index.test.ts that simulates a runSync() which resolves after CODEX_CLI_SELECTION_SYNC_TIMEOUT_MS to assert the stale completion does not change the lastCodexCliActiveSync* values.
🤖 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 2326-2328: The failover refresh can update fallbackAuth without
changing the fallbackAccount index, causing syncCodexCliSelectionNow to skip
reinjection due to unchanged index|addedAt; modify the failover refresh flow to
set a boolean flag (e.g., fallbackAuthRefreshed) whenever fallbackAuth is
replaced/rotated and use that flag to force
syncCodexCliSelectionNow(fallbackAccount, { force: true }) even if the account
index is unchanged (update the logic around where fallbackAuth is refreshed and
where you currently conditionally call syncCodexCliSelectionNow in the
post-failover path—refer to symbols fallbackAuth, fallbackAccount, and
syncCodexCliSelectionNow); reset the flag after the forced reinjection and add a
regression test in test/index.test.ts that simulates a failover which refreshes
credentials for the same account index and asserts that the CLI tokens are
reinjected (i.e., force path exercised).
- Around line 1007-1032: The manual selection sync calls
syncCodexCliActiveSelectionForIndex unconditionally; gate this by checking
codexCliDirectInjectionEnabled before invoking
queueCodexCliSelectionSync/reloadAccountManagerFromDisk/syncCodexCliActiveSelectionForIndex
so no sync occurs when codexCliDirectInjectionEnabled is false, and keep the
existing behavior of setting
lastCodexCliActiveSyncIndex/lastCodexCliActiveSyncSignature only when synced is
true; also add a vitest in test/index.test.ts that simulates account.select with
codexCliDirectInjectionEnabled = false and asserts that
syncCodexCliActiveSelectionForIndex is not called and codex CLI state is not
rewritten.
In `@test/index.test.ts`:
- Around line 1184-1221: The test never verifies that the actual network call is
blocked while the first CLI sync is pending; after awaiting syncStarted and
before calling resolveSync(), add assertions that globalThis.fetch has not been
invoked and that syncCodexCliSelectionMock was called exactly once to prove both
requests are queued (e.g., expect(globalThis.fetch).not.toHaveBeenCalled() and
expect(syncCodexCliSelectionMock).toHaveBeenCalledTimes(1) before
resolveSync()). Keep the existing variables (syncStarted, resolveSync,
syncCodexCliSelectionMock, firstFetch, secondFetch, sdk.fetch) to locate where
to insert these pre-release assertions.
- Around line 1302-1354: Add a deterministic positive cache-hit regression by
inserting a middle request that neither triggers token refresh nor a failed
write and assert that syncCodexCliSelectionMock is NOT called for that request;
specifically, in the test block that manipulates fetchHelpers.shouldRefreshToken
and fetchHelpers.refreshAndUpdateToken (the "forces CLI reinjection..." and
"retries CLI injection..." tests), add a third call sequence where
shouldRefreshToken returns false and syncCodexCliSelectionMock resolves true (or
does not change state) so you can call sdk.fetch! once more between the pre- and
post-refresh/-failure requests and then assert
syncCodexCliSelectionMock.mock.calls has the same count for the middle request
(i.e., no extra sync call), referencing syncCodexCliSelectionMock and sdk.fetch!
to locate where to add the extra request and assertion.
In `@test/settings-hub-utils.test.ts`:
- Around line 694-726: The current test queues the action object { type:
"open-rotation-quota" } directly which misses exercising the actual onInput("o")
shortcut branch; update or add a vitest that calls promptExperimentalSettings
(or the helper used in tests) but instead of queueing the action object, capture
the select() options emitted by the prompt and simulate the user pressing the
'o' shortcut on the "Rotation quota" option to ensure onInput("o") returns the
same { type: "open-rotation-quota" } action; use the existing
queueSelectResults/select capture helpers pattern so the test remains
deterministic and asserts the shortcut triggers the expected action object
rather than relying on the direct queued action.
---
Duplicate comments:
In `@index.ts`:
- Around line 932-968: The timeout path in queueCodexCliSelectionSync currently
only stops waiting but allows a late runSync() completion to overwrite
lastCodexCliActiveSyncIndex/lastCodexCliActiveSyncSignature; add a
generation/epoch guard or an AbortSignal that you pass into runSync() (or wrap
runSync) so that when the timeout branch resolves you mark that generation as
stale and any later completion of runSync() checks the generation/abort before
updating lastCodexCliActiveSyncIndex/lastCodexCliActiveSyncSignature; update
codexCliSelectionSyncQueue handling as needed to propagate the guard and add a
vitest in test/index.test.ts that simulates a runSync() which resolves after
CODEX_CLI_SELECTION_SYNC_TIMEOUT_MS to assert the stale completion does not
change the lastCodexCliActiveSync* values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: be49914c-4ba9-47f4-9a4c-6b0611a6154f
📒 Files selected for processing (4)
index.tslib/codex-manager/settings-hub.tstest/index.test.tstest/settings-hub-utils.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager/settings-hub.ts
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/settings-hub-utils.test.tstest/index.test.ts
🪛 Gitleaks (8.30.0)
test/settings-hub-utils.test.ts
[high] 704-704: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
lib/codex-manager/settings-hub.ts (1)
2467-2473:⚠️ Potential issue | 🟠 Majorpreserve the remaining experimental-only refresh settings on backend reset.
lib/codex-manager/settings-hub.ts:2467-2473now carries a few experimental values through reset, butproactiveRefreshGuardianandproactiveRefreshIntervalMsare still rebuilt from defaults even though they are only exposed fromlib/codex-manager/settings-hub.ts:2587-2606and are not part of the backend category UI inlib/codex-manager/settings-hub.ts:503-554. a backend reset will still silently rewrite those hidden settings on save. please carry both fields through this path and extendtest/settings-hub-utils.test.ts:738-758to lock it down.possible fix
if (result.type === "reset") { draft = cloneBackendPluginConfig({ ...BACKEND_DEFAULTS, codexCliDirectInjection: draft.codexCliDirectInjection, + proactiveRefreshGuardian: draft.proactiveRefreshGuardian, + proactiveRefreshIntervalMs: draft.proactiveRefreshIntervalMs, sessionAffinity: draft.sessionAffinity, retryAllAccountsRateLimited: draft.retryAllAccountsRateLimited, preemptiveQuotaEnabled: draft.preemptiveQuotaEnabled, });As per coding guidelines,
lib/**: verify every change cites affected tests (vitest).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager/settings-hub.ts` around lines 2467 - 2473, The backend-reset branch currently rebuilds experimental-only fields from BACKEND_DEFAULTS when creating draft via cloneBackendPluginConfig, which drops proactiveRefreshGuardian and proactiveRefreshIntervalMs; update the reset logic that builds draft (the cloneBackendPluginConfig call that seeds with BACKEND_DEFAULTS and copies codexCliDirectInjection/sessionAffinity/etc.) to also preserve draft.proactiveRefreshGuardian and draft.proactiveRefreshIntervalMs so they are carried through saves, and add assertions in the test/settings-hub-utils.test.ts unit test (around the existing 738-758 area) to lock these two fields so a backend reset does not overwrite them.
🤖 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 1009-1046: The manual account-switch path only reloads the
AccountManager when cachedAccountManager exists, so when cachedAccountManager is
null the Codex CLI sync (syncCodexCliActiveSelectionForIndex) is never
attempted; change the flow so that when
getCodexCliDirectInjection(loadPluginConfig()) is true you call
reloadAccountManagerFromDisk() unconditionally (or use its returned/updated
manager) before queuing queueCodexCliSelectionSync, then use the reloaded
manager (not cachedAccountManager) to call
syncCodexCliActiveSelectionForIndex(index) and update
lastCodexCliActiveSyncIndex/lastCodexCliActiveSyncSignature on success; also add
a regression test in test/index.test.ts that calls account.select before any
loader/fetch to assert the Codex CLI was synced.
In `@test/index.test.ts`:
- Around line 1900-1902: Replace one-shot stubs of getCodexCliDirectInjection
(currently using vi.mocked(...).mockReturnValueOnce(false)) with a persistent
mockReturnValue(false) for the duration of the test and ensure you restore the
original behavior (e.g., mockReturnValue(true)) in a finally/after block; update
both occurrences around the test setup where
vi.mocked(configModule.getCodexCliDirectInjection) is used so repeated calls
during retries/failover use the same false value and explicitly clean up the
mock at test end.
---
Duplicate comments:
In `@lib/codex-manager/settings-hub.ts`:
- Around line 2467-2473: The backend-reset branch currently rebuilds
experimental-only fields from BACKEND_DEFAULTS when creating draft via
cloneBackendPluginConfig, which drops proactiveRefreshGuardian and
proactiveRefreshIntervalMs; update the reset logic that builds draft (the
cloneBackendPluginConfig call that seeds with BACKEND_DEFAULTS and copies
codexCliDirectInjection/sessionAffinity/etc.) to also preserve
draft.proactiveRefreshGuardian and draft.proactiveRefreshIntervalMs so they are
carried through saves, and add assertions in the test/settings-hub-utils.test.ts
unit test (around the existing 738-758 area) to lock these two fields so a
backend reset does not overwrite them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6262bada-abc1-4a65-a814-8468c910b0c9
📒 Files selected for processing (4)
index.tslib/codex-manager/settings-hub.tstest/index.test.tstest/settings-hub-utils.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/settings-hub-utils.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/codex-manager/settings-hub.ts
🪛 Gitleaks (8.30.0)
test/settings-hub-utils.test.ts
[high] 704-704: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
|
@greptile @coderabbitai review |
|
✅ Actions performedFull review triggered. |
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 (1)
test/accounts-load-from-disk.test.ts (1)
162-185:⚠️ Potential issue | 🟡 Minorassert the false return contract for invalid indices.
test/accounts-load-from-disk.test.ts:180-182only checks that the writer was not called.lib/accounts.ts:391-395now promisesfalsefor invalid input, so a regression back toundefinedor an accidental earlytruewould still pass here. addresolves.toBe(false)for the invalid cases, and include one non-finite input so the new guard is actually exercised.proposed test fix
- await manager.syncCodexCliActiveSelectionForIndex(-1); - await manager.syncCodexCliActiveSelectionForIndex(9); + await expect( + manager.syncCodexCliActiveSelectionForIndex(Number.NaN), + ).resolves.toBe(false); + await expect(manager.syncCodexCliActiveSelectionForIndex(-1)).resolves.toBe( + false, + ); + await expect(manager.syncCodexCliActiveSelectionForIndex(9)).resolves.toBe( + false, + ); expect(setCodexCliActiveSelection).not.toHaveBeenCalled();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/accounts-load-from-disk.test.ts` around lines 162 - 185, The test should assert the false return contract for invalid indices: update the cases calling syncCodexCliActiveSelectionForIndex(-1) and syncCodexCliActiveSelectionForIndex(9) to also await and expect(...).resolves.toBe(false), and add one non-finite input (e.g., NaN or Infinity) with the same resolves.toBe(false) assertion so the guard in syncCodexCliActiveSelectionForIndex is exercised; keep the existing assertion that setCodexCliActiveSelection was not called and the valid-index case expecting resolves.toBe(true).
🤖 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 1030-1052: The runSync callback passed to
queueCodexCliSelectionSync ignores the provided context and may perform
unnecessary disk writes; modify the anonymous runSync to accept the context
parameter (e.g., runSync: async (context) => { ... }) and immediately
short-circuit with if (context.isStale()) return false; before calling
activeAccountManager.syncCodexCliActiveSelectionForIndex(index), keeping the
existing try/catch and logging logic intact (mirror the early-staleness pattern
used in syncCodexCliSelectionNow).
In `@lib/codex-manager/settings-hub.ts`:
- Around line 2467-2475: The reset-to-default logic is incorrectly preserving
user-visible backend toggles (sessionAffinity, retryAllAccountsRateLimited,
preemptiveQuotaEnabled); change the cloneBackendPluginConfig call that builds
draft (the block using BACKEND_DEFAULTS and cloneBackendPluginConfig in
settings-hub.ts) to only carry forward experimental-only fields (keep
codexCliDirectInjection, proactiveRefreshGuardian, proactiveRefreshIntervalMs)
and remove sessionAffinity, retryAllAccountsRateLimited, and
preemptiveQuotaEnabled so visible controls revert to BACKEND_DEFAULTS; update
the related test expectations in test/settings-hub-utils.test.ts (around lines
738-762) to assert that only the hidden experimental fields survive reset.
In `@test/index.test.ts`:
- Around line 1456-1801: The tests currently only assert
syncCodexCliSelectionMock was called but not that the actual outbound auth
changed; update each affected test (the ones that mock createCodexHeaders/switch
or refresh auth and call sdk.fetch) to also inspect globalThis.fetch.mock.calls
and assert the "x-test-access-token" header sequence changed as expected (e.g.,
map each call's request init to headers and extract
header.get("x-test-access-token") or the header value passed via
createCodexHeaders), so instead of only checking syncCodexCliSelectionMock you
assert the actual outbound token/account values in the fetch calls after
sdk.fetch and streaming failover paths (references: syncCodexCliSelectionMock,
createCodexHeaders, sdk.fetch, and globalThis.fetch.mock.calls).
- Around line 1934-2051: Add a deterministic vitest that covers the
forced-refresh path while direct CLI injection is disabled: mock
config.getCodexCliDirectInjection to return false, mock the function that
triggers refresh (shouldRefreshToken) to return true for the request, ensure
token refresh logic runs (e.g., via AccountManager.loadFromDisk or the same
customManager pattern used elsewhere) and call setupPlugin then sdk.fetch with a
request that would cause a refresh; finally assert syncCodexCliSelectionMock was
not called. Reference getCodexCliDirectInjection, shouldRefreshToken,
syncCodexCliSelectionMock, setupPlugin, and sdk.fetch to locate where to add and
hook the test.
- Around line 3382-3399: The test "persists account.select when the cache is
empty" is selecting index: 0 which is the default; change the selection to a
non-default value (e.g. index: 1) in the plugin.event call so the test actually
verifies the handler persists the requested account, and update the
corresponding expectations from mockStorage.activeIndex and
mockStorage.activeIndexByFamily["gpt-5.1"] to assert the new index value (1)
instead of 0; locate this test by the it(...) block name and the plugin.event({
event: { type: "account.select", properties: { index: ... } } }) call to apply
the change.
In `@test/plugin-config.test.ts`:
- Around line 815-825: Add a deterministic vitest that covers the
config-precedence branch of getCodexCliDirectInjection: with no
CODEX_AUTH_DIRECT_CLI_INJECTION env var set and calling
getCodexCliDirectInjection({ codexCliDirectInjection: false }) assert the result
is false; specifically update the test suite in test/plugin-config.test.ts (the
"direct cli injection setting" describe block) to include a case that clears the
env var, calls getCodexCliDirectInjection with codexCliDirectInjection: false,
and asserts false so the code path honoring pluginConfig.codexCliDirectInjection
is exercised.
---
Outside diff comments:
In `@test/accounts-load-from-disk.test.ts`:
- Around line 162-185: The test should assert the false return contract for
invalid indices: update the cases calling
syncCodexCliActiveSelectionForIndex(-1) and
syncCodexCliActiveSelectionForIndex(9) to also await and
expect(...).resolves.toBe(false), and add one non-finite input (e.g., NaN or
Infinity) with the same resolves.toBe(false) assertion so the guard in
syncCodexCliActiveSelectionForIndex is exercised; keep the existing assertion
that setCodexCliActiveSelection was not called and the valid-index case
expecting resolves.toBe(true).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b3830b24-fa48-4111-b980-220c2c85c6b6
📒 Files selected for processing (10)
index.tslib/accounts.tslib/codex-manager/settings-hub.tslib/config.tslib/schemas.tslib/ui/copy.tstest/accounts-load-from-disk.test.tstest/index.test.tstest/plugin-config.test.tstest/settings-hub-utils.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/schemas.tslib/accounts.tslib/config.tslib/ui/copy.tslib/codex-manager/settings-hub.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/accounts-load-from-disk.test.tstest/plugin-config.test.tstest/settings-hub-utils.test.tstest/index.test.ts
🔇 Additional comments (7)
index.ts (7)
930-979: queue serialization and timeout unblock look solid.the
queueCodexCliSelectionSyncimplementation atindex.ts:930-979correctly serializes writes throughcodexCliSelectionSyncQueueand uses the generation guard inisStale()to prevent stale completions from corrupting shared state after timeout.the documented behavior (lines 959-960) that timeout only unblocks the queue while letting the detached write finish is acceptable given the generation check at the end of
runSyncinsyncCodexCliSelectionNow.
1244-1250: signature now uses stableaddedAt— correctly addresses prior concern.
buildCodexCliSelectionSignatureatindex.ts:1244-1250usesaccount.indexandaccount.addedAtwhich are both stable per-account fields. this prevents the prior issue where two distinct accounts at the same index could collapse to the same signature due to optionalaccountId/expiresfields.
1259-1313:syncCodexCliSelectionNowcorrectly gates on direct injection and uses generation guard.the implementation at
index.ts:1259-1313:
- early-returns when
codexCliDirectInjectionEnabledis false (line 1259-1261)- checks
context.isStale()before starting sync (line 1268-1270)- checks
!context.isStale()again after sync completes before updating cache (line 1303)- uses
cachedAccountManager ?? accountManagerto get fresh manager state (line 1278)the double staleness check (before and after) is the right belt-and-suspenders approach for race conditions.
1016-1026: manager reload for first manual switch looks correct.at
index.ts:1021-1025, the conditioncachedAccountManager || codexCliDirectInjectionEnabledForEventensures we reload from disk even whencachedAccountManageris null, as long as direct injection is enabled. this addresses the prior concern where the first manual switch before any loader/fetch would skip sync.
2367-2375: force reinjection on same-account refresh during failover — correctly implemented.at
index.ts:2367-2375, the conditionfallbackAccount.index !== account.index || fallbackAuthRefreshedensures we force sync when:
- failover switches to a different account, or
- failover refreshes credentials for the same account
this addresses the prior issue where same-account failover refresh would skip reinjection due to unchanged
index|addedAtsignature.
1756-1759: request sync withforce: accountAuthRefreshedhandles credential rotation.at
index.ts:1756-1759, passingforce: accountAuthRefreshedensures codex cli gets fresh tokens when the request path refreshes credentials, even if the account index hasn't changed.
2492-2494: post-success sync uses captured generation — prevents stale request from overwriting manual selection.at
index.ts:2492-2494, passinggeneration: cliSelectionGenerationForRequest(captured at line 1454) means if a manual selection happened during the request, the staleness check insyncCodexCliSelectionNowwill detectcodexCliSelectionGeneration !== generationand skip the sync, preventing the request's account from overwriting a newer manual selection.
index.ts
Outdated
| runSync: async () => { | ||
| try { | ||
| return await activeAccountManager.syncCodexCliActiveSelectionForIndex( | ||
| index, | ||
| ); | ||
| } catch (error) { | ||
| logWarn( | ||
| `[${PLUGIN_NAME}] Codex CLI selection sync failed for manual account selection`, | ||
| { | ||
| accountIndex: index, | ||
| code: | ||
| error && | ||
| typeof error === "object" && | ||
| "code" in error | ||
| ? String((error as NodeJS.ErrnoException).code ?? "") | ||
| : "", | ||
| error: | ||
| error instanceof Error ? error.message : String(error), | ||
| }, | ||
| ); | ||
| return false; | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
manual selection runSync ignores context.isStale() — minor optimization opportunity.
the callback passed to queueCodexCliSelectionSync at index.ts:1030-1052 doesn't use the context parameter to check staleness before starting the disk write:
runSync: async () => { // context not destructured
try {
return await activeAccountManager.syncCodexCliActiveSelectionForIndex(index);compare with syncCodexCliSelectionNow at index.ts:1265-1270 which short-circuits early:
const runSync = async (context: { isStale: () => boolean }): Promise<boolean> => {
if (context.isStale()) {
return false;
}the post-sync check at lines 1054-1057 prevents cache corruption, so this isn't a correctness issue — the queue serialization guarantees correct final state. but if a newer manual selection increments the generation while this one waits in queue, we'd still do a disk write that gets immediately overwritten.
for high-frequency selection changes this could add unnecessary i/o and extend windows ebusy exposure.
optional: add early staleness check
if (activeAccountManager && codexCliDirectInjectionEnabledForEvent) {
const synced = await queueCodexCliSelectionSync({
generation: manualSelectionGeneration,
- runSync: async () => {
+ runSync: async (context) => {
+ if (context.isStale()) {
+ return false;
+ }
try {
return await activeAccountManager.syncCodexCliActiveSelectionForIndex(
index,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.ts` around lines 1030 - 1052, The runSync callback passed to
queueCodexCliSelectionSync ignores the provided context and may perform
unnecessary disk writes; modify the anonymous runSync to accept the context
parameter (e.g., runSync: async (context) => { ... }) and immediately
short-circuit with if (context.isStale()) return false; before calling
activeAccountManager.syncCodexCliActiveSelectionForIndex(index), keeping the
existing try/catch and logging logic intact (mirror the early-staleness pattern
used in syncCodexCliSelectionNow).
| draft = cloneBackendPluginConfig({ | ||
| ...BACKEND_DEFAULTS, | ||
| codexCliDirectInjection: draft.codexCliDirectInjection, | ||
| proactiveRefreshGuardian: draft.proactiveRefreshGuardian, | ||
| proactiveRefreshIntervalMs: draft.proactiveRefreshIntervalMs, | ||
| sessionAffinity: draft.sessionAffinity, | ||
| retryAllAccountsRateLimited: draft.retryAllAccountsRateLimited, | ||
| preemptiveQuotaEnabled: draft.preemptiveQuotaEnabled, | ||
| }); |
There was a problem hiding this comment.
backend reset should not preserve visible backend toggles.
lib/codex-manager/settings-hub.ts:2467-2475 now carries sessionAffinity, retryAllAccountsRateLimited, and preemptiveQuotaEnabled across "reset to default", but those same fields are editable in the backend hub at lib/codex-manager/settings-hub.ts:503-553. after this change, reset can leave the backend preview and saved config non-default even though the ui says it was reset. preserve only the experimental-only fields here, and let the visible backend controls fall back to BACKEND_DEFAULTS. test/settings-hub-utils.test.ts:738-762 should flip to assert that only the hidden experimental fields survive reset.
proposed fix
if (result.type === "reset") {
draft = cloneBackendPluginConfig({
...BACKEND_DEFAULTS,
codexCliDirectInjection: draft.codexCliDirectInjection,
proactiveRefreshGuardian: draft.proactiveRefreshGuardian,
proactiveRefreshIntervalMs: draft.proactiveRefreshIntervalMs,
- sessionAffinity: draft.sessionAffinity,
- retryAllAccountsRateLimited: draft.retryAllAccountsRateLimited,
- preemptiveQuotaEnabled: draft.preemptiveQuotaEnabled,
});
for (const category of BACKEND_CATEGORY_OPTIONS) {
focusByCategory[category.key] =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/codex-manager/settings-hub.ts` around lines 2467 - 2475, The
reset-to-default logic is incorrectly preserving user-visible backend toggles
(sessionAffinity, retryAllAccountsRateLimited, preemptiveQuotaEnabled); change
the cloneBackendPluginConfig call that builds draft (the block using
BACKEND_DEFAULTS and cloneBackendPluginConfig in settings-hub.ts) to only carry
forward experimental-only fields (keep codexCliDirectInjection,
proactiveRefreshGuardian, proactiveRefreshIntervalMs) and remove
sessionAffinity, retryAllAccountsRateLimited, and preemptiveQuotaEnabled so
visible controls revert to BACKEND_DEFAULTS; update the related test
expectations in test/settings-hub-utils.test.ts (around lines 738-762) to assert
that only the hidden experimental fields survive reset.
| it("forces CLI reinjection after a token refresh keeps the same signature fields", async () => { | ||
| const fetchHelpers = await import("../lib/request/fetch-helpers.js"); | ||
| vi.mocked(fetchHelpers.shouldRefreshToken) | ||
| .mockReturnValueOnce(false) | ||
| .mockReturnValueOnce(false) | ||
| .mockReturnValueOnce(true); | ||
| vi.mocked(fetchHelpers.refreshAndUpdateToken).mockResolvedValueOnce({ | ||
| type: "oauth", | ||
| access: "refreshed-access-token", | ||
| refresh: "refresh-token", | ||
| expires: Date.now() + 60_000, | ||
| multiAccount: true, | ||
| }); | ||
| globalThis.fetch = vi.fn().mockResolvedValue( | ||
| new Response(JSON.stringify({ content: "ok" }), { status: 200 }), | ||
| ); | ||
|
|
||
| const { sdk } = await setupPlugin(); | ||
| const firstResponse = await sdk.fetch!("https://api.openai.com/v1/chat", { | ||
| method: "POST", | ||
| body: JSON.stringify({ model: "gpt-5.1" }), | ||
| }); | ||
| const secondResponse = await sdk.fetch!("https://api.openai.com/v1/chat", { | ||
| method: "POST", | ||
| body: JSON.stringify({ model: "gpt-5.1" }), | ||
| }); | ||
| expect(syncCodexCliSelectionMock).toHaveBeenCalledTimes(1); | ||
| const thirdResponse = await sdk.fetch!("https://api.openai.com/v1/chat", { | ||
| method: "POST", | ||
| body: JSON.stringify({ model: "gpt-5.1" }), | ||
| }); | ||
|
|
||
| expect(firstResponse.status).toBe(200); | ||
| expect(secondResponse.status).toBe(200); | ||
| expect(thirdResponse.status).toBe(200); | ||
| expect(syncCodexCliSelectionMock.mock.calls.map((call) => call[0])).toEqual([0, 0]); | ||
| }); | ||
|
|
||
| it("retries CLI injection on the next request after a failed write", async () => { | ||
| syncCodexCliSelectionMock | ||
| .mockResolvedValueOnce(false) | ||
| .mockResolvedValueOnce(true); | ||
| globalThis.fetch = vi.fn().mockResolvedValue( | ||
| new Response(JSON.stringify({ content: "ok" }), { status: 200 }), | ||
| ); | ||
|
|
||
| const { sdk } = await setupPlugin(); | ||
| const firstResponse = await sdk.fetch!("https://api.openai.com/v1/chat", { | ||
| method: "POST", | ||
| body: JSON.stringify({ model: "gpt-5.1" }), | ||
| }); | ||
| const secondResponse = await sdk.fetch!("https://api.openai.com/v1/chat", { | ||
| method: "POST", | ||
| body: JSON.stringify({ model: "gpt-5.1" }), | ||
| }); | ||
|
|
||
| expect(firstResponse.status).toBe(200); | ||
| expect(secondResponse.status).toBe(200); | ||
| expect(syncCodexCliSelectionMock.mock.calls.map((call) => call[0])).toEqual([0, 0]); | ||
| }); | ||
|
|
||
| it("forces CLI reinjection when stream failover switches to another account", async () => { | ||
| vi.doMock("../lib/request/stream-failover.js", () => ({ | ||
| withStreamingFailover: async ( | ||
| response: Response, | ||
| failover: (attempt: number, emittedBytes: number) => Promise<Response | null>, | ||
| ) => (await failover(1, 128)) ?? response, | ||
| })); | ||
| try { | ||
| vi.resetModules(); | ||
| const fetchHelpers = await import("../lib/request/fetch-helpers.js"); | ||
| vi.mocked(fetchHelpers.createCodexHeaders).mockImplementation( | ||
| (_init, _accountId, accessToken) => | ||
| new Headers({ "x-test-access-token": String(accessToken) }), | ||
| ); | ||
| const { AccountManager } = await import("../lib/accounts.js"); | ||
| const accountOne = { | ||
| index: 0, | ||
| accountId: "acc-1", | ||
| email: "user1@example.com", | ||
| refreshToken: "refresh-1", | ||
| access: "access-account-1", | ||
| expires: Date.now() + 60_000, | ||
| addedAt: 1, | ||
| }; | ||
| const accountTwo = { | ||
| index: 1, | ||
| accountId: "acc-2", | ||
| email: "user2@example.com", | ||
| refreshToken: "refresh-2", | ||
| access: "access-account-2", | ||
| expires: Date.now() + 60_000, | ||
| addedAt: 2, | ||
| }; | ||
| const customManager = { | ||
| accounts: [accountOne, accountTwo], | ||
| getCurrentOrNextForFamilyHybrid: () => accountOne, | ||
| getAccountCount: () => 2, | ||
| getAccountsSnapshot: () => [accountOne, accountTwo], | ||
| isAccountAvailableForFamily: () => true, | ||
| getAccountByIndex: (index: number) => | ||
| index === 0 ? accountOne : index === 1 ? accountTwo : null, | ||
| toAuthDetails: (account: typeof accountOne) => ({ | ||
| type: "oauth" as const, | ||
| access: account.access, | ||
| refresh: account.refreshToken, | ||
| expires: account.expires, | ||
| multiAccount: true, | ||
| }), | ||
| consumeToken: vi.fn(() => true), | ||
| updateFromAuth: vi.fn(), | ||
| clearAuthFailures: vi.fn(), | ||
| saveToDiskDebounced: vi.fn(), | ||
| saveToDisk: vi.fn(), | ||
| hasRefreshToken: vi.fn(() => true), | ||
| syncCodexCliActiveSelectionForIndex: (index: number) => | ||
| syncCodexCliSelectionMock(index), | ||
| recordFailure: vi.fn(), | ||
| recordSuccess: vi.fn(), | ||
| recordRateLimit: vi.fn(), | ||
| refundToken: vi.fn(), | ||
| markRateLimitedWithReason: vi.fn(), | ||
| markSwitched: vi.fn(), | ||
| shouldShowAccountToast: () => false, | ||
| markToastShown: vi.fn(), | ||
| getMinWaitTimeForFamily: () => 0, | ||
| getCurrentAccountForFamily: () => accountOne, | ||
| }; | ||
| vi.spyOn(AccountManager, "loadFromDisk").mockResolvedValue(customManager as never); | ||
| globalThis.fetch = vi | ||
| .fn() | ||
| .mockResolvedValueOnce(new Response("streaming", { status: 200 })) | ||
| .mockResolvedValueOnce(new Response("retry later", { status: 429 })) | ||
| .mockResolvedValueOnce( | ||
| new Response(JSON.stringify({ content: "fallback-ok" }), { status: 200 }), | ||
| ); | ||
|
|
||
| const { sdk } = await setupPlugin(); | ||
| const response = await sdk.fetch!("https://api.openai.com/v1/chat", { | ||
| method: "POST", | ||
| body: JSON.stringify({ model: "gpt-5.1", stream: true }), | ||
| }); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(syncCodexCliSelectionMock.mock.calls.map((call) => call[0])).toEqual([0, 1]); | ||
| } finally { | ||
| vi.doUnmock("../lib/request/stream-failover.js"); | ||
| vi.resetModules(); | ||
| } | ||
| }); | ||
|
|
||
| it("forces failover CLI reinjection even when the fallback account was previously injected", async () => { | ||
| vi.doMock("../lib/request/stream-failover.js", () => ({ | ||
| withStreamingFailover: async ( | ||
| response: Response, | ||
| failover: (attempt: number, emittedBytes: number) => Promise<Response | null>, | ||
| ) => (await failover(1, 128)) ?? response, | ||
| })); | ||
| try { | ||
| vi.resetModules(); | ||
| const fetchHelpers = await import("../lib/request/fetch-helpers.js"); | ||
| vi.mocked(fetchHelpers.createCodexHeaders).mockImplementation( | ||
| (_init, _accountId, accessToken) => | ||
| new Headers({ "x-test-access-token": String(accessToken) }), | ||
| ); | ||
| const { AccountManager } = await import("../lib/accounts.js"); | ||
| const accountOne = { | ||
| index: 0, | ||
| accountId: "acc-1", | ||
| email: "user1@example.com", | ||
| refreshToken: "refresh-1", | ||
| access: "access-account-1", | ||
| expires: Date.now() + 60_000, | ||
| addedAt: 1, | ||
| }; | ||
| const accountTwo = { | ||
| index: 1, | ||
| accountId: "acc-2", | ||
| email: "user2@example.com", | ||
| refreshToken: "refresh-2", | ||
| access: "access-account-2", | ||
| expires: Date.now() + 60_000, | ||
| addedAt: 2, | ||
| }; | ||
| const currentAccount = vi | ||
| .fn() | ||
| .mockReturnValueOnce(accountTwo) | ||
| .mockReturnValueOnce(accountOne) | ||
| .mockImplementation(() => null); | ||
| const customManager = { | ||
| accounts: [accountOne, accountTwo], | ||
| getCurrentOrNextForFamilyHybrid: currentAccount, | ||
| getAccountCount: () => 2, | ||
| getAccountsSnapshot: () => [accountOne, accountTwo], | ||
| isAccountAvailableForFamily: () => true, | ||
| getAccountByIndex: (index: number) => | ||
| index === 0 ? accountOne : index === 1 ? accountTwo : null, | ||
| toAuthDetails: (account: typeof accountOne) => ({ | ||
| type: "oauth" as const, | ||
| access: account.access, | ||
| refresh: account.refreshToken, | ||
| expires: account.expires, | ||
| multiAccount: true, | ||
| }), | ||
| consumeToken: vi.fn(() => true), | ||
| updateFromAuth: vi.fn(), | ||
| clearAuthFailures: vi.fn(), | ||
| saveToDiskDebounced: vi.fn(), | ||
| saveToDisk: vi.fn(), | ||
| hasRefreshToken: vi.fn(() => true), | ||
| syncCodexCliActiveSelectionForIndex: (index: number) => | ||
| syncCodexCliSelectionMock(index), | ||
| recordFailure: vi.fn(), | ||
| recordSuccess: vi.fn(), | ||
| recordRateLimit: vi.fn(), | ||
| refundToken: vi.fn(), | ||
| markRateLimitedWithReason: vi.fn(), | ||
| markSwitched: vi.fn(), | ||
| shouldShowAccountToast: () => false, | ||
| markToastShown: vi.fn(), | ||
| getMinWaitTimeForFamily: () => 0, | ||
| getCurrentAccountForFamily: () => accountOne, | ||
| }; | ||
| vi.spyOn(AccountManager, "loadFromDisk").mockResolvedValue(customManager as never); | ||
| syncCodexCliSelectionMock | ||
| .mockResolvedValueOnce(true) | ||
| .mockResolvedValueOnce(false) | ||
| .mockResolvedValueOnce(true); | ||
| globalThis.fetch = vi | ||
| .fn() | ||
| .mockResolvedValueOnce( | ||
| new Response(JSON.stringify({ content: "seed" }), { status: 200 }), | ||
| ) | ||
| .mockResolvedValueOnce(new Response("streaming", { status: 200 })) | ||
| .mockResolvedValueOnce(new Response("retry later", { status: 429 })) | ||
| .mockResolvedValueOnce( | ||
| new Response(JSON.stringify({ content: "fallback-ok" }), { status: 200 }), | ||
| ); | ||
|
|
||
| const { sdk } = await setupPlugin(); | ||
| const seedResponse = await sdk.fetch!("https://api.openai.com/v1/chat", { | ||
| method: "POST", | ||
| body: JSON.stringify({ model: "gpt-5.1" }), | ||
| }); | ||
| const response = await sdk.fetch!("https://api.openai.com/v1/chat", { | ||
| method: "POST", | ||
| body: JSON.stringify({ model: "gpt-5.1", stream: true }), | ||
| }); | ||
|
|
||
| expect(seedResponse.status).toBe(200); | ||
| expect(response.status).toBe(200); | ||
| expect(syncCodexCliSelectionMock.mock.calls.map((call) => call[0])).toEqual([1, 0, 1]); | ||
| } finally { | ||
| vi.doUnmock("../lib/request/stream-failover.js"); | ||
| vi.resetModules(); | ||
| } | ||
| }); | ||
|
|
||
| it("forces failover CLI reinjection when refreshed fallback auth stays on the same account", async () => { | ||
| vi.doMock("../lib/request/stream-failover.js", () => ({ | ||
| withStreamingFailover: async ( | ||
| response: Response, | ||
| failover: (attempt: number, emittedBytes: number) => Promise<Response | null>, | ||
| ) => (await failover(1, 128)) ?? response, | ||
| })); | ||
| try { | ||
| vi.resetModules(); | ||
| const fetchHelpers = await import("../lib/request/fetch-helpers.js"); | ||
| vi.mocked(fetchHelpers.shouldRefreshToken) | ||
| .mockReturnValueOnce(false) | ||
| .mockReturnValueOnce(true); | ||
| vi.mocked(fetchHelpers.refreshAndUpdateToken).mockResolvedValueOnce({ | ||
| type: "oauth", | ||
| access: "refreshed-fallback-access", | ||
| refresh: "refresh-1", | ||
| expires: Date.now() + 60_000, | ||
| multiAccount: true, | ||
| }); | ||
| vi.mocked(fetchHelpers.createCodexHeaders).mockImplementation( | ||
| (_init, _accountId, accessToken) => | ||
| new Headers({ "x-test-access-token": String(accessToken) }), | ||
| ); | ||
| const { AccountManager } = await import("../lib/accounts.js"); | ||
| const accountOne = { | ||
| index: 0, | ||
| accountId: "acc-1", | ||
| email: "user1@example.com", | ||
| refreshToken: "refresh-1", | ||
| access: "access-account-1", | ||
| expires: Date.now() + 60_000, | ||
| addedAt: 1, | ||
| }; | ||
| const customManager = { | ||
| accounts: [accountOne], | ||
| getCurrentOrNextForFamilyHybrid: () => accountOne, | ||
| getAccountCount: () => 1, | ||
| getAccountsSnapshot: () => [accountOne], | ||
| isAccountAvailableForFamily: () => true, | ||
| getAccountByIndex: (index: number) => (index === 0 ? accountOne : null), | ||
| toAuthDetails: () => ({ | ||
| type: "oauth" as const, | ||
| access: accountOne.access, | ||
| refresh: accountOne.refreshToken, | ||
| expires: accountOne.expires, | ||
| multiAccount: true, | ||
| }), | ||
| consumeToken: vi.fn(() => true), | ||
| updateFromAuth: vi.fn(), | ||
| clearAuthFailures: vi.fn(), | ||
| saveToDiskDebounced: vi.fn(), | ||
| saveToDisk: vi.fn(), | ||
| hasRefreshToken: vi.fn(() => true), | ||
| syncCodexCliActiveSelectionForIndex: (index: number) => | ||
| syncCodexCliSelectionMock(index), | ||
| recordFailure: vi.fn(), | ||
| recordSuccess: vi.fn(), | ||
| recordRateLimit: vi.fn(), | ||
| refundToken: vi.fn(), | ||
| markRateLimitedWithReason: vi.fn(), | ||
| markSwitched: vi.fn(), | ||
| shouldShowAccountToast: () => false, | ||
| markToastShown: vi.fn(), | ||
| getMinWaitTimeForFamily: () => 0, | ||
| getCurrentAccountForFamily: () => accountOne, | ||
| }; | ||
| vi.spyOn(AccountManager, "loadFromDisk").mockResolvedValue(customManager as never); | ||
| globalThis.fetch = vi | ||
| .fn() | ||
| .mockResolvedValueOnce(new Response("streaming", { status: 200 })) | ||
| .mockResolvedValueOnce( | ||
| new Response(JSON.stringify({ content: "fallback-ok" }), { status: 200 }), | ||
| ); | ||
|
|
||
| const { sdk } = await setupPlugin(); | ||
| const response = await sdk.fetch!("https://api.openai.com/v1/chat", { | ||
| method: "POST", | ||
| body: JSON.stringify({ model: "gpt-5.1", stream: true }), | ||
| }); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(syncCodexCliSelectionMock.mock.calls.map((call) => call[0])).toEqual([0, 0]); | ||
| } finally { | ||
| vi.doUnmock("../lib/request/stream-failover.js"); | ||
| vi.resetModules(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
assert the outbound auth changed, not just the cli sync.
test/index.test.ts:1462 (Line 1462), test/index.test.ts:1527 (Line 1527), test/index.test.ts:1617 (Line 1617), and test/index.test.ts:1734 (Line 1734) set up refreshed or switched auth, but the assertions at test/index.test.ts:1491 (Line 1491), test/index.test.ts:1600 (Line 1600), test/index.test.ts:1707 (Line 1707), and test/index.test.ts:1796 (Line 1796) only prove that syncCodexCliSelectionMock fired. these tests still stay green if failover/refresh triggers the sync while the retry keeps sending the stale token/account. please pin the x-test-access-token sequence from globalThis.fetch.mock.calls so the regression covers the real auth switch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/index.test.ts` around lines 1456 - 1801, The tests currently only assert
syncCodexCliSelectionMock was called but not that the actual outbound auth
changed; update each affected test (the ones that mock createCodexHeaders/switch
or refresh auth and call sdk.fetch) to also inspect globalThis.fetch.mock.calls
and assert the "x-test-access-token" header sequence changed as expected (e.g.,
map each call's request init to headers and extract
header.get("x-test-access-token") or the header value passed via
createCodexHeaders), so instead of only checking syncCodexCliSelectionMock you
assert the actual outbound token/account values in the fetch calls after
sdk.fetch and streaming failover paths (references: syncCodexCliSelectionMock,
createCodexHeaders, sdk.fetch, and globalThis.fetch.mock.calls).
| it("skips automatic CLI injection when direct injection is disabled", async () => { | ||
| const configModule = await import("../lib/config.js"); | ||
| vi.mocked(configModule.getCodexCliDirectInjection).mockReturnValue(false); | ||
| try { | ||
| globalThis.fetch = vi.fn().mockResolvedValue( | ||
| new Response(JSON.stringify({ content: "test" }), { status: 200 }), | ||
| ); | ||
|
|
||
| const { sdk } = await setupPlugin(); | ||
| const response = await sdk.fetch!("https://api.openai.com/v1/chat", { | ||
| method: "POST", | ||
| body: JSON.stringify({ model: "gpt-5.1" }), | ||
| }); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(syncCodexCliSelectionMock).not.toHaveBeenCalled(); | ||
| } finally { | ||
| vi.mocked(configModule.getCodexCliDirectInjection).mockReturnValue(true); | ||
| } | ||
| }); | ||
|
|
||
| it("skips failover CLI injection when direct injection is disabled", async () => { | ||
| vi.doMock("../lib/request/stream-failover.js", () => ({ | ||
| withStreamingFailover: async ( | ||
| response: Response, | ||
| failover: (attempt: number, emittedBytes: number) => Promise<Response | null>, | ||
| ) => (await failover(1, 128)) ?? response, | ||
| })); | ||
| try { | ||
| vi.resetModules(); | ||
| const configModule = await import("../lib/config.js"); | ||
| vi.mocked(configModule.getCodexCliDirectInjection).mockReturnValue(false); | ||
| const fetchHelpers = await import("../lib/request/fetch-helpers.js"); | ||
| vi.mocked(fetchHelpers.createCodexHeaders).mockImplementation( | ||
| (_init, _accountId, accessToken) => | ||
| new Headers({ "x-test-access-token": String(accessToken) }), | ||
| ); | ||
| const { AccountManager } = await import("../lib/accounts.js"); | ||
| const accountOne = { | ||
| index: 0, | ||
| accountId: "acc-1", | ||
| email: "user1@example.com", | ||
| refreshToken: "refresh-1", | ||
| access: "access-account-1", | ||
| expires: Date.now() + 60_000, | ||
| addedAt: 1, | ||
| }; | ||
| const accountTwo = { | ||
| index: 1, | ||
| accountId: "acc-2", | ||
| email: "user2@example.com", | ||
| refreshToken: "refresh-2", | ||
| access: "access-account-2", | ||
| expires: Date.now() + 60_000, | ||
| addedAt: 2, | ||
| }; | ||
| const customManager = { | ||
| accounts: [accountOne, accountTwo], | ||
| getCurrentOrNextForFamilyHybrid: vi | ||
| .fn() | ||
| .mockReturnValueOnce(accountOne) | ||
| .mockReturnValueOnce(accountTwo) | ||
| .mockImplementation(() => null), | ||
| getAccountCount: () => 2, | ||
| getAccountsSnapshot: () => [accountOne, accountTwo], | ||
| isAccountAvailableForFamily: () => true, | ||
| getAccountByIndex: (index: number) => | ||
| index === 0 ? accountOne : index === 1 ? accountTwo : null, | ||
| toAuthDetails: (account: typeof accountOne) => ({ | ||
| type: "oauth" as const, | ||
| access: account.access, | ||
| refresh: account.refreshToken, | ||
| expires: account.expires, | ||
| multiAccount: true, | ||
| }), | ||
| consumeToken: vi.fn(() => true), | ||
| updateFromAuth: vi.fn(), | ||
| clearAuthFailures: vi.fn(), | ||
| saveToDiskDebounced: vi.fn(), | ||
| saveToDisk: vi.fn(), | ||
| hasRefreshToken: vi.fn(() => true), | ||
| syncCodexCliActiveSelectionForIndex: (index: number) => | ||
| syncCodexCliSelectionMock(index), | ||
| recordFailure: vi.fn(), | ||
| recordSuccess: vi.fn(), | ||
| recordRateLimit: vi.fn(), | ||
| refundToken: vi.fn(), | ||
| markRateLimitedWithReason: vi.fn(), | ||
| markSwitched: vi.fn(), | ||
| shouldShowAccountToast: () => false, | ||
| markToastShown: vi.fn(), | ||
| getMinWaitTimeForFamily: () => 0, | ||
| getCurrentAccountForFamily: () => accountOne, | ||
| }; | ||
| vi.spyOn(AccountManager, "loadFromDisk").mockResolvedValue(customManager as never); | ||
| globalThis.fetch = vi | ||
| .fn() | ||
| .mockResolvedValueOnce(new Response("streaming", { status: 200 })) | ||
| .mockResolvedValueOnce(new Response("retry later", { status: 429 })) | ||
| .mockResolvedValueOnce( | ||
| new Response(JSON.stringify({ content: "fallback-ok" }), { status: 200 }), | ||
| ); | ||
|
|
||
| const { sdk } = await setupPlugin(); | ||
| const response = await sdk.fetch!("https://api.openai.com/v1/chat", { | ||
| method: "POST", | ||
| body: JSON.stringify({ model: "gpt-5.1", stream: true }), | ||
| }); | ||
|
|
||
| expect(response.status).toBe(200); | ||
| expect(syncCodexCliSelectionMock).not.toHaveBeenCalled(); | ||
| } finally { | ||
| const configModule = await import("../lib/config.js"); | ||
| vi.mocked(configModule.getCodexCliDirectInjection).mockReturnValue(true); | ||
| vi.doUnmock("../lib/request/stream-failover.js"); | ||
| vi.resetModules(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
add the disabled-direct-injection refresh regression.
test/index.test.ts:1934 (Line 1934) and test/index.test.ts:1955 (Line 1955) only disable cli injection on the steady-state and failover paths. the forced refresh branches exercised at test/index.test.ts:1456 (Line 1456) and test/index.test.ts:1714 (Line 1714) are never combined with getCodexCliDirectInjection() === false, so a regression where refresh-driven reinjection bypasses the flag would still pass this suite. add one deterministic vitest case with shouldRefreshToken() returning true while direct injection is disabled and assert syncCodexCliSelectionMock never runs.
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.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/index.test.ts` around lines 1934 - 2051, Add a deterministic vitest that
covers the forced-refresh path while direct CLI injection is disabled: mock
config.getCodexCliDirectInjection to return false, mock the function that
triggers refresh (shouldRefreshToken) to return true for the request, ensure
token refresh logic runs (e.g., via AccountManager.loadFromDisk or the same
customManager pattern used elsewhere) and call setupPlugin then sdk.fetch with a
request that would cause a refresh; finally assert syncCodexCliSelectionMock was
not called. Reference getCodexCliDirectInjection, shouldRefreshToken,
syncCodexCliSelectionMock, setupPlugin, and sdk.fetch to locate where to add and
hook the test.
| it("persists account.select when the cache is empty", async () => { | ||
| vi.resetModules(); | ||
| const mockClient = createMockClient(); | ||
| const { OpenAIOAuthPlugin } = await import("../index.js"); | ||
| const plugin = await OpenAIOAuthPlugin({ client: mockClient } as never) as unknown as PluginType; | ||
|
|
||
| mockStorage.accounts = [ | ||
| { accountId: "acc-1", email: "user1@example.com", refreshToken: "refresh-1" }, | ||
| { accountId: "acc-2", email: "user2@example.com", refreshToken: "refresh-2" }, | ||
| ]; | ||
|
|
||
| await plugin.event({ | ||
| event: { type: "account.select", properties: { index: 0 } }, | ||
| }); | ||
|
|
||
| expect(mockStorage.activeIndex).toBe(0); | ||
| expect(mockStorage.activeIndexByFamily["gpt-5.1"]).toBe(0); | ||
| }); |
There was a problem hiding this comment.
use a non-default selection here.
test/index.test.ts:3393 (Line 3393) selects index: 0, which is already the default state established at test/index.test.ts:3266-3271. that means this case only proves activeIndexByFamily gets populated; it does not prove the empty-cache path honors the requested account. selecting the second account would catch a handler that always persists the first entry.
possible tightening
await plugin.event({
- event: { type: "account.select", properties: { index: 0 } },
+ event: { type: "account.select", properties: { index: 1 } },
});
- expect(mockStorage.activeIndex).toBe(0);
- expect(mockStorage.activeIndexByFamily["gpt-5.1"]).toBe(0);
+ expect(mockStorage.activeIndex).toBe(1);
+ expect(mockStorage.activeIndexByFamily["gpt-5.1"]).toBe(1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("persists account.select when the cache is empty", async () => { | |
| vi.resetModules(); | |
| const mockClient = createMockClient(); | |
| const { OpenAIOAuthPlugin } = await import("../index.js"); | |
| const plugin = await OpenAIOAuthPlugin({ client: mockClient } as never) as unknown as PluginType; | |
| mockStorage.accounts = [ | |
| { accountId: "acc-1", email: "user1@example.com", refreshToken: "refresh-1" }, | |
| { accountId: "acc-2", email: "user2@example.com", refreshToken: "refresh-2" }, | |
| ]; | |
| await plugin.event({ | |
| event: { type: "account.select", properties: { index: 0 } }, | |
| }); | |
| expect(mockStorage.activeIndex).toBe(0); | |
| expect(mockStorage.activeIndexByFamily["gpt-5.1"]).toBe(0); | |
| }); | |
| it("persists account.select when the cache is empty", async () => { | |
| vi.resetModules(); | |
| const mockClient = createMockClient(); | |
| const { OpenAIOAuthPlugin } = await import("../index.js"); | |
| const plugin = await OpenAIOAuthPlugin({ client: mockClient } as never) as unknown as PluginType; | |
| mockStorage.accounts = [ | |
| { accountId: "acc-1", email: "user1@example.com", refreshToken: "refresh-1" }, | |
| { accountId: "acc-2", email: "user2@example.com", refreshToken: "refresh-2" }, | |
| ]; | |
| await plugin.event({ | |
| event: { type: "account.select", properties: { index: 1 } }, | |
| }); | |
| expect(mockStorage.activeIndex).toBe(1); | |
| expect(mockStorage.activeIndexByFamily["gpt-5.1"]).toBe(1); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/index.test.ts` around lines 3382 - 3399, The test "persists
account.select when the cache is empty" is selecting index: 0 which is the
default; change the selection to a non-default value (e.g. index: 1) in the
plugin.event call so the test actually verifies the handler persists the
requested account, and update the corresponding expectations from
mockStorage.activeIndex and mockStorage.activeIndexByFamily["gpt-5.1"] to assert
the new index value (1) instead of 0; locate this test by the it(...) block name
and the plugin.event({ event: { type: "account.select", properties: { index: ...
} } }) call to apply the change.
| describe('direct cli injection setting', () => { | ||
| it('defaults to enabled', () => { | ||
| expect(getCodexCliDirectInjection({})).toBe(true); | ||
| }); | ||
|
|
||
| it('prioritizes environment override', () => { | ||
| process.env.CODEX_AUTH_DIRECT_CLI_INJECTION = '0'; | ||
| expect(getCodexCliDirectInjection({ codexCliDirectInjection: true })).toBe(false); | ||
| delete process.env.CODEX_AUTH_DIRECT_CLI_INJECTION; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
cover the config fallback branch for direct cli injection.
test/plugin-config.test.ts:815-825 checks the default and env override, but it never asserts the config leg of the precedence chain. if lib/config.ts:822-828 stopped honoring pluginConfig.codexCliDirectInjection, this suite would still stay green. add one vitest with no env var and { codexCliDirectInjection: false }.
proposed test fix
describe('direct cli injection setting', () => {
it('defaults to enabled', () => {
expect(getCodexCliDirectInjection({})).toBe(true);
});
+
+ it('uses config when no env override is set', () => {
+ delete process.env.CODEX_AUTH_DIRECT_CLI_INJECTION;
+ expect(
+ getCodexCliDirectInjection({ codexCliDirectInjection: false }),
+ ).toBe(false);
+ });
it('prioritizes environment override', () => {
process.env.CODEX_AUTH_DIRECT_CLI_INJECTION = '0';
expect(getCodexCliDirectInjection({ codexCliDirectInjection: true })).toBe(false);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/plugin-config.test.ts` around lines 815 - 825, Add a deterministic
vitest that covers the config-precedence branch of getCodexCliDirectInjection:
with no CODEX_AUTH_DIRECT_CLI_INJECTION env var set and calling
getCodexCliDirectInjection({ codexCliDirectInjection: false }) assert the result
is false; specifically update the test suite in test/plugin-config.test.ts (the
"direct cli injection setting" describe block) to include a case that clears the
env var, calls getCodexCliDirectInjection with codexCliDirectInjection: false,
and asserts false so the code path honoring pluginConfig.codexCliDirectInjection
is exercised.
1 participant
Summary
What Changed
Validation
npm test -- --run test/index.test.ts test/settings-hub-utils.test.ts test/accounts-load-from-disk.test.ts test/plugin-config.test.tsnpm run typechecknpm run buildnpm test(baseline failure:test/storage.test.ts > should fail export when no accounts exist)Docs and Governance Checklist
docs/getting-started.mdupdated: not neededdocs/features.mdupdated: not neededdocs/reference/*pages updated: not neededdocs/upgrade.mdupdated: not neededSECURITY.mdandCONTRIBUTING.mdreviewed for alignmentRisk and Rollback
feature/same-session-cli-rotationif any regression appears, then re-run the same validation sliceAdditional Notes