refactor(cli): extract sandbox connect action - #2891
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. |
📝 WalkthroughWalkthroughThe sandbox connection and process recovery logic is extracted from ChangesSandbox Connection & Recovery Extraction
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR extracts and refactors two large, logic-dense new modules (327 and 371 lines respectively) with complex flows spanning SSH execution, gateway health probing, process recovery, polling, and port forwarding. Multiple interdependent files must be reviewed to verify the extraction preserves behavior while wiring new functionality, and the new modules' implementation details (command execution, timeout handling, status detection, marker-based output parsing) require careful scrutiny.
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
## Summary Extract the sandbox skill install command implementation from `src/nemoclaw.ts` into a dedicated action module. This removes `sandboxSkillInstall` from the transitional runtime bridge while preserving the existing oclif wrapper and legacy fallback behavior. ## Stack Navigation - Position: 2 of 60 - Previous PR: [#2886 — refactor(cli): extract sandbox live state helpers](#2886) - Next PR: [#2891 — refactor(cli): extract sandbox connect action](#2891) ## Changes - Added `src/lib/sandbox-skill-install-action.ts` for skill install usage, plugin-shape detection, validation, upload, post-install, and verification flow. - Updated `src/lib/sandbox-runtime-actions.ts` and the legacy skill fallback to call the extracted action. - Removed `sandboxSkillInstall` 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> --------- Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/sandbox-gateway-state-action.ts (1)
35-46: 💤 Low valueConsider exporting
SandboxGatewayStatetype for better API ergonomics.The
SandboxGatewayStatetype is used as a return type for exported functions likeensureLiveSandboxOrExitandgetReconciledSandboxGatewayState, but the type itself is not exported. Consumers needing to type-annotate variables holding these results would benefit from direct access to the type.Proposed fix
-type SandboxGatewayState = { +export type SandboxGatewayState = { state: string;🤖 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-gateway-state-action.ts` around lines 35 - 46, Export the SandboxGatewayState type so consumers can import and use it directly; update its declaration (and optionally SandboxGatewayStateLookup) to be exported, e.g., change "type SandboxGatewayState = { ... }" to an exported type, and ensure any exported functions that return this type (ensureLiveSandboxOrExit, getReconciledSandboxGatewayState) keep their signatures consistent with the exported type for correct external typing.
🤖 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-skill-install-action.ts`:
- Around line 167-174: The captureOpenshell call that retrieves SSH config in
sandbox-skill-install-action.ts is missing a timeout and should pass
OPENSHELL_PROBE_TIMEOUT_MS like other modules; update the call to
captureOpenshell(["sandbox", "ssh-config", sandboxName], { ignoreError: true,
timeout: OPENSHELL_PROBE_TIMEOUT_MS }) so it uses the same probe timeout
constant (OPENSHELL_PROBE_TIMEOUT_MS) as in sandbox-process-recovery-action.ts;
keep the existing sshConfigResult.status check and error handling unchanged.
---
Nitpick comments:
In `@src/lib/sandbox-gateway-state-action.ts`:
- Around line 35-46: Export the SandboxGatewayState type so consumers can import
and use it directly; update its declaration (and optionally
SandboxGatewayStateLookup) to be exported, e.g., change "type
SandboxGatewayState = { ... }" to an exported type, and ensure any exported
functions that return this type (ensureLiveSandboxOrExit,
getReconciledSandboxGatewayState) keep their signatures consistent with the
exported type for correct external typing.
🪄 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: 2d927f63-4df6-4d7b-8c51-70f21f187e3f
📒 Files selected for processing (10)
src/lib/nemoclaw-runtime-bridge.tssrc/lib/sandbox-connect-action.tssrc/lib/sandbox-gateway-state-action.tssrc/lib/sandbox-process-recovery-action.tssrc/lib/sandbox-runtime-actions.tssrc/lib/sandbox-skill-install-action.tssrc/lib/share-command-deps.tssrc/lib/skill-install.tssrc/lib/terminal-style.tssrc/nemoclaw.ts
💤 Files with no reviewable changes (1)
- src/lib/nemoclaw-runtime-bridge.ts
| // 4. Get SSH config | ||
| const sshConfigResult = captureOpenshell(["sandbox", "ssh-config", sandboxName], { | ||
| ignoreError: true, | ||
| }); | ||
| if (sshConfigResult.status !== 0) { | ||
| console.error(" Failed to obtain SSH configuration for the sandbox."); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
Add timeout to SSH config capture for consistency with other modules.
The captureOpenshell call for SSH config is missing a timeout parameter. Other modules like sandbox-process-recovery-action.ts (line 43-46) use OPENSHELL_PROBE_TIMEOUT_MS for the same operation to prevent indefinite hangs.
Proposed fix
+import { OPENSHELL_PROBE_TIMEOUT_MS } from "./openshell-timeouts"; // 4. Get SSH config
- const sshConfigResult = captureOpenshell(["sandbox", "ssh-config", sandboxName], {
- ignoreError: true,
- });
+ const sshConfigResult = captureOpenshell(["sandbox", "ssh-config", sandboxName], {
+ ignoreError: true,
+ timeout: OPENSHELL_PROBE_TIMEOUT_MS,
+ });📝 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.
| // 4. Get SSH config | |
| const sshConfigResult = captureOpenshell(["sandbox", "ssh-config", sandboxName], { | |
| ignoreError: true, | |
| }); | |
| if (sshConfigResult.status !== 0) { | |
| console.error(" Failed to obtain SSH configuration for the sandbox."); | |
| process.exit(1); | |
| } | |
| // 4. Get SSH config | |
| const sshConfigResult = captureOpenshell(["sandbox", "ssh-config", sandboxName], { | |
| ignoreError: true, | |
| timeout: OPENSHELL_PROBE_TIMEOUT_MS, | |
| }); | |
| if (sshConfigResult.status !== 0) { | |
| console.error(" Failed to obtain SSH configuration for the sandbox."); | |
| 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/sandbox-skill-install-action.ts` around lines 167 - 174, The
captureOpenshell call that retrieves SSH config in
sandbox-skill-install-action.ts is missing a timeout and should pass
OPENSHELL_PROBE_TIMEOUT_MS like other modules; update the call to
captureOpenshell(["sandbox", "ssh-config", sandboxName], { ignoreError: true,
timeout: OPENSHELL_PROBE_TIMEOUT_MS }) so it uses the same probe timeout
constant (OPENSHELL_PROBE_TIMEOUT_MS) as in sandbox-process-recovery-action.ts;
keep the existing sshConfigResult.status check and error handling unchanged.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-process-recovery-action.ts`:
- Line 25: The file currently requires the built artifact via
require("../../bin/lib/agent-runtime"), which bypasses CLI type-checking;
replace that require with a typed import from the source module so the module
stays in the src graph—e.g., change the import of agentRuntime to use the source
module (require or ES import from "./agent-runtime" or import agentRuntime from
"./agent-runtime") so the symbol agentRuntime is resolved from
src/lib/agent-runtime.ts instead of the ../../bin build output.
- Around line 51-52: The current code builds a predictable tmpFile in
os.tmpdir() and writes it directly, which permits TOCTOU/symlink attacks; change
the flow to create a private temp directory with fs.mkdtempSync (e.g., const dir
= fs.mkdtempSync(path.join(os.tmpdir(), 'nemoclaw-ssh-'))), then join a config
filename inside that directory (e.g., path.join(dir, 'config')) and write the
file there using an exclusive write flag (fs.writeFileSync(file, data, { mode:
0o600, flag: 'wx' })), and finally clean up the temp directory when done; apply
the same change to the other block that currently creates tmpFile (the code
around lines 79–84) so both uses are protected.
🪄 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: ed16de63-dbec-49f3-86b6-30c44e2c88d5
📒 Files selected for processing (4)
src/lib/sandbox-connect-action.tssrc/lib/sandbox-process-recovery-action.tssrc/lib/sandbox-runtime-actions.tssrc/nemoclaw.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/sandbox-connect-action.ts
- src/nemoclaw.ts
| import { G, R } from "./terminal-style"; | ||
| import { sleepSeconds } from "./wait"; | ||
|
|
||
| const agentRuntime = require("../../bin/lib/agent-runtime"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
printf '== agent-runtime files ==\n'
fd -i 'agent-runtime.*' src bin
printf '\n== references to agent-runtime from src/lib ==\n'
rg -n -C2 'agent-runtime' src/lib
printf '\n== check whether src/lib/agent-runtime.ts already depends on sandbox-process-recovery-action ==\n'
rg -n -C2 'sandbox-process-recovery-action' src/lib/agent-runtime.tsRepository: NVIDIA/NemoClaw
Length of output: 1830
Use typed import from source module instead of built artifact.
Line 25 requires the built artifact ../../bin/lib/agent-runtime, which bypasses CLI type-checking required by tsconfig.cli.json and violates the src/**/*.ts guideline. No circular dependency exists—switch to require("./agent-runtime") to keep the module within the typed src/lib/ graph.
🤖 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-process-recovery-action.ts` at line 25, The file currently
requires the built artifact via require("../../bin/lib/agent-runtime"), which
bypasses CLI type-checking; replace that require with a typed import from the
source module so the module stays in the src graph—e.g., change the import of
agentRuntime to use the source module (require or ES import from
"./agent-runtime" or import agentRuntime from "./agent-runtime") so the symbol
agentRuntime is resolved from src/lib/agent-runtime.ts instead of the ../../bin
build output.
| const tmpFile = path.join(os.tmpdir(), `nemoclaw-ssh-${process.pid}-${Date.now()}.conf`); | ||
| fs.writeFileSync(tmpFile, sshConfigResult.output, { mode: 0o600 }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/lib/sandbox-process-recovery-action.ts | head -100Repository: NVIDIA/NemoClaw
Length of output: 3789
🏁 Script executed:
# Check for similar temp file patterns in the codebase
rg "os\.tmpdir\(\)|writeFileSync|mkdtempSync" -t ts -t js --context 2 | head -80Repository: NVIDIA/NemoClaw
Length of output: 5303
🏁 Script executed:
# Also check if mkdtempSync is already used elsewhere for comparison
rg "mkdtempSync" -t ts -t js -A 3Repository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
# Verify that flag: "wx" is a valid option for writeFileSync in Node.js
node -e "const fs = require('fs'); const tmp = fs.mkdtempSync('/tmp/test-'); try { fs.writeFileSync(require('path').join(tmp, 'test.txt'), 'content', { flag: 'wx' }); console.log('SUCCESS: wx flag works'); } catch (e) { console.log('ERROR: ' + e.message); }"Repository: NVIDIA/NemoClaw
Length of output: 82
Use a secure temporary directory for SSH config file.
Line 51–52 creates a predictable temp filename in os.tmpdir() and writes it without exclusive creation, leaving a symlink/TOCTOU window on multi-user hosts. SSH config files contain sensitive authentication data. Create a private temp directory using mkdtempSync instead.
Suggested fix
- const tmpFile = path.join(os.tmpdir(), `nemoclaw-ssh-${process.pid}-${Date.now()}.conf`);
- fs.writeFileSync(tmpFile, sshConfigResult.output, { mode: 0o600 });
+ const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-ssh-"));
+ const tmpFile = path.join(tmpDir, "config");
+ fs.writeFileSync(tmpFile, sshConfigResult.output, { mode: 0o600 });
try {
const result = spawnSync(
@@
} finally {
try {
- fs.unlinkSync(tmpFile);
+ fs.rmSync(tmpDir, { recursive: true, force: true });
} catch {
/* ignore */
}
}Also applies to: 79–84
🤖 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-process-recovery-action.ts` around lines 51 - 52, The current
code builds a predictable tmpFile in os.tmpdir() and writes it directly, which
permits TOCTOU/symlink attacks; change the flow to create a private temp
directory with fs.mkdtempSync (e.g., const dir =
fs.mkdtempSync(path.join(os.tmpdir(), 'nemoclaw-ssh-'))), then join a config
filename inside that directory (e.g., path.join(dir, 'config')) and write the
file there using an exclusive write flag (fs.writeFileSync(file, data, { mode:
0o600, flag: 'wx' })), and finally clean up the temp directory when done; apply
the same change to the other block that currently creates tmpFile (the code
around lines 79–84) so both uses are protected.
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
LGTM — clean mechanical extraction of sandboxConnect and the sandbox process-recovery helpers out of src/nemoclaw.ts. Public CLI surface (flags, help text, env knobs, error messages, exit codes) preserved verbatim; old code paths fully removed from src/nemoclaw.ts and nemoclaw-runtime-bridge.ts; full self-hosted e2e suite green on this hash.
Nit (non-blocking): parseForwardList is still in the destructure at src/nemoclaw.ts:88-92 but its only consumer (isSandboxForwardHealthy) moved to sandbox-process-recovery-action.ts. Can be dropped here or in a follow-up.
## Summary Extract the sandbox status implementation from `src/nemoclaw.ts` into a dedicated action module. This removes `sandboxStatus` from the transitional runtime bridge while preserving the existing status output, gateway reconciliation, process health, and NIM reporting behavior. ## Stack Navigation - Position: 4 of 60 - Previous PR: [#2891 — refactor(cli): extract sandbox connect action](#2891) - Next PR: [#2893 — refactor(cli): extract sandbox doctor action](#2893) ## Changes - Added `src/lib/sandbox-status-action.ts` for sandbox status rendering, gateway lookup handling, local inference health, active session reporting, process health, and NIM status. - Updated `src/lib/sandbox-runtime-actions.ts` to call the extracted status action. - Removed `sandboxStatus` 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 * **Refactoring** * Reorganized sandbox status command implementation for improved maintainability. * **Improvements** * Enhanced sandbox status reporting with comprehensive diagnostics including model information, provider details, inference status, GPU information, and policy checks. * Improved sandbox health verification with better error detection and guidance when state issues are encountered. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
3 participants
Summary
Extract sandbox connect and sandbox process recovery logic from
src/nemoclaw.tsinto dedicated CLI action modules. This removessandboxConnectfrom the transitional runtime bridge while keeping existing connect, probe, status health, and rebuild post-upgrade behavior intact.Stack Navigation
Changes
src/lib/sandbox-connect-action.tsfor connect help/argument parsing, probe-only checks, inference route swapping, readiness polling, and OpenShell connection handoff.src/lib/sandbox-process-recovery-action.tsfor sandbox SSH/exec helpers, gateway process probes, and automatic gateway process recovery.src/lib/sandbox-runtime-actions.tsto call the extracted connect action instead ofNemoClawRuntimeBridge.src/nemoclaw.tsstatus/rebuild paths to import the extracted process helpers.sandboxConnectfromsrc/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
New Features
--probe-onlyflag for sandbox connection to check gateway health without full connection.NEMOCLAW_CONNECT_TIMEOUT.Improvements