refactor: extract shared account view helpers (Architecture PR #1)#33
refactor: extract shared account view helpers (Architecture PR #1)#33
Conversation
Introduce a shared account-view helper module and reuse it across the plugin entrypoint and CLI manager to remove duplicated formatting logic while preserving behavior. Add unit/regression tests to lock formatting and active-index fallback behavior. Co-authored-by: Codex <noreply@openai.com>
📝 WalkthroughSeverity: Minor — This is a behavior-preserving refactoring that extracts ~150–220 lines of duplicated account-view formatting and active-index logic into three new shared modules ( Changes OverviewNew Shared Modules Created:
Refactored Files:
Test Coverage:
Summary: Net removal of ~180 lines of duplicated logic; no behavioral changes; per-family active-index tracking and rate-limit formatting now centralized and thoroughly tested. Walkthroughmoved active-index and rate-limit helper logic into new public modules ( Changes
Sequence Diagram(s)(silently skipped) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes observations from a senior staff engineer review
🚥 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 `@lib/accounts/account-view.ts`:
- Around line 35-41: Update the loop that scans Object.entries(times) to skip
non-finite numbers by using Number.isFinite(value) (so NaN/Infinity/-Infinity
are ignored) before comparing to now and updating minReset; this change affects
the code path that later calls formatRateLimitEntry, so ensure
formatRateLimitEntry still receives only finite timestamps. Add regression tests
in test/account-view.test.ts that set rateLimitResetTimes to NaN, Infinity, and
-Infinity and assert the rendered output is well-formed (i.e., those entries are
ignored and do not produce malformed output), referencing the same account-view
logic that computes minReset and formats entries.
- Around line 20-23: The active-index clamp currently can return fractional
values; coerce the chosen rawCandidate to an integer before clamping (use
Math.trunc or equivalent) so storage.activeIndexByFamily and storage.activeIndex
never produce fractional indices; update the normalization in
lib/accounts/account-view.ts where storage.activeIndexByFamily?.[family] ??
storage.activeIndex is processed to first coerce to an integer then apply
Math.max(0, Math.min(..., total - 1)). Add a regression vitest in
test/account-view.test.ts that sets fractional values for activeIndex and
activeIndexByFamily and asserts the returned active index is an integer and
within bounds; ensure the change does not log any sensitive tokens/emails and
mention the new test in the test suite metadata.
In `@test/account-view.test.ts`:
- Around line 9-33: Add a test checking resolveActiveIndex when accounts is an
empty array so the special-case total === 0 behavior is locked in: call
resolveActiveIndex({ activeIndex: X, activeIndexByFamily: {...}?, accounts: []
}) and assert it returns 0; place it alongside the existing tests that exercise
clamping and normalization to verify the function returns 0 for empty accounts
(refer to resolveActiveIndex in account-view tests).
In `@test/codex-manager-cli.test.ts`:
- Line 525: The test currently only asserts the log contains "rate-limited";
update the assertion to specifically verify the formatted reset indicator
produced by formatRateLimitEntry is present (e.g. assert the logged message
includes "resets in" or a regex matching "resets in .*s|m|h"), by changing the
expectation on logSpy.mock.calls (the same array used now) so it checks call[0]
for the reset phrasing; this ensures the lib/accounts/account-view.ts helper
formatRateLimitEntry is exercised instead of a generic substring match.
ℹ️ 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/accounts/account-view.tslib/codex-manager.tstest/account-view.test.tstest/codex-manager-cli.test.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)
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.tstest/account-view.test.tstest/codex-manager-cli.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/account-view.tslib/codex-manager.ts
🧬 Code graph analysis (3)
test/account-view.test.ts (1)
lib/accounts/account-view.ts (3)
resolveActiveIndex(14-23)getRateLimitResetTimeForFamily(25-45)formatRateLimitEntry(47-57)
lib/accounts/account-view.ts (1)
lib/prompts/codex.ts (1)
ModelFamily(47-52)
test/codex-manager-cli.test.ts (3)
scripts/benchmark-runtime-path.mjs (1)
now(84-84)scripts/codex.js (3)
exitCode(505-505)exitCode(532-532)runCodexMultiAuthCli(501-501)lib/codex-manager.ts (1)
runCodexMultiAuthCli(4001-4062)
🔇 Additional comments (5)
index.ts (1)
120-124: shared helper import wiring looks good.the extraction hook-up is clean, and it keeps behavior centralized in
lib/accounts/account-view.ts:14-57. no new windows fs or concurrency risk is introduced by this import-only change.lib/codex-manager.ts (1)
25-28: import refactor is consistent and safe.this import-only change cleanly reuses shared helpers from
lib/accounts/account-view.ts:14-57. no new windows fs edge case or concurrency path is introduced here. affected behavior is expected to be covered bytest/account-view.test.tsandtest/codex-manager-cli.test.ts.As per coding guidelines, "lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails."
test/index.test.ts (1)
734-747: lgtm - deterministic timing for rate-limit formatting test.good refactor to capture
nowupfront attest/index.test.ts:734and derive time-based fields from it. the assertion at line 747 properly locks the "resets in" formatting behavior fromlib/accounts/account-view.ts:46-56.test/account-view.test.ts (2)
35-68: good coverage of family prefix matching and edge cases.the test at
test/account-view.test.ts:43correctly verifies that"gpt-5.1"key is excluded when querying"codex"family, exercising the prefix logic atlib/accounts/account-view.ts:36-37.
70-92: lgtm - locks the human-readable output format.deterministic test with hardcoded timestamps. the assertion
"resets in 1m 30s"at line 81 ensuresformatWaitTimeintegration fromlib/accounts/account-view.ts:55produces expected output.
| const rawCandidate = storage.activeIndexByFamily?.[family] ?? storage.activeIndex; | ||
| const raw = Number.isFinite(rawCandidate) ? rawCandidate : 0; | ||
| return Math.max(0, Math.min(raw, total - 1)); | ||
| } |
There was a problem hiding this comment.
normalize active index to an integer before clamping.
line 22 in lib/accounts/account-view.ts can return a fractional index (for example 1.7). callers treat this as an array index in lib/codex-manager.ts:3340-3344 and lib/codex-manager.ts:3937-3944, which can produce undefined active-account reads.
proposed fix
export function resolveActiveIndex(
storage: ActiveAccountStorageView,
family: ModelFamily = "codex",
): number {
const total = storage.accounts.length;
if (total === 0) return 0;
const rawCandidate = storage.activeIndexByFamily?.[family] ?? storage.activeIndex;
- const raw = Number.isFinite(rawCandidate) ? rawCandidate : 0;
- return Math.max(0, Math.min(raw, total - 1));
+ const raw = Number.isFinite(rawCandidate) ? Math.floor(rawCandidate) : 0;
+ return Math.max(0, Math.min(raw, total - 1));
}please also add a regression in test/account-view.test.ts for fractional activeIndex and activeIndexByFamily values.
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."
📝 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.
| const rawCandidate = storage.activeIndexByFamily?.[family] ?? storage.activeIndex; | |
| const raw = Number.isFinite(rawCandidate) ? rawCandidate : 0; | |
| return Math.max(0, Math.min(raw, total - 1)); | |
| } | |
| const rawCandidate = storage.activeIndexByFamily?.[family] ?? storage.activeIndex; | |
| const raw = Number.isFinite(rawCandidate) ? Math.floor(rawCandidate) : 0; | |
| return Math.max(0, Math.min(raw, total - 1)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/accounts/account-view.ts` around lines 20 - 23, The active-index clamp
currently can return fractional values; coerce the chosen rawCandidate to an
integer before clamping (use Math.trunc or equivalent) so
storage.activeIndexByFamily and storage.activeIndex never produce fractional
indices; update the normalization in lib/accounts/account-view.ts where
storage.activeIndexByFamily?.[family] ?? storage.activeIndex is processed to
first coerce to an integer then apply Math.max(0, Math.min(..., total - 1)). Add
a regression vitest in test/account-view.test.ts that sets fractional values for
activeIndex and activeIndexByFamily and asserts the returned active index is an
integer and within bounds; ensure the change does not log any sensitive
tokens/emails and mention the new test in the test suite metadata.
| for (const [key, value] of Object.entries(times)) { | ||
| if (typeof value !== "number") continue; | ||
| if (value <= now) continue; | ||
| if (key !== family && !key.startsWith(prefix)) continue; | ||
| if (minReset === null || value < minReset) { | ||
| minReset = value; | ||
| } |
There was a problem hiding this comment.
filter out non-finite reset timestamps before selecting minimum.
line 36 currently accepts NaN/Infinity because they are still number. that can flow into formatRateLimitEntry (lib/accounts/account-view.ts:52-56) and generate malformed output.
proposed fix
for (const [key, value] of Object.entries(times)) {
- if (typeof value !== "number") continue;
+ if (typeof value !== "number" || !Number.isFinite(value)) continue;
if (value <= now) continue;
if (key !== family && !key.startsWith(prefix)) continue;
if (minReset === null || value < minReset) {
minReset = value;
}
}please add regression coverage in test/account-view.test.ts for NaN, Infinity, and -Infinity values inside rateLimitResetTimes.
As per coding guidelines, "lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails."
Also applies to: 52-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/accounts/account-view.ts` around lines 35 - 41, Update the loop that
scans Object.entries(times) to skip non-finite numbers by using
Number.isFinite(value) (so NaN/Infinity/-Infinity are ignored) before comparing
to now and updating minReset; this change affects the code path that later calls
formatRateLimitEntry, so ensure formatRateLimitEntry still receives only finite
timestamps. Add regression tests in test/account-view.test.ts that set
rateLimitResetTimes to NaN, Infinity, and -Infinity and assert the rendered
output is well-formed (i.e., those entries are ignored and do not produce
malformed output), referencing the same account-view logic that computes
minReset and formats entries.
| it("resolves active index from family mapping and clamps to bounds", () => { | ||
| expect( | ||
| resolveActiveIndex({ | ||
| activeIndex: 9, | ||
| activeIndexByFamily: { codex: 4 }, | ||
| accounts: [{}, {}], | ||
| }), | ||
| ).toBe(1); | ||
| }); | ||
|
|
||
| it("falls back to storage active index and normalizes non-finite candidates", () => { | ||
| expect( | ||
| resolveActiveIndex({ | ||
| activeIndex: Number.NaN, | ||
| activeIndexByFamily: { codex: Number.NaN }, | ||
| accounts: [{}, {}, {}], | ||
| }), | ||
| ).toBe(0); | ||
| expect( | ||
| resolveActiveIndex({ | ||
| activeIndex: 2, | ||
| accounts: [{}, {}, {}], | ||
| }), | ||
| ).toBe(2); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
consider adding edge case for empty accounts array.
the resolveActiveIndex impl at lib/accounts/account-view.ts:16-17 has special handling for total === 0 returning 0. adding a regression test for this edge case would lock that behavior.
🧪 proposed test case for empty accounts
it("falls back to storage active index and normalizes non-finite candidates", () => {
+ expect(
+ resolveActiveIndex({
+ activeIndex: 5,
+ activeIndexByFamily: { codex: 3 },
+ accounts: [],
+ }),
+ ).toBe(0);
expect(
resolveActiveIndex({
activeIndex: Number.NaN,📝 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("resolves active index from family mapping and clamps to bounds", () => { | |
| expect( | |
| resolveActiveIndex({ | |
| activeIndex: 9, | |
| activeIndexByFamily: { codex: 4 }, | |
| accounts: [{}, {}], | |
| }), | |
| ).toBe(1); | |
| }); | |
| it("falls back to storage active index and normalizes non-finite candidates", () => { | |
| expect( | |
| resolveActiveIndex({ | |
| activeIndex: Number.NaN, | |
| activeIndexByFamily: { codex: Number.NaN }, | |
| accounts: [{}, {}, {}], | |
| }), | |
| ).toBe(0); | |
| expect( | |
| resolveActiveIndex({ | |
| activeIndex: 2, | |
| accounts: [{}, {}, {}], | |
| }), | |
| ).toBe(2); | |
| }); | |
| it("resolves active index from family mapping and clamps to bounds", () => { | |
| expect( | |
| resolveActiveIndex({ | |
| activeIndex: 9, | |
| activeIndexByFamily: { codex: 4 }, | |
| accounts: [{}, {}], | |
| }), | |
| ).toBe(1); | |
| }); | |
| it("falls back to storage active index and normalizes non-finite candidates", () => { | |
| expect( | |
| resolveActiveIndex({ | |
| activeIndex: 5, | |
| activeIndexByFamily: { codex: 3 }, | |
| accounts: [], | |
| }), | |
| ).toBe(0); | |
| expect( | |
| resolveActiveIndex({ | |
| activeIndex: Number.NaN, | |
| activeIndexByFamily: { codex: Number.NaN }, | |
| accounts: [{}, {}, {}], | |
| }), | |
| ).toBe(0); | |
| expect( | |
| resolveActiveIndex({ | |
| activeIndex: 2, | |
| accounts: [{}, {}, {}], | |
| }), | |
| ).toBe(2); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/account-view.test.ts` around lines 9 - 33, Add a test checking
resolveActiveIndex when accounts is an empty array so the special-case total ===
0 behavior is locked in: call resolveActiveIndex({ activeIndex: X,
activeIndexByFamily: {...}?, accounts: [] }) and assert it returns 0; place it
alongside the existing tests that exercise clamping and normalization to verify
the function returns 0 for empty accounts (refer to resolveActiveIndex in
account-view tests).
|
|
||
| const exitCode = await runCodexMultiAuthCli(["auth", "status"]); | ||
| expect(exitCode).toBe(0); | ||
| expect(logSpy.mock.calls.some((call) => String(call[0]).includes("rate-limited"))).toBe(true); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
assertion could be more specific to verify the new formatRateLimitEntry behavior.
the test only checks for "rate-limited" substring, but the pr objective is to verify rate-limit reset formatting (the "resets in" indicator). per test/index.test.ts:747, the companion test explicitly asserts "resets in". consider strengthening this assertion to confirm the formatRateLimitEntry helper from lib/accounts/account-view.ts:46-56 is exercised.
🔧 proposed fix to verify reset formatting
- expect(logSpy.mock.calls.some((call) => String(call[0]).includes("rate-limited"))).toBe(true);
+ expect(logSpy.mock.calls.some((call) => String(call[0]).includes("rate-limited"))).toBe(true);
+ expect(logSpy.mock.calls.some((call) => String(call[0]).includes("resets in"))).toBe(true);📝 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.
| expect(logSpy.mock.calls.some((call) => String(call[0]).includes("rate-limited"))).toBe(true); | |
| expect(logSpy.mock.calls.some((call) => String(call[0]).includes("rate-limited"))).toBe(true); | |
| expect(logSpy.mock.calls.some((call) => String(call[0]).includes("resets in"))).toBe(true); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/codex-manager-cli.test.ts` at line 525, The test currently only asserts
the log contains "rate-limited"; update the assertion to specifically verify the
formatted reset indicator produced by formatRateLimitEntry is present (e.g.
assert the logged message includes "resets in" or a regex matching "resets in
.*s|m|h"), by changing the expectation on logSpy.mock.calls (the same array used
now) so it checks call[0] for the reset phrasing; this ensures the
lib/accounts/account-view.ts helper formatRateLimitEntry is exercised instead of
a generic substring match.
Add shared helpers for per-family active-index map creation and bulk updates, then reuse them across plugin, CLI manager, and codex-cli sync flows to reduce duplicated state-mutation logic. Extend tests to lock switch and selection behavior for all model-family indexes. Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/codex-manager.ts (1)
3628-3633:⚠️ Potential issue | 🟠 Majordeleting any account currently forces active selection to account 1.
at Line 3632 (
lib/codex-manager.ts:3632), the delete path resets all family indexes to0unconditionally. deleting a non-active account should preserve the active selection (or shift it by one when deleting a preceding row). this creates an unexpected account switch.i also do not see a targeted vitest regression for this manage-delete path in the changed tests (
test/index.test.ts:699-753,1914-1918focus switch/status/event flows).proposed fix
if (typeof menuResult.deleteAccountIndex === "number") { const idx = menuResult.deleteAccountIndex; if (idx >= 0 && idx < storage.accounts.length) { + const previousActive = resolveActiveIndex(storage, "codex"); storage.accounts.splice(idx, 1); - setActiveIndexForAllFamilies(storage, 0); + const nextActive = + storage.accounts.length === 0 + ? 0 + : idx < previousActive + ? previousActive - 1 + : Math.min(previousActive, storage.accounts.length - 1); + setActiveIndexForAllFamilies(storage, nextActive); await saveAccounts(storage); console.log(`Deleted account ${idx + 1}.`); } return; }as per coding guidelines,
lib/**: "verify every change cites affected tests (vitest)" andtest/**: "demand regression cases ... and reject changes that ... skip assertions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 3628 - 3633, The delete path currently calls setActiveIndexForAllFamilies(storage, 0) unconditionally which forces every family's active account to index 0; change the logic around menuResult.deleteAccountIndex so you compute and apply a new active index per family instead: for each family use its current activeIndex (or 0 if missing) and if the deleted index < currentActiveIndex decrement it by 1, if deleted index === currentActiveIndex pick a sensible fallback (e.g., max(0, currentActiveIndex - 1) or the next valid index) otherwise leave it unchanged, then call a helper (or update setActiveIndexForAllFamilies to accept a per-family mapping) and await saveAccounts(storage); also add a vitest regression that deletes a non-active account and asserts active selection is preserved/shifted correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/codex-manager.ts`:
- Around line 3628-3633: The delete path currently calls
setActiveIndexForAllFamilies(storage, 0) unconditionally which forces every
family's active account to index 0; change the logic around
menuResult.deleteAccountIndex so you compute and apply a new active index per
family instead: for each family use its current activeIndex (or 0 if missing)
and if the deleted index < currentActiveIndex decrement it by 1, if deleted
index === currentActiveIndex pick a sensible fallback (e.g., max(0,
currentActiveIndex - 1) or the next valid index) otherwise leave it unchanged,
then call a helper (or update setActiveIndexForAllFamilies to accept a
per-family mapping) and await saveAccounts(storage); also add a vitest
regression that deletes a non-active account and asserts active selection is
preserved/shifted correctly.
ℹ️ 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/accounts/active-index.tslib/codex-cli/sync.tslib/codex-manager.tstest/active-index.test.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/accounts/active-index.tslib/codex-cli/sync.tslib/codex-manager.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/active-index.test.tstest/index.test.ts
🧬 Code graph analysis (5)
lib/accounts/active-index.ts (1)
lib/prompts/codex.ts (2)
ModelFamily(47-52)MODEL_FAMILIES(58-64)
lib/codex-cli/sync.ts (1)
lib/accounts/active-index.ts (1)
setActiveIndexForAllFamilies(19-29)
lib/codex-manager.ts (1)
lib/accounts/active-index.ts (2)
createActiveIndexByFamily(8-17)setActiveIndexForAllFamilies(19-29)
index.ts (1)
lib/accounts/active-index.ts (1)
setActiveIndexForAllFamilies(19-29)
test/index.test.ts (2)
lib/prompts/codex.ts (1)
MODEL_FAMILIES(58-64)scripts/benchmark-runtime-path.mjs (1)
now(84-84)
🔇 Additional comments (9)
lib/accounts/active-index.ts (1)
8-29: helper extraction is clean and behavior-preserving.the new shared helpers in
lib/accounts/active-index.ts:8-29are straightforward and keep mutation scope tight. coverage exists intest/active-index.test.ts:9-53, and this change does not add windows io, queue behavior, or sensitive logging paths.as per coding guidelines,
lib/**: "verify every change cites affected tests (vitest) ... check for logging that leaks tokens or emails."lib/codex-cli/sync.ts (1)
7-7: delegation to shared active-index helper looks correct.
lib/codex-cli/sync.ts:162-167now routes throughsetActiveIndexForAllFamilies, which keeps this path aligned with the centralized behavior fromlib/accounts/active-index.ts:19-29. no new windows fs edge case or concurrency surface is introduced by this change.Also applies to: 162-167
test/active-index.test.ts (1)
8-54: test coverage here is strong and deterministic.
test/active-index.test.ts:8-54locks the core regressions for family map creation/update and object reuse behavior with clear assertions.as per coding guidelines,
test/**: "tests must stay deterministic and use vitest ... reject changes that ... skip assertions."lib/codex-manager.ts (1)
25-32: shared helper adoption is consistent in create/switch paths.the migrations at
lib/codex-manager.ts:1338,lib/codex-manager.ts:2349-2355, andlib/codex-manager.ts:3853correctly consolidate per-family index writes and keep the logic aligned withlib/accounts/active-index.ts.Also applies to: 1338-1338, 2349-2355, 3853-3853
test/index.test.ts (1)
699-703: nice regression strengthening for family-wide active index behavior.the added assertions in
test/index.test.ts:699-703andtest/index.test.ts:1914-1918correctly verify that all model families move together on switch/select flows. the status-case setup attest/index.test.ts:739-753also improves timing stability for rate-limit output checks.as per coding guidelines,
test/**: "tests must stay deterministic and use vitest. demand regression cases ...".Also applies to: 739-753, 1914-1918
index.ts (4)
120-125: imports look correct and all are used.verified that
resolveActiveIndex,getRateLimitResetTimeForFamily, andformatRateLimitEntryfromlib/accounts/account-view.jsare used throughout the file (lines 3030, 3388, 3403, 3447, 3572, 3586, 3610).setActiveIndexForAllFamiliesfromlib/accounts/active-index.jsis used at lines 959 and 3522.
959-959: correct usage of setActiveIndexForAllFamilies in event handler.storage null check at
index.ts:949guards this call. the helper sets bothstorage.activeIndexand populatesstorage.activeIndexByFamilyfor all model families, matching the previous inline behavior.
3522-3522: correct usage of setActiveIndexForAllFamilies in codex-switch tool.storage null check at
index.ts:3487andtargetIndexbounds validation atindex.ts:3499-3513guard this call. matches the extraction pattern described in pr objectives.
120-125: test coverage is adequate.test/active-index.test.ts:11-37 covers setActiveIndexForAllFamilies with the relevant scenarios: initializing activeIndexByFamily when missing, setting across all families, and preserving unrelated properties. the edge cases you mentioned (empty accounts array, index at bounds) don't apply here—this function doesn't take an accounts parameter or perform bounds checking. test/account-view.test.ts:9-92 properly covers the three imported helpers (resolveActiveIndex, getRateLimitResetTimeForFamily, formatRateLimitEntry) with boundary and fallback cases.
Extract active-index normalization into a shared helper and reuse it in plugin account-flow clamping, codex-cli sync, and doctor normalization paths. Add focused tests covering bounded normalization and empty-account map handling to preserve behavior across both clear and fill modes. Co-authored-by: Codex <noreply@openai.com>
Extract model-family rate-limit status rendering into account-view helpers and replace duplicate status mapping logic in the plugin status output paths. Add unit coverage for mixed limited/ok family labels to protect output behavior. Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/accounts/active-index.ts`:
- Around line 13-16: The clampIndex helper and the state-write paths that call
it are currently allowing fractional and non-finite numbers to persist; coerce
incoming index values to finite integers at the shared helper boundary by
validating with Number.isFinite and using Math.trunc(Number(index)) (or 0 for
non-finite values) before clamping. Update clampIndex to accept any numeric
input, normalize it to a finite integer first, then apply the existing clamp
logic, and likewise normalize inputs in the other state-write helpers referenced
(the functions that call clampIndex around the active-index logic) so only
integer indexes are ever saved.
ℹ️ 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/accounts/active-index.tslib/codex-cli/sync.tslib/codex-manager.tstest/active-index.test.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)
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/active-index.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-cli/sync.tslib/accounts/active-index.tslib/codex-manager.ts
🧬 Code graph analysis (5)
lib/codex-cli/sync.ts (1)
lib/accounts/active-index.ts (2)
setActiveIndexForAllFamilies(29-39)normalizeActiveIndexByFamily(41-82)
test/index.test.ts (1)
lib/prompts/codex.ts (1)
MODEL_FAMILIES(58-64)
index.ts (1)
lib/accounts/active-index.ts (2)
setActiveIndexForAllFamilies(29-39)normalizeActiveIndexByFamily(41-82)
lib/accounts/active-index.ts (1)
lib/prompts/codex.ts (2)
ModelFamily(47-52)MODEL_FAMILIES(58-64)
lib/codex-manager.ts (1)
lib/accounts/active-index.ts (3)
createActiveIndexByFamily(18-27)normalizeActiveIndexByFamily(41-82)setActiveIndexForAllFamilies(29-39)
🔇 Additional comments (12)
index.ts (1)
120-128: good reuse of shared account-index helpers.this is a clean extraction and the call sites are now consistent. coverage is in place for switch/event propagation in
test/index.test.ts:693,test/index.test.ts:699, andtest/index.test.ts:1918.As per coding guidelines,
lib/**: "verify every change cites affected tests (vitest)".Also applies to: 962-963, 2391-2393, 3514-3514
test/index.test.ts (1)
699-703: good regression updates for extracted index behavior.the new assertions pin per-family propagation and empty-map reset behavior, and the status check avoids brittle exact-time assertions.
references:
test/index.test.ts:699,test/index.test.ts:739,test/index.test.ts:811,test/index.test.ts:1918.As per coding guidelines,
test/**: "tests must stay deterministic and use vitest."Also applies to: 739-753, 806-813, 1918-1922
test/active-index.test.ts (1)
9-118: solid baseline coverage for shared active-index helpers.these tests lock the important normalization paths (nan, bounds, clear/fill empty semantics) and match the extracted helper surface.
references:
test/active-index.test.ts:56,test/active-index.test.ts:80,test/active-index.test.ts:101.lib/codex-manager.ts (6)
25-33: imports look correct; helpers are now centralized.the new imports from
lib/accounts/account-view.jsandlib/accounts/active-index.jsalign with the extracted utilities. no token-handling or filesystem code introduced here.
1338-1338: usage ofcreateActiveIndexByFamilyinpersistAccountPoolis correct.this replaces inline object construction and produces
Partial<Record<ModelFamily, number>>as expected. the helper iteratesMODEL_FAMILIESinternally, so all families receive the same index.
2348-2355:createEmptyAccountStoragecorrectly initializes per-family indexes.using
createActiveIndexByFamily(0)ensures the empty storage starts with all families pointing to index 0, matching the previous inline behavior.
3053-3055:normalizeDoctorIndexesdelegates to the shared helper.
normalizeActiveIndexByFamily(storage, storage.accounts.length)handles nan normalization, out-of-bounds clamping, and empty-account-map handling. the helper returns a boolean indicating whether mutations occurred, which is the expected return type here.
3614-3614:setActiveIndexForAllFamilies(storage, 0)after account deletion.resetting to index 0 when an account is deleted is consistent with prior behavior. the helper mutates
activeIndexand each family entry in-place.
3835-3835:setActiveIndexForAllFamilies(storage, targetIndex)inrunSwitch.the switch command now uses the shared helper to update the global and per-family indexes atomically. this is behavior-preserving and removes ~5 lines of inline duplication.
lib/codex-cli/sync.ts (3)
6-10: imports updated to use shared active-index helpers.removing
MODEL_FAMILIESand importingsetActiveIndexForAllFamilies/normalizeActiveIndexByFamilyfromlib/accounts/active-index.jsis consistent with the refactor. the type-only import forModelFamilyis retained forgetActiveSelectionForFamily:line 354.
165-170:writeFamilyIndexesnow delegates to the shared helper.
setActiveIndexForAllFamilies(storage, index)performs the same mutations: setsstorage.activeIndex, initializesactiveIndexByFamilyif missing, and writes each family. this is behavior-preserving.one consideration:
writeFamilyIndexesis now a single-line wrapper. if no other callers exist, you could inline it atlib/codex-cli/sync.ts:303andlib/codex-cli/sync.ts:314(vianormalizeStoredFamilyIndexesflow). not blocking, just a follow-up simplification.
187-189:normalizeStoredFamilyIndexesdelegates tonormalizeActiveIndexByFamily.the helper handles:
- clamping
activeIndexto valid range- nan fallback to
storage.activeIndex- per-family index normalization
the
optionsparam defaults correctly (noclearFamilyMapWhenEmpty), matching the old inline semantics. the docstring above (lines 172-186) documents concurrency and windows filesystem considerations, which satisfies thelib/**guideline for citing edge cases.
| function clampIndex(index: number, count: number): number { | ||
| if (count <= 0) return 0; | ||
| return Math.max(0, Math.min(index, count - 1)); | ||
| } |
There was a problem hiding this comment.
normalize indexes to finite integers before writing state.
line 13 and line 34 currently allow fractional values to persist (for example 1.5), which can corrupt active selection semantics across families. please coerce to integers at the shared helper boundary.
references: lib/accounts/active-index.ts:13, lib/accounts/active-index.ts:34, lib/accounts/active-index.ts:73.
proposed fix
function clampIndex(index: number, count: number): number {
- if (count <= 0) return 0;
- return Math.max(0, Math.min(index, count - 1));
+ if (count <= 0) return 0;
+ const normalized = Number.isFinite(index) ? Math.trunc(index) : 0;
+ return Math.max(0, Math.min(normalized, count - 1));
}
@@
export function setActiveIndexForAllFamilies(
storage: ActiveIndexFamilyStorage,
index: number,
families: readonly ModelFamily[] = MODEL_FAMILIES,
): void {
- storage.activeIndex = index;
+ const normalized = Number.isFinite(index) ? Math.max(0, Math.trunc(index)) : 0;
+ storage.activeIndex = normalized;
storage.activeIndexByFamily = storage.activeIndexByFamily ?? {};
for (const family of families) {
- storage.activeIndexByFamily[family] = index;
+ storage.activeIndexByFamily[family] = normalized;
}
}Also applies to: 29-38, 50-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/accounts/active-index.ts` around lines 13 - 16, The clampIndex helper and
the state-write paths that call it are currently allowing fractional and
non-finite numbers to persist; coerce incoming index values to finite integers
at the shared helper boundary by validating with Number.isFinite and using
Math.trunc(Number(index)) (or 0 for non-finite values) before clamping. Update
clampIndex to accept any numeric input, normalize it to a finite integer first,
then apply the existing clamp logic, and likewise normalize inputs in the other
state-write helpers referenced (the functions that call clampIndex around the
active-index logic) so only integer indexes are ever saved.
Move active-index-by-family label rendering into shared account-view helpers and reuse it in both v2 and legacy codex status output paths. Add helper tests to lock 1-based labels and placeholder output for missing families. Co-authored-by: Codex <noreply@openai.com>
Move codex-remove index/family reindexing into shared active-index helpers to centralize the mutation policy and reduce inline branching in the plugin entrypoint. Add helper tests for in-range and out-of-range removal behavior. Co-authored-by: Codex <noreply@openai.com>
Introduce shared account-storage view helpers for creating empty v3 storage and cloning mutable working copies, then reuse them across plugin login/check flow, codex-cli sync, and manager reset paths. Add unit tests to lock clone isolation and family-index initialization behavior. Co-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
index.ts (1)
955-968:⚠️ Potential issue | 🟠 Majorcoerce event index to an integer before persisting selection.
this path accepts any numeric value from event properties, so
1.7can pass bounds checks and then be written throughlib/accounts/active-index.ts:33-42. please normalize to an integer before validation and add a regression intest/index.test.tsfor fractional selection events.proposed fix
-const index = props.index ?? props.accountIndex; -if (typeof index === "number") { +const rawIndex = props.index ?? props.accountIndex; +const index = + typeof rawIndex === "number" && Number.isFinite(rawIndex) + ? Math.trunc(rawIndex) + : Number.NaN; +if (Number.isInteger(index)) { const storage = await loadAccounts(); if (!storage || index < 0 || index >= storage.accounts.length) { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.ts` around lines 955 - 968, The event index (props.index or props.accountIndex) must be coerced to an integer before validation and persistence: read the raw value from props.index ?? props.accountIndex, convert it using Math.trunc or Number.parseInt (ensuring NaN is handled), then perform the bounds checks against storage.accounts.length and use that integer when updating account.lastUsed/lastSwitchReason and calling setActiveIndexForAllFamilies; also add a regression test in test/index.test.ts that sends a fractional index (e.g., 1.7) and asserts the stored/active index is the truncated integer (1) and out-of-range fractional values are rejected.
♻️ Duplicate comments (3)
lib/accounts/active-index.ts (1)
17-20:⚠️ Potential issue | 🟠 Majornormalize indices to finite integers before writing state.
lib/accounts/active-index.ts:17-20andlib/accounts/active-index.ts:38-42still persist fractional/non-finite values. this can poisonactiveIndexand family maps withNaN/decimals. please normalize at the helper boundary and add vitest regressions intest/active-index.test.ts.proposed fix
function clampIndex(index: number, count: number): number { if (count <= 0) return 0; - return Math.max(0, Math.min(index, count - 1)); + const normalized = Number.isFinite(index) ? Math.trunc(index) : 0; + return Math.max(0, Math.min(normalized, count - 1)); } @@ ): void { - storage.activeIndex = index; + const normalized = Number.isFinite(index) ? Math.max(0, Math.trunc(index)) : 0; + storage.activeIndex = normalized; storage.activeIndexByFamily = storage.activeIndexByFamily ?? {}; for (const family of families) { - storage.activeIndexByFamily[family] = index; + storage.activeIndexByFamily[family] = normalized; } }As per coding guidelines, "lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails."
Also applies to: 33-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/accounts/active-index.ts` around lines 17 - 20, clampIndex currently allows fractional or non-finite numbers to pass through; normalize inputs at the helper boundary by coercing index and count to finite integers (e.g., via Number.isFinite check and Math.floor or Math.trunc) inside clampIndex and the analogous helper used around lines ~38-42 so activeIndex/state maps never receive NaN or decimals; add vitest cases in test/active-index.test.ts covering NaN, Infinity, negative decimals and fractional indices to assert they are clamped to valid integer bounds and that non-finite inputs default safely.lib/accounts/account-view.ts (2)
37-40:⚠️ Potential issue | 🟠 Majorfilter non-finite reset timestamps before selecting minimum.
lib/accounts/account-view.ts:37-40acceptsNaN/Infinity/-Infinitybecause it only checkstypeof value === "number". that can propagate malformed wait labels throughlib/accounts/account-view.ts:54-70. add finite checks and regressions intest/account-view.test.tsfor these values.proposed fix
for (const [key, value] of Object.entries(times)) { - if (typeof value !== "number") continue; + if (typeof value !== "number" || !Number.isFinite(value)) continue; if (value <= now) continue; if (key !== family && !key.startsWith(prefix)) continue;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/accounts/account-view.ts` around lines 37 - 40, The loop that filters entries from the times object (checking typeof value === "number") should also exclude non-finite numbers; update the condition in the for (const [key, value] of Object.entries(times)) block to skip values where !Number.isFinite(value) in addition to the existing checks for now and key/prefix/family, and ensure the subsequent logic that computes the minimum wait (the code around the current selection/aggregation used later in this module) will only see finite numbers; add vitest regressions in test/account-view.test.ts that insert NaN, Infinity, and -Infinity into the times map and assert they are ignored (no malformed wait labels produced) and that valid finite timestamps still determine behavior.
22-24:⚠️ Potential issue | 🟠 Majornormalize active index to an integer before clamping.
lib/accounts/account-view.ts:22-24still allows fractional values (for example1.7), which can lead to invalid array access in downstream callers. please coerce to an integer before clamp and add regression coverage intest/account-view.test.ts.proposed fix
const rawCandidate = storage.activeIndexByFamily?.[family] ?? storage.activeIndex; -const raw = Number.isFinite(rawCandidate) ? rawCandidate : 0; +const raw = Number.isFinite(rawCandidate) ? Math.trunc(rawCandidate) : 0; return Math.max(0, Math.min(raw, total - 1));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/accounts/account-view.ts` around lines 22 - 24, The value rawCandidate (from storage.activeIndexByFamily?.[family] ?? storage.activeIndex) must be coerced to an integer before clamping to avoid fractional indices: update the computation of raw to use an integer coercion (e.g., Math.trunc or Math.floor) only when Number.isFinite(rawCandidate) is true, falling back to 0 otherwise, then apply the existing Math.max/Math.min clamp; update the logic that assigns raw (symbol names: rawCandidate and raw in account-view.ts) and add a vitest regression in test/account-view.test.ts that passes a fractional active index (e.g., 1.7) and asserts it is truncated and clamped correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/accounts/active-index.ts`:
- Around line 93-95: The range check around targetIndex in
lib/accounts/active-index.ts currently allows non-integer values (e.g., 1.2)
which splice will coerce; update the guard in the function that computes/removes
by index (the targetIndex check) to first reject non-integers using
Number.isInteger(targetIndex) before the existing bounds check, returning false
for non-integer or out-of-range values, and add a regression test in
test/active-index.test.ts that calls the same removal/path with a non-integer
index (e.g., 1.2) and asserts no account was removed (false return or unchanged
array) to prevent future regressions.
In `@lib/accounts/storage-view.ts`:
- Line 23: The current accounts mapping in storage-view.ts uses a shallow copy
(accounts: storage.accounts.map((account) => ({ ...account }))) which leaves
nested mutable fields shared; replace that shallow clone with a deep-clone of
each account so nested objects/maps/arrays are duplicated (manually clone
specific mutable fields or use a safe deep-copy helper) and ensure any Map/Set
are re-created rather than copied by reference; update the test at
test/account-storage-view.test.ts (around lines 25-41) to assert that mutating
nested fields on the returned view does not change the original
storage.accounts, and add coverage for nested Map/Set/array mutation cases.
In `@lib/codex-manager.ts`:
- Around line 3588-3590: clearAccountsAndReset currently calls
createEmptyAccountStorageBase directly which bypasses the local wrapper used
elsewhere and risks diverging defaults; change clearAccountsAndReset to build
the empty storage via the same local wrapper used at the earlier call site (the
project’s empty-storage constructor wrapper) and pass that result to
saveAccounts instead of calling createEmptyAccountStorageBase directly so
shape/defaults remain consistent (i.e., replace the
createEmptyAccountStorageBase usage in clearAccountsAndReset with the local
wrapper used elsewhere, then call saveAccounts with that value).
- Around line 3605-3607: The deletion currently calls
setActiveIndexForAllFamilies(storage, 0) which resets every family's active
index to 0; instead update only families affected by the removed account:
compute removedIdx (the idx being spliced) and for each family in
storage.families, if family.activeIndex === removedIdx set it to
Math.min(removedIdx, storage.accounts.length - 1) (or 0 if no accounts remain),
else if family.activeIndex > removedIdx decrement it by 1; then call
saveAccounts(storage). Modify the call site (where storage.accounts.splice(idx,
1) occurs) or extend setActiveIndexForAllFamilies to accept removedIdx and apply
this logic so only impacted families' active indices change.
- Line 3589: Add vitest regression tests in test/codex-manager-cli.test.ts that
exercise manager-level flows: call clearAccountsAndReset and verify family-index
initialization and persistence across storage restarts; simulate delete actions
via handleManageAction (which invokes setActiveIndexForAllFamilies) and assert
correct reindexing when the active selection becomes invalid; run runSwitch
(which also uses setActiveIndexForAllFamilies) concurrently with delete and with
simulated EBUSY file-write contention to validate retry mechanics and ordering
against persisted state; use helpers to orchestrate concurrent operations and
persistence reloads to confirm invariants hold across process/storage
boundaries.
In `@test/account-storage-view.test.ts`:
- Around line 25-41: Add a nested-field mutation assertion to the existing clone
isolation test by extending the original object with a nested mutable structure
(e.g., a rateLimits map or nested object) and then verifying after calling
cloneAccountStorage that mutating the nested field on the clone does not change
the original; specifically update the test that uses cloneAccountStorage (from
lib/accounts/storage-view.ts) to include a nested map under accounts[0] or
top-level (e.g., rateLimits: { window: { limit: 10 } }), clone it, mutate the
nested value on the clone, and add an expect asserting the original's nested
value is unchanged to ensure cloneAccountStorage performs a deep (not shallow)
copy.
In `@test/account-view.test.ts`:
- Around line 21-35: Add deterministic regression tests in
test/account-view.test.ts that assert resolveActiveIndex correctly coerces
fractional activeIndex values (e.g., 1.5 or Number.POSITIVE_INFINITY) down to a
valid integer index and falls back to storage/length when given NaN, and also
add tests that pass non-finite values (Number.NaN, Number.POSITIVE_INFINITY,
Number.NEGATIVE_INFINITY) into the rateLimitResetTimes input exercised by the
account view functions so the code paths in lib/accounts/account-view.ts (the
normalization around lines handling activeIndex and the branch that normalizes
rateLimitResetTimes) are covered and assert the normalized/reset behavior
remains enforced.
In `@test/active-index.test.ts`:
- Around line 57-79: Add deterministic regression tests that pass non-finite and
fractional values into normalizeActiveIndexByFamily and into the routines that
set active indexes for all families to ensure the sanitization logic in
normalizeActiveIndexByFamily (lib/accounts/active-index.ts around the function
normalizeActiveIndexByFamily) and the removal/normalization code path (the logic
referenced near lines 93-95 in lib/accounts/active-index.ts that handles
targetIndex removal) correctly coerce NaN, Infinity, negative fractions and
non-integer targetIndex to integer bounds. Specifically, extend
test/active-index.test.ts to include cases where storage.activeIndex and entries
in activeIndexByFamily are NaN, Infinity, negative fractions, and where
setActiveIndexForAllFamilies or the remove-by-targetIndex code is given
fractional/non-integer targetIndex, then assert that returned changed flags,
storage.activeIndex, and each storage.activeIndexByFamily[family] are finite
integers within [0, upperBound-1] (and that removal uses Math.floor or
equivalent integer coercion), so the functions referenced
(normalizeActiveIndexByFamily and the targetIndex removal logic) are exercised
for these edge inputs.
---
Outside diff comments:
In `@index.ts`:
- Around line 955-968: The event index (props.index or props.accountIndex) must
be coerced to an integer before validation and persistence: read the raw value
from props.index ?? props.accountIndex, convert it using Math.trunc or
Number.parseInt (ensuring NaN is handled), then perform the bounds checks
against storage.accounts.length and use that integer when updating
account.lastUsed/lastSwitchReason and calling setActiveIndexForAllFamilies; also
add a regression test in test/index.test.ts that sends a fractional index (e.g.,
1.7) and asserts the stored/active index is the truncated integer (1) and
out-of-range fractional values are rejected.
---
Duplicate comments:
In `@lib/accounts/account-view.ts`:
- Around line 37-40: The loop that filters entries from the times object
(checking typeof value === "number") should also exclude non-finite numbers;
update the condition in the for (const [key, value] of Object.entries(times))
block to skip values where !Number.isFinite(value) in addition to the existing
checks for now and key/prefix/family, and ensure the subsequent logic that
computes the minimum wait (the code around the current selection/aggregation
used later in this module) will only see finite numbers; add vitest regressions
in test/account-view.test.ts that insert NaN, Infinity, and -Infinity into the
times map and assert they are ignored (no malformed wait labels produced) and
that valid finite timestamps still determine behavior.
- Around line 22-24: The value rawCandidate (from
storage.activeIndexByFamily?.[family] ?? storage.activeIndex) must be coerced to
an integer before clamping to avoid fractional indices: update the computation
of raw to use an integer coercion (e.g., Math.trunc or Math.floor) only when
Number.isFinite(rawCandidate) is true, falling back to 0 otherwise, then apply
the existing Math.max/Math.min clamp; update the logic that assigns raw (symbol
names: rawCandidate and raw in account-view.ts) and add a vitest regression in
test/account-view.test.ts that passes a fractional active index (e.g., 1.7) and
asserts it is truncated and clamped correctly.
In `@lib/accounts/active-index.ts`:
- Around line 17-20: clampIndex currently allows fractional or non-finite
numbers to pass through; normalize inputs at the helper boundary by coercing
index and count to finite integers (e.g., via Number.isFinite check and
Math.floor or Math.trunc) inside clampIndex and the analogous helper used around
lines ~38-42 so activeIndex/state maps never receive NaN or decimals; add vitest
cases in test/active-index.test.ts covering NaN, Infinity, negative decimals and
fractional indices to assert they are clamped to valid integer bounds and that
non-finite inputs default safely.
ℹ️ 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)
index.tslib/accounts/account-view.tslib/accounts/active-index.tslib/accounts/storage-view.tslib/codex-cli/sync.tslib/codex-manager.tstest/account-storage-view.test.tstest/account-view.test.tstest/active-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)
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/active-index.test.tstest/account-view.test.tstest/account-storage-view.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-cli/sync.tslib/accounts/storage-view.tslib/accounts/account-view.tslib/accounts/active-index.tslib/codex-manager.ts
🧬 Code graph analysis (7)
test/active-index.test.ts (2)
lib/accounts/active-index.ts (4)
createActiveIndexByFamily(22-31)setActiveIndexForAllFamilies(33-43)normalizeActiveIndexByFamily(45-86)removeAccountAndReconcileActiveIndexes(88-124)lib/prompts/codex.ts (1)
MODEL_FAMILIES(58-64)
lib/codex-cli/sync.ts (2)
lib/accounts/active-index.ts (2)
setActiveIndexForAllFamilies(33-43)normalizeActiveIndexByFamily(45-86)lib/accounts/storage-view.ts (2)
cloneAccountStorage(20-29)createEmptyAccountStorage(8-18)
lib/accounts/storage-view.ts (1)
lib/accounts/active-index.ts (1)
createActiveIndexByFamily(22-31)
test/account-view.test.ts (1)
lib/accounts/account-view.ts (5)
resolveActiveIndex(16-25)getRateLimitResetTimeForFamily(27-47)formatRateLimitEntry(49-59)formatRateLimitStatusByFamily(61-71)formatActiveIndexByFamilyLabels(73-82)
test/account-storage-view.test.ts (2)
lib/accounts/storage-view.ts (2)
createEmptyAccountStorage(8-18)cloneAccountStorage(20-29)lib/prompts/codex.ts (1)
MODEL_FAMILIES(58-64)
lib/accounts/active-index.ts (1)
lib/prompts/codex.ts (2)
ModelFamily(47-52)MODEL_FAMILIES(58-64)
index.ts (3)
lib/accounts/active-index.ts (3)
setActiveIndexForAllFamilies(33-43)normalizeActiveIndexByFamily(45-86)removeAccountAndReconcileActiveIndexes(88-124)lib/accounts/storage-view.ts (2)
cloneAccountStorage(20-29)createEmptyAccountStorage(8-18)lib/accounts/account-view.ts (2)
formatActiveIndexByFamilyLabels(73-82)formatRateLimitStatusByFamily(61-71)
🔇 Additional comments (2)
lib/codex-cli/sync.ts (1)
149-154: good helper consolidation in sync flow.
lib/codex-cli/sync.ts:149-154,lib/codex-cli/sync.ts:171-173, andlib/codex-cli/sync.ts:255cleanly delegate to shared helpers, which reduces duplication and drift across reconcile paths. coverage intest/active-index.test.tsandtest/account-storage-view.test.tsaligns with this extraction.Also applies to: 171-173, 255-255
lib/codex-manager.ts (1)
25-36: good extraction and reuse of shared account-index helpers.the wiring at
lib/codex-manager.ts:25-36,lib/codex-manager.ts:1341,lib/codex-manager.ts:2352,lib/codex-manager.ts:3051, andlib/codex-manager.ts:3827cleanly removes local duplication without introducing new token/email logging paths.Also applies to: 1341-1341, 2352-2352, 3051-3051, 3827-3827
| if (targetIndex < 0 || targetIndex >= storage.accounts.length) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
reject non-integer target indices before splice.
lib/accounts/active-index.ts:93-95 only checks range. a value like 1.2 passes and splice coerces it, potentially removing the wrong account. guard with Number.isInteger(targetIndex) and add a regression in test/active-index.test.ts.
proposed fix
-if (targetIndex < 0 || targetIndex >= storage.accounts.length) {
+if (!Number.isInteger(targetIndex) || targetIndex < 0 || targetIndex >= storage.accounts.length) {
return false;
}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."
📝 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.
| if (targetIndex < 0 || targetIndex >= storage.accounts.length) { | |
| return false; | |
| } | |
| if (!Number.isInteger(targetIndex) || targetIndex < 0 || targetIndex >= storage.accounts.length) { | |
| return false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/accounts/active-index.ts` around lines 93 - 95, The range check around
targetIndex in lib/accounts/active-index.ts currently allows non-integer values
(e.g., 1.2) which splice will coerce; update the guard in the function that
computes/removes by index (the targetIndex check) to first reject non-integers
using Number.isInteger(targetIndex) before the existing bounds check, returning
false for non-integer or out-of-range values, and add a regression test in
test/active-index.test.ts that calls the same removal/path with a non-integer
index (e.g., 1.2) and asserts no account was removed (false return or unchanged
array) to prevent future regressions.
| export function cloneAccountStorage(storage: AccountStorageV3): AccountStorageV3 { | ||
| return { | ||
| version: 3, | ||
| accounts: storage.accounts.map((account) => ({ ...account })), |
There was a problem hiding this comment.
clone helper is not fully isolated for nested mutable fields.
lib/accounts/storage-view.ts:23 only shallow-clones each account object. nested maps/objects remain shared, so mutations can leak back into the source storage. this conflicts with the "isolated mutable copy" intent; add deep cloning for mutable nested fields and cover it in test/account-storage-view.test.ts:25-41.
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/accounts/storage-view.ts` at line 23, The current accounts mapping in
storage-view.ts uses a shallow copy (accounts: storage.accounts.map((account) =>
({ ...account }))) which leaves nested mutable fields shared; replace that
shallow clone with a deep-clone of each account so nested objects/maps/arrays
are duplicated (manually clone specific mutable fields or use a safe deep-copy
helper) and ensure any Map/Set are re-created rather than copied by reference;
update the test at test/account-storage-view.test.ts (around lines 25-41) to
assert that mutating nested fields on the returned view does not change the
original storage.accounts, and add coverage for nested Map/Set/array mutation
cases.
| async function clearAccountsAndReset(): Promise<void> { | ||
| await saveAccounts({ | ||
| version: 3, | ||
| accounts: [], | ||
| activeIndex: 0, | ||
| activeIndexByFamily: {}, | ||
| }); | ||
| await saveAccounts(createEmptyAccountStorageBase()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
use a single empty-storage constructor in reset path.
lib/codex-manager.ts:3589 bypasses the local wrapper used at lib/codex-manager.ts:2351-2353. this creates a shape-consistency risk if base defaults diverge.
proposed fix
async function clearAccountsAndReset(): Promise<void> {
- await saveAccounts(createEmptyAccountStorageBase());
+ await saveAccounts(createEmptyAccountStorage());
}📝 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.
| async function clearAccountsAndReset(): Promise<void> { | |
| await saveAccounts({ | |
| version: 3, | |
| accounts: [], | |
| activeIndex: 0, | |
| activeIndexByFamily: {}, | |
| }); | |
| await saveAccounts(createEmptyAccountStorageBase()); | |
| } | |
| async function clearAccountsAndReset(): Promise<void> { | |
| await saveAccounts(createEmptyAccountStorage()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` around lines 3588 - 3590, clearAccountsAndReset
currently calls createEmptyAccountStorageBase directly which bypasses the local
wrapper used elsewhere and risks diverging defaults; change
clearAccountsAndReset to build the empty storage via the same local wrapper used
at the earlier call site (the project’s empty-storage constructor wrapper) and
pass that result to saveAccounts instead of calling
createEmptyAccountStorageBase directly so shape/defaults remain consistent
(i.e., replace the createEmptyAccountStorageBase usage in clearAccountsAndReset
with the local wrapper used elsewhere, then call saveAccounts with that value).
| activeIndex: 0, | ||
| activeIndexByFamily: {}, | ||
| }); | ||
| await saveAccounts(createEmptyAccountStorageBase()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "changed manager call sites:"
rg -n "clearAccountsAndReset|handleManageAction|runSwitch|setActiveIndexForAllFamilies\(|createEmptyAccountStorageBase\(" lib/codex-manager.ts
echo
echo "current vitest coverage touching manager paths:"
rg -n "clearAccountsAndReset|deleteAccountIndex|runSwitch|setActiveIndexForAllFamilies|normalizeActiveIndexByFamily|createEmptyAccountStorage" test --glob "*.test.ts"
echo
echo "windows ebusy / 429 coverage signals in tests:"
rg -n "EBUSY|429|rate-limited|queuedRefresh|saveAccounts" test --glob "*.test.ts"
echo
echo "storage-layer ebusy/retry handling locations:"
rg -n "EBUSY|retry|backoff|saveAccounts|writeFile" libRepository: ndycode/codex-multi-auth
Length of output: 41935
add manager-level regression tests for reset/delete/switch with windows and concurrency edge cases.
lib/codex-manager.ts:3589 (clearAccountsAndReset), 3606 (setActiveIndexForAllFamilies in handleManageAction), and 3827 (setActiveIndexForAllFamilies in runSwitch) lack explicit vitest coverage in manager flows. helper-layer tests exist (test/active-index.test.ts:5-111, test/storage.test.ts:1407-1730) but don't verify:
- reset's family-index initialization invariants across storage persistence.
- delete reindex behavior when active selection becomes invalid.
- concurrent switch/delete sequencing against persisted state.
- windows ebusy write contention in these paths (test/codex-manager-cli.test.ts has isolated EBUSY tests at 1557-1632 but not for reset/delete/switch flows).
add regression tests in test/codex-manager-cli.test.ts covering reset + concurrent operations, delete + reindex stability, and switch + ebusy retry mechanics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` at line 3589, Add vitest regression tests in
test/codex-manager-cli.test.ts that exercise manager-level flows: call
clearAccountsAndReset and verify family-index initialization and persistence
across storage restarts; simulate delete actions via handleManageAction (which
invokes setActiveIndexForAllFamilies) and assert correct reindexing when the
active selection becomes invalid; run runSwitch (which also uses
setActiveIndexForAllFamilies) concurrently with delete and with simulated EBUSY
file-write contention to validate retry mechanics and ordering against persisted
state; use helpers to orchestrate concurrent operations and persistence reloads
to confirm invariants hold across process/storage boundaries.
| storage.accounts.splice(idx, 1); | ||
| storage.activeIndex = 0; | ||
| storage.activeIndexByFamily = {}; | ||
| for (const family of MODEL_FAMILIES) { | ||
| storage.activeIndexByFamily[family] = 0; | ||
| } | ||
| setActiveIndexForAllFamilies(storage, 0); | ||
| await saveAccounts(storage); |
There was a problem hiding this comment.
deleting an account should not always reset active selection to index 0.
lib/codex-manager.ts:3606 forces every family to index 0, which drops current selection unexpectedly and can pick a different account than intended after removing a non-active row.
proposed fix
if (idx >= 0 && idx < storage.accounts.length) {
- storage.accounts.splice(idx, 1);
- setActiveIndexForAllFamilies(storage, 0);
+ const previousActive = resolveActiveIndex(storage, "codex");
+ storage.accounts.splice(idx, 1);
+ if (storage.accounts.length === 0) {
+ setActiveIndexForAllFamilies(storage, 0);
+ } else {
+ const nextActive = idx < previousActive
+ ? previousActive - 1
+ : Math.min(previousActive, storage.accounts.length - 1);
+ setActiveIndexForAllFamilies(storage, nextActive);
+ }
+ normalizeActiveIndexByFamily(storage, storage.accounts.length);
await saveAccounts(storage);
console.log(`Deleted account ${idx + 1}.`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/codex-manager.ts` around lines 3605 - 3607, The deletion currently calls
setActiveIndexForAllFamilies(storage, 0) which resets every family's active
index to 0; instead update only families affected by the removed account:
compute removedIdx (the idx being spliced) and for each family in
storage.families, if family.activeIndex === removedIdx set it to
Math.min(removedIdx, storage.accounts.length - 1) (or 0 if no accounts remain),
else if family.activeIndex > removedIdx decrement it by 1; then call
saveAccounts(storage). Modify the call site (where storage.accounts.splice(idx,
1) occurs) or extend setActiveIndexForAllFamilies to accept removedIdx and apply
this logic so only impacted families' active indices change.
| it("clones account storage to isolated mutable copies", () => { | ||
| const original = { | ||
| version: 3 as const, | ||
| accounts: [{ refreshToken: "r1", email: "a@example.com", enabled: true }], | ||
| activeIndex: 0, | ||
| activeIndexByFamily: { codex: 0 }, | ||
| }; | ||
|
|
||
| const clone = cloneAccountStorage(original); | ||
| expect(clone).toEqual(original); | ||
|
|
||
| clone.accounts[0]!.email = "b@example.com"; | ||
| clone.activeIndexByFamily.codex = 1; | ||
|
|
||
| expect(original.accounts[0]!.email).toBe("a@example.com"); | ||
| expect(original.activeIndexByFamily.codex).toBe(0); | ||
| }); |
There was a problem hiding this comment.
expand clone isolation test to cover nested mutable fields.
test/account-storage-view.test.ts:25-41 currently verifies only shallow-field isolation. add a nested-field mutation assertion (for example a nested rate-limit map) to ensure lib/accounts/storage-view.ts:23 does not leak references.
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/account-storage-view.test.ts` around lines 25 - 41, Add a nested-field
mutation assertion to the existing clone isolation test by extending the
original object with a nested mutable structure (e.g., a rateLimits map or
nested object) and then verifying after calling cloneAccountStorage that
mutating the nested field on the clone does not change the original;
specifically update the test that uses cloneAccountStorage (from
lib/accounts/storage-view.ts) to include a nested map under accounts[0] or
top-level (e.g., rateLimits: { window: { limit: 10 } }), clone it, mutate the
nested value on the clone, and add an expect asserting the original's nested
value is unchanged to ensure cloneAccountStorage performs a deep (not shallow)
copy.
| it("falls back to storage active index and normalizes non-finite candidates", () => { | ||
| expect( | ||
| resolveActiveIndex({ | ||
| activeIndex: Number.NaN, | ||
| activeIndexByFamily: { codex: Number.NaN }, | ||
| accounts: [{}, {}, {}], | ||
| }), | ||
| ).toBe(0); | ||
| expect( | ||
| resolveActiveIndex({ | ||
| activeIndex: 2, | ||
| accounts: [{}, {}, {}], | ||
| }), | ||
| ).toBe(2); | ||
| }); |
There was a problem hiding this comment.
add regressions for fractional active indices and non-finite reset times.
test/account-view.test.ts:21-35 does not lock fractional index coercion, and test/account-view.test.ts:37-70 does not cover NaN/Infinity/-Infinity in rateLimitResetTimes. please add both so fixes in lib/accounts/account-view.ts:22-24 and lib/accounts/account-view.ts:37-40 stay enforced.
As per coding guidelines, "test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions."
Also applies to: 37-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/account-view.test.ts` around lines 21 - 35, Add deterministic regression
tests in test/account-view.test.ts that assert resolveActiveIndex correctly
coerces fractional activeIndex values (e.g., 1.5 or Number.POSITIVE_INFINITY)
down to a valid integer index and falls back to storage/length when given NaN,
and also add tests that pass non-finite values (Number.NaN,
Number.POSITIVE_INFINITY, Number.NEGATIVE_INFINITY) into the rateLimitResetTimes
input exercised by the account view functions so the code paths in
lib/accounts/account-view.ts (the normalization around lines handling
activeIndex and the branch that normalizes rateLimitResetTimes) are covered and
assert the normalized/reset behavior remains enforced.
| it("normalizes active and per-family indexes within account bounds", () => { | ||
| const storage: { | ||
| activeIndex: number; | ||
| activeIndexByFamily?: Partial<Record<(typeof MODEL_FAMILIES)[number], number>>; | ||
| } = { | ||
| activeIndex: 99, | ||
| activeIndexByFamily: { | ||
| codex: Number.NaN, | ||
| "gpt-5.1": -3, | ||
| }, | ||
| }; | ||
|
|
||
| const changed = normalizeActiveIndexByFamily(storage, 2); | ||
|
|
||
| expect(changed).toBe(true); | ||
| expect(storage.activeIndex).toBe(1); | ||
| for (const family of MODEL_FAMILIES) { | ||
| expect(storage.activeIndexByFamily?.[family]).toBeGreaterThanOrEqual(0); | ||
| expect(storage.activeIndexByFamily?.[family]).toBeLessThanOrEqual(1); | ||
| } | ||
| expect(storage.activeIndexByFamily?.codex).toBe(1); | ||
| expect(storage.activeIndexByFamily?.["gpt-5.1"]).toBe(0); | ||
| }); |
There was a problem hiding this comment.
add regressions for non-finite/fractional index inputs.
test/active-index.test.ts:57-79 and test/active-index.test.ts:143-158 do not cover NaN/fractional inputs for activeIndex/setActiveIndexForAllFamilies or non-integer targetIndex removal. please add those cases to lock fixes for lib/accounts/active-index.ts:17-42 and lib/accounts/active-index.ts:93-95.
As per coding guidelines, "test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions."
Also applies to: 143-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/active-index.test.ts` around lines 57 - 79, Add deterministic regression
tests that pass non-finite and fractional values into
normalizeActiveIndexByFamily and into the routines that set active indexes for
all families to ensure the sanitization logic in normalizeActiveIndexByFamily
(lib/accounts/active-index.ts around the function normalizeActiveIndexByFamily)
and the removal/normalization code path (the logic referenced near lines 93-95
in lib/accounts/active-index.ts that handles targetIndex removal) correctly
coerce NaN, Infinity, negative fractions and non-integer targetIndex to integer
bounds. Specifically, extend test/active-index.test.ts to include cases where
storage.activeIndex and entries in activeIndexByFamily are NaN, Infinity,
negative fractions, and where setActiveIndexForAllFamilies or the
remove-by-targetIndex code is given fractional/non-integer targetIndex, then
assert that returned changed flags, storage.activeIndex, and each
storage.activeIndexByFamily[family] are finite integers within [0, upperBound-1]
(and that removal uses Math.floor or equivalent integer coercion), so the
functions referenced (normalizeActiveIndexByFamily and the targetIndex removal
logic) are exercised for these edge inputs.
1 participant
Summary
Why
PR #1 from the architecture roadmap reduces coupling and duplicate workflow logic without changing external behavior.
Validation
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
clean architecture refactoring that extracts 235 lines of duplicate account-view logic into three focused modules (
lib/accounts/account-view.ts,active-index.ts,storage-view.ts). removes duplication fromindex.tsandlib/codex-manager.tswithout changing external behavior.key improvements:
concurrency & token safety:
cloneAccountStoragepreserves existing shallow-copy pattern (nested objects likerateLimitResetTimesshare references between original and clone, but this matches prior behavior)test coverage:
activeIndexByFamilyreconciliation across all model familiesConfidence Score: 5/5
Important Files Changed
Last reviewed commit: 6b7b2e8