refactor(cli): extract upgrade sandboxes action - #2897
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. |
|
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 (2)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughThis PR refactors the CLI sandbox runtime bridge by extracting action implementations from the centralized ChangesSandbox Action Module Extraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
## Summary Extract sandbox rebuild orchestration from `src/nemoclaw.ts` into a dedicated action module. This removes `sandboxRebuild` from the transitional runtime bridge while preserving rebuild preflight, backup, recreate, restore, policy preset restore, and post-upgrade checks. ## Stack Navigation - Position: 7 of 60 - Previous PR: [#2895 — refactor(cli): extract sandbox destroy action](#2895) - Next PR: [#2897 — refactor(cli): extract upgrade sandboxes action](#2897) ## Changes - Added `src/lib/sandbox-rebuild-action.ts` for rebuild confirmation, credential preflight, backup, delete/recreate, restore, policy preset replay, and post-restore checks. - Updated `src/lib/sandbox-runtime-actions.ts` to call the extracted rebuild action. - Updated `upgrade-sandboxes` to call the extracted rebuild action directly. - Removed `sandboxRebuild` 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** * Safer, user-facing sandbox rebuild flow with interactive confirmations, SSH-session warnings, credential preflight, workspace backup/restore, and clear completion/recovery messages. * **Refactor** * Moved sandbox rebuild responsibilities to a dedicated action; runtime bridge no longer exposes a direct rebuild method. <!-- 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>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/lib/sandbox-doctor-action.ts (1)
429-430: 🏗️ Heavy liftBreak
runSandboxDoctorinto smaller diagnostic phases.Line 429 is already suppressing complexity on a brand-new command entrypoint. Please extract the host, gateway, sandbox, inference, and local-service sections into helpers so
runSandboxDoctorstays orchestration-only. As per coding guidelines, "Keep function complexity low; existing complexity hotspots are tracked separately".🤖 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-doctor-action.ts` around lines 429 - 430, The runSandboxDoctor function is doing too much; extract each diagnostic section into small helper functions so the top-level function becomes orchestration-only. Create helpers like diagnoseHost(), diagnoseGateway(), diagnoseSandbox(), diagnoseInference(), and diagnoseLocalService() and move the corresponding logic (host checks, gateway checks, sandbox checks, inference checks, local service checks) from runSandboxDoctor into those helpers; have runSandboxDoctor simply call these helpers in sequence, forward args/state as needed, and preserve existing return/error behavior and logging (refer to runSandboxDoctor and any local variables it uses to pass state into the new helpers).src/lib/sandbox-rebuild-action.ts (1)
35-506: 🏗️ Heavy liftSplit
rebuildSandbox()into smaller phases before this grows further.This extracted action still packs confirmation, credential preflight, backup/delete, recreate rollback handling, restore, preset replay, doctor, and registry updates into one long function. Pulling those into phase helpers would make the destructive paths much easier to reason about and test.
As per coding guidelines: "Keep function complexity low; existing complexity hotspots are tracked separately."
🤖 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-rebuild-action.ts` around lines 35 - 506, rebuildSandbox() is too large and should be split into clear phase helpers; extract logical sections into smaller functions (e.g. confirmRebuildPrompt(), preflightCredentialCheck(session, sandboxName), ensureSandboxLiveAndBackup(sandboxName), deleteSandboxAndPrepareRecreate(sandboxName, backupManifest, sbMeta), runOnboardWithExitInterception(onboardArgs), restoreStateAndPresets(sandboxName, backupManifest), postRestoreAgentMigration(sandboxName, agent, backupManifest) and updateRegistryVersion(sandboxName, agentDef)) preserving existing behavior (use the same bail handler, logging via log/_rebuildLog, and session updates). Move the long try/catch/process.exit interception block into runOnboardWithExitInterception so rollback and onboardFailed handling lives with the recreate step, and have each helper return structured results (e.g. backupManifest, restore result, onboardExitCode) so the top-level rebuildSandbox coordinates phases and handles error paths unchanged.src/lib/sandbox-status-action.ts (1)
37-38: 🏗️ Heavy liftPlease split
showSandboxStatusinstead of exempting it from the complexity rule.The extracted action now owns registry rendering, gateway reconciliation, recovery guidance, and in-sandbox health checks in one function. Breaking those into small helpers will keep this module from becoming the next monolith and removes the need for a new inline complexity suppression. As per coding guidelines, "Keep function complexity low; existing complexity hotspots are tracked separately".
🤖 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-status-action.ts` around lines 37 - 38, The function showSandboxStatus is doing too many responsibilities (registry rendering, gateway reconciliation, recovery guidance, and in-sandbox health checks); instead of disabling complexity, extract each responsibility into small helper functions (e.g., renderRegistryStatus(sandboxName), reconcileGatewayForSandbox(sandboxName), provideRecoveryGuidance(sandboxName, context), runInSandboxHealthChecks(sandboxName)) and call them from showSandboxStatus, move any large inline blocks into those helpers, and then remove the "// eslint-disable-next-line complexity" comment so the function's cyclomatic complexity is reduced and each helper can be unit-tested independently.
🤖 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-doctor-cli-command.ts`:
- Around line 11-19: Declare an oclif argument instead of reading this.argv: add
a static args = { sandboxName: Args.string({ required: true }) } on the
SandboxDoctor command and change static usage to use oclif templates (e.g. "<%=
config.bin %> <%= command.id %> [--json]"). In run(), call
this.parse(SandboxDoctor) and pull sandboxName from the parsed args (do not
destructure this.argv), and forward that sandboxName (and any flags) to
runSandboxDoctor; ensure you reference the class name SandboxDoctor, the static
args, and the runSandboxDoctor invocation when making these edits.
In `@src/lib/sandbox-process-recovery-action.ts`:
- Around line 238-245: ensureSandboxPortForward currently ignores the result of
the "forward start" call so callers may report success even if OpenShell failed;
modify ensureSandboxPortForward to capture the return/result (or thrown error)
from runOpenshell(["forward","start",...]) and only consider the port-forward
restored when that call succeeds (e.g., return true/false or rethrow on
failure), and ensure callers use that boolean/exception to decide whether to
print "Dashboard port forward re-established". Update references to runOpenshell
in ensureSandboxPortForward and adjust its signature/return value or error
behavior so success is reliably indicated.
- Around line 50-52: The tmpFile name created with path.join(os.tmpdir(),
`nemoclaw-ssh-${process.pid}-${Date.now()}.conf`) is predictable and
fs.writeFileSync follows existing symlinks, causing a race/symlink attack;
change the implementation in sandbox-process-recovery-action.ts so you create an
exclusive temp artifact (either create a dedicated random temp directory via
fs.mkdtempSync and write the config file inside it, or open the file with an
exclusive flag like fs.openSync/tmp file creation with flag 'wx' before writing)
instead of plain writeFileSync, then write sshConfigResult.output with mode
0o600 and ensure you atomically close and remove the temp file/dir on cleanup or
error.
In `@src/lib/sandbox-rebuild-action.ts`:
- Around line 83-84: The post-rebuild logic still uses the pre-sync session
agent captured in agent = agentRuntime.getSessionAgent(sandboxName) and
agentName, which can cause wrong post-restore branches and agentVersion
persistence; update the code to derive post-restore decisions from the agent
returned by rebuildAgent (or by reloading the sandbox metadata after await
onboard(...)) instead of the earlier agent variable—replace uses of agent and
agentName in the Step 6/7 block (and the other occurrences around the commented
ranges) with the rebuilt agent/its display name (or freshly loaded sandbox
metadata) so the final status text and agentVersion are set from the recreated
sandbox.
In `@src/lib/sandbox-skill-install-action.ts`:
- Around line 176-180: The current write of tmpSshConfig using a predictable
path and fs.writeFileSync is vulnerable to symlink/race attacks; instead create
a private temporary directory (e.g., using fs.mkdtemp with os.tmpdir() and a
unique prefix based on process.pid/Date.now()), write the SSH config file inside
that directory using fs.writeFileSync or fs.promises.writeFile with the flag
"wx" and mode 0o600 to ensure exclusive creation, and then remove the file and
directory in a finally block; apply the same pattern to the other SSH-config
temp write in this file that uses sshConfigResult.output so both uses are
created exclusively and cleaned up.
In `@src/lib/skill-install.ts`:
- Around line 148-151: The JSDoc claims sshExec() uses the same SSH flags as
executeSandboxCommand(), but sshExec() currently sets ConnectTimeout=10 while
executeSandboxCommand() sets ConnectTimeout=5; update code so both helpers use
the same SSH options (e.g., unify ConnectTimeout to 5 or 10) or change the
comment to say they use similar SSH settings. Locate sshExec and
executeSandboxCommand and either align their SSH flag values (including
ConnectTimeout) or soften the JSDoc to avoid asserting exact parity.
---
Nitpick comments:
In `@src/lib/sandbox-doctor-action.ts`:
- Around line 429-430: The runSandboxDoctor function is doing too much; extract
each diagnostic section into small helper functions so the top-level function
becomes orchestration-only. Create helpers like diagnoseHost(),
diagnoseGateway(), diagnoseSandbox(), diagnoseInference(), and
diagnoseLocalService() and move the corresponding logic (host checks, gateway
checks, sandbox checks, inference checks, local service checks) from
runSandboxDoctor into those helpers; have runSandboxDoctor simply call these
helpers in sequence, forward args/state as needed, and preserve existing
return/error behavior and logging (refer to runSandboxDoctor and any local
variables it uses to pass state into the new helpers).
In `@src/lib/sandbox-rebuild-action.ts`:
- Around line 35-506: rebuildSandbox() is too large and should be split into
clear phase helpers; extract logical sections into smaller functions (e.g.
confirmRebuildPrompt(), preflightCredentialCheck(session, sandboxName),
ensureSandboxLiveAndBackup(sandboxName),
deleteSandboxAndPrepareRecreate(sandboxName, backupManifest, sbMeta),
runOnboardWithExitInterception(onboardArgs), restoreStateAndPresets(sandboxName,
backupManifest), postRestoreAgentMigration(sandboxName, agent, backupManifest)
and updateRegistryVersion(sandboxName, agentDef)) preserving existing behavior
(use the same bail handler, logging via log/_rebuildLog, and session updates).
Move the long try/catch/process.exit interception block into
runOnboardWithExitInterception so rollback and onboardFailed handling lives with
the recreate step, and have each helper return structured results (e.g.
backupManifest, restore result, onboardExitCode) so the top-level rebuildSandbox
coordinates phases and handles error paths unchanged.
In `@src/lib/sandbox-status-action.ts`:
- Around line 37-38: The function showSandboxStatus is doing too many
responsibilities (registry rendering, gateway reconciliation, recovery guidance,
and in-sandbox health checks); instead of disabling complexity, extract each
responsibility into small helper functions (e.g.,
renderRegistryStatus(sandboxName), reconcileGatewayForSandbox(sandboxName),
provideRecoveryGuidance(sandboxName, context),
runInSandboxHealthChecks(sandboxName)) and call them from showSandboxStatus,
move any large inline blocks into those helpers, and then remove the "//
eslint-disable-next-line complexity" comment so the function's cyclomatic
complexity is reduced and each helper can be unit-tested independently.
🪄 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: 564dcf19-e814-4e23-9a1b-32bc2df75d69
📒 Files selected for processing (21)
src/lib/global-cli-actions.tssrc/lib/legacy-oclif-dispatch.test.tssrc/lib/legacy-oclif-dispatch.tssrc/lib/nemoclaw-runtime-bridge.tssrc/lib/oclif-commands.tssrc/lib/sandbox-connect-action.tssrc/lib/sandbox-destroy-action.tssrc/lib/sandbox-doctor-action.tssrc/lib/sandbox-doctor-cli-command.tssrc/lib/sandbox-gateway-state-action.tssrc/lib/sandbox-process-recovery-action.tssrc/lib/sandbox-rebuild-action.tssrc/lib/sandbox-runtime-actions.tssrc/lib/sandbox-skill-install-action.tssrc/lib/sandbox-status-action.tssrc/lib/share-command-deps.tssrc/lib/skill-install.tssrc/lib/terminal-style.tssrc/lib/upgrade-sandboxes-action.tssrc/nemoclaw.tstest/image-cleanup.test.ts
| static id = "sandbox:doctor"; | ||
| static strict = false; | ||
| static summary = "Diagnose sandbox and gateway health"; | ||
| static description = "Run host, gateway, sandbox, inference, messaging, and local service diagnostics."; | ||
| static usage = ["<name> doctor [--json]"]; | ||
|
|
||
| public async run(): Promise<void> { | ||
| const [sandboxName, ...actionArgs] = this.argv; | ||
| await runSandboxDoctor(sandboxName, actionArgs); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In @oclif/core 4.10.5, what is the recommended way to declare a required positional argument for a Command, and how should the usage/help text be defined for a command with id "sandbox:doctor"?
💡 Result:
Recommended way to declare a required positional argument (in @oclif/core 4.10.5) Use the static args property on your Command class and mark the positional argument as required with required: true on the Args.* helper, e.g.: class SandboxDoctor extends Command { static args = { // required positional argument commandId: Args.string({ required: true, name: 'command-id', description: 'ID of the command to diagnose', }), } async run { const {args} = await this.parse(SandboxDoctor) // args.commandId } } The key points are: positional arguments are declared under static args, and “required positional argument” is expressed via Args.({... required: true ...}). How to define usage/help text for a command with id "sandbox:doctor" Define either (a) static description / static summary (and/or static examples), and/or (b) override the help “USAGE” line with static usage. If you want the usage line tailored to the command id, use the oclif template <%= command.id %>. For example: export class DoctorSandbox extends Command { static summary = 'Health checks and quick fixes for the sandbox' static description = 'Run health checks and apply safe repair steps for sandbox mode.' // Overrides the default usage line in --help static usage = '<%= config.bin %> <%= command.id %> [--repair]' static examples = [ '<%= config.bin %> <%= command.id %> --help', '<%= config.bin %> <%= command.id %> --repair', ] static args = { // example required positional argument target: Args.string({ required: true, description: 'Resource to check' }), } async run {} } With id "sandbox:doctor", <%= command.id %> resolves to sandbox:doctor in the rendered help/usage text.
Citations:
- 1: https://oclif.github.io/docs/args/
- 2: https://oclif.github.io/docs/commands/
- 3: https://npmx.dev/package/@oclif/core/v/4.10.5
- 4: https://oclif.io/docs/commands
Declare sandboxName as a required oclif argument instead of manually parsing this.argv.
The current code destructures this.argv directly (line 18), which bypasses oclif's argument validation. This causes:
sandbox:doctor --jsonto treat--jsonas the sandbox name instead of a flagsandbox:doctor(no args) to passundefinedas the sandbox name- The usage string (line 15) to advertise the legacy
<name> doctorsyntax instead of the registered command id
Use static args with Args.string({ required: true }) and parse via this.parse(SandboxDoctor). Update the usage string to use oclif templates (e.g., <%= config.bin %> <%= command.id %> [--json]) to match the actual command surface.
🤖 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-doctor-cli-command.ts` around lines 11 - 19, Declare an oclif
argument instead of reading this.argv: add a static args = { sandboxName:
Args.string({ required: true }) } on the SandboxDoctor command and change static
usage to use oclif templates (e.g. "<%= config.bin %> <%= command.id %>
[--json]"). In run(), call this.parse(SandboxDoctor) and pull sandboxName from
the parsed args (do not destructure this.argv), and forward that sandboxName
(and any flags) to runSandboxDoctor; ensure you reference the class name
SandboxDoctor, the static args, and the runSandboxDoctor invocation when making
these edits.
| const tmpFile = path.join(os.tmpdir(), `nemoclaw-ssh-${process.pid}-${Date.now()}.conf`); | ||
| fs.writeFileSync(tmpFile, sshConfigResult.output, { mode: 0o600 }); | ||
| try { |
There was a problem hiding this comment.
Use an exclusive temp file for the SSH config.
This filename is predictable, and writeFileSync will follow a pre-created symlink in /tmp, so a local user can race this into overwriting another file before the SSH call runs. Create a dedicated random temp directory or open the file with an exclusive flag (wx) before writing.
Proposed hardening
- 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, flag: "wx" });
try {
const result = spawnSync(
"ssh",
@@
} finally {
try {
- fs.unlinkSync(tmpFile);
+ fs.rmSync(tmpDir, { recursive: true, force: true });
} catch {
/* ignore */
}
}📝 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.
| const tmpFile = path.join(os.tmpdir(), `nemoclaw-ssh-${process.pid}-${Date.now()}.conf`); | |
| fs.writeFileSync(tmpFile, sshConfigResult.output, { mode: 0o600 }); | |
| try { | |
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-ssh-")); | |
| const tmpFile = path.join(tmpDir, "config"); | |
| fs.writeFileSync(tmpFile, sshConfigResult.output, { mode: 0o600, flag: "wx" }); | |
| try { |
🤖 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 50 - 52, The tmpFile
name created with path.join(os.tmpdir(),
`nemoclaw-ssh-${process.pid}-${Date.now()}.conf`) is predictable and
fs.writeFileSync follows existing symlinks, causing a race/symlink attack;
change the implementation in sandbox-process-recovery-action.ts so you create an
exclusive temp artifact (either create a dedicated random temp directory via
fs.mkdtempSync and write the config file inside it, or open the file with an
exclusive flag like fs.openSync/tmp file creation with flag 'wx' before writing)
instead of plain writeFileSync, then write sshConfigResult.output with mode
0o600 and ensure you atomically close and remove the temp file/dir on cleanup or
error.
| const tmpSshConfig = path.join( | ||
| os.tmpdir(), | ||
| `nemoclaw-ssh-skill-${process.pid}-${Date.now()}.conf`, | ||
| ); | ||
| fs.writeFileSync(tmpSshConfig, sshConfigResult.output, { mode: 0o600 }); |
There was a problem hiding this comment.
Use an exclusive temp file for the SSH config.
This path is predictable in /tmp, and writeFileSync() will follow a pre-existing file or symlink. That makes this flow vulnerable to local file clobbering and attacker-controlled SSH config reuse. Create a private temp dir and write the config with flag: "wx" before removing that directory in finally.
🔒 Suggested hardening
- const tmpSshConfig = path.join(
- os.tmpdir(),
- `nemoclaw-ssh-skill-${process.pid}-${Date.now()}.conf`,
- );
- fs.writeFileSync(tmpSshConfig, sshConfigResult.output, { mode: 0o600 });
+ const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-ssh-skill-"));
+ const tmpSshConfig = path.join(tmpDir, "ssh.conf");
+ fs.writeFileSync(tmpSshConfig, sshConfigResult.output, {
+ mode: 0o600,
+ flag: "wx",
+ });
…
} finally {
try {
- fs.unlinkSync(tmpSshConfig);
+ fs.rmSync(tmpDir, { recursive: true, force: true });
} catch {
/* ignore */
}
}Also applies to: 217-223
🤖 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 176 - 180, The current
write of tmpSshConfig using a predictable path and fs.writeFileSync is
vulnerable to symlink/race attacks; instead create a private temporary directory
(e.g., using fs.mkdtemp with os.tmpdir() and a unique prefix based on
process.pid/Date.now()), write the SSH config file inside that directory using
fs.writeFileSync or fs.promises.writeFile with the flag "wx" and mode 0o600 to
ensure exclusive creation, and then remove the file and directory in a finally
block; apply the same pattern to the other SSH-config temp write in this file
that uses sshConfigResult.output so both uses are created exclusively and
cleaned up.
| /** | ||
| * Run a command on the sandbox via SSH with optional stdin content. | ||
| * Uses the same SSH flags as executeSandboxCommand in nemoclaw.ts. | ||
| * Uses the same SSH flags as executeSandboxCommand in sandbox-process-recovery-action.ts. | ||
| */ |
There was a problem hiding this comment.
The new JSDoc is already out of sync with the implementation.
sshExec() still uses ConnectTimeout=10, while executeSandboxCommand() uses ConnectTimeout=5, so “same SSH flags” is false today. Either align the options or soften the comment to say the helpers use similar SSH invocation settings.
🤖 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/skill-install.ts` around lines 148 - 151, The JSDoc claims sshExec()
uses the same SSH flags as executeSandboxCommand(), but sshExec() currently sets
ConnectTimeout=10 while executeSandboxCommand() sets ConnectTimeout=5; update
code so both helpers use the same SSH options (e.g., unify ConnectTimeout to 5
or 10) or change the comment to say they use similar SSH settings. Locate
sshExec and executeSandboxCommand and either align their SSH flag values
(including ConnectTimeout) or soften the JSDoc to avoid asserting exact parity.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
LGTM. Clean extraction of upgradeSandboxes into src/lib/upgrade-sandboxes-action.ts. Public surface preserved (--check/--auto/--yes, all output strings byte-identical, exit codes unchanged). The literal ⚠/✓ chars vs prior ⚠/✓ escapes are identical bytes — no behavior change.
Strong cleanup this round:
NemoClawRuntimeBridgeis now empty (final action removed); next PR (#2898) drops the bridge entirely.- The
sandboxRebuildalias #2896 left innemoclaw.tsforupgradeSandboxesis gone now that its consumer moved. sandboxVersion,captureOpenshell,parseLiveSandboxNames,askPromptimports also removed.sandbox-runtime-actions.tsdrops its lastgetNemoClawRuntimeBridgeimport.
Zero new orphans introduced — the cumulative ~16 orphans + 2 stale comments in src/nemoclaw.ts remain pending the separate cleanup PR.
CI: first fully-green hash in the stack. pr.yaml + pr-self-hosted (build-sandbox-images, build-sandbox-images-arm64, test-e2e-sandbox, test-e2e-gateway-isolation, test-e2e-port-overrides, test-e2e-ollama-proxy) all SUCCESS.
## Summary Remove the now-empty transitional NemoClaw runtime bridge from the oclif CLI layer. Credentials tests now inject gateway/OpenShell hooks directly through `global-cli-actions` instead of stubbing `../nemoclaw`. ## Stack Navigation - Position: 9 of 60 - Previous PR: [#2897 — refactor(cli): extract upgrade sandboxes action](#2897) - Next PR: [#2899 — refactor(cli): remove legacy dispatch fallbacks](#2899) ## Changes - Deleted `src/lib/nemoclaw-runtime-bridge.ts`. - Added explicit test/runtime hook injection to `src/lib/global-cli-actions.ts` for credential command gateway recovery and provider operations. - Updated credentials CLI tests to use the explicit global action hooks instead of stubbing `dist/nemoclaw.js`. - Verified no `src/lib` code imports or requires `../nemoclaw`. ## 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 * **Refactor** * Refactored internal runtime architecture to streamline dependencies and enhance code modularity. * **Tests** * Enhanced test infrastructure with improved runtime mocking mechanism for better test isolation and control. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
3 participants
Summary
Extract upgrade-sandboxes orchestration from
src/nemoclaw.tsinto a dedicated action module. This removes the final command action from the transitional runtime bridge while preserving stale-version detection and rebuild-loop behavior.Stack Navigation
Changes
src/lib/upgrade-sandboxes-action.tsfor stale/unknown version classification and optional rebuild orchestration.src/lib/global-cli-actions.tsto call the extracted upgrade action.upgradeSandboxesfromsrc/nemoclaw.tsand emptied theNemoClawRuntimeBridgeaction surface.src/lib/sandbox-runtime-actions.ts.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
--check,--auto, and--yesflag support, remains available as before.