refactor(cli): align public dispatch with oclif conventions#3635
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a public CLI dispatcher, tokenized route matching, generated oclif metadata and public-help rewriting; introduces shared flag factories, centralizes thrown CLI error types, converts many command entrypoints to in-file NemoClawCommand classes, and updates tests to the new contracts. ChangesCLI Infrastructure and Flags Migration
Estimated code review effort 🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/nemoclaw.ts (1)
52-55: 💤 Low valueConsider removing empty section markers.
These section comments no longer contain any code. If the removed functionality won't return, consider deleting these placeholder headers.
🤖 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/nemoclaw.ts` around lines 52 - 55, Remove the two empty placeholder comment headers left in src/nemoclaw.ts (the "// ── Pre-upgrade backup ───────────────────────────────────────────" and "// ── Snapshot ─────────────────────────────────────────────────────" section markers) since they contain no code; if the functionality is permanently removed, delete these lines to avoid clutter, otherwise replace them with a brief TODO comment linking to the intended implementation in the future.
🤖 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/nemoclaw.ts`:
- Around line 52-55: Remove the two empty placeholder comment headers left in
src/nemoclaw.ts (the "// ── Pre-upgrade backup
───────────────────────────────────────────" and "// ── Snapshot
─────────────────────────────────────────────────────" section markers) since
they contain no code; if the functionality is permanently removed, delete these
lines to avoid clutter, otherwise replace them with a brief TODO comment linking
to the intended implementation in the future.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d96b6bc8-b3dc-4e9d-9015-19afac331392
📒 Files selected for processing (6)
src/commands/sandbox/config/set.tssrc/lib/cli/oclif-dispatch.tssrc/lib/cli/public-oclif-help.test.tssrc/lib/cli/public-oclif-help.tssrc/lib/commands/sandbox/config/set.tssrc/nemoclaw.ts
Selective E2E Results — ✅ All requested jobs passedRun: 25946151043
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/cli/public-dispatch.ts (1)
161-164: 💤 Low valueMinor:
argsshadows normalizedactionArgsin scope.
args = argv.slice(1)is used only for connect-order detection (lines 199, 214), whilenormalized.actionArgsis used for actual dispatch. This dual representation is correct but the variable nameargsis generic and could be clearer.Consider renaming to
rawArgsAfterCmdor similar for clarity, though this is optional.🤖 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/public-dispatch.ts` around lines 161 - 164, The local variable name args (assigned from argv.slice(1)) is shadowing/competing with normalized.actionArgs and causes confusion; rename args to a clearer name like rawArgsAfterCmd (or rawArgvAfterCmd) where it’s declared and used in connect-order detection logic so callers still use normalized.actionArgs for dispatch (update occurrences that check connect-order to reference the new name and leave normalized.actionArgs unchanged).
🤖 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/public-dispatch.ts`:
- Around line 161-164: The local variable name args (assigned from
argv.slice(1)) is shadowing/competing with normalized.actionArgs and causes
confusion; rename args to a clearer name like rawArgsAfterCmd (or
rawArgvAfterCmd) where it’s declared and used in connect-order detection logic
so callers still use normalized.actionArgs for dispatch (update occurrences that
check connect-order to reference the new name and leave normalized.actionArgs
unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ee13fcad-d034-42ff-8a28-8084c03ab896
📒 Files selected for processing (15)
src/commands/sandbox/shields/down.tssrc/lib/cli/common-flags.tssrc/lib/cli/oclif-dispatch.test.tssrc/lib/cli/oclif-dispatch.tssrc/lib/cli/public-dispatch.tssrc/lib/commands/credentials/reset.tssrc/lib/commands/maintenance/gc.tssrc/lib/commands/maintenance/update.tssrc/lib/commands/maintenance/upgrade-sandboxes.tssrc/lib/commands/sandbox/channels/common.tssrc/lib/commands/sandbox/destroy.tssrc/lib/commands/sandbox/hosts/common.tssrc/lib/commands/sandbox/policy/common.tssrc/lib/commands/sandbox/rebuild.tssrc/nemoclaw.ts
✅ Files skipped from review due to trivial changes (3)
- src/commands/sandbox/shields/down.ts
- src/lib/commands/sandbox/rebuild.ts
- src/lib/commands/maintenance/upgrade-sandboxes.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Selective E2E Results — ✅ All requested jobs passedRun: 25947131613
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/cli/public-oclif-help.ts (1)
41-49: ⚡ Quick winKeep help tokenization aligned with dispatch tokenization.
publicRouteTokens()already drifted fromliteralTokensFromUsage()insrc/lib/cli/oclif-dispatch.ts: this helper does not stop on(. A usage like<name> foo (bar|baz)would be rewritten as if(bar|baz)were a literal route token even though dispatch treats it as a boundary. Reusing the same tokenizer, or at least matching the same stop rules here, will keep routing and help output in sync.Minimal fix
- if (token.startsWith("[") || token.startsWith("<") || token.startsWith("-")) break; + if ( + token.startsWith("[") || + token.startsWith("<") || + token.startsWith("(") || + token.startsWith("-") + ) break;🤖 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/public-oclif-help.ts` around lines 41 - 49, publicRouteTokens currently treats tokens starting with "(" as literals, causing divergence from literalTokensFromUsage; update publicRouteTokens to use the same stop rules as literalTokensFromUsage (or directly call literalTokensFromUsage) so it breaks on "(" as well as "[" "<" and "-" — locate the publicRouteTokens function and either reuse literalTokensFromUsage for tokenization or add a check to break when token.startsWith("(") to keep dispatch and help output aligned.
🤖 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/lib/cli/oclif-dispatch.ts`:
- Around line 211-212: The current branch unconditionally returns
parentHelp(action) for any action string "config" or "shields", which hides
invalid-subcommand errors; update the logic in oclif-dispatch.ts so that you
only show parentHelp for the legitimate parent invocation (exact/expected
token), and for any unmatched/typo input for these groups preserve a non-zero
failure path similar to how the "channels" branch does — e.g., validate the
subcommand name/args and for invalid values call the same error/exit routine
used by channels (or invoke parentHelp with a non-zero exit indicator) instead
of returning plain help; refer to the action variable and parentHelp function to
implement this change.
---
Nitpick comments:
In `@src/lib/cli/public-oclif-help.ts`:
- Around line 41-49: publicRouteTokens currently treats tokens starting with "("
as literals, causing divergence from literalTokensFromUsage; update
publicRouteTokens to use the same stop rules as literalTokensFromUsage (or
directly call literalTokensFromUsage) so it breaks on "(" as well as "[" "<" and
"-" — locate the publicRouteTokens function and either reuse
literalTokensFromUsage for tokenization or add a check to break when
token.startsWith("(") to keep dispatch and help output aligned.
🪄 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: b7d42fc5-293a-446f-a3f8-ec39718d2011
📒 Files selected for processing (5)
src/lib/cli/oclif-dispatch.test.tssrc/lib/cli/oclif-dispatch.tssrc/lib/cli/public-dispatch.tssrc/lib/cli/public-oclif-help.test.tssrc/lib/cli/public-oclif-help.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/cli/public-oclif-help.test.ts
- src/lib/cli/oclif-dispatch.test.ts
- src/lib/cli/public-dispatch.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Selective E2E Results — ✅ All requested jobs passedRun: 25947423915
|
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/lib/sandbox/config.ts`:
- Around line 819-822: The hint text uses the old internal command form; update
the recovery guidance in the configFail call so it references the sandbox-scoped
public command grammar by replacing "Use `nemoclaw config rotate-token` for
credential changes." with "Use `nemoclaw sandbox rotate-token` for credential
changes." (edit the second string in the array passed to configFail).
🪄 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: 95308dc4-f704-4e87-8603-0e3978b930c6
📒 Files selected for processing (10)
src/lib/cli/oclif-runner.test.tssrc/lib/cli/oclif-runner.tssrc/lib/cli/public-oclif-help.test.tssrc/lib/commands/credentials/reset.tssrc/lib/commands/sandbox/config/get.tssrc/lib/commands/sandbox/config/set.tssrc/lib/commands/sandbox/oclif-command-adapters.test.tssrc/lib/sandbox/config.tstest/cli.test.tstest/config-set-nested-ssrf.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/cli/oclif-runner.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/commands/credentials/reset.ts
Selective E2E Results — ✅ All requested jobs passedRun: 25948148546
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/lib/cli/oclif-dispatch.ts (1)
211-213:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInvalid
config/shieldssubcommands return success-style help instead of error.This is the issue flagged in a previous review. The current code unconditionally returns
parentHelp(action)without an error message or exit code for any unmatched input (e.g.,config bogus,shields typo). This causes these invalid subcommand cases to exit like successful help requests, unlike thechannelscase at line 207 which properly returns an error message.This regresses the invalid-subcommand diagnostic for these groups and makes scripting against failures harder.
Proposed fix
if (action === "config" || action === "shields") { + if (hasHelpFlag(actionArgs)) return parentHelp(action); + const subcommand = actionArgs[0]; + return parentHelp( + action, + subcommand ? `Unknown ${action} subcommand: ${subcommand}` : undefined, + ); - return parentHelp(action); }🤖 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-dispatch.ts` around lines 211 - 213, The current branch for action === "config" || action === "shields" returns parentHelp(action) which produces a success-style help exit; change it to follow the same error path used for "channels" so invalid subcommands produce an error message and non-zero exit. Concretely, replace the unconditional return parentHelp(action) call with the same error-handling call used for channels (e.g., call the helper that prints parent help as an error or invoke parentHelp with an error flag / throw a CLI error) so that "config bogus" and "shields typo" emit an explicit invalid-subcommand error and a non-zero exit code; keep the reference to parentHelp and action so reviewers can find the change.
🧹 Nitpick comments (1)
src/lib/cli/public-dispatch.ts (1)
218-225: 💤 Low valueUnreachable code path or redundant check.
Lines 218-225 check
!registry.getSandbox(cmd)after the earlier block (lines 187-216) already handles the case where the sandbox doesn't exist and exits. If control reaches line 218, either:
- The sandbox wasn't a valid action (line 187's condition failed), or
- Recovery succeeded and
registry.getSandbox(cmd)now returns truthyIn case (1),
sandboxActions.includes(requestedSandboxAction)was false, so the user provided a non-sandbox-action. The code at line 227 then checksregistry.getSandbox(cmd)again. This path seems intended for whencmdisn't a known sandbox AND the action isn't a sandbox action — but the suggestion logic here doesn't account for that distinction.Consider whether this block is reachable or if the logic should be consolidated.
🤖 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/public-dispatch.ts` around lines 218 - 225, The duplicate check of registry.getSandbox(cmd) is redundant and creates an unreachable or confusing path; update the logic in public-dispatch.ts around the sandbox resolution (the block that uses sandboxActions, requestedSandboxAction, and registry.getSandbox) to consolidate suggestion handling into a single place: after determining that the command is not a known sandbox and requestedSandboxAction is not a sandbox action, call suggestGlobalCommand(cmd) once and log the "Unknown command / Did you mean" message and exit; remove the second registry.getSandbox(cmd) conditional (the later block at lines ~218-225) or merge its suggestion logic into the earlier failure branch so suggestion only runs for truly unknown commands. Ensure you reference and update the code that computes requestedSandboxAction, sandboxActions, and calls registry.getSandbox(cmd).
🤖 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/lib/cli/oclif-dispatch.test.ts`:
- Around line 197-216: The test currently checks
resolveLegacySandboxDispatch("alpha", "config", ["bogus"]) and ("shields",
["bogus"]) only for kind, commandId and publicUsage; update these expectations
to also assert that invalid subcommands produce a non-zero exit code and an
error message like the `channels` test does — i.e. include exitCode: 1 and a
message string indicating the invalid subcommand (e.g. "Unknown subcommand
'bogus' for config" or similar) so the tests mirror the behavior asserted in the
`channels` case and preserve diagnostic parity; modify the expectations for the
`config` and `shields` assertions in oclif-dispatch.test.ts to include exitCode
and message fields referencing resolveLegacySandboxDispatch results.
In `@src/lib/cli/public-dispatch.ts`:
- Around line 111-117: The "usageError" switch case can fall through into
"unknownAction" because TypeScript doesn't know printDispatchUsageError will
exit; update the switch handling in public-dispatch by making the "usageError"
branch explicitly not continue execution — e.g., add a return immediately after
calling printDispatchUsageError(result, opts.sandboxName) (or otherwise
restructure so the function never reaches the "unknownAction" branch) to prevent
unintended fall-through to the unknownAction case that logs and exits.
---
Duplicate comments:
In `@src/lib/cli/oclif-dispatch.ts`:
- Around line 211-213: The current branch for action === "config" || action ===
"shields" returns parentHelp(action) which produces a success-style help exit;
change it to follow the same error path used for "channels" so invalid
subcommands produce an error message and non-zero exit. Concretely, replace the
unconditional return parentHelp(action) call with the same error-handling call
used for channels (e.g., call the helper that prints parent help as an error or
invoke parentHelp with an error flag / throw a CLI error) so that "config bogus"
and "shields typo" emit an explicit invalid-subcommand error and a non-zero exit
code; keep the reference to parentHelp and action so reviewers can find the
change.
---
Nitpick comments:
In `@src/lib/cli/public-dispatch.ts`:
- Around line 218-225: The duplicate check of registry.getSandbox(cmd) is
redundant and creates an unreachable or confusing path; update the logic in
public-dispatch.ts around the sandbox resolution (the block that uses
sandboxActions, requestedSandboxAction, and registry.getSandbox) to consolidate
suggestion handling into a single place: after determining that the command is
not a known sandbox and requestedSandboxAction is not a sandbox action, call
suggestGlobalCommand(cmd) once and log the "Unknown command / Did you mean"
message and exit; remove the second registry.getSandbox(cmd) conditional (the
later block at lines ~218-225) or merge its suggestion logic into the earlier
failure branch so suggestion only runs for truly unknown commands. Ensure you
reference and update the code that computes requestedSandboxAction,
sandboxActions, and calls registry.getSandbox(cmd).
🪄 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: c7189628-dc9e-4c1a-9376-0c42f14b5548
📒 Files selected for processing (5)
src/commands/credentials/reset.tssrc/lib/cli/oclif-dispatch.test.tssrc/lib/cli/oclif-dispatch.tssrc/lib/cli/public-dispatch.tstest/cli.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/commands/credentials/reset.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/cli.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Selective E2E Results — ✅ All requested jobs passedRun: 25949522967
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Selective E2E Results — ✅ All requested jobs passedRun: 25949902841
|
…-cleanup # Conflicts: # .coderabbit.yaml # src/commands/sandbox/exec.test.ts # src/lib/cli/command-registry.test.ts # src/lib/cli/oclif-dispatch.test.ts # src/lib/cli/oclif-dispatch.ts # src/nemoclaw.ts # test/cli-oclif-compatibility.test.ts
…nto refactor/oclif-router-cleanup # Conflicts: # .coderabbit.yaml
Selective E2E Results — ❌ Some jobs failedRun: 26110131067
|
Selective E2E Results — ✅ All requested jobs passedRun: 26112718750
|
Selective E2E Results — ❌ Some jobs failedRun: 26112986647
|
Selective E2E Results — ✅ All requested jobs passedRun: 26116643258
|
|
🔴 E2E blocker: the advisor-dispatched required E2E run is still failing. Run: https://github.com/NVIDIA/NemoClaw/actions/runs/26112986647 Failure details:
Please address or rerun with evidence if this is an unrelated flaky/environmental failure. Since this PR rewires broad CLI dispatch/command adapters and the E2E advisor marked |
ericksoa
left a comment
There was a problem hiding this comment.
Approved with one accepted follow-up risk from local review.
Focused validation passed locally: npm run build:cli, npm run typecheck:cli, focused CLI routing/metadata tests (10 files, 85 tests), and git diff --check origin/main...HEAD. Visible GitHub checks are green.
Comment for follow-up: the built CLI currently routes parent help for nested sandbox groups that do not have parent command files to non-existent oclif command IDs, e.g. nemoclaw alpha channels --help -> command sandbox:channels not found, similarly config --help and shields --help. share, snapshot, and skill work because those parent commands exist. This is a user-facing help regression worth fixing in a follow-up, but per maintainer direction I am not blocking merge on it.
Selective E2E Results — ✅ All requested jobs passedRun: 26117524739
|
Selective E2E Results — ❌ Some jobs failedRun: 26119271812
|
Summary
This PR keeps the public
nemoclaw <sandbox-name> <action>grammar while moving command discovery, dispatch, parsing, help, JSON output, and error reporting closer to oclif conventions. It reduces custom router behavior, colocates command display metadata with command classes, and converts several lower-levelprocess.exitpaths into oclif-owned command failures.Changes
src/lib/cli/public-dispatch.tsand keepsrc/nemoclaw.tsas a thin entrypoint shim.src/commands/**wrappers.nemoclaw sandbox ...grammar.inference get --jsonanddoctor --json.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
Bug Fixes
Documentation