refactor(cli): fold internal-dispatch and pin help banner - #1456
Conversation
📝 WalkthroughWalkthroughThe CLI removes the internal dispatcher, inlines tray and startup-health runners, adds help entries for ChangesCLI dispatch and help alignment
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
✅ Deterministic PR hygiene checks passed. |
dfc99e5 to
68a610b
Compare
Phase 5 of the CLI deepening: - fold src/cli/internal-dispatch.ts into src/cli/dispatch.ts as three explicit runner entries (__tray-start, __tray-restart, __startup-health) that use the injected deps directly; delete the module and its dedicated test - add banner-coverage test to cli-registry.test.ts and fix the drift it caught: printUsage now lists the previously-missing visible commands (route, logs, api-key) Behavior preserved: typecheck green; 144 pass / 4 known pre-existing environmental failures; smoke tests (version 0, help nosuch 1, ready invalid 64, --help header+new lines) pass
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/dispatch.ts`:
- Around line 319-321: Update the "__tray-start" command handler to use the
boolean result from deps.handleTrayProxyStart() when determining the command
status, returning a nonzero status when it returns false and preserving success
when startup succeeds.
In `@tests/cli-registry.test.ts`:
- Around line 121-123: Update the coverage check in the test’s help-source
matching logic to recognize a command only when `ocx <canonical-name>` appears
at the start of a banner line, while retaining the full `entry.usage` match.
Escape `entry.name` before constructing the regular expression so command names
cannot alter the pattern.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8a1c1759-6eba-4bdd-8863-d3ef1177873c
📒 Files selected for processing (5)
src/cli/dispatch.tssrc/cli/help.tssrc/cli/internal-dispatch.tstests/cli-registry.test.tstests/internal-cli-dispatch.test.ts
💤 Files with no reviewable changes (2)
- src/cli/internal-dispatch.ts
- tests/internal-cli-dispatch.test.ts
Addresses two CodeRabbit findings on #1456: - __tray-start now returns 1 when handleTrayProxyStart() returns false (could not make the proxy live) instead of always reporting success - banner-coverage test now matches 'ocx <canonical-name>' at the start of a banner line (escaped) rather than any substring; adds the previously uncovered usage/storage/memory banner lines
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/cli-registry.test.ts (1)
81-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a focused regression test for the
__tray-startexit status.This new block checks registry parity only. It does not execute
commandRunners["__tray-start"]. The changed runner insrc/cli/dispatch.tsLine 320 mapsfalsefromdeps.handleTrayProxyStart()to status1. Add a Bun test that asserts the failure path returns1and the success path returns0. This protects the corrected exit-code contract after the dedicated internal-dispatch test was removed.As per path instructions, a behavior change in
src/should have a focused regression test near the existing tests for that subsystem.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cli-registry.test.ts` around lines 81 - 95, Add a focused Bun regression test near the existing CLI dispatch tests that invokes commandRunners["__tray-start"] with mocked deps.handleTrayProxyStart results. Assert the runner returns status 1 when the handler returns false and status 0 when it returns true, preserving the corrected exit-code contract.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/cli-registry.test.ts`:
- Around line 81-95: Add a focused Bun regression test near the existing CLI
dispatch tests that invokes commandRunners["__tray-start"] with mocked
deps.handleTrayProxyStart results. Assert the runner returns status 1 when the
handler returns false and status 0 when it returns true, preserving the
corrected exit-code contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5a84942d-bbba-4421-81c2-384cd07c6e22
📒 Files selected for processing (3)
src/cli/dispatch.tssrc/cli/help.tstests/cli-registry.test.ts
Summary
Fold the internal CLI dispatcher and pin the help banner to the registry.
internal-dispatch.ts: the three internal runners (__tray-start,__tray-restart,__startup-health) are now explicit entries insrc/cli/dispatch.tsthat use the injected deps directly. Deletessrc/cli/internal-dispatch.tsand its dedicated test.tests/cli-registry.test.tsasserting every visible registry command appears in theprintUsagebanner, and fixes the drift it caught —route,logs, andapi-keywere visible commands missing from the banner and are now listed.Behavior is unchanged except the banner now lists those three previously-omitted commands.
Verification
bun run typecheck— exit 0.cli-restore-back(2) andPOST /api/syncownership (2) environmental cases.ocx --version— exit 0.ocx help nosuch— exit 1,Unknown command.ocx ready --timeout 5— exit 64.ocx --help— containsopencodex (ocx),ocx route,ocx logs,ocx api-key, exit 0.internal-dispatchinsrc/ortests/.No GUI changes; no screenshot required.
Checklist
Review notes (stacked PR): stacks on #1455 (
codex/cli-exit). Does not targetdev. Diff is only the internal-dispatch fold + banner pin (300c9cda..e897d990): 5 files, +61/−72. Merge only after #1444, #1446, #1451, #1455 land.Summary by CodeRabbit
New Features
route,logs,usage,storage,memory, andapi-keycommands, including aliases and descriptions.Bug Fixes