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>
|
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:
📝 Walkthroughwalkthroughmoved active-index, rate-limit, and storage helpers 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.
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.
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
…conciliation - normalize event and helper indexes to safe integer bounds\n- ignore non-finite rate-limit reset timestamps\n- deep-clone account rate-limit maps in storage views\n- reconcile active selection when deleting accounts instead of forcing index 0\n- add regression coverage for fractional/non-finite inputs and manager delete flow\n\nCo-authored-by: Codex <noreply@openai.com>
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Update the mocked model-family set and deletion assertion so the test validates that non-codex family indexes are reindexed correctly after account removal.\n\nCo-authored-by: Codex <noreply@openai.com>
There was a problem hiding this comment.
Actionable comments posted: 3
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-971:⚠️ Potential issue | 🟠 Majormake account selection writes atomic to avoid lost updates.
index.ts:955-971does a load-mutate-save sequence without transaction boundaries. two concurrent selection events can overwrite each other or race with other writers.proposed fix
- const storage = await loadAccounts(); - if (!storage || index < 0 || index >= storage.accounts.length) { - return; - } - const now = Date.now(); - const account = storage.accounts[index]; - if (account) { - account.lastUsed = now; - account.lastSwitchReason = "rotation"; - } - setActiveIndexForAllFamilies(storage, index); - await saveAccounts(storage); + await withAccountStorageTransaction(async (storage, persist) => { + if (!storage || index < 0 || index >= storage.accounts.length) return; + const now = Date.now(); + const account = storage.accounts[index]; + if (account) { + account.lastUsed = now; + account.lastSwitchReason = "rotation"; + } + setActiveIndexForAllFamilies(storage, index); + await persist(storage); + });please add a vitest regression that fires two concurrent
account.selectevents and asserts final persisted selection is consistent.
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 `@index.ts` around lines 955 - 971, The load-mutate-save sequence in the account selection handler (the block using loadAccounts, modifying account.lastUsed/lastSwitchReason, calling setActiveIndexForAllFamilies, then saveAccounts) is not atomic and can be lost to races; change it to use a per-accounts lock/queue or file-level atomic update (e.g., acquire a mutex or enqueue writes before calling loadAccounts/saveAccounts) so concurrent account.select handlers serialize updates and retry on transient EBUSY/429 errors, preserving last-used/active-index changes; update the handler that processes the account.select event to obtain the lock, re-load storage inside the lock, validate index, mutate, and call saveAccounts before releasing the lock, and ensure logging during the flow never prints tokens or emails. Add a vitest regression that fires two concurrent account.select events (calling the same handler) and asserts the final persisted storage (via loadAccounts) has a single consistent active index and both updates applied deterministically; reference the functions loadAccounts, saveAccounts, setActiveIndexForAllFamilies and the account.select event handler in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.ts`:
- Around line 2398-2400: The clamp-only call to normalizeActiveIndexByFamily is
causing per-family active pointers to drift on deletions; replace that
clamp-only post-processing with the reindexing/reconciliation logic used in
lib/accounts/active-index.ts (the logic around lines 94-139) so families whose
active index should shift left when an earlier account is removed are updated
correctly (i.e., use the reconcile/reindex function or inline its algorithm
instead of plain clamping) and then add a vitest regression that constructs
activeIndexByFamily like { codex: 2, "gpt-5.1": 1 }, deletes index 0, runs the
same normalization, and asserts both family selections are preserved correctly
after reindexing; reference normalizeActiveIndexByFamily and activeIndexByFamily
in your changes and ensure the new test is named to indicate deletion reindex
regression.
In `@lib/codex-manager.ts`:
- Around line 3607-3609: The delete flow currently calls
removeAccountAndReconcileActiveIndexes(idx) then unconditionally calls
saveAccounts(storage) and console.log; change it to check the boolean result
returned by removeAccountAndReconcileActiveIndexes (and/or deleteAccountIndex
wrapper) and only call saveAccounts(storage) and print the "Deleted..." message
when the result is true (i.e., an actual removal occurred); if false, do not
persist or log success and instead return/throw as appropriate. Add a vitest
regression that calls manage-mode deleteAccountIndex with a fractional index,
asserting that saveAccounts was not invoked, no success log was emitted, and
storage remains unchanged. Ensure the fix references
removeAccountAndReconcileActiveIndexes and saveAccounts so tests and logging
behavior are updated accordingly.
---
Outside diff comments:
In `@index.ts`:
- Around line 955-971: The load-mutate-save sequence in the account selection
handler (the block using loadAccounts, modifying
account.lastUsed/lastSwitchReason, calling setActiveIndexForAllFamilies, then
saveAccounts) is not atomic and can be lost to races; change it to use a
per-accounts lock/queue or file-level atomic update (e.g., acquire a mutex or
enqueue writes before calling loadAccounts/saveAccounts) so concurrent
account.select handlers serialize updates and retry on transient EBUSY/429
errors, preserving last-used/active-index changes; update the handler that
processes the account.select event to obtain the lock, re-load storage inside
the lock, validate index, mutate, and call saveAccounts before releasing the
lock, and ensure logging during the flow never prints tokens or emails. Add a
vitest regression that fires two concurrent account.select events (calling the
same handler) and asserts the final persisted storage (via loadAccounts) has a
single consistent active index and both updates applied deterministically;
reference the functions loadAccounts, saveAccounts, setActiveIndexForAllFamilies
and the account.select event handler in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 46e6b32d-4da1-4008-a6e9-e8afcfd3a44c
📒 Files selected for processing (10)
index.tslib/accounts/account-view.tslib/accounts/active-index.tslib/accounts/storage-view.tslib/codex-manager.tstest/account-storage-view.test.tstest/account-view.test.tstest/active-index.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/account-view.test.tstest/index.test.tstest/account-storage-view.test.tstest/codex-manager-cli.test.tstest/active-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/account-view.tslib/accounts/active-index.tslib/accounts/storage-view.tslib/codex-manager.ts
🧬 Code graph analysis (8)
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/index.test.ts (1)
lib/prompts/codex.ts (1)
MODEL_FAMILIES(58-64)
lib/accounts/account-view.ts (1)
lib/prompts/codex.ts (2)
ModelFamily(47-52)MODEL_FAMILIES(58-64)
lib/accounts/active-index.ts (1)
lib/prompts/codex.ts (2)
ModelFamily(47-52)MODEL_FAMILIES(58-64)
lib/accounts/storage-view.ts (1)
lib/accounts/active-index.ts (1)
createActiveIndexByFamily(27-36)
test/codex-manager-cli.test.ts (2)
lib/codex-manager.ts (1)
runCodexMultiAuthCli(3966-4027)lib/accounts/account-view.ts (1)
formatRateLimitEntry(49-59)
lib/codex-manager.ts (2)
lib/accounts/active-index.ts (4)
createActiveIndexByFamily(27-36)normalizeActiveIndexByFamily(51-92)removeAccountAndReconcileActiveIndexes(94-139)setActiveIndexForAllFamilies(38-49)lib/accounts/storage-view.ts (1)
createEmptyAccountStorage(8-18)
index.ts (3)
lib/accounts/active-index.ts (3)
setActiveIndexForAllFamilies(38-49)normalizeActiveIndexByFamily(51-92)removeAccountAndReconcileActiveIndexes(94-139)lib/accounts/storage-view.ts (2)
cloneAccountStorage(20-36)createEmptyAccountStorage(8-18)lib/accounts/account-view.ts (2)
formatActiveIndexByFamilyLabels(73-82)formatRateLimitStatusByFamily(61-71)
🔇 Additional comments (7)
lib/accounts/active-index.ts (1)
17-25: good normalization and reconcile boundaries.
lib/accounts/active-index.ts:17-25andlib/accounts/active-index.ts:94-139now enforce finite integer handling and safe delete reconciliation. this closes the fractional/non-finite and splice-coercion risk paths cleanly.Also applies to: 94-139
lib/accounts/storage-view.ts (1)
20-35: clone isolation fix looks correct.
lib/accounts/storage-view.ts:20-35correctly isolatesrateLimitResetTimeswhile cloning accounts, so nested mutations on clones do not leak back.lib/accounts/account-view.ts (1)
16-25: active index and rate-limit parsing hardening is solid.
lib/accounts/account-view.ts:16-25andlib/accounts/account-view.ts:27-47correctly floor/clamp indexes and ignore non-finite reset timestamps before min-selection.Also applies to: 27-47
test/account-storage-view.test.ts (1)
25-50: good deterministic regression for nested clone isolation.
test/account-storage-view.test.ts:25-50now validates top-level and nested mutation isolation (rateLimitResetTimes) with explicit assertions.test/codex-manager-cli.test.ts (1)
502-529: new cli regressions are useful and deterministic.
test/codex-manager-cli.test.ts:502-529andtest/codex-manager-cli.test.ts:1974-2022strengthen status and delete/reindex coverage under vitest with stable inputs.Also applies to: 1974-2022
test/account-view.test.ts (1)
21-52: coverage here is strong for the edge cases that mattered.
test/account-view.test.ts:21-52andtest/account-view.test.ts:72-102lock empty/fractional/non-finite index behavior and non-finite reset timestamp filtering in deterministic vitest cases.Also applies to: 72-102
test/active-index.test.ts (1)
37-56: good regression coverage for index sanitization and removal guards.
test/active-index.test.ts:37,test/active-index.test.ts:78, andtest/active-index.test.ts:182lock finite-integer coercion, bounds clamping, and non-integer deletion rejection in a deterministic vitest suite.Also applies to: 78-101, 182-197
- use removal reconciliation (not clamp-only) in plugin delete flows to keep per-family selection stable - guard manage delete persistence/logging on actual removal result - align index test model-family mock with production families and add multi-family reindex regression - add codex-manager CLI regression for fractional delete index handling Co-authored-by: Codex <noreply@openai.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- normalize activeIndexByFamily after codex-remove reconciliation to align with other delete paths - add regression for removal when activeIndexByFamily is undefined Co-authored-by: Codex <noreply@openai.com>
Ensure removeAccountAndReconcileActiveIndexes initializes and updates activeIndexByFamily even when legacy storage omits that field. Add regression coverage for deletion from legacy-shaped storage. Co-authored-by: Codex <noreply@openai.com>
Keep cloneAccountStorage from normalizing undefined activeIndexByFamily to an empty object so legacy-shaped snapshots do not trigger spurious changed-state signals. Add regression coverage. Co-authored-by: Codex <noreply@openai.com>
|
Merged via integration PR to safely combine multiple approved changes. |
1 participant
Summary - extract duplicated account-view formatting logic into a shared helper module - reuse the shared helpers in index.ts and lib/codex-manager.ts - add focused unit + regression tests to lock active-index fallback and rate-limit formatting behavior ## Why PR #1 from the architecture roadmap reduces coupling and duplicate workflow logic without changing external behavior. ## Validation - npm run build - npm run lint - npm run typecheck - npm test ## PR Resolution Update (2026-03-05) ### What Changed - Normalized account index handling to finite integers across shared helpers and event intake ( esolveActiveIndex, setActiveIndexForAllFamilies, emoveAccountAndReconcileActiveIndexes, ^Gccount.select event path). - Hardened rate-limit rendering by ignoring non-finite reset timestamps before min-selection. - Updated manager delete flow to reconcile active selections instead of forcing every family to index 0, and aligned reset path to the manager wrapper createEmptyAccountStorage() constructor. - Strengthened clone isolation in storage views by cloning nested ateLimitResetTimes objects. - Added/updated regression tests for fractional/non-finite index and rate-limit inputs, non-integer remove indices, nested clone isolation, manager delete reindexing, and event fractional index coercion. ### How To Test - pm run typecheck
n- pm run lintn- pm run buildn- pm testn ### Risk / Rollout Notes - Behavior change is intentional for delete reconciliation: removing a non-active account now preserves the previously selected account by reindexing instead of resetting to account 1. - Status output now includes explicit reset detail text for rate-limited accounts ( esets in ...), improving operator clarity. ## PR Resolution Update (2026-03-05, follow-up) ### What changed - Replaced clamp-only delete normalization inindex.tswith per-removal reconciliation usingremoveAccountAndReconcileActiveIndexes, including the flagged-removal path and manage-delete path. - Guarded manage-delete persistence inlib/codex-manager.tsso save/log success only occurs when removal actually succeeds. - Updatedtest/index.test.tsmockMODEL_FAMILIESto match production (gpt-5-codex,codex-max,codex,gpt-5.2,gpt-5.1) and added a multi-family delete reindex regression. - Added fractional manage-delete regression intest/codex-manager-cli.test.ts. ### How to test -npm test -- test/index.test.ts test/codex-manager-cli.test.ts-npm run typecheck-npm run lint-npm run build-npm test### Risk / rollout notes - No intended external behavior change. - Primary risk area was cross-family active-index drift after deletions; now covered by helper-based reconciliation and regression tests.PR Resolution Update (2026-03-05, follow-up 2)
What changed
index.tscodex-removeflow to normalize family active-index state afterremoveAccountAndReconcileActiveIndexes(...).activeIndexByFamilyon older storage snapshots.test/index.test.tsfor remove-with-undefined-family-map normalization.How to test
npm test -- test/index.test.tsnpm run typechecknpm run lintRisk / rollout notes
Follow-up Review Resolution (2026-03-05)
How to test
Risk / rollout notes
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr extracts duplicated active-index and rate-limit formatting logic from
index.ts,lib/codex-manager.ts, andlib/codex-cli/sync.tsinto three focused shared modules (lib/accounts/account-view.ts,lib/accounts/active-index.ts,lib/accounts/storage-view.ts), fixing delete-reindex semantics to preserve relative selection position rather than resetting to account 1 and hardening index/timestamp inputs against fractional and non-finite values throughout.key changes and observations:
removeAccountAndReconcileActiveIndexesnow always initializesactiveIndexByFamilyfor legacy storage where the field wasundefined, closing the pre-migration persistence gap across all delete pathscloneAccountStoragepreservesundefinedactiveIndexByFamilyrather than coercing to{}, which is the correct behavior but means first-reconcile of pre-migration storage will still detect achanged = trueand trigger a save on windows — no ebusy retry guard exists at eithersaveAccountscall site (codex-removetool,account.selectevent handler)formatActiveIndexByFamilyLabelsguards withNumber.isFinitebut notNumber.isInteger, so fractional values from unrecovered legacy snapshots would render as "1.5"-style labels rather than a clean "-"handleManageActionincodex-manager.tssilently swallows out-of-bounds delete indices (the inner fractional guard logs an error, but the outer bounds check exits without any log when the index is stale/out-of-range)saveAccountsebusy path on windowsConfidence Score: 4/5
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[delete / account.select / rotate event] --> B{code path} B --> C[codex-remove tool\nindex.ts] B --> D[account.select event\nindex.ts] B --> E[handleManageAction delete\ncodex-manager.ts] B --> F[batch-delete wrapper\nindex.ts] C --> G[removeAccountAndReconcileActiveIndexes\nlib/accounts/active-index.ts] E --> G F --> G G --> H{targetIndex valid?\nNumber.isInteger + bounds} H -- no --> I[return false\nno mutation] H -- yes --> J[splice account\nreconcile activeIndex\ninitialize activeIndexByFamily\nreconcile per-family indexes] C --> K[normalizeActiveIndexByFamily\nclearFamilyMapWhenEmpty: true] F --> K D --> L[Math.floor rawIndex\nsetActiveIndexForAllFamilies\nlib/accounts/active-index.ts] J --> M[saveAccounts\nno EBUSY retry ⚠️] K --> M L --> M style I fill:#f9f,stroke:#333 style M fill:#ff9,stroke:#333Last reviewed commit: d818232
Context used:
dashboard- What: Every code change must explain how it defends against Windows filesystem concurrency bugs and ... (source)