refactor: extract best command

[ndycode]

Summary

• extract the codex auth best implementation into a dedicated command module
• add direct unit coverage for help, invalid-option handling, and already-best JSON output while preserving the live best-account behavior

What Changed

• added lib/codex-manager/commands/best.ts for best-account selection, live probe refresh handling, and JSON/human output paths
• updated lib/codex-manager.ts so runBest(...) delegates through the extracted command module with injected dependencies
• added test/codex-manager-best-command.test.ts for command-level coverage

Validation

• npm run test -- test/codex-manager-best-command.test.ts test/codex-manager-cli.test.ts
• npm run lint
• npm run typecheck
• npm run build

Risk and Rollback

• Risk level: medium-low
• Rollback plan: revert 30854b7 to restore the inline best-account implementation

Additional Notes

• this keeps the manager split moving while preserving the existing best-account regression coverage in the CLI suite

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 the codex auth best implementation from the monolithic lib/codex-manager.ts into lib/codex-manager/commands/best.ts, following the same command-module pattern used by check, forecast, and report. the two issues flagged in previous threads (refreshed tokens dropped on early-exit paths and the shouldSyncCurrentBest gate dropping non-best token saves) are both addressed — persistProbeChangesIfNeeded now accepts a beforeSave callback that sets lastUsed in-place and is called unconditionally in the already-best branch, and the changed-flag detection logic is refactored to snapshot-then-diff rather than conditional-assign.

key points:

• delegation: runBest in codex-manager.ts is now a 1-line delegate; all 14 production deps are injected, making the command fully unit-testable without filesystem or network
• injectable clock/logger: getNow and logInfo/logWarn/logError deps mean tests can freeze time and assert on exact log calls without touching console
• resolveActiveIndex O(n) call: called once per account inside the forecastInputs map — should be hoisted to a local before the map (see inline comment)
• test coverage gaps: the switch path with changed = true (refreshed tokens surviving an account switch), the queuedRefresh failure branch (probeErrors propagation), and the loadAccounts → null path are not yet covered — token safety on windows depends on the switch-path gap being exercised

Confidence Score: 4/5

• safe to merge — pure refactor with no behavioral regressions, but test coverage has three gaps worth closing before the next feature lands on top of this module
• the logic port is faithful, the two previously-flagged persistence bugs are fixed, and the delegation wire-up is correct. score held back from 5 by the missing switch-path-with-changed test (token safety risk on windows if persistAndSyncSelectedAccount ever becomes a partial-save), the unexercised refresh-failure branch, and the null-storage gap
• test/codex-manager-best-command.test.ts — needs three additional cases to reach the same coverage bar as other command tests in this suite

Important Files Changed

Filename	Overview
lib/codex-manager/commands/best.ts	new extracted command module — faithful port of the inline logic with injected deps, injectable logger/clock, and fixed persistProbeChangesIfNeeded callback pattern; minor: resolveActiveIndex called O(n) times inside the forecastInputs map
lib/codex-manager.ts	delegation wired correctly — runBest now delegates to runBestCommand with all required deps, BestCliOptions interface moved to the new module and re-exported; no logic changes
test/codex-manager-best-command.test.ts	good coverage of help, invalid-option, already-best, early-exit persistence, and no-op save paths; missing: switch path with changed = true, queuedRefresh failure branch (probeErrors propagation), and loadAccounts returning null

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[runBestCommand] --> B{--help/-h?}
    B -- yes --> C[printBestUsage, return 0]
    B -- no --> D{parseBestArgs ok?}
    D -- no --> E[logError + printBestUsage, return 1]
    D -- yes --> F{modelProvided && !live?}
    F -- yes --> G[logError + printBestUsage, return 1]
    F -- no --> H[loadAccounts]
    H --> I{storage empty?}
    I -- yes --> J[output error, return 1]
    I -- no --> K[Live probe loop per account]
    K --> L{hasUsableAccessToken?}
    L -- no --> M[queuedRefresh]
    M -- failure --> N[store in refreshFailures, continue]
    M -- success --> O[update tokens, set changed=true if delta]
    O --> P[fetchCodexQuotaSnapshot]
    L -- yes --> P
    P --> Q[evaluateForecastAccounts + recommendForecastAccount]
    Q --> R{recommendedIndex null?}
    R -- yes --> S[persistProbeChangesIfNeeded, output error, return 1]
    R -- no --> T{bestAccount exists?}
    T -- no --> U[persistProbeChangesIfNeeded, output error, return 1]
    T -- yes --> V{currentIndex === bestIndex?}
    V -- yes --> W[persistProbeChangesIfNeeded with lastUsed=now]
    W --> X{shouldSyncCurrentBest?}
    X -- yes --> Y[setCodexCliActiveSelection]
    X -- no --> Z[output already-best, return 0]
    Y --> Z
    V -- no --> AA[persistAndSyncSelectedAccount]
    AA --> AB[output switched, return 0]

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: lib/codex-manager/commands/best.ts
Line: 546-553

Comment:
**`resolveActiveIndex` called O(n) times inside `map`**

`deps.resolveActiveIndex(storage, "codex")` is invoked once per account on line 549. if this dep ever has side effects (e.g. logs, reads from disk) or is replaced by a costlier impl in a future extraction step, it will fire n times silently. hoist it before the map.

```suggestion
	const activeIndex = deps.resolveActiveIndex(storage, "codex");
	const forecastInputs = storage.accounts.map((account, index) => ({
		index,
		account,
		isCurrent: index === activeIndex,
		now,
		refreshFailure: refreshFailures.get(index),
		liveQuota: liveQuotaByIndex.get(index),
	}));
```

`resolveActiveIndex` is also called again independently at line 595 — that second call should then use the hoisted `activeIndex` too.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: test/codex-manager-best-command.test.ts
Line: 1034-1060

Comment:
**missing vitest coverage: switch path with `changed = true`**

the "switches to recommended account" test runs with `hasUsableAccessToken` defaulting to `true`, so `changed` stays `false` and the scenario where tokens were refreshed during the live probe before the account switches is never exercised. if `persistAndSyncSelectedAccount` ever does a partial save instead of a full storage flush, those refreshed tokens are silently dropped on this path — a real token safety risk on windows where stale tokens can trigger extra oauth round-trips and hit rate-limit budget.

add a variant that sets `hasUsableAccessToken` to return `false` for one non-best account, ensures `queuedRefresh` returns fresh tokens, and asserts those tokens appear inside the storage object passed to `persistAndSyncSelectedAccount`.

also missing vitest coverage:
- `queuedRefresh` returning a non-success result (the `refreshFailures` map path) — no test verifies the error message lands in the json `probeErrors` output
- `loadAccounts` returning `null` (only the empty-accounts branch is covered)

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: "fix: preserve best c..."}

[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]

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Review skipped — only excluded labels are configured. (1)

• skip-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: c3caec7c-5dff-4bd7-a7f1-e160d3412e74

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/pr1-best-command

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch refactor/pr1-best-command

---

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]

Closing because this work is now included in main via #318 and #319.