refactor(cli): colocate display metadata with commands#3098
Merged
Conversation
This reverts commit 4ebeae4.
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors many command entry modules to wrap command classes with a new withCommandDisplay helper, introduces typed command display metadata, extends oclif metadata to carry display entries, and replaces the static command registry with a dynamic registry derived from oclif command-class metadata. Tests updated to validate the registry and dispatch. ChangesCommand Display Infrastructure & Dynamic Registry
Sequence Diagram(s)sequenceDiagram
participant CmdFile as Command file (wrapped)
participant Oclif as oclif loader
participant Registry as Command registry
participant Tests as Tests
CmdFile->>CmdFile: call withCommandDisplay(Command, [display...])
Oclif->>Registry: getRegisteredOclifCommandsMetadata()
Registry->>Registry: displayEntriesFromOclifMetadata() (reads each command.display + commandId)
Registry->>Tests: export COMMANDS, globalCommandTokens, sandboxActionTokens
Tests->>Registry: verify COMMANDS entries and resolveGlobalOclifDispatch(...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Collaborator
Author
Automated PR review summaryReviewed PR #3098: refactor(cli): colocate display metadata with commands Recommendation
Installation and setup findings
What was validated
Failing tests and unresolved impact
Passing tests and why they matteredPassing test 1: Root help still projects command display metadata correctly
Passing test 2: Hidden help/version aliases still function after metadata relocation
Passing test 3: Command-specific help still uses colocated metadata for global and sandbox commands
Passing test 4: Metadata projection still preserves real sandbox command routing
Bottom line
|
Contributor
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 `@src/commands/sandbox/channels/remove.ts`:
- Around line 9-11: The command metadata for the "channels remove" command has a
misleading description ("Clear credentials and rebuild") that doesn't match the
usage/flags; update the description field in the command object (replace the
value of the description key used alongside usage: "nemoclaw <name> channels
remove" and flags: "<channel> [--dry-run]") to accurately describe the command's
purpose (e.g., "Remove channel credentials" and mention the optional --dry-run
behavior), ensuring the description string aligns with the usage and flags in
the command definition.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 80e63c82-c2ce-49fd-b8ab-b0fef87ab0dd
📒 Files selected for processing (50)
src/commands/backup-all.tssrc/commands/credentials/list.tssrc/commands/credentials/reset.tssrc/commands/debug.tssrc/commands/deploy.tssrc/commands/gc.tssrc/commands/list.tssrc/commands/onboard.tssrc/commands/root/help.tssrc/commands/root/version.tssrc/commands/sandbox/channels/add.tssrc/commands/sandbox/channels/list.tssrc/commands/sandbox/channels/remove.tssrc/commands/sandbox/channels/start.tssrc/commands/sandbox/channels/stop.tssrc/commands/sandbox/config/get.tssrc/commands/sandbox/connect.tssrc/commands/sandbox/destroy.tssrc/commands/sandbox/doctor.tssrc/commands/sandbox/gateway/token.tssrc/commands/sandbox/logs.tssrc/commands/sandbox/policy/add.tssrc/commands/sandbox/policy/list.tssrc/commands/sandbox/policy/remove.tssrc/commands/sandbox/rebuild.tssrc/commands/sandbox/share/mount.tssrc/commands/sandbox/share/status.tssrc/commands/sandbox/share/unmount.tssrc/commands/sandbox/shields/down.tssrc/commands/sandbox/shields/status.tssrc/commands/sandbox/shields/up.tssrc/commands/sandbox/skill/install.tssrc/commands/sandbox/snapshot/create.tssrc/commands/sandbox/snapshot/list.tssrc/commands/sandbox/snapshot/restore.tssrc/commands/sandbox/status.tssrc/commands/setup-spark.tssrc/commands/setup.tssrc/commands/start.tssrc/commands/status.tssrc/commands/stop.tssrc/commands/tunnel/start.tssrc/commands/tunnel/stop.tssrc/commands/uninstall.tssrc/commands/upgrade-sandboxes.tssrc/lib/cli/command-display.tssrc/lib/cli/oclif-metadata.tssrc/lib/command-display-metadata.test.tssrc/lib/command-registry.tstest/image-cleanup.test.ts
cjagwani
approved these changes
May 6, 2026
cjagwani
left a comment
Contributor
There was a problem hiding this comment.
metadata moves are 1:1 — verified 3 random commands (debug, gc, sandbox:connect) preserve usage/description/flags/group/scope exactly. image-cleanup test now exercises real dispatch instead of source strings — that's a strengthening, not a regression.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/cli/oclif-metadata.ts (1)
107-114: ⚡ Quick winAvoid exposing a mutable registry reference from the public API.
On Line 108, returning the loader result directly can leak a shared object (especially from the compatibility
requirepath). External mutation of the returned map can affect later calls.♻️ Suggested hardening
-export function getRegisteredOclifCommandsMetadata(): Record<string, OclifCommandMetadata> { - return loadOclifCommands() ?? {}; +export function getRegisteredOclifCommandsMetadata(): Readonly< + Record<string, OclifCommandMetadata> +> { + const commands = loadOclifCommands() ?? {}; + return { ...commands }; }🤖 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/lib/cli/oclif-metadata.ts` around lines 107 - 114, getRegisteredOclifCommandsMetadata currently returns the raw object from loadOclifCommands(), exposing a mutable shared registry; make the function return a defensive copy (e.g., shallow clone or frozen copy) of the map so callers cannot mutate the internal registry, and update getRegisteredOclifCommandMetadata to read from that safe copy (call getRegisteredOclifCommandsMetadata and then lookup the commandId) to avoid leaking the original object; reference functions: getRegisteredOclifCommandsMetadata, getRegisteredOclifCommandMetadata, and loadOclifCommands.
🤖 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.
Nitpick comments:
In `@src/lib/cli/oclif-metadata.ts`:
- Around line 107-114: getRegisteredOclifCommandsMetadata currently returns the
raw object from loadOclifCommands(), exposing a mutable shared registry; make
the function return a defensive copy (e.g., shallow clone or frozen copy) of the
map so callers cannot mutate the internal registry, and update
getRegisteredOclifCommandMetadata to read from that safe copy (call
getRegisteredOclifCommandsMetadata and then lookup the commandId) to avoid
leaking the original object; reference functions:
getRegisteredOclifCommandsMetadata, getRegisteredOclifCommandMetadata, and
loadOclifCommands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 69ebf76a-25e7-4680-8841-013a3f6dff3d
📒 Files selected for processing (2)
src/commands/sandbox/channels/remove.tssrc/lib/cli/oclif-metadata.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
prekshivyas
approved these changes
May 6, 2026
prekshivyas
left a comment
Contributor
There was a problem hiding this comment.
Solid colocation refactor. Display metadata migrates from a 477-line central array in command-registry.ts onto each oclif command entrypoint via a withCommandDisplay(Command, [...]) decorator at src/lib/cli/command-display.ts:47. The legacy CommandDef/COMMANDS API is preserved by deriving it from oclif metadata: displayEntriesFromOclifMetadata() flattens every command's display[], sorts by order, and strips the order field to produce the historical shape. CommandGroup is re-exported for back-compat.
Multi-entry commands distribute correctly: onboard.ts has 2 entries (onboard, onboard --from), root/help.ts has 3 (help, --help, -h), root/version.ts has 2 (--version, -v), sandbox/config/set.ts has 2 (config set, config rotate-token).
Initial CI failure was a real catch — command-registry.test.ts count assertions (52→49, 29→26, 41→40, 11→9, 18→17) flagged that sandbox/config/set.ts and sandbox/recover.ts hadn't been decorated. Carlos pushed a fix-up adding those, and all previously-failing checks now pass: checks (3m30s), macos-e2e (2m34s), build-sandbox-images-arm64 (4m39s), test-e2e-ollama-proxy. wsl-e2e / build-sandbox-images / current self-hosted run still in progress at approval time.
test/image-cleanup.test.ts (+7/-8) is the only file outside src/commands/** and src/lib/** and improves test quality — replaces source-string-grep assertions (registrySrc.toContain('\"nemoclaw gc\"')) with API-level assertions (COMMANDS.toContainEqual(...), resolveGlobalOclifDispatch(\"gc\", []).toEqual({...})). Necessary for the architectural change since display entries no longer live in command-registry.ts source.
New round-trip test in command-display-metadata.test.ts (+15) asserts getRegisteredOclifCommandsMetadata() flattened display entries equal COMMANDS in length and content — proves per-command decorations and the central derived view stay in sync.
Observations (non-blocking):
order: numberis hand-managed (onboard0/1,config/set29/30,root/help44/45/46). Inserting a new command mid-group needs renumbering. Modest maintenance cost; a priority-style scheme would scale better but matches the explicitGROUP_ORDERarray elsewhere.- The count assertion in
command-registry.test.tsis doing real work — caught the missing-decoration bug before review. Valuable invariant going forward.
This was referenced May 6, 2026
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Moves root-help/display metadata out of the central command registry array and onto the oclif command entrypoints discovered under
src/commands/**.command-registry.tsnow projects oclif command-class metadata into the legacyCommandDefshape so command discovery and display metadata continue converging on oclif as the source of truth.Changes
withCommandDisplay(...)metadata helper and attach display entries to public/hidden oclif command entrypoints.COMMANDS, grouped help entries, and legacy command token helpers from oclif-discovered display metadata.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Refactor
Tests