refactor: split repair and doctor cli flows#144
refactor: split repair and doctor cli flows#144ndycode wants to merge 16 commits intoplan/03-cli-diagnostics-splitfrom
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughextracted three cli subcommands ( Changes
Sequence Diagram(s)sequenceDiagram
participant cli as "cli (user)"
participant repair as "lib/codex-manager/repair-commands.ts"
participant storage as "account/flagged storage (lib/storage.ts)"
participant token as "token refresh / probe"
participant codex as "codex auth state"
cli->>repair: invoke `fix|verify-flagged|doctor` args
repair->>storage: load accounts and flagged entries
repair->>token: probe refresh / quota (optional live)
token-->>repair: probe/refresh results
alt apply fixes / restore
repair->>storage: transactional save accounts and/or flagged
repair->>codex: sync active index (doctor --fix)
else dry-run / no-restore
repair-->>cli: output json or formatted report
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes specific concerns
suggested labels
🚥 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)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 2068-2078: The delete/toggle menu branches mutate the in-memory
storage (storage.accounts, storage.activeIndex, storage.activeIndexByFamily) and
call saveAccounts(storage) directly, which risks overwriting concurrent updates;
wrap the entire mutation + save logic for menuResult.deleteAccountIndex (and the
similar branch at 2083-2088) inside withAccountStorageTransaction so the
read-modify-write is atomic, call saveAccounts from inside that transaction
callback, and propagate/handle transactional errors; update or add vitest cases
exercising concurrent menu actions vs an overlapping repair/rotation write and
simulate filesystem/EBUSY and 429 retry behavior to verify the transaction
prevents lost updates.
- Around line 2167-2209: The refresh path currently falls through and calls
setCodexCliActiveSelection with stale
syncAccessToken/syncRefreshToken/syncExpiresAt even when queuedRefresh fails;
change the logic in the block around hasUsableAccessToken/queuedRefresh so that
on queuedRefresh returning a non-"success" result you immediately return false
(or otherwise bail out) and do not call setCodexCliActiveSelection or
saveAccounts with stale sync* values; ensure that when refresh succeeds you
continue to update account fields, set the
syncAccessToken/syncRefreshToken/syncExpiresAt/syncIdToken and call saveAccounts
and setCodexCliActiveSelection as before; add a vitest covering the autosync
branch that simulates queuedRefresh failure and asserts no call to
setCodexCliActiveSelection and that the function returns false.
In `@lib/codex-manager/repair-commands.ts`:
- Around line 1693-1756: The active-account sync must be skipped when a refresh
attempt fails: in repair-commands.ts, after the deps.hasUsableAccessToken(...)
branch and the queuedRefresh(...) call, ensure you only call
setCodexCliActiveSelection(...) when you have a usable token (i.e.,
refreshResult.type === "success" or deps.hasUsableAccessToken(...) was true) —
gate the call by checking syncAccessToken/syncRefreshToken or the success flag
so a failed queuedRefresh does not overwrite Codex state with stale manager
tokens; update variables (syncAccessToken, syncRefreshToken, syncExpiresAt,
syncIdToken) only on success and set storage/fix flags there. Add a vitest
covering the failed-refresh branch asserting setCodexCliActiveSelection is not
called and that a warn check is emitted; update/mention the affected tests and
ensure any new queuedRefresh/queue logic used handles EBUSY/429 retry semantics
per the queue utilities.
- Around line 1245-1247: The code currently calls saveQuotaCache when
workingQuotaCache && quotaCacheChanged even under a dry run; update both
callsites (the saveQuotaCache invocations at the locations referencing
workingQuotaCache and quotaCacheChanged) to skip persistence when options.dryRun
is true (e.g., guard with if (!options.dryRun) around saveQuotaCache) so codex
auth fix --dry-run remains read-only; add/modify vitest unit tests to cover the
dry-run path asserting the cache is not written and cite those tests in the
change, and ensure any new queue logic you touch includes retry/backoff behavior
for EBUSY/429 scenarios.
- Around line 1453-1460: The doctor currently places raw emails/account IDs into
checks[].details via addCheck (e.g., the call constructing
"codex-auth-readable") and later emits them verbatim in the JSON output; change
the code that sets details (the addCheck call and other places building
checks[].details) to redact/mask identifiers before storing (e.g., replace
local-part or all but last 4 chars with asterisks, or use a redactIdentifier
helper), and update the JSON emission path that prints checks (the code that
serializes/emits checks for --json) to ensure it uses the masked value instead
of any raw identifier; add or reuse a small utility function (e.g.,
redactIdentifier or maskEmail/maskAccountId) and call it from addCheck sites and
the JSON emitter to prevent leaking emails/account IDs.
- Around line 1008-1064: The live-probe failure path in the block using
deps.hasUsableAccessToken / options.live / fetchCodexQuotaSnapshot currently
pushes a "warning-soft-failure" report on probe error and then immediately
pushes a "healthy" report for the same account, causing duplicate/conflicting
entries; change control flow so that a failed live probe falls through into the
refresh-token branch (i.e., do not emit the "access token still valid" healthy
report after the catch). Concretely: when options.live is true and
fetchCodexQuotaSnapshot throws inside the try/catch, keep the catch that pushes
the warning but remove or avoid the subsequent reports.push(...) that emits the
healthy message (or wrap that healthy push in the successful-probe branch), so
the code continues to the refresh fallback logic instead of emitting a second
report; update/add vitest tests covering the "auth fix --live" probe-failure
case and note ebusy/429 queue handling changes as required.
In `@test/codex-manager-cli.test.ts`:
- Around line 1390-1396: The new assertions only exercise the restore path and
miss the bare saveFlaggedAccounts branch; add a deterministic vitest regression
that runs the CLI command "auth verify-flagged --no-restore --json" and
stubs/mocks the persistence to force flagged persistence to fail (simulate
saveFlaggedAccounts failing in lib/codex-manager/repair-commands.ts around the
saveFlaggedAccounts branch) while using
withAccountAndFlaggedStorageTransactionMock and saveAccountsMock to verify
saveAccounts is not partially written and the active snapshot remains unchanged;
ensure the test uses vitest deterministic timers/mocks and asserts
saveFlaggedAccounts error path is hit and saveAccountsMock was not called or was
called with the original empty accounts snapshot to prove no partial write
occurred.
- Around line 5688-5755: Add a deterministic failure-path vitest that mirrors
the existing "runs doctor --fix" test but makes saveAccountsMock reject (e.g.,
mockImplementationOnce to throw or return a rejected Promise) when called inside
the transaction, then call
runCodexMultiAuthCli(["auth","doctor","--fix","--json"]) and assert the
transaction was attempted (withAccountStorageTransactionMock called),
saveAccountsMock was called, the CLI returns a non-zero exit code, and the
in-memory storageState (the variable used in the test) remains identical to its
pre-fix snapshot (no activeIndex/activeIndexByFamily/accounts mutations). Reuse
the same helpers referenced in the file (loadAccountsMock, saveAccountsMock,
queuedRefreshMock, setCodexCliActiveSelectionMock, runCodexMultiAuthCli) so the
test is deterministic and verifies rollback on persistence failure.
- Around line 504-508: The fallback currently uses
loadAccountsMock.mock.results[previousCallCount - 1]?.value which only checks
the immediately previous entry and can be null; update the logic in the test
around loadAccountsMock (the block using previousCallCount and current) to scan
backward through loadAccountsMock.mock.results from index previousCallCount - 1
down to 0 and pick the first non-null/undefined .value as the fallback, or
alternatively implement an explicit in-memory snapshot that the transaction
mocks update on each non-null call and use that snapshot when current is null;
ensure you reference loadAccountsMock, previousCallCount and mock.results in the
updated logic so the fallback reliably returns the last real snapshot.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 966baca3-fc26-4392-911f-701a9aeaead7
📒 Files selected for processing (3)
lib/codex-manager.tslib/codex-manager/repair-commands.tstest/codex-manager-cli.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-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/codex-manager.tslib/codex-manager/repair-commands.ts
🔇 Additional comments (2)
test/codex-manager-cli.test.ts (1)
1938-1941: good guard on the fix transaction boundary.the extra assertion at test/codex-manager-cli.test.ts:1940 is the right check for this refactor. it will fail fast if
auth fixstops going throughwithAccountStorageTransactionMockagain.lib/codex-manager/repair-commands.ts (1)
842-857: nice catch wrapping both verify-flagged save paths in the combined transaction.
lib/codex-manager/repair-commands.ts:842-857andlib/codex-manager/repair-commands.ts:873-882now route both restore and--no-restorepersistence throughwithAccountAndFlaggedStorageTransaction, which closes the stale-write window on the flagged-storage path.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.Also applies to: 873-882
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
test/codex-manager-cli.test.ts (2)
255-273:⚠️ Potential issue | 🟠 Majorthe fallback still replays stale pre-save state.
line 255 in
test/codex-manager-cli.test.ts:255only walksloadAccountsMock.mock.results. after a transaction persists throughtest/codex-manager-cli.test.ts:538ortest/codex-manager-cli.test.ts:562, a laterloadAccountsMock()that returnsnullcan still fall back to an older loaded snapshot instead of the last saved one. that keeps the helper order-dependent and can hide stale-write regressions. keep an explicit in-memory snapshot that is updated on successful loads and successful saves, then use that for the null fallback. 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.Also applies to: 524-528, 544-548
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-manager-cli.test.ts` around lines 255 - 273, The helper getLastLoadedAccountSnapshot currently scans loadAccountsMock.mock.results and can return a stale pre-save snapshot when a later load returns null; change this to maintain an explicit in-memory "lastSnapshot" variable that is assigned whenever a load returns a non-null value and also updated after a successful save/persist, and then have getLastLoadedAccountSnapshot return lastSnapshot when a mock result is null; apply the same in-memory snapshot pattern to the other test helpers that perform the null-fallback (the other occurrences flagged in this test) so tests deterministically use the last successfully loaded or saved state rather than walking mock.results.
1375-1428:⚠️ Potential issue | 🟠 Majorthis
--no-restoreregression still misses the live verify-flagged hazards.lines 1425-1427 in
test/codex-manager-cli.test.ts:1425only prove the final state is rolled back. they still expect twosaveAccountsMockcalls, so the redundant active write called out aroundlib/codex-manager/repair-commands.ts:872-877stays allowed, and the test never mutates flagged storage between pre-scan and commit to reproduce the merge race called out aroundlib/codex-manager/repair-commands.ts:860-882. tighten this case soauth verify-flagged --no-restore --jsonfails on any unnecessary active write, and add a deterministic concurrent-addition check for the live flagged snapshot. as per coding guidelines,test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 2078-2102: The current resolveManageActionAccountIndex can return
a numeric fallback index even when the provided AccountMetadataV3 failed to
match the loaded storage, which can cause delete/toggle to operate on a
different account after concurrent deletions; update
resolveManageActionAccountIndex so that when an account argument is provided and
findMatchingAccountIndex does not find it, the function returns null (only use
fallbackIndex when no account argument was supplied), and additionally add a
defensive verification in the delete and toggle handlers (the functions that
call resolveManageActionAccountIndex and perform the splice/enable/disable) to
check that storage.accounts[index]?.accountId === account.accountId before
mutating—if the ids don't match, abort the operation and return null/error.
- Around line 2343-2358: Add vitest regression tests for repair-commands.ts
covering the main success and edge flows: write tests exercising runFix,
runVerify, and runDoctor to assert token refresh occurs (use the exported
helpers hasUsableAccessToken, hasLikelyInvalidRefreshToken,
applyTokenAccountIdentity, updateQuotaCacheForAccount to observe/spy behavior),
assert quota cache mutations via
cloneQuotaCacheData/pruneUnsafeQuotaEmailCacheEntry/formatCompactQuotaSnapshot,
and include cases where refresh fails or rotates tokens (simulate invalid
refresh tokens) to verify fallback behavior; additionally add explicit tests for
the refresh queue handling rate-limits by simulating 429 and EBUSY responses and
asserting retry/backoff behavior and quota state remains consistent.
- Around line 2284-2286: The save is non-transactional in
autoSyncActiveAccountToCodex: storage is loaded, mutated, and then saved via
saveAccounts(storage), which can overwrite concurrent updates; change this to
call saveAccounts inside withAccountStorageTransaction so the load-modify-save
happens inside the transaction used elsewhere (see other usages at
lib/codex-manager.ts where withAccountStorageTransaction is used), i.e.,
refactor autoSyncActiveAccountToCodex to perform its modifications and call
saveAccounts within a withAccountStorageTransaction callback to ensure atomic
merge/commit; also add a regression test in test/codex-manager-cli.test.ts that
simulates a concurrent modification during autoSync (e.g., spawn a concurrent
save/rotation or auth fix) to assert no lost writes.
In `@lib/codex-manager/repair-commands.ts`:
- Around line 754-899: The current code builds nextFlaggedAccounts from a
pre-scan snapshot and then overwrites flagged storage on commit, which can drop
concurrently-added flagged accounts; to fix, move the merge logic into the
flagged storage transaction (or reload and merge the live flagged list before
calling persist) so applyRefreshChecks updates entries against the live flagged
list obtained inside withAccountAndFlaggedStorageTransaction (use
loadedStorage.accounts or loadAccounts() inside the transaction), reconcile
entries into nextFlaggedAccounts rather than replacing, and only persist the
merged result; update code paths using nextFlaggedAccounts, applyRefreshChecks,
storageChanged and flaggedChanged accordingly, and add vitest regressions that
simulate concurrent flagged additions for both --restore and --no-restore flows
(referencing test/codex-manager-cli.test.ts) and ensure
queuedRefresh/error-queue handling covers EBUSY/429 scenarios.
- Around line 915-924: The current branch uses
withAccountAndFlaggedStorageTransaction and always persists active account
storage even when --no-restore; change the code path in the conditional (the
block that checks !options.dryRun && flaggedChanged && !options.restore) to use
the flagged-only transaction/persistence (e.g., withFlaggedStorageTransaction or
the existing flagged persist helper) and only write flagged data
(nextFlaggedAccounts) so active account storage is not opened/rewritten;
update/remove calls to persist(nextStorage, ...) and instead persist only
flagged storage. Add a vitest in test/codex-manager-cli.test.ts for the
verify-flagged --no-restore flow that runs the command and asserts the active
account storage file/state is unchanged (timestamp or contents) after the run;
ensure the test also simulates/notes EBUSY/429 handling behavior for queued
writes per IO/concurrency guidelines.
In `@test/codex-manager-cli.test.ts`:
- Around line 5994-6036: Add an assertion that setCodexCliActiveSelectionMock
was not called in the "preserves pre-fix storage when doctor --fix transaction
save fails" test: after the expect(...).rejects and the
withAccountStorageTransactionMock assertion, assert
setCodexCliActiveSelectionMock.toHaveBeenCalledTimes(0) (or
.not.toHaveBeenCalled()) to ensure codex sync stayed off when saveAccountsMock
rejected; reference the test's use of runCodexMultiAuthCli,
setCodexCliActiveSelectionMock, and withAccountStorageTransactionMock to locate
where to add this assertion.
---
Duplicate comments:
In `@test/codex-manager-cli.test.ts`:
- Around line 255-273: The helper getLastLoadedAccountSnapshot currently scans
loadAccountsMock.mock.results and can return a stale pre-save snapshot when a
later load returns null; change this to maintain an explicit in-memory
"lastSnapshot" variable that is assigned whenever a load returns a non-null
value and also updated after a successful save/persist, and then have
getLastLoadedAccountSnapshot return lastSnapshot when a mock result is null;
apply the same in-memory snapshot pattern to the other test helpers that perform
the null-fallback (the other occurrences flagged in this test) so tests
deterministically use the last successfully loaded or saved state rather than
walking mock.results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2cb9e4b9-142f-4067-ad02-5ca9666171a4
📒 Files selected for processing (3)
lib/codex-manager.tslib/codex-manager/repair-commands.tstest/codex-manager-cli.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-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/codex-manager.tslib/codex-manager/repair-commands.ts
🔇 Additional comments (8)
lib/codex-manager.ts (6)
2114-2142: transaction wrapper addresses the prior concurrency concern for delete.
lib/codex-manager.ts:2119-2136now wraps the delete mutation insidewithAccountStorageTransaction, fixing the read-modify-write race flagged in the previous review. the only remaining risk is the index fallback behavior inresolveManageActionAccountIndex(see comment above).
2250-2254: refresh failure now returns early - previous issue resolved.
lib/codex-manager.ts:2252-2254returnsfalsewhenqueuedRefreshfails, preventing stale credentials from being synced to codex cli state. this addresses the prior review concern about falling through tosetCodexCliActiveSelectionwith expired tokens.
73-77: lgtm - repair commands delegation wired correctly.the imports from
lib/codex-manager/repair-commands.tsalign with the thin delegator pattern below.
2042-2052: clean delegation to repair-commands module.thin wrappers forward to
runVerifyFlaggedCommand,runFixCommand,runDoctorCommandwith shared deps. no concerns here.
2144-2177: toggle also properly wrapped in transaction now.same pattern as delete - atomic read-modify-write via
withAccountStorageTransaction. toggling the wrong account due to fallback is less severe than deleting it, but worth addressing per the earlier comment.
2042-2052: test coverage exists for ebusy/429 scenarios and transaction patterns—no gaps found.the search results show comprehensive test coverage for the concerns raised:
test/storage.test.ts:2597-2821coversEBUSY/EPERMretry logic, concurrent transaction failures, and rollback scenarios. lines 658-895 exercise overlapping flagged + account storage writes under contention.test/cli-auth-menu.test.ts:96,:154,:240,:253testdeleteAccountIndexandtoggleAccountIndexmenu actions.test/codex-manager-cli.test.tsmocks and verifieswithAccountStorageTransactionextensively (lines 2079, 3201, 5911, 6034).test/refresh-queue.test.ts:890covers lease acquisition failures ("EBUSY lease dir").the code at
lib/codex-manager.ts:2042-2052(runVerifyFlagged, runFix, runDoctor) is trivial delegators that pass through to command handlers; the test coverage lives in those command implementations, which are well-covered.no token/email leaks found in delegators. existing regression tests sufficient for these patterns.
lib/codex-manager/repair-commands.ts (2)
546-570: good masking on doctor identity details.the new masking path is solid: identifiers are redacted before being emitted in checks/json (
lib/codex-manager/repair-commands.ts:546-570,lib/codex-manager/repair-commands.ts:1504-1509,lib/codex-manager/repair-commands.ts:1734-1740). this closes the email/account-id leakage path in diagnostics output.As per coding guidelines,
lib/**: check for logging that leaks tokens or emails.Also applies to: 1504-1509, 1734-1740
1291-1293: dry-run quota-cache persistence guard looks correct now.both quota-cache write sites are correctly gated by
!options.dryRunin json and non-json paths (lib/codex-manager/repair-commands.ts:1291-1293,lib/codex-manager/repair-commands.ts:1392-1394), keepingauth fix --dry-run --liveread-only.Also applies to: 1392-1394
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
lib/codex-manager/repair-commands.ts (1)
298-317:⚠️ Potential issue | 🟠 Majorthe live-probe fallback still double-counts one account in
fixoutput.at
lib/codex-manager/repair-commands.ts:1154-1160, the probe failure is pushed intoreports, and the refresh branch atlib/codex-manager/repair-commands.ts:1241-1270pushes a second final row for the same account.summarizeFixReports()atlib/codex-manager/repair-commands.ts:298-317counts both, soauth fix --livecan report more outcomes than accounts scanned. keep the fallback note out ofreports, or replace the interim row with the final refresh result instead of appending twice. i don't see a vitest for probe-failure + refresh-fallback intest/repair-commands.test.ts: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.Also applies to: 1154-1160, 1241-1270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager/repair-commands.ts` around lines 298 - 317, summarizeFixReports is double-counting accounts because the probe-failure fallback pushes an interim FixAccountReport into reports and the later refresh branch appends a final report for the same account; update the probe failure handling (the block that currently pushes the fallback at the probe step) to either not push that interim "fallback" entry into reports or to update/replace the existing report entry when the refresh result arrives instead of appending a second row, ensuring summarizeFixReports (and the FixAccountReport array) only contains one final outcome per account; add a vitest that simulates probe-failure followed by refresh-fallback to assert reports length equals accounts scanned and outcomes aggregate correctly, and mention the affected symbols: summarizeFixReports and the probe/fallback/reporting code paths that push FixAccountReport so reviewers can locate and change the push-to-reports behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/codex-manager.ts`:
- Around line 2058-2063: The current resetManageActionSelection function
unconditionally zeroes storage.activeIndex and every storage.activeIndexByFamily
entry (using MODEL_FAMILIES), causing the active selection to jump when deleting
a different row; modify resetManageActionSelection (or the delete-path caller
that currently calls it) to accept the removedRowIndex and the new list length,
decrement any activeIndex/activeIndexByFamily values greater than
removedRowIndex by 1, only choose a new active index when the removed row was
the current active index or the list becomes empty, and ensure
activeIndexByFamily updates follow the same relative adjustment per family; add
Vitest coverage for deleting a non-active row and for deleting the active row,
and update tests under lib/** to cover auth rotation and concurrency (ebusy/429)
scenarios affected by index changes.
In `@lib/codex-manager/repair-commands.ts`:
- Around line 1084-1086: runFix currently prints plain text and returns early
when no storage/accounts, and it treats quota-cache writes separately from the
public "changed" state; update runFix to honor the --json flag by emitting a
JSON empty payload (e.g., { changed: false, message: "" } or equivalent contract
used elsewhere) on the early no-accounts/quotacache-only return instead of plain
text, and fold quota-cache writes into the reported change state by making
saveQuotaCache (and the workingQuotaCache updates in the blocks around
workingQuotaCache at runFix locations) set or return a boolean indicating
whether it actually wrote/changed the cache and OR that into the central changed
variable used when producing the final JSON/text output; ensure runFix uses that
unified changed value when producing the final message/JSON and update/add
vitest coverage in test/repair-commands.test.ts to cover the
no-account/quota-cache-only branches and Windows EBUSY/IO concurrency scenarios
as noted.
- Around line 1341-1353: The refactor in runFix builds accountMutations (via
collectAccountStorageMutations) and applies them inside
withAccountStorageTransaction, which exposes a race window for
concurrent-writes; add a regression test simulating a concurrent writer (in
test/repair-commands.test.ts and mirror in test/codex-manager-cli.test.ts) that
snapshots, then injects a conflicting write during the runFix flow to assert the
retry/merge behavior; update withAccountStorageTransaction and/or runFix to
detect transient filesystem/write conflicts (EBUSY) or rate-limit errors (429)
and retry with exponential backoff/merge (reloading loadedStorage, recomputing
or reapplying applyAccountStorageMutations) before failing, and ensure the new
tests exercise Windows-style timing by introducing a delayed conflicting persist
to reproduce the race.
- Around line 773-786: The JSON output path omits or miscomputes
remainingFlagged when no restore is performed: after calling
applyRefreshChecks() recompute the count of remaining flagged transactions and
set remainingFlagged into the JSON payload for both the zero-account fast path
and the normal path (even when flaggedChanged is false and no write occurs);
ensure the empty/json fast-path payload emitted in the --json branch includes
remainingFlagged, and add a vitest that calls verify-flagged with --no-restore
--json to assert the count is correct; touch symbols: applyRefreshChecks(),
flaggedChanged, remainingFlagged, VerifyFlaggedReport and the
options.restore/options.json handling so the field is always present and
accurate.
- Around line 1720-1728: The loop over storage.accounts in repair-commands.ts
currently continues on !sanitizeEmail(account.email) before checking
refresh-token shape, so move the
deps.hasLikelyInvalidRefreshToken(account.refreshToken) check (and increment of
likelyInvalidRefreshTokenCount) out of the email-dependent early-continue branch
so every account is evaluated for token shape even when email is missing; update
the corresponding refresh-token-shape logic at the 1746-1753 region the same
way, adjust variable usage for
likelyInvalidRefreshTokenCount/duplicateEmailCount/placeholderEmailCount
accordingly, add a vitest in test/repair-commands.test.ts covering an account
with no email to assert the token-shape counter increments, and when touching
related queue/rotation code follow project guidelines to verify new queues
handle EBUSY and 429 retry scenarios.
- Around line 1653-1661: The auto-fix check is being emitted too early — move
the addCheck(...) for "auto-fix" to after all fix branches (including the
active-account refresh/sync branches that run after applyDoctorFixes()) and
compute its severity/message from the final actions set (e.g., derive fixChanged
= actions.length > 0 and use actions.length plus options.dryRun to decide the
message) instead of the intermediate applyDoctorFixes() value; update any places
that inspect fix.changed to read this final state, add/adjust vitest cases in
repair-commands.test.ts for "expired active account + --fix --dry-run" and
"refresh-success + --fix", and ensure any new queues or retry paths you add
handle EBUSY and 429 retry semantics per project guidelines.
In `@lib/storage.ts`:
- Line 2123: Replace the direct calls to loadFlaggedAccountsFromPath(...) (the
ones that set currentFlagged at the two call sites) with the lock-safe loader
used by loadFlaggedAccounts() so the same reset-marker/legacy-file handling and
enoent/parse fallback ({ version: 1, accounts: [] }) occur; either call
loadFlaggedAccounts() directly or extract its lock-and-retry logic into a shared
helper (preserving getFlaggedAccountsPath(), clearFlaggedAccounts() semantics)
and use that helper at both places, and ensure the implementation uses
file-locking/retries tuned for Windows filesystem behavior and retries on
EBUSY/429 per concurrency guidelines and that tests in
test/storage.test.ts:866-1013 are updated/covered.
In `@test/codex-manager-cli.test.ts`:
- Around line 313-320: The test harness currently returns a permanent empty
default on extra flagged reads and only updates lastFlaggedStorageSnapshot, so
mocked persistence doesn't reflect real storage or partial writes; modify the
mocked storage by introducing and using a shared in-memory flagged storage state
that loadFlaggedAccountsMock reads from and saveFlaggedAccountsMock mutates
(update the shared state before throwing to simulate partial writes and ensure
save callbacks persist into that state), and adjust getCurrentFlaggedSnapshot to
rely on loadFlaggedAccountsMock and getLastLoadedFlaggedSnapshot as fallbacks
while preserving updateLastFlaggedStorageSnapshot semantics; update any
references to lastFlaggedStorageSnapshot, saveFlaggedAccountsMock,
loadFlaggedAccountsMock, updateLastFlaggedStorageSnapshot, and
getLastLoadedFlaggedSnapshot so tests model real persisted flagged state and
deterministic rollback behavior.
- Around line 6216-6283: Add a deterministic regression test for "doctor --fix"
that simulates a concurrent write between the pre-scan and the in-transaction
read: create a new test (mirror pattern used by the auth fix at the earlier
tests) that uses loadAccountsMock.mockImplementationOnce to return the initial
snapshot, then returns a second snapshot (with a newly added account) for the
subsequent read inside the storage transaction; wire saveAccountsMock to capture
what is persisted and assert that the code either merges/preserves the
concurrently added account (or fails with the expected retry/abort behavior),
and assert withAccountStorageTransactionMock and
runCodexMultiAuthCli("auth","doctor","--fix") are exercised; reference
runCodexMultiAuthCli, loadAccountsMock, saveAccountsMock, and
withAccountStorageTransactionMock when adding the test so it reproduces the
concurrent-write scenario deterministically under vitest.
In `@test/repair-commands.test.ts`:
- Around line 218-283: The test currently feeds the same snapshot to
loadAccountsMock and withAccountStorageTransactionMock so it only verifies DI
rather than a concurrent-write race; change loadAccountsMock to return an older
prescan snapshot (e.g., email/access/refresh values) and make
withAccountStorageTransactionMock invoke the handler with a newer in-transaction
snapshot (different account row values) and capture persistedAccountStorage from
the transaction commit; keep applyTokenAccountIdentity as the injected applier,
then assert that the refreshed fields
(accessToken/refreshToken/email/accountId/accountIdSource) are merged onto the
newer in-transaction row (persistedAccountStorage) rather than overwriting with
the old prescan snapshot and that runFix still returns exitCode 0 and calls
applyTokenAccountIdentity.
---
Duplicate comments:
In `@lib/codex-manager/repair-commands.ts`:
- Around line 298-317: summarizeFixReports is double-counting accounts because
the probe-failure fallback pushes an interim FixAccountReport into reports and
the later refresh branch appends a final report for the same account; update the
probe failure handling (the block that currently pushes the fallback at the
probe step) to either not push that interim "fallback" entry into reports or to
update/replace the existing report entry when the refresh result arrives instead
of appending a second row, ensuring summarizeFixReports (and the
FixAccountReport array) only contains one final outcome per account; add a
vitest that simulates probe-failure followed by refresh-fallback to assert
reports length equals accounts scanned and outcomes aggregate correctly, and
mention the affected symbols: summarizeFixReports and the
probe/fallback/reporting code paths that push FixAccountReport so reviewers can
locate and change the push-to-reports behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: de10a1c0-f680-4d15-a019-937ea7dfc9bd
📒 Files selected for processing (6)
lib/codex-manager.tslib/codex-manager/repair-commands.tslib/storage.tstest/codex-manager-cli.test.tstest/repair-commands.test.tstest/storage.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/storage.test.tstest/repair-commands.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/storage.tslib/codex-manager.tslib/codex-manager/repair-commands.ts
🔇 Additional comments (1)
test/codex-manager-cli.test.ts (1)
1729-1793: good concurrent-write coverage forauth fix.
test/codex-manager-cli.test.ts:1729-1793exercises the stale-scan vs in-lock merge path instead of another same-snapshot happy path, which is the right regression for this command.
1 participant
Summary
What Changed
lib/codex-manager/repair-commands.tsforauth fix,auth verify-flagged, andauth doctor.lib/codex-manager.tsfocused on dispatch and shared helper wiring for the extracted repair flows.Validation
npm run lintnpm run typechecknpm testnpm test -- test/documentation.test.tsnpm run buildDocs and Governance Checklist
docs/getting-started.mdupdated (if onboarding flow changed) - not needed; onboarding behavior unchangeddocs/features.mdupdated (if capability surface changed) - not needed; capability surface unchangeddocs/reference/*pages updated (if commands/settings/paths changed) - not needed; command behavior unchangeddocs/upgrade.mdupdated (if migration behavior changed) - not needed; no migration changeSECURITY.mdandCONTRIBUTING.mdreviewed for alignmentRisk and Rollback
9a7d396.Additional Notes
plan/03-cli-diagnostics-split).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
runFix,runVerifyFlagged, andrunDoctorfrom the monolithiclib/codex-manager.tsinto a newlib/codex-manager/repair-commands.tsmodule wired via a flatRepairCommandDepsinterface. it is a meaningful step forward on the modularisation stack and converges on most of the serious issues raised in the prior review round.what's addressed from prior rounds:
runFixnow useswithAccountStorageTransactionwith pre/post mutation replay — no longer mutates and saves outside a lockrunVerifyFlagged--restorepath useswithAccountAndFlaggedStorageTransactionwith in-transaction drift filtering viahasFlaggedRefreshTokenDrift;--no-restorepath uses the newwithFlaggedStorageTransactionwith the same drift guardrunDoctorconsolidates to a singlewithAccountStorageTransaction;pendingCodexActiveSyncis now guarded by(!doctorRefreshMutation || storageFixChanged)so codex-cli sync is not written when the storage transaction abortedexistsSyncTOCTOU in the file-presence checks is fixed by storing results in local consts before constructingaddCheckobjectsfixChangedcorrectly derives fromstorageFixChanged || fixActions.length > 0rather than actions alone"doctor-refresh"warn checks instead of silent skipsone new finding:
withAccountAndFlaggedStorageTransactionpassescurrentFlaggedto the handler without cloning, whilewithFlaggedStorageTransactionpassesstructuredClone(snapshot). current callers are safe (they clone immediately), but the API inconsistency is a footgun for future callers — see inline comment onstorage.tsremaining coverage gaps worth tracking:
--live --jsonwhen only the quota cache was refreshed (thequotaCacheChanged/changeddistinction)--no-restorewith concurrent token drift at persistence timeConfidence Score: 4/5
Important Files Changed
withFlaggedStorageTransaction,cloneFlaggedStorageForPersistence, andloadFlaggedAccountsWithFallback; extendswithAccountAndFlaggedStorageTransactionto passcurrentFlaggedas a third handler arg. The newwithFlaggedStorageTransactioncorrectly clones before handing to the caller;withAccountAndFlaggedStorageTransactiondoes not clonecurrentFlagged, creating an API inconsistency that is safe today but a latent footgun.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD CLI["codex-manager.ts\n(dispatch)"] -->|"runFix(args)"| FIX["runFix\nrepair-commands.ts"] CLI -->|"runVerifyFlagged(args)"| VF["runVerifyFlagged\nrepair-commands.ts"] CLI -->|"runDoctor(args)"| DOC["runDoctor\nrepair-commands.ts"] FIX --> SCAN_FIX["scan accounts\n(queuedRefresh per account)"] SCAN_FIX --> MUTATE_FIX["collect mutations\noutside lock"] MUTATE_FIX --> TX_FIX["withAccountStorageTransaction\napplyAccountStorageMutations"] TX_FIX --> SAVE_FIX["saveAccountsUnlocked\n+ rollback on failure"] VF --> SCAN_VF["collectRefreshChecks\n(outside lock)"] SCAN_VF -->|"--restore"| TX_BOTH["withAccountAndFlaggedStorageTransaction\ndrift-filter → applyRefreshChecks\n+ applyFlaggedStorageMutations"] SCAN_VF -->|"--no-restore"| TX_FLAG["withFlaggedStorageTransaction\ndrift-filter → applyFlaggedStorageMutations"] TX_BOTH --> SAVE_BOTH["saveAccountsUnlocked\nsaveFlaggedAccountsUnlocked"] TX_FLAG --> SAVE_FLAG["saveFlaggedAccountsUnlocked\n+ rollback on failure"] DOC --> CHECKS["run diagnostic checks\n(applyDoctorFixes on clone)"] CHECKS --> TX_DOC["withAccountStorageTransaction\napplyDoctorFixes in-tx\n+ doctorRefreshMutation replay"] TX_DOC --> SYNC["setCodexCliActiveSelection\n(guarded: !doctorRefreshMutation || storageFixChanged)"]Comments Outside Diff (1)
lib/storage.ts, line 2162-2217 (link)withAccountAndFlaggedStorageTransactionloads flagged snapshot inside the account lock but outside the flagged write windowloadFlaggedAccountsWithFallbackis called unconditionally insidewithStorageLock(which guards account writes), but there is no separate flagged-storage lock preventing a concurrentwithFlaggedStorageTransactionfrom writing between this read and the eventualpersistcall. thecurrentFlaggedsnapshot handed to the handler may be stale by the timepersist(accountStorage, flaggedStorage)fires.this is distinct from the pre-existing behavior — the old implementation also lacked a flagged read-lock here, but the new
currentFlaggedparameter implies to callers that they are operating on a freshly-consistent flagged snapshot. callers that rely oncurrentFlaggedbeing coherent with the final write (e.g. thehasFlaggedRefreshTokenDriftguard inrunVerifyFlagged) are subtly racing on windows when the plugin writes flagged storage concurrently.the cleanest fix is to extend
withStorageLockto cover flagged reads, or document explicitly thatcurrentFlaggedis a best-effort snapshot rather than a transaction-locked view.Prompt To Fix With AI
Prompt To Fix All With AI
Last reviewed commit: "fix: guard repair co..."