refactor(cli): derive legacy sandbox dispatch from metadata#3101
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. |
|
Caution Review failedPull request was closed or merged during review 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)
📝 WalkthroughWalkthroughAdds a legacy token-based sandbox dispatch resolver and global dispatch resolution, updates help/usage metadata for sandbox snapshot commands, integrates the legacy resolver into the main CLI dispatch flow, adjusts public help rendering for a sandbox subcommand, and adds tests and a metadata discovery helper for command lookup. ChangesLegacy Sandbox Dispatch
Sequence DiagramsequenceDiagram
participant CLI as CLI Input
participant Legacy as Legacy Resolver
participant Registry as Command Registry
participant Oclif as Oclif Dispatcher
participant Help as Help Renderer
CLI->>Legacy: sandbox <action> <args>
Legacy->>Registry: tokenize usages & match legacy routes
alt matched to help
Registry->>Help: provide publicUsage + commandId
Help-->>CLI: { kind: "help", publicUsage, commandId }
else mapped to oclif command
Registry->>Oclif: commandId + mapped args
Oclif-->>CLI: { kind: "oclif", commandId, args }
else fallback / unknown
Legacy-->>CLI: { kind: "usageError" | "unknownAction" }
end
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/cli/oclif-dispatch.test.ts (1)
72-77: 💤 Low valueMinor: Extra blank line.
There's an extra blank line at line 73 before
kind: "oclif". This doesn't affect functionality but slightly breaks the formatting pattern of other test assertions.📝 Suggested fix
expect(resolveLegacySandboxDispatch("alpha", "skill", ["bogus"])).toEqual({ - kind: "oclif", commandId: "sandbox:skill", args: ["alpha", "bogus"], });🤖 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.test.ts` around lines 72 - 77, Remove the stray blank line in the test assertion inside resolveLegacySandboxDispatch's test (the expect(...) call) so the object literal entries are consistently formatted; locate the expect(resolveLegacySandboxDispatch("alpha", "skill", ["bogus"])) assertion and delete the empty line preceding the "kind: \"oclif\"" property so the object keys appear on consecutive lines like the other tests.
🤖 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-dispatch.test.ts`:
- Around line 72-77: Remove the stray blank line in the test assertion inside
resolveLegacySandboxDispatch's test (the expect(...) call) so the object literal
entries are consistently formatted; locate the
expect(resolveLegacySandboxDispatch("alpha", "skill", ["bogus"])) assertion and
delete the empty line preceding the "kind: \"oclif\"" property so the object
keys appear on consecutive lines like the other tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6bda775c-19bd-4549-b541-67ac91f141fc
📒 Files selected for processing (6)
src/commands/sandbox/snapshot/create.tssrc/commands/sandbox/snapshot/restore.tssrc/lib/cli/oclif-dispatch.test.tssrc/lib/cli/oclif-dispatch.tssrc/nemoclaw.tstest/image-cleanup.test.ts
Automated PR review summaryReviewed PR #3101: refactor(cli): derive legacy sandbox dispatch from metadata Recommendation
Installation and setup findings
What was validated
Failing tests and unresolved impact
Passing tests and why they matteredPassing test 1: Legacy snapshot create help preserves public metadata-derived usage
Passing test 2: Legacy snapshot restore help and bad nested token do not misroute
Passing test 3: Real legacy sandbox dispatch still reaches live command execution
Bottom line
|
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/cli/oclif-dispatch.test.ts (1)
35-46: ⚡ Quick winAdd
-halias assertions for help-path parity.These tests currently lock
--helpbehavior only; adding equivalent-hcases would fully cover the documented dual-flag help handling path.Also applies to: 67-71
🤖 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.test.ts` around lines 35 - 46, Add assertions to the existing tests in src/lib/cli/oclif-dispatch.test.ts that verify the short help flag `-h` yields the same dispatch results as `--help`; update the two test blocks that call resolveLegacySandboxDispatch("alpha", "status", ["--help"]) and resolveLegacySandboxDispatch("alpha", "logs", ["--help"]) (and the similar block around the later lines 67-71) to include equivalent expectations for resolveLegacySandboxDispatch(..., ["-h"]) returning identical objects (same kind, commandId, and publicUsage) to ensure help-path parity with the long flag.
🤖 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-dispatch.test.ts`:
- Around line 35-46: Add assertions to the existing tests in
src/lib/cli/oclif-dispatch.test.ts that verify the short help flag `-h` yields
the same dispatch results as `--help`; update the two test blocks that call
resolveLegacySandboxDispatch("alpha", "status", ["--help"]) and
resolveLegacySandboxDispatch("alpha", "logs", ["--help"]) (and the similar block
around the later lines 67-71) to include equivalent expectations for
resolveLegacySandboxDispatch(..., ["-h"]) returning identical objects (same
kind, commandId, and publicUsage) to ensure help-path parity with the long flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d3a282c5-cd33-40e6-baa2-4acf90152435
📒 Files selected for processing (1)
src/lib/cli/oclif-dispatch.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
heads-up — verified locally on the built PR head:
undocumented path so not a blocker, but the error is misleading. one-line fix is restoring the |
prekshivyas
left a comment
There was a problem hiding this comment.
Solid metadata-derived dispatch refactor. Removes the two hand-maintained route tables (FLAT_SANDBOX_ROUTES for 10 flat actions and NESTED_SANDBOX_ROUTES for 6 parent groups, ~200 lines of static config) in favor of a legacyRoutes() helper that walks sandboxCommands() (which comes from #3098's oclif-derived registry), extracts tokens from each command's usage (stripping nemoclaw <name> prefix and placeholders), and greedy-matches the longest token prefix first.
Special-cases preserved deliberately: connect falls through to oclif, channels with no args → channels:list, skill install --help → sandbox:skill parent, share --help short-form via a commandId === \"sandbox:share\" carve-out in public-oclif-help.ts, parent-action fallback for share/skill/snapshot unknown subcommands, and config/shields usage-error tables.
HelpDispatch shape changes from usage to publicUsage (full <name> ... form). Caller runDispatchResult updated symmetrically — identical render path. Backward-compat alias export const resolveSandboxOclifDispatch = resolveLegacySandboxDispatch retained for external import sites.
Test coverage strengthened: hyphenated→colon ID translation (policy-add → sandbox:policy:add, gateway-token → sandbox:gateway:token), share parent help shape, channels unknown-subcommand error path, config/shields usage-error tables.
Two minor undeclared behavior shifts (non-blocking, both arguably improvements):
connect --helppreviously returned{kind: \"help\", usage: \"connect\"}rendering as a one-line usage. Nowconnectalways forwards all args (including--help) to oclif, so users see oclif's full description+flags+examples block. UX win.- Parent-action
--help(e.g.skill --help,snapshot --help) previously passed--helpthrough to oclif as args; new code strips--helpbefore forwarding (hasHelpFlag(actionArgs) ? [] : actionArgs). Both render help in practice — oclif on a topic parent with no args shows topic help. Functionally equivalent.
The snapshot display-flag normalization ([--name <label>] → [--name <name>], [v<N>|...|timestamp] [--to <dst>] (omit ...) → [selector] [--to <dst>]) is declared in the PR description; a minor consistency side effect is that root help (nemoclaw --help) now shows the shorter strings too, since display metadata flows to both legacy public help and root help.
CI: pr.yaml lightweight checks pass (commit-lint, dco, layer-boundary, check-hash, changes). checks / macos-e2e / wsl-e2e / build-sandbox-images / current self-hosted run still in progress at approval time.
Summary
Derives legacy name-first sandbox dispatch from oclif command display metadata instead of maintaining a parallel sandbox route table. This keeps the legacy
nemoclaw <name> ...UX while moving sandbox command routing closer to the oclif-discovered command surface.Changes
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
Documentation
Refactor
Tests