refactor(cli): extract sandbox skill install action - #2887
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. |
📝 WalkthroughWalkthroughSkill installation functionality is refactored from the ChangesSkill Installation Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
…r/oclif-extract-skill-install-action
## Summary Extract shared sandbox gateway-state and liveness helpers from `src/nemoclaw.ts` into reusable CLI library modules. This reduces the legacy entrypoint surface before moving connect, status, skill install, and lifecycle actions out of the runtime bridge. ## Stack Navigation - Position: 1 of 60 - Previous PR: base branch `main` - Next PR: [#2887 — refactor(cli): extract sandbox skill install action](#2887) ## Changes - Added `src/lib/sandbox-gateway-state-action.ts` for sandbox liveness, gateway reconciliation, live-policy merging, and lifecycle guidance helpers. - Added `src/lib/terminal-style.ts` for shared CLI color constants. - Updated `src/nemoclaw.ts` to import the extracted sandbox gateway-state helpers instead of defining them inline. - Updated `src/lib/share-command-deps.ts` to depend on extracted helpers instead of requiring `../nemoclaw`. - Removed stale `sandboxLogs` from `NemoClawRuntimeBridge`. ## 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 - [ ] 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** * Enhanced sandbox diagnostics with live-policy display and guided recovery steps. * **Bug Fixes** * More accurate sandbox presence/status detection, identity-drift handling, and gateway reconnection. * **Chores** * Refactored sandbox/gateway wiring and share-command startup for reliability. * Unified terminal color handling. * Removed sandbox log streaming (log-following is no longer available). <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/nemoclaw.ts (1)
2534-2540: Run the recommended sandbox recovery E2Es before merge.Since this touches
src/nemoclaw.tsdispatch, run the two selective nightly jobs to guard against unintended regressions in sandbox recovery flows.As per coding guidelines, "E2E test recommendation:
sandbox-survival-e2e— gateway restart recovery;sandbox-operations-e2e— process recovery after gateway kill."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 2534 - 2540, This touches the sandbox dispatch branch in src/nemoclaw.ts (case "skill" calling installSandboxSkill(sandboxName, actionArgs)), so before merging run the two recommended sandbox recovery E2E jobs: execute the nightly "sandbox-survival-e2e" (gateway restart recovery) and "sandbox-operations-e2e" (process recovery after gateway kill) against the current branch to verify no regressions in sandbox recovery flows; if either test fails, capture failures, fix the dispatch/installation flow around installSandboxSkill and re-run until both pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/nemoclaw.ts`:
- Around line 2534-2540: This touches the sandbox dispatch branch in
src/nemoclaw.ts (case "skill" calling installSandboxSkill(sandboxName,
actionArgs)), so before merging run the two recommended sandbox recovery E2E
jobs: execute the nightly "sandbox-survival-e2e" (gateway restart recovery) and
"sandbox-operations-e2e" (process recovery after gateway kill) against the
current branch to verify no regressions in sandbox recovery flows; if either
test fails, capture failures, fix the dispatch/installation flow around
installSandboxSkill and re-run until both pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 286b7e2a-2be7-4762-a46f-19ec2f4efc71
📒 Files selected for processing (4)
src/lib/nemoclaw-runtime-bridge.tssrc/lib/sandbox-runtime-actions.tssrc/lib/sandbox-skill-install-action.tssrc/nemoclaw.ts
💤 Files with no reviewable changes (1)
- src/lib/nemoclaw-runtime-bridge.ts
prekshivyas
left a comment
There was a problem hiding this comment.
LGTM. Clean lift-and-shift — verified the four extracted symbols are byte-identical to the on-main originals (only export keywords, the sandboxSkillInstall → installSandboxSkill rename, and an inconsequential template-literal-to-string tweak). Argument shape preserved end-to-end through both the oclif wrapper and the legacy case "skill" dispatch. No zombies — block edits to migrated legacy paths check confirms.
Scope is tight: the work commit touches exactly the 4 expected files, no test churn, no Dockerfile drift. Notably cleaner than #2886.
One non-blocking note: the new action consumes D, G, R, YW from terminal-style.ts, making it the second module to depend on the extracted color constants. That's the start of the migration #2886 deferred — worth mentioning in the description, but not blocking.
Auto-merge will land this once build-sandbox-images (x86) finishes — arm64 and everything else are already green.
## Summary Extract sandbox connect and sandbox process recovery logic from `src/nemoclaw.ts` into dedicated CLI action modules. This removes `sandboxConnect` from the transitional runtime bridge while keeping existing connect, probe, status health, and rebuild post-upgrade behavior intact. ## Stack Navigation - Position: 3 of 60 - Previous PR: [#2887 — refactor(cli): extract sandbox skill install action](#2887) - Next PR: [#2892 — refactor(cli): extract sandbox status action](#2892) ## Changes - Added `src/lib/sandbox-connect-action.ts` for connect help/argument parsing, probe-only checks, inference route swapping, readiness polling, and OpenShell connection handoff. - Added `src/lib/sandbox-process-recovery-action.ts` for sandbox SSH/exec helpers, gateway process probes, and automatic gateway process recovery. - Updated `src/lib/sandbox-runtime-actions.ts` to call the extracted connect action instead of `NemoClawRuntimeBridge`. - Updated `src/nemoclaw.ts` status/rebuild paths to import the extracted process helpers. - Removed `sandboxConnect` from `src/nemoclaw.ts` and `NemoClawRuntimeBridge`. ## 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 - [ ] 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 `--probe-only` flag for sandbox connection to check gateway health without full connection. * Enhanced sandbox connection with automatic gateway recovery and configurable timeout via `NEMOCLAW_CONNECT_TIMEOUT`. * Automatic dashboard port forwarding verification during sandbox connection. * **Improvements** * Better error detection and recovery for failed sandboxes during connection. * Clearer status feedback when sandbox connection is ready. <!-- 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>
Summary
Extract the sandbox skill install command implementation from
src/nemoclaw.tsinto a dedicated action module. This removessandboxSkillInstallfrom the transitional runtime bridge while preserving the existing oclif wrapper and legacy fallback behavior.Stack Navigation
Changes
src/lib/sandbox-skill-install-action.tsfor skill install usage, plugin-shape detection, validation, upload, post-install, and verification flow.src/lib/sandbox-runtime-actions.tsand the legacy skill fallback to call the extracted action.sandboxSkillInstallfromsrc/nemoclaw.tsandNemoClawRuntimeBridge.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