Skip to content

refactor: extract status and features handlers#151

Closed
ndycode wants to merge 2 commits intorefactor/pr1-manager-command-splitfrom
refactor/pr1-status-and-features
Closed

refactor: extract status and features handlers#151
ndycode wants to merge 2 commits intorefactor/pr1-manager-command-splitfrom
refactor/pr1-status-and-features

Conversation

@ndycode
Copy link
Owner

@ndycode ndycode commented Mar 20, 2026

Summary

  • extract the codex auth list/status and codex auth features handlers into a dedicated command module
  • add focused unit tests for the extracted status and features flows while preserving the current CLI output contract

What Changed

  • added lib/codex-manager/commands/status.ts for account status output and implemented-features output
  • updated lib/codex-manager.ts so the status and features paths delegate through the new command module
  • added test/codex-manager-status-command.test.ts for empty-pool, account-marker, and feature-list coverage

Validation

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

Risk and Rollback

  • Risk level: low
  • Rollback plan: revert 0abd05a to restore the inline status/features handlers

Additional Notes

  • this PR is stacked on #150 to keep the manager split reviewable in one-worktree slices

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 codex auth list/status and codex auth features from the monolithic codex-manager.ts into a dedicated lib/codex-manager/commands/status.ts command module with injected dependencies, continuing the manager command-split refactor from pr #150.

the extraction is architecturally sound — getStoragePath, loadAccounts, resolveActiveIndex, and formatRateLimitEntry are all correctly injected through StatusCommandDeps, and the delegation callsite in codex-manager.ts is clean. two gaps need attention before this is fully solid:

  • formatCooldown is imported directly rather than injected like the other account-level helpers. the cooldown marker branch in runStatusCommand is unreachable from unit tests, inconsistent with how formatRateLimitEntry is treated, and has no test coverage.
  • the accounts.length === 0 branch is untested — the existing empty-pool test only covers loadAccounts returning null, leaving the zero-entry array path dark.

Confidence Score: 4/5

  • safe to merge — no runtime regressions, but two test coverage gaps should be closed in a follow-up
  • the production delegation path is correct and the DI wiring in codex-manager.ts is clean. the only issues are that formatCooldown is not injected (leaving one marker branch untestable) and the empty-array path has no test case — neither causes a runtime error, but both are coverage gaps that matter at this project's 80% threshold
  • lib/codex-manager/commands/status.ts (formatCooldown injection) and test/codex-manager-status-command.test.ts (missing branches)

Important Files Changed

Filename Overview
lib/codex-manager/commands/status.ts new module correctly injects most deps (getStoragePath, formatRateLimitEntry, resolveActiveIndex) but hardwires formatCooldown as a direct import — cooldown marker branch is untestable from unit tests and has zero coverage
lib/codex-manager.ts clean delegation — showAccountStatus and runFeaturesReport now route through the new command module with all required deps injected; formatCooldown removal from this file is fine since it moved into status.ts
test/codex-manager-status-command.test.ts good baseline coverage for null-storage and happy-path account rows, but the accounts.length === 0 empty-array branch and the cooldown marker branch both lack test cases

Sequence Diagram

sequenceDiagram
    participant CLI as codex-manager.ts
    participant SC as status.ts<br/>runStatusCommand
    participant ST as storage.ts<br/>loadAccounts / getStoragePath
    participant AC as accounts.ts<br/>formatRateLimitEntry / formatCooldown

    CLI->>SC: runStatusCommand({ setStoragePath, getStoragePath, loadAccounts, ... })
    SC->>ST: deps.setStoragePath(null)
    SC->>ST: deps.loadAccounts()
    ST-->>SC: AccountStorageV3 | null
    SC->>ST: deps.getStoragePath()
    ST-->>SC: string | null

    alt no accounts
        SC-->>CLI: 0 (prints empty state)
    else accounts present
        loop each account
            SC->>SC: deps.formatRateLimitEntry(account, now, "codex")
            SC->>AC: formatCooldown(account, now)  [hardwired import — not injected]
            AC-->>SC: string | null
        end
        SC-->>CLI: 0 (prints account rows)
    end
Loading

Fix All in Codex

Prompt To Fix All With AI
This is a comment left during a code review.
Path: lib/codex-manager/commands/status.ts
Line: 56-57

Comment:
**`formatCooldown` hard-wired import breaks test isolation**

`formatCooldown` is called directly from the module import rather than being injected through `StatusCommandDeps`. this is inconsistent with `formatRateLimitEntry`, which IS injected. the practical consequence is:

1. the `cooldown:${cooldown}` marker branch is completely untested — there's no test case that exercises it, and there's no way to force `formatCooldown` to return a truthy value from a unit test without reaching into the real account data shape.
2. any future change to `formatCooldown` (e.g. cooldown reason format) will silently affect the status output with no test coverage to catch a regression.

add `formatCooldown` to `StatusCommandDeps` and inject it the same way as `formatRateLimitEntry`:

```suggestion
		const rateLimit = deps.formatRateLimitEntry(account, now, "codex");
		if (rateLimit) markers.push("rate-limited");
		const cooldown = deps.formatCooldown(account, now);
		if (cooldown) markers.push(`cooldown:${cooldown}`);
```

then add a `formatCooldown` field to the interface and pass `formatCooldown` from `codex-manager.ts`'s callsite, matching the pattern already used for `formatRateLimitEntry`.

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-status-command.test.ts
Line: 50-60

Comment:
**empty `accounts` array branch untested**

the empty-pool test only covers `loadAccounts` returning `null`. the guard in `runStatusCommand` is `!storage || storage.accounts.length === 0`, so the `accounts.length === 0` branch is a distinct code path that has no coverage at all.

add a second empty-pool case:

```typescript
it("prints empty storage state for empty accounts array", async () => {
    const deps = createStatusDeps({
        loadAccounts: vi.fn(async () => ({
            version: 3,
            activeIndex: 0,
            activeIndexByFamily: {},
            accounts: [],
        })),
    });

    const result = await runStatusCommand(deps);

    expect(result).toBe(0);
    expect(deps.logInfo).toHaveBeenCalledWith("No accounts configured.");
    expect(deps.logInfo).toHaveBeenCalledWith("Storage: /tmp/codex.json");
});
```

this matters because an account file that exists on disk but contains zero entries will exercise this branch in production, not the `null` branch.

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

Last reviewed commit: "fix(status): inject ..."

@chatgpt-codex-connector
Copy link

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
Copy link
Contributor

coderabbitai bot commented Mar 20, 2026

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: 8f3a0580-c38e-4b16-ae9a-a0440ad2a57b

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-status-and-features
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch refactor/pr1-status-and-features

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

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

@ndycode
Copy link
Owner Author

ndycode commented Mar 23, 2026

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

@ndycode ndycode closed this Mar 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant