Skip to content

refactor: route list and status through command module#155

Closed
ndycode wants to merge 1 commit intorefactor/pr1-list-status-cli-testsfrom
refactor/pr1-remove-inline-status-wrapper
Closed

refactor: route list and status through command module#155
ndycode wants to merge 1 commit intorefactor/pr1-list-status-cli-testsfrom
refactor/pr1-remove-inline-status-wrapper

Conversation

@ndycode
Copy link
Owner

@ndycode ndycode commented Mar 20, 2026

Summary

  • remove the remaining inline showAccountStatus() wrapper from lib/codex-manager.ts
  • route codex auth list and codex auth status directly through the extracted status command module

What Changed

  • deleted the now-redundant inline showAccountStatus() helper
  • updated the list / status dispatch path to return runStatusCommand(...) directly
  • keeps the new CLI coverage from #154 exercising both passive commands through the extracted module

Validation

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

Risk and Rollback

  • Risk level: low
  • Rollback plan: revert 54a5068 to restore the inline wrapper

Additional Notes

  • this is a follow-on cleanup stacked on the command extraction train 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 is a clean, low-risk cleanup that eliminates the one-liner showAccountStatus() wrapper from lib/codex-manager.ts and routes codex auth list / codex auth status directly through runStatusCommand(...).

  • removes the intermediate async function showAccountStatus() wrapper (9 lines) that served no purpose beyond indirection
  • the dispatch block now returns runStatusCommand({...}) directly, which is strictly better than the old await + return 0 because it propagates any future non-zero exit codes from the module
  • deps injected (setStoragePath, loadAccounts, resolveActiveIndex, formatRateLimitEntry) are identical to what the wrapper was passing — no behavioral change
  • optional getNow and logInfo deps still default to Date.now() / console.log, same as before
  • existing integration coverage in codex-manager-cli.test.ts (lines 601, 616) exercises both list and status through the new path; codex-manager-status-command.test.ts covers the module in isolation

Confidence Score: 5/5

  • safe to merge — pure dead-code removal with no behavioral change and existing integration coverage
  • the diff is a 9-line deletion plus a 4-line substitution. runStatusCommand always returns 0 today so the old await + return 0 and the new return runStatusCommand(...) are equivalent. no filesystem writes, no token handling, no concurrency surface introduced. both cli integration tests pass through the new dispatch path.
  • no files require special attention

Important Files Changed

Filename Overview
lib/codex-manager.ts removes the showAccountStatus() inline wrapper and inlines the runStatusCommand call directly into the dispatch block — functionally equivalent, no behavioral regression, propagation of future non-zero exit codes is now correct

Sequence Diagram

sequenceDiagram
    participant CLI as runCodexMultiAuthCli
    participant OLD as showAccountStatus() (deleted)
    participant CMD as runStatusCommand (commands/status.ts)
    participant STORE as loadAccounts / storage

    Note over CLI,OLD: before this PR
    CLI->>OLD: await showAccountStatus()
    OLD->>CMD: runStatusCommand({deps})
    CMD->>STORE: setStoragePath(null) + loadAccounts()
    STORE-->>CMD: AccountStorageV3 | null
    CMD-->>OLD: return 0
    OLD-->>CLI: void
    CLI-->>CLI: return 0 (hardcoded)

    Note over CLI,CMD: after this PR
    CLI->>CMD: return runStatusCommand({deps})
    CMD->>STORE: setStoragePath(null) + loadAccounts()
    STORE-->>CMD: AccountStorageV3 | null
    CMD-->>CLI: return 0 (propagated)
Loading

Fix All in Codex

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

Comment:
**missing vitest coverage for `loadAccounts` rejection path**

`runStatusCommand` does no try/catch — if `loadAccounts()` rejects (e.g. storage i/o error, windows EBUSY on the accounts json), the rejection bubbles straight out of `runCodexMultiAuthCli`. neither `codex-manager-cli.test.ts` nor `codex-manager-status-command.test.ts` has a case for this. wasn't introduced by this pr, but the new direct dispatch makes it slightly more visible. worth adding a test along the lines of:

```ts
it("returns 1 when loadAccounts rejects", async () => {
    loadAccountsMock.mockRejectedValueOnce(new Error("EBUSY: file locked"));
    const { runCodexMultiAuthCli } = await import("../lib/codex-manager.js");
    await expect(runCodexMultiAuthCli(["auth", "list"])).rejects.toThrow("EBUSY");
});
```

or, if the intent is to swallow storage errors and return a non-zero code, add try/catch inside `runStatusCommand`.

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

Last reviewed commit: "refactor: route list..."

@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: 01e0ca2e-ca40-401d-a440-deef494395e1

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

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.

@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.

@ndycode ndycode added the passed label Mar 20, 2026
@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