refactor(cli): registry-driven command dispatch module - #1451
Conversation
📝 WalkthroughWalkthroughThe CLI command switch moved from ChangesCLI dispatch centralization
Estimated code review effort: 4 (Complex) | ~60 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 |
github-actions
Bot
added
the
chore
|
✅ Deterministic PR hygiene checks passed. |
0626634 to
487d644
Compare
0bb19e1 to
ee7fdbe
Compare
487d644 to
3562810
Compare
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
I am deferring the content review because the stack no longer has reviewable ancestry after the root rewrite.
Current state:
- #1446 now has head
35628105on currentdev@87e3ff9f. - #1451's head
cb952c07is 6 commits ahead and 8 behind that new base; GitHub reportsmergeable: false,mergeable_state: dirty, andrebaseable: false. - #1455, #1456, and #1457 are stacked on the old #1451 lineage. Their previously green jobs therefore do not validate the stack that would actually land after the #1446 rewrite.
- The new exact-head #1446 run is currently red in Linux shard 3 and macOS (with shard 4 still running at the time of review). #1457's old run also ended red because the aggregate test job was cancelled after shard 3 hit the 15-minute bound. Those failures should not be “fixed” on stale child heads.
Please rebuild the stack sequentially: rebase #1451 onto the current #1446 head, then #1455 onto the new #1451 head, #1456 onto the new #1455 head, and #1457 onto the new #1456 head. Resolve the root exact-head CI failure first, then rerun CI on every resulting child head. Once ancestry is linear and the exact commits are green, request review again; reviewing the current diverged diff would mostly review code that the rebase will discard or rewrite.
Phase 3 of the CLI deepening: extract the command switch out of src/cli/index.ts into src/cli/dispatch.ts as a registry-driven runner table. index.ts becomes a thin main that passes its local lifecycle helpers through CliDispatchDeps; aliases resolve via the registry alias map. - dispatchCommand(head, deps) replaces the 61-case switch - behavior preserved: restore/sync/sync-cache/claude/route/integration/ health/ready/gui/codex-shim/update runners match the original bodies - source-level tests migrated to read dispatch.ts - typecheck green; CLI suite 211 pass (4 known pre-existing failures)
Phase 3 moved the command switch into src/cli/dispatch.ts as runner keys, but tests/codex-app-server-processes.test.ts still sliced the old case labels out of src/cli/index.ts. Update it to read the sync, sync-cache, v2, and gui runner bodies from dispatch.ts (using deps.args), which restores the #476 sync/sync-cache app-server-wiring assertions.
cb952c0 to
46b18f0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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`:
- Line 497: Capture the canonical runner key in src/cli/dispatch.ts lines
497-497, update CommandRunner to accept a name argument, and pass that resolved
name to the runner. In src/cli/dispatch.ts lines 391-406, make the logs, usage,
storage, and memory runners use the bound name or new parameter when calling
handleObserveCommand. In src/cli/index.ts lines 915-944, remove the redundant
command property from the deps literal and derive the command from head.command
in dispatch.ts.
- Around line 491-504: Update the runner lookup in dispatchCommand to prevent
prototype-inherited keys from resolving as commands: use a null-prototype
commandRunners table or explicitly require own-property membership before
selecting a runner. Preserve declared command and alias resolution, while
ensuring inputs such as constructor, toString, and __defineGetter__ reach the
existing unknown-command error path.
🪄 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: 473cf018-8063-47e2-b881-b9db0963aae7
📒 Files selected for processing (12)
src/cli/dispatch.tssrc/cli/index.tstests/cli-ready.test.tstests/cli-registry.test.tstests/codex-app-server-processes.test.tstests/codex-retained-root-serialization.test.tstests/grok-lifecycle.test.tstests/stale-state-purge.test.tstests/uninstall.test.tstests/update-notify.test.tstests/update-stop-first.test.tstests/windows-deploy-close-regressions.test.ts
| export async function dispatchCommand(head: CliHead, deps: CliDispatchDeps): Promise<void> { | ||
| const command = head.command; | ||
| if (command === undefined || command === "help" || command === "--help" || command === "-h") { | ||
| printUsage(); | ||
| return; | ||
| } | ||
| const runner = commandRunners[command] ?? commandRunners[aliasTargets.get(command) ?? ""]; | ||
| if (!runner) { | ||
| console.error(`Unknown command: ${command}`); | ||
| printUsage(); | ||
| process.exit(1); | ||
| } | ||
| await runner(deps); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Guard the runner lookup against Object.prototype keys; unknown commands can silently succeed.
commandRunners is a plain object literal, so commandRunners[command] walks the prototype chain. Line 497 indexes it directly with unvalidated user input.
Failure mode, reproducible today:
ocx constructor
commandRunners["constructor"] resolves to Object, which is truthy. The !runner guard at Line 498 does not fire. Line 503 then evaluates await Object(deps), which returns deps without throwing. The CLI exits 0 and prints nothing, instead of printing Unknown command: constructor and exiting 1. The same holds for toString, valueOf, hasOwnProperty, and __defineGetter__.
DISPATCH_COMMANDS at Line 488 uses Object.keys, so a command-parity test over that set will not detect this.
Use a null-prototype table so only declared runners resolve.
🐛 Proposed fix using a null-prototype runner table
-const commandRunners: Record<string, CommandRunner> = {
+const commandRunners: Record<string, CommandRunner> = Object.assign(Object.create(null) as Record<string, CommandRunner>, {
init: async () => {Close the literal accordingly:
"-h": async () => {
printUsage();
},
-};
+});An Object.hasOwn(commandRunners, command) check at Line 497 is an equivalent fix if you prefer to keep the literal shape.
🤖 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 `@src/cli/dispatch.ts` around lines 491 - 504, Update the runner lookup in
dispatchCommand to prevent prototype-inherited keys from resolving as commands:
use a null-prototype commandRunners table or explicitly require own-property
membership before selecting a runner. Preserve declared command and alias
resolution, while ensuring inputs such as constructor, toString, and
__defineGetter__ reach the existing unknown-command error path.
| printUsage(); | ||
| return; | ||
| } | ||
| const runner = commandRunners[command] ?? commandRunners[aliasTargets.get(command) ?? ""]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Dispatch treats the user-typed token and the matched runner key as the same value. Line 497 resolves an alias to a canonical runner but discards which key matched, so downstream code that needs the canonical name reads deps.command instead and receives the alias the user typed. Every site below is a consequence of that one modeling gap.
src/cli/dispatch.ts#L497-L497: capture the resolved key, then pass it to the runner. ChangeCommandRunnerto(deps: CliDispatchDeps, name: string) => Promise<void>, computeconst name = commandRunners[command] ? command : (aliasTargets.get(command) ?? ""), and callawait runner(deps, name). This makes the canonical name authoritative at the single point where it is known.src/cli/dispatch.ts#L391-L406: stop readingdeps.command!in thelogs,usage,storage, andmemoryrunners. Use the bound name from the factory proposed on that comment, or the newnameparameter, sohandleObserveCommandalways receives the canonical subcommand.src/cli/index.ts#L915-L944: drop the redundantcommandproperty from the deps literal and derive it fromhead.commandinsidedispatch.ts. Passingargs,command, andheadgives three representations of one parse result, which is what lets the alias and the canonical name drift apart unnoticed.
📍 Affects 2 files
src/cli/dispatch.ts#L497-L497(this comment)src/cli/dispatch.ts#L391-L406src/cli/index.ts#L915-L944
🤖 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 `@src/cli/dispatch.ts` at line 497, Capture the canonical runner key in
src/cli/dispatch.ts lines 497-497, update CommandRunner to accept a name
argument, and pass that resolved name to the runner. In src/cli/dispatch.ts
lines 391-406, make the logs, usage, storage, and memory runners use the bound
name or new parameter when calling handleObserveCommand. In src/cli/index.ts
lines 915-944, remove the redundant command property from the deps literal and
derive the command from head.command in dispatch.ts.
2 participants
Summary
Extract command dispatch out of the entry module into a registry-driven runner table.
src/cli/dispatch.ts:dispatchCommand(head, deps)replaces the 61-case switch insrc/cli/index.ts. Each command is a runner keyed by name; aliases (setup/eject/remove/model) resolve through the registry alias map.src/cli/index.tsbecomes a thin main: it runs the head, then callsdispatchCommandwith its local lifecycle helpers (handleStart,handleStop,handleEnsure,handleStatus,handleReady, …) passed asCliDispatchDeps.dispatch.ts(cli-registry,cli-ready,grok-lifecycle,stale-state-purge,update*,uninstall,windows-deploy-*,codex-retained-root-serialization).Behavior is preserved: the
restore,sync,sync-cache,claude,route,integration,health,ready,gui,codex-shim, andupdaterunners match their original case bodies, including exit codes and the--helpshort-circuit.Verification
bun run typecheck— exit 0.cli-restore-back(2) andPOST /api/syncTask Scheduler (2) cases, proven identical on clean upstream/dev and unrelated to this change.ocx --version—opencodex 2.10.2, exit 0.ocx help nosuch— exit 1,Unknown command: nosuch.ocx ready --timeout 5— exit 64.ocx sync --help—Usage: ocx sync …, exit 0.No GUI changes; no screenshot required.
Checklist
Review notes (stacked PR): this PR stacks on #1446 (
codex/cli-registry). It does not targetdev. The diff here is only the dispatch extraction (7feb16c42..0bb19e1e): 11 files, +585/−513. Merge only after #1444 and #1446 land; the base will be retargeted then.Summary by CodeRabbit
New Features
Bug Fixes
Tests