refactor: extract switch command handler

[ndycode]

Summary

• extract the codex auth switch implementation into a dedicated command module
• add direct unit coverage for the extracted switch command while keeping the existing CLI behavior unchanged

What Changed

• added lib/codex-manager/commands/switch.ts with the switch command flow and injected dependencies for storage, sync, and output
• trimmed lib/codex-manager.ts so the switch path delegates to the new command module instead of keeping the command body inline
• added test/codex-manager-switch-command.test.ts for missing-index, range-validation, and sync-warning behavior

Validation

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

Docs and Governance Checklist

• README updated (if user-visible behavior changed)
• docs/getting-started.md updated (if onboarding flow changed)
• docs/features.md updated (if capability surface changed)
• relevant docs/reference/* pages updated (if commands/settings/paths changed)
• docs/upgrade.md updated (if migration behavior changed)
• SECURITY.md and CONTRIBUTING.md reviewed for alignment

Risk and Rollback

• Risk level: low
• Rollback plan: revert 585ee57 to restore the inline switch handler in lib/codex-manager.ts

Additional Notes

• this PR is intentionally stacked on #149 so the manager split stays reviewable in small slices from the same worktree

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 switch inline handler into lib/codex-manager/commands/switch.ts with injectable deps for storage, sync, and output — a clean, focused slice of the ongoing codex-manager split. the bulk of the lib/codex-manager.ts diff is just prettier reformatting; the functional change is the eight-line runSwitch delegation. the new unit test covers the three paths called out in the PR description.

two issues worth resolving before merge:

• log deps not wired at the call site — runSwitch() passes only the three mandatory deps and omits logError, logWarn, and logInfo. all switch output therefore falls back to raw console.* in production, which matches the old inline behaviour but leaves the injectable-logger mechanism unused and the call site untestable via the injected log mocks.
• missing vitest coverage — the null-storage path, non-numeric/out-of-bounds index formats, and the clean nominal success case (synced: true, wasDisabled: false) are not covered; given the project's 80% branch threshold this is worth closing before the stacked PRs accumulate more untested branches.
• concurrency note — the loadAccounts() → persistAndSyncSelectedAccount() sequence is not atomic; two concurrent codex auth switch calls can race and the last writer wins. this is pre-existing and not introduced here, but the extraction makes it easier to address in a follow-up with withAccountStorageTransaction.

Confidence Score: 4/5

• safe to merge; no behavioural regressions, two low-severity gaps to close
• the refactor is a pure extraction — identical logic, no new failure modes, no token/filesystem changes. the missing log wiring is a style gap (not a bug), and the test coverage holes don't affect production correctness. concurrency risk is pre-existing.
• test/codex-manager-switch-command.test.ts needs additional cases; lib/codex-manager.ts call site should wire up log deps

Important Files Changed

Filename	Overview
lib/codex-manager/commands/switch.ts	new module with clean injectable-deps design; switchReason is hardcoded to "rotation" (inherited from the inline original) and all log deps default to raw console.* since the call site doesn't wire them up
lib/codex-manager.ts	bulk of the diff is prettier reformatting; the meaningful change is the runSwitch delegation, but logError/logWarn/logInfo deps are not wired up, leaving the injectable logging unused in production
test/codex-manager-switch-command.test.ts	covers missing-index, out-of-range, and a partial success path, but misses null-storage, invalid-format index, and the clean nominal success case (synced=true, wasDisabled=false)

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[runSwitchCommand] --> B[setStoragePath null]
    B --> C{args index present?}
    C -- no --> D[logError: Missing index]
    D --> E[return 1]
    C -- yes --> F[parseInt indexArg]
    F --> G{isFinite AND ge 1?}
    G -- no --> H[logError: Invalid index]
    H --> E
    G -- yes --> I[targetIndex = parsed - 1]
    I --> J[loadAccounts]
    J --> K{storage non-empty?}
    K -- no --> L[logError: No accounts]
    L --> E
    K -- yes --> M{targetIndex in range?}
    M -- no --> N[logError: Out of range]
    N --> E
    M -- yes --> O{account exists?}
    O -- no --> P[logError: Not found]
    P --> E
    O -- yes --> Q[persistAndSyncSelectedAccount]
    Q --> R{synced?}
    R -- no --> S[logWarn: sync incomplete]
    S --> T[logInfo: Switched to account N]
    R -- yes --> T
    T --> U[return 0]

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: lib/codex-manager.ts
Line: 5088-5094

Comment:
**log deps not wired at call site**

`runSwitch()` passes only the three required deps and omits `logError`, `logWarn`, and `logInfo`. that means all switch-command output (errors, the sync-incomplete warning, the success line) falls back to raw `console.error/warn/log` in production, bypassing the module-level `log` object created from `createLogger("codex-manager")` on line 109.

the original inline code also used `console.*`, so this is not a regression — but the whole point of the injectable `log*` deps is to allow callers to redirect output (and to allow future testing of the call site). leaving them unwired makes the `SwitchCommandDeps.logError/Warn/Info` fields dead in production.

consider wiring `log.warn` and `console.log` (or a wrapper) at the call site:

```typescript
async function runSwitch(args: string[]): Promise<number> {
	return runSwitchCommand(args, {
		setStoragePath,
		loadAccounts,
		persistAndSyncSelectedAccount,
		logError: (msg) => log.warn(msg),
		logWarn: (msg) => log.warn(msg),
		logInfo: (msg) => console.log(msg),
	});
}
```

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-switch-command.test.ts
Line: 47-94

Comment:
**missing vitest coverage for several branches**

three paths in `runSwitchCommand` are not exercised by the new test file, leaving coverage gaps against the project's 80% threshold:

1. **null storage** — `loadAccounts()` returning `null` hits the `"No accounts configured."` branch (line 43-44 of `switch.ts`). the current test always returns a two-account storage, so this branch is never hit.

2. **invalid index formats** — `"0"`, `"-1"`, `"1.5"`, and `"abc"` all hit the `"Invalid index"` guard (line 36-39). only the out-of-range path is covered; the `!Number.isFinite(parsed) || parsed < 1` branch (NaN / float / zero) is untested.

3. **nominal success path** — the only passing test overrides `persistAndSyncSelectedAccount` to return `{ synced: false, wasDisabled: true }`, which exercises both `logWarn` and the `(re-enabled)` suffix. the most common production path — `synced: true, wasDisabled: false` — is never verified, so we can't confirm `logWarn` is *not* called and the info line has no `(re-enabled)` trailer.

example additions:

```typescript
it("returns an error when storage is null", async () => {
    const deps = createDeps({ loadAccounts: vi.fn(async () => null) });
    const result = await runSwitchCommand(["1"], deps);
    expect(result).toBe(1);
    expect(deps.logError).toHaveBeenCalledWith("No accounts configured.");
});

it("returns an error for a non-numeric index", async () => {
    const deps = createDeps();
    const result = await runSwitchCommand(["abc"], deps);
    expect(result).toBe(1);
    expect(deps.logError).toHaveBeenCalledWith("Invalid index: abc");
});

it("succeeds cleanly when sync completes and account was enabled", async () => {
    const deps = createDeps();
    const result = await runSwitchCommand(["1"], deps);
    expect(result).toBe(0);
    expect(deps.logWarn).not.toHaveBeenCalled();
    expect(deps.logInfo).toHaveBeenCalledWith(
        "Switched to account 1: Account 1 (one@example.com)",
    );
});
```

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

_{Last reviewed commit: "refactor: extract sw..."}

[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: 5221e089-34d6-43df-8436-177662b317f0

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-manager-command-split

✨ Simplify code

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

---

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.