refactor: extract check command wrapper#152
refactor: extract check command wrapper#152ndycode wants to merge 2 commits intorefactor/pr1-status-and-featuresfrom
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughthe pull request extracts the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Key observations
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/codex-manager-check-command.test.ts`:
- Around line 8-17: Add a failing-path unit test for runCheckCommand that
asserts it rejects when the injected runHealthCheck throws: create a new test in
the same file that supplies deps: CheckCommandDeps with runHealthCheck:
vi.fn(async () => { throw new Error("boom") }) (or vi.fn(() =>
Promise.reject(new Error("boom")))), call await
expect(runCheckCommand(deps)).rejects.toThrow("boom"), and also assert
runHealthCheck was called with { liveProbe: true }; reference the
runCheckCommand function and the runHealthCheck mock when adding the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8174b345-01b9-4479-b192-01440d5cfca1
📒 Files selected for processing (3)
lib/codex-manager.tslib/codex-manager/commands/check.tstest/codex-manager-check-command.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/codex-manager/commands/check.tslib/codex-manager.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/codex-manager-check-command.test.ts
🔇 Additional comments (2)
lib/codex-manager.ts (1)
39-39: good extraction with behavior parity.this keeps dispatch clean in
lib/codex-manager.ts:5550-5552while preserving the sameauth checkbehavior through the wrapper import atlib/codex-manager.ts:39. coverage is present intest/codex-manager-check-command.test.ts:8-17. no new windows filesystem or concurrency risk is introduced in this segment.as per coding guidelines,
lib/**: "verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios."Also applies to: 5550-5552
lib/codex-manager/commands/check.ts (1)
1-8: wrapper implementation is clean and deterministic.the command wrapper in
lib/codex-manager/commands/check.ts:1-8correctly delegates torunHealthCheck({ liveProbe: true })and returns exit code0. coverage exists intest/codex-manager-check-command.test.ts:8-17. no windows filesystem or concurrency risk is added here.as per coding guidelines,
lib/**: "verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios."
All review threads are resolved and follow-up commits addressed this stale automated change request.
Summary
codex auth checkentry point into a dedicated command moduleWhat Changed
lib/codex-manager/commands/check.tsas a dedicatedcheckcommand wrapperlib/codex-manager.tsso thecheckdispatch delegates to the new command moduletest/codex-manager-check-command.test.tsto verify the command always invokesrunHealthCheck({ liveProbe: true })Validation
npm run lintnpm run typechecknpm run test -- test/codex-manager-check-command.test.ts test/codex-manager-status-command.test.ts test/codex-manager-switch-command.test.ts test/codex-manager-cli.test.tsnpm run buildRisk and Rollback
c37bebato restore the inlinecheckdispatchAdditional Notes
#151so the command split remains easy to review in one-worktree slicesnote: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
extracts the
codex auth checkdispatch intolib/codex-manager/commands/check.tsusing the same dependency-injection pattern established by the siblingstatusandswitchcommand modules already landed in #151.runCheckCommandis an 8-line wrapper that injectsrunHealthCheckas a dep — clean, testable, no behavior changecodex-manager.tsdispatch is now a single-line delegation:return runCheckCommand({ runHealthCheck })0) and the rejection path (propagates error), consistent with sibling test fileslib/AGENTS.mdis not updated to document thecommands/subdirectory; ai agents using the knowledge base will misscheck.ts,status.ts, andswitch.ts— worth a follow-up updaterunHealthCheckis a single sequential call with no shared mutable stateConfidence Score: 5/5
Important Files Changed
await runHealthCheck(...)replaced byreturn runCheckCommand({ runHealthCheck })— semantically identical, no behavior deltaSequence Diagram
sequenceDiagram participant CLI as runCodexMultiAuthCli participant CMD as runCheckCommand participant HC as runHealthCheck CLI->>CMD: runCheckCommand({ runHealthCheck }) CMD->>HC: runHealthCheck({ liveProbe: true }) HC-->>CMD: resolves (void) CMD-->>CLI: return 0Prompt To Fix All With AI
Last reviewed commit: "test(check): cover w..."
Context used: