refactor(cli): extract sandbox rebuild action - #2896
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 (1)
📝 WalkthroughWalkthroughThe PR removes ChangesSandbox Rebuild Extraction
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as NemoClaw CLI
participant Onboard
participant Openshell
participant Registry
participant SandboxState
participant Policies
User->>CLI: run rebuildSandbox(sandboxName, args)
CLI->>Registry: load sandbox entry
CLI->>Onboard: detect active onboard sessions
CLI->>Openshell: check sandbox liveness (sandbox list)
CLI->>SandboxState: backupSandboxState()
CLI->>Openshell: stop NIM container / delete sandbox
CLI->>Registry: remove registry entry
CLI->>Onboard: onboard(resume, recreateSandbox, nonInteractive)
Onboard-->>CLI: success / failure
CLI->>SandboxState: restoreSandboxState()
CLI->>Policies: applyPreset(...) for each saved preset
CLI->>Registry: update agentVersion
CLI->>User: print completion status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 destroy and image cleanup helpers from `src/nemoclaw.ts` into a dedicated action module. This removes `sandboxDestroy` from the transitional runtime bridge while preserving sandbox deletion, messaging provider cleanup, gateway teardown, and Docker image cleanup behavior. ## Stack Navigation - Position: 6 of 60 - Previous PR: [#2893 — refactor(cli): extract sandbox doctor action](#2893) - Next PR: [#2896 — refactor(cli): extract sandbox rebuild action](#2896) ## Changes - Added `src/lib/sandbox-destroy-action.ts` for sandbox destroy orchestration, delete-result classification, gateway cleanup, service cleanup, and image cleanup helpers. - Updated `src/lib/sandbox-runtime-actions.ts` to call the extracted destroy action. - Removed `sandboxDestroy` and destroy-specific helpers from `src/nemoclaw.ts` and `NemoClawRuntimeBridge`. - Updated rebuild to use the extracted image/registry removal helper. - Reworked image cleanup tests to cover helper behavior instead of source-shape assertions against `src/nemoclaw.ts`. ## 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 * **Bug Fixes** * Improved handling for sandbox deletion when the sandbox has already been removed. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-rebuild-action.ts`:
- Around line 83-84: The code captures agent with
agentRuntime.getSessionAgent(sandboxName) into agent/agentName early, but later
logic that loads agentDef (used for post-restore migrations/version updates)
still uses that stale agent; after the session rewrite (the rebuild of the
sandbox session), re-fetch the current agent and display name (call
agentRuntime.getSessionAgent(sandboxName) and
agentRuntime.getAgentDisplayName(...) again) or replace uses of the original
agent/agentName with the rebuilt-session agent variable so agentDef is derived
from the up-to-date agent; update the code paths that load agentDef (the block
currently around where agentDef is used for migrations/version updates) to use
the fresh agent reference.
- Around line 127-143: The current branch in sandbox-rebuild-action.ts skips
credential preflight when onboardSession.loadSession() points to a different
sandbox, which allows destructive operations without checking credentials;
instead, when session.sandboxName !== sandboxName, fall back to resolving the
credential environment for the target sandbox and set rebuildCredentialEnv
accordingly rather than skipping preflight: keep using
onboardSession.loadSession() to read the existing session, but replace the skip
block that logs the warning with logic that calls the registry-backed provider
resolution used elsewhere (i.e., the same provider selection/onboard resume
path) to obtain the credential env for sandboxName and assign it to
rebuildCredentialEnv (preserving the existing null fallback behavior if none
found), ensuring the preflight uses rebuildCredentialEnv for subsequent checks.
- Around line 48-52: The bailout handler declaration for bail currently defines
a second parameter named code that is unused in the throwing branch, violating
the unused-variable rule; update the throwing branch's parameter name to _code
(i.e., change (msg: string, code = 1) => { throw new Error(msg); } to use (msg:
string, _code = 1) => { throw new Error(msg); }) while leaving the non-throwing
branch unchanged so opts.throwOnError and bail semantics remain the same.
🪄 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: 9ce940f5-911a-4452-9237-41fbfcc8e960
📒 Files selected for processing (4)
src/lib/nemoclaw-runtime-bridge.tssrc/lib/sandbox-rebuild-action.tssrc/lib/sandbox-runtime-actions.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 extraction of sandboxRebuild + _rebuildLog into src/lib/sandbox-rebuild-action.ts. Public surface preserved (--yes/--force/--verbose/-v, NEMOCLAW_REBUILD_VERBOSE=1, all log/confirm strings byte-identical, exit codes unchanged). The throwOnError opt for upgradeSandboxes's batch loop is preserved. The RD as _RD re-aliasing is inconsistent with #2891/#2892/#2893 (which dropped the underscore) but resolves to the same color — no behavior change.
Nit / follow-up:
Six new orphan imports introduced in src/nemoclaw.ts (last consumer was sandboxRebuild):
hydrateCredentialEnv(line 38, in destructure with the still-usedisNonInteractive)getActiveSandboxSessions(line 81)createSessionDeps/createSystemDeps(line 82)sandboxStateandparseRestoreArgs(lines 78–79)policies(line 45)
These plus the carryover from #2891/#2893/#2895 (~10 imports + 2 stale sandboxDestroy() comments) bring the total to ~16. Tracked for the separate cleanup PR.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
## Summary Extract upgrade-sandboxes orchestration from `src/nemoclaw.ts` into 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 - Position: 8 of 60 - Previous PR: [#2896 — refactor(cli): extract sandbox rebuild action](#2896) - Next PR: [#2898 — refactor(cli): remove runtime bridge](#2898) ## Changes - Added `src/lib/upgrade-sandboxes-action.ts` for stale/unknown version classification and optional rebuild orchestration. - Updated `src/lib/global-cli-actions.ts` to call the extracted upgrade action. - Removed `upgradeSandboxes` from `src/nemoclaw.ts` and emptied the `NemoClawRuntimeBridge` action surface. - Removed the remaining runtime-bridge dependency from `src/lib/sandbox-runtime-actions.ts`. ## 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 * **Refactor** * Internal reorganization of the sandbox upgrade system to improve code modularity and maintainability. The upgrade functionality, including sandbox stale detection and batch rebuild operations with `--check`, `--auto`, and `--yes` flag support, remains available as before. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Summary
Extract sandbox rebuild orchestration from
src/nemoclaw.tsinto a dedicated action module. This removessandboxRebuildfrom the transitional runtime bridge while preserving rebuild preflight, backup, recreate, restore, policy preset restore, and post-upgrade checks.Stack Navigation
Changes
src/lib/sandbox-rebuild-action.tsfor rebuild confirmation, credential preflight, backup, delete/recreate, restore, policy preset replay, and post-restore checks.src/lib/sandbox-runtime-actions.tsto call the extracted rebuild action.upgrade-sandboxesto call the extracted rebuild action directly.sandboxRebuildfromsrc/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
Refactor