Skip to content

test: cover list and status cli output#154

Closed
ndycode wants to merge 4 commits intorefactor/pr1-report-commandfrom
refactor/pr1-list-status-cli-tests
Closed

test: cover list and status cli output#154
ndycode wants to merge 4 commits intorefactor/pr1-report-commandfrom
refactor/pr1-list-status-cli-tests

Conversation

@ndycode
Copy link
Owner

@ndycode ndycode commented Mar 20, 2026

Summary

  • add black-box CLI coverage for codex auth list and codex auth status
  • lock the passive account-status output path before further manager extraction continues

What Changed

  • extended test/codex-manager-cli.test.ts with coverage for:
    • empty storage output via codex auth list
    • populated account output via codex auth status
  • the new tests assert the storage-path line, current/disabled markers, and that the status path still resets storage path resolution before reading accounts

Validation

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

Risk and Rollback

  • Risk level: low
  • Rollback plan: revert 5862826 to remove the new list/status regression coverage

Additional Notes

  • this keeps the command-split train reviewer-friendly by closing the remaining passive-command coverage gap identified in the guardrail audit

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 adds black-box vitest coverage for codex auth list (empty storage path) and codex auth status (populated storage path), and fixes the formatAccountLabel mock format to align with the real implementation. all three issues from the previous review round are resolved: the rateLimitResetTimes field name is now correct, both new tests assert setStoragePath(null) and the Storage: log line, and the doubled-index label bug is gone.

  • formatAccountLabel mock updated from `${index + 1}. ${email}` to `Account ${index + 1} (${email})` — matches the real implementation's email-only branch
  • new test: auth list with null storage asserts "No accounts configured.", Storage: path, and setStoragePath(null) call
  • new test: auth status with two accounts asserts Accounts (2), Storage: path, [current] on account 1, and [disabled, rate-limited] on account 2
  • remaining formatting-only diff (lines 98–130) is pure line-length reformatting with no behavioral change
  • account 1's coolingDownUntil fixture field is silently suppressed by the global formatCooldown mock, leaving the cooldown marker branch uncovered — minor coverage gap worth addressing

Confidence Score: 4/5

  • safe to merge — test-only change, no production code touched, no windows filesystem or token safety exposure
  • all three previous review issues are resolved, the new assertions are logically correct, module isolation via vi.restoreAllMocks() in afterEach is in place, and the 60-second rate-limit buffer prevents any timing flakiness. one minor coverage gap remains (cooldown branch) but it doesn't break anything
  • no files require special attention beyond the minor cooldown fixture issue flagged at line 630

Important Files Changed

Filename Overview
test/codex-manager-cli.test.ts adds two black-box CLI tests for auth list (empty storage) and auth status (populated storage); the three issues flagged in the previous review round are all resolved (rateLimitResetTimes field name, setStoragePath(null) assertion, doubled-index label format), but account 1's coolingDownUntil fixture field is silently suppressed by the global formatCooldown mock, leaving the cooldown marker branch uncovered.

Sequence Diagram

sequenceDiagram
    participant Test
    participant CLI as runCodexMultiAuthCli
    participant Status as runStatusCommand
    participant Storage as loadAccounts / getStoragePath

    Note over Test: "auth list" — empty path
    Test->>CLI: runCodexMultiAuthCli(["auth", "list"])
    CLI->>Status: runStatusCommand(deps)
    Status->>Status: deps.setStoragePath(null)
    Status->>Storage: loadAccounts() → null
    Status->>Storage: getStoragePath() → "/mock/..."
    Status-->>Test: log "No accounts configured." + "Storage: ..."
    Test->>Test: assert exitCode=0, setStoragePath(null), log lines ✓

    Note over Test: "auth status" — populated path
    Test->>CLI: runCodexMultiAuthCli(["auth", "status"])
    CLI->>Status: runStatusCommand(deps)
    Status->>Status: deps.setStoragePath(null)
    Status->>Storage: loadAccounts() → {accounts: [active, disabled]}
    Status->>Storage: getStoragePath() → "/mock/..."
    Status->>Status: resolveActiveIndex → 0
    Status->>Status: formatAccountLabel(account, 0) → "Account 1 (active@example.com)"
    Status->>Status: formatRateLimitEntry(account1) → null
    Status->>Status: formatCooldown(account1) → null (mocked)
    Status-->>Test: log "1. Account 1 (...) [current] used 1s ago"
    Status->>Status: formatAccountLabel(account, 1) → "Account 2 (disabled@example.com)"
    Status->>Status: formatRateLimitEntry(account2, codex) → "resets in 1m 0s"
    Status-->>Test: log "2. Account 2 (...) [disabled, rate-limited] used 1s ago"
    Test->>Test: assert all log lines ✓
Loading

Fix All in Codex

Prompt To Fix All With AI
This is a comment left during a code review.
Path: test/codex-manager-cli.test.ts
Line: 630-631

Comment:
**Misleading fixture — cooldown branch never exercised**

account 1 carries `coolingDownUntil: now + 60_000`, which reads as if it exercises the cooldown marker path. but the global `formatCooldown` mock at line 89 always returns `null`, so `markers.push(...)` for the cooldown branch in `status.ts:56` is never reached. the field is silently swallowed and the cooldown code path stays uncovered.

either drop the field from the fixture to make the intent obvious, or add a scoped per-test mock that returns a real value and assert `[current, cooldown:...]` in the expected string. as-is the fixture data is misleading for future readers.

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

Last reviewed commit: "align auth status te..."

@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: eee8ecce-6f62-45b4-a3c9-50a313358c36

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

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 21, 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