fix: add preuninstall hook and uninstall CLI command (#468)

[ndycode]

Summary

Resolves #468 — npm uninstall codex-multi-auth left residual artifacts (launcher entries, plugin entries in Codex.json, cached node_modules, app-bind rotation state).

• Adds an npm preuninstall lifecycle script (scripts/preuninstall.js) that runs cleanup automatically on package removal, exits 0 even on partial failures, and skips in CI / --ignore-scripts contexts.
• Adds codex-multi-auth uninstall [--dry-run] [--json] [--clear-accounts] CLI for manual remediation on existing installs.
• Conservative bun.lock deletion: only removed when this plugin is the sole entry or no config exists; preserved when other plugins remain or config is corrupt.
• Bundled correctness fixes surfaced during testing: stream-failover read-before-timeout, config-toml restoration of model_provider / disable_response_storage, storage transaction releaseLock init + currentFlagged wiring, app-bind double-write removal, and quota cache save ordering.

Why this PR

This is a re-open of #469 (closed) with all CodeRabbit / greptile audit findings addressed in subsequent commits:

• c58444d wires clearAccounts as a function (not boolean) and corrects the launcher import path.
• e38f3b2 initializes releaseLock as a typed no-op and removes the false CI heuristic.
• 4cfbe24 / abfdadf / da0cec5 / 68b2850 address the remaining app-bind, transaction, and quota cache findings.
• 904ebaa repairs test regressions caused by the audit fixes.

Branch is rebased on the latest main (0 commits behind).

Test plan

• npm test passes locally (vitest specs include test/uninstall-command.test.ts, test/preuninstall.test.ts, test/install-codex-auth.test.ts, test/device-auth.test.ts).
• node scripts/preuninstall.js is a no-op in CI (npm_config_ignore_scripts=true).
• codex-multi-auth uninstall --dry-run --json reports planned actions without mutation.
• codex-multi-auth uninstall removes the plugin entry from Codex.json, restores the launcher, unbinds app rotation, and clears node_modules cache.
• bun.lock is preserved when other plugins remain in Codex.json.
• --clear-accounts clears stored credentials when supplied.

🤖 Generated with Claude Code

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

adds npm preuninstall lifecycle cleanup and codex-multi-auth uninstall CLI to reverse postinstall artifacts (app-bind, launcher, Codex.json plugin entry, node_modules cache). bundles correctness fixes surfaced during testing: restoreTopLevelResponseStorage missing !handled && originalLine splice, withStorageLock uninitialised releaseLock, hydrateRuntimeEmails patch-by-index to handle accounts sharing undefined accountId, parseAbsoluteExpirationMs for device-auth expires_at, and pump().catch() for stream-failover. all previously flagged findings (bare rmSync, loadCurrentFlagged wiring, config-toml restoration, hydrate-emails coverage) are addressed in this revision.

Confidence Score: 5/5

safe to merge — only P2 findings remain; no blocking bugs or security issues found

all P1 findings from prior review rounds have been addressed; the two remaining findings are both P2 (misleading dry-run log and non-atomic Codex.json write); test coverage is comprehensive including EBUSY retry, concurrent writes, dry-run idempotency, and all bun.lock decision-table branches; windows filesystem safety conventions (removeWithRetry in afterEach) are correctly followed

lib/codex-manager/commands/uninstall.ts and scripts/preuninstall.js — Codex.json write should be made atomic to match the install path pattern

Important Files Changed

Filename	Overview
lib/codex-manager/commands/uninstall.ts	new uninstall CLI command — solid windows retry logic, conservative bun.lock guard, and correct dry-run deferred launcher load; two P2 issues: misleading dry-run log when no plugins array, and non-atomic Codex.json write
scripts/preuninstall.js	npm lifecycle preuninstall script — CI/ignore-scripts detection correct, home resolution from env handled safely; same dry-run log and non-atomic write P2 as uninstall.ts; tests use removeWithRetry
lib/runtime/config-toml.ts	adds !handled && originalLine splice for both restoreTopLevelModelProvider and restoreTopLevelResponseStorage; removes premature early-return; test/config-toml-restore.test.ts covers all four branches
lib/storage/transactions.ts	releaseLock initialised as typed no-op, loadCurrentFlagged optional dep wired and passed to handler, flaggedStorage accounts spread-cloned before persist; transactions.test.ts covers both paths
lib/runtime/app-bind.ts	port-reuse guard tightened to require router !== null + state=running + isProcessAlive; orphan router cleanup on port=0 failure; post-stop liveness warning on unbind
lib/request/stream-failover.ts	readPromise hoisted above timeout windows to prevent silent chunk drop; pump() now propagates errors to controller.error() — concurrency fix correct
lib/auth/device-auth.ts	parseAbsoluteExpirationMs correctly handles numeric seconds, numeric ms, numeric strings, and ISO date strings; all four paths covered in test/device-auth.test.ts
lib/runtime/hydrate-emails.ts	patch-by-index replaces direct array assignment to avoid collapsing accounts sharing undefined accountId; test/hydrate-emails.test.ts covers all paths
test/uninstall-command.test.ts	comprehensive coverage of parseUninstallArgs, resolveUninstallPaths, and runUninstallCommand; uses removeWithRetry in afterEach; covers all bun.lock decision table branches
test/preuninstall.test.ts	uses removeWithRetry in afterEach; covers CI skip, bun.lock decision table, dry-run idempotency, and concurrent read-modify-write correctness
test/uninstall-ebusy-retry.test.ts	injects one-shot EBUSY via vi.mock for both readFile and rm; verifies withFileOperationRetry loop exits cleanly; uses removeWithRetry in afterEach

Sequence Diagram

sequenceDiagram
    participant User
    participant CLI as codex-multi-auth CLI
    participant Uninstall as uninstall.ts
    participant AppBind as app-bind.ts
    participant Launcher as codex-app-launcher.js
    participant Config as Codex.json
    participant Cache as Cache dirs

    User->>CLI: uninstall flags
    CLI->>Uninstall: runUninstallCommand
    Uninstall->>AppBind: unbindCodexAppRuntimeRotation
    AppBind-->>Uninstall: ok or partial failure
    Uninstall->>Launcher: remove launcher
    Launcher-->>Uninstall: ok or partial failure
    Uninstall->>Config: read parse removePluginFromList
    Config-->>Uninstall: bunLockState computed
    Uninstall->>Config: write updated plugins
    Uninstall->>Cache: rm node_modules with EBUSY retry
    alt bunLockState is safe
        Uninstall->>Cache: rm bun.lock
    end
    alt clear-accounts flag set
        Uninstall->>Uninstall: deps.clearAccounts
    end
    Uninstall-->>CLI: exit 0 or 1

Loading

Comments Outside Diff (5)

1. test/preuninstall.test.ts, line 1197-1203 (link)

   bare rmSync in afterEach — windows EBUSY/EPERM risk

   both new test files use rmSync(root, { recursive: true, force: true }) directly in afterEach cleanup. the project anti-pattern (test/AGENTS.md) explicitly says "do not use bare fs.rm in test cleanup; use removeWithRetry for Windows safety." on Windows these temp dirs can still have open handles immediately after a test completes, causing EBUSY/EPERM failures on CI. the same pattern appears in test/uninstall-command.test.ts afterEach as well.

   Context Used: test/AGENTS.md (source)

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: test/preuninstall.test.ts
   Line: 1197-1203
   
   Comment:
   **bare `rmSync` in afterEach — windows EBUSY/EPERM risk**
   
   both new test files use `rmSync(root, { recursive: true, force: true })` directly in `afterEach` cleanup. the project anti-pattern (test/AGENTS.md) explicitly says "do not use bare `fs.rm` in test cleanup; use `removeWithRetry` for Windows safety." on Windows these temp dirs can still have open handles immediately after a test completes, causing `EBUSY`/`EPERM` failures on CI. the same pattern appears in `test/uninstall-command.test.ts` `afterEach` as well.
   
   **Context Used:** test/AGENTS.md ([source](https://app.greptile.com/review/custom-context?memory=6e2636a9-e514-4bab-b249-4b4a84aa4ae3))
   
   How can I resolve this? If you propose a fix, please make it concise.

   

2. lib/request/stream-failover.ts, line 512-518 (link)

   pump().catch() path has no vitest coverage

   the change from void pump() to pump().catch((err) => controller.error(err)) ensures unhandled async errors from the pump coroutine reach the ReadableStream consumer instead of becoming silent unhandled rejections. this is a meaningful concurrency correctness fix, but test/stream-failover.test.ts has no test case that triggers an error thrown directly from the pump() body (as opposed to errors originating inside the primary or failover stream reads). a fake-timer test that injects a pump-level throw would pin the contract.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: lib/request/stream-failover.ts
   Line: 512-518
   
   Comment:
   **`pump().catch()` path has no vitest coverage**
   
   the change from `void pump()` to `pump().catch((err) => controller.error(err))` ensures unhandled async errors from the pump coroutine reach the `ReadableStream` consumer instead of becoming silent unhandled rejections. this is a meaningful concurrency correctness fix, but `test/stream-failover.test.ts` has no test case that triggers an error thrown directly from the `pump()` body (as opposed to errors originating inside the primary or failover stream reads). a fake-timer test that injects a pump-level throw would pin the contract.
   
   How can I resolve this? If you propose a fix, please make it concise.

   

3. lib/runtime/config-toml.ts, line 587-591 (link)

   no vitest coverage for the !handled && originalLine restoration fix

   the !handled && originalLine guard added to restoreTopLevelModelProvider is a real bug fix — without it, if the model_provider line appears nowhere in currentConfig (e.g. the bind wrote it but it was later stripped by another tool), the original line is never appended back. there are no tests in the current suite exercising this path for either restoreTopLevelModelProvider or the parallel restoreTopLevelResponseStorage originalLine-absent branch. both are called on every unbind; untested regressions here would silently corrupt config.toml.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: lib/runtime/config-toml.ts
   Line: 587-591
   
   Comment:
   **no vitest coverage for the `!handled && originalLine` restoration fix**
   
   the `!handled && originalLine` guard added to `restoreTopLevelModelProvider` is a real bug fix — without it, if the `model_provider` line appears nowhere in `currentConfig` (e.g. the bind wrote it but it was later stripped by another tool), the original line is never appended back. there are no tests in the current suite exercising this path for either `restoreTopLevelModelProvider` or the parallel `restoreTopLevelResponseStorage` `originalLine`-absent branch. both are called on every unbind; untested regressions here would silently corrupt `config.toml`.
   
   How can I resolve this? If you propose a fix, please make it concise.

   

4. lib/runtime/hydrate-emails.ts, line 622-633 (link)

   patch-by-ID rewrite of storage.accounts has no vitest coverage

   the switch from storage.accounts = accountsCopy to building a patchById map and merging per accountId is a correctness fix — it avoids dropping accounts that weren't included in accountsCopy. no test in the repo exercises this function (hydrateRuntimeEmails has no dedicated test file). a missed regression here could silently lose account records on hydration when accountsCopy is a partial slice.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: lib/runtime/hydrate-emails.ts
   Line: 622-633
   
   Comment:
   **patch-by-ID rewrite of `storage.accounts` has no vitest coverage**
   
   the switch from `storage.accounts = accountsCopy` to building a `patchById` map and merging per `accountId` is a correctness fix — it avoids dropping accounts that weren't included in `accountsCopy`. no test in the repo exercises this function (`hydrateRuntimeEmails` has no dedicated test file). a missed regression here could silently lose account records on hydration when `accountsCopy` is a partial slice.
   
   How can I resolve this? If you propose a fix, please make it concise.

   

5. lib/runtime/config-toml.ts, line 154-191 (link)

   !handled && originalLine guard missing from restoreTopLevelResponseStorage

   restoreTopLevelModelProvider received a post-loop block (lines 138-149) that splices originalLine back into the root table when the bind-written model_provider line was stripped from currentConfig. restoreTopLevelResponseStorage never got the equivalent block.

   Concrete failure: user originally had disable_response_storage = true. Bind overwrites it with disable_response_storage = false. User or another tool deletes that line. On unbind, handled stays false, originalLine is non-null, but no code re-inserts it — the setting is silently dropped and the user's preference is lost. The same fix pattern applies:

   if (!handled && originalLine) {
       const firstSectionIdx = output.findIndex(
           (line) => readTomlTableName(line) !== null,
       );
       if (firstSectionIdx === -1) {
           output.push(originalLine);
       } else {
           output.splice(firstSectionIdx, 0, originalLine);
       }
   }

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: lib/runtime/config-toml.ts
   Line: 154-191
   
   Comment:
   **`!handled && originalLine` guard missing from `restoreTopLevelResponseStorage`**
   
   `restoreTopLevelModelProvider` received a post-loop block (lines 138-149) that splices `originalLine` back into the root table when the bind-written `model_provider` line was stripped from `currentConfig`. `restoreTopLevelResponseStorage` never got the equivalent block.
   
   Concrete failure: user originally had `disable_response_storage = true`. Bind overwrites it with `disable_response_storage = false`. User or another tool deletes that line. On unbind, `handled` stays `false`, `originalLine` is non-null, but no code re-inserts it — the setting is silently dropped and the user's preference is lost. The same fix pattern applies:
   
   ```ts
   if (!handled && originalLine) {
       const firstSectionIdx = output.findIndex(
           (line) => readTomlTableName(line) !== null,
       );
       if (firstSectionIdx === -1) {
           output.push(originalLine);
       } else {
           output.splice(firstSectionIdx, 0, originalLine);
       }
   }
   ```
   
   How can I resolve this? If you propose a fix, please make it concise.

   

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
lib/codex-manager/commands/uninstall.ts:225-230
**Misleading dry-run log when config has no `plugins` array**

the `else if (dryRun)` branch fires when `Codex.json` exists but has no `plugins` field — e.g. a bare `{}` or a schema-less config. in that case nothing needs to be removed, yet the log still says `"[dry-run] Would remove codex-multi-auth from <path>"`, giving the user false confidence that an entry was found. the same pattern exists in `scripts/preuninstall.js` at the equivalent branch. a clearer message such as `"[dry-run] codex-multi-auth not found in <path> (no plugins field)"` would avoid the ambiguity.

### Issue 2 of 2
lib/codex-manager/commands/uninstall.ts:237-244
**non-atomic `Codex.json` write risks partial-write corruption**

`writeFile` overwrites in-place — a SIGKILL or power loss mid-write leaves truncated JSON. the install path already uses an atomic temp-file+rename pattern (`atomicWritePluginList` in `install-codex-auth-utils.js`). the uninstall path should match: write to a sibling `.tmp`, then `rename`. on windows the rename is atomic (`MoveFileExW`) and `EBUSY` on rename is retried. without this, a crashed uninstall leaves `Codex.json` unparseable, which the conservative `bun.lock` guard then treats as "uncertain" — but the plugin list is gone. the same pattern applies to the equivalent `writeFile` call in `scripts/preuninstall.js`.

_{Reviews (8): Last reviewed commit: "fix: address coderabbit's third-round fi..." | Re-trigger Greptile}

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[coderabbitai]

📝 Walkthrough

Walkthrough

this PR adds a full uninstall CLI plus an npm preuninstall hook for removing codex-multi-auth, changes device auth parsing to treat expires_at as an absolute timestamp, and hardens several runtime/storage areas with file-size checks, retry wrappers, and safer state updates.

Changes

uninstall flow

Layer / File(s)	Summary
cli dispatch & help lib/codex-manager.ts:79, 208-212, 3609-3611, lib/codex-manager/help.ts:19	imports and dispatches runUninstallCommand, registers "uninstall" in the command whitelist, and updates usage text.
paths & plugin util lib/codex-manager/commands/uninstall.ts:41-69, scripts/install-codex-auth-utils.js:90-98	implements resolveUninstallPaths(...) for platform-specific config/cache locations and exports removePluginFromList(list) to strip codex-multi-auth (and codex-multi-auth@*) from plugin arrays.
retry framework lib/codex-manager/commands/uninstall.ts:7-39	adds retryable fs error set, exponential-backoff jitter, and withFileOperationRetry wrapper used by uninstall operations.
cli parsing & types lib/codex-manager/commands/uninstall.ts:71-128	adds UninstallCliOptions, ParsedUninstallArgs, parseUninstallArgs, and printUninstallUsage.
core uninstall implementation lib/codex-manager/commands/uninstall.ts:158-363	implements runUninstallCommand that unbinds runtime rotation, removes OS launcher (pluggable/dynamically loaded), edits Codex.json (removes plugin, computes bun.lock safety), clears caches with retry, optionally calls clearAccounts, records removed/warnings and returns 0/1 on success/partial-failure.
npm lifecycle hook scripts/preuninstall.js:1-254, package.json:112-113	adds runPreuninstallCleanup script that runs preuninstall logic (unbind, remove launcher, remove plugin from Codex.json, clear caches) and wires it as the preuninstall npm lifecycle script.
integration tests test/uninstall-command.test.ts:*, test/preuninstall.test.ts:*, test/uninstall-ebusy-retry.test.ts:*, test/codex-manager-uninstall-dispatch.test.ts:*	comprehensive tests cover arg parsing, path resolution, dry-run behavior, multiple Codex.json states (missing/corrupt/other-plugins), retry on EBUSY for read/rm, launcher error partial-failure behavior, and dispatcher wiring for clearAccounts.

device auth expires_at parsing

Layer / File(s)	Summary
absolute expiration parser lib/auth/device-auth.ts:244-260	adds parseAbsoluteExpirationMs(value) that accepts epoch seconds, epoch milliseconds, numeric strings, or date strings and normalizes to ms; returns null for invalid values.
payload parsing lib/auth/device-auth.ts:296-303	parseDeviceCodePayload now prefers parseAbsoluteExpirationMs(payload.expires_at) for expiresAtMs and falls back to existing relative expires_in parsing when needed.
tests test/device-auth.test.ts:83-173	adds four tests validating numeric seconds, epoch-ms boundary, numeric-string seconds, and fallback to expires_in.

runtime & storage robustness

Layer / File(s)	Summary
status file size checks lib/runtime/runtime-current-account.ts:52,133-138, lib/codex-manager/commands/rotation.ts:443-454	both readAppRuntimeHelperStatus() functions add statSync size cap (MAX_STATUS_FILE_BYTES = 1 MB) and return null if stat fails or file is too large before JSON parse.
app-bind safety lib/runtime/app-bind.ts:696-716,759-763	tightens reuse of existing router state only when router was not newly started and status shows a running alive PID; attempts best-effort stop and re-read before throwing on port<=0; warns if PID still alive after stop.
config toml restore behavior lib/runtime/config-toml.ts:135-150,160-207	restoreTopLevelModelProvider and restoreTopLevelResponseStorage now splice original top-level lines back into the root table when not handled during scanning, and drop bind-time residue appropriately.
hydrate emails patching lib/runtime/hydrate-emails.ts:87-97	applies hydrated account changes by index-mapped patching (storage.accounts = storage.accounts.map(...)) instead of replacing the array, preserving shape/order and saving only when changed.
flagged storage in transactions lib/storage/transactions.ts:29-32,71-126	withStorageLock initializes releaseLock to a no-op; withAccountAndFlaggedStorageTransaction accepts optional loadCurrentFlagged, calls it to produce currentFlagged (or defaults {version:1,accounts:[]}), passes it as a third handler arg, and persists flagged storage via a shallow clone of accounts.
quota-cache timing lib/codex-manager/repair-commands.ts:1493-1519,1556-1563	saves quota cache immediately after account-storage transactions (when changed and not dry-run), catches/save errors into quotaCacheSaveError, includes that in JSON output, and logs a warning in non-JSON runs.
stream failover error routing lib/request/stream-failover.ts:92-95,230-236	documents reader.read() hoisting to reuse an in-flight read across soft/hard timeouts; replaces void pump() with pump().catch(...) to forward pump errors to controller.error, ignoring errors when controller closed.
tests for transactions & stream failover test/transactions.test.ts:41-97, test/stream-failover.test.ts:208-258	adds tests ensuring loadCurrentFlagged is forwarded to handler, default flagged storage is provided when loader omitted, concurrent transaction lock release after rejection, and regression test for no unhandledRejection when reader.releaseLock throws during failover.

Sequence Diagram

sequenceDiagram
    actor user as user
    participant cli as codex-multi-auth cli
    participant unbind as unbindCodexAppRuntimeRotation()
    participant launcher as os launcher removal
    participant config as Codex.json
    participant cache as node_modules & bun.lock
    participant accounts as clearAccounts

    user->>cli: codex-multi-auth uninstall [--clear-accounts]
    cli->>cli: parse args (`lib/codex-manager/commands/uninstall.ts:71-128`)
    cli->>unbind: attempt unbind (optional,dynamic) (`scripts/preuninstall.js:65-87`)
    unbind-->>cli: ok / warn
    cli->>launcher: remove launcher (optional,dynamic) (`scripts/preuninstall.js:88-103`)
    launcher-->>cli: ok / warn
    cli->>config: read & edit Codex.json with `removePluginFromList` (`lib/codex-manager/commands/uninstall.ts:158-363`, `scripts/install-codex-auth-utils.js:90-98`)
    config-->>cli: updated plugins, compute bun.lock safety
    cli->>cache: remove node_modules (retry) and conditionally bun.lock (`lib/codex-manager/commands/uninstall.ts:158-363`)
    cache-->>cli: ok / warn
    alt --clear-accounts
        cli->>accounts: call clearAccounts() if wired
        accounts-->>cli: ok / warn
    end
    cli-->>user: json or human summary and exit code 0/1

Loading

missing or notable test/edge coverage

• windows edge cases: several uninstall path computations and path assumptions exist in lib/codex-manager/commands/uninstall.ts:41-69 and scripts/preuninstall.js:104-176. tests cover xdg-like and windows defaults (test/uninstall-command.test.ts:94-115) but verify that windows env variations are exercised in CI. ensure additional windows-specific filesystem permission/locking behaviors are tested.
• concurrency risks: multiple file mutations with retries (config edits + cache rm) in runUninstallCommand and scripts/preuninstall.js could race when invoked concurrently against the same home; tests include a parallel preuninstall concurrency run (test/preuninstall.test.ts:268-303) but review should validate atomicity guarantees and whether withFileOperationRetry plus partial-failure tracking sufficiently prevents inconsistent state.
• missing regression tests: device-auth absolute parsing is covered (test/device-auth.test.ts:83-173). no additional regression tests noted for app-bind router restart fallback changes (lib/runtime/app-bind.ts:696-716) or for the stricter status-file size cap (lib/runtime/runtime-current-account.ts:133-138, lib/codex-manager/commands/rotation.ts:443-454); consider adding targeted tests for those guards.

estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Suggested labels

bug, feature, enhancement

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 22.45% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	title follows conventional commits format (fix: lowercase summary ≤72 chars) and accurately describes the main change: adding preuninstall hook and uninstall CLI command.
Linked Issues check	✅ Passed	code changes fully address #468 objectives: npm preuninstall cleanup, manual uninstall CLI with --dry-run/--json/--clear-accounts flags, conservative bun.lock deletion, launcher removal, plugin entry cleanup, and app-bind unbind.
Out of Scope Changes check	✅ Passed	all code changes align with uninstall objectives or are necessary correctness fixes surfaced during testing (device-auth expires_at parsing, config-toml restoration, stream-failover pump().catch(), storage transactions wiring, app-bind liveness checks, quota-cache save ordering, hydrate-emails patch-by-index).
Description check	✅ Passed	PR description comprehensively covers summary, changes, validation steps, test plan, and risk/rollback context with clear justification.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

📋 Issue Planner

Built with CodeRabbit's Coding Plans for faster development and fewer bugs.

View plan used: #468

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/uninstall-cleanup

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch fix/uninstall-cleanup

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[ndycode]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 14

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/commands/rotation.ts (1)

443-473: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

add a vitest for the new helper-status size guard.

the 1 mb cap now exists in both lib/codex-manager/commands/rotation.ts:445-473 and lib/runtime/runtime-current-account.ts:130-156, but i do not see a focused regression test in the provided context for oversized or unreadable status files. this path silently downgrades to null, so the two copies can drift again without a test.

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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@lib/codex-manager/commands/rotation.ts` around lines 443 - 473, Add a vitest
that asserts readAppRuntimeHelperStatus (in rotation.ts) returns null when the
helper-status file exceeds MAX_STATUS_FILE_BYTES (1MB) and similarly add/extend
a test for the analogous guard in runtime-current-account.ts; create a temporary
status file >1MB and verify the function returns null (and does not throw or
leak contents), also include a test case for unreadable/corrupted JSON to ensure
it returns null; reference readAppRuntimeHelperStatus and the size-guard
constant MAX_STATUS_FILE_BYTES to locate the code, and ensure tests run on
Windows-style paths/permissions to catch EBUSY-like behaviors without logging
sensitive fields.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lib/codex-manager.ts`:
- Around line 3609-3611: Add a vitest that exercises the dispatcher branch by
calling runCodexMultiAuthCli(["uninstall", "--clear-accounts"]) and asserting
the supplied storage/uninstall handler (the handler passed into
runUninstallCommand) is invoked with clearAccounts=true; update or add to
test/uninstall-command.test.ts alongside the existing cases (around 331-373) to
mirror the handler-based call path instead of directly invoking
runUninstallCommand. Ensure the test registers a spy/mock for the storage
handler used by runUninstallCommand so the assertion verifies the dispatcher
wiring, and keep the test free of sensitive logging (no tokens/emails) and
robust to Windows FS/E BUSY or 429-style retry semantics in other tests by using
deterministic mocks.

In `@lib/codex-manager/commands/uninstall.ts`:
- Around line 207-217: The uninstall command currently calls await
loadDefaultLauncher() before checking dryRun, causing preview (--dry-run) to
import the launcher module; change the logic in uninstall.ts so
loadDefaultLauncher() is only called when not in dryRun (i.e., move the await
loadDefaultLauncher() behind a !dryRun branch and only call removeLauncher when
!dryRun), and update deps handling so deps.removeLauncher still overrides
behavior; also add a regression test in test/uninstall-command.test.ts that runs
the default uninstall with --dry-run (no deps override) to assert it completes
without requiring scripts/codex-app-launcher.js to be present.

In `@lib/codex-manager/repair-commands.ts`:
- Around line 1493-1495: The code currently awaits saveQuotaCache() after
account-storage commits and lets any saveQuotaCache error bubble up, turning a
partial success into a hard failure; change the block in runFix() that calls
saveQuotaCache(workingQuotaCache) to catch errors (especially EBUSY/EPERM on
Windows) and do not rethrow—log a warning via the existing logger and record a
partial-failure marker on the run result (e.g.,
result.partialFailures.quotaCache = { code: err.code, message: err.message }) so
the overall run still succeeds; add a vitest that mocks saveQuotaCache to throw
an EBUSY/EPERM and asserts account save succeeded while the returned run report
contains the quotaCache partialFailure and the process does not throw.

In `@lib/request/stream-failover.ts`:
- Around line 230-236: Add a regression test in test/stream-failover.test.ts
that forces the pump() promise to reject while it is actively calling
controller.enqueue() so the .catch branch at lib/request/stream-failover.ts (the
pump().catch((err) => { try { controller.error(err) } catch { } })) is executed;
specifically, create a reader/read loop that starts pump(), then cancel the
reader (or close the underlying stream) in the middle of an enqueue so
controller.enqueue() throws and pump() rejects, and assert that no double error
is emitted and the code does not throw (i.e., stream handling swallows the
secondary controller.error() throw). Ensure the test references pump(),
controller.enqueue(), controller.error(), and uses cancel()/reader.cancel() to
trigger the rejection during enqueue.

In `@lib/runtime/app-bind.ts`:
- Around line 696-699: The branch that reuses existingState currently only
checks router !== null and can reuse a stale or dead router; update the
condition in the else-if that references existingState to also require
router.state === "running" && isProcessAlive(router.pid) (and ensure
spawnDetached is considered) before assigning port = existingState.port and
baseUrl = existingState.baseUrl so we only reuse a live router; additionally add
a vitest that simulates a stale status/dead-router (router.state !== "running"
or isProcessAlive returns false) to verify the code falls back and does not
rewrite config.toml to a non-listening baseUrl.

In `@lib/runtime/config-toml.ts`:
- Around line 171-174: Add a unit test that exercises the originalLine === null
branch of restoreTopLevelResponseStorage: create an "original" TOML string that
does NOT contain disable_response_storage, create a "current" TOML string that
does contain the residue line disable_response_storage = false (the state
produced by enableTopLevelResponseStorage when the key didn't previously exist),
call restoreTopLevelResponseStorage with those inputs, and assert the returned
config does NOT contain any disable_response_storage line; reference
restoreTopLevelResponseStorage and enableTopLevelResponseStorage in
lib/runtime/config-toml.ts and add the test alongside the existing tests in
test/app-bind.test.ts.
- Around line 138-141: The fallback that appends originalLine when !handled
currently pushes the model_provider into the last TOML section; update the
restore path in rewriteTopLevelModelProvider to mirror the write path by finding
the first section header (line starting with '[') in output and splicing
originalLine immediately before that index, falling back to push if no section
header exists; also add a unit test that simulates currentConfig missing the
runtime provider while originalConfig contains model_provider to ensure the
restored line lands in the root table (refer to function
rewriteTopLevelModelProvider and the restore loop handling !handled &&
originalLine).

In `@lib/runtime/hydrate-emails.ts`:
- Around line 88-97: The code builds patchById keyed only by accountId which
collapses entries with undefined accountId and lets duplicate accountIds
overwrite each other; update the merge in hydrate-emails.ts so patches are keyed
by a stable original-array identifier (e.g., use the accountsCopy index or a
composite key like `${accountId ?? 'undefined'}:${index}`) and then apply
patches by matching that stable key to the corresponding storage.accounts entry
(use the same index or stored composite key) instead of
patchById.get(account.accountId); add a vitest regression test that hydrates
multiple accounts with accountId === undefined to ensure no cross-contamination,
and ensure any logging in this flow does not print tokens/emails and conforms to
existing EBUSY/429 retry handling expectations.

In `@scripts/preuninstall.js`:
- Around line 119-156: During dry-run the code skips reading Codex.json so
bunLockState remains "uncertain"; fix by applying the same bun.lock safety
decision logic used in the uninstall preview branch to the dryRun path: when
dryRun is true, still attempt to determine bunLockState by reading
paths.configPath (using readFile/JSON.parse and Array.isArray(config.plugins))
and setting bunLockState to "safe" when the file is ENOENT or when
config.plugins becomes empty after removePluginFromList, otherwise "uncertain";
keep the existing error handling (fileError/code === "ENOENT") and the
withFileOperationRetry/writeFile usage for the non-dry path, and add a vitest
regression in test/preuninstall.test.ts that asserts dry-run logs report
bun.lock as safe for both ENOENT and single-plugin (codex-multi-auth only)
scenarios so the dry-run output matches the preview decision table implemented
in uninstall.ts.

In `@test/preuninstall.test.ts`:
- Around line 53-242: Add a concurrency regression test that spawns two
simultaneous runPreuninstallCleanup invocations against the same temp home:
create a temp home using makeTempHome(), env via envFor(home), resolve paths
with resolveTempPaths(home), write Codex.json with plugins
["codex-multi-auth","other"], then call
Promise.all([runPreuninstallCleanup(opts), runPreuninstallCleanup(opts)]) where
opts includes env, home, log, unbindCodexApp, removeLauncher, clearCache;
finally read and JSON.parse(paths.configPath) and assert the resulting
config.plugins does not contain "codex-multi-auth" and still contains "other";
name the test "concurrent invocations leave Codex.json in a consistent state"
and add it to the same describe("runPreuninstallCleanup", ...) block.
- Around line 39-51: The test's resolveTempPaths duplicates production path
logic and should be replaced by calling the real resolver: import the production
function resolveInstallPaths (from the module that currently defines it, e.g.,
lib/storage/paths or lib/runtime-paths) and invoke resolveInstallPaths with the
temporary home value instead of duplicating platform-specific logic in
resolveTempPaths; ensure the test uses the same return shape/field names as
resolveInstallPaths (map/rename fields if necessary) so assertions remain
identical to production behavior.
- Line 112: The test contains a trivially-true filesystem assertion: remove or
change the existsSync(paths.cacheBunLock) check because clearCache (the stub at
clearCache) never touches the fs so the file written earlier will always exist;
either delete the existsSync assertion or switch the test to call the real
clearCache implementation (instead of the stub) and then assert the file remains
present, and keep the meaningful check
expect(removedBunLock).not.toHaveBeenCalled() intact to validate removal was not
invoked.

In `@test/transactions.test.ts`:
- Around line 41-97: Add a deterministic vitest regression that exercises the
withStorageLock mutex-release path: write a queued-transaction test using
withAccountAndFlaggedStorageTransaction where you call it twice in sequence (or
trigger queueing) with the first handler rejecting (throwing) and the second
resolving, and assert the second handler runs despite the first rejection;
reference the withStorageLock implementation in lib/storage/transactions.ts and
the withAccountAndFlaggedStorageTransaction helper, use vi.fn() spies for
handlers and loadCurrentFlagged/loadCurrent stubs, ensure the test awaits both
transactions and verifies the mutex was released (e.g., second handler called
once and no hanging promise), and keep the test deterministic using async/await
(no real timers or filesystem races).

In `@test/uninstall-command.test.ts`:
- Around line 117-399: Add a deterministic Vitest that targets the retry branch
in the uninstall cleanup loop by making one filesystem call (e.g.,
fs.promises.rm or fs.promises.writeFile/readFile used by runUninstallCommand)
fail once with an Error { code: "EBUSY" } (or "EPERM") and then succeed on the
next call; use vi.spyOn or vi.fn to stub the specific fs.promises method
referenced by the retry loop in lib/codex-manager/commands/uninstall.ts, have
the stub throw the EBUSY error on its first invocation and return normally
thereafter, call runUninstallCommand (or the same helper used across the tests)
with appropriate stubbed paths, and assert the command returns success and that
the filesystem state is correct (i.e., retry succeeded) while ensuring the test
is deterministic and cleans up mocks.

---

Outside diff comments:
In `@lib/codex-manager/commands/rotation.ts`:
- Around line 443-473: Add a vitest that asserts readAppRuntimeHelperStatus (in
rotation.ts) returns null when the helper-status file exceeds
MAX_STATUS_FILE_BYTES (1MB) and similarly add/extend a test for the analogous
guard in runtime-current-account.ts; create a temporary status file >1MB and
verify the function returns null (and does not throw or leak contents), also
include a test case for unreadable/corrupted JSON to ensure it returns null;
reference readAppRuntimeHelperStatus and the size-guard constant
MAX_STATUS_FILE_BYTES to locate the code, and ensure tests run on Windows-style
paths/permissions to catch EBUSY-like behaviors without logging sensitive
fields.

🪄 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: 8ab68f1c-5ef9-483f-9803-e1a943782855

📥 Commits

Reviewing files that changed from the base of the PR and between f5ed52d and f447ac4.

📒 Files selected for processing (20)

• lib/auth/device-auth.ts
• lib/codex-manager.ts
• lib/codex-manager/commands/rotation.ts
• lib/codex-manager/commands/uninstall.ts
• lib/codex-manager/help.ts
• lib/codex-manager/repair-commands.ts
• lib/request/stream-failover.ts
• lib/runtime/app-bind.ts
• lib/runtime/config-toml.ts
• lib/runtime/hydrate-emails.ts
• lib/runtime/runtime-current-account.ts
• lib/storage/transactions.ts
• package.json
• scripts/install-codex-auth-utils.js
• scripts/preuninstall.js
• test/device-auth.test.ts
• test/install-codex-auth.test.ts
• test/preuninstall.test.ts
• test/transactions.test.ts
• test/uninstall-command.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/runtime/hydrate-emails.ts
• lib/codex-manager/help.ts
• lib/runtime/runtime-current-account.ts
• lib/codex-manager/commands/rotation.ts
• lib/codex-manager.ts
• lib/request/stream-failover.ts
• lib/storage/transactions.ts
• lib/codex-manager/repair-commands.ts
• lib/runtime/config-toml.ts
• lib/runtime/app-bind.ts
• lib/codex-manager/commands/uninstall.ts
• lib/auth/device-auth.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/transactions.test.ts
• test/device-auth.test.ts
• test/preuninstall.test.ts
• test/uninstall-command.test.ts
• test/install-codex-auth.test.ts

🔇 Additional comments (2)

lib/request/stream-failover.ts (1)

92-96: hoisted readPromise comment is accurate — lgtm.

the comment correctly explains the invariant: a single in-flight reader.read() promise is shared across the soft and hard timeout windows. re-issuing reader.read() after a soft timeout would silently drop the first chunk if it resolved between the two calls.

package.json (1)

112-112: ⚡ Quick win

no action needed — guard is already in place.

the top-level invocation in scripts/preuninstall.js:169-180 correctly guards against unhandled rejections with .catch((error) => { ... process.exitCode = 0; }). the pattern prevents npm from blocking uninstall if a dynamic import() fails before runPreuninstallCleanup() completes.

[ndycode]

@coderabbitai review

The original CHANGES_REQUESTED findings have been addressed across commits fd8097e (six P0 correctness fixes — hydrate-emails undefined-accountId Map collapse → index-stable, repair-commands saveQuotaCache wrapped in try/catch with quotaCacheSaveError surfaced, app-bind reuse predicate strengthened to require live router, config-toml restoreTopLevelModelProvider splice-before-section, uninstall.ts dry-run no longer loads launcher module, preuninstall.js dry-run runs the same Codex.json read/decision block as the live path), 84f9b32 (regression tests in test/hydrate-emails.test.ts and test/config-toml-restore.test.ts pin the bug fixes; test/transactions.test.ts now covers the withStorageLock release-on-rejection path; trivially-true existsSync assertion dropped from test/preuninstall.test.ts; new dry-run state-machine assertions in test/uninstall-command.test.ts and test/preuninstall.test.ts), and 86c461b (parallel restoreTopLevelResponseStorage !handled && originalLine splice that mirrors the modelProvider fix). Greptile re-reviewed at Confidence Score 5/5 with 0 new comments. Please re-evaluate.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check the latest state of the key files mentioned in the fixes

# Check hydrate-emails.ts for the index-stable fix
echo "=== lib/runtime/hydrate-emails.ts ==="
cat lib/runtime/hydrate-emails.ts 2>/dev/null || echo "NOT FOUND"

# Check repair-commands.ts for saveQuotaCache try/catch
echo ""
echo "=== lib/codex-manager/repair-commands.ts ==="
cat lib/codex-manager/repair-commands.ts 2>/dev/null || echo "NOT FOUND"

# Check app-bind.ts for reuse predicate fix
echo ""
echo "=== lib/runtime/app-bind.ts ==="
cat lib/runtime/app-bind.ts 2>/dev/null || echo "NOT FOUND"

# Check config-toml.ts for both restore fixes
echo ""
echo "=== lib/runtime/config-toml.ts ==="
cat lib/runtime/config-toml.ts 2>/dev/null || echo "NOT FOUND"

# Check uninstall.ts dry-run path
echo ""
echo "=== lib/codex-manager/commands/uninstall.ts ==="
cat lib/codex-manager/commands/uninstall.ts 2>/dev/null || echo "NOT FOUND"

# Check preuninstall.js dry-run path
echo ""
echo "=== scripts/preuninstall.js ==="
cat scripts/preuninstall.js 2>/dev/null || echo "NOT FOUND"

# Check stream-failover for pump().catch()
echo ""
echo "=== lib/request/stream-failover.ts ==="
cat lib/request/stream-failover.ts 2>/dev/null || echo "NOT FOUND"

# Check transactions.ts for releaseLock and withStorageLock rejection path
echo ""
echo "=== lib/storage/transactions.ts ==="
cat lib/storage/transactions.ts 2>/dev/null || echo "NOT FOUND"

Length of output: 132622

---

@ndycode i've gone through the code for all six p0 fixes across those three commits. here's what i found:

---

lib/runtime/hydrate-emails.ts:86-90 — patch-by-index confirmed. accountsCopy is built via .map so indices align with storage.accounts. accountsToHydrate items are the same object refs as accountsCopy items, so parallel mutations in the Promise.all propagate correctly before the final patch. ✅

lib/codex-manager/repair-commands.ts — saveQuotaCache is outside the json-render block, in its own try/catch. quotaCacheSaveError is surfaced in both the json path and the human-readable path. changed stays accountStorageChanged-only which matches the stated intent (quota cache is a derived artifact). ✅

lib/runtime/app-bind.ts — the reuse else if branch now correctly gates on router !== null && router.state === "running" && isProcessAlive(router.pid). stale status JSON from a dead router no longer passes. ✅

lib/runtime/config-toml.ts — both restoreTopLevelModelProvider and restoreTopLevelResponseStorage use output.findIndex(line => readTomlTableName(line) !== null) + output.splice(firstSectionIdx, 0, originalLine). the 86c461b parallel fix for restoreTopLevelResponseStorage mirrors modelProvider correctly. the !originalLine drop-residue case is also handled correctly. ✅

lib/codex-manager/commands/uninstall.ts — loadDefaultLauncher() is called only inside the else (non-dry-run) branch. dry-run path logs without touching the module. ✅

scripts/preuninstall.js — readFile + JSON.parse + removePluginFromList runs regardless of dryRun; only writeFile is gated. bunLockState is set correctly on both paths, including the ENOENT → "safe" case. ✅

lib/storage/transactions.ts:32 — let releaseLock: () => void = () => undefined satisfies strict TS and guarantees .finally() always fires. withAccountAndFlaggedStorageTransaction correctly loads currentFlagged (or falls back to { version: 1, accounts: [] }) and threads it as the third handler argument. ✅

lib/request/stream-failover.ts — pump().catch((err) => { try { controller.error(err); } catch { /* already closed */ } }) swallows the double-error case cleanly. ✅

---

one minor tech-debt item worth a follow-up (not a blocker):

lib/codex-manager/commands/uninstall.ts has a local copy of withFileOperationRetry that duplicates the export in scripts/install-codex-auth-utils.js. both use the same retry codes / backoff logic — worth consolidating into a shared module in a follow-up to avoid drift.

would you like me to open a follow-up issue for that duplication?

[approve]

[coderabbitai]

Actionable comments posted: 11

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lib/codex-manager/repair-commands.ts`:
- Around line 1494-1496: Multiple call sites invoke saveQuotaCache (e.g., the
call inside saveQuotaCache in repair-commands.ts, and the callers in
lib/codex-manager.ts and lib/codex-manager/commands/forecast.ts) without error
handling, causing EBUSY windows race issues; update those callers to wrap
saveQuotaCache in a try/catch or extract a small helper (e.g.,
trySaveQuotaCache(cache): Promise<string|null>) that performs the save inside
try/catch and returns any error message (or null on success), then replace
direct calls to saveQuotaCache with calls to the new helper or add equivalent
try/catch blocks around saveQuotaCache in the identified functions
(saveQuotaCache, forecast handlers and codex-manager callers) so failures are
logged/handled instead of crashing.

In `@scripts/preuninstall.js`:
- Around line 9-14: There are two divergent implementations of
removePluginFromList (one used by scripts/preuninstall.js and another in
lib/codex-manager/commands/uninstall.ts) that differ in null-handling;
consolidate by exporting a single canonical removePluginFromList from
install-codex-auth-utils.js that normalizes input (e.g., const list = (input ||
[]).filter(Boolean)) before removing the target plugin, then update callers
(preuninstall.js and uninstall.ts) to import and use that shared function so
both CLI and pre-uninstall behavior match exactly.

In `@test/codex-manager-cli.test.ts`:
- Around line 10931-10989: The test for quota cache save failure currently shows
the warning but does not verify that primary account mutations persist before
the quota cache fails. To fix this, enhance the test by adding a stale-token
scenario that forces a token refresh, ensuring an account change occurs and
persists first. Then, assert this persistence explicitly before confirming the
quota cache save failure results in a soft warning rather than a hard failure.
Focus on modifying the test function (the one with the description about quota
cache save failure) in test/codex-manager-cli.test.ts.

In `@test/config-toml-restore.test.ts`:
- Around line 1-46: Add a new test in test/config-toml-restore.test.ts that
feeds restoreTopLevelModelProvider CRLF-terminated strings (use '\r\n' line
endings) for both original and current inputs and asserts the restored output
preserves CRLFs and the model_provider line positioning (e.g., equality when
full round-trip, or contains provider before first section when current omits
it); reference the restoreTopLevelModelProvider function to locate where to call
it and mirror one of the existing test scenarios but with '\r\n' line endings to
ensure Windows line-ending behavior is covered.

In `@test/hydrate-emails.test.ts`:
- Around line 59-127: Add a regression test to assert the "changed === false"
path in hydrateRuntimeEmails by ensuring saveAccounts is not called when every
queuedRefresh returns { type: "failed" }; create a minimal storage row via
makeStorage (e.g., one account with refreshToken), stub queuedRefresh to always
return a failed TokenResult, set
extractAccountId/extractAccountEmail/sanitizeEmail/shouldUpdateAccountIdFromToken
to return values that prevent any update, call hydrateRuntimeEmails with the
saveAccounts spy, and expect saveAccounts.not.toHaveBeenCalled() to guard
against regressions that unconditionally call saveAccounts.
- Around line 96-126: The test "preserves untouched accounts when only some are
hydrated" declares saveAccounts as a mock (saveAccounts = vi.fn(...)) but never
asserts it was called; add an assertion after the call to hydrateRuntimeEmails
to verify saveAccounts was invoked exactly once
(expect(saveAccounts).toHaveBeenCalledTimes(1)) so the test fails if
saveAccounts is not called or called multiple times; locate the saveAccounts
mock in that test and place the assertion right after the
storage/email/accountId expectations.
- Around line 8-28: withTestEnv restores environment variables in the finally
block synchronously which can occur before the async work inside fn() completes;
update withTestEnv to either use vitest's vi.stubEnv/vi.unstubAllEnvs to manage
env vars automatically or make withTestEnv async and await fn() inside the try
block so the finally block runs only after the returned promise resolves; change
the function signature (withTestEnv) and its call sites accordingly and ensure
hydrateRuntimeEmails (and other callers) see the intended env state for the
duration of their async operations.

In `@test/stream-failover.test.ts`:
- Around line 209-259: The test currently never triggers the pump().catch()
regression because the patched releaseLock()'s throw is swallowed inside
releaseCurrentReader()'s try-catch; instead arrange a secondary failure inside
pump() by making controller.error throw (for example patch the upstream
ReadableStream's start to provide a controller whose error method throws or mark
the controller as closed so controller.error() throws) so that pump() itself
throws and the outer .catch path in pump() is exercised; update the test that
calls withStreamingFailover/response.text to assert the specific expected error
message (pin the rejection message) and remove relying on unscoped rejection
behavior, referencing the test helpers and functions releaseCurrentReader, pump,
withStreamingFailover, and response.text to locate where to change the
monkey-patch and assertions.

In `@test/transactions.test.ts`:
- Around line 99-120: The current test uses withAccountStorageTransaction to
exercise the withStorageLock finally path, but the past review requested
coverage for withAccountAndFlaggedStorageTransaction as well; add a parallel
test case that mirrors the existing "releases the storage lock when a queued
transaction rejects" scenario but calls withAccountAndFlaggedStorageTransaction
instead of withAccountStorageTransaction (use the same deps, failing/succeeding
helpers and order assertions) to ensure both lock chains are covered if they
diverge in future refactors; keep the same assertions and error/success
expectations as the original test.

In `@test/uninstall-command.test.ts`:
- Around line 29-42: The test currently duplicates production path-resolution
logic in pathsForTempHome; replace that duplication by importing and using the
production helper resolveUninstallPaths from the module under test instead of
pathsForTempHome. Remove the local pathsForTempHome function, import
resolveUninstallPaths, and call it with the temporary home value to derive
configPath, cacheNodeModules, cacheBunLock, configDir and cacheDir so the test
uses the exact production logic.

In `@test/uninstall-ebusy-retry.test.ts`:
- Around line 61-77: The test duplicates platform-specific path resolution in
pathsForTempHome; replace it by importing and calling the production resolver
(e.g., resolveUninstallPaths or resolveInstallPaths) so tests follow real logic.
Remove the pathsForTempHome function, add an import for the appropriate resolver
used in production, call that resolver with the temporary home (and any platform
override/mock if needed) and use the returned
configDir/configPath/cacheNodeModules/cacheBunLock values in the test, matching
the approach used in test/preuninstall.test.ts where the production resolver is
invoked.

🪄 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: ee1c4865-0a93-471c-b43d-cef3a796bfb9

📥 Commits

Reviewing files that changed from the base of the PR and between f447ac4 and 3fa7011.

📒 Files selected for processing (15)

• lib/codex-manager/commands/uninstall.ts
• lib/codex-manager/repair-commands.ts
• lib/runtime/app-bind.ts
• lib/runtime/config-toml.ts
• lib/runtime/hydrate-emails.ts
• scripts/preuninstall.js
• test/codex-manager-cli.test.ts
• test/codex-manager-uninstall-dispatch.test.ts
• test/config-toml-restore.test.ts
• test/hydrate-emails.test.ts
• test/preuninstall.test.ts
• test/stream-failover.test.ts
• test/transactions.test.ts
• test/uninstall-command.test.ts
• test/uninstall-ebusy-retry.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/hydrate-emails.test.ts
• test/config-toml-restore.test.ts
• test/codex-manager-uninstall-dispatch.test.ts
• test/transactions.test.ts
• test/stream-failover.test.ts
• test/uninstall-ebusy-retry.test.ts
• test/codex-manager-cli.test.ts
• test/preuninstall.test.ts
• test/uninstall-command.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/runtime/hydrate-emails.ts
• lib/runtime/config-toml.ts
• lib/codex-manager/repair-commands.ts
• lib/runtime/app-bind.ts
• lib/codex-manager/commands/uninstall.ts

🔇 Additional comments (19)

lib/codex-manager/repair-commands.ts (2)

1493-1505: lgtm – quota cache save failure correctly downgraded to partial-success warning.

the try/catch captures EBUSY/EPERM (common on windows) and stores the message in quotaCacheSaveError instead of rejecting after the account-storage commit already landed. the test at test/codex-manager-cli.test.ts:10933-10990 validates that the run completes with exit 0 and surfaces quotaCacheSaveError in the json payload.

---

1519-1519: output paths surface the cache-save error correctly.

json payload includes quotaCacheSaveError at line 1519, and non-json mode prints a styled warning at lines 1556-1563. no sensitive data (tokens/emails) leaks into the error message since it's derived from the filesystem exception.

Also applies to: 1556-1563

lib/runtime/config-toml.ts (2)

138-149: lgtm — splice-before-section correctly mirrors the write path.

the fix at lib/runtime/config-toml.ts:141-147 now matches rewriteTopLevelModelProvider (lines 62-65), ensuring restored model_provider lands in the root table. verified by test/config-toml-restore.test.ts:15-31.

---

180-204: lgtm — residue removal and splice logic are correct.

lib/runtime/config-toml.ts:180-183 drops bind-written disable_response_storage = false when originalLine is null (covered by test/config-toml-restore.test.ts:56-65). the post-loop splice at lines 196-202 handles the edge case where the bind-written line was stripped by downstream tooling, putting the user's original setting back into the root table.

test/config-toml-restore.test.ts (1)

48-104: test coverage for restoreTopLevelResponseStorage is comprehensive.

the suite covers:

• bind-time residue removal when original lacks the key (test/config-toml-restore.test.ts:56-65)
• splice-before-section when current omits the line (test/config-toml-restore.test.ts:67-87)
• in-section settings preserved (test/config-toml-restore.test.ts:96-103)

this addresses the prior audit finding at lib/runtime/config-toml.ts:180-183.

lib/codex-manager/commands/uninstall.ts (3)

64-69: divergent null-handling from install-codex-auth-utils.js.

the removePluginFromList at lib/codex-manager/commands/uninstall.ts:64-69 does not pre-filter with list.filter(Boolean), while the version in scripts/install-codex-auth-utils.js may. this divergence was already flagged in the preuninstall.js review — consolidate to avoid subtle mismatches.

---

207-226: lgtm: launcher import deferred to non-dry-run path.

lib/codex-manager/commands/uninstall.ts:210-219 now only calls loadDefaultLauncher() inside the !dryRun branch, addressing the prior review finding about dry-run requiring the launcher module to be present.

---

26-39: note: duplicate withFileOperationRetry implementation.

lib/codex-manager/commands/uninstall.ts:26-39 duplicates the retry logic from lib/fs-retry.js. coderabbitai already flagged this as tech debt for consolidation in a follow-up. not blocking, but worth tracking.

lib/runtime/app-bind.ts (2)

696-720: lgtm: router liveness check now gates existingState reuse.

the condition at lib/runtime/app-bind.ts:696-703 correctly gates on router?.state === "running" && isProcessAlive(router.pid) before reusing existingState.port, which addresses the prior review finding. the orphan-stop fallback at lines 712-716 is also properly wrapped in .catch() for best-effort cleanup.

---

759-763: lgtm: warning log on orphan router does not leak sensitive data.

the warning at lib/runtime/app-bind.ts:759-762 logs only the PID, which is safe. cleanup continues regardless, matching the stated intent.

scripts/preuninstall.js (1)

48-222: lgtm: cleanup flow correctly computes bunLockState in both dry-run and real modes.

the code at scripts/preuninstall.js:119-170 now reads Codex.json even during dry-run and correctly sets bunLockState to "safe" only when the resulting plugins list is empty or the file is missing. the sequential error isolation and best-effort semantics match the stated intent.

test/preuninstall.test.ts (2)

40-44: lgtm: test now uses production path resolver.

test/preuninstall.test.ts:42-43 calls resolveInstallPaths from the production module, eliminating the drift risk flagged in prior review.

---

268-302: lgtm: concurrency regression test added.

the test at test/preuninstall.test.ts:268-302 exercises two parallel runPreuninstallCleanup invocations and asserts the resulting Codex.json is valid JSON with codex-multi-auth removed. this satisfies the coding guideline requirement for concurrency regression cases.

test/codex-manager-uninstall-dispatch.test.ts (1)

45-70: lgtm: dispatcher wiring tests are deterministic and verify clearAccounts forwarding.

test/codex-manager-uninstall-dispatch.test.ts:46-69 validates the dispatcher always wires clearAccounts and that invoking it calls storage.clearAccounts. clean separation from command-level tests in test/uninstall-command.test.ts.

test/uninstall-ebusy-retry.test.ts (1)

79-151: lgtm: deterministic EBUSY retry regression tests.

test/uninstall-ebusy-retry.test.ts:80-112 and test/uninstall-ebusy-retry.test.ts:114-150 inject one-shot EBUSY errors and verify the retry loop recovers. this satisfies the coding guideline requirement for windows filesystem behavior regression cases.

test/uninstall-command.test.ts (1)

117-466: lgtm: comprehensive coverage for runUninstallCommand.

test/uninstall-command.test.ts:117-466 covers help, unknown options, dry-run behavior, plugin removal, bun.lock state decision table (safe/uncertain), partial failure handling, --clear-accounts gating, and JSON output. the regression tests at lines 161-192 and 194-226 pin the fixes for launcher import deferral and bun.lock dry-run computation.

test/transactions.test.ts (2)

41-71: lgtm — loadCurrentFlagged forwarding correctly validated.

vi.fn() spy at test/transactions.test.ts:46 verifies the dep is invoked exactly once, and toEqual at line 50 confirms the return value is passed as the third argument to the handler — matching lib/storage/transactions.ts:92-94 exactly.

---

73-97: lgtm — default flagged-storage fallback correctly validated.

seen array at test/transactions.test.ts:74 captures the value the handler receives, and toEqual([{ version: 1, accounts: [] }]) at line 96 matches the literal constructed at lib/storage/transactions.ts:95. deterministic with no spy needed here.

lib/runtime/hydrate-emails.ts (1)

87-97: LGTM — index-stable patch correctly resolves the accountId collision.

the accountsCopy[index] ?? account merge is safe: accountsCopy is always built with the same length and order as storage.accounts (via .map), so there are no out-of-bounds risks, and the ?? correctly falls back to the original entry for null slots. saveAccounts is called exactly once under the changed guard. logging at lib/runtime/hydrate-emails.ts:81 emits no token or email data — compliant with the no-leak rule.