refactor(cli): add command registry as single metadata source - #1446
Conversation
📝 WalkthroughWalkthroughThe CLI now uses a central registry for visible and hidden command metadata. Registry lookup supports exact names and aliases. Subcommand help uses the registry, and tests verify command parity, aliases, hidden commands, and canonical-name uniqueness. ChangesCLI command registry
Estimated code review effort: 3 (Moderate) | ~20 minutes 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. |
github-actions
Bot
added
the
chore
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/cli-registry.test.ts`:
- Around line 29-34: Update the registry coverage test around CLI_COMMANDS so
each canonical entry.name must be present directly in caseSet; remove the
fallback that allows an alias to satisfy the canonical command assertion. If
aliases are also required to have dispatcher cases, add a separate assertion
covering entry.aliases.
🪄 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: 2a81bdef-5b74-4431-b093-c218757f30b8
📒 Files selected for processing (3)
src/cli/help.tssrc/cli/registry.tstests/cli-registry.test.ts
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
The command registry direction is valuable, but one blocker remains on exact head 7feb16c426.
tests/cli-registry.test.ts allows an alias switch case to satisfy a missing canonical dispatcher case. If case "init" disappears while case "setup" remains, CLI_COMMANDS still advertises init and the current test passes even though ocx init no longer dispatches. Require every entry.name directly in caseSet; if aliases are also part of the dispatch contract, assert them separately.
The current macOS failure is a Bun 1.3.14 SIGTRAP/segmentation fault after the suite had progressed through 10k+ tests, not a registry assertion. It should be rerun after the test fix and after parent #1444 is rebased onto current dev; the stacked child must then move with the parent so exact-head CI represents the integration result.
Addresses CodeRabbit finding on #1446: the previous assertion let an alias case satisfy a missing canonical case, so dropping e.g. 'init' while 'setup' remained would pass. Require each entry.name directly in caseSet.
2c9694d to
c2da0f6
Compare
Addresses CodeRabbit finding on #1446: the previous assertion let an alias case satisfy a missing canonical case, so dropping e.g. 'init' while 'setup' remained would pass. Require each entry.name directly in caseSet.
0626634 to
487d644
Compare
|
Both blockers from the review are addressed on the current head (\487d644):
The registry test fix also carries into the later dispatch phases (the same assertion now checks runner keys directly). |
Phase 2 of the CLI deepening: command names, aliases, usage, summary, and details move from src/cli/help.ts into src/cli/registry.ts (CLI_COMMANDS + findCommand). help.ts becomes a thin renderer over the registry; behavior is unchanged. - 48 visible entries in original order, plus 6 hidden __* entries - alias pairs: init/setup, restore/eject, uninstall/remove, models/model - exact-name-wins lookup keeps alias-name entries' own help text - tests/cli-registry.test.ts pins switch-case <-> registry parity - all 48 help outputs byte-identical; typecheck green
Addresses CodeRabbit finding on #1446: the previous assertion let an alias case satisfy a missing canonical case, so dropping e.g. 'init' while 'setup' remained would pass. Require each entry.name directly in caseSet.
487d644 to
3562810
Compare
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 `@tests/cli-registry.test.ts`:
- Around line 56-70: Extend the hidden-command test around CLI_COMMANDS to
render top-level help and assert every hidden entry.name is absent from that
output, while preserving the existing caseSet and findCommand assertions. Use
the established help-rendering API from the CLI help subsystem rather than
testing raw command metadata.
- Around line 72-75: Extend the “entry names are unique” test to also assert
uniqueness of all aliases by flattening each entry’s optional aliases and
comparing the Set size with the flattened array length. Keep aliases that match
canonical command names valid; only reject duplicate alias declarations across
entries.
🪄 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: f8678394-f21f-4133-939c-0adfff3592b3
📒 Files selected for processing (1)
tests/cli-registry.test.ts
| test("hidden entries are flagged and do not appear in help lookups by accident", () => { | ||
| const hidden = CLI_COMMANDS.filter(entry => entry.hidden); | ||
| expect(hidden.map(entry => entry.name).sort()).toEqual([ | ||
| "__gui-update-worker", | ||
| "__refresh-version", | ||
| "__startup-health", | ||
| "__tray-host", | ||
| "__tray-restart", | ||
| "__tray-start", | ||
| ]); | ||
| for (const entry of hidden) { | ||
| expect(caseSet.has(entry.name)).toBe(true); | ||
| expect(findCommand(entry.name)?.name).toBe(entry.name); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover hidden-command visibility in rendered help.
This test checks the hidden flag, dispatcher coverage, and direct findCommand resolution. It does not check that hidden commands are absent from the rendered top-level help. A regression in src/cli/help.ts could expose hidden commands while this test still passes. Assert that every hidden entry.name is absent from the rendered top-level help. Keep the direct lookup assertion if ocx help <hidden-command> is intentionally supported.
As per path instructions: “A behavior change in src/ should come with 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 56 - 70, Extend the hidden-command
test around CLI_COMMANDS to render top-level help and assert every hidden
entry.name is absent from that output, while preserving the existing caseSet and
findCommand assertions. Use the established help-rendering API from the CLI help
subsystem rather than testing raw command metadata.
Source: Path instructions
| test("entry names are unique", () => { | ||
| const names = CLI_COMMANDS.map(entry => entry.name); | ||
| expect(new Set(names).size).toBe(names.length); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject duplicate alias declarations.
This assertion checks only canonical entry.name values. findCommand resolves aliases by selecting the first matching registry entry, so duplicate aliases silently make lookup order determine the help metadata. Add a uniqueness assertion over CLI_COMMANDS.flatMap(entry => entry.aliases ?? []). Do not reject aliases that also have canonical entries; exact-name precedence is intentional for setup, eject, remove, and model.
Based on the registry lookup contract in src/cli/registry.ts Lines 375-376, duplicate aliases are order-dependent.
Proposed assertion
test("entry names are unique", () => {
const names = CLI_COMMANDS.map(entry => entry.name);
expect(new Set(names).size).toBe(names.length);
+ const aliases = CLI_COMMANDS.flatMap(entry => entry.aliases ?? []);
+ expect(new Set(aliases).size).toBe(aliases.length);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("entry names are unique", () => { | |
| const names = CLI_COMMANDS.map(entry => entry.name); | |
| expect(new Set(names).size).toBe(names.length); | |
| }); | |
| test("entry names are unique", () => { | |
| const names = CLI_COMMANDS.map(entry => entry.name); | |
| expect(new Set(names).size).toBe(names.length); | |
| const aliases = CLI_COMMANDS.flatMap(entry => entry.aliases ?? []); | |
| expect(new Set(aliases).size).toBe(aliases.length); | |
| }); |
🤖 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 72 - 75, Extend the “entry names are
unique” test to also assert uniqueness of all aliases by flattening each entry’s
optional aliases and comparing the Set size with the flattened array length.
Keep aliases that match canonical command names valid; only reject duplicate
alias declarations across entries.
2 participants
Summary
Extract CLI command metadata into a single source of truth.
src/cli/registry.ts: one typed registry of everyocxcommand (name, aliases, usage, summary, details, hidden flag) withfindCommandandcommandNameshelpers.src/cli/help.tsas a thin renderer over the registry: usage text,help/--helpdispatch, and per-subcommand help now read from the same metadata. No user-visible output changes.tests/cli-registry.test.ts: parity between the registry and the command dispatch insrc/cli/index.ts, plus alias, hidden-entry, and uniqueness coverage.This is a pure refactor. It is the first step toward one metadata source for CLI help, docs, and shell completion.
Verification
bun run typecheck— exit 0.help.tsrenderer.tests/cli-registry.test.ts— 6/6 pass.cli-restore-backcases, proven identical on clean upstream/dev and unrelated to this change.ocx help nosuch— exit 1 with the unknown-command error.ocx --version—opencodex 2.10.2.ready --timeout 5— exit 64 (invalid-argument handling preserved).No GUI changes; no screenshot required.
Checklist
Review notes (stacked PR): this PR stacks on #1444 (
codex/cli-deepening). It does not targetdev. The diff here is only the registry extraction (2c9694df1..7feb16c42): 3 files, +463/−277. Merge this only after #1444 lands; the base will be retargeted todevthen.Summary by CodeRabbit
New Features
Bug Fixes
Tests