Skip to content

refactor(cli): align public dispatch with oclif conventions#3635

Open
cv wants to merge 19 commits into
mainfrom
refactor/oclif-router-cleanup
Open

refactor(cli): align public dispatch with oclif conventions#3635
cv wants to merge 19 commits into
mainfrom
refactor/oclif-router-cleanup

Conversation

@cv
Copy link
Copy Markdown
Collaborator

@cv cv commented May 15, 2026

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-level process.exit paths into oclif-owned command failures.

Changes

  • Extract public grammar routing into src/lib/cli/public-dispatch.ts and keep src/nemoclaw.ts as a thin entrypoint shim.
  • Derive global and sandbox parent usage from registered command metadata, and render parent usage through the public oclif help adapter.
  • Colocate global and sandbox display metadata on command classes, simplifying src/commands/** wrappers.
  • Rewrite public sandbox help examples away from oclif-native nemoclaw sandbox ... grammar.
  • Move more validation into oclif parsing for config, credentials, and inference commands.
  • Use oclif JSON handling for inference get --json and doctor --json.
  • Convert config, share, and host-alias action failures to typed errors handled by oclif command adapters.
  • Add shared common flag builders and tests for public-help drift plus native oclif taxonomy behavior.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Verification

  • npx prek run --all-files passes
  • npm test passes
  • Tests added or updated for new or changed behavior
  • No secrets, API keys, or credentials committed
  • Docs updated for user-facing behavior changes
  • make docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Carlos Villela cvillela@nvidia.com

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv cv self-assigned this May 15, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 15, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Token-based global dispatch replaces static maps; a new public-dispatcher normalizes argv and renders public help; public-help rewrites sandbox examples; shared flag helpers added and migrated across commands; sandbox config error handling centralized via SandboxConfigError/configFail; command metadata and tests updated; top-level entrypoint simplified.

Changes

CLI Infrastructure and Flags Migration

Layer / File(s) Summary
Global dispatch token-based routing
src/lib/cli/oclif-dispatch.ts
Derives literal token sequences from command usage, matches incoming cmd+args by prefix to select targets, and returns metadata-derived parent help for certain unknown subcommands.
Public help generation & example rewriting
src/lib/cli/public-oclif-help.ts, src/lib/cli/public-oclif-help.test.ts
Adds PublicUsage type, tokenizers and example rewrites to present sandbox commands in public CLI grammar; tests updated to assert public-form examples and parent-usage rendering.
Public compatibility dispatcher (dispatchCli)
src/lib/cli/public-dispatch.ts
New dispatcher that normalizes argv into root/global/sandbox, delegates to oclif or legacy sandbox dispatch, renders public help, handles registry recovery for missing sandbox names, and centralizes usage/unknown-action output.
Shared common flags & migrations
src/lib/cli/common-flags.ts, multiple command modules
Introduces yesFlag, forceFlag, dryRunFlag, jsonFlag and migrates many commands to consume these helpers instead of inline Flags.boolean, simplifying imports.
oclif dispatch & runner tests
src/lib/cli/oclif-dispatch.test.ts, src/lib/cli/oclif-runner.test.ts
Updates tests to expect help-style dispatch results for unsupported subcommands and to assert formatted required-argument parse errors trigger the exit code path.
Sandbox config commands & adapters
src/commands/sandbox/config/set.ts, src/lib/commands/sandbox/config/get.ts, src/lib/commands/sandbox/oclif-command-adapters.test.ts
Adds required-flag metadata for set, wraps get/set invocations in try/catch to handle SandboxConfigError via failWithLines, and updates adapter mocks/tests to exercise the new error class behavior.
Sandbox config core
src/lib/sandbox/config.ts, test/config-set-nested-ssrf.test.ts
Adds exported SandboxConfigError and configFail() helper; replaces direct stderr+process.exit flows with thrown structured errors; tests updated to assert thrown errors and redaction semantics.
Shields down & credentials metadata
src/commands/sandbox/shields/down.ts, src/commands/credentials/reset.ts
Adds advertised flags strings showing supported flag syntax for shields down and credentials reset commands.
Top-level shim
src/nemoclaw.ts
Replaces prior in-file CLI front-controller with a thin shim that re-exports and invokes dispatchCli from the public dispatcher.
Misc command flag migrations
src/lib/commands/* (gc, update, upgrade-sandboxes, destroy, channels/hosts/policy/rebuild, credentials reset)
Commands updated to import and use shared flag helpers (yesFlag, forceFlag, dryRunFlag) instead of inline Flags.boolean definitions; imports cleaned accordingly.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

NemoClaw CLI, status: rfr

Suggested reviewers

  • jyaunches
  • cjagwani

Poem

"🐇 I hopped through tokens with a tiny drum,
I stitched shared flags and made help hum.
Examples now sing in public CLI light,
Config errors catch, not vanish in night.
Hooray — the CLI hops, carrot code bite."

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.20% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'refactor(cli): align public dispatch with oclif conventions' accurately describes the main objective of consolidating CLI routing and help behavior around oclif display metadata while preserving the public grammar.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/oclif-router-cleanup

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 15, 2026

E2E Advisor Recommendation

Required E2E: cloud-onboard-e2e, sandbox-operations-e2e, diagnostics-e2e, openclaw-inference-switch-e2e, network-policy-e2e, shields-config-e2e, channels-stop-start-e2e
Optional E2E: credential-migration-e2e, tunnel-lifecycle-e2e, snapshot-commands-e2e, state-backup-restore-e2e, upgrade-stale-sandbox-e2e

Dispatch hint: cloud-onboard-e2e,sandbox-operations-e2e,diagnostics-e2e,openclaw-inference-switch-e2e,network-policy-e2e,shields-config-e2e,channels-stop-start-e2e

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • cloud-onboard-e2e (medium): Validates the install/onboard public user path after command entrypoints, public dispatch, and onboard command adapters changed.
  • sandbox-operations-e2e (high): Highest-signal coverage for broad sandbox command dispatch and lifecycle operations affected by the oclif/public-dispatch refactor, including status/logs/connect/destroy-style real sandbox operations.
  • diagnostics-e2e (medium): Covers changed debug, version, credentials list/reset, and diagnostics support flows, including no-secret-leak assertions for credential output.
  • openclaw-inference-switch-e2e (medium): Covers changed inference get/set command paths against a running OpenClaw sandbox and verifies route/config/live-request behavior after switching providers.
  • network-policy-e2e (medium): Required for changed policy and config command adapters plus SSRF/security-sensitive config behavior; validates live policy-add, dry-run, hot reload, inference exemption, and SSRF validation.
  • shields-config-e2e (medium): Required because shields up/down/status and sandbox config commands changed; verifies filesystem/security shield lifecycle and config immutability in a real sandbox.
  • channels-stop-start-e2e (high): Required because channels start/stop/add/remove/list command adapters changed and these commands control credential-backed messaging bridge lifecycle with rebuilds.

Optional E2E

  • credential-migration-e2e (medium): Useful additional confidence for credential storage/gateway interactions after credentials commands and onboarding paths changed, but diagnostics already covers list/reset and leak checks.
  • tunnel-lifecycle-e2e (medium): Tunnel start/stop and deprecated start/stop aliases changed via command wrapper refactoring; run if extra confidence is desired for service lifecycle dispatch.
  • snapshot-commands-e2e (medium): Snapshot create/list/restore command adapters changed; useful adjacent coverage if sandbox-operations does not exercise snapshot command syntax deeply enough.
  • state-backup-restore-e2e (high): Backup/share/state related command entrypoints changed; useful confidence for state preservation but less directly targeted than the required lifecycle/security jobs.
  • upgrade-stale-sandbox-e2e (high): Upgrade-sandboxes/update/rebuild command paths changed; optional because it is expensive and the required onboard/sandbox tests should catch basic dispatch regressions.

New E2E recommendations

  • Host alias lifecycle (high): src/lib/actions/sandbox/host-aliases.ts changed error handling and command behavior, and no existing E2E match was found for hosts-add, hosts-list, or hosts-remove. These commands mutate sandbox pod hostAliases and affect network/security boundaries.
    • Suggested test: Add an E2E that onboards a sandbox, runs nemoclaw <name> hosts-add <hostname> <ip>, verifies /etc/hosts or Kubernetes hostAliases in the live sandbox, runs hosts-list, then hosts-remove and verifies removal including invalid hostname/IP error paths.
  • Public CLI help/unknown-subcommand compatibility (medium): The PR changes metadata-derived parent help for tunnel, inference, credentials, config, shields, and channels. Unit tests cover dispatch objects, but a real packaged CLI E2E should verify user-visible output and exit codes after build/install.
    • Suggested test: Add a lightweight E2E or scenario suite that installs/builds the CLI and asserts nemoclaw tunnel bogus, nemoclaw inference bogus, nemoclaw credentials bogus, nemoclaw <name> channels bogus, nemoclaw <name> config bogus, and nemoclaw <name> shields bogus print branded public usage and exit non-zero without stack traces.
  • Sandbox doctor JSON contract (medium): runSandboxDoctor now builds and can return a DoctorReport with quiet JSON handling. Existing diagnostics coverage mentions config/credentials/debug but no direct E2E evidence was found for nemoclaw <name> doctor --json.
    • Suggested test: Add an E2E assertion to diagnostics or sandbox-operations that runs nemoclaw <name> doctor --json, validates schemaVersion/status/checks fields, and verifies non-zero exit behavior only when failing checks are present.

Dispatch hint

  • Workflow: nightly-e2e.yaml
  • jobs input: cloud-onboard-e2e,sandbox-operations-e2e,diagnostics-e2e,openclaw-inference-switch-e2e,network-policy-e2e,shields-config-e2e,channels-stop-start-e2e

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/nemoclaw.ts (1)

52-55: 💤 Low value

Consider 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

📥 Commits

Reviewing files that changed from the base of the PR and between fdae301 and 5ac7144.

📒 Files selected for processing (6)
  • src/commands/sandbox/config/set.ts
  • src/lib/cli/oclif-dispatch.ts
  • src/lib/cli/public-oclif-help.test.ts
  • src/lib/cli/public-oclif-help.ts
  • src/lib/commands/sandbox/config/set.ts
  • src/nemoclaw.ts

@cv cv added the v0.0.45 Release target label May 15, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 25946151043
Target ref: 5ac7144c8d111fe96830e119f63a72f0f03604bc
Workflow ref: main
Requested jobs: cloud-e2e,sandbox-operations-e2e,shields-config-e2e
Summary: 3 passed, 0 failed, 0 skipped

Job Result
cloud-e2e ✅ success
sandbox-operations-e2e ✅ success
shields-config-e2e ✅ success

cv added 3 commits May 15, 2026 16:47
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/lib/cli/public-dispatch.ts (1)

161-164: 💤 Low value

Minor: args shadows normalized actionArgs in scope.

args = argv.slice(1) is used only for connect-order detection (lines 199, 214), while normalized.actionArgs is used for actual dispatch. This dual representation is correct but the variable name args is generic and could be clearer.

Consider renaming to rawArgsAfterCmd or 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

📥 Commits

Reviewing files that changed from the base of the PR and between 5ac7144 and a0a7081.

📒 Files selected for processing (15)
  • src/commands/sandbox/shields/down.ts
  • src/lib/cli/common-flags.ts
  • src/lib/cli/oclif-dispatch.test.ts
  • src/lib/cli/oclif-dispatch.ts
  • src/lib/cli/public-dispatch.ts
  • src/lib/commands/credentials/reset.ts
  • src/lib/commands/maintenance/gc.ts
  • src/lib/commands/maintenance/update.ts
  • src/lib/commands/maintenance/upgrade-sandboxes.ts
  • src/lib/commands/sandbox/channels/common.ts
  • src/lib/commands/sandbox/destroy.ts
  • src/lib/commands/sandbox/hosts/common.ts
  • src/lib/commands/sandbox/policy/common.ts
  • src/lib/commands/sandbox/rebuild.ts
  • src/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>
@github-actions
Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 25947131613
Target ref: a0a7081673e5906e591293890fb84a5430f7fb80
Workflow ref: main
Requested jobs: sandbox-operations-e2e,shields-config-e2e,network-policy-e2e,upgrade-stale-sandbox-e2e,channels-stop-start-e2e
Summary: 1 passed, 0 failed, 0 skipped

Job Result
channels-stop-start-e2e ⚠️ cancelled
network-policy-e2e ⚠️ cancelled
sandbox-operations-e2e ⚠️ cancelled
shields-config-e2e ✅ success
upgrade-stale-sandbox-e2e ⚠️ cancelled

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/lib/cli/public-oclif-help.ts (1)

41-49: ⚡ Quick win

Keep help tokenization aligned with dispatch tokenization.

publicRouteTokens() already drifted from literalTokensFromUsage() in src/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

📥 Commits

Reviewing files that changed from the base of the PR and between a0a7081 and 20972c8.

📒 Files selected for processing (5)
  • src/lib/cli/oclif-dispatch.test.ts
  • src/lib/cli/oclif-dispatch.ts
  • src/lib/cli/public-dispatch.ts
  • src/lib/cli/public-oclif-help.test.ts
  • src/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

Comment thread src/lib/cli/oclif-dispatch.ts
cv added 3 commits May 15, 2026 17:26
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions
Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 25947423915
Target ref: 20972c8ab43a02420c2df1fc1a61cf588780044b
Workflow ref: main
Requested jobs: sandbox-operations-e2e,shields-config-e2e,network-policy-e2e,upgrade-stale-sandbox-e2e,diagnostics-e2e,channels-stop-start-e2e
Summary: 6 passed, 0 failed, 0 skipped

Job Result
channels-stop-start-e2e ✅ success
diagnostics-e2e ✅ success
network-policy-e2e ✅ success
sandbox-operations-e2e ✅ success
shields-config-e2e ✅ success
upgrade-stale-sandbox-e2e ✅ success

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 20972c8 and d87bee5.

📒 Files selected for processing (10)
  • src/lib/cli/oclif-runner.test.ts
  • src/lib/cli/oclif-runner.ts
  • src/lib/cli/public-oclif-help.test.ts
  • src/lib/commands/credentials/reset.ts
  • src/lib/commands/sandbox/config/get.ts
  • src/lib/commands/sandbox/config/set.ts
  • src/lib/commands/sandbox/oclif-command-adapters.test.ts
  • src/lib/sandbox/config.ts
  • test/cli.test.ts
  • test/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

Comment thread src/lib/sandbox/config.ts
Comment on lines +819 to +822
configFail([
" Cannot modify the gateway section directly.",
" Use `nemoclaw config rotate-token` for credential changes.",
]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use the sandbox-scoped public command in this hint.

This message still tells users to run nemoclaw config rotate-token, but the public grammar in this PR is nemoclaw <name> .... As written, the recovery guidance points to the wrong command form.

Suggested fix
-      "  Use `nemoclaw config rotate-token` for credential changes.",
+      "  Use `nemoclaw <name> config rotate-token` for credential changes.",
📝 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.

Suggested change
configFail([
" Cannot modify the gateway section directly.",
" Use `nemoclaw config rotate-token` for credential changes.",
]);
configFail([
" Cannot modify the gateway section directly.",
" Use `nemoclaw <name> config rotate-token` for credential changes.",
]);
🤖 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/sandbox/config.ts` around lines 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).

@github-actions
Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 25948148546
Target ref: d87bee5f2cb941ad001f184c5f8036985787d943
Workflow ref: main
Requested jobs: shields-config-e2e,sandbox-operations-e2e,diagnostics-e2e
Summary: 3 passed, 0 failed, 0 skipped

Job Result
diagnostics-e2e ✅ success
sandbox-operations-e2e ✅ success
shields-config-e2e ✅ success

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
src/lib/cli/oclif-dispatch.ts (1)

211-213: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Invalid config/shields subcommands 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 the channels case 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 value

Unreachable 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:

  1. The sandbox wasn't a valid action (line 187's condition failed), or
  2. Recovery succeeded and registry.getSandbox(cmd) now returns truthy

In case (1), sandboxActions.includes(requestedSandboxAction) was false, so the user provided a non-sandbox-action. The code at line 227 then checks registry.getSandbox(cmd) again. This path seems intended for when cmd isn'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

📥 Commits

Reviewing files that changed from the base of the PR and between d87bee5 and 4dcc675.

📒 Files selected for processing (5)
  • src/commands/credentials/reset.ts
  • src/lib/cli/oclif-dispatch.test.ts
  • src/lib/cli/oclif-dispatch.ts
  • src/lib/cli/public-dispatch.ts
  • test/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

Comment thread src/lib/cli/oclif-dispatch.test.ts
Comment on lines +111 to +117
case "usageError":
printDispatchUsageError(result, opts.sandboxName);
case "unknownAction":
console.error(` Unknown action: ${result.action}`);
console.error(` Valid actions: ${VALID_SANDBOX_ACTIONS}`);
process.exit(1);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Missing return causes fall-through from usageError to unknownAction case.

The usageError case at line 111 calls printDispatchUsageError which ends with process.exit(1), but TypeScript's control flow analysis doesn't know this. More critically, the code structure suggests a fall-through bug: if printDispatchUsageError ever doesn't exit (e.g., during testing with mocked process.exit), execution falls through to the unknownAction case.

Add an explicit return after the call, or restructure to be explicit about the never-returning behavior.

Proposed fix
     case "usageError":
       printDispatchUsageError(result, opts.sandboxName);
+      return; // printDispatchUsageError calls process.exit, but be explicit
     case "unknownAction":
📝 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.

Suggested change
case "usageError":
printDispatchUsageError(result, opts.sandboxName);
case "unknownAction":
console.error(` Unknown action: ${result.action}`);
console.error(` Valid actions: ${VALID_SANDBOX_ACTIONS}`);
process.exit(1);
}
case "usageError":
printDispatchUsageError(result, opts.sandboxName);
return; // printDispatchUsageError calls process.exit, but be explicit
case "unknownAction":
console.error(` Unknown action: ${result.action}`);
console.error(` Valid actions: ${VALID_SANDBOX_ACTIONS}`);
process.exit(1);
}
🤖 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 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.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions
Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 25949522967
Target ref: 4dcc67576de01e60f117aaad4b01bc4239b5a4b3
Workflow ref: main
Requested jobs: shields-config-e2e,sandbox-operations-e2e,diagnostics-e2e
Summary: 2 passed, 0 failed, 0 skipped

Job Result
diagnostics-e2e ✅ success
sandbox-operations-e2e ⚠️ cancelled
shields-config-e2e ✅ success

cv added 5 commits May 15, 2026 19:02
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>
@github-actions
Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 25949902841
Target ref: f2142f050bb9b82586d999d0a3cf9bba8b14956a
Workflow ref: main
Requested jobs: cloud-e2e,shields-config-e2e,diagnostics-e2e,sandbox-operations-e2e,rebuild-openclaw-e2e
Summary: 5 passed, 0 failed, 0 skipped

Job Result
cloud-e2e ✅ success
diagnostics-e2e ✅ success
rebuild-openclaw-e2e ✅ success
sandbox-operations-e2e ✅ success
shields-config-e2e ✅ success

cv added 3 commits May 15, 2026 19:32
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions
Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 25950804306
Target ref: 2fb62b0828e76bdaf09b26affc7e2f6e0d8ff01a
Workflow ref: main
Requested jobs: cloud-onboard-e2e,sandbox-operations-e2e,openclaw-inference-switch-e2e,diagnostics-e2e,shields-config-e2e,network-policy-e2e
Summary: 6 passed, 0 failed, 0 skipped

Job Result
cloud-onboard-e2e ✅ success
diagnostics-e2e ✅ success
network-policy-e2e ✅ success
openclaw-inference-switch-e2e ✅ success
sandbox-operations-e2e ✅ success
shields-config-e2e ✅ success

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv cv changed the title refactor(cli): tighten oclif routing and help metadata refactor(cli): align public dispatch with oclif conventions May 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v0.0.45 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant