refactor(cli): tighten policy and channel parser validation - #2907
Merged
Conversation
|
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
📝 WalkthroughWalkthroughThis PR removes the legacy ChangesRemove Legacy Policy Command & Enforce Required Arguments
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 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 |
This was referenced May 4, 2026
cv
added a commit
that referenced
this pull request
May 4, 2026
## Summary Improve the oclif metadata and parser-owned validation for read-only sandbox diagnostic commands. This keeps public sandbox-scoped help output stable while making `doctor` and `config get` validation stricter in the command adapter. ## Stack Navigation - Position: 14 of 60 - Previous PR: [#2905 — refactor(cli): validate logs flags with oclif](#2905) - Next PR: [#2907 — refactor(cli): tighten policy and channel parser validation](#2907) ## Changes - Added examples for `connect`, sandbox-scoped `status`, `doctor`, `config get`, `policy-list`, and `channels list` adapters. - Made `sandbox:doctor` a strict oclif command with a required sandbox arg and `--json` flag. - Moved `config get --format` validation into oclif with `json|yaml` options and removed the adapter-level manual format check. - Preserved public `doctor --help` output through legacy dispatch and expanded diagnostics validation coverage. - Updated the hidden command registry metadata for `config get` flags. ## Type of Change - [x] 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 - [x] `npx prek run --all-files` passes - [x] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] 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](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added command usage examples to help documentation for configuration, status, policy, and channels commands. * Enhanced doctor command with explicit `--json` flag support. * **Improvements** * Updated configuration command help text to clearly advertise available options (`--key` and `--format json|yaml`). * Restricted `--format` flag to `json` and `yaml` values with stricter validation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv
marked this pull request as ready for review
May 4, 2026 22:43
cv
enabled auto-merge (squash)
May 4, 2026 22:43
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/channels-mutate-cli-commands.test.ts (1)
52-64: ⚡ Quick winConsider testing all four commands for missing channel validation.
The new test correctly verifies that
ChannelsAddCommandrejects when the requiredchannelargument is missing. However, per the PR objectives, all four channel mutation commands (add/remove/start/stop) should enforce this requirement. Consider adding similar test cases forChannelsRemoveCommand,ChannelsStartCommand, andChannelsStopCommandto ensure comprehensive coverage of the new validation behavior.📋 Suggested test additions
it("requires a channel before dispatch", async () => { const runtime = { sandboxChannelsAdd: vi.fn().mockResolvedValue(undefined), sandboxChannelsRemove: vi.fn().mockResolvedValue(undefined), sandboxChannelsStart: vi.fn().mockResolvedValue(undefined), sandboxChannelsStop: vi.fn().mockResolvedValue(undefined), }; setChannelsRuntimeBridgeFactoryForTest(() => runtime); await expect(ChannelsAddCommand.run(["alpha"], rootDir)).rejects.toThrow(/channel/i); + await expect(ChannelsRemoveCommand.run(["alpha"], rootDir)).rejects.toThrow(/channel/i); + await expect(ChannelsStartCommand.run(["alpha"], rootDir)).rejects.toThrow(/channel/i); + await expect(ChannelsStopCommand.run(["alpha"], rootDir)).rejects.toThrow(/channel/i); expect(runtime.sandboxChannelsAdd).not.toHaveBeenCalled(); + expect(runtime.sandboxChannelsRemove).not.toHaveBeenCalled(); + expect(runtime.sandboxChannelsStart).not.toHaveBeenCalled(); + expect(runtime.sandboxChannelsStop).not.toHaveBeenCalled(); });🤖 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/channels-mutate-cli-commands.test.ts` around lines 52 - 64, Add equivalent tests for ChannelsRemoveCommand, ChannelsStartCommand, and ChannelsStopCommand mirroring the existing ChannelsAddCommand test: reuse setChannelsRuntimeBridgeFactoryForTest to inject a runtime with sandboxChannelsRemove/sandboxChannelsStart/sandboxChannelsStop mocked, call <Command>.run([] or missing channel arg, rootDir) and assert it rejects with a /channel/i error and that the respective runtime method (sandboxChannelsRemove, sandboxChannelsStart, sandboxChannelsStop) was not called; follow the same pattern used for ChannelsAddCommand to ensure consistent validation coverage across all four commands.
🤖 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/command-registry.ts`:
- Around line 235-258: The command metadata for the four channel commands
currently puts the required positional <channel> into flags, so
canonicalUsageList() omits it; update each entry for "nemoclaw <name> channels
add", "nemoclaw <name> channels remove", "nemoclaw <name> channels stop", and
"nemoclaw <name> channels start" to include "<channel>" in the usage string
(e.g., "nemoclaw <name> channels add <channel>") and leave flags as
"[--dry-run]" only, ensuring canonicalUsageList() will serialize the required
positional correctly.
---
Nitpick comments:
In `@src/lib/channels-mutate-cli-commands.test.ts`:
- Around line 52-64: Add equivalent tests for ChannelsRemoveCommand,
ChannelsStartCommand, and ChannelsStopCommand mirroring the existing
ChannelsAddCommand test: reuse setChannelsRuntimeBridgeFactoryForTest to inject
a runtime with sandboxChannelsRemove/sandboxChannelsStart/sandboxChannelsStop
mocked, call <Command>.run([] or missing channel arg, rootDir) and assert it
rejects with a /channel/i error and that the respective runtime method
(sandboxChannelsRemove, sandboxChannelsStart, sandboxChannelsStop) was not
called; follow the same pattern used for ChannelsAddCommand to ensure consistent
validation coverage across all four commands.
🪄 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: a1dffe3f-9354-440e-a52d-a2c67f271730
📒 Files selected for processing (10)
src/lib/channels-mutate-cli-commands.test.tssrc/lib/channels-mutate-cli-commands.tssrc/lib/command-registry.tssrc/lib/legacy-oclif-dispatch.test.tssrc/lib/legacy-oclif-dispatch.tssrc/lib/oclif-commands.tssrc/lib/policy-mutate-cli-commands.test.tssrc/lib/policy-mutate-cli-commands.tstest/cli.test.tstest/policies.test.ts
💤 Files with no reviewable changes (1)
- src/lib/legacy-oclif-dispatch.ts
Comment on lines
235
to
+258
| usage: "nemoclaw <name> channels add", | ||
| description: "Save credentials and rebuild", | ||
| flags: "<channel> [--dry-run]", | ||
| group: "Messaging Channels", | ||
| scope: "sandbox", | ||
| }, | ||
| { | ||
| usage: "nemoclaw <name> channels remove", | ||
| description: "Clear credentials and rebuild", | ||
| flags: "<channel> [--dry-run]", | ||
| group: "Messaging Channels", | ||
| scope: "sandbox", | ||
| }, | ||
| { | ||
| usage: "nemoclaw <name> channels stop", | ||
| description: "Disable channel (keeps credentials)", | ||
| flags: "<channel> [--dry-run]", | ||
| group: "Messaging Channels", | ||
| scope: "sandbox", | ||
| }, | ||
| { | ||
| usage: "nemoclaw <name> channels start", | ||
| description: "Re-enable a previously stopped channel", | ||
| flags: "<channel> [--dry-run]", |
Contributor
There was a problem hiding this comment.
Move required <channel> into usage.
canonicalUsageList() only serializes usage, so these entries still advertise channels add/remove/start/stop as if they had no required positional argument. Put <channel> in usage and leave flags for [--dry-run].
✏️ Suggested metadata update
{
- usage: "nemoclaw <name> channels add",
+ usage: "nemoclaw <name> channels add <channel>",
description: "Save credentials and rebuild",
- flags: "<channel> [--dry-run]",
+ flags: "[--dry-run]",
group: "Messaging Channels",
scope: "sandbox",
},
{
- usage: "nemoclaw <name> channels remove",
+ usage: "nemoclaw <name> channels remove <channel>",
description: "Clear credentials and rebuild",
- flags: "<channel> [--dry-run]",
+ flags: "[--dry-run]",
group: "Messaging Channels",
scope: "sandbox",
},
{
- usage: "nemoclaw <name> channels stop",
+ usage: "nemoclaw <name> channels stop <channel>",
description: "Disable channel (keeps credentials)",
- flags: "<channel> [--dry-run]",
+ flags: "[--dry-run]",
group: "Messaging Channels",
scope: "sandbox",
},
{
- usage: "nemoclaw <name> channels start",
+ usage: "nemoclaw <name> channels start <channel>",
description: "Re-enable a previously stopped channel",
- flags: "<channel> [--dry-run]",
+ flags: "[--dry-run]",
group: "Messaging Channels",
scope: "sandbox",
},🤖 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/command-registry.ts` around lines 235 - 258, The command metadata for
the four channel commands currently puts the required positional <channel> into
flags, so canonicalUsageList() omits it; update each entry for "nemoclaw <name>
channels add", "nemoclaw <name> channels remove", "nemoclaw <name> channels
stop", and "nemoclaw <name> channels start" to include "<channel>" in the usage
string (e.g., "nemoclaw <name> channels add <channel>") and leave flags as
"[--dry-run]" only, ensuring canonicalUsageList() will serialize the required
positional correctly.
prekshivyas
approved these changes
May 4, 2026
prekshivyas
left a comment
prekshivyas
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. 10 files / +64 / -32 — tightens policy and channel parser-owned validation and removes the temporary PolicyAddRawCommand scaffolding from #2899.
Three intentional public-surface tightenings, all declared and tested:
channelArgflips torequired: true—<name> channels add(no channel) now rejected at parse time.PolicyAddRawCommandremoved —<name> policy-add --from-file(no path) now caught by oclif's strict parser instead of the hidden raw adapter. Slight error-wording change (oclif's--from-file ... value/argument/pathvs the old hand-rolled"--from-file requires a path argument"); same non-zero exit.test/policies.test.ts:1499-1500relaxes the regex accordingly.hasMissingFlagValuehelper and the missing-value-routing branch inresolveSandboxOclifDispatchare gone — coherent end-to-end cleanup.
Tests: +14/+14/+13 across channels-mutate-cli-commands.test.ts, policy-mutate-cli-commands.test.ts, test/cli.test.ts. The two-line legacy-oclif-dispatch.test.ts adjustment correctly flips the expected commandId from sandbox:policy-add:raw → sandbox:policy-add.
CI: pr.yaml fully green including CodeRabbit; pr-self-hosted builds + wsl-e2e still in flight at review time. src/nemoclaw.ts untouched — cumulative orphan debt unchanged.
cv
added a commit
that referenced
this pull request
May 5, 2026
## Summary Improve the oclif shape for sandbox snapshot commands by adding examples and making the parent command a strict adapter. This keeps public snapshot help stable while allowing unknown snapshot subcommands to fail before reaching the snapshot action. ## Stack Navigation - Position: 16 of 60 - Previous PR: [#2907 — refactor(cli): tighten policy and channel parser validation](#2907) - Next PR: [#2909 — refactor(cli): require skill install path in oclif](#2909) ## Changes - Added examples for the snapshot parent plus `create`, `list`, and `restore` subcommands. - Made the parent `sandbox:snapshot` command strict and sandbox-arg aware. - Routed `nemoclaw <name> snapshot --help` through the parent adapter while preserving public usage text. - Added adapter and CLI coverage for parent usage and unknown snapshot subcommands. ## Type of Change - [x] 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 - [x] `npx prek run --all-files` passes - [x] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] 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](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added usage examples and clearer help documentation for snapshot commands. * **Bug Fixes** * Help flag now shows snapshot usage instead of being treated as an argument. * Invalid snapshot subcommands are rejected earlier (extra/unknown args now error). * Installer: tightened non-interactive check to fail fast when stdin is not a TTY. * **Tests** * Added tests verifying snapshot help display and invalid-subcommand error handling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com>
latenighthackathon
added a commit
to latenighthackathon/NemoClaw
that referenced
this pull request
May 5, 2026
…aging (NVIDIA#1691) When a sandbox is created without enabling Telegram (or Discord, or Slack) during `nemoclaw onboard`, applying the matching policy preset via `nemoclaw <name> policy-add` only opens network egress to the channel API. The bot token, channel configuration, and in-sandbox bridge are wired up at onboard time, so users who apply the preset after onboarding without having enabled the channel can reasonably believe they have enabled messaging when only the firewall has been widened. Add `getMessagingPresetWarning()` in `src/lib/policies.ts` and surface it in `addSandboxPolicy()` (now in `src/lib/policy-channel-actions.ts` after the recent CLI dispatch refactor) before the apply confirmation so users see, for example, that the `telegram` preset alone does not enable Telegram bots and that re-running `nemoclaw onboard` with Telegram selected is the path to actually enabling the channel. This is a rebase of an earlier branch onto current main: - Hook moved from the legacy `src/nemoclaw.ts` dispatcher to the new `src/lib/policy-channel-actions.ts:addSandboxPolicy` after NVIDIA#2899 / NVIDIA#2901 / NVIDIA#2907 extracted dispatch. - `getMessagingPresetWarning` got an explicit TS signature (`presetName: string): string | null`) to match the rest of `src/lib/policies.ts`. - Replaced the em dash in the warning message with a period for consistency with project style. Originally three commits (warning logic + ordering assertion + tmpDir cleanup) on the prior branch; consolidated here because the rebase needed the dispatcher hook ported to a new file. Closes NVIDIA#1691 Re-ran `npx vitest run test/policies.test.ts` after rebase: 120/120 pass. Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
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
3 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
Tighten oclif-owned validation and help metadata for policy and messaging channel mutation commands. This moves missing custom policy path and channel-name validation into strict command parsing before action dispatch.
Stack Navigation
Changes
policy-add,policy-remove, and channel add/remove/start/stop commands.policy-addadapter so missing--from-fileand--from-dirvalues are handled by oclif.<channel>arg before dispatch.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
--from-fileflag includes a path value.Chores